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:07, Michael Köppl wrote:
> Just added a suggestion inline
> 
> 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}'),
>> +                            mailto,
>> +                        );
>> +                    } else {
>> +                        return Ext.String.format(
>> +                            gettext('On failure, use sendmail to send an email to: {0}'),
>> +                            mailto,
>> +                        );
>> +                    }
> 
> Could maybe shorten this to
> 
> 	return Ext.String.format(
> 		gettext((mailnotification === 'always' ? 'Always' : 'On failure,') + '
> use sendmail to send an email to: {0}'),
> 		mailto,
> 	);
> 
> Admittedly, opinions might differ regarding readability.
> 

The parameter to the gettext function needs to be a static string, otherwise xgettext[1,2] cannot
extract it to generate the .pot file. While the xgettext tool has some basic understanding of the
syntax of supported languages to parse these strings, it does not evaluate or execute any code.

That's also the reason why we have to do a 

Ext.String.format(gettext("... {0}"), var)

instead of a

gettext(`.... ${var}`)

Also, I'm not the biggest fan of the ternary operator, I'm not sure if your suggestion is any easier
to read and comprehend. But that might just be me :)


[1] https://man7.org/linux/man-pages/man1/xgettext.1.html
[2] https://git.proxmox.com/?p=proxmox-i18n.git;a=blob;f=Makefile;h=34859f5cfa776111b991da23e922cc5eb83cf306;hb=HEAD#l140


-- 
- Lukas



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