| Message ID | 20210218143016.972-6-hofmann@leo-andres.de |
|---|---|
| State | Accepted |
| Headers |
Return-Path: <development-bounces@lists.ipfire.org> Received: from mail01.ipfire.org (mail01.haj.ipfire.org [172.28.1.202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) client-signature ECDSA (P-384)) (Client CN "mail01.haj.ipfire.org", Issuer "R3" (verified OK)) by web04.haj.ipfire.org (Postfix) with ESMTPS id 4DhHFP2xKCz3wps for <patchwork@web04.haj.ipfire.org>; Thu, 18 Feb 2021 14:31:05 +0000 (UTC) Received: from mail02.haj.ipfire.org (mail02.haj.ipfire.org [172.28.1.201]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) client-signature ECDSA (P-384)) (Client CN "mail02.haj.ipfire.org", Issuer "R3" (verified OK)) by mail01.ipfire.org (Postfix) with ESMTPS id 4DhHFN70kPz295; Thu, 18 Feb 2021 14:31:04 +0000 (UTC) Received: from mail02.haj.ipfire.org (localhost [127.0.0.1]) by mail02.haj.ipfire.org (Postfix) with ESMTP id 4DhHFN6PTgz2xkD; Thu, 18 Feb 2021 14:31:04 +0000 (UTC) Received: from mail01.ipfire.org (mail01.haj.ipfire.org [172.28.1.202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) client-signature ECDSA (P-384)) (Client CN "mail01.haj.ipfire.org", Issuer "R3" (verified OK)) by mail02.haj.ipfire.org (Postfix) with ESMTPS id 4DhHFM1yc0z2xX7 for <development@lists.ipfire.org>; Thu, 18 Feb 2021 14:31:03 +0000 (UTC) Received: from arche.uberspace.de (arche.uberspace.de [185.26.156.147]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mail01.ipfire.org (Postfix) with ESMTPS id 4DhHFM0zjNz295 for <development@lists.ipfire.org>; Thu, 18 Feb 2021 14:31:03 +0000 (UTC) Received: (qmail 28825 invoked from network); 18 Feb 2021 14:30:49 -0000 Received: from localhost (HELO localhost) (127.0.0.1) by arche.uberspace.de with SMTP; 18 Feb 2021 14:30:49 -0000 From: Leo-Andres Hofmann <hofmann@leo-andres.de> To: development@lists.ipfire.org Subject: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs Date: Thu, 18 Feb 2021 15:30:16 +0100 Message-Id: <20210218143016.972-6-hofmann@leo-andres.de> X-Mailer: git-send-email 2.27.0.windows.1 In-Reply-To: <20210218143016.972-1-hofmann@leo-andres.de> References: <20210218143016.972-1-hofmann@leo-andres.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit ARC-Authentication-Results: i=1; mail01.ipfire.org; dkim=none; spf=pass (mail01.ipfire.org: domain of hofmann@leo-andres.de designates 185.26.156.147 as permitted sender) smtp.mailfrom=hofmann@leo-andres.de; dmarc=none ARC-Seal: i=1; s=202003rsa; d=lists.ipfire.org; t=1613658663; a=rsa-sha256; cv=none; b=Uul+xUP6qPzPt2ZLFHaldKAJ+d8epPvDJpW9OGn1YU/HfQc4VhRAWBTfvqCSWDkDF7vgeV tzSZJ6LqlzF+jJPmFbVe2gL9JXJQiZAi3mC6S9PpdLC0aNDLLWp1npAfZ53uYdjL/QMs68 VSt0ISbsyCxBupZHkoHjVP1Qgx8M7dE0m6u/NinexxRNYs7TNxjIoTdAp/RG27/S6DgsOO lqJEZ6of+jaN1V2MFaTkzMRdJedz+ki3y1Dv+GM16bghxzGca8k1JnhT8X+tjcB0mt7UzE ezy9hmI2jKlOyJNiVxcZul86NRadfz7N+tdlW6Tlt64ndQJBI4t9DfykWA+22g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=lists.ipfire.org; s=202003rsa; t=1613658663; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=P3Zn1Q4M9HZsluCwVQ6injHrYHMxu6BF6CNgl3k1uu4=; b=Fb4pY8lrxqPWEYavzl7qRt8lk/BIXU0oSFmKfd5q3HmDqIJvBgY+kgfAjXAva+YG/Uek9v gFjncNUOiIfJiCHQpEHWOZ1Lf3V+05KVxyFcf8saqTYVhApMPRuMrT96cGT1qZtmYa2tBt M+QbBHrxu2+LiL6VH9x18+YEMhiXPANm6SVtvbK5z4wSgmXgS3XWCqwhhAtWXeQuYuUhDB kMpCH+gvmWqC+omNBbuEFeoCA0BDh2vY2mBa43wZQfqENTfR7Uw4UHX2EK3ClW3JlK+Eg8 BPJA9VXORDZhDyONbePUb3xgbdj22aWfRO0QDu+comF2rWWuE+bIBn1ojmbM7A== Authentication-Results: mail01.ipfire.org; dkim=none; spf=pass (mail01.ipfire.org: domain of hofmann@leo-andres.de designates 185.26.156.147 as permitted sender) smtp.mailfrom=hofmann@leo-andres.de; dmarc=none X-Rspamd-Server: mail01.haj.ipfire.org X-Spamd-Result: default: False [-0.01 / 11.00]; ARC_NA(0.00)[]; BAYES_HAM(-3.00)[99.99%]; FROM_HAS_DN(0.00)[]; R_SPF_ALLOW(-0.20)[+mx:c]; R_MISSING_CHARSET(2.50)[]; MIME_GOOD(-0.10)[text/plain]; TO_DN_NONE(0.00)[]; BROKEN_CONTENT_TYPE(1.50)[]; RCPT_COUNT_ONE(0.00)[1]; RCVD_TLS_LAST(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MID_CONTAINS_FROM(1.00)[]; IP_REPUTATION_HAM(-1.71)[asn: 205766(-0.24), country: DE(-0.01), ip: 185.26.156.147(-0.61)]; DMARC_NA(0.00)[leo-andres.de]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:205766, ipnet:185.26.156.0/24, country:DE]; RCVD_COUNT_TWO(0.00)[2]; ARC_SIGNED(0.00)[lists.ipfire.org:s=202003rsa:i=1] X-Rspamd-Queue-Id: 4DhHFM0zjNz295 X-BeenThere: development@lists.ipfire.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: IPFire development talk <development.lists.ipfire.org> List-Unsubscribe: <https://lists.ipfire.org/mailman/options/development>, <mailto:development-request@lists.ipfire.org?subject=unsubscribe> List-Archive: <http://lists.ipfire.org/pipermail/development/> List-Post: <mailto:development@lists.ipfire.org> List-Help: <mailto:development-request@lists.ipfire.org?subject=help> List-Subscribe: <https://lists.ipfire.org/mailman/listinfo/development>, <mailto:development-request@lists.ipfire.org?subject=subscribe> Errors-To: development-bounces@lists.ipfire.org Sender: "Development" <development-bounces@lists.ipfire.org> |
| Series |
[v2,1/6] zoneconf.cgi: Change NIC display order, improve code
|
|
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
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 >
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
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
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
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
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
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
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 ;