Check that the card is NSG before serving xlarge images #411
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #411 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 76 76
Lines 1348 1352 +4
Branches 181 183 +2
=======================================
+ Hits 1327 1331 +4
Misses 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Oops, closed this by accident. |
The tests for the resources overall could use some love, but you can add test cases, just a bit indirectly. If you add new cards in test/fixtures/, you can seed things that you can then request. Check out spec/resources/card_resource_reads_spec.rb and the 'fields match' test for an example. You can do more like that but more targeted. |
app/resources/card_resource.rb
Outdated
| @@ -163,16 +163,16 @@ class CardResource < ApplicationResource # rubocop:disable Metrics/ClassLength | |||
|
|
|||
| private | |||
|
|
|||
| def images(id, has_narrative_image: false, face_index: nil) | |||
| def images(id, designed_by, has_narrative_image: false, face_index: nil) | |||
There was a problem hiding this comment.
if we are going by set, we want released_by, not designed_by.
For example. the latest printing of sure gamble does not have xlarge, but the class act does right now:
There was a problem hiding this comment.
Sorry i was not clear about the distinctions between released by and designed by in chat.
app/resources/card_resource.rb
Outdated
| @@ -164,15 +164,16 @@ class CardResource < ApplicationResource # rubocop:disable Metrics/ClassLength | |||
| private | |||
|
|
|||
| def images(id, has_narrative_image: false, face_index: nil) | |||
| printing = Printing.find(id) | |||
There was a problem hiding this comment.
In CardResource, you can use card_set_ids and pluck the first item out. That is the latest printing (they are sorted by date_release DESC) to avoid this extra read. You will need to do that in attribute :latest_printing_images
There was a problem hiding this comment.
or add the equivalent xlarge_image? method in the Card model and use that. EIther way, don't add an extra find in here.
There was a problem hiding this comment.
Oddly it seems like they're sorted the other way around? Had to use printings.last. By the way, why do we have latest_printing_id and not latest_printing? The latter seems more useful to me.
app/resources/card_resource.rb
Outdated
| @@ -74,7 +74,8 @@ class CardResource < ApplicationResource # rubocop:disable Metrics/ClassLength | |||
|
|
|||
| unless @object.num_extra_faces.zero? | |||
| @object.face_indices.each do |index| | |||
| f = { index:, images: images(@object.latest_printing_id, face_index: index) } | |||
| f = { index:, | |||
| images: images(@object.latest_printing_id, @object.printings.last.xlarge_image?, face_index: index) } | |||
There was a problem hiding this comment.
ah, ok. so there is a lightly tricky thing here.
You removed a read in favor of a different read.
@object.printings does a fetch for all related printings and it is only an accident tht you found this in the last one. technically they are unordered but sometimes do the right thing.
you want @object.printing_ids_in_database.first which will use the array field from the already loaded object. You can't just use printing_ids because printings is a relationship and printing_ids is a helper for relationships that will give you the ids after a query and loading those associated records.
There was a problem hiding this comment.
ok, i was too fixated on the "avoid a read" bit and missed the "this doesn't solve your problem" bit.
We will need to pull an extra attribute from the printing into the card view, which i can do quickly as a follow-up.
The actual, helpful, fix you need here is an explicit ordering of the printings or it will sometimes misbehave.
@object.printings.order(:date_release).reverse_order.first.xlarge_image? in this case.
app/resources/card_resource.rb
Outdated
| @@ -74,7 +74,8 @@ class CardResource < ApplicationResource # rubocop:disable Metrics/ClassLength | |||
|
|
|||
| unless @object.num_extra_faces.zero? | |||
| @object.face_indices.each do |index| | |||
| f = { index:, images: images(@object.latest_printing_id, face_index: index) } | |||
| f = { index:, | |||
| images: images(@object.latest_printing_id, @object.printings.last.xlarge_image?, face_index: index) } | |||
There was a problem hiding this comment.
ok, i was too fixated on the "avoid a read" bit and missed the "this doesn't solve your problem" bit.
We will need to pull an extra attribute from the printing into the card view, which i can do quickly as a follow-up.
The actual, helpful, fix you need here is an explicit ordering of the printings or it will sometimes misbehave.
@object.printings.order(:date_release).reverse_order.first.xlarge_image? in this case.
I really wanted to add some tests for this, but I'm not really sure how to go about that. It didn't look like it belonged in acceptance/cards_spec.rb or resources/card_resource_reads_spec.rb. Also, not really sure how to test a resource specifically.