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

[Michael Tremer <michael.tremer@ipfire.org>]

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

Hi,

> On 21 Feb 2021, at 10:38, Leo Hofmann <hofmann(a)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(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?
>> 
>> 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