Re: [PATCH] improve DHCP dynamic leases list usability

[Leo Hofmann <hofmann@leo-andres.de>]

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