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

fix: Translations related to the date range filter #26074

Merged
merged 31 commits into from
Feb 21, 2024

Conversation

Ralkion
Copy link
Contributor

@Ralkion Ralkion commented Nov 22, 2023

SUMMARY

This PR partially addresses discussion #25994 & issue #26061. It fixes the issue with the date range filter not showing translated values.

The issue was that the front-end client was sending requests that asked for strings like "Last year" and displaying the timeRange property in the response. As it stands now, the backend translates the human-readable string into actual dates, along with the requested string in the timeRange property (see api.py#L98. The front-end needed to translate the property coming back, but also needed to be sure that it expected the same value it sent (see changes in DateRangeFilter.tsx).

One thing I will say is that it was not clear at first how to go about fixing translation issues. I initially did the same as #26010, thinking that updating the .po and .json files was the correct path.
It wasn't until later that I realized that a 'proper' fix would follow the instructions in the docs. Most of this PR is actually the result of trying to resolve the issues that existed in the translation pipeline.

I'm going to outline it here. I know I was confused for a while, and it seems like others were also confused as per the discussion in #26010.
The translation is a multi-step process:

  1. One should do a full extraction /scripts/babel_update.sh
  • This will generate the .pot file based on the strings in the code
  • Then it will merge the .pot file with the existing .po files to the best of its ability
    • This will sometimes cause the tool to make 'guesses' at what the value should be if it doesn't already exist. Those guesses are annotated as 'fuzzy'.
  1. The next step is to review/edit/fix the values in the .po file
  2. Next generate the .json file(s) with the /scripts/po2json.sh file.
  • The .json file is used by the frontend code for translations
  • 'fuzzy' values in the .po are not translated to the .json files.
  1. Finally generate the .mo file(s) with the command pybabel compile -d superset/translations
  • The .mo file is used by the backend code for translations
  • The .mo file is not checked into the git codebase. I'm unclear as to why the .json file is if the .mo file is not.
  • It is unclear looking at the UI which strings are generated by the frontend and which are generated by the backend

What is not addressed by this PR from the original issue is the time-grain values. I've looked into fixing this, but since the front-end is using standard 'select' input fields to display this, there's no good location in the front-end to force a translation. I do have a branch that does the translation in the backend (which is why I had to dig so deep into the translation stuff for the backend .mo files), but that's breaking some integration tests so I need to find another way to do this. I'm open to suggestions if anyone has any.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
image
After:
image

TESTING INSTRUCTIONS

  1. Launch a new instance of superset loaded with examples and the French language enabled
  2. Open the "Birth Rates" dashboard
  3. Create a new "Time Range" filter
  4. Switch to French
  5. Verify that the new filter display the properly translated values after selecting one of the 'last X' options instead of the English versions

ADDITIONAL INFORMATION

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@Ralkion
Copy link
Contributor Author

Ralkion commented Feb 13, 2024

Thanks so much @rusackas. I've merged main into the branch, so it should be all good now I think.

@rusackas
Copy link
Member

Uh oh... I didn't realize it, but another PR updating .pot files and all the rest caused another nasty pile of conflicts. It's hard to tell how many changes you made to the .po files for this PR, but I think you might just need to rebase again, accept all changes, and re-generate the pot/po/json files. Reach out if you want to go through it on slack/zoom, since it's... kinda my fault this is stuck again.

@Ralkion
Copy link
Contributor Author

Ralkion commented Feb 16, 2024

No worries @rusackas. I'm working through the conflicts now. Quick question as I wanted to verify.

_("..%(var)s", var="str") is preferred over _("..%(var)s") % {"var": "str"} correct?

@rusackas
Copy link
Member

No worries @rusackas. I'm working through the conflicts now. Quick question as I wanted to verify.

_("..%(var)s", var="str") is preferred over _("..%(var)s") % {"var": "str"} correct?

I believe so, yes... and I think if you get it wrong, one of the scripts complains :)

@Ralkion
Copy link
Contributor Author

Ralkion commented Feb 16, 2024

@rusackas. I think I finally got through all the conflicts and re-applied my FR translations. Thanks for your assistance.

@geido
Copy link
Member

geido commented Feb 21, 2024

@Ralkion thanks for this! Some pre-commit checks are failing. I'll stamp this but those should be fixed to merge this in

@Ralkion
Copy link
Contributor Author

Ralkion commented Feb 21, 2024

Thanks. I've managed to fix the pre-commit issue, but I've been struggling with the cypress one. I can't get cypress to run on my local machine (running on Ubuntu in WSL 1 due to corp restrictions).

I think I've traced it to this line though (nativeFilters.test.ts#L429). Without being able to run it, I'm not sure why it's finding China when it's expecting to find nothing.

@rusackas rusackas merged commit cc2f6f1 into apache:master Feb 21, 2024
42 checks passed
@michael-s-molina michael-s-molina added v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch and removed v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch labels Feb 22, 2024
@Ralkion Ralkion deleted the issue/26061-date-range-filter branch February 23, 2024 18:51
michael-s-molina pushed a commit that referenced this pull request Mar 4, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
@mistercrunch mistercrunch added 🍒 4.0.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the REST API 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels i18n:brazilian i18n:chinese Translation related to Chinese language i18n:dutch i18n:french Translation related to French language i18n:italian Translation related to Italian language i18n:japanese Translation related to Japanese language i18n:korean Translation related to Korean language i18n:portuguese i18n:russian Translation related to Russian language i18n:slovak i18n:spanish Translation related to Spanish language i18n:ukrainian i18n Namespace | Anything related to localization size/XXL v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch 🍒 4.0.0 🍒 4.0.1 🍒 4.0.2
Projects
None yet
8 participants