superset icon indicating copy to clipboard operation
superset copied to clipboard

fix: Translations related to the date range filter

Open Ralkion opened this issue 2 years ago • 8 comments

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

  • [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

Ralkion avatar Nov 22 '23 19:11 Ralkion

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.

codecov[bot] avatar Nov 25 '23 21:11 codecov[bot]

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.

Ralkion avatar Nov 27 '23 19:11 Ralkion

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.

fatiiates avatar Dec 27 '23 14:12 fatiiates

Hi @Ralkion, you just have to change your title to have the last test to pass the : nedd to be right after fix

aehanno avatar Jan 12 '24 19:01 aehanno

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!

Ralkion avatar Jan 17 '24 14:01 Ralkion

Thanks @Ralkion for this PR. What is missing for this PR? Any hope to see it moving forward?

Edern avatar Feb 12 '24 10:02 Edern

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.

Ralkion avatar Feb 12 '24 15:02 Ralkion

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 :)

rusackas avatar Feb 12 '24 17:02 rusackas

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

Ralkion avatar Feb 13 '24 14:02 Ralkion

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.

rusackas avatar Feb 13 '24 16:02 rusackas

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?

Ralkion avatar Feb 16 '24 13:02 Ralkion

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 avatar Feb 16 '24 16:02 rusackas

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

Ralkion avatar Feb 16 '24 19:02 Ralkion

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

geido avatar Feb 21 '24 16:02 geido

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.

Ralkion avatar Feb 21 '24 20:02 Ralkion