From mboxrd@z Thu Jan  1 00:00:00 1970
From: Leo Hofmann <hofmann@leo-andres.de>
To: development@lists.ipfire.org
Subject: Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs
Date: Sun, 21 Feb 2021 11:38:24 +0100
Message-ID: <ff64e6fb-5f9b-3306-67d2-3c2c0cc666df@leo-andres.de>
In-Reply-To: <CBB9ED31-F28F-4303-A0F2-49E94D236167@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============8302466041999734063=="
List-Id: <development.lists.ipfire.org>

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

Hello Michael,

thank you for looking into these patches. I'll answer below!

Am 19.02.2021 um 20:22 schrieb Michael Tremer:
> Hello,
>
>> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wro=
te:
>>
>> Add default values and mark fields as required.
>>
>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>> ---
>> html/cgi-bin/zoneconf.cgi | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> 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 conn=
ect the wrong VLAN with another one?
>
> Usability issues like that cannot be prevented entirely, but I was wonderin=
g if this didn=E2=80=99t make it easier to run into that error.

Agreed, this could happen if you are careless!
However, I hope you only enable VLAN if you are sure which ID you are suppose=
d to use. In that case, having a default value appear makes it obvious where =
you need to put your desired ID.
But now I'm not sure about this one. I can revert this, of course.

>
>> 		print <<END
>> 		<td class=3D"$highlight">
>> @@ -486,7 +489,7 @@ END
>> 				<option value=3D"NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zonecon=
f access native"}</option>
>> 				<option value=3D"VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang::=
tr{"zoneconf access vlan"}</option>
>> 			</select>
>> -			<input type=3D"number" class=3D"vlanid" id=3D"TAG-$uc-$mac" name=3D"TA=
G $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"TA=
G $uc $mac" min=3D"1" max=3D"4095" value=3D"$zone_vlan_id" required $field_di=
sabled>
>> 		</td>
>> END
>> ;
>> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and prepare form el=
ements for each zone
>> 	my $stp_available =3D $ethsettings{"${uc}_MODE"} eq "bridge"; # STP is on=
ly 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.
Thanks :)
>
> Since this is in this patchset and comes with the dependency to the other c=
ode above, I cannot pull this in with the STP patchset where it actually belo=
ngs.
>
> But I guess we should merge this all together anyways :)
I still have these commits in my local git. I could just submit these last tw=
o changes if that makes your job easier (and you don't mind me submitting eve=
rything multiple times)!
>
> -Michael
>
>> 	# 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" =
name=3D"STP-PRIORITY-$uc" min=3D"1" max=3D"65535" value=3D"$stp_priority" $di=
sabled>
>> +			<input type=3D"number" class=3D"stp-priority" id=3D"STP-PRIORITY-$uc" =
name=3D"STP-PRIORITY-$uc" min=3D"1" max=3D"65535" value=3D"$stp_priority" req=
uired $disabled>
>> 		</td>
>> END
>> ;
>> --=20
>> 2.27.0.windows.1
>>
Regards, Leo

--===============8302466041999734063==--