close
Skip to content

Pass associated_class to collection from show#2238

Merged
pablobm merged 1 commit into
thoughtbot:mainfrom
stormmaster42:fix_i18n_collection_for_association_using_wrong_resource
Aug 15, 2022
Merged

Pass associated_class to collection from show#2238
pablobm merged 1 commit into
thoughtbot:mainfrom
stormmaster42:fix_i18n_collection_for_association_using_wrong_resource

Conversation

@stormmaster42
Copy link
Copy Markdown
Contributor

This fixes an issue where the embedded collection is using the parent's class to define i18n headers.

Example: Ressource order has_many order_items.
Order dashboard declares order_items: Field::HasMany

When the show which includes the order_items association is rendered within the collection partial it uses the parent's resource (Order) instead of OrderItem to determine the header label default.

Not sure how to add a test for this behavior.

this fixes an issue where the embedded collection is using the parent's class to define i18n headers
@pablobm
Copy link
Copy Markdown
Collaborator

pablobm commented Aug 11, 2022

Thank you for the MR 🙂 I'm trying to reproduce the issue locally, but I'm not having any luck. Would you be able to explain it in terms of the example app?

For instance, in the example app we have Customer.has_many :orders, and this is reflected in CustomerDashboard. I have tried to replicate the issue by adding this translation:

---
en:
  # ...
  helpers:
    label:
      order:
        address_state: "State code"

When I visit a customer show page, I do see the translation:

Customer dashboard showing the translation

Perhaps I'm looking at the wrong thing?

@jubilee2
Copy link
Copy Markdown
Contributor

@stormmaster42
Copy link
Copy Markdown
Contributor Author

stormmaster42 commented Aug 12, 2022

Oh yes. Sorry forgot to add that detail. Yes it's related to the active record fallback.
So if you declare the translation on attribute level like so:

en:
  activerecord:
    attributes:
      order:
        address_state: "State code"

it would be picked up by the orders dashboard, but not the embedded orders list in the customer show.

@pablobm
Copy link
Copy Markdown
Collaborator

pablobm commented Aug 15, 2022

Oh, I see! Thank you for explaining. The fact that resource_class can be either a method or a variable was confusing in particular. I get it now 👍

Don't worry about testing. This is too tricky (I already spent a lot of time trying to test something similar elsewhere and I'm not sure that there's much return of investment to be had). Merging.

@pablobm pablobm merged commit d5acc10 into thoughtbot:main Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants