-
Notifications
You must be signed in to change notification settings - Fork 9
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/backend) Fix/email in receiving user language #401
base: main
Are you sure you want to change the base?
🐛(i18n/backend) Fix/email in receiving user language #401
Conversation
c645b5b
to
72b5272
Compare
62ebf55
to
332e770
Compare
@sampaccoud |
fef3dea
to
3b3e90c
Compare
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 !
A few comments.
@sampaccoud Should we add a migration to make all our current users french by default, because they will all switch to english after that, wdyt ?
src/frontend/apps/impress/src/features/language/api/useChangeUserLanguage.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/language/api/useChangeUserLanguage.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/language/LanguagePicker.tsx
Outdated
Show resolved
Hide resolved
b3534d3
to
704550b
Compare
704550b
to
d9bad8c
Compare
src/frontend/apps/impress/src/features/language/LanguagePicker.tsx
Outdated
Show resolved
Hide resolved
d9bad8c
to
f773350
Compare
1b5eb7b
to
38639c9
Compare
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.
🎉 Good job, works perfect ^^
I think Sam wants to have a look at it before merging.
I was expecting the language to be set on the user on first logging here: Otherwise how and when is this field set on the user? 🤔 |
src/backend/core/tests/documents/test_api_document_invitations.py
Outdated
Show resolved
Hide resolved
// Switch i18n.language and user.language via API | ||
const switchLanguage = (targetLocale: string): void => { | ||
const actions: Promise<unknown>[] = [i18n.changeLanguage(targetLocale)]; | ||
|
||
if (userData?.id) { | ||
actions.push( | ||
changeUserLanguage({ | ||
userId: userData.id, | ||
language: targetLocale, | ||
}), | ||
); | ||
} |
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 was expecting the language to be set on the user on first logging here: https://github.com/numerique-gouv/impress/blob/main/src/backend/core/authentication/backends.py#L91
Otherwise how and when is this field set on the user? 🤔
When we have a user, we will always have a language set on the user, the choice is not nullable.
So we set, whatever has been set on the user, as language in the frontend.
If the user is unsatisfied with the language that is set, he switches it here (code above) and in turn it updates the attribute at the user.
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.
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.
Ok but language will default to settings.LANGUAGE_CODE in the backend. Since the user can only be created via a login, it is a pity not to try harder to set the user's language on first login!
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 thought users are created through the Identity Provider? I was thinking to let the IdP transmit the preferred language for initial Account creation.
Also since its not nullable, from the frontend, i have no way of knowing, that what is set on the user, was not set by the IdP or by him.
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.
Ok. It could be one possibility but it's not in the case of our idP. If it came from the idP it would have to be added here right as well right?
If it's not set by the idP, you sould set it from the request at the moment of user creation ie on first loggging like I propose?
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.
Ok, i will try to find an elegant way and implement it on monday
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.
Unfortunately, i did not find an elegant way, with the time i had.
I'm in the holidays now and will revist this in january 2025.
E-mails sent for granting access are sent in the receiving users language. Falling back to system default language.
On Language change in the frontend, the user language is updated via API. If user language is available, it will be preferred and set in the frontend.
62c3dd6
to
6a562b4
Compare
Implements the following:
Closes #323