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