Skip to content

Set Default User Locale #2028

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.

Sign up for GitHub

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

Jump to bottom
Merged
battermann merged 6 commits into from
Jan 11, 2022
Merged

Set Default User Locale #2028

battermann merged 6 commits into from
Jan 11, 2022

Conversation

@battermann
Copy link
Contributor

@battermann battermann commented Jan 6, 2022 •
edited
Loading

https://wearezeta.atlassian.net/browse/SQSERVICES-1097

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.
    • If new config options introduced: added usage description under docs/reference/config-options.md
    • If new config options introduced: recommended measures to be taken by on-premise instance operators.

@battermann battermann force-pushed the SQSERVICES-1097-introduce-default-preferred-langauge branch from af2fb97 to fde065d Compare January 6, 2022 16:12
fisx
fisx previously approved these changes Jan 6, 2022
View reviewed changes
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

Nice, especially the LOC stats! :)

Yes, this could work. Please test as you suggested before merging, though.

charts/brig/templates/configmap.yaml Outdated
Copy link
Contributor

@fisx fisx Jan 6, 2022

Choose a reason for hiding this comment

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

Given that with our current i8n status the only valid locale here is en, shoudl we really make it configurable? I see no great harm, as long as any other values crashes on startup, but not much value either.

@battermann battermann marked this pull request as ready for review January 7, 2022 09:16
@fisx fisx dismissed their stale review January 7, 2022 10:44

i want to take another look at the new changes.

fisx
fisx reviewed Jan 9, 2022
View reviewed changes
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

I'm assuming you tested the DefaultUserLocale trick manually? Then except for the missing details in the haddocks this looks very shippable.

@battermann battermann force-pushed the SQSERVICES-1097-introduce-default-preferred-langauge branch from bfb6318 to 159b927 Compare January 10, 2022 11:54
@battermann battermann force-pushed the SQSERVICES-1097-introduce-default-preferred-langauge branch from 159b927 to 1e7bf3a Compare January 10, 2022 11:56
@battermann battermann requested a review from fisx January 10, 2022 13:02
fisx
fisx approved these changes Jan 10, 2022
View reviewed changes
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

NB the suggestion to fix the cycle (black hole), otherwise LGTM!

services/brig/src/Brig/Options.hs Outdated
defaultTemplateLocale = Locale (Language EN) Nothing

defaultUserLocale :: Locale
defaultUserLocale = defaultUserLocale
Copy link
Contributor

@fisx fisx Jan 10, 2022

Choose a reason for hiding this comment

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

cycle.

Suggested change
defaultUserLocale = defaultUserLocale
defaultUserLocale = defaultTemplateLocale

(anyway it'd also be ok with me if you made both of them just en.)

@battermann battermann merged commit 8057636 into develop Jan 11, 2022
@battermann battermann deleted the SQSERVICES-1097-introduce-default-preferred-langauge branch January 11, 2022 11:24
@akshaymankar akshaymankar mentioned this pull request Jan 18, 2022
Merged
jschaul
jschaul reviewed Jan 19, 2022
View reviewed changes
docs/reference/config-options.md

The brig server config option `setDefaultLocale` has been replaced by `setDefaultUserLocale` and `setDefaultTemplateLocale`. Both settings are optional and `setDefaultTemplateLocale` defaults to `EN` and `setDefaultLocale` defaults to `setDefaultTemplateLocale`. If `setDefaultLocale` was not set or set to `EN` before this change, nothing needs to be done. If `setDefaultLocale` was set to any other language other than `EN` the name of the setting should be changed to `setDefaultTemplateLocale`.

#### `setDefaultTemplateLocale`
Copy link
Member

@jschaul jschaul Jan 19, 2022

Choose a reason for hiding this comment

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

Which templates are actually complete in wire-server other than EN? i.e. this settings can't be set to anything other than EN at this point in time unless more templates are provided inside wire-server's brig/deb/templates folder, right? Perhaps the description here should make that clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jschaul jschaul jschaul left review comments

@fisx fisx fisx approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@battermann @jschaul @fisx