Skip to content
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

Add language choice for installation #664

Conversation

CodeShakingSheep
Copy link
Member

@CodeShakingSheep CodeShakingSheep commented Feb 9, 2024

Problem

Nextcloud shows the following warning:

Your installation has no default phone region set. This is required to validate phone numbers in the profile settings without a country code. To allow numbers without a country code, please add "default_phone_region" with the respective ISO 3166-1 code of the region to your config file. For more details see the documentation ↗.

image

Solution

Added question for language in manifest.toml. Set 3 language variables (see here: https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html#default-language ) in install:

  • default_language
  • default_locale
  • default_phone_region

In upgrade I set default_phone_region if it's not set according to default_locale (which always has a value as it has a default value).

Edit: Setting default_phone_region for existing installations should be handled in config panel, cf. #638.

PR Status

  • Code finished and ready to be reviewed/tested
  • The fix/enhancement were manually tested (if applicable)

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

Add language select
Configure language settings via occ
Fix no default phone region
Fix language values for config.php
Fix language code format, add German
@CodeShakingSheep CodeShakingSheep changed the base branch from master to testing February 9, 2024 13:32
Fix setting default phone region
@CodeShakingSheep
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

🌻
Test Badge

Fix upgrading with from installation with no default locale
@CodeShakingSheep
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

Fingers crossed!
Test Badge

Fix upgrading from installation with no default locale
@CodeShakingSheep
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

✌️
Test Badge

scripts/upgrade Outdated
@@ -143,6 +143,10 @@ then
# Fix warning "Your installation has no default phone region set"
if [ "$(exec_occ config:system:get default_phone_region)" == "" ]; then
config_locale=$(exec_occ config:system:get default_locale)
if [ "$config_locale" == "" ]; then
# If it's not set in config use "US" as default
config_locale="US"
Copy link
Member

Choose a reason for hiding this comment

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

Why not FR by default?

Choose a reason for hiding this comment

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

Why not any other code. It would be more logical that the user chooses from a drop down list at the installation or change it in the app panel

Copy link
Member

Choose a reason for hiding this comment

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

We are talking about a default value set if there was no previous value set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @ericgaspar,
I set "US" because in manifest.toml I used "en_US" as default install language and so I wanted to use the default country here as well.

Copy link
Member

Choose a reason for hiding this comment

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

let's then change the default language to fr (because it is by far the most used...)

Fix for checking if config_locale is set
@CodeShakingSheep
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

Alrighty!
Test Badge

@CodeShakingSheep
Copy link
Member Author

Alrighty! Test Badge

What is going on on the test machine? There is this error which is completely unrelated to this package. Can someone look into that?

WARNING W: GPG error: https://packages.sury.org/php bullseye InRelease: The following signatures were invalid: EXPKEYSIG B188E2B695BD4743 DEB.SURY.ORG Automatic Signing Key <[email protected]>

@@ -87,6 +87,17 @@ ynh_script_progression --message="Configuring $app..." --weight=8
# Set the mysql.utf8mb4 config to true in config.php
exec_occ config:system:set mysql.utf8mb4 --type boolean --value="true"

# Set default language in config.php
raw_language=$(echo "$language" | awk -F'_' '{print $1}')
exec_occ config:system:set default_language --value="$raw_language"
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be just $language here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, Nextcloud's default_language uses ISO_639-1 language codes such as en for English, see https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html#default-language

That's why we need to cut the first part from $language, e.g. take en from en_US.

@ericgaspar
Copy link
Member

is this language choice not best handled with config panels? cf. #638

@Tagadda
Copy link
Member

Tagadda commented Feb 19, 2024

I restarted the lastest CI job https://ci-apps-dev.yunohost.org/ci/job/13727

Remove fix for "no default phone region" (to be handled via config panel)
@CodeShakingSheep CodeShakingSheep changed the title Fix warning "no default phone region defined" and add language choice Add language choice for installation Feb 21, 2024
@CodeShakingSheep
Copy link
Member Author

is this language choice not best handled with config panels? cf. #638

Thanks for the hint. I hadn't checked out that PR. Indeed it seems to be the cleaner solution to set a value in config panel rather than to use a fixed default value during upgrade. So, I removed the fix in upgrade and renamed this PR accordingly.

@CodeShakingSheep
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

May the CI gods be with you!
Test Badge

@alexAubin
Copy link
Member

alexAubin commented Dec 3, 2024

(Discussed during today's meeting) Closing because it's been almost one year and it should really be something in the config panel rather than in the install questions ... in fact it's handled in #638 which does a bunch of other things, so closing in favor or #638

@alexAubin alexAubin closed this Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants