fix: Translations related to the date range filter
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:
- 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'.
- The next step is to review/edit/fix the values in the .po file
- 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.
- 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:
After:
TESTING INSTRUCTIONS
- Launch a new instance of superset loaded with examples and the French language enabled
- Open the "Birth Rates" dashboard
- Create a new "Time Range" filter
- Switch to French
- [ ] 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
- [X] Has associated issue:
- Associated to but does not fix #17779
- Associated to but does not fix #25728
- Associated to but does not fix #26061
- Related to PR #26010
- Related to Discussion #25994
- [ ] Required feature flags:
- [X] Changes UI
- [ ] Includes DB Migration (follow approval process in SIP-59)
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Codecov Report
Attention: 2 lines in your changes are missing coverage. Please review.
Comparison is base (
983a164) 67.20% compared to head (cde535a) 69.53%. Report is 4 commits behind head on master.
| Files | Patch % | Lines |
|---|---|---|
| ...ontrols/DateFilterControl/components/DateLabel.tsx | 0.00% | 0 Missing and 1 partial :warning: |
| superset/views/api.py | 0.00% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## master #26074 +/- ##
==========================================
+ Coverage 67.20% 69.53% +2.33%
==========================================
Files 1905 1905
Lines 74498 74529 +31
Branches 8324 8338 +14
==========================================
+ Hits 50066 51825 +1759
+ Misses 22377 20654 -1723
+ Partials 2055 2050 -5
| Flag | Coverage Δ | |
|---|---|---|
| hive | 53.80% <0.00%> (?) |
|
| javascript | 56.97% <0.00%> (+0.02%) |
:arrow_up: |
| mysql | 78.02% <0.00%> (ø) |
|
| postgres | 78.12% <0.00%> (ø) |
|
| presto | 53.75% <0.00%> (?) |
|
| python | 83.08% <0.00%> (+4.82%) |
:arrow_up: |
| sqlite | 77.63% <0.00%> (ø) |
|
| unit | 56.42% <0.00%> (?) |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for running the checks.
- I fixed the PR title for the lint
- I'm not sure what I can do about the Docker build failing
As to the code coverage warnings. I don't see where those can be fixed. I looked for tests covering the code around the 2 flagged issues, but found nothing. If someone can point me in the right direction, I'm happy to fix those as well.
I was looking for a solution for this exact problem and came across your PR, thank you very much for your effort @Ralkion
The docker issue in the workflow looks like a 502 error, it may have been a temporary outage. If you can, you should try restarting the workflow.
Hi @Ralkion, you just have to change your title to have the last test to pass the : nedd to be right after fix
Thanks @michael-s-molina for fixing the name. When it was mentioned that I had to fix the name I honestly thought I had done so already, so figured something else must've been up.
Due to the size of the PR, and the failing checks, I had planned on splitting this into two PRs -> one to address the functionality and one to address the translations, but never found the time to do so. Glad to see that it's passing the checks now!
Thanks @Ralkion for this PR. What is missing for this PR? Any hope to see it moving forward?
Thanks @Edern . I'm honestly not entirely sure what needs to come next. I was thinking it might make sense to split the PR into two parts - one for the functionality and one for the language files. That might make the review easier; I just haven't done this yet.
And honestly the PR was made so long ago there may be some things out-of-date, so It'd probably be a good idea for me (or the code owner(s)) to check and make sure everything still makes sense.
I think most of this PR looked good... it just got bogged down because it's busy and disables lint warnings, probably :P
If you want to rebase it to resolve the conflicts, I think we can try harder to shove it through. You did all the right things here as far as translated strings (using the %()s substitutions, and making changes to the .po files rather than the .json like so many do.
I don't think you need to split it... we just shouldn't have let it slip under the radar like it did. If you rebase it, I'll test/review it :)
Thanks so much @rusackas. I've merged main into the branch, so it should be all good now I think.
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.
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?
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 :)
@rusackas. I think I finally got through all the conflicts and re-applied my FR translations. Thanks for your assistance.
@Ralkion thanks for this! Some pre-commit checks are failing. I'll stamp this but those should be fixed to merge this in
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.