[Backport] Fix translatable bugs #2985
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change forces us to use nested attributes for translations, instead
of using the more convenient `:"title_#{locale}"` methods.
On the other hand, we can use Rails' native `_destroy` attribute to
remove existing translations, so we don't have to use our custom
`delete_translations`, which was a bit buggy since it didn't consider
failed updates.
Creating a new form builder might be too much. My idea was so the view uses more or less the same syntax it would use with Rails' default builder, and so we can use `text_field` instead of `translatable_text_field`.
We were reloading the values from the database and ignoring the parameters sent by the browser.
After adding a new translation with invalid data and sending the form, we were disabling the new translation when displaying the form again to the user, which was confusing.
After removing a translation while editing another one with invalid data and sending the form, we were displaying the removed translation to the user. We now remove that translation from the form, but we don't remove it from the database until the form has been sent without errors.
The same way we did for banners. We needed to add new translation keys so the labels are displayed in the correct language. I've kept the original `title` and `body` attributes so they can be used in other places. While backporting, we also added the original translations because they hadn't been backported yet.
Note the title field was hidden since commit 01b9aa8, even though it was required and translatable. I've removed the required validation rule, since it doesn't seem to make much sense and made the translatable tests harder to write. Also note the method `I18n.localize`, which is used to set the milestone's title, uses `I18n.locale` even if it's inside a `Globalize.with_locale` block, and so the same format is generated for every locale.
Updating it required reorganizing the form so translatable fields are together. We also needed to add a `hint` option to the form label and input methods so the hint wouldn't show up for every language. Finally, the markdown editor needed to use the same globalize attributes as inputs, labels and hints, which adds a bit of duplication.
If we enabled the locale and then added an option, the "add option" link added the partial which was generated before enabling the translation, and so it generated a field where the translation was disabled. Enabling the translation after inserting each field solves the issue.
The `:name` attribute is still allowed in the controller because some forks use it when creating a poll from a budget.
We need to replace ".title=" by ".title_#{locale}=" in one place because
for some reason globalize builds a new translation record when using the
latter but it doesn't build one when using the former.
We needed to bring back support for CKEditor in our translatable form, which we had temporarily remove. And now we support CKEditor in our translatable specs, and so we can remove the duplicated specs for poll question answers.
This refactor is going to be useful when we change these rules within the next few commits.
Globalize creates a translation for the current locale, and the only way I've found to change this behaviour is to monkey-patch it. The original code uses `translation.locale` instead of `Globalize.locale`. Since `translation.locale` loads the translation with empty attributes. It both makes the record invalid if there are validations and it makes it almost impossible to create a record with translations which don't include the current locale. See also the following convertations: globalize/globalize#328 globalize/globalize#468 globalize/globalize#578 https://github.com/shioyama/mobility/wiki/Migrating-from-Globalize#blank-translations
I've chosen the name "Globalizable" because "Translatable" already existed.
This way we guarantee there will be at least one translation for a model and we keep compatibility with the rest of the application, which ideally isn't aware of globalize.
also add unit tests for this function and a description into the model of the behaviour of the function
In order to be consistent with similar specs, the I18nContent#begins_with_key spec was improved by explicitly specifying an I18n key for one (1) factory that relied on its default value
When using the OR operator, if the left side of the expression evaluates to false, its right side is taken into consideration. Since in Ruby nil is false, we can avoid using conditionals for this particular scenario
This part used the code we deleted in order to make it easier to refactor the rest of the translatable models. Now we add the code back.
Unfortunately the builder didn't offer any options for the error message and we just had to overwrite the `error_for` methods.
This way we can show/hide that div when displaying translations, and we can remove the duplication applying the same logic to the label, the input, the error and the CKEditor. This way we also solve the problem of the textarea of the CKEditor taking space when we switch locales, as well as CKEditor itself taking space even when not displayed.
When we grouped the fields together, the last one turned into a `last-child`, which foundation automatically aligns to the right. The markdown editor also needed to be tweaked a little bit.
The current locale wasn't being marked for destruction and so saving the record tried to create a new translation for the current locale.
We broke this behaviour by introducing translations and not allowing the `id` parameter anymore.
We were allowing the `_destroy` field for translations, but not for the options themselves.
This way we fix the rubocop warning for line too long and make the code a bit easier to read.
The new options were being added inside the `.column` div, when they needed to be added before it.
We don't use that attribute since we added translations for this model.
We use underscores for IDs and hyphens for classes.
By using the input and finding it by its name, it's easier to see the difference between this input and the delete-language link.
The same way it's done in the rest of the application.
Having translations makes this validation too complex and not worth the effort, since now we can't just scope in a single column but we need to scope in the translations locale and the question ID.
When we were visiting a page showing the content of a record which uses globalize and our locale was the default one and there was no translation for the default locale, the application was crashing in some places because there are no fallbacks for the default locale. For example, when visiting a legislation process, the line with `CGI.escape(title)` was crashing because `title` was `nil` for the default locale. We've decided to solve this issue by using any available translations as globalize fallbacks instead of showing a 404 error or a translation missing error because these solutions would (we thinkg) either require modifying many places in the application or making the translatable logic even more complex. Initially we tried to add this solution to an initializer, but it must be called after initializing the application so I18n.fallbacks[locale] gets the value defined in config.i18n.fallbacks. Also note the line: fallbacks[locale] = I18n.fallbacks[locale] + I18n.available_locales Doesn't mention `I18n.default_locale` because the method `I18n.fallbacks[locale]` automatically adds the default locale.
This spec depends on French falling back to Spanish and was failing on forks using a different fallback.
Since we've recently added German to the available languages, and we might support every language in the future, we're using the fictional world language to check a locale which isn't available. Another option could be to set the available locales in the test environment (or the rspec helper), but then we'd have to make sure it's executed before the call to `globalize_accessors` in the model, and it might be confusing for developers.
Ruby can't have hyphens in method names, so sending something like `title_pt-BR=` would raise an exception.
javierm
force-pushed
the
backport-2875-translatable_bugs
branch
from
fbe5ccc
to
ccdbdb2
Compare
voodoorai2000
approved these changes
Oct 25, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Notes