[pve-devel] [PATCH pve-guest-common v2 1/1] backup job: remove 'notification-policy' and 'notification-target' options

[pve-devel] [PATCH pve-guest-common v2 1/1] backup job: remove 'notification-policy' and 'notification-target' options

[Lukas Wagner]

Those were only used in the first iteration of the new notification
stack, which unfortunately hit pvetest too soon. These two keys have no
effect and were proactively removed by the GUI when changing
backup job settings.

With the major update to PVE 9 these can finally be dropped. The pve8to9
script will gain a check to check for any left-over keys.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

Notes:
    Changes since v1:
      - rebase onto latest master

 src/PVE/VZDump/Common.pm | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
index 37f5509..902829f 100644
--- a/src/PVE/VZDump/Common.pm
+++ b/src/PVE/VZDump/Common.pm
@@ -266,19 +266,6 @@ my $confdesc = {
         enum => ['auto', 'legacy-sendmail', 'notification-system'],
         default => 'auto',
     },
-    'notification-policy' => {
-        type => 'string',
-        description => "Deprecated: Do not use",
-        optional => 1,
-        enum => ['always', 'failure', 'never'],
-        default => 'always',
-    },
-    'notification-target' => {
-        type => 'string',
-        format => 'pve-configid',
-        description => "Deprecated: Do not use",
-        optional => 1,
-    },
     tmpdir => {
         type => 'string',
         description => "Store temporary files to specified directory.",
-- 
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 v2 1/2] ui: remove handling of obsolete notification-policy/target settings

[Lukas Wagner]

These were only used in the 'old' revamped notification stack which was
briefly available on pvetest. With PVE 9 we can finally get completely
rid of these.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

