From ffe41ad5e3babca70746b5a7ac3bb1c7aa5ebaa1 Mon Sep 17 00:00:00 2001
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Date: Sat, 16 Nov 2024 14:35:57 +0100
Subject: [PATCH] network edit: shorten and improve bridge vlan ID validator

This does a few things, but all affecting the same validator and there
would not be much value with separate commits, so just use one.

Namely:
- reduce code by a lot, mostly by having a explicit ordered if-else
  chain that avoids the need for some extra checks as further branches
  can assume that former did not evaluate to true, thus we cans safe
  the closure that checked invalidity for a range-atom.
  While this allows entering "useless" ranges like "2-2" it's not
  clear why that should be a disallowed range, it's perfectly clear
  about what it represents.
- use Number.isNaN to avoid oddities from global isNaN that MDN warns
  against [0]
- give explicit error messages for different failure cases like
  out-of-range or not-a-number
- place error messages under gettext, our translators frequently ask
  to avoid untranslatable literals.
- support the same lists as the backend does, i.e. allow multiple
  whitespace and comma and semicolon as separators.

[0]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/isNaN#description

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/node/NetworkEdit.js | 35 +++++++++--------------------------
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/src/node/NetworkEdit.js b/src/node/NetworkEdit.js
index e4fe2db..fbd4f21 100644
--- a/src/node/NetworkEdit.js
+++ b/src/node/NetworkEdit.js
@@ -74,39 +74,22 @@ Ext.define('Proxmox.node.NetworkEdit', {
 			return true;
 		    }
 
-		    let vid_list = value.split(' ');
-
-		    let invalidVid = function(tag) {
-			if (!isNaN(tag) && (tag < 2 || tag > 4094)) {
-			    return `not a valid VLAN ID '${tag}'`;
-			}
-
-			return false;
-		    };
-
-		    for (const vid of vid_list) {
+		    for (const vid of value.split(/\s+[,;]?/)) {
 			if (!vid) {
 			    continue;
 			}
 			let res = vid.match(/^(\d+)(?:-(\d+))?$/);
 			if (!res) {
-			    return `not a valid VLAN configuration '${vid}'`;
+			    return Ext.String.format(gettext("not a valid bridge VLAN ID entry: {0}"), vid);
 			}
-			let start = Number(res[1]);
-			let end = Number(res[2]);
+			let start = Number(res[1]), end = Number(res[2] ?? res[1]); // end=start for single IDs
 
-			res = invalidVid(start);
-			if (res) {
-			    return res;
-			}
-
-			res = invalidVid(end);
-			if (res) {
-			    return res;
-			}
-
-			if (start >= end) {
-			    return `VID range must go from lower to higher tag: '${vid}'`;
+			if (Number.isNaN(start) || Number.isNaN(end)) {
+			    return Ext.String.format(gettext('VID range includes not-a-number: {0}'), vid);
+			} else if (start > end) {
+			    return Ext.String.format(gettext('VID range must go from lower to higher tag: {0}'), vid);
+			} else if (start < 2 || end > 4094) { // check just one each, we already ensured start < end
+			    return Ext.String.format(gettext('VID range outside of allowed 2 and 4094 limit: {0}'), vid);
 			}
 		    }
 		    return true;