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

Adds missing keys from i18n tasks output #11958

Conversation

filipefurtad0
Copy link
Contributor

@filipefurtad0 filipefurtad0 commented Dec 20, 2023

What? Why?

Related to #11938.

The output from i18n-tasks missing has been added to this PR - it will be deleted on the last commit once all the entries have been verified. The idea is to check all of them and either ignore (if the key is found on en.yml - this seems to happen for dynamic keys) or add it to en.yml it it is indeed verified to be missing. In some case, the view file has been deleted, this is the case for:

  • partial delete on app/views/devise/shared/_links.html.erb Remove unused Devise login links partial #12298
  • app/views/checkout/payment/_gateway.html.haml (commit in this PR)
  • app/views/home/_connect.html.haml (commit in this PR)
  • app/views/home/_learn.html.haml (commit in this PR)

What should we test?

The scope is missing translations, so ideally, a quick test in using en.yml and a second locale should be done for the following pages, which should render well:

  1. /admin/stripe_connect_settings/edit -> Instance Publishable Key
    image

  2. verify that translations work as before on all pages, during customer checkout
    I've removed this file, I don't think it is used:

  3. As superadmin, check taxonomy pages, namely editing and creating new taxonomies. Can you find a remove button? Is it rendering correctly?

  4. As a customer, initiate the checkout process, by adding some items to the cart. Edit the cart. Is the Continue shopping button displayed correctly, as before?

  5. As admin, edit an order without any payments. Are you able to see any message stating that "the order has no payments"? I believe this was never the case. In summary: going through the side tabs, when editing an order as admin, should display all information correctly.

  6. Ideally, we would trigger the flash message: "The order could not be updated" but I don't know how to trigger this. Will ask testers and update instructions.

  7. As superadmin, editing states pages should display correctly

  8. As superadmin, under /admin/contents/edit, verify that it is possible to enable/disable the Menu 6 icon name and Menu 7 icon name sections of the homepage.

image

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@filipefurtad0 filipefurtad0 marked this pull request as draft December 20, 2023 12:19
@filipefurtad0 filipefurtad0 self-assigned this Dec 20, 2023
@filipefurtad0 filipefurtad0 force-pushed the adds_missing_keys_from_i18n_tasks_output branch from e357ce1 to e408f3e Compare February 21, 2024 12:36
@filipefurtad0 filipefurtad0 force-pushed the adds_missing_keys_from_i18n_tasks_output branch from ad2b09b to 4ec0085 Compare February 29, 2024 20:09
@filipefurtad0 filipefurtad0 force-pushed the adds_missing_keys_from_i18n_tasks_output branch from 0c8b0f5 to 88dbcdb Compare March 18, 2024 17:08
@filipefurtad0 filipefurtad0 force-pushed the adds_missing_keys_from_i18n_tasks_output branch from 8a1489e to 88dbcdb Compare March 20, 2024 12:21
@filipefurtad0 filipefurtad0 force-pushed the adds_missing_keys_from_i18n_tasks_output branch 3 times, most recently from e032490 to d1fb704 Compare March 21, 2024 12:22
missing_keys.txt Outdated
@@ -47,7 +47,7 @@
| all | spree.remove | app/views/spree/admin/taxons/_taxon_table.html.haml:15 | - missing translation, added key -> not sure though, where to trigger this missing translation
| all | spree.shipment_mailer.base_subject.picked_up_subject | app/mailers/spree/shipment_mailer.rb:15 | - exists in en.yml, seems to be a false positive
| all | spree.taxon_edit | app/views/spree/admin/taxons/edit.html.haml:4 | - missing translation, added key
| all | thank_you_for_your_order | app/views/spree/orders/show.html.haml:26 |
| all | thank_you_for_your_order | app/views/spree/orders/show.html.haml:26 | - missing translation, added key
| all | unrecognized_card_type | app/views/spree/admin/payments/source_forms/_gateway.html.haml:25 |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option could have been to reuse the key thanks: "Thank you for your business.".

unrecognized_card_type: ""
use_new_cc: "Use a new credit card"
what_is_this: "What is this?"
your_cart_is_empty: "Your cart is empty"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure these keys are needed... But they are indeed missing.

@filipefurtad0 filipefurtad0 force-pushed the adds_missing_keys_from_i18n_tasks_output branch from d1fb704 to e284d11 Compare March 21, 2024 17:49
@filipefurtad0 filipefurtad0 marked this pull request as ready for review March 21, 2024 17:49
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Great pull request! Easy to follow, step by step.

And nice cleanup along the way. 😍

@sigmundpetersen sigmundpetersen added the user facing changes Thes pull requests affect the user experience label Mar 22, 2024
@filipefurtad0 filipefurtad0 force-pushed the adds_missing_keys_from_i18n_tasks_output branch from 741420f to 278e56c Compare March 22, 2024 13:10
@filipefurtad0
Copy link
Contributor Author

I've noticed there were these two files (which I've marked as needing issues to fix missing translations):

  • app/views/home/_connect.html.haml
  • app/views/home/_learn.html.haml

However, after a second look, I'm convinced they are not in use, so I've removed them still within this PR. Re-requesting review 🙏

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Nice one ! Thanks for that 🙏 , it seems like an unpleasant task.

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

I'm convinced they are not in use, so I've removed them still within this PR.

Thank you! I forgot to remove them in a712f25.

@RachL
Copy link
Contributor

RachL commented Apr 10, 2024

@filipefurtad0 will this PR close #2626 ?

@filipefurtad0
Copy link
Contributor Author

will this PR close #2626 ?

Hey @RachL ,
Not exactly. This PR uses a gem to locally look for missing translations and fixes / removes some unnecessary files.

I'd say the actual PR contributing to #2626 is #11906, as per this comment.

I'm not sure we should keep it open though as we've improved the previous code to detect more types of missing translations, and because we've opted not to implement any of the follow-up suggestions (references, comment above and here) like:

  • integrating a build node to check the build
  • and a Bugsnag warning to spot missing translations in production;

So, I'd say yes, let's close!

@drummer83 drummer83 self-assigned this Apr 10, 2024
@drummer83 drummer83 added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Apr 10, 2024
I've checked staging, and I could not find this message, nor the entry in the en.yml file - I think we're not rendering this message currently, hence, the removal.
I'm not sure how to trigger this error, and triggering an update error message seems appropriate too - it's sort of an edge case, perhaps this is a valid approach
Adds string to missing key

To be squashed
Done to prevent/fix merge conflicts
Deletes file

We only needed this file for tracking progress, during review, we should not keep it in master I think
_learn and _connect seem to appear at the top of the homepage (and not as pane). Also, there was some hard coded URLs which seems not to be used, as the translations in the Configuration/Content section seem to work correctly.
@filipefurtad0 filipefurtad0 force-pushed the adds_missing_keys_from_i18n_tasks_output branch from 278e56c to d2c2e20 Compare April 15, 2024 14:06
@filipefurtad0
Copy link
Contributor Author

Hi @drummer83 , thanks for testing this one!

There are cases in which indeed, I don't know how to trigger the respective behavior. Some of them looked like dead code, so I've removed them in this PR. In other cases, I had no basis to assume that, and I felt it is better to add a key, then being to thorough on the grooming, which could actually create missing translations... Adding a key should not have any negative impact on the functionality of the app; worst case scenario, it's adding a key which is never rendered in the frontend. I don't see this as a big risk, so I've added keys on points

  • 3
  • 6 -> flash message commit is here f130470

On 4:

You replaced the old string 'your_cart_is empty' with the new one, but at the same time you added it as missing translation to en.yml line 265. I think it's not needed then anymore?

Good point. I guess this was my initial intention but left the key there, hanging. Done here. Thanks for spotting it!

As superadmin, under /admin/contents/edit, verify that it is possible to enable/disable the Menu 6 icon name and Menu 7 icon name sections of the homepage. ➡️ ✔️ Yes, this is working. But which part of the PR is affecting it?

None, the PR should not affect the pages. However, some files were removed, which could affect the page, but are not supposed to. This is the commit.

NB: The string 'taxon_edit' is used as page title only on a subpage of a page which uses 'taxonomy_edit' as page title. I think we could combine both of them in the existing 'taxonomy_edit'?

Nice catch! Done on the latest commit bfd4b73.

I think I've addressed everything. There are two new minor commits so I'm moving to code review.

Thanks again @drummer83 🙏

@drummer83 drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Apr 16, 2024
@drummer83
Copy link
Contributor

Thanks, @filipefurtad0, for clarifying and updating the last bits!

I have tested the changes of your latest commits.
It's all looking good! 🎉
Merging! 🚀
Thanks again!

@drummer83 drummer83 merged commit 31c1eee into openfoodfoundation:master Apr 16, 2024
52 checks passed
@drummer83 drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Apr 16, 2024
@dacook
Copy link
Member

dacook commented Apr 18, 2024

@filipefurtad0 Can you please confirm if this PR contained user-facing changes or not? I quickly scanned and couldn't see any evidence, but could be wrong.

@filipefurtad0
Copy link
Contributor Author

Hey @dacook,
I've marked it as user facing as it introduces some new translation keys, on en.yml. I've checked previous releases, I think this is the current process. What do you think?

@dacook
Copy link
Member

dacook commented Apr 19, 2024

Sorry I missed that, yes that sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user facing changes Thes pull requests affect the user experience
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants