[pve-devel] [PATCH manager 1/2] ui: backup job edit: move notification related settings to separate tab

[pve-devel] [PATCH manager 1/2] ui: backup job edit: move notification related settings to separate tab

[Lukas Wagner]

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
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.

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.wagner@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'),
+	    emptyText: 'test@example.com, ...',
+	    name: 'mailto',
+	    padding: '0 0 0 50',
+	    disabled: true,
+	    bind: {
+		disabled: '{!showMailtoFields}',
+	    },
+	},
+	{
+	    xtype: 'pveEmailNotificationSelector',
+	    fieldLabel: gettext('When'),
+	    name: 'mailnotification',
+	    padding: '0 0 0 50',
+	    disabled: true,
+	    value: 'always',
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	    bind: {
+		disabled: '{!showMailtoFields}',
+	    },
+	},
+    ],
+});
-- 
2.39.5



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

[pve-devel] [PATCH manager 2/2] ui: one-shot backup: remove 'auto' notification mode for clarity

[Lukas Wagner]

The 'auto' mode does not really add any functionality but only adds
confusion about what it actually does, so we completely remove it from
the UI. It is still supported by the backend, but in the UI we map it to
a concrete mode (either notification-system or legacy-sendmail,
depending on whether mailto is set).

We also stop using the term 'notification system' in the UI, instead it
is called "Global Notification Settings".

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 www/manager6/window/Backup.js | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index 70b51409..d80a6e85 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -33,22 +33,22 @@ Ext.define('PVE.window.Backup', {
 	let mailtoField = Ext.create('Ext.form.field.Text', {
 	    fieldLabel: gettext('Send email to'),
 	    name: 'mailto',
+	    hidden: true,
 	    emptyText: Proxmox.Utils.noneText,
 	});
 
 	let notificationModeSelector = Ext.create({
 	    xtype: 'proxmoxKVComboBox',
 	    comboItems: [
-		['auto', gettext('Auto')],
-		['legacy-sendmail', gettext('Email (legacy)')],
-		['notification-system', gettext('Notification system')],
+		['notification-system', gettext('Use global settings')],
+		['legacy-sendmail', gettext('Use sendmail')],
 	    ],
-	    fieldLabel: gettext('Notification mode'),
+	    fieldLabel: gettext('Notification'),
 	    name: 'notification-mode',
-	    value: 'auto',
+	    value: 'notification-system',
 	    listeners: {
 		change: function(field, value) {
-		    mailtoField.setDisabled(value === 'notification-system');
+		    mailtoField.setHidden(value === 'notification-system');
 		},
 	    },
 	});
@@ -170,11 +170,21 @@ Ext.define('PVE.window.Backup', {
 			success: function(response, opts) {
 			    const data = response.result.data;
 
-			    if (!initialDefaults && data.mailto !== undefined) {
-				mailtoField.setValue(data.mailto);
-			    }
-			    if (!initialDefaults && data['notification-mode'] !== undefined) {
-				notificationModeSelector.setValue(data['notification-mode']);
+			    if (!initialDefaults) {
+				let notificationMode = data['notification-mode'] ?? 'auto';
+				let mailto = data.mailto;
+
+				if (notificationMode === 'auto' && mailto !== undefined) {
+				    notificationMode = 'legacy-sendmail';
+				}
+				if (notificationMode === 'auto' && mailto === undefined) {
+				    notificationMode = 'notification-system';
+				}
+
+				notificationModeSelector.setValue(notificationMode);
+				if (mailto !== undefined) {
+				    mailtoField.setValue(mailto);
+				}
 			    }
 			    if (!initialDefaults && data.mode !== undefined) {
 				modeSelector.setValue(data.mode);
-- 
2.39.5



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

Re: [pve-devel] [PATCH manager 1/2] ui: backup job edit: move notification related settings to separate tab

[Michael Köppl]

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.koeppl@proxmox.com>

The remaining comments are mostly suggestions, so with the Recipient
typo addressed, also consider this:
Reviewed-by: Michael Köppl <m.koeppl@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

> 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.

> 
> 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.wagner@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

> +	    emptyText: 'test@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?

> +	},
> +	{
> +	    xtype: 'pveEmailNotificationSelector',
> +	    fieldLabel: gettext('When'),
> +	    name: 'mailnotification',
> +	    padding: '0 0 0 50',
> +	    disabled: true,
> +	    value: 'always',
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	    bind: {
> +		disabled: '{!showMailtoFields}',
> +	    },
> +	},
> +    ],
> +});



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

Re: [pve-devel] [PATCH manager 2/2] ui: one-shot backup: remove 'auto' notification mode for clarity

[Michael Köppl]

On 6/11/25 14:59, Lukas Wagner wrote:
> The 'auto' mode does not really add any functionality but only adds
> confusion about what it actually does, so we completely remove it from
> the UI. It is still supported by the backend, but in the UI we map it to
> a concrete mode (either notification-system or legacy-sendmail,
> depending on whether mailto is set).
> 
> We also stop using the term 'notification system' in the UI, instead it
> is called "Global Notification Settings".
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  www/manager6/window/Backup.js | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
> index 70b51409..d80a6e85 100644
> --- a/www/manager6/window/Backup.js
> +++ b/www/manager6/window/Backup.js
> @@ -33,22 +33,22 @@ Ext.define('PVE.window.Backup', {
>  	let mailtoField = Ext.create('Ext.form.field.Text', {
>  	    fieldLabel: gettext('Send email to'),
>  	    name: 'mailto',
> +	    hidden: true,
>  	    emptyText: Proxmox.Utils.noneText,
>  	});
>  
>  	let notificationModeSelector = Ext.create({
>  	    xtype: 'proxmoxKVComboBox',
>  	    comboItems: [
> -		['auto', gettext('Auto')],
> -		['legacy-sendmail', gettext('Email (legacy)')],
> -		['notification-system', gettext('Notification system')],
> +		['notification-system', gettext('Use global settings')],
> +		['legacy-sendmail', gettext('Use sendmail')],
>  	    ],
> -	    fieldLabel: gettext('Notification mode'),
> +	    fieldLabel: gettext('Notification'),
>  	    name: 'notification-mode',
> -	    value: 'auto',
> +	    value: 'notification-system',
>  	    listeners: {
>  		change: function(field, value) {
> -		    mailtoField.setDisabled(value === 'notification-system');
> +		    mailtoField.setHidden(value === 'notification-system');

While this does work, I think I'd prefer it if the field is always
visible and is enabled or disabled depending on the selection of the
notification system. With the hidden field, UI shifts a tiny bit once it
becomes visible (Firefox 139.0.1). Since the Backup window is not super
cluttered, I think this additional field could always be visible, but
that's just my two cents.

>  		},
>  	    },
>  	});
> @@ -170,11 +170,21 @@ Ext.define('PVE.window.Backup', {
>  			success: function(response, opts) {
>  			    const data = response.result.data;
>  
> -			    if (!initialDefaults && data.mailto !== undefined) {
> -				mailtoField.setValue(data.mailto);
> -			    }
> -			    if (!initialDefaults && data['notification-mode'] !== undefined) {
> -				notificationModeSelector.setValue(data['notification-mode']);
> +			    if (!initialDefaults) {
> +				let notificationMode = data['notification-mode'] ?? 'auto';
> +				let mailto = data.mailto;
> +
> +				if (notificationMode === 'auto' && mailto !== undefined) {
> +				    notificationMode = 'legacy-sendmail';
> +				}
> +				if (notificationMode === 'auto' && mailto === undefined) {
> +				    notificationMode = 'notification-system';
> +				}
> +
> +				notificationModeSelector.setValue(notificationMode);
> +				if (mailto !== undefined) {
> +				    mailtoField.setValue(mailto);
> +				}
>  			    }
>  			    if (!initialDefaults && data.mode !== undefined) {
>  				modeSelector.setValue(data.mode);



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

Re: [pve-devel] [PATCH manager 2/2] ui: one-shot backup: remove 'auto' notification mode for clarity

[Lukas Wagner]

Thanks for testing and the review!

On  2025-06-13 15:51, Michael Köppl wrote:
>> --- a/www/manager6/window/Backup.js
>> +++ b/www/manager6/window/Backup.js
>> @@ -33,22 +33,22 @@ Ext.define('PVE.window.Backup', {
>>  	let mailtoField = Ext.create('Ext.form.field.Text', {
>>  	    fieldLabel: gettext('Send email to'),
>>  	    name: 'mailto',
>> +	    hidden: true,
>>  	    emptyText: Proxmox.Utils.noneText,
>>  	});
>>  
>>  	let notificationModeSelector = Ext.create({
>>  	    xtype: 'proxmoxKVComboBox',
>>  	    comboItems: [
>> -		['auto', gettext('Auto')],
>> -		['legacy-sendmail', gettext('Email (legacy)')],
>> -		['notification-system', gettext('Notification system')],
>> +		['notification-system', gettext('Use global settings')],
>> +		['legacy-sendmail', gettext('Use sendmail')],
>>  	    ],
>> -	    fieldLabel: gettext('Notification mode'),
>> +	    fieldLabel: gettext('Notification'),
>>  	    name: 'notification-mode',
>> -	    value: 'auto',
>> +	    value: 'notification-system',
>>  	    listeners: {
>>  		change: function(field, value) {
>> -		    mailtoField.setDisabled(value === 'notification-system');
>> +		    mailtoField.setHidden(value === 'notification-system');
> 
> While this does work, I think I'd prefer it if the field is always
> visible and is enabled or disabled depending on the selection of the
> notification system. With the hidden field, UI shifts a tiny bit once it
> becomes visible (Firefox 139.0.1). Since the Backup window is not super
> cluttered, I think this additional field could always be visible, but
> that's just my two cents.
> 

In my first draft I left the field in the 'disabled' state, but later I realized that this
can become misleading in some rare cases. The fields in this dialog are pre-filled with
the values from /etc/vzdump.conf. If there is a setting for 'mailto' in this file,
this field in the UI will contain the configured email address, even if the field is
disabled (which is the case when "Use global settings" is selected).
I think some users could interpret this is a "The global settings will send to this address" -
for this reason I opted to hide the field.

Admittedly, this is somewhat of an edge case, but I think I'd prefer to keep
hiding the field. Regarding the small shift of UI elements I'll include a fix
in v2.

-- 
- Lukas



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

Re: [pve-devel] [PATCH manager 1/2] ui: backup job edit: move notification related settings to separate tab

[Lukas Wagner]

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.koeppl@proxmox.com>
> 
> The remaining comments are mostly suggestions, so with the Recipient
> typo addressed, also consider this:
> Reviewed-by: Michael Köppl <m.koeppl@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.wagner@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: 'test@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

[pve-devel] superseded: [PATCH manager 1/2] ui: backup job edit: move notification related settings to separate tab

[Lukas Wagner]

Superseded by v2:
https://lore.proxmox.com/all/20250617084448.87536-1-l.wagner@proxmox.com/T/#t

-- 
- Lukas



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