-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Clearer error message and clean up for Product Categories #12779
Clearer error message and clean up for Product Categories #12779
Conversation
fe6a2be
to
4211605
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for tackling this. Could you reduce your change to only en.yml? The other files will get updated automatically in our release process. It will be easier to see your actual changes then as well.
config/locales/ar.yml
Outdated
@@ -2653,7 +2653,6 @@ ar: | |||
spree_admin_single_enterprise_hint: "لمحة: للسماح للأشخاص بالعثور عليك ، قم بتشغيل الرؤية الخاصة بك أسفل" | |||
spree_admin_eg_pickup_from_school: "على سبيل المثال. "البيك اب من المدرسة الابتدائية"" | |||
spree_admin_eg_collect_your_order: "على سبيل المثال. "يرجى جمع طلبك من 123 Imaginary St، Northcote ، 3070"" | |||
spree_classification_primary_taxon_error: "Taxon %{taxon} هو التصنيف الأساسي %{product} ولا يمكن حذفه" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never change other locales than en.yml because they are updated automatically through our Transifex integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never change other locales than en.yml because they are updated automatically through our Transifex integration.
OK.I will update it.
4211605
to
503148b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you!
Note that we usually don't merge master but rebase on master if needed. You don't have to do that for this pull request though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks. However as per the issue, can you please also confirm if spree.new_taxon
translation is being used?
Ok,I see.Thank you tell me. |
Maybe this use it.I'm not sure. |
I think the following translation is being used here because it's using Rail's lazy translation lookup and it should get translated to this key based upon the file path in which it's used, openfoodnetwork/config/locales/en.yml Line 4587 in 9170799
I think we should validate this and remove |
I have already deleted spree.new_taxon.Thanks |
Really sorry, but I believe you have pushed that change in another branch which might not be relevant to that change. Can you please validate and push that change in this branch so that the relevant issue's changes are within their respective branches? |
Thank you for bringing this to my attention. I apologize for the confusion earlier. I have now verified and corrected the issue. |
No worries @EdwardLi-coder, all good now. Thanks |
60e637e
to
05315ff
Compare
Hi @EdwardLi-coder, I have tested the product categories and couldn't find any problems. Also the new error message is shown: Excellent! Merging! 🎉 |
Ok.I'm glad my code can merge.Thanks |
What? Why?
I found that the code had no place to use spree_classification_primary_taxon_error and invalid_taxonomy_id, so I deleted them
What should we test?
As described in the issue:
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates