Re: [pbs-devel] [PATCH proxmox-backup v4 28/43] ui: tape backup job: add selector for notification-mode

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

Thanks for the feedback, greatly appreciated!

On  2024-04-22 16:32, Dominik Csapak wrote:
> question
> 
> do we really want to add
> ---
> notification-mode notification-system
> ---
> 
> to every new job config/api call?
> 
> since it seems to be the default when none is set, couldn't we
> just leave it out of the config (at least when setting from the gui?)

The default for this option is 'sendmail-legacy' as to not change behavior for 
existing datastores. I do not feel comfortable switching
over existing datastores from the old system (configured by 'notify-user' and 'notify' as
a per-datastore setting) to the new automatically (that would imply automatically
generating targets/matchers)

For new datastores created via the UI we opt in by default.

With the next major release, we could switch the default or even remove
the legacy behavior entirely.

Side note: In PVE we have this whole 'Auto' mode, that uses the legacy
system if an email is configured for a backup job, and the new system if not.
That created quite a bit confusion for our users, so I wanted to keep it
simpler in PBS.

> 
> comment inline:
> 
> both the question and comment is valid for the next patches too
> 
> On 4/22/24 14:38, Lukas Wagner wrote:
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> Tested-by: Gabriel Goller <g.goller@proxmox.com>
>> Reviewed-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>   www/tape/window/TapeBackupJob.js | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/www/tape/window/TapeBackupJob.js b/www/tape/window/TapeBackupJob.js
>> index abbbaa0b..309dda0b 100644
>> --- a/www/tape/window/TapeBackupJob.js
>> +++ b/www/tape/window/TapeBackupJob.js
>> @@ -46,6 +46,15 @@ Ext.define('PBS.TapeManagement.BackupJobEdit', {
>>       },
>>       },
>>   +    viewModel: {
>> +    data: {
>> +        notificationMode: 'notification-system',
>> +    },
>> +    formulas: {
>> +        notificationSystemSelected: (get) => get('notificationMode') === 'notification-system',
>> +    },
>> +    },
>> +
>>       items: {
>>       xtype: 'tabpanel',
>>       bodyPadding: 10,
>> @@ -109,6 +118,18 @@ Ext.define('PBS.TapeManagement.BackupJobEdit', {
>>               fieldLabel: gettext('Drive'),
>>               name: 'drive',
>>               },
>> +            {
>> +            xtype: 'proxmoxKVComboBox',
>> +            comboItems: [
>> +                ['legacy-sendmail', gettext('Email (legacy)')],
>> +                ['notification-system', gettext('Notification system')],
>> +            ],
>> +            fieldLabel: gettext('Notification mode'),
>> +            name: 'notification-mode',
>> +            bind: {
>> +                value: '{notificationMode}',
>> +            },
> 
> if you do this, an edit on an entry where no value is set will
> enable the 'reset' button and reset the field to the empty value
> 
> normally we use '__default__' to represent this default value
> (and set it by default too)

Okay, thanks! Will introduce a __default__ choice here.
> 
>> +            },
>>               {
>>               xtype: 'pmxUserSelector',
>>               name: 'notify-user',
>> @@ -117,6 +138,9 @@ Ext.define('PBS.TapeManagement.BackupJobEdit', {
>>               allowBlank: true,
>>               value: null,
>>               renderer: Ext.String.htmlEncode,
>> +            bind: {
>> +                disabled: "{notificationSystemSelected}",
>> +            },
>>               },
>>           ],
>>   
> 
> 

-- 
- Lukas


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