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

[pve-devel] [PATCH pve-guest-common v3 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>
Tested-by: Michael Köppl <m.koeppl@proxmox.com>
Reviewed-by: Michael Köppl <m.koeppl@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 v3 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>
Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Michael Köppl <m.koeppl@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 v3 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 v2:
      - Instead of rendering the recipient email addresses
        in the 'Notification' field, use a separate 'Recipients'
        display field. The field is only displayed if the legacy-mode
        (or auto with mailto set) is configured.
    
      - Rephrase some strings to make the more concise. While
        the previous ones fitted within the width of the dialog,
        they were indeed quite long. Some translations might
        exceed the width of the dialog, leading to bad UX.
    
    Changes since v1:
      - Rebased onto latest master (PVE 9)

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

diff --git a/www/manager6/dc/BackupJobDetail.js b/www/manager6/dc/BackupJobDetail.js
index 58cb7bef..464bb558 100644
--- a/www/manager6/dc/BackupJobDetail.js
+++ b/www/manager6/dc/BackupJobDetail.js
@@ -165,6 +165,7 @@ Ext.define('PVE.dc.BackupInfo', {
     viewModel: {
         data: {
             retentionType: 'none',
+            hideRecipients: true,
         },
         formulas: {
             hasRetention: (get) => get('retentionType') !== 'none',
@@ -206,28 +207,37 @@ 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';
-
-                let when = gettext('Always');
-                if (policy === 'failure') {
-                    when = gettext('On failure only');
-                } else if (policy === 'never') {
-                    when = gettext('Never');
+                if ((value === 'auto' && mailto === undefined) || (value === 'notification-system')) {
+                    return gettext('Use global notification settings');
+                } else if (mailnotification === 'always') {
+                    return gettext('Always send email');
+                } else {
+                    return gettext('Send email on failure');
+                }
+            },
+        },
+        {
+            xtype: 'displayfield',
+            name: 'mailto',
+            fieldLabel: gettext('Recipients'),
+            hidden: true,
+            bind: {
+                hidden: '{hideRecipients}',
+            },
+            renderer: function (value) {
+                if (!value) {
+                    return gettext('No recipients configured');
                 }
 
-                // Notification-target takes precedence
-                let target =
-                    record?.['notification-target'] ||
-                    record?.mailto ||
-                    gettext('No target configured');
-
-                return `${when} (${target})`;
+                return value;
             },
         },
         {
@@ -382,6 +392,12 @@ Ext.define('PVE.dc.BackupInfo', {
             vm.set('retentionType', 'none');
         }
 
+        let notificationMode = values['notification-mode'] ?? 'auto';
+        let mailto = values.mailto;
+
+        let hideRecipients = (notificationMode === 'auto' && mailto === undefined) || (notificationMode === 'notification-system');
+        vm.set('hideRecipients', hideRecipients);
+
         // selection Mode depends on the presence/absence of several keys
         let selModeField = me.query('[isFormField][name=selMode]')[0];
         let selMode = 'none';
-- 
2.39.5



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

[pve-devel] applied: [PATCH manager v3 1/2] ui: remove handling of obsolete notification-policy/target settings

[Thomas Lamprecht]

On Wed, 09 Jul 2025 10:14:31 +0200, Lukas Wagner wrote:
> 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.
> 
> 

Applied, thanks!

[1/2] ui: remove handling of obsolete notification-policy/target settings
      commit: df147ed0ffea3b8220560b2d1b4f104e80a47fc0


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

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

[Thomas Lamprecht]

On Wed, 09 Jul 2025 10:14:32 +0200, 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.
> 
> 
> [...]

Applied, thanks!

[2/2] ui: backup job details: show notification-mode instead of legacy keys
      commit: 0640648bcd3d6e9a9f9757ae20b83f0ed58f5801


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

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

[Thomas Lamprecht]

On Wed, 09 Jul 2025 10:14:30 +0200, 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.
> 
> [...]

Applied, thanks!

[1/1] backup job: remove 'notification-policy' and 'notification-target' options
      (no commit info)


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

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

[Gabriel Goller]

proxmox-biome didn't like this patch :(


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

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

[Lukas Wagner]

On  2025-07-11 17:04, Gabriel Goller wrote:
> proxmox-biome didn't like this patch :(
> 
> 

   $ make check
   [....]
   Checked 327 files in 23ms. No fixes applied.


All good on my end, could you provide some more detail? What is the issue?

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

[Gabriel Goller]

On 14.07.2025 09:26, Lukas Wagner wrote:
>
>
>On  2025-07-11 17:04, Gabriel Goller wrote:
>> proxmox-biome didn't like this patch :(
>>
>>
>
>   $ make check
>   [....]
>   Checked 327 files in 23ms. No fixes applied.
>
>
>All good on my end, could you provide some more detail? What is the issue?

There is a formatting issue. `make check` only runs the linter, not the
formatter. Try `proxmox-biome check ./www/manager6`.

Make we should change the Makefile to run `proxmox-biome check`?


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

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

[Lukas Wagner]

On  2025-07-14 10:30, Gabriel Goller wrote:
> On 14.07.2025 09:26, Lukas Wagner wrote:
>>
>>
>> On  2025-07-11 17:04, Gabriel Goller wrote:
>>> proxmox-biome didn't like this patch :(
>>>
>>>
>>
>>   $ make check
>>   [....]
>>   Checked 327 files in 23ms. No fixes applied.
>>
>>
>> All good on my end, could you provide some more detail? What is the issue?
> 
> There is a formatting issue. `make check` only runs the linter, not the
> formatter. Try `proxmox-biome check ./www/manager6`.

Ah, didn't know that there was both, lint and check for biome. TIL.
I'll send a patch for the formatting fix.

> 
> Make we should change the Makefile to run `proxmox-biome check`?

Maybe, but I think a separate make target could make more sense than including it
in `make check`. The latter is also run during a package build process and it would
be quite annoying to fail that because of a trivial formatting issue. We also don't do
that for Rust, neither for clippy nor rustfmt.

What definitely would make sense is to add biome format/check --write to the `make tidy`
target, at the moment we seem to only run perltidy there.


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

[Thomas Lamprecht]

Am 14.07.25 um 10:42 schrieb Lukas Wagner:
> On  2025-07-14 10:30, Gabriel Goller wrote:

>> Make we should change the Makefile to run `proxmox-biome check`?
> 
> Maybe, but I think a separate make target could make more sense than including it
> in `make check`. The latter is also run during a package build process and it would
> be quite annoying to fail that because of a trivial formatting issue. We also don't do
> that for Rust, neither for clippy nor rustfmt.

+1

If, we should IMO only warn on code failing a formatting check.

> What definitely would make sense is to add biome format/check --write to the `make tidy`
> target, at the moment we seem to only run perltidy there.

Yeah, that's still missing. Any input on target name? I used `tidy` due to
correlation with perltidy's name, but would something else be more sensible
here? 


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

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

[Lukas Wagner]

On  2025-07-15 21:59, Thomas Lamprecht wrote:
> Am 14.07.25 um 10:42 schrieb Lukas Wagner:
>> On  2025-07-14 10:30, Gabriel Goller wrote:
> 
>>> Make we should change the Makefile to run `proxmox-biome check`?
>>
>> Maybe, but I think a separate make target could make more sense than including it
>> in `make check`. The latter is also run during a package build process and it would
>> be quite annoying to fail that because of a trivial formatting issue. We also don't do
>> that for Rust, neither for clippy nor rustfmt.
> 
> +1
> 
> If, we should IMO only warn on code failing a formatting check.
> 
>> What definitely would make sense is to add biome format/check --write to the `make tidy`
>> target, at the moment we seem to only run perltidy there.
> 
> Yeah, that's still missing. Any input on target name? I used `tidy` due to
> correlation with perltidy's name, but would something else be more sensible
> here? 

I think `make tidy` is fine. Alternatives that come to mind are `make format` or `make fmt`.
I don't have too much of an opinion here, as long as we use the same target name across all relevant
repos.

-- 
- Lukas



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