Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enum translations #442

Merged
merged 9 commits into from
Feb 17, 2023
Merged

Enum translations #442

merged 9 commits into from
Feb 17, 2023

Conversation

difernandez
Copy link
Contributor

@difernandez difernandez commented Feb 13, 2023

This PR adds enum translation support in tag column and rows, as well as in select input filters. Translation is already supported in formtastic, but this only works for inputs in a form for the model object, when used in filters the object is a Ransack object, and the translation didn't work the same. Changes include:

  • New EnumUtils with tanslation related methods. The options_for_select method allows for the enum option name or the value in database to be used. This is because the AA filter must use the database value, while the interactive tag_column must use the name
  • Use of Enum utils in tag builder, including interactive option support
  • Extension of ActiveAdmin::Inputs::Filters::SelectInput to allow translation when using with an enum and not including collection option

Plus, last three commits include circleci and test suite related fixes.

Question for the reviewer: I'm not sure I put the code of the filter extension where it should be. As it was an extension, I put it in the /support folder, following the example of the RansackFormBuilderExtension. And given that it is changing behavior of an input filter, I put a test for this in spec/features/inputs. Let me know if you would change any of this

@difernandez difernandez force-pushed the enum-translations branch 3 times, most recently from fae4493 to 09ff1ac Compare February 14, 2023 17:43
Copy link
Member

@ldlsegovia ldlsegovia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏🏻

eq('Archivado, ya no se considera')
)
expect(page.find("#{option_selector}[value='42']").text).to eq('Estado que no existe')
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be good to test that the collection is actually filtered, what do you think? maybe it's not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, especially considering that the filter only works with the db value, that way we make sure it's working. I'll add the test soon before merging

@difernandez difernandez force-pushed the enum-translations branch 2 times, most recently from e5ee82e to e3f660b Compare February 17, 2023 13:27
@difernandez
Copy link
Contributor Author

@ldlsegovia requested re-review, as I had to include a fix elsewhere for the new tests to pass: default_select setting in select2_spec was leaking to tests that ran after it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants