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

[Lukas Wagner <l.wagner@proxmox.com>]

On  2025-07-08 20:17, Michael Köppl wrote:
> 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.
> 

Good point. For v3, I slightly shortened the strings to

  "Always send email"
  "Send email on failure"

Also, instead of rendering the recipient addresses in the same field by appending them to the string,
I created a second display field "Recipients", which is only visible if the legacy mode
will be used. If it is visible, but no mail address has been added

  "No recipients configured"

is now shown.

What do you think about this approach?

-- 
- Lukas



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