[pve-devel] [PATCH docs/manager/widget-toolkit v5 00/16] notifications: notification metadata matching improvements

[pve-devel] [PATCH docs/manager/widget-toolkit v5 00/16] notifications: notification metadata matching improvements

[Lukas Wagner]

This patch series attempts to improve the user experience when creating
notification matchers.

Some of the noteworthy changes:
  - Fixup inconsistent 'hostname' field. Some notification events sent
  the hostname including a domain, while other did not.
  This series unifies the behavior, now the field only includes the hostname
  without a domain. Also updated the docs to reflect this change.
  - Allow setting a custom backup job ID, similar how we handle it for
  sync/prune jobs in PBS (to allow recognizable names used in matchers)
  - Adding columns for backup job ID/replication job ID in the UI
  - New metadata fields:
    - backup-job: ID of the backup job (set for backups, unless they are 'ad-hoc' backups)
    - replication-job: ID of the replication job
  - Add an API that enumerates known notification metadata fields/values
  - Suggest known fields/values in match rule window
  - Some code clean up for match rule edit window
  - Extended the 'exact' match-field mode - it now allows setting multiple
    allowed values, separated via ',':
      e.g. `match-field exact:type=replication,fencing
    Originally, I created a separate 'list' match type for this, but
    since the semantics for a list with one value and 'exact' mode
    are identical, I decided to just extend 'exact'.
    This is a safe change since there are are no values where a ','
    makes any sense (config IDs, hostnames)

Inter-Dependencies:
  - the widget-toolkit dep in pve-manager needs to be bumped
    to at least 4.1.4
    (we need "utils: add mechanism to add and override translatable notification event
    descriptions in the product specific UIs", otherwise the UI breaks
    with the pve-manager patches applied)
  - widget-toolkit relies on a new API endpoint provided by pve-manager
  - the changes in the backend for matching multiple values in "exact"
    mode have already been rolled out as of libpve-rs-perl 0.8.8
    --> this means that libpve-notify-perl (which pulls in libpve-rs-perl)
    must depend on libpve-rs-perl 0.8.8 at minimum - otherwise
    the notification stack cannot understand the comma-separated
    array of matched values

Changelog:
  - v5:
    - Rebased onto latest master, resolving some small conflict
  - v4:
    - widget-toolkit: break out changes for the utils module so that they
      can be applied ahead of time to ease dep bumping
    - don't show Job IDs in the backup/replication job columns
  - v3:
    - Drop already applied patches for `proxmox`
    - Rebase onto latest master - minor conflict resolution was needed
  - v2:
    - include 'type' metadata field for forwarded mails
      --> otherwise it's not possible to match them
    - include Maximilliano's T-b trailer in UI patches

pve-manager:

Lukas Wagner (9):
  api: notifications: add 'smtp' to target index
  api: jobs: vzdump: pass job 'id' parameter
  ui: dc: backup: send 'id' property when starting a backup job manually
  ui: dc: backup: allow to set custom job id in  advanced settings
  api: replication: add 'replication-job' to notification metadata
  vzdump: apt: notification: do not include domain in 'hostname' field
  api: replication: include 'hostname' field for notifications
  api: notification: add API for getting known metadata fields/values
  ui: utils: add overrides for translatable notification fields/values

 PVE/API2/APT.pm                   |   3 +-
 PVE/API2/Cluster/Notifications.pm | 155 +++++++++++++++++++++++++++++-
 PVE/API2/Replication.pm           |   4 +-
 PVE/API2/VZDump.pm                |   8 ++
 PVE/Jobs/VZDump.pm                |   4 +-
 PVE/VZDump.pm                     |  14 +--
 www/manager6/Utils.js             |  16 +++
 www/manager6/dc/Backup.js         |  12 ++-
 8 files changed, 204 insertions(+), 12 deletions(-)


proxmox-widget-toolkit:

Lukas Wagner (4):
  notification: matcher: match-field: show known fields/values
  notification: matcher: move match-field formulas to local viewModel
  notification: matcher: move match-calendar fields to panel
  notification: matcher: move match-severity fields to panel

 src/data/model/NotificationConfig.js  |  12 +
 src/window/NotificationMatcherEdit.js | 613 ++++++++++++++++++--------
 2 files changed, 441 insertions(+), 184 deletions(-)


pve-docs:

Lukas Wagner (3):
  notification: clarify that 'hostname' does not include the domain
  notifications: describe new notification metadata fields
  notifications: match-field 'exact'-mode can now match multiple values

 notifications.adoc | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)


Summary over all repositories:
  11 files changed, 663 insertions(+), 217 deletions(-)

-- 
Generated by git-murpp 0.7.1

[pve-devel] [PATCH manager v5 01/16] api: notifications: add 'smtp' to target index

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 PVE/API2/Cluster/Notifications.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notifications.pm
index 7047f0b1..68fdda2a 100644
--- a/PVE/API2/Cluster/Notifications.pm
+++ b/PVE/API2/Cluster/Notifications.pm
@@ -107,6 +107,7 @@ __PACKAGE__->register_method ({
 	my $result = [
 	    { name => 'gotify' },
 	    { name => 'sendmail' },
+	    { name => 'smtp' },
 	];
 
 	return $result;
@@ -143,7 +144,7 @@ __PACKAGE__->register_method ({
 		'type' => {
 		    description => 'Type of the target.',
 		    type  => 'string',
-		    enum => [qw(sendmail gotify)],
+		    enum => [qw(sendmail gotify smtp)],
 		},
 		'comment' => {
 		    description => 'Comment',
-- 
2.39.2

[pve-devel] [PATCH manager v5 02/16] api: jobs: vzdump: pass job 'id' parameter

[Lukas Wagner]

This allows us to access us the backup job id in the send_notification
function, where we can set it as metadata for the notification.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 PVE/API2/VZDump.pm | 8 ++++++++
 PVE/Jobs/VZDump.pm | 4 +++-
 PVE/VZDump.pm      | 6 +++---
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index f66fc740..6bc0b792 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -52,6 +52,14 @@ __PACKAGE__->register_method ({
     parameters => {
 	additionalProperties => 0,
 	properties => PVE::VZDump::Common::json_config_properties({
+	    id => {
+		description => "The ID of the backup job. If set, the 'backup-job' metadata field"
+		    . " of the backup notification will be set to this value.",
+		type => 'string',
+		format => 'pve-configid',
+		maxLength => 64,
+		optional => 1,
+	    },
 	    stdout => {
 		type => 'boolean',
 		description => "Write tar to stdout, not to a file.",
diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
index b8e57945..2f7c72ab 100644
--- a/PVE/Jobs/VZDump.pm
+++ b/PVE/Jobs/VZDump.pm
@@ -12,7 +12,7 @@ use PVE::API2::VZDump;
 use base qw(PVE::VZDump::JobBase);
 
 sub run {
-    my ($class, $conf) = @_;
+    my ($class, $conf, $id) = @_;
 
     my $props = $class->properties();
     # remove all non vzdump related options
@@ -20,6 +20,8 @@ sub run {
 	delete $conf->{$opt} if !defined($props->{$opt});
     }
 
+    $conf->{id} = $id;
+
     # Required as string parameters # FIXME why?! we could just check ref()
     for my $key (keys $PVE::VZDump::Common::PROPERTY_STRINGS->%*) {
 	if ($conf->{$key} && ref($conf->{$key}) eq 'HASH') {
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 152eb3e5..88f42962 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -453,6 +453,7 @@ sub send_notification {
     my ($self, $tasklist, $total_time, $err, $detail_pre, $detail_post) = @_;
 
     my $opts = $self->{opts};
+    my $job_id = $opts->{id};
     my $mailto = $opts->{mailto};
     my $cmdline = $self->{cmdline};
     my $policy = $opts->{mailnotification} // 'always';
@@ -499,12 +500,11 @@ sub send_notification {
     };
 
     my $fields = {
-	# TODO: There is no straight-forward way yet to get the
-	# backup job id here... (I think pvescheduler would need
-	# to pass that to the vzdump call?)
 	type => "vzdump",
 	hostname => $hostname,
     };
+    # Add backup-job metadata field in case this is a backup job.
+    $fields->{'backup-job'} = $job_id if $job_id;
 
     my $severity = $failed ? "error" : "info";
     my $email_configured = $mailto && scalar(@$mailto);
-- 
2.39.2

[pve-devel] [PATCH manager v5 03/16] ui: dc: backup: send 'id' property when starting a backup job manually

[Lukas Wagner]

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

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 70903bdc..4beb84c0 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -569,7 +569,6 @@ Ext.define('PVE.dc.BackupView', {
 	    delete job.enabled;
 	    delete job.starttime;
 	    delete job.dow;
-	    delete job.id;
 	    delete job.schedule;
 	    delete job.type;
 	    delete job.node;
-- 
2.39.2

[pve-devel] [PATCH manager v5 04/16] ui: dc: backup: allow to set custom job id in advanced settings

[Lukas Wagner]

This might be useful if somebody wants to match on the new
'backup-job' field in a notification match rule.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 www/manager6/Utils.js     |  4 ++++
 www/manager6/dc/Backup.js | 11 +++++++++++
 2 files changed, 15 insertions(+)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 287d651a..d4b5f3e6 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1952,6 +1952,10 @@ Ext.define('PVE.Utils', {
     singleton: true,
     constructor: function() {
 	var me = this;
+
+	// Same regex as 'pve-configid
+	me.CONFIGID_RE = /^[A-Za-z][A-Za-z0-9_-]+$/;
+
 	Ext.apply(me, me.utilities);
 
 	Proxmox.Utils.override_task_descriptions({
diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 4beb84c0..5b6f6688 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -397,6 +397,17 @@ Ext.define('PVE.dc.BackupEdit', {
 				},
 			    ],
 			    advancedColumn1: [
+				{
+				    xtype: 'pmxDisplayEditField',
+				    fieldLabel: gettext('Job ID'),
+				    emptyText: gettext('Autogenerate'),
+				    name: 'id',
+				    allowBlank: true,
+				    regex: PVE.Utils.CONFIGID_RE,
+				    cbind: {
+					editable: '{isCreate}',
+				    },
+				},
 				{
 				    xtype: 'proxmoxcheckbox',
 				    fieldLabel: gettext('Repeat missed'),
-- 
2.39.2

[pve-devel] [PATCH manager v5 05/16] api: replication: add 'replication-job' to notification metadata

[Lukas Wagner]

This allows users to create notification match rules for specific
replication jobs, if they so desire.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 PVE/API2/Replication.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
index 0dc944c9..703640f5 100644
--- a/PVE/API2/Replication.pm
+++ b/PVE/API2/Replication.pm
@@ -140,8 +140,8 @@ my sub _handle_job_err {
     };
 
     my $metadata_fields = {
-	# TODO: Add job-id?
 	type => "replication",
+	"replication-job" => $job->{id},
     };
 
     eval {
-- 
2.39.2

[pve-devel] [PATCH manager v5 06/16] vzdump: apt: notification: do not include domain in 'hostname' field

[Lukas Wagner]

 - The man page warns about the usage of `hostname -f`, since a host
   may have multiple domains (or none at all)
 - The fallback PVE::INotify::nodename() already only returned the
   hostname without the domain part
 - Fencing notifications didn't include the domain part anyway

This may result in soft-breakage for any users who have already relied
on the domain being present. If there is need for it, it could include
a fqdn metadata field.

The hostname property used for rendering the notification template
is unaffected for now.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 PVE/API2/APT.pm | 3 ++-
 PVE/VZDump.pm   | 8 ++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index 54121ec2..71c83581 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -354,7 +354,8 @@ __PACKAGE__->register_method({
 		# matchers.
 		my $metadata_fields = {
 		    type => 'package-updates',
-		    hostname => $hostname,
+		    # Hostname (without domain part)
+		    hostname => PVE::INotify::nodename(),
 		};
 
 		PVE::Notify::info(
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 88f42962..c24ff38e 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -487,10 +487,9 @@ sub send_notification {
 	    "See Task History for details!\n";
     };
 
-    my $hostname = get_hostname();
-
     my $notification_props = {
-	"hostname" => $hostname,
+	# Hostname, might include domain part
+	"hostname" => get_hostname(),
 	"error-message" => $err,
 	"guest-table" => build_guest_table($tasklist),
 	"logs" => $text_log_part,
@@ -501,7 +500,8 @@ sub send_notification {
 
     my $fields = {
 	type => "vzdump",
-	hostname => $hostname,
+	# Hostname (without domain part)
+	hostname => PVE::INotify::nodename(),
     };
     # Add backup-job metadata field in case this is a backup job.
     $fields->{'backup-job'} = $job_id if $job_id;
-- 
2.39.2

[pve-devel] [PATCH manager v5 07/16] api: replication: include 'hostname' field for notifications

[Lukas Wagner]

The field contains the hostname of the host (without any domain part)
which sends the notification. This field can be used in match-field
match rules.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 PVE/API2/Replication.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
index 703640f5..192b8af3 100644
--- a/PVE/API2/Replication.pm
+++ b/PVE/API2/Replication.pm
@@ -142,6 +142,8 @@ my sub _handle_job_err {
     my $metadata_fields = {
 	type => "replication",
 	"replication-job" => $job->{id},
+	# Hostname (without domain part)
+	hostname => PVE::INotify::nodename(),
     };
 
     eval {
-- 
2.39.2

[pve-devel] [PATCH manager v5 08/16] api: notification: add API for getting known metadata fields/values

[Lukas Wagner]

This new API route returns known notification metadata fields and
a list of known possible values. This will be used by the UI to
provide suggestions when adding/modifying match rules.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 PVE/API2/Cluster/Notifications.pm | 152 ++++++++++++++++++++++++++++++
 1 file changed, 152 insertions(+)

diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notifications.pm
index 68fdda2a..16548bec 100644
--- a/PVE/API2/Cluster/Notifications.pm
+++ b/PVE/API2/Cluster/Notifications.pm
@@ -79,12 +79,164 @@ __PACKAGE__->register_method ({
 	    { name => 'endpoints' },
 	    { name => 'matchers' },
 	    { name => 'targets' },
+	    { name => 'fields' },
+	    { name => 'values' },
 	];
 
 	return $result;
     }
 });
 
+__PACKAGE__->register_method ({
+    name => 'get_fields',
+    path => 'fields',
+    method => 'GET',
+    description => 'Returns known notification metadata fields and their known values',
+    permissions => {
+	check => ['or',
+	    ['perm', '/mapping/notifications', ['Mapping.Modify']],
+	    ['perm', '/mapping/notifications', ['Mapping.Audit']],
+	],
+    },
+    protected => 1,
+    parameters => {
+	additionalProperties => 0,
+	properties => {},
+    },
+    returns => {
+	type => 'array',
+	items => {
+	    type => 'object',
+	    properties => {
+		name => {
+		    description => 'Name of the field.',
+		    type => 'string',
+		},
+	    },
+	},
+	links => [ { rel => 'child', href => '{name}' } ],
+    },
+    code => sub {
+	# TODO: Adapt this API handler once we have a 'notification registry'
+
+	my $result = [
+	    { name => 'type' },
+	    { name => 'hostname' },
+	    { name => 'backup-job' },
+	    { name => 'replication-job' },
+	];
+
+	return $result;
+    }
+});
+
+__PACKAGE__->register_method ({
+    name => 'get_field_values',
+    path => 'values',
+    method => 'GET',
+    description => 'Returns known notification metadata fields and their known values',
+    permissions => {
+	check => ['or',
+	    ['perm', '/mapping/notifications', ['Mapping.Modify']],
+	    ['perm', '/mapping/notifications', ['Mapping.Audit']],
+	],
+    },
+    protected => 1,
+    parameters => {
+	additionalProperties => 0,
+    },
+    returns => {
+	type => 'array',
+	items => {
+	    type => 'object',
+	    properties => {
+		'value' => {
+		    description => 'Notification metadata value known by the system.',
+		    type => 'string'
+		},
+		'comment' => {
+		    description => 'Additional comment for this value.',
+		    type => 'string',
+		    optional => 1,
+		},
+		'field' => {
+		    description => 'Field this value belongs to.',
+		    type => 'string',
+		    optional => 1,
+		},
+		'internal' => {
+		    description => 'Set if "value" was generated by the system and can'
+		       . ' safely be used as base for translations.',
+		    type => 'boolean',
+		    optional => 1,
+		},
+	    },
+	},
+    },
+    code => sub {
+	# TODO: Adapt this API handler once we have a 'notification registry'
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $user = $rpcenv->get_user();
+
+	my $values = [
+	    {
+		value => 'package-updates',
+		internal => 1,
+		field => 'type',
+	    },
+	    {
+		value => 'fencing',
+		internal => 1,
+		field => 'type',
+	    },
+	    {
+		value => 'replication',
+		internal => 1,
+		field => 'type',
+	    },
+	    {
+		value => 'vzdump',
+		internal => 1,
+		field => 'type',
+	    },
+	    {
+		value => 'system-mail',
+		internal => 1,
+		field => 'type',
+	    },
+	];
+
+	# Here we need a manual permission check.
+	if ($rpcenv->check($user, "/", ["Sys.Audit"], 1)) {
+	    for my $backup_job (@{PVE::API2::Backup->index({})}) {
+		push @$values, {
+		    value => $backup_job->{id},
+		    comment => $backup_job->{comment},
+		    field => 'backup-job'
+		};
+	    }
+	}
+	# The API call returns only returns jobs for which the user
+	# has adequate permissions.
+	for my $sync_job (@{PVE::API2::ReplicationConfig->index({})}) {
+	    push @$values, {
+		value => $sync_job->{id},
+		comment => $sync_job->{comment},
+		field => 'replication-job'
+	    };
+	}
+
+	for my $node (@{PVE::Cluster::get_nodelist()}) {
+	    push @$values, {
+		value => $node,
+		field => 'hostname',
+	    }
+	}
+
+	return $values;
+    }
+});
+
 __PACKAGE__->register_method ({
     name => 'endpoints_index',
     path => 'endpoints',
-- 
2.39.2

[pve-devel] [PATCH manager v5 09/16] ui: utils: add overrides for translatable notification fields/values

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 www/manager6/Utils.js | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index d4b5f3e6..c46ec4df 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -2052,6 +2052,18 @@ Ext.define('PVE.Utils', {
 	    zfscreate: [gettext('ZFS Storage'), gettext('Create')],
 	    zfsremove: ['ZFS Pool', gettext('Remove')],
 	});
+
+	Proxmox.Utils.overrideNotificationFieldName({
+	    'backup-job': gettext('Backup job ID'),
+	    'replication-job': gettext('Replication job ID'),
+	});
+
+	Proxmox.Utils.overrideNotificationFieldValue({
+	    'package-updates': gettext('Package updates are available'),
+	    'vzdump': gettext('Backup notifications'),
+	    'replication': gettext('Replication job notifications'),
+	    'fencing': gettext('Node fencing notifications'),
+	});
     },
 
 });
-- 
2.39.2

[pve-devel] [PATCH widget-toolkit v5 10/16] notification: matcher: match-field: show known fields/values

[Lukas Wagner]

These changes introduce combogrid pickers for the 'field' and 'value'
form elements for 'match-field' match rules. The 'field' picker shows
a list of all known metadata fields, while the 'value' picker shows a
list of all known values, filtered depending on the current value of
'field'.

The list of known fields/values is retrieved from new API endpoints.
Some values are marked 'internal' by the backend. This means that the
'value' field was not user-created (counter example: backup job
IDs) and can therefore be used as a base for translations.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/data/model/NotificationConfig.js  |  12 ++
 src/window/NotificationMatcherEdit.js | 300 +++++++++++++++++++++-----
 2 files changed, 256 insertions(+), 56 deletions(-)

diff --git a/src/data/model/NotificationConfig.js b/src/data/model/NotificationConfig.js
index e8ebf28..a2c365b 100644
--- a/src/data/model/NotificationConfig.js
+++ b/src/data/model/NotificationConfig.js
@@ -15,3 +15,15 @@ Ext.define('proxmox-notification-matchers', {
     },
     idProperty: 'name',
 });
+
+Ext.define('proxmox-notification-fields', {
+    extend: 'Ext.data.Model',
+    fields: ['name', 'description'],
+    idProperty: 'name',
+});
+
+Ext.define('proxmox-notification-field-values', {
+    extend: 'Ext.data.Model',
+    fields: ['value', 'comment', 'internal', 'field'],
+    idProperty: 'value',
+});
diff --git a/src/window/NotificationMatcherEdit.js b/src/window/NotificationMatcherEdit.js
index e717ad7..34ed573 100644
--- a/src/window/NotificationMatcherEdit.js
+++ b/src/window/NotificationMatcherEdit.js
@@ -79,7 +79,7 @@ Ext.define('Proxmox.window.NotificationMatcherEdit', {
 	labelWidth: 120,
     },
 
-    width: 700,
+    width: 800,
 
     initComponent: function() {
 	let me = this;
@@ -416,10 +416,22 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
 		    let me = this;
 		    let record = me.get('selectedRecord');
 		    let currentData = record.get('data');
+
+		    let newValue = [];
+
+		    // Build equivalent regular expression if switching
+		    // to 'regex' mode
+		    if (value === 'regex') {
+			let regexVal = "^(";
+			regexVal += currentData.value.join('|') + ")$";
+			newValue.push(regexVal);
+		    }
+
 		    record.set({
 			data: {
 			    ...currentData,
 			    type: value,
+			    value: newValue,
 			},
 		    });
 		},
@@ -441,6 +453,8 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
 			data: {
 			    ...currentData,
 			    field: value,
+			    // Reset value if field changes
+			    value: [],
 			},
 		    });
 		},
@@ -549,6 +563,9 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
     column2: [
 	{
 	    xtype: 'pmxNotificationMatchRuleSettings',
+	    cbind: {
+		baseUrl: '{baseUrl}',
+	    },
 	},
 
     ],
@@ -601,7 +618,7 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
 		let value = data.value;
 		text = Ext.String.format(gettext("Match field: {0}={1}"), field, value);
 		iconCls = 'fa fa-square-o';
-		if (!field || !value) {
+		if (!field || !value || (Ext.isArray(value) && !value.length)) {
 		    iconCls += ' internal-error';
 		}
 	    } break;
@@ -821,6 +838,11 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
 		if (type === undefined) {
 		    type = "exact";
 		}
+
+		if (type === 'exact') {
+		    matchedValue = matchedValue.split(',');
+		}
+
 		return {
 		    type: 'match-field',
 		    data: {
@@ -982,7 +1004,9 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
 Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
     extend: 'Ext.panel.Panel',
     xtype: 'pmxNotificationMatchRuleSettings',
+    mixins: ['Proxmox.Mixin.CBind'],
     border: false,
+    layout: 'anchor',
 
     items: [
 	{
@@ -1000,6 +1024,8 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
 		['notall', gettext('At least one rule does not match')],
 		['notany', gettext('No rule matches')],
 	    ],
+	    // Hide initially to avoid glitches when opening the window
+	    hidden: true,
 	    bind: {
 		hidden: '{!showMatchingMode}',
 		disabled: '{!showMatchingMode}',
@@ -1011,7 +1037,8 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
 	    fieldLabel: gettext('Node type'),
 	    isFormField: false,
 	    allowBlank: false,
-
+	    // Hide initially to avoid glitches when opening the window
+	    hidden: true,
 	    bind: {
 		value: '{nodeType}',
 		hidden: '{!showMatcherType}',
@@ -1025,57 +1052,9 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
 	    ],
 	},
 	{
-	    fieldLabel: gettext('Match Type'),
-	    xtype: 'proxmoxKVComboBox',
-	    reference: 'type',
-	    isFormField: false,
-	    allowBlank: false,
-	    submitValue: false,
-	    field: 'type',
-
-	    bind: {
-		hidden: '{!typeIsMatchField}',
-		disabled: '{!typeIsMatchField}',
-		value: '{matchFieldType}',
-	    },
-
-	    comboItems: [
-		['exact', gettext('Exact')],
-		['regex', gettext('Regex')],
-	    ],
-	},
-	{
-	    fieldLabel: gettext('Field'),
-	    xtype: 'proxmoxKVComboBox',
-	    isFormField: false,
-	    submitValue: false,
-	    allowBlank: false,
-	    editable: true,
-	    displayField: 'key',
-	    field: 'field',
-	    bind: {
-		hidden: '{!typeIsMatchField}',
-		disabled: '{!typeIsMatchField}',
-		value: '{matchFieldField}',
-	    },
-	    // TODO: Once we have a 'notification registry', we should
-	    // retrive those via an API call.
-	    comboItems: [
-		['type', ''],
-		['hostname', ''],
-	    ],
-	},
-	{
-	    fieldLabel: gettext('Value'),
-	    xtype: 'textfield',
-	    isFormField: false,
-	    submitValue: false,
-	    allowBlank: false,
-	    field: 'value',
-	    bind: {
-		hidden: '{!typeIsMatchField}',
-		disabled: '{!typeIsMatchField}',
-		value: '{matchFieldValue}',
+	    xtype: 'pmxNotificationMatchFieldSettings',
+	    cbind: {
+		baseUrl: '{baseUrl}',
 	    },
 	},
 	{
@@ -1085,7 +1064,8 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
 	    allowBlank: true,
 	    multiSelect: true,
 	    field: 'value',
-
+	    // Hide initially to avoid glitches when opening the window
+	    hidden: true,
 	    bind: {
 		value: '{matchSeverityValue}',
 		hidden: '{!typeIsMatchSeverity}',
@@ -1108,7 +1088,8 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
 	    editable: true,
 	    displayField: 'key',
 	    field: 'value',
-
+	    // Hide initially to avoid glitches when opening the window
+	    hidden: true,
 	    bind: {
 		value: '{matchCalendarValue}',
 		hidden: '{!typeIsMatchCalendar}',
@@ -1122,3 +1103,210 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
 	},
     ],
 });
+
+Ext.define('Proxmox.panel.MatchFieldSettings', {
+    extend: 'Ext.panel.Panel',
+    xtype: 'pmxNotificationMatchFieldSettings',
+    border: false,
+    layout: 'anchor',
+    // Hide initially to avoid glitches when opening the window
+    hidden: true,
+    bind: {
+	hidden: '{!typeIsMatchField}',
+    },
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	control: {
+	    'field[reference=fieldSelector]': {
+		change: function(field) {
+		    let view = this.getView();
+		    let valueField = view.down('field[reference=valueSelector]');
+		    let store = valueField.getStore();
+		    let val = field.getValue();
+
+		    if (val) {
+			store.setFilters([
+			    {
+				property: 'field',
+				value: val,
+			    },
+			]);
+		    }
+		},
+	    },
+	},
+    },
+
+
+    initComponent: function() {
+	let me = this;
+
+	let store = Ext.create('Ext.data.Store', {
+	    model: 'proxmox-notification-fields',
+	    autoLoad: true,
+	    proxy: {
+		type: 'proxmox',
+		url: `/api2/json/${me.baseUrl}/fields`,
+	    },
+	    listeners: {
+		'load': function() {
+		    this.each(function(record) {
+			record.set({
+			    description:
+				Proxmox.Utils.formatNotificationFieldName(
+				    record.get('name'),
+				),
+			});
+		    });
+
+		    // Commit changes so that the description field is not marked
+		    // as dirty
+		    this.commitChanges();
+		},
+	    },
+	});
+
+	let valueStore = Ext.create('Ext.data.Store', {
+	    model: 'proxmox-notification-field-values',
+	    autoLoad: true,
+	    proxy: {
+		type: 'proxmox',
+
+		url: `/api2/json/${me.baseUrl}/values`,
+	    },
+	    listeners: {
+		'load': function() {
+		    this.each(function(record) {
+			// if 'internal' is set, we know that 'value' was provided
+			// by the system and not by the user. We can then use
+			// it safely for deriving translations.
+			if (record.get('internal')) {
+			    record.set({
+				comment:
+				    Proxmox.Utils.formatNotificationFieldValue(
+					record.get('value'),
+				    ),
+			    });
+			}
+		    });
+
+		    // Commit changes so that the description field is not marked
+		    // as dirty
+		    this.commitChanges();
+		},
+	    },
+	});
+
+	Ext.apply(me, {
+	    viewModel: Ext.create('Ext.app.ViewModel', {
+		parent: me.up('pmxNotificationMatchRulesEditPanel').getViewModel(),
+		formulas: {
+		    isRegex: function(get) {
+			return get('matchFieldType') === 'regex';
+		    },
+		},
+	    }),
+	    items: [
+		{
+		    fieldLabel: gettext('Match Type'),
+		    xtype: 'proxmoxKVComboBox',
+		    reference: 'type',
+		    isFormField: false,
+		    allowBlank: false,
+		    submitValue: false,
+		    field: 'type',
+
+		    bind: {
+			value: '{matchFieldType}',
+		    },
+
+		    comboItems: [
+			['exact', gettext('Exact')],
+			['regex', gettext('Regex')],
+		    ],
+		},
+		{
+		    fieldLabel: gettext('Field'),
+		    reference: 'fieldSelector',
+		    xtype: 'proxmoxComboGrid',
+		    isFormField: false,
+		    submitValue: false,
+		    allowBlank: false,
+		    editable: false,
+		    store: store,
+		    queryMode: 'local',
+		    valueField: 'name',
+		    displayField: 'description',
+		    field: 'field',
+		    bind: {
+			value: '{matchFieldField}',
+		    },
+		    listConfig: {
+			columns: [
+			    {
+				header: gettext('Description'),
+				dataIndex: 'description',
+				flex: 2,
+			    },
+			    {
+				header: gettext('Field Name'),
+				dataIndex: 'name',
+				flex: 1,
+			    },
+			],
+		    },
+		},
+		{
+		    fieldLabel: gettext('Value'),
+		    reference: 'valueSelector',
+		    xtype: 'proxmoxComboGrid',
+		    autoSelect: false,
+		    editable: false,
+		    isFormField: false,
+		    submitValue: false,
+		    allowBlank: false,
+		    showClearTrigger: true,
+		    field: 'value',
+		    store: valueStore,
+		    valueField: 'value',
+		    displayField: 'value',
+		    notFoundIsValid: false,
+		    multiSelect: true,
+		    bind: {
+			value: '{matchFieldValue}',
+			hidden: '{isRegex}',
+		    },
+		    listConfig: {
+			columns: [
+			    {
+				header: gettext('Value'),
+				dataIndex: 'value',
+				flex: 1,
+			    },
+			    {
+				header: gettext('Comment'),
+				dataIndex: 'comment',
+				flex: 2,
+			    },
+			],
+		    },
+		},
+		{
+		    fieldLabel: gettext('Regex'),
+		    xtype: 'proxmoxtextfield',
+		    editable: true,
+		    isFormField: false,
+		    submitValue: false,
+		    allowBlank: false,
+		    field: 'value',
+		    bind: {
+			value: '{matchFieldValue}',
+			hidden: '{!isRegex}',
+		    },
+		},
+	    ],
+	});
+	me.callParent();
+    },
+});
-- 
2.39.2

[pve-devel] [PATCH widget-toolkit v5 11/16] notification: matcher: move match-field formulas to local viewModel

[Lukas Wagner]

This should make the code more cohesive and easier to follow.

No functional changes.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/window/NotificationMatcherEdit.js | 186 +++++++++++++-------------
 1 file changed, 92 insertions(+), 94 deletions(-)

diff --git a/src/window/NotificationMatcherEdit.js b/src/window/NotificationMatcherEdit.js
index 34ed573..ded9db5 100644
--- a/src/window/NotificationMatcherEdit.js
+++ b/src/window/NotificationMatcherEdit.js
@@ -380,15 +380,6 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
 		}
 		return !record.isRoot();
 	    },
-	    typeIsMatchField: {
-		bind: {
-		    bindTo: '{selectedRecord}',
-		    deep: true,
-		},
-		get: function(record) {
-		    return record?.get('type') === 'match-field';
-		},
-	    },
 	    typeIsMatchSeverity: {
 		bind: {
 		    bindTo: '{selectedRecord}',
@@ -407,89 +398,13 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
 		    return record?.get('type') === 'match-calendar';
 		},
 	    },
-	    matchFieldType: {
-		bind: {
-		    bindTo: '{selectedRecord}',
-		    deep: true,
-		},
-		set: function(value) {
-		    let me = this;
-		    let record = me.get('selectedRecord');
-		    let currentData = record.get('data');
-
-		    let newValue = [];
-
-		    // Build equivalent regular expression if switching
-		    // to 'regex' mode
-		    if (value === 'regex') {
-			let regexVal = "^(";
-			regexVal += currentData.value.join('|') + ")$";
-			newValue.push(regexVal);
-		    }
-
-		    record.set({
-			data: {
-			    ...currentData,
-			    type: value,
-			    value: newValue,
-			},
-		    });
-		},
-		get: function(record) {
-		    return record?.get('data')?.type;
-		},
-	    },
-	    matchFieldField: {
-		bind: {
-		    bindTo: '{selectedRecord}',
-		    deep: true,
-		},
-		set: function(value) {
-		    let me = this;
-		    let record = me.get('selectedRecord');
-		    let currentData = record.get('data');
-
-		    record.set({
-			data: {
-			    ...currentData,
-			    field: value,
-			    // Reset value if field changes
-			    value: [],
-			},
-		    });
-		},
-		get: function(record) {
-		    return record?.get('data')?.field;
-		},
-	    },
-	    matchFieldValue: {
-		bind: {
-		    bindTo: '{selectedRecord}',
-		    deep: true,
-		},
-		set: function(value) {
-		    let me = this;
-		    let record = me.get('selectedRecord');
-		    let currentData = record.get('data');
-		    record.set({
-			data: {
-			    ...currentData,
-			    value: value,
-			},
-		    });
-		},
-		get: function(record) {
-		    return record?.get('data')?.value;
-		},
-	    },
 	    matchSeverityValue: {
 		bind: {
 		    bindTo: '{selectedRecord}',
 		    deep: true,
 		},
 		set: function(value) {
-		    let me = this;
-		    let record = me.get('selectedRecord');
+		    let record = this.get('selectedRecord');
 		    let currentData = record.get('data');
 		    record.set({
 			data: {
@@ -1137,7 +1052,95 @@ Ext.define('Proxmox.panel.MatchFieldSettings', {
 	    },
 	},
     },
+    viewModel: {
+	// parent is set in `initComponents`
+	formulas: {
+	    typeIsMatchField: {
+		bind: {
+		    bindTo: '{selectedRecord}',
+		    deep: true,
+		},
+		get: function(record) {
+		    return record?.get('type') === 'match-field';
+		},
+	    },
+	    isRegex: function(get) {
+		return get('matchFieldType') === 'regex';
+	    },
+	    matchFieldType: {
+		bind: {
+		    bindTo: '{selectedRecord}',
+		    deep: true,
+		},
+		set: function(value) {
+		    let record = this.get('selectedRecord');
+		    let currentData = record.get('data');
 
+		    let newValue = [];
+
+		    // Build equivalent regular expression if switching
+		    // to 'regex' mode
+		    if (value === 'regex') {
+			let regexVal = "^(";
+			regexVal += currentData.value.join('|') + ")$";
+			newValue.push(regexVal);
+		    }
+
+		    record.set({
+			data: {
+			    ...currentData,
+			    type: value,
+			    value: newValue,
+			},
+		    });
+		},
+		get: function(record) {
+		    return record?.get('data')?.type;
+		},
+	    },
+	    matchFieldField: {
+		bind: {
+		    bindTo: '{selectedRecord}',
+		    deep: true,
+		},
+		set: function(value) {
+		    let record = this.get('selectedRecord');
+		    let currentData = record.get('data');
+
+		    record.set({
+			data: {
+			    ...currentData,
+			    field: value,
+			    // Reset value if field changes
+			    value: [],
+			},
+		    });
+		},
+		get: function(record) {
+		    return record?.get('data')?.field;
+		},
+	    },
+	    matchFieldValue: {
+		bind: {
+		    bindTo: '{selectedRecord}',
+		    deep: true,
+		},
+		set: function(value) {
+		    let record = this.get('selectedRecord');
+		    let currentData = record.get('data');
+		    record.set({
+			data: {
+			    ...currentData,
+			    value: value,
+			},
+		    });
+		},
+		get: function(record) {
+		    return record?.get('data')?.value;
+		},
+	    },
+	},
+    },
 
     initComponent: function() {
 	let me = this;
@@ -1198,15 +1201,10 @@ Ext.define('Proxmox.panel.MatchFieldSettings', {
 	    },
 	});
 
+	Ext.apply(me.viewModel, {
+	    parent: me.up('pmxNotificationMatchRulesEditPanel').getViewModel(),
+	});
 	Ext.apply(me, {
-	    viewModel: Ext.create('Ext.app.ViewModel', {
-		parent: me.up('pmxNotificationMatchRulesEditPanel').getViewModel(),
-		formulas: {
-		    isRegex: function(get) {
-			return get('matchFieldType') === 'regex';
-		    },
-		},
-	    }),
 	    items: [
 		{
 		    fieldLabel: gettext('Match Type'),
-- 
2.39.2

[pve-devel] [PATCH widget-toolkit v5 12/16] notification: matcher: move match-calendar fields to panel

[Lukas Wagner]

Also introduce a local viewModel that is linked to a parent viewModel,
allowing us to move the formulas to the panel.
This should make the code more cohesive and easier to follow.

No functional changes.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/window/NotificationMatcherEdit.js | 92 +++++++++++++++++----------
 1 file changed, 60 insertions(+), 32 deletions(-)

diff --git a/src/window/NotificationMatcherEdit.js b/src/window/NotificationMatcherEdit.js
index ded9db5..75eee42 100644
--- a/src/window/NotificationMatcherEdit.js
+++ b/src/window/NotificationMatcherEdit.js
@@ -389,15 +389,6 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
 		    return record?.get('type') === 'match-severity';
 		},
 	    },
-	    typeIsMatchCalendar: {
-		bind: {
-		    bindTo: '{selectedRecord}',
-		    deep: true,
-		},
-		get: function(record) {
-		    return record?.get('type') === 'match-calendar';
-		},
-	    },
 	    matchSeverityValue: {
 		bind: {
 		    bindTo: '{selectedRecord}',
@@ -417,26 +408,6 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
 		    return record?.get('data')?.value;
 		},
 	    },
-	    matchCalendarValue: {
-		bind: {
-		    bindTo: '{selectedRecord}',
-		    deep: true,
-		},
-		set: function(value) {
-		    let me = this;
-		    let record = me.get('selectedRecord');
-		    let currentData = record.get('data');
-		    record.set({
-			data: {
-			    ...currentData,
-			    value: value,
-			},
-		    });
-		},
-		get: function(record) {
-		    return record?.get('data')?.value;
-		},
-	    },
 	    rootMode: {
 		bind: {
 		    bindTo: '{selectedRecord}',
@@ -995,6 +966,58 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
 		['unknown', gettext('Unknown')],
 	    ],
 	},
+	{
+	    xtype: 'pmxNotificationMatchCalendarSettings',
+	},
+    ],
+});
+
+Ext.define('Proxmox.panel.MatchCalendarSettings', {
+    extend: 'Ext.panel.Panel',
+    xtype: 'pmxNotificationMatchCalendarSettings',
+    border: false,
+    layout: 'anchor',
+    // Hide initially to avoid glitches when opening the window
+    hidden: true,
+    bind: {
+	hidden: '{!typeIsMatchCalendar}',
+    },
+    viewModel: {
+	// parent is set in `initComponents`
+	formulas: {
+	    typeIsMatchCalendar: {
+		bind: {
+		    bindTo: '{selectedRecord}',
+		    deep: true,
+		},
+		get: function(record) {
+		    return record?.get('type') === 'match-calendar';
+		},
+	    },
+
+	    matchCalendarValue: {
+		bind: {
+		    bindTo: '{selectedRecord}',
+		    deep: true,
+		},
+		set: function(value) {
+		    let me = this;
+		    let record = me.get('selectedRecord');
+		    let currentData = record.get('data');
+		    record.set({
+			data: {
+			    ...currentData,
+			    value: value,
+			},
+		    });
+		},
+		get: function(record) {
+		    return record?.get('data')?.value;
+		},
+	    },
+	},
+    },
+    items: [
 	{
 	    xtype: 'proxmoxKVComboBox',
 	    fieldLabel: gettext('Timespan to match'),
@@ -1003,11 +1026,8 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
 	    editable: true,
 	    displayField: 'key',
 	    field: 'value',
-	    // Hide initially to avoid glitches when opening the window
-	    hidden: true,
 	    bind: {
 		value: '{matchCalendarValue}',
-		hidden: '{!typeIsMatchCalendar}',
 		disabled: '{!typeIsMatchCalender}',
 	    },
 
@@ -1017,6 +1037,14 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
 	    ],
 	},
     ],
+
+    initComponent: function() {
+	let me = this;
+	Ext.apply(me.viewModel, {
+	    parent: me.up('pmxNotificationMatchRulesEditPanel').getViewModel(),
+	});
+	me.callParent();
+    },
 });
 
 Ext.define('Proxmox.panel.MatchFieldSettings', {
-- 
2.39.2

[pve-devel] [PATCH widget-toolkit v5 13/16] notification: matcher: move match-severity fields to panel

[Lukas Wagner]

Also introduce a local viewModel that is linked to a parent viewModel,
allowing us to move the formulas to the panel.
This should make the code more cohesive and easier to follow.

No functional changes.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/window/NotificationMatcherEdit.js | 129 ++++++++++++++++----------
 1 file changed, 80 insertions(+), 49 deletions(-)

diff --git a/src/window/NotificationMatcherEdit.js b/src/window/NotificationMatcherEdit.js
index 75eee42..874140b 100644
--- a/src/window/NotificationMatcherEdit.js
+++ b/src/window/NotificationMatcherEdit.js
@@ -380,34 +380,7 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
 		}
 		return !record.isRoot();
 	    },
-	    typeIsMatchSeverity: {
-		bind: {
-		    bindTo: '{selectedRecord}',
-		    deep: true,
-		},
-		get: function(record) {
-		    return record?.get('type') === 'match-severity';
-		},
-	    },
-	    matchSeverityValue: {
-		bind: {
-		    bindTo: '{selectedRecord}',
-		    deep: true,
-		},
-		set: function(value) {
-		    let record = this.get('selectedRecord');
-		    let currentData = record.get('data');
-		    record.set({
-			data: {
-			    ...currentData,
-			    value: value,
-			},
-		    });
-		},
-		get: function(record) {
-		    return record?.get('data')?.value;
-		},
-	    },
+
 	    rootMode: {
 		bind: {
 		    bindTo: '{selectedRecord}',
@@ -944,27 +917,7 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
 	    },
 	},
 	{
-	    xtype: 'proxmoxKVComboBox',
-	    fieldLabel: gettext('Severities to match'),
-	    isFormField: false,
-	    allowBlank: true,
-	    multiSelect: true,
-	    field: 'value',
-	    // Hide initially to avoid glitches when opening the window
-	    hidden: true,
-	    bind: {
-		value: '{matchSeverityValue}',
-		hidden: '{!typeIsMatchSeverity}',
-		disabled: '{!typeIsMatchSeverity}',
-	    },
-
-	    comboItems: [
-		['info', gettext('Info')],
-		['notice', gettext('Notice')],
-		['warning', gettext('Warning')],
-		['error', gettext('Error')],
-		['unknown', gettext('Unknown')],
-	    ],
+	    xtype: 'pmxNotificationMatchSeveritySettings',
 	},
 	{
 	    xtype: 'pmxNotificationMatchCalendarSettings',
@@ -1047,6 +1000,84 @@ Ext.define('Proxmox.panel.MatchCalendarSettings', {
     },
 });
 
+Ext.define('Proxmox.panel.MatchSeveritySettings', {
+    extend: 'Ext.panel.Panel',
+    xtype: 'pmxNotificationMatchSeveritySettings',
+    border: false,
+    layout: 'anchor',
+    // Hide initially to avoid glitches when opening the window
+    hidden: true,
+    bind: {
+	hidden: '{!typeIsMatchSeverity}',
+    },
+    viewModel: {
+	// parent is set in `initComponents`
+	formulas: {
+	    typeIsMatchSeverity: {
+		bind: {
+		    bindTo: '{selectedRecord}',
+		    deep: true,
+		},
+		get: function(record) {
+		    return record?.get('type') === 'match-severity';
+		},
+	    },
+	    matchSeverityValue: {
+		bind: {
+		    bindTo: '{selectedRecord}',
+		    deep: true,
+		},
+		set: function(value) {
+		    let record = this.get('selectedRecord');
+		    let currentData = record.get('data');
+		    record.set({
+			data: {
+			    ...currentData,
+			    value: value,
+			},
+		    });
+		},
+		get: function(record) {
+		    return record?.get('data')?.value;
+		},
+	    },
+	},
+    },
+    items: [
+	{
+	    xtype: 'proxmoxKVComboBox',
+	    fieldLabel: gettext('Severities to match'),
+	    isFormField: false,
+	    allowBlank: true,
+	    multiSelect: true,
+	    field: 'value',
+	    // Hide initially to avoid glitches when opening the window
+	    hidden: true,
+	    bind: {
+		value: '{matchSeverityValue}',
+		hidden: '{!typeIsMatchSeverity}',
+		disabled: '{!typeIsMatchSeverity}',
+	    },
+
+	    comboItems: [
+		['info', gettext('Info')],
+		['notice', gettext('Notice')],
+		['warning', gettext('Warning')],
+		['error', gettext('Error')],
+		['unknown', gettext('Unknown')],
+	    ],
+	},
+    ],
+
+    initComponent: function() {
+	let me = this;
+	Ext.apply(me.viewModel, {
+	    parent: me.up('pmxNotificationMatchRulesEditPanel').getViewModel(),
+	});
+	me.callParent();
+    },
+});
+
 Ext.define('Proxmox.panel.MatchFieldSettings', {
     extend: 'Ext.panel.Panel',
     xtype: 'pmxNotificationMatchFieldSettings',
-- 
2.39.2

[pve-devel] [PATCH docs v5 14/16] notification: clarify that 'hostname' does not include the domain

[Lukas Wagner]

This was a bit inconsistent between the different notification types:
  - APT/VZDump included the domain part
  - fence notifications did not

A decision has been made to unify this by removing the domain part
from APT/VZDump notifications.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 notifications.adoc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/notifications.adoc b/notifications.adoc
index 46aff6a..57053c8 100644
--- a/notifications.adoc
+++ b/notifications.adoc
@@ -301,7 +301,7 @@ Notification Events
 |=======================================================================
 | Field name | Description
 | `type`     | Type of the notifcation
-| `hostname` | Hostname, including domain (e.g. `pve1.example.com`)
+| `hostname` | Hostname, without domain (e.g. `pve1`)
 |=======================================================================
 
 System Mail Forwarding
-- 
2.39.2

[pve-devel] [PATCH docs v5 15/16] notifications: describe new notification metadata fields

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 notifications.adoc | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/notifications.adoc b/notifications.adoc
index 57053c8..0eeed6a 100644
--- a/notifications.adoc
+++ b/notifications.adoc
@@ -289,19 +289,22 @@ Notification Events
 
 [width="100%",options="header"]
 |===========================================================================
-| Event                        | `type`            | Severity | Metadata fields (in addition to `type`)
-| System updates available     |`package-updates`  | `info`   | `hostname`
-| Cluster node fenced          |`fencing`          | `error`  | `hostname`
-| Storage replication failed   |`replication`      | `error`  | -
-| Backup finished              |`vzdump`           | `info` (`error` on failure) | `hostname`
-| Mail for root                |`system-mail`      | `unknown`| -
+| Event                            | `type`            | Severity | Metadata fields (in addition to `type`)
+| System updates available         |`package-updates`  | `info`   | `hostname`
+| Cluster node fenced              |`fencing`          | `error`  | `hostname`
+| Storage replication job failed   |`replication`      | `error`  | `hostname`, `replication-job`
+| Backup succeeded                 |`vzdump`           | `info`   | `hostname`, `backup-job` (only for backup jobs)
+| Backup failed                    |`vzdump`           | `error`  | `hostname`, `backup-job` (only for backup jobs)
+| Mail for root                    |`system-mail`      | `unknown`| `hostname`
 |===========================================================================
 
 [width="100%",options="header"]
 |=======================================================================
-| Field name | Description
-| `type`     | Type of the notifcation
-| `hostname` | Hostname, without domain (e.g. `pve1`)
+| Field name        | Description
+| `type`            | Type of the notifcation
+| `hostname`        | Hostname, without domain (e.g. `pve1`)
+| `backup-job`      | Backup job ID
+| `replication-job` | Replication job ID
 |=======================================================================
 
 System Mail Forwarding
-- 
2.39.2

[pve-devel] [PATCH docs v5 16/16] notifications: match-field 'exact'-mode can now match multiple values

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 notifications.adoc | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/notifications.adoc b/notifications.adoc
index 0eeed6a..912b048 100644
--- a/notifications.adoc
+++ b/notifications.adoc
@@ -221,11 +221,16 @@ configurable schedule.
 Field Matching Rules
 ~~~~~~~~~~~~~~~~~~~~
 Notifications have a selection of metadata fields that can be matched.
+When using `exact` as a matching mode, a `,` can be used as a separator.
+The matching rule then matches if the metadata field has *any* of the specified
+values.
 
 * `match-field exact:type=vzdump` Only match notifications about backups.
+* `match-field exact:type=replication,fencing` Match `replication` and `fencing` notifications.
 * `match-field regex:hostname=^.+\.example\.com$` Match the hostname of
 the node.
 
+
 If a matched metadata field does not exist, the notification will not be
 matched.
 For instance, a `match-field regex:hostname=.*` directive will only match
@@ -267,18 +272,7 @@ matcher: backup-failures
         comment Send notifications about backup failures to one group of admins
 
 matcher: cluster-failures
-        match-field exact:type=replication
-        match-field exact:type=fencing
-        mode any
-        target cluster-admins
-        comment Send cluster-related notifications to other group of admins
-----
-
-The last matcher could also be rewritten using a field matcher with a regular
-expression:
-----
-matcher: cluster-failures
-        match-field regex:type=^(replication|fencing)$
+        match-field exact:type=replication,fencing
         target cluster-admins
         comment Send cluster-related notifications to other group of admins
 ----
-- 
2.39.2

Re: [pve-devel] [PATCH manager v5 04/16] ui: dc: backup: allow to set custom job id in advanced settings

[Fiona Ebner]

Am 15.04.24 um 10:26 schrieb Lukas Wagner:
> This might be useful if somebody wants to match on the new
> 'backup-job' field in a notification match rule.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>

Needs a rebase, because the advanced settings were moved to a new
"Advanced" tab.


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

Re: [pve-devel] [PATCH manager v5 04/16] ui: dc: backup: allow to set custom job id in advanced settings

[Fiona Ebner]

Am 15.04.24 um 10:26 schrieb Lukas Wagner:
> This might be useful if somebody wants to match on the new
> 'backup-job' field in a notification match rule.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  www/manager6/Utils.js     |  4 ++++
>  www/manager6/dc/Backup.js | 11 +++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 287d651a..d4b5f3e6 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1952,6 +1952,10 @@ Ext.define('PVE.Utils', {
>      singleton: true,
>      constructor: function() {
>  	var me = this;
> +
> +	// Same regex as 'pve-configid
> +	me.CONFIGID_RE = /^[A-Za-z][A-Za-z0-9_-]+$/;

This already exists (with nice verification errors), by using
vtype: 'ConfigId'
for the field. It's defined in widget-toolkit, Toolkit.js

> +
>  	Ext.apply(me, me.utilities);
>  
>  	Proxmox.Utils.override_task_descriptions({
> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index 4beb84c0..5b6f6688 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js
> @@ -397,6 +397,17 @@ Ext.define('PVE.dc.BackupEdit', {
>  				},
>  			    ],
>  			    advancedColumn1: [
> +				{
> +				    xtype: 'pmxDisplayEditField',
> +				    fieldLabel: gettext('Job ID'),
> +				    emptyText: gettext('Autogenerate'),
> +				    name: 'id',
> +				    allowBlank: true,
> +				    regex: PVE.Utils.CONFIGID_RE,
> +				    cbind: {
> +					editable: '{isCreate}',
> +				    },
> +				},
>  				{
>  				    xtype: 'proxmoxcheckbox',
>  				    fieldLabel: gettext('Repeat missed'),


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

[pve-devel] applied: [PATCH manager v5 01/16] api: notifications: add 'smtp' to target index

[Fiona Ebner]

Am 15.04.24 um 10:25 schrieb Lukas Wagner:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  PVE/API2/Cluster/Notifications.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notifications.pm
> index 7047f0b1..68fdda2a 100644
> --- a/PVE/API2/Cluster/Notifications.pm
> +++ b/PVE/API2/Cluster/Notifications.pm
> @@ -107,6 +107,7 @@ __PACKAGE__->register_method ({
>  	my $result = [
>  	    { name => 'gotify' },
>  	    { name => 'sendmail' },
> +	    { name => 'smtp' },
>  	];
>  
>  	return $result;
> @@ -143,7 +144,7 @@ __PACKAGE__->register_method ({
>  		'type' => {
>  		    description => 'Type of the target.',
>  		    type  => 'string',
> -		    enum => [qw(sendmail gotify)],
> +		    enum => [qw(sendmail gotify smtp)],
>  		},
>  		'comment' => {
>  		    description => 'Comment',

applied this fix already, thanks!


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

Re: [pve-devel] [PATCH manager v5 02/16] api: jobs: vzdump: pass job 'id' parameter

[Fiona Ebner]

Am 15.04.24 um 10:26 schrieb Lukas Wagner:
> This allows us to access us the backup job id in the send_notification
> function, where we can set it as metadata for the notification.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  PVE/API2/VZDump.pm | 8 ++++++++
>  PVE/Jobs/VZDump.pm | 4 +++-
>  PVE/VZDump.pm      | 6 +++---
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
> index f66fc740..6bc0b792 100644
> --- a/PVE/API2/VZDump.pm
> +++ b/PVE/API2/VZDump.pm
> @@ -52,6 +52,14 @@ __PACKAGE__->register_method ({
>      parameters => {
>  	additionalProperties => 0,
>  	properties => PVE::VZDump::Common::json_config_properties({
> +	    id => {
It's very generic and it's not the ID of the invocation, so maybe 'job-id'?

Hmm, ideally it would be internal-use only. Like this we have to
remember never to fully "trust" this 'id', because a user can lie when
issuing a vzdump API call.

> +		description => "The ID of the backup job. If set, the 'backup-job' metadata field"
> +		    . " of the backup notification will be set to this value.",
> +		type => 'string',
> +		format => 'pve-configid',
> +		maxLength => 64,

Hmm, where does that limit come from? Seems like we do not explicitly
limit it yet upon job creation, which we should too! For creation, it
will fail for long lengths with

500 unable to open file
'/var/lib/pve-manager/jobs/vzdump-backup-a01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789.json.tmp.200643'
- File name too long

Seems like the limit is 256, so a bit shorter for backup job IDs, but
IDs longer than 64 can exist right now. We can either choose 64, which
might break it for a (small) number of users or choose a limit closer to
256.

> +		optional => 1,
> +	    },
>  	    stdout => {
>  		type => 'boolean',
>  		description => "Write tar to stdout, not to a file.",


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

Re: [pve-devel] [PATCH manager v5 05/16] api: replication: add 'replication-job' to notification metadata

[Fiona Ebner]

Am 15.04.24 um 10:26 schrieb Lukas Wagner:
> This allows users to create notification match rules for specific
> replication jobs, if they so desire.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  PVE/API2/Replication.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
> index 0dc944c9..703640f5 100644
> --- a/PVE/API2/Replication.pm
> +++ b/PVE/API2/Replication.pm
> @@ -140,8 +140,8 @@ my sub _handle_job_err {
>      };
>  
>      my $metadata_fields = {
> -	# TODO: Add job-id?
>  	type => "replication",
> +	"replication-job" => $job->{id},
>      };
>  
>      eval {

Not sure if we should use "replication-job" and "backup-job" for the
metadata entries rather then just "job-id". The type is already
something that can be matched, why re-do it implicitly with the field
name? E.g. I want to see all jobs with -fiona- on the system, now I'd
have to create a matcher rule for each job type.


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

Re: [pve-devel] [PATCH manager v5 05/16] api: replication: add 'replication-job' to notification metadata

[Lukas Wagner]

On  2024-04-19 14:02, Fiona Ebner wrote:
> Am 15.04.24 um 10:26 schrieb Lukas Wagner:
>> This allows users to create notification match rules for specific
>> replication jobs, if they so desire.
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>>  PVE/API2/Replication.pm | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
>> index 0dc944c9..703640f5 100644
>> --- a/PVE/API2/Replication.pm
>> +++ b/PVE/API2/Replication.pm
>> @@ -140,8 +140,8 @@ my sub _handle_job_err {
>>      };
>>  
>>      my $metadata_fields = {
>> -	# TODO: Add job-id?
>>  	type => "replication",
>> +	"replication-job" => $job->{id},
>>      };
>>  
>>      eval {
> 
> Not sure if we should use "replication-job" and "backup-job" for the
> metadata entries rather then just "job-id". The type is already
> something that can be matched, why re-do it implicitly with the field
> name? E.g. I want to see all jobs with -fiona- on the system, now I'd
> have to create a matcher rule for each job type.

Had a 'job-id' field at first, but I *think* (can't be too sure after more than 
4 months of not working on this) the reason why I changed it to this approach
were the replication job IDs, which look like '100-0' or similar.
Giving them and other job IDs a unique field made it a bit easier to
understand what is what when creating a matcher in the improved UI.

For instance, if you just have 'job-id', the pre-filled combo box in the 
match-field edit UI might contain these values

  - backup-gaasdgh7asdfg
  - 100-0
  - 101-0

IMO it's a bit hard to understand that the last two are replication jobs. The separate
job fields make this easier.


-- 
- 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 v5 04/16] ui: dc: backup: allow to set custom job id in advanced settings

[Lukas Wagner]

On  2024-04-19 12:31, Fiona Ebner wrote:
> Am 15.04.24 um 10:26 schrieb Lukas Wagner:
>> This might be useful if somebody wants to match on the new
>> 'backup-job' field in a notification match rule.
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>>  www/manager6/Utils.js     |  4 ++++
>>  www/manager6/dc/Backup.js | 11 +++++++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
>> index 287d651a..d4b5f3e6 100644
>> --- a/www/manager6/Utils.js
>> +++ b/www/manager6/Utils.js
>> @@ -1952,6 +1952,10 @@ Ext.define('PVE.Utils', {
>>      singleton: true,
>>      constructor: function() {
>>  	var me = this;
>> +
>> +	// Same regex as 'pve-configid
>> +	me.CONFIGID_RE = /^[A-Za-z][A-Za-z0-9_-]+$/;
> 
> This already exists (with nice verification errors), by using
> vtype: 'ConfigId'
> for the field. It's defined in widget-toolkit, Toolkit.js
> 

Fixed, thanks!

-- 
- 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 v5 08/16] api: notification: add API for getting known metadata fields/values

[Fiona Ebner]

Am 15.04.24 um 10:26 schrieb Lukas Wagner:
> This new API route returns known notification metadata fields and
> a list of known possible values. This will be used by the UI to
> provide suggestions when adding/modifying match rules.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  PVE/API2/Cluster/Notifications.pm | 152 ++++++++++++++++++++++++++++++
>  1 file changed, 152 insertions(+)
> 
> diff --git a/PVE/API2/Cluster/Notifications.pm b/PVE/API2/Cluster/Notifications.pm
> index 68fdda2a..16548bec 100644
> --- a/PVE/API2/Cluster/Notifications.pm
> +++ b/PVE/API2/Cluster/Notifications.pm
> @@ -79,12 +79,164 @@ __PACKAGE__->register_method ({
>  	    { name => 'endpoints' },
>  	    { name => 'matchers' },
>  	    { name => 'targets' },
> +	    { name => 'fields' },

matcher-fields ?

> +	    { name => 'values' },

(matcher-)field-values ?

>  	];
>  
>  	return $result;
>      }
>  });
>  
> +__PACKAGE__->register_method ({
> +    name => 'get_fields',
> +    path => 'fields',
> +    method => 'GET',
> +    description => 'Returns known notification metadata fields and their known values',

It does not return their values.

> +    permissions => {
> +	check => ['or',
> +	    ['perm', '/mapping/notifications', ['Mapping.Modify']],
> +	    ['perm', '/mapping/notifications', ['Mapping.Audit']],
> +	],
> +    },
> +    protected => 1,

No need for protected when all it's doing is returning static info.
(This would have the privileged pvedaemon execute the request).

> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => 'object',
> +	    properties => {
> +		name => {
> +		    description => 'Name of the field.',
> +		    type => 'string',
> +		},
> +	    },
> +	},
> +	links => [ { rel => 'child', href => '{name}' } ],
> +    },
> +    code => sub {
> +	# TODO: Adapt this API handler once we have a 'notification registry'
> +
> +	my $result = [
> +	    { name => 'type' },
> +	    { name => 'hostname' },
> +	    { name => 'backup-job' },
> +	    { name => 'replication-job' },
> +	];
> +
> +	return $result;
> +    }
> +});
> +
> +__PACKAGE__->register_method ({
> +    name => 'get_field_values',
> +    path => 'values',
> +    method => 'GET',
> +    description => 'Returns known notification metadata fields and their known values',
> +    permissions => {
> +	check => ['or',
> +	    ['perm', '/mapping/notifications', ['Mapping.Modify']],
> +	    ['perm', '/mapping/notifications', ['Mapping.Audit']],
> +	],
> +    },
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => 'object',
> +	    properties => {
> +		'value' => {
> +		    description => 'Notification metadata value known by the system.',
> +		    type => 'string'
> +		},
> +		'comment' => {
> +		    description => 'Additional comment for this value.',
> +		    type => 'string',
> +		    optional => 1,
> +		},
> +		'field' => {
> +		    description => 'Field this value belongs to.',
> +		    type => 'string',
> +		    optional => 1,

Why is the field optional? All values are associated to a field below.

> +		},
> +		'internal' => {
> +		    description => 'Set if "value" was generated by the system and can'
> +		       . ' safely be used as base for translations.',
> +		    type => 'boolean',
> +		    optional => 1,
> +		},

Hmm, not sure about this one. It's only used for the type field right?
Can't we just handle that specially in the UI?


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

Re: [pve-devel] [PATCH manager v5 05/16] api: replication: add 'replication-job' to notification metadata

[Fiona Ebner]

Am 19.04.24 um 14:22 schrieb Lukas Wagner:
> 
> 
> On  2024-04-19 14:02, Fiona Ebner wrote:
>> Am 15.04.24 um 10:26 schrieb Lukas Wagner:
>>> This allows users to create notification match rules for specific
>>> replication jobs, if they so desire.
>>>
>>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>>> ---
>>>  PVE/API2/Replication.pm | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
>>> index 0dc944c9..703640f5 100644
>>> --- a/PVE/API2/Replication.pm
>>> +++ b/PVE/API2/Replication.pm
>>> @@ -140,8 +140,8 @@ my sub _handle_job_err {
>>>      };
>>>  
>>>      my $metadata_fields = {
>>> -	# TODO: Add job-id?
>>>  	type => "replication",
>>> +	"replication-job" => $job->{id},
>>>      };
>>>  
>>>      eval {
>>
>> Not sure if we should use "replication-job" and "backup-job" for the
>> metadata entries rather then just "job-id". The type is already
>> something that can be matched, why re-do it implicitly with the field
>> name? E.g. I want to see all jobs with -fiona- on the system, now I'd
>> have to create a matcher rule for each job type.
> 
> Had a 'job-id' field at first, but I *think* (can't be too sure after more than 
> 4 months of not working on this) the reason why I changed it to this approach
> were the replication job IDs, which look like '100-0' or similar.
> Giving them and other job IDs a unique field made it a bit easier to
> understand what is what when creating a matcher in the improved UI.
> 
> For instance, if you just have 'job-id', the pre-filled combo box in the 
> match-field edit UI might contain these values
> 
>   - backup-gaasdgh7asdfg
>   - 100-0
>   - 101-0
> 
> IMO it's a bit hard to understand that the last two are replication jobs. The separate
> job fields make this easier.

We know that it either starts with "backup-" (or "realmsync-", should
those get notifications), or is a replication job. So we could also just
display something that indicates they are replication jobs (e.g.
"replication-XYZ" or "XYZ (replication)"), until we turn replication
jobs into proper jobs in the backend. Otherwise, each job type we add
will just have a new matcher field for its ID.


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

Re: [pve-devel] [PATCH manager v5 05/16] api: replication: add 'replication-job' to notification metadata

[Lukas Wagner]

On  2024-04-19 15:11, Fiona Ebner wrote:
> Am 19.04.24 um 14:22 schrieb Lukas Wagner:
>>
>>
>> On  2024-04-19 14:02, Fiona Ebner wrote:
>>> Am 15.04.24 um 10:26 schrieb Lukas Wagner:
>>>> This allows users to create notification match rules for specific
>>>> replication jobs, if they so desire.
>>>>
>>>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>>>> ---
>>>>  PVE/API2/Replication.pm | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
>>>> index 0dc944c9..703640f5 100644
>>>> --- a/PVE/API2/Replication.pm
>>>> +++ b/PVE/API2/Replication.pm
>>>> @@ -140,8 +140,8 @@ my sub _handle_job_err {
>>>>      };
>>>>  
>>>>      my $metadata_fields = {
>>>> -	# TODO: Add job-id?
>>>>  	type => "replication",
>>>> +	"replication-job" => $job->{id},
>>>>      };
>>>>  
>>>>      eval {
>>>
>>> Not sure if we should use "replication-job" and "backup-job" for the
>>> metadata entries rather then just "job-id". The type is already
>>> something that can be matched, why re-do it implicitly with the field
>>> name? E.g. I want to see all jobs with -fiona- on the system, now I'd
>>> have to create a matcher rule for each job type.
>>
>> Had a 'job-id' field at first, but I *think* (can't be too sure after more than 
>> 4 months of not working on this) the reason why I changed it to this approach
>> were the replication job IDs, which look like '100-0' or similar.
>> Giving them and other job IDs a unique field made it a bit easier to
>> understand what is what when creating a matcher in the improved UI.
>>
>> For instance, if you just have 'job-id', the pre-filled combo box in the 
>> match-field edit UI might contain these values
>>
>>   - backup-gaasdgh7asdfg
>>   - 100-0
>>   - 101-0
>>
>> IMO it's a bit hard to understand that the last two are replication jobs. The separate
>> job fields make this easier.
> 
> We know that it either starts with "backup-" (or "realmsync-", should
> those get notifications), or is a replication job. So we could also just
> display something that indicates they are replication jobs (e.g.
> "replication-XYZ" or "XYZ (replication)"), until we turn replication
> jobs into proper jobs in the backend. Otherwise, each job type we add
> will just have a new matcher field for its ID.

Ok, fine with me. :)

-- 
- 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 v5 08/16] api: notification: add API for getting known metadata fields/values

[Fiona Ebner]

Am 15.04.24 um 10:26 schrieb Lukas Wagner:
> +
> +__PACKAGE__->register_method ({
> +    name => 'get_field_values',
> +    path => 'values',
> +    method => 'GET',
> +    description => 'Returns known notification metadata fields and their known values',
> +    permissions => {
> +	check => ['or',
> +	    ['perm', '/mapping/notifications', ['Mapping.Modify']],
> +	    ['perm', '/mapping/notifications', ['Mapping.Audit']],
> +	],
> +    },
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => 'object',
> +	    properties => {
> +		'value' => {
> +		    description => 'Notification metadata value known by the system.',
> +		    type => 'string'
> +		},
> +		'comment' => {
> +		    description => 'Additional comment for this value.',
> +		    type => 'string',
> +		    optional => 1,
> +		},
> +		'field' => {
> +		    description => 'Field this value belongs to.',
> +		    type => 'string',
> +		    optional => 1,
> +		},
> +		'internal' => {
> +		    description => 'Set if "value" was generated by the system and can'
> +		       . ' safely be used as base for translations.',
> +		    type => 'boolean',
> +		    optional => 1,
> +		},

And wouldn't it be nicer to return already grouped by field? Then maybe
we also don't really need the dedicated fields API endpoint either as
those are just the top-level of the result (with empty array when there
are no values so we don't ever miss any fields).

> +	    },
> +	},
> +    },


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

Re: [pve-devel] [PATCH manager v5 08/16] api: notification: add API for getting known metadata fields/values

[Lukas Wagner]

On  2024-04-19 15:45, Fiona Ebner wrote:
> Am 15.04.24 um 10:26 schrieb Lukas Wagner:
>> +
>> +__PACKAGE__->register_method ({
>> +    name => 'get_field_values',
>> +    path => 'values',
>> +    method => 'GET',
>> +    description => 'Returns known notification metadata fields and their known values',
>> +    permissions => {
>> +	check => ['or',
>> +	    ['perm', '/mapping/notifications', ['Mapping.Modify']],
>> +	    ['perm', '/mapping/notifications', ['Mapping.Audit']],
>> +	],
>> +    },
>> +    protected => 1,
>> +    parameters => {
>> +	additionalProperties => 0,
>> +    },
>> +    returns => {
>> +	type => 'array',
>> +	items => {
>> +	    type => 'object',
>> +	    properties => {
>> +		'value' => {
>> +		    description => 'Notification metadata value known by the system.',
>> +		    type => 'string'
>> +		},
>> +		'comment' => {
>> +		    description => 'Additional comment for this value.',
>> +		    type => 'string',
>> +		    optional => 1,
>> +		},
>> +		'field' => {
>> +		    description => 'Field this value belongs to.',
>> +		    type => 'string',
>> +		    optional => 1,
>> +		},
>> +		'internal' => {
>> +		    description => 'Set if "value" was generated by the system and can'
>> +		       . ' safely be used as base for translations.',
>> +		    type => 'boolean',
>> +		    optional => 1,
>> +		},
> 
> And wouldn't it be nicer to return already grouped by field? Then maybe
> we also don't really need the dedicated fields API endpoint either as
> those are just the top-level of the result (with empty array when there
> are no values so we don't ever miss any fields).
> 

The design of both endpoints was mostly driven by the intention
of keeping the ExtJS side as simple as possible.
Two comboboxes, each with their own api endpoint to fetch data from,
one setting a filter for the other one.
I tried using a single endpoint at first, but was quickly frustrated
by ExtJS and its documentation and settled for this approach as a consequence.

So I'd prefer to leave it as is :D

Regarding the 'internal' flag: Yes, you are right, right now we only need it for 'type'.
I'll leave it out then and handle everything in the UI.

-- 
- Lukas


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