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};
> > -    }
> > +        }
> >      }
> >  }
> > 
> > 
> > 
> 
>