Notes:
    Changes since v1:
      - Rebased onto latest master (PVE 9)

 www/manager6/dc/Backup.js | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 3808aa25..5eb8b5fb 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -37,14 +37,6 @@ Ext.define('PVE.dc.BackupEdit', {
                 delete values.node;
             }
 
-            // Get rid of new-old parameters for notification settings.
-            // These should only be set for those selected few who ran
-            // pve-manager from pvetest.
-            if (!isCreate) {
-                Proxmox.Utils.assemble_field_data(values, { delete: 'notification-policy' });
-                Proxmox.Utils.assemble_field_data(values, { delete: 'notification-target' });
-            }
-
             let selMode = values.selMode;
             delete values.selMode;
 
@@ -158,14 +150,6 @@ Ext.define('PVE.dc.BackupEdit', {
             let me = this;
             let viewModel = me.getViewModel();
 
-            // Migrate 'new'-old notification-policy back to old-old mailnotification.
-            // Only should affect users who used pve-manager from pvetest. This was a remnant of
-            // notifications before the  overhaul.
-            let policy = data['notification-policy'];
-            if (policy === 'always' || policy === 'failure') {
-                data.mailnotification = policy;
-            }
-
             if (data.exclude) {
                 data.vmid = data.exclude;
                 data.selMode = 'exclude';
-- 
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 v2 2/2] ui: backup job details: show notification-mode instead of legacy keys

[Lukas Wagner]

The backup job details view was never updated after the overhaul of the
notification system. In this commit we remove the left-over
notification-policy/target handling and change the view so that we
display the current configuration based on notification-mode, mailto and
mailnotification.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

Notes:
    Changes since v1:
      - Rebased onto latest master (PVE 9)

 www/manager6/dc/BackupJobDetail.js | 37 +++++++++++++++++-------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/www/manager6/dc/BackupJobDetail.js b/www/manager6/dc/BackupJobDetail.js
index 58cb7bef..8d10a0da 100644
--- a/www/manager6/dc/BackupJobDetail.js
+++ b/www/manager6/dc/BackupJobDetail.js
@@ -206,28 +206,33 @@ Ext.define('PVE.dc.BackupInfo', {
     column2: [
         {
             xtype: 'displayfield',
-            name: 'notification-policy',
+            name: 'notification-mode',
             fieldLabel: gettext('Notification'),
             renderer: function (value) {
+                value = value ?? 'auto';
                 let record = this.up('pveBackupInfo')?.record;
+                let mailto = record?.mailto;
+                let mailnotification = record?.mailnotification ?? 'always';
 
-                // Fall back to old value, in case this option is not migrated yet.
-                let policy = value || record?.mailnotification || 'always';
+                if ((value === 'auto' && mailto === undefined) || value === 'notification-system') {
+                    return gettext('Use global notification settings');
+                } else {
+                    if (mailto === undefined) {
+                        mailto = '-';
+                    }
 
-                let when = gettext('Always');
-                if (policy === 'failure') {
-                    when = gettext('On failure only');
-                } else if (policy === 'never') {
-                    when = gettext('Never');
+                    if (mailnotification === 'always') {
+                        return Ext.String.format(
+                            gettext('Always use sendmail to send an email to: {0}'),
+                            mailto,
+                        );
+                    } else {
+                        return Ext.String.format(
+                            gettext('On failure, use sendmail to send an email to: {0}'),
+                            mailto,
+                        );
+                    }
                 }
-
-                // Notification-target takes precedence
-                let target =
-                    record?.['notification-target'] ||
-                    record?.mailto ||
-                    gettext('No target configured');
-
-                return `${when} (${target})`;
             },
         },
         {
-- 
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 v2 2/2] ui: backup job details: show notification-mode instead of legacy keys

[Michael Köppl]

Just added a suggestion inline

On 6/24/25 13:28, Lukas Wagner wrote:
> The backup job details view was never updated after the overhaul of the
> notification system. In this commit we remove the left-over
> notification-policy/target handling and change the view so that we
> display the current configuration based on notification-mode, mailto and
> mailnotification.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> 
> Notes:
>     Changes since v1:
>       - Rebased onto latest master (PVE 9)
> 
>  www/manager6/dc/BackupJobDetail.js | 37 +++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/www/manager6/dc/BackupJobDetail.js b/www/manager6/dc/BackupJobDetail.js
> index 58cb7bef..8d10a0da 100644
> --- a/www/manager6/dc/BackupJobDetail.js
> +++ b/www/manager6/dc/BackupJobDetail.js
> @@ -206,28 +206,33 @@ Ext.define('PVE.dc.BackupInfo', {
>      column2: [
>          {
>              xtype: 'displayfield',
> -            name: 'notification-policy',
> +            name: 'notification-mode',
>              fieldLabel: gettext('Notification'),
>              renderer: function (value) {
> +                value = value ?? 'auto';
>                  let record = this.up('pveBackupInfo')?.record;
> +                let mailto = record?.mailto;
> +                let mailnotification = record?.mailnotification ?? 'always';
>  
> -                // Fall back to old value, in case this option is not migrated yet.
> -                let policy = value || record?.mailnotification || 'always';
> +                if ((value === 'auto' && mailto === undefined) || value === 'notification-system') {
> +                    return gettext('Use global notification settings');
> +                } else {
> +                    if (mailto === undefined) {
> +                        mailto = '-';
> +                    }
>  
> -                let when = gettext('Always');
> -                if (policy === 'failure') {
> -                    when = gettext('On failure only');
> -                } else if (policy === 'never') {
> -                    when = gettext('Never');
> +                    if (mailnotification === 'always') {
> +                        return Ext.String.format(
> +                            gettext('Always use sendmail to send an email to: {0}'),
> +                            mailto,
> +                        );
> +                    } else {
> +                        return Ext.String.format(
> +                            gettext('On failure, use sendmail to send an email to: {0}'),
> +                            mailto,
> +                        );
> +                    }

Could maybe shorten this to

	return Ext.String.format(
		gettext((mailnotification === 'always' ? 'Always' : 'On failure,') + '
use sendmail to send an email to: {0}'),
		mailto,
	);

Admittedly, opinions might differ regarding readability.


>                  }
> -
> -                // Notification-target takes precedence
> -                let target =
> -                    record?.['notification-target'] ||
> -                    record?.mailto ||
> -                    gettext('No target configured');
> -
> -                return `${when} (${target})`;
>              },
>          },
>          {



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

Re: [pve-devel] [PATCH manager v2 2/2] ui: backup job details: show notification-mode instead of legacy keys

[Lukas Wagner]

On  2025-07-08 20:07, Michael Köppl wrote:
> Just added a suggestion inline
> 
> On 6/24/25 13:28, Lukas Wagner wrote:
>> The backup job details view was never updated after the overhaul of the
>> notification system. In this commit we remove the left-over
>> notification-policy/target handling and change the view so that we
>> display the current configuration based on notification-mode, mailto and
>> mailnotification.
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>>
>> Notes:
>>     Changes since v1:
>>       - Rebased onto latest master (PVE 9)
>>
>>  www/manager6/dc/BackupJobDetail.js | 37 +++++++++++++++++-------------
>>  1 file changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/www/manager6/dc/BackupJobDetail.js b/www/manager6/dc/BackupJobDetail.js
>> index 58cb7bef..8d10a0da 100644
>> --- a/www/manager6/dc/BackupJobDetail.js
>> +++ b/www/manager6/dc/BackupJobDetail.js
>> @@ -206,28 +206,33 @@ Ext.define('PVE.dc.BackupInfo', {
>>      column2: [
>>          {
>>              xtype: 'displayfield',
>> -            name: 'notification-policy',
>> +            name: 'notification-mode',
>>              fieldLabel: gettext('Notification'),
>>              renderer: function (value) {
>> +                value = value ?? 'auto';
>>                  let record = this.up('pveBackupInfo')?.record;
>> +                let mailto = record?.mailto;
>> +                let mailnotification = record?.mailnotification ?? 'always';
>>  
>> -                // Fall back to old value, in case this option is not migrated yet.
>> -                let policy = value || record?.mailnotification || 'always';
>> +                if ((value === 'auto' && mailto === undefined) || value === 'notification-system') {
>> +                    return gettext('Use global notification settings');
>> +                } else {
>> +                    if (mailto === undefined) {
>> +                        mailto = '-';
>> +                    }
>>  
>> -                let when = gettext('Always');
>> -                if (policy === 'failure') {
>> -                    when = gettext('On failure only');
>> -                } else if (policy === 'never') {
>> -                    when = gettext('Never');
>> +                    if (mailnotification === 'always') {
>> +                        return Ext.String.format(
>> +                            gettext('Always use sendmail to send an email to: {0}'),
>> +                            mailto,
>> +                        );
>> +                    } else {
>> +                        return Ext.String.format(
>> +                            gettext('On failure, use sendmail to send an email to: {0}'),
>> +                            mailto,
>> +                        );
>> +                    }
> 
> Could maybe shorten this to
> 
> 	return Ext.String.format(
> 		gettext((mailnotification === 'always' ? 'Always' : 'On failure,') + '
> use sendmail to send an email to: {0}'),
> 		mailto,
> 	);
> 
> Admittedly, opinions might differ regarding readability.
> 

The parameter to the gettext function needs to be a static string, otherwise xgettext[1,2] cannot
extract it to generate the .pot file. While the xgettext tool has some basic understanding of the
syntax of supported languages to parse these strings, it does not evaluate or execute any code.

That's also the reason why we have to do a 

Ext.String.format(gettext("... {0}"), var)

instead of a

gettext(`.... ${var}`)

Also, I'm not the biggest fan of the ternary operator, I'm not sure if your suggestion is any easier
to read and comprehend. But that might just be me :)


[1] https://man7.org/linux/man-pages/man1/xgettext.1.html
[2] https://git.proxmox.com/?p=proxmox-i18n.git;a=blob;f=Makefile;h=34859f5cfa776111b991da23e922cc5eb83cf306;hb=HEAD#l140


-- 
- 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 v2 2/2] ui: backup job details: show notification-mode instead of legacy keys

[Michael Köppl]

On 7/9/25 09:12, Lukas Wagner wrote:
> The parameter to the gettext function needs to be a static string, otherwise xgettext[1,2] cannot
> extract it to generate the .pot file. While the xgettext tool has some basic understanding of the
> syntax of supported languages to parse these strings, it does not evaluate or execute any code.
> 
> That's also the reason why we have to do a 
> 
> Ext.String.format(gettext("... {0}"), var)
> 
> instead of a
> 
> gettext(`.... ${var}`)

I see, I hadn't considered that. Thanks for the detailed response and
the links!

> 
> Also, I'm not the biggest fan of the ternary operator, I'm not sure if your suggestion is any easier
> to read and comprehend. But that might just be me :)
> 

Yeah, you're probably right. Shortening the code should not come at the
expense of readability and thinking about it again, I prefer the way you
implemented this.

> 
> [1] https://man7.org/linux/man-pages/man1/xgettext.1.html
> [2] https://git.proxmox.com/?p=proxmox-i18n.git;a=blob;f=Makefile;h=34859f5cfa776111b991da23e922cc5eb83cf306;hb=HEAD#l140
> 
> 


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

Re: [pve-devel] [PATCH manager v2 2/2] ui: backup job details: show notification-mode instead of legacy keys

[Michael Köppl]

On 6/24/25 13:28, Lukas Wagner wrote:
> The backup job details view was never updated after the overhaul of the
> notification system. In this commit we remove the left-over
> notification-policy/target handling and change the view so that we
> display the current configuration based on notification-mode, mailto and
> mailnotification.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> 
> Notes:
>     Changes since v1:
>       - Rebased onto latest master (PVE 9)
> 
>  www/manager6/dc/BackupJobDetail.js | 37 +++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/www/manager6/dc/BackupJobDetail.js b/www/manager6/dc/BackupJobDetail.js
> index 58cb7bef..8d10a0da 100644
> --- a/www/manager6/dc/BackupJobDetail.js
> +++ b/www/manager6/dc/BackupJobDetail.js
> @@ -206,28 +206,33 @@ Ext.define('PVE.dc.BackupInfo', {
>      column2: [
>          {
>              xtype: 'displayfield',
> -            name: 'notification-policy',
> +            name: 'notification-mode',
>              fieldLabel: gettext('Notification'),
>              renderer: function (value) {
> +                value = value ?? 'auto';
>                  let record = this.up('pveBackupInfo')?.record;
> +                let mailto = record?.mailto;
> +                let mailnotification = record?.mailnotification ?? 'always';
>  
> -                // Fall back to old value, in case this option is not migrated yet.
> -                let policy = value || record?.mailnotification || 'always';
> +                if ((value === 'auto' && mailto === undefined) || value === 'notification-system') {
> +                    return gettext('Use global notification settings');
> +                } else {
> +                    if (mailto === undefined) {
> +                        mailto = '-';
> +                    }
>  
> -                let when = gettext('Always');
> -                if (policy === 'failure') {
> -                    when = gettext('On failure only');
> -                } else if (policy === 'never') {
> -                    when = gettext('Never');
> +                    if (mailnotification === 'always') {
> +                        return Ext.String.format(
> +                            gettext('Always use sendmail to send an email to: {0}'),

Could this maybe instead be "Always send email" and then later on append
either " to: {0}" or " (no address configured)"? I compared this in the
UI to the texts used before this patch and I think the old "Always (No
target configured)" looked more polished and also gave more info than
the "-" if no addresses were configured. Also, the new text is very long
and in my browser (Firefox 139) almost reaches the end of the details
dialog. Just a suggestion, though.

> +                            mailto,
> +                        );
> +                    } else {
> +                        return Ext.String.format(
> +                            gettext('On failure, use sendmail to send an email to: {0}'),
> +                            mailto,
> +                        );
> +                    }
>                  }
> -
> -                // Notification-target takes precedence
> -                let target =
> -                    record?.['notification-target'] ||
> -                    record?.mailto ||
> -                    gettext('No target configured');
> -
> -                return `${when} (${target})`;
>              },
>          },
>          {



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

Re: [pve-devel] [PATCH manager v2 2/2] ui: backup job details: show notification-mode instead of legacy keys

[Lukas Wagner]

On  2025-07-08 20:17, Michael Köppl wrote:
> On 6/24/25 13:28, Lukas Wagner wrote:
>> The backup job details view was never updated after the overhaul of the
>> notification system. In this commit we remove the left-over
>> notification-policy/target handling and change the view so that we
>> display the current configuration based on notification-mode, mailto and
>> mailnotification.
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>>
>> Notes:
>>     Changes since v1:
>>       - Rebased onto latest master (PVE 9)
>>
>>  www/manager6/dc/BackupJobDetail.js | 37 +++++++++++++++++-------------
>>  1 file changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/www/manager6/dc/BackupJobDetail.js b/www/manager6/dc/BackupJobDetail.js
>> index 58cb7bef..8d10a0da 100644
>> --- a/www/manager6/dc/BackupJobDetail.js
>> +++ b/www/manager6/dc/BackupJobDetail.js
>> @@ -206,28 +206,33 @@ Ext.define('PVE.dc.BackupInfo', {
>>      column2: [
>>          {
>>              xtype: 'displayfield',
>> -            name: 'notification-policy',
>> +            name: 'notification-mode',
>>              fieldLabel: gettext('Notification'),
>>              renderer: function (value) {
>> +                value = value ?? 'auto';
>>                  let record = this.up('pveBackupInfo')?.record;
>> +                let mailto = record?.mailto;
>> +                let mailnotification = record?.mailnotification ?? 'always';
>>  
>> -                // Fall back to old value, in case this option is not migrated yet.
>> -                let policy = value || record?.mailnotification || 'always';
>> +                if ((value === 'auto' && mailto === undefined) || value === 'notification-system') {
>> +                    return gettext('Use global notification settings');
>> +                } else {
>> +                    if (mailto === undefined) {
>> +                        mailto = '-';
>> +                    }
>>  
>> -                let when = gettext('Always');
>> -                if (policy === 'failure') {
>> -                    when = gettext('On failure only');
>> -                } else if (policy === 'never') {
>> -                    when = gettext('Never');
>> +                    if (mailnotification === 'always') {
>> +                        return Ext.String.format(
>> +                            gettext('Always use sendmail to send an email to: {0}'),
> 
> Could this maybe instead be "Always send email" and then later on append
> either " to: {0}" or " (no address configured)"? I compared this in the
> UI to the texts used before this patch and I think the old "Always (No
> target configured)" looked more polished and also gave more info than
> the "-" if no addresses were configured. Also, the new text is very long
> and in my browser (Firefox 139) almost reaches the end of the details
> dialog. Just a suggestion, though.
> 

Good point. For v3, I slightly shortened the strings to

  "Always send email"
  "Send email on failure"

Also, instead of rendering the recipient addresses in the same field by appending them to the string,
I created a second display field "Recipients", which is only visible if the legacy mode
will be used. If it is visible, but no mail address has been added

  "No recipients configured"

is now shown.

What do you think about this approach?

-- 
- 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 v2 2/2] ui: backup job details: show notification-mode instead of legacy keys

[Michael Köppl]

On 7/9/25 10:18, Lukas Wagner wrote:
> 
> Good point. For v3, I slightly shortened the strings to
> 
>   "Always send email"
>   "Send email on failure"
> 
> Also, instead of rendering the recipient addresses in the same field by appending them to the string,
> I created a second display field "Recipients", which is only visible if the legacy mode
> will be used. If it is visible, but no mail address has been added
> 
>   "No recipients configured"
> 
> is now shown.
> 
> What do you think about this approach?
> 

Just had a look at how you implemented this in v3 and I think that works
a lot better. Thanks for adapting it!


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

Re: [pve-devel] [PATCH pve-guest-common v2 1/1] backup job: remove 'notification-policy' and 'notification-target' options

[Michael Köppl]

Had a closer look at the implementation, which apart from 2 suggestions
on pve-manager 2/2 looks good.

Quickly had a look at the Backup job details dialog as well and tested
through the various combinations for notification settings. Information
for both the notification system and email notifications are displayed
as expected. I just think the text displayed ("Always use sendmail
to...") could maybe be replaced by something more concise, as noted in
more detail on the respective patch.

Please consider this:
Tested-by: Michael Köppl <m.koeppl@proxmox.com>
Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>

On 6/24/25 13:28, Lukas Wagner wrote:
> Those were only used in the first iteration of the new notification
> stack, which unfortunately hit pvetest too soon. These two keys have no
> effect and were proactively removed by the GUI when changing
> backup job settings.
> 
> With the major update to PVE 9 these can finally be dropped. The pve8to9
> script will gain a check to check for any left-over keys.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> 
> Notes:
>     Changes since v1:
>       - rebase onto latest master
> 
>  src/PVE/VZDump/Common.pm | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
> index 37f5509..902829f 100644
> --- a/src/PVE/VZDump/Common.pm
> +++ b/src/PVE/VZDump/Common.pm
> @@ -266,19 +266,6 @@ my $confdesc = {
>          enum => ['auto', 'legacy-sendmail', 'notification-system'],
>          default => 'auto',
>      },
> -    'notification-policy' => {
> -        type => 'string',
> -        description => "Deprecated: Do not use",
> -        optional => 1,
> -        enum => ['always', 'failure', 'never'],
> -        default => 'always',
> -    },
> -    'notification-target' => {
> -        type => 'string',
> -        format => 'pve-configid',
> -        description => "Deprecated: Do not use",
> -        optional => 1,
> -    },
>      tmpdir => {
>          type => 'string',
>          description => "Store temporary files to specified directory.",



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

[pve-devel] superseded: [PATCH pve-guest-common v2 1/1] backup job: remove 'notification-policy' and 'notification-target' options

[Lukas Wagner]

On  2025-07-08 20:20, Michael Köppl wrote:
> Had a closer look at the implementation, which apart from 2 suggestions
> on pve-manager 2/2 looks good.
> 
> Quickly had a look at the Backup job details dialog as well and tested
> through the various combinations for notification settings. Information
> for both the notification system and email notifications are displayed
> as expected. I just think the text displayed ("Always use sendmail
> to...") could maybe be replaced by something more concise, as noted in
> more detail on the respective patch.
> 
> Please consider this:
> Tested-by: Michael Köppl <m.koeppl@proxmox.com>
> Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
> 

Thanks a lot for the review, Michael.

I've incorporated some of your suggestions into a v3:

https://lore.proxmox.com/all/20250709081432.91868-1-l.wagner@proxmox.com/T/#t

I've added your T-b and R-b to the first two patches but not the third, since
the third one has changed significantly from v2.

-- 
- Lukas



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