[pve-devel] [PATCH manager 1/2] ui: dc: backup: improve UX for the different 'notification-mode's

[pve-devel] [PATCH manager 1/2] ui: dc: backup: improve UX for the different 'notification-mode's

[Lukas Wagner]

  - Switch order of 'mailto' and 'mailnotification' field
  - When mode is 'auto', disable 'mailtnotification' field
  - When mode is 'auto' and 'mailto' is empty, show
    hint that the notification system will be used

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 www/manager6/dc/Backup.js | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 70903bdc..780315db 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -207,13 +207,26 @@ Ext.define('PVE.dc.BackupEdit', {
 	data: {
 	    selMode: 'include',
 	    notificationMode: '__default__',
+	    mailto: '',
+	    mailNotification: '',
 	},
 
 	formulas: {
 	    poolMode: (get) => get('selMode') === 'pool',
-	    disableVMSelection: (get) => get('selMode') !== 'include' && get('selMode') !== 'exclude',
+	    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';
+	    },
+	    hintTextVisible: (get) =>
+		['auto', '__default__'].includes(get('notificationMode')) && !get('mailto'),
 	},
     },
 
@@ -325,6 +338,15 @@ Ext.define('PVE.dc.BackupEdit', {
 					value: '{notificationMode}',
 				    },
 				},
