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

[Dominik Csapak]

On 4/23/24 10:14, Lukas Wagner wrote:
> 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.

but as it is now, editing an existing value will automatically convert
to the notification system if one is not careful and simply presses
ok again after opening

so having the empty state explicitly in the kvcombobox with it's
meaning would be good

> 
> 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 at proxmox.com>
>>> Tested-by: Gabriel Goller <g.goller at proxmox.com>
>>> Reviewed-by: Gabriel Goller <g.goller at 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}",
>>> +            },
>>>                },
>>>            ],
>>>    
>>
>>
>