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

Message ID 20210218143016.972-6-hofmann@leo-andres.de
State Accepted
Headers show
Series [v2,1/6] zoneconf.cgi: Change NIC display order, improve code | expand

Commit Message

Leo-Andres Hofmann Feb. 18, 2021, 2:30 p.m. UTC
Add default values and mark fields as required.

Signed-off-by: Leo-Andres Hofmann <hofmann@leo-andres.de>
---
 html/cgi-bin/zoneconf.cgi | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Michael Tremer Feb. 19, 2021, 7:22 p.m. UTC | #1
Hello,

> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann@leo-andres.de> wrote:
> 
> Add default values and mark fields as required.
> 
> Signed-off-by: Leo-Andres Hofmann <hofmann@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?

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

> 
> 		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.

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 :)

-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
>
Leo-Andres Hofmann Feb. 21, 2021, 10:38 a.m. UTC | #2
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@leo-andres.de> wrote:
>>
>> Add default values and mark fields as required.
>>
>> Signed-off-by: Leo-Andres Hofmann <hofmann@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?
>
> Usability issues like that cannot be prevented entirely, but I was wondering if this didn’t 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 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.

>
>> 		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
Jonatan Schlag Feb. 21, 2021, 7:06 p.m. UTC | #3
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@leo-andres.de> wrote:
> > > 
> > > Add default values and mark fields as required.
> > > 
> > > Signed-off-by: Leo-Andres Hofmann <hofmann@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
Michael Tremer Feb. 22, 2021, 7:02 p.m. UTC | #4
Hi,

> On 21 Feb 2021, at 10:38, Leo Hofmann <hofmann@leo-andres.de> wrote:
> 
> 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@leo-andres.de> wrote:
>>> 
>>> Add default values and mark fields as required.
>>> 
>>> Signed-off-by: Leo-Andres Hofmann <hofmann@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?
>> 
>> Usability issues like that cannot be prevented entirely, but I was wondering if this didn’t 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 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.

Well, we do not have many votes, but let’s maybe leave it empty by default.

I will merge the patch and edit this change out.

>> 
>>> 		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)!

No need for such a small thing.

But thanks anyways :)

-Michael

>> 
>> -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
Adolf Belka Feb. 22, 2021, 10:27 p.m. UTC | #5
Hi all,

On 22/02/2021 20:02, Michael Tremer wrote:
> Hi,
>
>> On 21 Feb 2021, at 10:38, Leo Hofmann <hofmann@leo-andres.de> wrote:
>>
>> 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@leo-andres.de> wrote:
>>>>
>>>> Add default values and mark fields as required.
>>>>
>>>> Signed-off-by: Leo-Andres Hofmann <hofmann@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?
>>>
>>> Usability issues like that cannot be prevented entirely, but I was wondering if this didn’t 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 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.
> Well, we do not have many votes, but let’s maybe leave it empty by default.

Sorry for late reply. For me, I had no problem figuring out where I should put the VLAN ID. I am happy with the system as it was but no hard feelings against having a default value either.

A decision to stay with empty by default is fine with me.


Regards,

Adolf

> I will merge the patch and edit this change out.
>
>>>> 		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)!
> No need for such a small thing.
>
> But thanks anyways :)
>
> -Michael
>
>>> -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
Leo-Andres Hofmann Feb. 22, 2021, 11:22 p.m. UTC | #6
Hi,

Am 22.02.2021 um 20:02 schrieb Michael Tremer:
> Hi,
>
>> On 21 Feb 2021, at 10:38, Leo Hofmann <hofmann@leo-andres.de> wrote:
>>
>> 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@leo-andres.de> wrote:
>>>>
>>>> Add default values and mark fields as required.
>>>>
>>>> Signed-off-by: Leo-Andres Hofmann <hofmann@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?
>>>
>>> Usability issues like that cannot be prevented entirely, but I was wondering if this didn’t 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 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.
> Well, we do not have many votes, but let’s maybe leave it empty by default.
I just read Jonatan's email and was about to write that I agree with both of you, but you beat me to it.
>
> I will merge the patch and edit this change out.

Awesome, I didn't know you could do that. Thanks, this saves me from submitting this a third time :D

Best,
Leo

>
>>>> 		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)!
> No need for such a small thing.
>
> But thanks anyways :)
>
> -Michael
>
>>> -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
Leo-Andres Hofmann Feb. 22, 2021, 11:46 p.m. UTC | #7
Hi Jonatan,

thank you for looking into this, I'll answer below!

Am 21.02.2021 um 20:06 schrieb Jonatan Schlag:
> 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@leo-andres.de> wrote:
>>>>
>>>> Add default values and mark fields as required.
>>>>
>>>> Signed-off-by: Leo-Andres Hofmann <hofmann@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.)
I just tried this on my test system. There is a range check but no error message.
Instead, it reverts the entire assignment, removes the NIC from the zone and saves the configuration like this. Well, now what...

I don't think this is very critical right now because the HTML range check will usually prevent this. But I think we should have a proper check in the backend at some point.
I'll put it on my list.

>
> 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 :-)

$uc is the upper case zone name (RED, ..). This is already used everywhere, so I decided to keep it. I can put this on my list too.

Best regards
Leo

>
>
>>>> 		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

Patch

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);
 
 		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);
 
 	# 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
 ;