Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs

[Jonatan Schlag <jonatan.schlag@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 4834 bytes --]

Hi,

first thanks for the patchset. Overall it looks quite good

Am Sonntag, den 21.02.2021, 11:38 +0100 schrieb Leo Hofmann:
> 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> wrote:
> > > 
> > > 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 = $_;
> > > 		}
> > > +		
> > > +		# default VLAN tag if not configured
> > > +		$zone_vlan_id = 1 unless
> > > looks_like_number($zone_vlan_id);
> > I am not sure if it is a good idea to add a default here.
> > 
> > Isn’t there a danger that people will hit save prematurely and
> > connect the wrong VLAN with another one?

This was also my first thought.
> > 
> > Usability issues like that cannot be prevented entirely, but I was
> > wondering if this didn’t make it easier to run into that error.
Thats definitly the case. 
> 
> Agreed, this could happen if you are careless!
> However, I hope you only enable VLAN if you are sure which ID you are
> supposed 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.

I would vote for without default value. There is just absolutey no save
default value here and people could assume that this is something like,
"I am clicking save and I am done". And thats not the case people have
to think about vlan ids. I unterstand that field might be hard to spot,
but that should not be solved by a default value. 

Another question
which came to my mind while reviewing this: Is there any good error
when the vlan id is wrong? Shure it is checked in the html, but i have
not found any backend check ... (Not a problem with your patch, but i
want to mention it.)

Apart from this linter is happy too.

Nice work.

Jonat
an

PS.: Apart from this reading the file took 10 mins, till I found out
that $uc has something todo with zones ... . At least a good learning
how to not name variables :-)


> 
> > > 		print <<END
> > > 		<td class="$highlight">
> > > @@ -486,7 +489,7 @@ END
> > > 				<option value="NATIVE"
> > > $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access
> > > native"}</option>
> > > 				<option value="VLAN"
> > > $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf
> > > access vlan"}</option>
> > > 			</select>
> > > -			<input type="number" class="vlanid" id="TAG-
> > > $uc-$mac" name="TAG $uc $mac" min="1" max="4095"
> > > value="$zone_vlan_id" $field_disabled>
> > > +			<input type="number" class="vlanid" id="TAG-
> > > $uc-$mac" name="TAG $uc $mac" min="1" max="4095"
> > > value="$zone_vlan_id" required $field_disabled>
> > > 		</td>
> > > END
> > > ;
> > > @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and
> > > prepare form elements for each zone
> > > 	my $stp_available = $ethsettings{"${uc}_MODE"} eq "bridge"; #
> > > STP is only available in bridge mode
> > > 	my $stp_enabled = $ethsettings{"${uc}_STP"} eq "on";
> > > 	my $stp_priority = $ethsettings{"${uc}_STP_PRIORITY"};
> > > +	
> > > +	# set priority to default value if no numerical value is
> > > configured
> > > +	$stp_priority = 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 code above, I cannot pull this in with the STP patchset where
> > it actually belongs.
> > 
> > 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 two changes if that makes your job easier (and you don't mind me
> submitting everything multiple times)!
> > -Michael
> > 
> > > 	# form element modifiers
> > > 	my $checked = "";
> > > @@ -532,7 +538,7 @@ END
> > > 	# priority input box HTML
> > > 	my $row_2 = <<END
> > > 		<td>
> > > -			<input type="number" class="stp-priority"
> > > id="STP-PRIORITY-$uc" name="STP-PRIORITY-$uc" min="1" max="65535"
> > > value="$stp_priority" $disabled>
> > > +			<input type="number" class="stp-priority"
> > > id="STP-PRIORITY-$uc" name="STP-PRIORITY-$uc" min="1" max="65535"
> > > value="$stp_priority" required $disabled>
> > > 		</td>
> > > END
> > > ;
> > > -- 
> > > 2.27.0.windows.1
> > > 
> Regards, Leo