Use `fullname` from `$admin` user info instead of deprecated `firstname`/`lastname` arguments

daniel-fahey commented 7 months ago

Problem

Emails from the app are sent with "null null" as the sender display name

MAIL_FROM_NAME="null null" in .env
Manual edits to .env are overwritten during upgrade

Reason

$admin user's info firstname/lastname keys are deprecated and no longer available since YunoHost version 11.1, so when they are retrieved by

email_firstname="$(ynh_user_get_info --username=$admin --key=firstname)"
email_lastname="$(ynh_user_get_info --username=$admin --key=lastname)"

and set by

ynh_app_setting_set --app=$app --key=email_firstname --value="$email_firstname"
ynh_app_setting_set --app=$app --key=email_lastname --value="$email_lastname"

email_firstname and email_lastname app settings are set to null.

During install and upgrade ynh_add_config replaces MAIL_FROM_NAME="__EMAIL_FIRSTNAME__ __EMAIL_LASTNAME__ " with MAIL_FROM_NAME="null null"

Solution

Use fullname keys instead of firstname/lastname
Update app settings during upgrade to handle the deprecation
- Feedback on this very welcome

PR Status

[x] Code finished and ready to be reviewed/tested
[x] 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)

rndmh3ro commented 7 months ago

!testme

yunohost-bot commented 7 months ago

May the CI gods be with you!

ericgaspar commented 7 months ago

The .env file may need to regenerated for change_url script so that .env gets updated with those modifications.

daniel-fahey commented 7 months ago

The .env file may need to regenerated for change_url script so that .env gets updated with those modifications.

Thanks @ericgaspar, yeah the change_url script currently only gets the app's settings state (i.e. the stuff stored in /etc/yunohost/apps/invoiceninja5/settings.yml) for $install_dir. It just uses ynh_replace_string() directly to replace any instance of $old_domain$old_path in $install_dir/.env. It might be better to just let ynh_add_config() use $domain, but that's for a different pull request / discussion. Changes here shouldn't break change_url anyway.

For this PR I think we could stop using the app's state to store $email nor $email_fullname and get them from the live system as needed. Please check out my new commit 973cc41c1de98446067ca35e03ee456caf028982; fewer moving parts, less state to manage.

Edits: factual corrections

daniel-fahey commented 7 months ago

@rndmh3ro please re-open, PR was closed when testing was accidentally deleted

rndmh3ro commented 7 months ago

Sorry!

daniel-fahey commented 7 months ago

No worries! Anything I can do to help get this merged?

rndmh3ro commented 7 months ago

!testme

yunohost-bot commented 7 months ago

:v:

rndmh3ro commented 7 months ago

@daniel-fahey let's check if the tests pass, then this PR should be good to go.

rndmh3ro commented 7 months ago

The mail that gets sent needs an update:

==========
Please open your invoiceninja5 domain: https://sub.domain.tld__PATH_URL__

The username is: __EMAIL__
The password is the administrator one you filled during the installation
The secret is: BfeaZziH0NKBPs9J063BARjt6ekbIVlB

Please note that if you did NOT install the application in public mode, you should go to the Yunohost login portal first to authenticate yourself in order to access to the application.
==========

Here: https://github.com/YunoHost-Apps/invoiceninja5_ynh/blob/testing/doc/POST_INSTALL.md

daniel-fahey commented 7 months ago

Thanks @rndmh3ro, also note the __PATH_URL__ doesn't work either. According to Packaging v2 I think it should just be __PATH__. For another time in another PR anyway.

rndmh3ro commented 7 months ago

Thanks!

YunoHost-Apps / invoiceninja5_ynh