Missing translations for tax category

[ashishp91]

What? Why?

• Closes [Missing translation] "none" is not translatable #13021

When tax category is null for a product then by default the row value is set to none. Changing the return value to I18n("none") returns the value none in the appropriate translation.

What should we test?

1. Login as an admin.
2. Go to profile icon -> administration.
3. Click on Reports.
4. Scroll down to All Products and click on it.
5. Generate a report for a product without a tax category.

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

[dacook]

Hi, welcome and thanks for the contribution!

I don't think it's necessary to add a unit test, as it's already well covered by report tests (as you found out!)

I think it was a good idea to consider the existing translation. However, this none translation string is a very general string (it is at the top level of the locale file) and it would be good to have a more specific string this specific purpose.
Would you be able to create a new entry under the reports: section instead? (https://github.com/openfoodfoundation/openfoodnetwork/blob/master/config/locales/en.yml#L1762). This will allow translators more flexibility to choose the best translation for that context.

It would be helpful to use the existing lowercase "none" string, which will avoid the need to change any report specs.

[ashishp91]

Hello @dacook I've moved the none translation under reports. Please let me know if you need anything else. Thanks

[dacook]

Thanks for following up and making extra changes. I'm sorry but I'll need to request some more changes though:

1. There's no need to add translations for other languages. Can you please remove these because the manager of each locale may need to choose a different word to suit their context. The only file for us to change is the default en.yml
2. For en.yml, can you please choose lowercase "none"? This is what it was before, so users won't experience any change. (Imagine if somebody was using a spreadsheet formula on a report which failed because the string didn't match).
3. In tests, we actually prefer to hardcode the expected text instead of using the I18n.t helper. This is easier to read, which helps avoid errors. So if the string remains lowercase "none", the specs can stay the same as before.

Feel free to rewrite the git history if you're comfortable with that (eg with git rebase -i, otherwise it's ok to keep adding commits.

[ashishp91]

Hello @dacook, I have made the changes as requested. Silly of me that I didn't ask that earlier in slack dm, I thought about hardcoding and updating all the locales previously. But then I thought you might have your own bandwidth for tasks and dm'ing you might consume that.

[dacook]

Hi @ashishp91 , that's ok, there's always lots of questions when getting started on this codebase, it's not easy at all. Another challenge is that we might not be online at the same times, so don't be afraid to ask questions in the #dev channel, where people in your timezone may be able to help.

I'm sorry if I sounded negative before. We really appreciate the effort you put in to set this up, and I think it's worth continuing with what you had here. But sadly I see you have deleted your fork. Are you sure you don't want to continue on it?

[ashishp91]

Hey @dacook, My apologies, but I don't know how my repo got deleted. I've changed my github password. I've created #13256 to cover the changes.

I appreciate you guys taking time to review the PR and the way you've created the repo to help open source contributions. Thank you for all the hard work you guys have done. My contribution won't possible without that :)