* [PATCH] improve DHCP dynamic leases list usability @ 2020-10-18 22:19 Leo Hofmann 2020-10-19 10:26 ` Michael Tremer 0 siblings, 1 reply; 4+ messages in thread From: Leo Hofmann @ 2020-10-18 22:19 UTC (permalink / raw) To: development [-- Attachment #1: Type: text/plain, Size: 10688 bytes --] Some users have complained that displaying the active and expired leases in one unordered list is confusing. This patch improves the DHCP user interface by visually separating active and expired leases. The list is now sorted by active and expired leases and they are divided by a horizontal line. Sorting by IP/MAC/host/time and creating static leases remains unchanged. See forum topic for context: https://community.ipfire.org/t/dhcp-server-expired-leases/ Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de> --- config/cfgroot/header.pl | 167 +++++++++++++++++++++++---------------- 1 file changed, 97 insertions(+), 70 deletions(-) diff --git a/config/cfgroot/header.pl b/config/cfgroot/header.pl index 1046f5992..b5d153a55 100644 --- a/config/cfgroot/header.pl +++ b/config/cfgroot/header.pl @@ -403,88 +403,110 @@ sub PrintActualLeases <th width='25%' align='center'><a href='$ENV{'SCRIPT_NAME'}?ETHER'><b>$tr{'mac address'}</b></a></th> <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?HOSTNAME'><b>$tr{'hostname'}</b></a></th> <th width='25%' align='center'><a href='$ENV{'SCRIPT_NAME'}?ENDTIME'><b>$tr{'lease expires'} (local time d/m/y)</b></a></th> -<th width='5%' align='center'><b>Add to fix leases<b></th> +<th width='5%' align='center'><b>Add to fix leases</b></th> </tr> END ; open(LEASES,"/var/state/dhcp/dhcpd.leases") or die "Can't open dhcpd.leases"; - while ($line = <LEASES>) { - next if( $line =~ /^\s*#/ ); - chomp($line); - @temp = split (' ', $line); - - if ($line =~ /^\s*lease/) { - $ip = $temp[1]; - #All field are not necessarily read. Clear everything - $endtime = 0; - $ether = ""; - $hostname = ""; - } + while (my $line = <LEASES>) { + next if( $line =~ /^\s*#/ ); + chomp($line); + @temp = split (' ', $line); + + if ($line =~ /^\s*lease/) { + $ip = $temp[1]; + #All field are not necessarily read. Clear everything + $endtime = 0; + $endtime_print = ""; + $expired = 0; + $ether = ""; + $hostname = ""; + } - if ($line =~ /^\s*ends/) { - $line =~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/; - $endtime = timegm($6, $5, $4, $3, $2 - 1, $1 - 1900); - } + if ($line =~ /^\s*ends/) { + $line =~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/; + $endtime = timegm($6, $5, $4, $3, $2 - 1, $1 - 1900); + ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = localtime($endtime); + $endtime_print = sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$year+1900,$hour,$min,$sec); + $expired = $endtime < time(); + } - if ($line =~ /^\s*hardware ethernet/) { - $ether = $temp[2]; - $ether =~ s/;//g; - } + if ($line =~ /^\s*hardware ethernet/) { + $ether = $temp[2]; + $ether =~ s/;//g; + } - if ($line =~ /^\s*client-hostname/) { - $hostname = "$temp[1] $temp[2] $temp[3]"; - $hostname =~ s/;//g; - $hostname =~ s/\"//g; - } + if ($line =~ /^\s*client-hostname/) { + $hostname = "$temp[1] $temp[2] $temp[3]"; + $hostname =~ s/;//g; + $hostname =~ s/\"//g; + } - if ($line eq "}") { - @record = ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOSTNAME',$hostname); - $record = {}; # create a reference to empty hash - %{$record} = @record; # populate that hash with @record - $entries{$record->{'IPADDR'}} = $record; # add this to a hash of hashes - } + if ($line eq "}") { + @record = ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOSTNAME',$hostname,'endtime_print',$endtime_print,'expired',$expired); + $record = {}; # create a reference to empty hash + %{$record} = @record; # populate that hash with @record + $entries{$record->{'IPADDR'}} = $record; # add this to a hash of hashes + } } close(LEASES); my $id = 0; - my $col=""; + my $col = ""; + my $separator_printed = 0; foreach my $key (sort leasesort keys %entries) { - print "<form method='post' action='/cgi-bin/dhcp.cgi'>\n"; - my $hostname = &cleanhtml($entries{$key}->{HOSTNAME},"y"); - - if ($id % 2) { - print "<tr>"; - $col="bgcolor='$table1colour'"; - } - else { - print "<tr>"; - $col="bgcolor='$table2colour'"; - } - - print <<END + my $hostname = &cleanhtml($entries{$key}->{HOSTNAME},"y"); + my $hostname_print = $hostname; + if($hostname_print eq "") { #print blank space if no hostname is found + $hostname_print = " "; + } + + # separate active and expired leases with a horizontal line + if(($entries{$key}->{expired}) && ($separator_printed == 0)) { + $separator_printed = 1; + if ($id % 2) { + print "<tr><td colspan='5' bgcolor='$table1colour'><hr size='1'></td></tr>\n"; + } else { + print "<tr><td colspan='5' bgcolor='$table2colour'><hr size='1'></td></tr>\n"; + } + $id++; + } + + print "<form method='post' action='/cgi-bin/dhcp.cgi'>\n"; + if ($id % 2) { + print "<tr>"; + $col="bgcolor='$table1colour'"; + } else { + print "<tr>"; + $col="bgcolor='$table2colour'"; + } + + if($entries{$key}->{expired}) { + print <<END +<td align='center' $col><input type='hidden' name='FIX_ADDR' value='$entries{$key}->{IPADDR}' /><strike><i>$entries{$key}->{IPADDR}</i></strike></td> +<td align='center' $col><input type='hidden' name='FIX_MAC' value='$entries{$key}->{ETHER}' /><strike><i>$entries{$key}->{ETHER}</i></strike></td> +<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' /><strike><i>$hostname_print<i><strike></td> +<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' /><strike><i>$entries{$key}->{endtime_print}</i></strike></td> +END +; + } else { + print <<END <td align='center' $col><input type='hidden' name='FIX_ADDR' value='$entries{$key}->{IPADDR}' />$entries{$key}->{IPADDR}</td> <td align='center' $col><input type='hidden' name='FIX_MAC' value='$entries{$key}->{ETHER}' />$entries{$key}->{ETHER}</td> -<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' /> $hostname</td> -<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' /> +<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' />$hostname_print</td> +<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' />$entries{$key}->{endtime_print}</td> END -; - - ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = localtime ($entries{$key}->{ENDTIME}); - $enddate = sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$year+1900,$hour,$min,$sec); - - if ($entries{$key}->{ENDTIME} < time() ){ - print "<strike>$enddate</strike>"; - } else { - print "$enddate"; - } - print <<END -</td><td $col><input type='hidden' name='ACTION' value='$Lang::tr{'add'}2' /><input type='submit' name='SUBMIT' value='$Lang::tr{'add'}' /> -</td></tr></form> +; + } + + print <<END +<td $col><input type='hidden' name='ACTION' value='$Lang::tr{'add'}2' /><input type='submit' name='SUBMIT' value='$Lang::tr{'add'}' /></td> +</tr></form> END -; - $id++; - } +; + $id++; + } print "</table>"; &closebox(); @@ -499,11 +521,14 @@ sub leasesort { if ($qs eq 'IPADDR') { @a = split(/\./,$entries{$a}->{$qs}); @b = split(/\./,$entries{$b}->{$qs}); + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || ($b[0]<=>$a[0]) || ($b[1]<=>$a[1]) || ($b[2]<=>$a[2]) || ($b[3]<=>$a[3]); - }else { + ; + } else { + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || $entries{$b}->{$qs} cmp $entries{$a}->{$qs}; } } @@ -511,15 +536,17 @@ sub leasesort { { $qs=$dhcpsettings{'SORT_LEASELIST'}; if ($qs eq 'IPADDR') { - @a = split(/\./,$entries{$a}->{$qs}); + @a = split(/\./,$entries{$a}->{$qs}); @b = split(/\./,$entries{$b}->{$qs}); + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || ($a[0]<=>$b[0]) || - ($a[1]<=>$b[1]) || - ($a[2]<=>$b[2]) || + ($a[1]<=>$b[1]) || + ($a[2]<=>$b[2]) || ($a[3]<=>$b[3]); - }else { + } else { + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || $entries{$a}->{$qs} cmp $entries{$b}->{$qs}; - } + } } } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] improve DHCP dynamic leases list usability 2020-10-18 22:19 [PATCH] improve DHCP dynamic leases list usability Leo Hofmann @ 2020-10-19 10:26 ` Michael Tremer 2020-10-19 11:02 ` Aw: " Bernhard Bitsch 0 siblings, 1 reply; 4+ messages in thread From: Michael Tremer @ 2020-10-19 10:26 UTC (permalink / raw) To: development [-- Attachment #1: Type: text/plain, Size: 11164 bytes --] Good Morning Leo, Thank you very much for submitting your patch. I agree that this is an issue that could be fixed. However, your patch does not apply. It seems to have been corrupted while being sent. root(a)michael:/build/ipfire-2.x# pwclient git-am -s 3586 Applying patch #3586 using u'git am -s' Description: improve DHCP dynamic leases list usability warning: Patch sent with format=flowed; space at the end of lines might be lost. Applying: improve DHCP dynamic leases list usability error: corrupt patch at line 10 Patch failed at 0001 improve DHCP dynamic leases list usability hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". 'git am' failed with exit status 128 You seem to have re-indented the code as well as making changes to it which makes reviewing very difficult. It would be better to do this in two steps: a) fix the code that is messy, and then b) extend it with new functionality. Could you please try to rebase this patch against the next branch and submit again? Please let me know if you need any help with this. Best, -Michael > On 18 Oct 2020, at 23:19, Leo Hofmann <hofmann(a)leo-andres.de> wrote: > > Some users have complained that displaying the active and expired leases in one > unordered list is confusing. This patch improves the DHCP user interface by > visually separating active and expired leases. The list is now sorted by active and > expired leases and they are divided by a horizontal line. > Sorting by IP/MAC/host/time and creating static leases remains unchanged. > > See forum topic for context: https://community.ipfire.org/t/dhcp-server-expired-leases/ > > Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de> > > --- > config/cfgroot/header.pl | 167 +++++++++++++++++++++++---------------- > 1 file changed, 97 insertions(+), 70 deletions(-) > > diff --git a/config/cfgroot/header.pl b/config/cfgroot/header.pl > index 1046f5992..b5d153a55 100644 > --- a/config/cfgroot/header.pl > +++ b/config/cfgroot/header.pl > @@ -403,88 +403,110 @@ sub PrintActualLeases > <th width='25%' align='center'><a href='$ENV{'SCRIPT_NAME'}?ETHER'><b>$tr{'mac address'}</b></a></th> > <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?HOSTNAME'><b>$tr{'hostname'}</b></a></th> > <th width='25%' align='center'><a href='$ENV{'SCRIPT_NAME'}?ENDTIME'><b>$tr{'lease expires'} (local time d/m/y)</b></a></th> > -<th width='5%' align='center'><b>Add to fix leases<b></th> > +<th width='5%' align='center'><b>Add to fix leases</b></th> > </tr> > END > ; > > open(LEASES,"/var/state/dhcp/dhcpd.leases") or die "Can't open dhcpd.leases"; > - while ($line = <LEASES>) { > - next if( $line =~ /^\s*#/ ); > - chomp($line); > - @temp = split (' ', $line); > - > - if ($line =~ /^\s*lease/) { > - $ip = $temp[1]; > - #All field are not necessarily read. Clear everything > - $endtime = 0; > - $ether = ""; > - $hostname = ""; > - } > + while (my $line = <LEASES>) { > + next if( $line =~ /^\s*#/ ); > + chomp($line); > + @temp = split (' ', $line); > + > + if ($line =~ /^\s*lease/) { > + $ip = $temp[1]; > + #All field are not necessarily read. Clear everything > + $endtime = 0; > + $endtime_print = ""; > + $expired = 0; > + $ether = ""; > + $hostname = ""; > + } > > - if ($line =~ /^\s*ends/) { > - $line =~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/; > - $endtime = timegm($6, $5, $4, $3, $2 - 1, $1 - 1900); > - } > + if ($line =~ /^\s*ends/) { > + $line =~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/; > + $endtime = timegm($6, $5, $4, $3, $2 - 1, $1 - 1900); > + ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = localtime($endtime); > + $endtime_print = sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$year+1900,$hour,$min,$sec); > + $expired = $endtime < time(); > + } > > - if ($line =~ /^\s*hardware ethernet/) { > - $ether = $temp[2]; > - $ether =~ s/;//g; > - } > + if ($line =~ /^\s*hardware ethernet/) { > + $ether = $temp[2]; > + $ether =~ s/;//g; > + } > > - if ($line =~ /^\s*client-hostname/) { > - $hostname = "$temp[1] $temp[2] $temp[3]"; > - $hostname =~ s/;//g; > - $hostname =~ s/\"//g; > - } > + if ($line =~ /^\s*client-hostname/) { > + $hostname = "$temp[1] $temp[2] $temp[3]"; > + $hostname =~ s/;//g; > + $hostname =~ s/\"//g; > + } > > - if ($line eq "}") { > - @record = ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOSTNAME',$hostname); > - $record = {}; # create a reference to empty hash > - %{$record} = @record; # populate that hash with @record > - $entries{$record->{'IPADDR'}} = $record; # add this to a hash of hashes > - } > + if ($line eq "}") { > + @record = ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOSTNAME',$hostname,'endtime_print',$endtime_print,'expired',$expired); > + $record = {}; # create a reference to empty hash > + %{$record} = @record; # populate that hash with @record > + $entries{$record->{'IPADDR'}} = $record; # add this to a hash of hashes > + } > } > close(LEASES); > > my $id = 0; > - my $col=""; > + my $col = ""; > + my $separator_printed = 0; > foreach my $key (sort leasesort keys %entries) { > - print "<form method='post' action='/cgi-bin/dhcp.cgi'>\n"; > - my $hostname = &cleanhtml($entries{$key}->{HOSTNAME},"y"); > - > - if ($id % 2) { > - print "<tr>"; > - $col="bgcolor='$table1colour'"; > - } > - else { > - print "<tr>"; > - $col="bgcolor='$table2colour'"; > - } > - > - print <<END > + my $hostname = &cleanhtml($entries{$key}->{HOSTNAME},"y"); > + my $hostname_print = $hostname; > + if($hostname_print eq "") { #print blank space if no hostname is found > + $hostname_print = " "; > + } > + > + # separate active and expired leases with a horizontal line > + if(($entries{$key}->{expired}) && ($separator_printed == 0)) { > + $separator_printed = 1; > + if ($id % 2) { > + print "<tr><td colspan='5' bgcolor='$table1colour'><hr size='1'></td></tr>\n"; > + } else { > + print "<tr><td colspan='5' bgcolor='$table2colour'><hr size='1'></td></tr>\n"; > + } > + $id++; > + } > + > + print "<form method='post' action='/cgi-bin/dhcp.cgi'>\n"; > + if ($id % 2) { > + print "<tr>"; > + $col="bgcolor='$table1colour'"; > + } else { > + print "<tr>"; > + $col="bgcolor='$table2colour'"; > + } > + > + if($entries{$key}->{expired}) { > + print <<END > +<td align='center' $col><input type='hidden' name='FIX_ADDR' value='$entries{$key}->{IPADDR}' /><strike><i>$entries{$key}->{IPADDR}</i></strike></td> > +<td align='center' $col><input type='hidden' name='FIX_MAC' value='$entries{$key}->{ETHER}' /><strike><i>$entries{$key}->{ETHER}</i></strike></td> > +<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' /><strike><i>$hostname_print<i><strike></td> > +<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' /><strike><i>$entries{$key}->{endtime_print}</i></strike></td> > +END > +; > + } else { > + print <<END > <td align='center' $col><input type='hidden' name='FIX_ADDR' value='$entries{$key}->{IPADDR}' />$entries{$key}->{IPADDR}</td> > <td align='center' $col><input type='hidden' name='FIX_MAC' value='$entries{$key}->{ETHER}' />$entries{$key}->{ETHER}</td> > -<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' /> $hostname</td> > -<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' /> > +<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' />$hostname_print</td> > +<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' />$entries{$key}->{endtime_print}</td> > END > -; > - > - ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = localtime ($entries{$key}->{ENDTIME}); > - $enddate = sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$year+1900,$hour,$min,$sec); > - > - if ($entries{$key}->{ENDTIME} < time() ){ > - print "<strike>$enddate</strike>"; > - } else { > - print "$enddate"; > - } > - print <<END > -</td><td $col><input type='hidden' name='ACTION' value='$Lang::tr{'add'}2' /><input type='submit' name='SUBMIT' value='$Lang::tr{'add'}' /> > -</td></tr></form> > +; > + } > + > + print <<END > +<td $col><input type='hidden' name='ACTION' value='$Lang::tr{'add'}2' /><input type='submit' name='SUBMIT' value='$Lang::tr{'add'}' /></td> > +</tr></form> > END > -; > - $id++; > - } > +; > + $id++; > + } > > print "</table>"; > &closebox(); > @@ -499,11 +521,14 @@ sub leasesort { > if ($qs eq 'IPADDR') { > @a = split(/\./,$entries{$a}->{$qs}); > @b = split(/\./,$entries{$b}->{$qs}); > + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || > ($b[0]<=>$a[0]) || > ($b[1]<=>$a[1]) || > ($b[2]<=>$a[2]) || > ($b[3]<=>$a[3]); > - }else { > + ; > + } else { > + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || > $entries{$b}->{$qs} cmp $entries{$a}->{$qs}; > } > } > @@ -511,15 +536,17 @@ sub leasesort { > { > $qs=$dhcpsettings{'SORT_LEASELIST'}; > if ($qs eq 'IPADDR') { > - @a = split(/\./,$entries{$a}->{$qs}); > + @a = split(/\./,$entries{$a}->{$qs}); > @b = split(/\./,$entries{$b}->{$qs}); > + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || > ($a[0]<=>$b[0]) || > - ($a[1]<=>$b[1]) || > - ($a[2]<=>$b[2]) || > + ($a[1]<=>$b[1]) || > + ($a[2]<=>$b[2]) || > ($a[3]<=>$b[3]); > - }else { > + } else { > + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || > $entries{$a}->{$qs} cmp $entries{$b}->{$qs}; > - } > + } > } > } > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Aw: Re: [PATCH] improve DHCP dynamic leases list usability 2020-10-19 10:26 ` Michael Tremer @ 2020-10-19 11:02 ` Bernhard Bitsch 2020-10-20 21:55 ` Leo Hofmann 0 siblings, 1 reply; 4+ messages in thread From: Bernhard Bitsch @ 2020-10-19 11:02 UTC (permalink / raw) To: development [-- Attachment #1: Type: text/plain, Size: 12145 bytes --] Hi, nevertheless the functionality is ok. I just tested and reviewed by hand. I agree, that the separation of 'cosmetics' and 'extendings/error fixes' helps a lot in review. Regards, Bernhard > Gesendet: Montag, 19. Oktober 2020 um 12:26 Uhr > Von: "Michael Tremer" <michael.tremer(a)ipfire.org> > An: "Leo Hofmann" <hofmann(a)leo-andres.de> > Cc: development(a)lists.ipfire.org > Betreff: Re: [PATCH] improve DHCP dynamic leases list usability > > Good Morning Leo, > > Thank you very much for submitting your patch. > > I agree that this is an issue that could be fixed. However, your patch does not apply. It seems to have been corrupted while being sent. > > root(a)michael:/build/ipfire-2.x# pwclient git-am -s 3586 > Applying patch #3586 using u'git am -s' > Description: improve DHCP dynamic leases list usability > warning: Patch sent with format=flowed; space at the end of lines might be lost. > Applying: improve DHCP dynamic leases list usability > error: corrupt patch at line 10 > Patch failed at 0001 improve DHCP dynamic leases list usability > hint: Use 'git am --show-current-patch' to see the failed patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > 'git am' failed with exit status 128 > > You seem to have re-indented the code as well as making changes to it which makes reviewing very difficult. It would be better to do this in two steps: a) fix the code that is messy, and then b) extend it with new functionality. > > Could you please try to rebase this patch against the next branch and submit again? > > Please let me know if you need any help with this. > > Best, > -Michael > > > On 18 Oct 2020, at 23:19, Leo Hofmann <hofmann(a)leo-andres.de> wrote: > > > > Some users have complained that displaying the active and expired leases in one > > unordered list is confusing. This patch improves the DHCP user interface by > > visually separating active and expired leases. The list is now sorted by active and > > expired leases and they are divided by a horizontal line. > > Sorting by IP/MAC/host/time and creating static leases remains unchanged. > > > > See forum topic for context: https://community.ipfire.org/t/dhcp-server-expired-leases/ > > > > Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de> > > > > --- > > config/cfgroot/header.pl | 167 +++++++++++++++++++++++---------------- > > 1 file changed, 97 insertions(+), 70 deletions(-) > > > > diff --git a/config/cfgroot/header.pl b/config/cfgroot/header.pl > > index 1046f5992..b5d153a55 100644 > > --- a/config/cfgroot/header.pl > > +++ b/config/cfgroot/header.pl > > @@ -403,88 +403,110 @@ sub PrintActualLeases > > <th width='25%' align='center'><a href='$ENV{'SCRIPT_NAME'}?ETHER'><b>$tr{'mac address'}</b></a></th> > > <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?HOSTNAME'><b>$tr{'hostname'}</b></a></th> > > <th width='25%' align='center'><a href='$ENV{'SCRIPT_NAME'}?ENDTIME'><b>$tr{'lease expires'} (local time d/m/y)</b></a></th> > > -<th width='5%' align='center'><b>Add to fix leases<b></th> > > +<th width='5%' align='center'><b>Add to fix leases</b></th> > > </tr> > > END > > ; > > > > open(LEASES,"/var/state/dhcp/dhcpd.leases") or die "Can't open dhcpd.leases"; > > - while ($line = <LEASES>) { > > - next if( $line =~ /^\s*#/ ); > > - chomp($line); > > - @temp = split (' ', $line); > > - > > - if ($line =~ /^\s*lease/) { > > - $ip = $temp[1]; > > - #All field are not necessarily read. Clear everything > > - $endtime = 0; > > - $ether = ""; > > - $hostname = ""; > > - } > > + while (my $line = <LEASES>) { > > + next if( $line =~ /^\s*#/ ); > > + chomp($line); > > + @temp = split (' ', $line); > > + > > + if ($line =~ /^\s*lease/) { > > + $ip = $temp[1]; > > + #All field are not necessarily read. Clear everything > > + $endtime = 0; > > + $endtime_print = ""; > > + $expired = 0; > > + $ether = ""; > > + $hostname = ""; > > + } > > > > - if ($line =~ /^\s*ends/) { > > - $line =~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/; > > - $endtime = timegm($6, $5, $4, $3, $2 - 1, $1 - 1900); > > - } > > + if ($line =~ /^\s*ends/) { > > + $line =~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/; > > + $endtime = timegm($6, $5, $4, $3, $2 - 1, $1 - 1900); > > + ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = localtime($endtime); > > + $endtime_print = sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$year+1900,$hour,$min,$sec); > > + $expired = $endtime < time(); > > + } > > > > - if ($line =~ /^\s*hardware ethernet/) { > > - $ether = $temp[2]; > > - $ether =~ s/;//g; > > - } > > + if ($line =~ /^\s*hardware ethernet/) { > > + $ether = $temp[2]; > > + $ether =~ s/;//g; > > + } > > > > - if ($line =~ /^\s*client-hostname/) { > > - $hostname = "$temp[1] $temp[2] $temp[3]"; > > - $hostname =~ s/;//g; > > - $hostname =~ s/\"//g; > > - } > > + if ($line =~ /^\s*client-hostname/) { > > + $hostname = "$temp[1] $temp[2] $temp[3]"; > > + $hostname =~ s/;//g; > > + $hostname =~ s/\"//g; > > + } > > > > - if ($line eq "}") { > > - @record = ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOSTNAME',$hostname); > > - $record = {}; # create a reference to empty hash > > - %{$record} = @record; # populate that hash with @record > > - $entries{$record->{'IPADDR'}} = $record; # add this to a hash of hashes > > - } > > + if ($line eq "}") { > > + @record = ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOSTNAME',$hostname,'endtime_print',$endtime_print,'expired',$expired); > > + $record = {}; # create a reference to empty hash > > + %{$record} = @record; # populate that hash with @record > > + $entries{$record->{'IPADDR'}} = $record; # add this to a hash of hashes > > + } > > } > > close(LEASES); > > > > my $id = 0; > > - my $col=""; > > + my $col = ""; > > + my $separator_printed = 0; > > foreach my $key (sort leasesort keys %entries) { > > - print "<form method='post' action='/cgi-bin/dhcp.cgi'>\n"; > > - my $hostname = &cleanhtml($entries{$key}->{HOSTNAME},"y"); > > - > > - if ($id % 2) { > > - print "<tr>"; > > - $col="bgcolor='$table1colour'"; > > - } > > - else { > > - print "<tr>"; > > - $col="bgcolor='$table2colour'"; > > - } > > - > > - print <<END > > + my $hostname = &cleanhtml($entries{$key}->{HOSTNAME},"y"); > > + my $hostname_print = $hostname; > > + if($hostname_print eq "") { #print blank space if no hostname is found > > + $hostname_print = " "; > > + } > > + > > + # separate active and expired leases with a horizontal line > > + if(($entries{$key}->{expired}) && ($separator_printed == 0)) { > > + $separator_printed = 1; > > + if ($id % 2) { > > + print "<tr><td colspan='5' bgcolor='$table1colour'><hr size='1'></td></tr>\n"; > > + } else { > > + print "<tr><td colspan='5' bgcolor='$table2colour'><hr size='1'></td></tr>\n"; > > + } > > + $id++; > > + } > > + > > + print "<form method='post' action='/cgi-bin/dhcp.cgi'>\n"; > > + if ($id % 2) { > > + print "<tr>"; > > + $col="bgcolor='$table1colour'"; > > + } else { > > + print "<tr>"; > > + $col="bgcolor='$table2colour'"; > > + } > > + > > + if($entries{$key}->{expired}) { > > + print <<END > > +<td align='center' $col><input type='hidden' name='FIX_ADDR' value='$entries{$key}->{IPADDR}' /><strike><i>$entries{$key}->{IPADDR}</i></strike></td> > > +<td align='center' $col><input type='hidden' name='FIX_MAC' value='$entries{$key}->{ETHER}' /><strike><i>$entries{$key}->{ETHER}</i></strike></td> > > +<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' /><strike><i>$hostname_print<i><strike></td> > > +<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' /><strike><i>$entries{$key}->{endtime_print}</i></strike></td> > > +END > > +; > > + } else { > > + print <<END > > <td align='center' $col><input type='hidden' name='FIX_ADDR' value='$entries{$key}->{IPADDR}' />$entries{$key}->{IPADDR}</td> > > <td align='center' $col><input type='hidden' name='FIX_MAC' value='$entries{$key}->{ETHER}' />$entries{$key}->{ETHER}</td> > > -<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' /> $hostname</td> > > -<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' /> > > +<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' />$hostname_print</td> > > +<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' />$entries{$key}->{endtime_print}</td> > > END > > -; > > - > > - ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = localtime ($entries{$key}->{ENDTIME}); > > - $enddate = sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$year+1900,$hour,$min,$sec); > > - > > - if ($entries{$key}->{ENDTIME} < time() ){ > > - print "<strike>$enddate</strike>"; > > - } else { > > - print "$enddate"; > > - } > > - print <<END > > -</td><td $col><input type='hidden' name='ACTION' value='$Lang::tr{'add'}2' /><input type='submit' name='SUBMIT' value='$Lang::tr{'add'}' /> > > -</td></tr></form> > > +; > > + } > > + > > + print <<END > > +<td $col><input type='hidden' name='ACTION' value='$Lang::tr{'add'}2' /><input type='submit' name='SUBMIT' value='$Lang::tr{'add'}' /></td> > > +</tr></form> > > END > > -; > > - $id++; > > - } > > +; > > + $id++; > > + } > > > > print "</table>"; > > &closebox(); > > @@ -499,11 +521,14 @@ sub leasesort { > > if ($qs eq 'IPADDR') { > > @a = split(/\./,$entries{$a}->{$qs}); > > @b = split(/\./,$entries{$b}->{$qs}); > > + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || > > ($b[0]<=>$a[0]) || > > ($b[1]<=>$a[1]) || > > ($b[2]<=>$a[2]) || > > ($b[3]<=>$a[3]); > > - }else { > > + ; > > + } else { > > + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || > > $entries{$b}->{$qs} cmp $entries{$a}->{$qs}; > > } > > } > > @@ -511,15 +536,17 @@ sub leasesort { > > { > > $qs=$dhcpsettings{'SORT_LEASELIST'}; > > if ($qs eq 'IPADDR') { > > - @a = split(/\./,$entries{$a}->{$qs}); > > + @a = split(/\./,$entries{$a}->{$qs}); > > @b = split(/\./,$entries{$b}->{$qs}); > > + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || > > ($a[0]<=>$b[0]) || > > - ($a[1]<=>$b[1]) || > > - ($a[2]<=>$b[2]) || > > + ($a[1]<=>$b[1]) || > > + ($a[2]<=>$b[2]) || > > ($a[3]<=>$b[3]); > > - }else { > > + } else { > > + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || > > $entries{$a}->{$qs} cmp $entries{$b}->{$qs}; > > - } > > + } > > } > > } > > > > > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] improve DHCP dynamic leases list usability 2020-10-19 11:02 ` Aw: " Bernhard Bitsch @ 2020-10-20 21:55 ` Leo Hofmann 0 siblings, 0 replies; 4+ messages in thread From: Leo Hofmann @ 2020-10-20 21:55 UTC (permalink / raw) To: development [-- Attachment #1: Type: text/plain, Size: 12513 bytes --] Hi Michael, Hi Bernhard, thank you for your feedback and support! As you suggested, I have split my patch and based it on the "next" branch. I'll submit the two new patches in this thread soon. I hope it is okay to do it this way. Best regards, Leo Am 19.10.2020 um 13:02 schrieb Bernhard Bitsch: > Hi, > > nevertheless the functionality is ok. I just tested and reviewed by hand. > I agree, that the separation of 'cosmetics' and 'extendings/error fixes' helps a lot in review. > > Regards, > Bernhard > >> Gesendet: Montag, 19. Oktober 2020 um 12:26 Uhr >> Von: "Michael Tremer" <michael.tremer(a)ipfire.org> >> An: "Leo Hofmann" <hofmann(a)leo-andres.de> >> Cc: development(a)lists.ipfire.org >> Betreff: Re: [PATCH] improve DHCP dynamic leases list usability >> >> Good Morning Leo, >> >> Thank you very much for submitting your patch. >> >> I agree that this is an issue that could be fixed. However, your patch does not apply. It seems to have been corrupted while being sent. >> >> root(a)michael:/build/ipfire-2.x# pwclient git-am -s 3586 >> Applying patch #3586 using u'git am -s' >> Description: improve DHCP dynamic leases list usability >> warning: Patch sent with format=flowed; space at the end of lines might be lost. >> Applying: improve DHCP dynamic leases list usability >> error: corrupt patch at line 10 >> Patch failed at 0001 improve DHCP dynamic leases list usability >> hint: Use 'git am --show-current-patch' to see the failed patch >> When you have resolved this problem, run "git am --continue". >> If you prefer to skip this patch, run "git am --skip" instead. >> To restore the original branch and stop patching, run "git am --abort". >> 'git am' failed with exit status 128 >> >> You seem to have re-indented the code as well as making changes to it which makes reviewing very difficult. It would be better to do this in two steps: a) fix the code that is messy, and then b) extend it with new functionality. >> >> Could you please try to rebase this patch against the next branch and submit again? >> >> Please let me know if you need any help with this. >> >> Best, >> -Michael >> >>> On 18 Oct 2020, at 23:19, Leo Hofmann <hofmann(a)leo-andres.de> wrote: >>> >>> Some users have complained that displaying the active and expired leases in one >>> unordered list is confusing. This patch improves the DHCP user interface by >>> visually separating active and expired leases. The list is now sorted by active and >>> expired leases and they are divided by a horizontal line. >>> Sorting by IP/MAC/host/time and creating static leases remains unchanged. >>> >>> See forum topic for context: https://community.ipfire.org/t/dhcp-server-expired-leases/ >>> >>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de> >>> >>> --- >>> config/cfgroot/header.pl | 167 +++++++++++++++++++++++---------------- >>> 1 file changed, 97 insertions(+), 70 deletions(-) >>> >>> diff --git a/config/cfgroot/header.pl b/config/cfgroot/header.pl >>> index 1046f5992..b5d153a55 100644 >>> --- a/config/cfgroot/header.pl >>> +++ b/config/cfgroot/header.pl >>> @@ -403,88 +403,110 @@ sub PrintActualLeases >>> <th width='25%' align='center'><a href='$ENV{'SCRIPT_NAME'}?ETHER'><b>$tr{'mac address'}</b></a></th> >>> <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?HOSTNAME'><b>$tr{'hostname'}</b></a></th> >>> <th width='25%' align='center'><a href='$ENV{'SCRIPT_NAME'}?ENDTIME'><b>$tr{'lease expires'} (local time d/m/y)</b></a></th> >>> -<th width='5%' align='center'><b>Add to fix leases<b></th> >>> +<th width='5%' align='center'><b>Add to fix leases</b></th> >>> </tr> >>> END >>> ; >>> >>> open(LEASES,"/var/state/dhcp/dhcpd.leases") or die "Can't open dhcpd.leases"; >>> - while ($line = <LEASES>) { >>> - next if( $line =~ /^\s*#/ ); >>> - chomp($line); >>> - @temp = split (' ', $line); >>> - >>> - if ($line =~ /^\s*lease/) { >>> - $ip = $temp[1]; >>> - #All field are not necessarily read. Clear everything >>> - $endtime = 0; >>> - $ether = ""; >>> - $hostname = ""; >>> - } >>> + while (my $line = <LEASES>) { >>> + next if( $line =~ /^\s*#/ ); >>> + chomp($line); >>> + @temp = split (' ', $line); >>> + >>> + if ($line =~ /^\s*lease/) { >>> + $ip = $temp[1]; >>> + #All field are not necessarily read. Clear everything >>> + $endtime = 0; >>> + $endtime_print = ""; >>> + $expired = 0; >>> + $ether = ""; >>> + $hostname = ""; >>> + } >>> >>> - if ($line =~ /^\s*ends/) { >>> - $line =~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/; >>> - $endtime = timegm($6, $5, $4, $3, $2 - 1, $1 - 1900); >>> - } >>> + if ($line =~ /^\s*ends/) { >>> + $line =~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/; >>> + $endtime = timegm($6, $5, $4, $3, $2 - 1, $1 - 1900); >>> + ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = localtime($endtime); >>> + $endtime_print = sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$year+1900,$hour,$min,$sec); >>> + $expired = $endtime < time(); >>> + } >>> >>> - if ($line =~ /^\s*hardware ethernet/) { >>> - $ether = $temp[2]; >>> - $ether =~ s/;//g; >>> - } >>> + if ($line =~ /^\s*hardware ethernet/) { >>> + $ether = $temp[2]; >>> + $ether =~ s/;//g; >>> + } >>> >>> - if ($line =~ /^\s*client-hostname/) { >>> - $hostname = "$temp[1] $temp[2] $temp[3]"; >>> - $hostname =~ s/;//g; >>> - $hostname =~ s/\"//g; >>> - } >>> + if ($line =~ /^\s*client-hostname/) { >>> + $hostname = "$temp[1] $temp[2] $temp[3]"; >>> + $hostname =~ s/;//g; >>> + $hostname =~ s/\"//g; >>> + } >>> >>> - if ($line eq "}") { >>> - @record = ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOSTNAME',$hostname); >>> - $record = {}; # create a reference to empty hash >>> - %{$record} = @record; # populate that hash with @record >>> - $entries{$record->{'IPADDR'}} = $record; # add this to a hash of hashes >>> - } >>> + if ($line eq "}") { >>> + @record = ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOSTNAME',$hostname,'endtime_print',$endtime_print,'expired',$expired); >>> + $record = {}; # create a reference to empty hash >>> + %{$record} = @record; # populate that hash with @record >>> + $entries{$record->{'IPADDR'}} = $record; # add this to a hash of hashes >>> + } >>> } >>> close(LEASES); >>> >>> my $id = 0; >>> - my $col=""; >>> + my $col = ""; >>> + my $separator_printed = 0; >>> foreach my $key (sort leasesort keys %entries) { >>> - print "<form method='post' action='/cgi-bin/dhcp.cgi'>\n"; >>> - my $hostname = &cleanhtml($entries{$key}->{HOSTNAME},"y"); >>> - >>> - if ($id % 2) { >>> - print "<tr>"; >>> - $col="bgcolor='$table1colour'"; >>> - } >>> - else { >>> - print "<tr>"; >>> - $col="bgcolor='$table2colour'"; >>> - } >>> - >>> - print <<END >>> + my $hostname = &cleanhtml($entries{$key}->{HOSTNAME},"y"); >>> + my $hostname_print = $hostname; >>> + if($hostname_print eq "") { #print blank space if no hostname is found >>> + $hostname_print = " "; >>> + } >>> + >>> + # separate active and expired leases with a horizontal line >>> + if(($entries{$key}->{expired}) && ($separator_printed == 0)) { >>> + $separator_printed = 1; >>> + if ($id % 2) { >>> + print "<tr><td colspan='5' bgcolor='$table1colour'><hr size='1'></td></tr>\n"; >>> + } else { >>> + print "<tr><td colspan='5' bgcolor='$table2colour'><hr size='1'></td></tr>\n"; >>> + } >>> + $id++; >>> + } >>> + >>> + print "<form method='post' action='/cgi-bin/dhcp.cgi'>\n"; >>> + if ($id % 2) { >>> + print "<tr>"; >>> + $col="bgcolor='$table1colour'"; >>> + } else { >>> + print "<tr>"; >>> + $col="bgcolor='$table2colour'"; >>> + } >>> + >>> + if($entries{$key}->{expired}) { >>> + print <<END >>> +<td align='center' $col><input type='hidden' name='FIX_ADDR' value='$entries{$key}->{IPADDR}' /><strike><i>$entries{$key}->{IPADDR}</i></strike></td> >>> +<td align='center' $col><input type='hidden' name='FIX_MAC' value='$entries{$key}->{ETHER}' /><strike><i>$entries{$key}->{ETHER}</i></strike></td> >>> +<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' /><strike><i>$hostname_print<i><strike></td> >>> +<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' /><strike><i>$entries{$key}->{endtime_print}</i></strike></td> >>> +END >>> +; >>> + } else { >>> + print <<END >>> <td align='center' $col><input type='hidden' name='FIX_ADDR' value='$entries{$key}->{IPADDR}' />$entries{$key}->{IPADDR}</td> >>> <td align='center' $col><input type='hidden' name='FIX_MAC' value='$entries{$key}->{ETHER}' />$entries{$key}->{ETHER}</td> >>> -<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' /> $hostname</td> >>> -<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' /> >>> +<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' />$hostname_print</td> >>> +<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' />$entries{$key}->{endtime_print}</td> >>> END >>> -; >>> - >>> - ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = localtime ($entries{$key}->{ENDTIME}); >>> - $enddate = sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$year+1900,$hour,$min,$sec); >>> - >>> - if ($entries{$key}->{ENDTIME} < time() ){ >>> - print "<strike>$enddate</strike>"; >>> - } else { >>> - print "$enddate"; >>> - } >>> - print <<END >>> -</td><td $col><input type='hidden' name='ACTION' value='$Lang::tr{'add'}2' /><input type='submit' name='SUBMIT' value='$Lang::tr{'add'}' /> >>> -</td></tr></form> >>> +; >>> + } >>> + >>> + print <<END >>> +<td $col><input type='hidden' name='ACTION' value='$Lang::tr{'add'}2' /><input type='submit' name='SUBMIT' value='$Lang::tr{'add'}' /></td> >>> +</tr></form> >>> END >>> -; >>> - $id++; >>> - } >>> +; >>> + $id++; >>> + } >>> >>> print "</table>"; >>> &closebox(); >>> @@ -499,11 +521,14 @@ sub leasesort { >>> if ($qs eq 'IPADDR') { >>> @a = split(/\./,$entries{$a}->{$qs}); >>> @b = split(/\./,$entries{$b}->{$qs}); >>> + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || >>> ($b[0]<=>$a[0]) || >>> ($b[1]<=>$a[1]) || >>> ($b[2]<=>$a[2]) || >>> ($b[3]<=>$a[3]); >>> - }else { >>> + ; >>> + } else { >>> + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || >>> $entries{$b}->{$qs} cmp $entries{$a}->{$qs}; >>> } >>> } >>> @@ -511,15 +536,17 @@ sub leasesort { >>> { >>> $qs=$dhcpsettings{'SORT_LEASELIST'}; >>> if ($qs eq 'IPADDR') { >>> - @a = split(/\./,$entries{$a}->{$qs}); >>> + @a = split(/\./,$entries{$a}->{$qs}); >>> @b = split(/\./,$entries{$b}->{$qs}); >>> + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || >>> ($a[0]<=>$b[0]) || >>> - ($a[1]<=>$b[1]) || >>> - ($a[2]<=>$b[2]) || >>> + ($a[1]<=>$b[1]) || >>> + ($a[2]<=>$b[2]) || >>> ($a[3]<=>$b[3]); >>> - }else { >>> + } else { >>> + $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} || >>> $entries{$a}->{$qs} cmp $entries{$b}->{$qs}; >>> - } >>> + } >>> } >>> } >>> >>> >>> >> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-10-20 21:55 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-18 22:19 [PATCH] improve DHCP dynamic leases list usability Leo Hofmann 2020-10-19 10:26 ` Michael Tremer 2020-10-19 11:02 ` Aw: " Bernhard Bitsch 2020-10-20 21:55 ` Leo Hofmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox