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

Validations create empty translations in DB #569

Open
mrbrdo opened this issue Apr 15, 2022 · 4 comments · May be fixed by #572
Open

Validations create empty translations in DB #569

mrbrdo opened this issue Apr 15, 2022 · 4 comments · May be fixed by #572

Comments

@mrbrdo
Copy link
Contributor

mrbrdo commented Apr 15, 2022

I am using fallbacks. One of my locales is "en-GB", and I think the I18n fallbacks defaults to "en" as a fallback.
However this bug exists also without fallbacks, it's just easier to explain this kind of case.

During validations, translation_for will be called:
https://github.com/shioyama/mobility/blob/master/lib/mobility/backends/active_record/table.rb#L291

It will build a translation for this "en" locale, even though this is not necessary.
Additionally I have a presence validation on the Translation class (friendly_id slug). Therefore due to this, I cannot save my models, because fallbacks is generating empty translations on the model. Even if I did not have that validation, it would create unwanted empty translations in the DB upon saving.

Temporarily I fixed it by patching generate_fallbacks to exclude the 'en' locale, but that only partly fixes it - only for models which have translations for all locales I use.

Possible solution would be for the reader to not call build, and only writer to call build. I can whiff up a PR if you think that solution is the best we can do.

@mrbrdo mrbrdo changed the title Validations create empty translations Validations create empty translations in DB Apr 18, 2022
@shioyama
Copy link
Owner

shioyama commented May 3, 2022

Please write a failing test, that's much clearer than a long explanation.

@mrbrdo
Copy link
Contributor Author

mrbrdo commented May 3, 2022

See this failing spec: https://github.com/mrbrdo/mobility/blob/mrbrdo/spec/mobility/backends/active_record/table_spec.rb#L112

Also, I'm not sure that a fallback for en-XX should always include en, even if it's explicitly not specified (such as fallbacks({ :'en-US' => 'de-DE', :pt => 'de-DE' }) will still include en and de fallbacks). This makes the above issue even worse, and I can imagine it causing problems in other situations too. I understand it from perspective of I18n with Yaml files, but it makes less sense for Mobility fallbacks, as if I am using en-US and not en, there is absolutely no reason I would have en translations in my DB.
I cannot add a failing spec for this part, because it is a discussion.

mrbrdo added a commit to mrbrdo/mobility that referenced this issue May 4, 2022
@mrbrdo mrbrdo linked a pull request May 4, 2022 that will close this issue
@mrbrdo
Copy link
Contributor Author

mrbrdo commented May 4, 2022

@shioyama I think the fix could be as simple as #572
I did some testing with my app (Spree) and it seems its working well. My failing spec is now passing with this change. Some existing specs need to be updated due to the change.

mrbrdo added a commit to mrbrdo/mobility that referenced this issue May 4, 2022
@shioyama
Copy link
Owner

Unfortunately the fix in #572 doesn't work with caching (several tests are failing on that branch).

mrbrdo added a commit to mrbrdo/mobility that referenced this issue May 24, 2023
mrbrdo added a commit to mrbrdo/mobility that referenced this issue May 24, 2023
mrbrdo added a commit to mrbrdo/mobility that referenced this issue Jun 12, 2023
mrbrdo added a commit to mrbrdo/mobility that referenced this issue Jun 12, 2023
mrbrdo added a commit to mrbrdo/mobility that referenced this issue Jun 12, 2023
mrbrdo added a commit to mrbrdo/mobility that referenced this issue Jan 5, 2024
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 a pull request may close this issue.

2 participants