Skip to content

Fix translatable presence validator for hyphenated locales#8795

Merged
ahukkanen merged 2 commits intodecidim:developfrom
EduardoGHdez:fix-hyphenated-locales
Feb 28, 2022
Merged

Fix translatable presence validator for hyphenated locales#8795
ahukkanen merged 2 commits intodecidim:developfrom
EduardoGHdez:fix-hyphenated-locales

Conversation

@EduardoGHdez
Copy link
Copy Markdown
Contributor

@EduardoGHdez EduardoGHdez commented Feb 9, 2022 •
edited
Loading

🎩 What? Why?

Fix TranslatablePresenceValidator - Hyphenated locale

When translatable-attributes are defined, it does replaces - with
__. The validator was missing this conversion and wasn't able to find
the proper method

This fixes the translatable-presence-validator for hypenated locales

📌 Related Issues

Testing

Have added tests, but I think other way of validating this fix is by following the described issue at #8787

📋 Checklist

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@EduardoGHdez EduardoGHdez force-pushed the fix-hyphenated-locales branch 2 times, most recently from 534aae1 to b6e80a9 Compare February 9, 2022 05:08
@EduardoGHdez EduardoGHdez changed the title WIP - Fix hyphenated locales Fix hyphenated locales Feb 9, 2022
@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Feb 9, 2022

We are no longer using Rectify for the forms as of #8669.

It has been replaced with Decidim::AttributeObject::Form.

The new form class is not converting the hash keys with the .underscore method, so those changes are unnecessary.

However, the fix you have done at the TranslatablePresenceValidator is valid if you remove the .underscore at the end of the method:

decidim/decidim-core/app/forms/translatable_presence_validator.rb

Line 12 in b6e80a9

translated_attr = "#{attribute}_#{default_locale_for(record)}".gsub("-","__").underscore

There seems to be another bug also in that class at the end of the default_locale_for method:

decidim/decidim-core/app/forms/translatable_presence_validator.rb

Line 25 in b6e80a9

[]

Instead of returning an array at the end of the method, I believe it should return I18n.default_locale.

@EduardoGHdez
Copy link
Copy Markdown
Contributor Author

EduardoGHdez commented Feb 9, 2022

We are no longer using Rectify for the forms as of #8669.

It has been replaced with Decidim::AttributeObject::Form.

The new form class is not converting the hash keys with the .underscore method, so those changes are unnecessary.

Wohoo! These are great news!

I'll update the PR to address your comments
Thanks for the quick response. Much appreciated 😄

@EduardoGHdez EduardoGHdez force-pushed the fix-hyphenated-locales branch from b6e80a9 to 9f41e6f Compare February 9, 2022 16:18
@EduardoGHdez
Copy link
Copy Markdown
Contributor Author

EduardoGHdez commented Feb 9, 2022

I've updated the PR 😄

Regarding this:

Instead of returning an array at the end of the method, I believe it should return I18n.default_locale

Have default to Decidim.default_locale instead, but still not sure if this is the best approach. I think the intention was to not mark as valid if there is no organization associated 🤔

I'm open to any suggestions 😄

@EduardoGHdez EduardoGHdez force-pushed the fix-hyphenated-locales branch from 9f41e6f to bec2c48 Compare February 9, 2022 16:26
@EduardoGHdez EduardoGHdez changed the title Fix hyphenated locales Fix translatable presence validator for hyphenated locales Feb 9, 2022
@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Feb 10, 2022

Have default to Decidim.default_locale instead, but still not sure if this is the best approach. I think the intention was to not mark as valid if there is no organization associated 🤔

I'm open to any suggestions 😄

I did not mean to remove the error adding. I think adding the error is fine but returning the array will cause some issues if it ever goes there (which is pretty rare). What I meant was just to replace the line that says [] as you have done, so if you just add the error back above it, it should be fine.

Please also run rubocop, there is a linter issue with your code. See:
https://docs.decidim.org/en/develop/guide_commands/#_rubocop

@andreslucena andreslucena added module: core type: fix PRs that implement a fix for a bug labels Feb 14, 2022
@EduardoGHdez
Fix TranslatablePresenceValidator - Hyphenated lcl
05ca4db 
When translatable-attributes are defined, it does replaces `-`  with
`__`. The validator was missing this conversion and wasn't able to find
the proper method

This fixes the translatable-presence-validator for hypenated locales
@EduardoGHdez
Fix Translatable Presence Validator - no-org
8715b66 
Returns the proper error when there is no current_organization during
validation
@EduardoGHdez EduardoGHdez force-pushed the fix-hyphenated-locales branch from bec2c48 to 8715b66 Compare February 17, 2022 04:01
@EduardoGHdez
Copy link
Copy Markdown
Contributor Author

EduardoGHdez commented Feb 17, 2022

Have default to Decidim.default_locale instead, but still not sure if this is the best approach. I think the intention was to not mark as valid if there is no organization associated 🤔
I'm open to any suggestions 😄

I did not mean to remove the error adding. I think adding the error is fine but returning the array will cause some issues if it ever goes there (which is pretty rare). What I meant was just to replace the line that says [] as you have done, so if you just add the error back above it, it should be fine.

Please also run rubocop, there is a linter issue with your code. See: https://docs.decidim.org/en/develop/guide_commands/#_rubocop

Got it!

PR is ready again, just put back the error-adding and have run rubocop as well

Thanks for all the input @ahukkanen 😄
It's much appreciated 🙌🏽

ahukkanen
ahukkanen approved these changes Feb 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Really good fix with added tests. Tested this locally by setting the default locale to es-MX and it works correctly.

Great work!

@ahukkanen ahukkanen merged commit 97bce84 into decidim:develop Feb 28, 2022
entantoencuanto added a commit that referenced this pull request Mar 2, 2022
@entantoencuanto
Merge branch 'develop' into feature/process_types
f78e953 
* develop: (57 commits)
  Add a subtitle to assemblies and processes pages (#8918)
  Add a subtitle to votings page (#8919)
  Fix profile notifications (#8943)
  Fix email for verification conflict with managed users (#8926)
  Move VAPID keys generators to core (#8923)
  Fix officialized user event missing translations (#8927)
  Fix verification report with multitenants: notify it only to admins of that organization (#8929)
  Fix processes creation form with stats, metrics and announcements (#8925)
  Fix flaky spec in meetings multi-date selectors (#8924)
  Local HTML validator for the CI (#8937)
  Fix translatable presence validator for hyphenated locales (#8795)
  Fix link to docs in initiatives admin (#8921)
  Fix initiatives signatures issues (#8448)
  Fix the meetings export to also include unpublished meetings (#8874)
  Fix heading order in the consultation question page (#8920)
  Fix diff mode selector roles and tabindexes (#8912)
  Fix budget hard dependency and caching flag issues in comments (#8899)
  Fix emoji picker hiding Foundation Abide form errors (#8886)
  Fix logical heading order for the endorsers list (#8892)
  Fix Foundation Abide errors for Rails remote (AJAX) forms (#8889)
  ...
@EduardoGHdez EduardoGHdez deleted the fix-hyphenated-locales branch March 10, 2022 04:00
@verarojman verarojman mentioned this pull request Aug 11, 2022
Open
@alecslupu alecslupu added this to the 0.27.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahukkanen ahukkanen ahukkanen approved these changes

Assignees

No one assigned

Labels

module: core type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

0.27.0

Development

Successfully merging this pull request may close these issues.

Form saving not working for hyphenated locales

4 participants

@EduardoGHdez @ahukkanen @alecslupu @andreslucena