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 lineif(($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};
- }
}}