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

Use JS I18n function in assets to avoid parsing error #6750

Merged
merged 1 commit into from
Jan 28, 2021
Merged

Use JS I18n function in assets to avoid parsing error #6750

merged 1 commit into from
Jan 28, 2021

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Jan 27, 2021

What? Why?

Closes #6749

I checked all assets for the same pattern and they seem fine:

$ git ls-files -- app/assets/**.erb* | xargs git grep -w t -- 
app/assets/javascripts/admin/spree/base.js.erb:  // Trunkate text in page_title that didn't fit
app/assets/javascripts/admin/spree/orders/variant_autocomplete.js.erb:        alert(t("js.admin.orders.quantity_adjusted"));
app/assets/javascripts/admin/spree/orders/variant_autocomplete.js.erb:      alert(t("js.admin.orders.quantity_unchanged"));

What should we test?

Repeat the steps in #5989 (comment).

Release notes

Changelog Category: Technical changes

Use javascript translate function in all assets.

A deployment to the French server failed because a translation contained
an apostrophe `'` and we were rendering it without escaping in
Javascript. We don't have that problem and avoid other issues by using
the javascript translate function. That way the error message is
translated in the browser with the user's language and we don't have to
do any additional escaping.
@mkllnk mkllnk self-assigned this Jan 27, 2021
@mkllnk mkllnk requested a review from andrewpbrett January 27, 2021 03:24
Copy link
Contributor

@andrewpbrett andrewpbrett left a comment

Choose a reason for hiding this comment

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

Looks good - nice catch, and good thinking to check elsewhere for the same pattern.

@filipefurtad0 filipefurtad0 self-assigned this Jan 27, 2021
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 27, 2021
@filipefurtad0
Copy link
Contributor

Hey @mkllnk ,

I think the previous PR addresses correctly the editing an order, but there are some issues still around the creation of an order.

With a stock limit of 5 items, for that product, with no inventory overrides we can observe the following, while

Editing an order - this is OK - the stock can only be increased till 10 👍

Peek 2021-01-27 17-47

Creating an new order:

Peek 2021-01-27 17-46

The selector does not set a limit. Also, after creating the line item it's possible to increase the stock till 6, which is incorrect.

Updating issue #6739 to include this last bit.

This last case is not introduced by this PR. This is ready to go.

@filipefurtad0 filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 27, 2021
@Matt-Yorkley Matt-Yorkley merged commit de530c2 into openfoodfoundation:master Jan 28, 2021
@mkllnk mkllnk deleted the 6749-i18n-in-assets branch January 28, 2021 22:38
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.

Deployment fails on French letters in javascript compilation
4 participants