Thanks a lot for testing and the review, Michael.
On 2025-06-13 15:51, Michael Köppl wrote:
> Tested both creating a scheduled backup job and creating a backup of a
> single guest. For both cases I tested:
> - using the global settings
> - using sendemail with specific email addresses
> Both worked as expected and I think UI-wise the tab makes more sense and
> unclutters the General tab a bit.
>
> Additionally, I set the notification mode to 'Auto' before applying the
> patch and checked that the mapping is applied correctly in the new tab,
> i.e.:
> - if an email was entered in addition to auto mode, the Notifications
> tab correctly selected sendemail mode with the email address
> - if no email was entered, the Notifications tab correctly selected the
> global notification settings
>
> Consider this:
> Tested-by: Michael Köppl <m.koe...@proxmox.com>
>
> The remaining comments are mostly suggestions, so with the Recipient
> typo addressed, also consider this:
> Reviewed-by: Michael Köppl <m.koe...@proxmox.com>
>
> On 6/11/25 14:59, Lukas Wagner wrote:
>> The notification settings in the 'General' tab were unfortunately a
>> source of regular confusion for many people. This was primarily due to
>> the behavior of the 'motification mode'. The notification mode can
>
> nit: s/motification/notification
Fixed, thanks!
>
>> one of the following:
>> - notification-system: Emit a notification event to the global
>> notification system, where it can be matched on by notification
>> matchers and then sent to one or more targets.
>> - legacy-sendmail: Old-style notifications, where one can directly
>> enter some email address. The system uses 'sendmail' to
>> send the notification to the specified address, circumventing
>> the regular notification stack.
>> - auto: Use legacy-sendmail if an email is entered and the
>> notification system if not
>>
>> The behavior of 'auto' is quite surprising for many users, and therefore
>> we remove it from the UI altogether.
>
> nit: I think this can be rephrased as "Remove the 'Auto' option from the
> notification mode selection, since the behavior is quite surprising for
> many users" to avoid using "we" and imperatively state the effect of the
> change first and some rationale for the change after.
>
Rephrased in v2, thank you!
>>
>> In the new 'Notifications' tab one can now choose between
>> ( ) Use global notification settings
>> (x) Use sendmail to send an email
>> Recipients: [ ]
>> When: [Always/On Error]
>>
>> 'Recipients' and 'When' are disabled if the first radio box is selected.
>>
>> The new tab can later also be used to house other controls. For example,
>> we could display all matchers that could potentially match this backup
>> job, or maybe even allow to create a new matcher with a pre-populated
>> match-field rule.>
>> We also stop using the term 'Notification System' altogether in the UI.
>> It is not necessarily clear to a user that this refers to the settings
>> in Datacenter > Notifications.
>>
>> Signed-off-by: Lukas Wagner <l.wag...@proxmox.com>
>> ---
>> www/manager6/Makefile | 1 +
>> www/manager6/dc/Backup.js | 65 ++---------
>> .../panel/BackupNotificationOptions.js | 103 ++++++++++++++++++
>> 3 files changed, 111 insertions(+), 58 deletions(-)
>> create mode 100644 www/manager6/panel/BackupNotificationOptions.js
>>
>> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
>> index fdf0e816..5eb17edb 100644
>> --- a/www/manager6/Makefile
>> +++ b/www/manager6/Makefile
>> @@ -100,6 +100,7 @@ JSSRC=
>> \
>> grid/ResourceGrid.js \
>> panel/ConfigPanel.js \
>> panel/BackupAdvancedOptions.js \
>> + panel/BackupNotificationOptions.js \
>> panel/BackupJobPrune.js \
>> panel/HealthWidget.js \
>> panel/IPSet.js \
>> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
>> index 381402ca..f861ed3d 100644
>> --- a/www/manager6/dc/Backup.js
>> +++ b/www/manager6/dc/Backup.js
>> @@ -224,24 +224,12 @@ Ext.define('PVE.dc.BackupEdit', {
>> viewModel: {
>> data: {
>> selMode: 'include',
>> - notificationMode: '__default__',
>> - mailto: '',
>> - mailNotification: 'always',
>> },
>>
>> formulas: {
>> poolMode: (get) => get('selMode') === 'pool',
>> disableVMSelection: (get) => get('selMode') !== 'include' &&
>> get('selMode') !== 'exclude',
>> - showMailtoFields: (get) =>
>> - ['auto', 'legacy-sendmail',
>> '__default__'].includes(get('notificationMode')),
>> - enableMailnotificationField: (get) => {
>> - let mode = get('notificationMode');
>> - let mailto = get('mailto');
>> -
>> - return (['auto', '__default__'].includes(mode) && mailto) ||
>> - mode === 'legacy-sendmail';
>> - },
>> },
>> },
>>
>> @@ -331,52 +319,6 @@ Ext.define('PVE.dc.BackupEdit', {
>> },
>> ],
>> column2: [
>> - {
>> - xtype: 'proxmoxKVComboBox',
>> - comboItems: [
>> - [
>> - '__default__',
>> - Ext.String.format(
>> - gettext('{0} (Auto)'),
>> Proxmox.Utils.defaultText,
>> - ),
>> - ],
>> - ['auto', gettext('Auto')],
>> - ['legacy-sendmail', gettext('Email
>> (legacy)')],
>> - ['notification-system',
>> gettext('Notification system')],
>> - ],
>> - fieldLabel: gettext('Notification mode'),
>> - name: 'notification-mode',
>> - value: '__default__',
>> - cbind: {
>> - deleteEmpty: '{!isCreate}',
>> - },
>> - bind: {
>> - value: '{notificationMode}',
>> - },
>> - },
>> - {
>> - xtype: 'textfield',
>> - fieldLabel: gettext('Send email to'),
>> - name: 'mailto',
>> - bind: {
>> - hidden: '{!showMailtoFields}',
>> - value: '{mailto}',
>> - },
>> - },
>> - {
>> - xtype: 'pveEmailNotificationSelector',
>> - fieldLabel: gettext('Send email'),
>> - name: 'mailnotification',
>> - cbind: {
>> - value: (get) => get('isCreate') ?
>> 'always' : '',
>> - deleteEmpty: '{!isCreate}',
>> - },
>> - bind: {
>> - hidden: '{!showMailtoFields}',
>> - disabled:
>> '{!enableMailnotificationField}',
>> - value: '{mailNotification}',
>> - },
>> - },
>> {
>> xtype: 'pveBackupCompressionSelector',
>> reference: 'compressionSelector',
>> @@ -439,6 +381,13 @@ Ext.define('PVE.dc.BackupEdit', {
>> },
>> ],
>> },
>> + {
>> + xtype: 'pveBackupNotificationOptionsPanel',
>> + title: gettext('Notifications'),
>> + cbind: {
>> + isCreate: '{isCreate}',
>> + },
>> + },
>> {
>> xtype: 'pveBackupJobPrunePanel',
>> title: gettext('Retention'),
>> diff --git a/www/manager6/panel/BackupNotificationOptions.js
>> b/www/manager6/panel/BackupNotificationOptions.js
>> new file mode 100644
>> index 00000000..f0b2bf33
>> --- /dev/null
>> +++ b/www/manager6/panel/BackupNotificationOptions.js
>> @@ -0,0 +1,103 @@
>> +/*
>> + * Input panel for notification options of backup jobs.
>> + */
>> +Ext.define('PVE.panel.BackupNotificationOptions', {
>> + extend: 'Proxmox.panel.InputPanel',
>> + xtype: 'pveBackupNotificationOptionsPanel',
>> + mixins: ['Proxmox.Mixin.CBind'],
>> +
>> + onlineHelp: 'chapter_notifications',
>> +
>> + cbindData: function() {
>> + let me = this;
>> + me.isCreate = !!me.isCreate;
>> + return {};
>> + },
>> +
>> + viewModel: {
>> + data: {
>> + notificationMode: undefined,
>> + },
>> + formulas: {
>> + showMailtoFields: (get) => {
>> + let mode = get('notificationMode');
>> + return mode['notification-mode'] === 'legacy-sendmail';
>> + },
>> + },
>> + },
>> +
>> + onSetValues: function(values) {
>> + let me = this;
>> +
>> + let mode = values['notification-mode'] ?? 'auto';
>> + let mailto = values.mailto;
>> +
>> + let mappedMode = 'legacy-sendmail';
>> +
>> + // The 'auto' mode is a bit annoying and confusing, so we try
>> + // to map it to the equivalent behavior.
>> + if ((mode === 'auto' && !mailto) || mode === 'notification-system') {
>> + mappedMode = 'notification-system';
>> + }
>> +
>> + me.getViewModel().set('notificationMode', { 'notification-mode':
>> mappedMode });
>> +
>> + values['notification-mode'] = mappedMode;
>> + return values;
>> + },
>> +
>> + items: [
>> + {
>> + xtype: 'radiogroup',
>> + height: '15px',
>> + layout: {
>> + type: 'vbox',
>> + },
>> + bind: {
>> + value: '{notificationMode}',
>> + },
>> + items: [
>> + {
>> + xtype: 'radiofield',
>> + name: 'notification-mode',
>> + inputValue: 'notification-system',
>> + boxLabel: 'Use global notification settings',
>> + cbind: {
>> + checked: '{isCreate}',
>> + },
>> + },
>> + {
>> + xtype: 'radiofield',
>> + name: 'notification-mode',
>> + inputValue: 'legacy-sendmail',
>> + boxLabel: 'Use sendmail to send an email',
>> + },
>> + ],
>> + },
>> + {
>> + xtype: 'textfield',
>> + fieldLabel: gettext('Recipents'),
>
> s/Recipents/Recipients
Fixed, thank you!
>
>> + emptyText: 't...@example.com, ...',
>> + name: 'mailto',
>> + padding: '0 0 0 50',
>> + disabled: true,
>> + bind: {
>> + disabled: '{!showMailtoFields}',
>> + },
>
> In my tests I was able to create a backup job with "Use sendmail to send
> an email" but with this field left empty. I'm aware that this is the
> current behavior as well, but UX-wise I think it would make sense to
> make this field mandatory if sendmail is used. Or is this to allow users
> to not send anything at all for that specific backup job? If so, maybe a
> radio button "Off" would make this more clear UX-wise?
>
I can definitely see your point, thanks for the suggestion.
In my original plans for the overhaul I planned to include a "Do not send a
notification"
radio toggle as well, doing exactly what you described. However, during
development
I realized that this becomes very very awkward with any node-local fallback
values for
mailto in /etc/vzdump.conf. Without introducing a new notifcation-mode 'none'
(or similar),
'no notifications' boils down to:
notification-mode: legacy-sendmail
(no mailto setting)
Now, when 'mailto' is set via /etc/vzdump.conf, this would still mean that an
email would be
sent, even though "Don't send" is selected in the UI. This is due to the value
from /etc/vzdump.conf being used as a fallback for any value that is not
explicitly set for a backup job.
Hope this makes it somewhat clear :)
--
- Lukas
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