+				{
+				    xtype: 'textfield',
+				    fieldLabel: gettext('Send email to'),
+				    name: 'mailto',
+				    bind: {
+					hidden: '{!showMailtoFields}',
+					value: '{mailto}',
+				    },
+				},
 				{
 				    xtype: 'pveEmailNotificationSelector',
 				    fieldLabel: gettext('Send email'),
@@ -334,15 +356,18 @@ Ext.define('PVE.dc.BackupEdit', {
 					deleteEmpty: '{!isCreate}',
 				    },
 				    bind: {
-					disabled: '{!showMailtoFields}',
+					hidden: '{!showMailtoFields}',
+					disabled: '{!enableMailnotificationField}',
+					value: '{mailNotification}',
 				    },
 				},
 				{
-				    xtype: 'textfield',
-				    fieldLabel: gettext('Send email to'),
-				    name: 'mailto',
+				    xtype: 'displayfield',
+				    userCls: 'pmx-hint',
+				    hidden: true,
+				    value: gettext('No email configured, the notification system will be used'),
 				    bind: {
-					disabled: '{!showMailtoFields}',
+					hidden: '{!hintTextVisible}',
 				    },
 				},
 				{
-- 
2.39.2

[pve-devel] [PATCH manager 2/2] ui: one-off backup: show hint if notification-system is used

[Lukas Wagner]

When mode is 'auto' and 'mailto' is empty, show hint that the
notification system will be used.

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

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index 4418a9c7..ce679971 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -30,10 +30,27 @@ Ext.define('PVE.window.Backup', {
 	    name: 'mode',
 	});
 
+	let viewModel = new Ext.app.ViewModel({
+	    formulas: {
+		showMailtoField: (get) =>
+		    ['auto', 'legacy-sendmail'].includes(get('notificationMode')),
+		hintTextVisible: (get) => get('notificationMode') === 'auto' && !get('mailto'),
+	    },
+	    data: {
+		mailto: '',
+		notificationMode: 'auto',
+	    },
+	});
+
 	let mailtoField = Ext.create('Ext.form.field.Text', {
 	    fieldLabel: gettext('Send email to'),
 	    name: 'mailto',
 	    emptyText: Proxmox.Utils.noneText,
+	    viewModel,
+	    bind: {
+		value: '{mailto}',
+		hidden: '{!showMailtoField}',
+	    },
 	});
 
 	let notificationModeSelector = Ext.create({
@@ -46,10 +63,21 @@ Ext.define('PVE.window.Backup', {
 	    fieldLabel: gettext('Notification mode'),
 	    name: 'notification-mode',
 	    value: 'auto',
-	    listeners: {
-		change: function(field, value) {
-		    mailtoField.setDisabled(value === 'notification-system');
-		},
+	    viewModel,
+	    bind: {
+		value: '{notificationMode}',
+	    },
+	});
+
+	let notificationSystemHint = Ext.create({
+	    xtype: 'displayfield',
+	    padding: '0 0 0 5',
+	    userCls: 'pmx-hint',
+	    hidden: true,
+	    value: gettext('No email configured, the notification system will be used'),
+	    viewModel,
+	    bind: {
+		hidden: '{!hintTextVisible}',
 	    },
 	});
 
@@ -198,6 +226,7 @@ Ext.define('PVE.window.Backup', {
 		compressionSelector,
 		notificationModeSelector,
 		mailtoField,
+		notificationSystemHint,
 		removeCheckbox,
 	    ],
 	    columnB: [
-- 
2.39.2

Re: [pve-devel] [PATCH manager 1/2] ui: dc: backup: improve UX for the different 'notification-mode's

[Maximiliano Sandoval]

Lukas Wagner <l.wagner@proxmox.com> writes:

>   - Switch order of 'mailto' and 'mailnotification' field
>   - When mode is 'auto', disable 'mailtnotification' field
>   - When mode is 'auto' and 'mailto' is empty, show
>     hint that the notification system will be used
>
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>

Tested both commits, they do what they promise.

> +	let notificationSystemHint = Ext.create({
> +	    xtype: 'displayfield',
> +	    padding: '0 0 0 5',
> +	    userCls: 'pmx-hint',
> +	    hidden: true,
> +	    value: gettext('No email configured, the notification system will be used'),
> +	    viewModel,
> +	    bind: {
> +		hidden: '{!hintTextVisible}',

I think the `pmx-hint` being displayed as yellow suggests it is a
warning/error rather than a hint. I wonder if there is a better
approach, as this is certainly not a warning.

Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
--
Maximiliano

Re: [pve-devel] [PATCH manager 1/2] ui: dc: backup: improve UX for the different 'notification-mode's

[Lukas Wagner]

ping

On 11/23/23 17:09, Lukas Wagner wrote:
>    - Switch order of 'mailto' and 'mailnotification' field
>    - When mode is 'auto', disable 'mailtnotification' field
>    - When mode is 'auto' and 'mailto' is empty, show
>      hint that the notification system will be used
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>   www/manager6/dc/Backup.js | 37 +++++++++++++++++++++++++++++++------
>   1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index 70903bdc..780315db 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js
> @@ -207,13 +207,26 @@ Ext.define('PVE.dc.BackupEdit', {
>   	data: {
>   	    selMode: 'include',
>   	    notificationMode: '__default__',
> +	    mailto: '',
> +	    mailNotification: '',
>   	},
>   
>   	formulas: {
>   	    poolMode: (get) => get('selMode') === 'pool',
> -	    disableVMSelection: (get) => get('selMode') !== 'include' && get('selMode') !== 'exclude',
> +	    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';
> +	    },
> +	    hintTextVisible: (get) =>
> +		['auto', '__default__'].includes(get('notificationMode')) && !get('mailto'),
>   	},
>       },
>   
> @@ -325,6 +338,15 @@ Ext.define('PVE.dc.BackupEdit', {
>   					value: '{notificationMode}',
>   				    },
>   				},
> +				{
> +				    xtype: 'textfield',
> +				    fieldLabel: gettext('Send email to'),
> +				    name: 'mailto',
> +				    bind: {
> +					hidden: '{!showMailtoFields}',
> +					value: '{mailto}',
> +				    },
> +				},
>   				{
>   				    xtype: 'pveEmailNotificationSelector',
>   				    fieldLabel: gettext('Send email'),
> @@ -334,15 +356,18 @@ Ext.define('PVE.dc.BackupEdit', {
>   					deleteEmpty: '{!isCreate}',
>   				    },
>   				    bind: {
> -					disabled: '{!showMailtoFields}',
> +					hidden: '{!showMailtoFields}',
> +					disabled: '{!enableMailnotificationField}',
> +					value: '{mailNotification}',
>   				    },
>   				},
>   				{
> -				    xtype: 'textfield',
> -				    fieldLabel: gettext('Send email to'),
> -				    name: 'mailto',
> +				    xtype: 'displayfield',
> +				    userCls: 'pmx-hint',
> +				    hidden: true,
> +				    value: gettext('No email configured, the notification system will be used'),
>   				    bind: {
> -					disabled: '{!showMailtoFields}',
> +					hidden: '{!hintTextVisible}',
>   				    },
>   				},
>   				{

-- 
- Lukas

Re: [pve-devel] [PATCH manager 1/2] ui: dc: backup: improve UX for the different 'notification-mode's

[Lukas Wagner]

ping 🙂 still applies on latest master. Also tested it again to make sure
that no other changes interfered with it in the meanwhile.

On 1/8/24 11:43, Lukas Wagner wrote:
> ping
> 
> On 11/23/23 17:09, Lukas Wagner wrote:
>>    - Switch order of 'mailto' and 'mailnotification' field
>>    - When mode is 'auto', disable 'mailtnotification' field
>>    - When mode is 'auto' and 'mailto' is empty, show
>>      hint that the notification system will be used
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>


-- 
- Lukas

Re: [pve-devel] [PATCH manager 1/2] ui: dc: backup: improve UX for the different 'notification-mode's

[Maximiliano Sandoval]

Lukas Wagner <l.wagner@proxmox.com> writes:

>   - Switch order of 'mailto' and 'mailnotification' field
>   - When mode is 'auto', disable 'mailtnotification' field
>   - When mode is 'auto' and 'mailto' is empty, show
>     hint that the notification system will be used
>
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---

ping 👻

--
Maximiliano

Re: [pve-devel] [PATCH manager 1/2] ui: dc: backup: improve UX for the different 'notification-mode's

[Thomas Lamprecht]

Am 14/12/2023 um 11:26 schrieb Maximiliano Sandoval:
> 
> Lukas Wagner <l.wagner@proxmox.com> writes:
> 
>>   - Switch order of 'mailto' and 'mailnotification' field
>>   - When mode is 'auto', disable 'mailtnotification' field
>>   - When mode is 'auto' and 'mailto' is empty, show
>>     hint that the notification system will be used
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> 
> Tested both commits, they do what they promise.
> 
>> +	let notificationSystemHint = Ext.create({
>> +	    xtype: 'displayfield',
>> +	    padding: '0 0 0 5',
>> +	    userCls: 'pmx-hint',
>> +	    hidden: true,
>> +	    value: gettext('No email configured, the notification system will be used'),
>> +	    viewModel,
>> +	    bind: {
>> +		hidden: '{!hintTextVisible}',
> 
> I think the `pmx-hint` being displayed as yellow suggests it is a
> warning/error rather than a hint. I wonder if there is a better
> approach, as this is certainly not a warning.

Yeah, the hint is even often used as warning, having something more like a
"notice" or well, actual "hint" level, that isn't as flashy, might be a
good idea in the long run..

For now, we could also set a 'Hint' label and then add the pmx-hint class only
to the labelClsExtra config?

But I'm fine with this as is if you, Lukas, think that it's warranted (and
maybe even not showed that often in practice anyway).

Re: [pve-devel] [PATCH manager 1/2] ui: dc: backup: improve UX for the different 'notification-mode's

[Lukas Wagner]

On  2024-04-11 09:44, Thomas Lamprecht wrote:
>> Tested both commits, they do what they promise.
>>
>>> +	let notificationSystemHint = Ext.create({
>>> +	    xtype: 'displayfield',
>>> +	    padding: '0 0 0 5',
>>> +	    userCls: 'pmx-hint',
>>> +	    hidden: true,
>>> +	    value: gettext('No email configured, the notification system will be used'),
>>> +	    viewModel,
>>> +	    bind: {
>>> +		hidden: '{!hintTextVisible}',
>>
>> I think the `pmx-hint` being displayed as yellow suggests it is a
>> warning/error rather than a hint. I wonder if there is a better
>> approach, as this is certainly not a warning.
> 
> Yeah, the hint is even often used as warning, having something more like a
> "notice" or well, actual "hint" level, that isn't as flashy, might be a
> good idea in the long run..
> 
> For now, we could also set a 'Hint' label and then add the pmx-hint class only
> to the labelClsExtra config?
> 
> But I'm fine with this as is if you, Lukas, think that it's warranted (and
> maybe even not showed that often in practice anyway).

Actually this is visible when creating new backup jobs, because 'Notification mode' is 
set to 'Default (auto)' and there is no email entered. So yeah, a more gentle 
hint would be better. That being said, I'd like to get this merged ASAP to
resolve pontential for confusion for our users[1].

I would suggest to merge the variant with 'pmx-hint' (I'll send a v2 though, because
I fixed a minor issue just now when reevaluating this) and then revisit
this soonish to introduce a more hinty, less warning CSS class. There are other places where
that one could be useful (e.g. second page of the same dialog - Retention, or Rentention
settings for storage plugins), so we could fix them all in one go in a followup.


[1] https://forum.proxmox.com/threads/pve-sending-email-notifications-on-successful-backup-jobs.136768/post-635769
-- 
- Lukas