Re: [pve-devel] [PATCH manager v2 2/2] ui: backup job details: show notification-mode instead of legacy keys

["Michael Köppl" <m.koeppl@proxmox.com>]

On 6/24/25 13:28, Lukas Wagner wrote:
> The backup job details view was never updated after the overhaul of the
> notification system. In this commit we remove the left-over
> notification-policy/target handling and change the view so that we
> display the current configuration based on notification-mode, mailto and
> mailnotification.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> 
> Notes:
>     Changes since v1:
>       - Rebased onto latest master (PVE 9)
> 
>  www/manager6/dc/BackupJobDetail.js | 37 +++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/www/manager6/dc/BackupJobDetail.js b/www/manager6/dc/BackupJobDetail.js
> index 58cb7bef..8d10a0da 100644
> --- a/www/manager6/dc/BackupJobDetail.js
> +++ b/www/manager6/dc/BackupJobDetail.js
> @@ -206,28 +206,33 @@ Ext.define('PVE.dc.BackupInfo', {
>      column2: [
>          {
>              xtype: 'displayfield',
> -            name: 'notification-policy',
> +            name: 'notification-mode',
>              fieldLabel: gettext('Notification'),
>              renderer: function (value) {
> +                value = value ?? 'auto';
>                  let record = this.up('pveBackupInfo')?.record;
> +                let mailto = record?.mailto;
> +                let mailnotification = record?.mailnotification ?? 'always';
>  
> -                // Fall back to old value, in case this option is not migrated yet.
> -                let policy = value || record?.mailnotification || 'always';
> +                if ((value === 'auto' && mailto === undefined) || value === 'notification-system') {
> +                    return gettext('Use global notification settings');
> +                } else {
> +                    if (mailto === undefined) {
> +                        mailto = '-';
> +                    }
>  
> -                let when = gettext('Always');
> -                if (policy === 'failure') {
> -                    when = gettext('On failure only');
> -                } else if (policy === 'never') {
> -                    when = gettext('Never');
> +                    if (mailnotification === 'always') {
> +                        return Ext.String.format(
> +                            gettext('Always use sendmail to send an email to: {0}'),

Could this maybe instead be "Always send email" and then later on append
either " to: {0}" or " (no address configured)"? I compared this in the
UI to the texts used before this patch and I think the old "Always (No
target configured)" looked more polished and also gave more info than
the "-" if no addresses were configured. Also, the new text is very long
and in my browser (Firefox 139) almost reaches the end of the details
dialog. Just a suggestion, though.

> +                            mailto,
> +                        );
> +                    } else {
> +                        return Ext.String.format(
> +                            gettext('On failure, use sendmail to send an email to: {0}'),
> +                            mailto,
> +                        );
> +                    }
>                  }
> -
> -                // Notification-target takes precedence
> -                let target =
> -                    record?.['notification-target'] ||
> -                    record?.mailto ||
> -                    gettext('No target configured');
> -
> -                return `${when} (${target})`;
>              },
>          },
>          {



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel