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

Fix simple_form regression #10

Merged
merged 1 commit into from
Sep 26, 2014

Conversation

thiagoa
Copy link
Contributor

@thiagoa thiagoa commented Sep 25, 2014

Hi,

I just fixed a regression on the simple_form integration module, I’m sorry! I guess we need some tests, but that’s OK for now :)

It blew up with a collection like this, which previously worked:

f.input :status, collection: [:editing, :approved] # Ops...

Note that it also accepts an input format like this (shown in the README file):

f.input :status, collection: Foo.statuses.except(:draft)

I also needed to changed the format of the hash returned by “Foo.collection_i18n” to something more useful. It was in the following format: { ‘Translated key’ => integer code }. I realized the integer code had little to no use, because integer codes are meant to be used internally by the model. They shouldn’t be needed on the views. I guess the “Foo.statuses” (or similar) method that comes with Rails is meant for querying the database, and I see little use outside that context. For example:

Foo.where status: Foo.statuses[‘editing'] # Fetches all foos with “editing” status

The new i18n collection hash can be used like this, which is much more useful:

translated_statuses = Foo.statuses_i18n
translated_statuses['editing’] # Returns the translation of the “editing” status

That method is also useful because:

  • There is a centralised place to fetch all the translations for an enum attribute, if the user needs to (the simple form extension also fetches the translations from that method)
  • The user can easily query a translation for any enum label, like shown above
  • The user doesn’t need to mess with a raw i18n query (enums.model.field.label)

I also retired the “except” option previously accepted by the i18n collection method. Now that the keys are the enum identifiers, we don’t need that anymore. Instead we can do:

Foo.statuses_i18n.except(:editing) # Or...
Foo.statuses_i18n.slice(:editing)

I’ll be glad to hear an opinion from you, thanks.

Fixed an error that happened when the "collection" option was
explicitly used in array format.

The format of the hash returned by Foo.collection_i18n also changed to
something more useful. More info about that on the pull request.
@zmbacker
Copy link
Owner

Hi,
Thank you for your test and fixed the error when pass the collection parameter with array like this: [:editing, :approved] .
That case slipped my attention. I think you are right, we need some tests to guarantee against the errors like this. I will add some test codes later.

I agree with your opinion about the i18n collection method. it useful for users who want to do some logic in views. Rails origin collection method is use for the logic in models, and it is suggested by rails's core team member .

Good job for this. Thank you.

zmbacker added a commit that referenced this pull request Sep 26, 2014
@zmbacker zmbacker merged commit ef70414 into zmbacker:master Sep 26, 2014
@thiagoa thiagoa deleted the simple_form_regression_fix branch September 26, 2014 01:41
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.

2 participants