improve DHCP dynamic leases list usability
Commit Message
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@leo-andres.de>
---
config/cfgroot/header.pl | 167 +++++++++++++++++++++++----------------
1 file changed, 97 insertions(+), 70 deletions(-)
Comments
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@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@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@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};
> - }
> + }
> }
> }
>
>
>
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@ipfire.org>
> An: "Leo Hofmann" <hofmann@leo-andres.de>
> Cc: development@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@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@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@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};
> > - }
> > + }
> > }
> > }
> >
> >
> >
>
>
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@ipfire.org>
>> An: "Leo Hofmann" <hofmann@leo-andres.de>
>> Cc: development@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@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@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@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};
>>> - }
>>> + }
>>> }
>>> }
>>>
>>>
>>>
>>
@@ -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};
- }
+ }
}
}