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
[i18n] Save preferred_language for auth user in db #3103
Conversation
@ghislaineguerin @kgodey assigned you for a product related review. |
@rajatvijay Looks good to me. |
Could we name the option "UI Language" or "Display Language"? I think that's clearer than "Preferred Language". I'd also update the backend name to match. Otherwise, looks good. |
"Preferred Language" and "Display Language" both seem fine to me. But "UI Language" seems like is has the potential to confuse some people who are not familiar with that acronym. |
Let's do "Display Language" then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the changes to the backend code and it looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noted a couple small comments on the diff, but a larger issue here is that we'll need to find a way to hide this UI before we can merge it. Otherwise we should hold off on merging until we have the full Japanese translation available.
mathesar_ui/src/systems/users-and-permissions/SelectPreferredLanguage.svelte
Outdated
Show resolved
Hide resolved
mathesar_ui/src/systems/users-and-permissions/UserDetailsForm.svelte
Outdated
Show resolved
Hide resolved
Commented out the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now 👍🏻
I'm holding off merge since this is stacked on top of #3102. @rajatvijay feel free to merge this yourself once that PR is merged. |
I'm trying to move this PR along in order to reduce our PR backlog so I spent some time resolving git merge conflicts in several files. Even though I'm not familiar with the code changes in this PR, the git conflicts seemed straightforward enough that I resolved them in c0b6cb3. However, when I try to run Mathesar after that commit, the service container prints:
In an attempt to fix this problem, I ran the following command:
That gave me this output, which seems quite strange:
Aside from the "Conflicting migrations detected" message, Mathesar appears to function normally. I would like a back-end engineer to look into this. |
I figured it out — it was an issue with accepting user input when using
I committed that migration in 825a36f |
If all the tests pass, I'm assuming we're good to go here, so I've queued this to merge now. |
Relates #2927
Technical details
This PR does the following things -
Locale
middleware in BEpreferred_language
attribute in theusers
model and serializers. Migrations for this field.Preferred Language
dropdown in the user's profile page.Screenshots
users_lang_in_db.mp4
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin