From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs
Date: Fri, 19 Feb 2021 19:22:27 +0000
Message-ID: <CBB9ED31-F28F-4303-A0F2-49E94D236167@ipfire.org>
In-Reply-To: <20210218143016.972-6-hofmann@leo-andres.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============8703163750546905227=="
List-Id: <development.lists.ipfire.org>

--===============8703163750546905227==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hello,

> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrot=
e:
>=20
> Add default values and mark fields as required.
>=20
> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> ---
> html/cgi-bin/zoneconf.cgi | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>=20
> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
> index 9d01d06ce..bbd042ffc 100644
> --- a/html/cgi-bin/zoneconf.cgi
> +++ b/html/cgi-bin/zoneconf.cgi
> @@ -478,6 +478,9 @@ END
> 		if ($access_selected{"NONE"} eq "") {
> 			$highlight =3D $_;
> 		}
> +	=09
> +		# default VLAN tag if not configured
> +		$zone_vlan_id =3D 1 unless looks_like_number($zone_vlan_id);

I am not sure if it is a good idea to add a default here.

Isn=E2=80=99t there a danger that people will hit save prematurely and connec=
t the wrong VLAN with another one?

Usability issues like that cannot be prevented entirely, but I was wondering =
if this didn=E2=80=99t make it easier to run into that error.

>=20
> 		print <<END
> 		<td class=3D"$highlight">
> @@ -486,7 +489,7 @@ END
> 				<option value=3D"NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zoneconf=
 access native"}</option>
> 				<option value=3D"VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang::t=
r{"zoneconf access vlan"}</option>
> 			</select>
> -			<input type=3D"number" class=3D"vlanid" id=3D"TAG-$uc-$mac" name=3D"TAG=
 $uc $mac" min=3D"1" max=3D"4095" value=3D"$zone_vlan_id" $field_disabled>
> +			<input type=3D"number" class=3D"vlanid" id=3D"TAG-$uc-$mac" name=3D"TAG=
 $uc $mac" min=3D"1" max=3D"4095" value=3D"$zone_vlan_id" required $field_dis=
abled>
> 		</td>
> END
> ;
> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and prepare form ele=
ments for each zone
> 	my $stp_available =3D $ethsettings{"${uc}_MODE"} eq "bridge"; # STP is onl=
y available in bridge mode
> 	my $stp_enabled =3D $ethsettings{"${uc}_STP"} eq "on";
> 	my $stp_priority =3D $ethsettings{"${uc}_STP_PRIORITY"};
> +=09
> +	# set priority to default value if no numerical value is configured
> +	$stp_priority =3D 32768 unless looks_like_number($stp_priority);

This is very good.

Since this is in this patchset and comes with the dependency to the other cod=
e above, I cannot pull this in with the STP patchset where it actually belong=
s.

But I guess we should merge this all together anyways :)

-Michael

>=20
> 	# form element modifiers
> 	my $checked =3D "";
> @@ -532,7 +538,7 @@ END
> 	# priority input box HTML
> 	my $row_2 =3D <<END
> 		<td>
> -			<input type=3D"number" class=3D"stp-priority" id=3D"STP-PRIORITY-$uc" n=
ame=3D"STP-PRIORITY-$uc" min=3D"1" max=3D"65535" value=3D"$stp_priority" $dis=
abled>
> +			<input type=3D"number" class=3D"stp-priority" id=3D"STP-PRIORITY-$uc" n=
ame=3D"STP-PRIORITY-$uc" min=3D"1" max=3D"65535" value=3D"$stp_priority" requ=
ired $disabled>
> 		</td>
> END
> ;
> --=20
> 2.27.0.windows.1
>=20


--===============8703163750546905227==--