Attachment 114071 Details for Bug 21886 – Bug 21886: (QA follow-up) Fix indentation, prevent warns, fix parameter, simplify SQL

[patch] Bug 21886: (QA follow-up) Fix indentation, prevent warns, fix parameter, simplify SQL

Bug-21886-QA-follow-up-Fix-indentation-prevent-war.patch (text/plain), 7.88 KB, created by Nick Clemens (kidclamp) on 2020-12-01 13:24:49 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2020-12-01 13:24:49 UTC

Size: 7.88 KB

patch

obsolete

>From e66ed13a39524328ce78de05a2275bda7ed68148 Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>Date: Tue, 1 Dec 2020 12:50:28 +0000
>Subject: [PATCH] Bug 21886: (QA follow-up) Fix indentation, prevent warns, fix
> parameter, simplify SQL
>
>The SQL code was duplicated, I combine them here
>
>frombranch needs to take input
>
>switch owning to frombranch in second script
>
>initialize hash as empty list, not a reference
>
>add a newline after printed output if not mailing notices
>
>Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
>---
> C4/Circulation.pm                | 33 ++++++++-------------------------
> misc/cronjobs/advance_notices.pl | 38 +++++++++++++++++++-------------------
> misc/cronjobs/overdue_notices.pl | 15 +++++++++++----
> 3 files changed, 38 insertions(+), 48 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index a0ed814606..2960c30bb9 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -2700,31 +2700,14 @@ sub GetUpcomingDueIssues {
>     $params->{'days_in_advance'} = 7 unless exists $params->{'days_in_advance'};
>     my $dbh = C4::Context->dbh;
>     my $statement;
>-    if($params->{'owning_library'}) {
>-    $statement = <<"END_SQL";
>-SELECT *
>-FROM (
>-    SELECT issues.*, items.itype as itemtype, items.homebranch, TO_DAYS( date_due )-TO_DAYS( NOW() ) as days_until_due, branches.branchemail
>-    FROM issues
>-    LEFT JOIN items USING (itemnumber)
>-    LEFT OUTER JOIN branches ON (branches.branchcode = items.homebranch)
>-    WHERE returndate is NULL
>-) tmp
>-WHERE days_until_due >= 0 AND days_until_due <= ?
>-END_SQL
>-    } else {
>-    $statement = <<"END_SQL";
>-SELECT *
>-FROM (
>-    SELECT issues.*, items.itype as itemtype, items.homebranch, TO_DAYS( date_due )-TO_DAYS( NOW() ) as days_until_due, branches.branchemail
>-    FROM issues
>-    LEFT JOIN items USING (itemnumber)
>-    LEFT OUTER JOIN branches USING (branchcode)
>-    WHERE returndate is NULL
>-) tmp
>-WHERE days_until_due >= 0 AND days_until_due <= ?
>-END_SQL
>-    }
>+    $statement = q{
>+        SELECT issues.*, items.itype as itemtype, items.homebranch, TO_DAYS( date_due )-TO_DAYS( NOW() ) as days_until_due, branches.branchemail
>+        FROM issues
>+        LEFT JOIN items USING (itemnumber)
>+        LEFT JOIN branches ON branches.branchcode =
>+    };
>+    $statement .= $params->{'owning_library'} ? " items.homebranch " : " issues.branchcode ";
>+    $statement .= " WHERE returndate is NULL AND TO_DAYS( date_due )-TO_DAYS( NOW() ) BETWEEN 0 AND ?";
>     my @bind_parameters = ( $params->{'days_in_advance'} );
>     
>     my $sth = $dbh->prepare( $statement );
>diff --git a/misc/cronjobs/advance_notices.pl b/misc/cronjobs/advance_notices.pl
>index c1a95b4870..fb65cd4343 100755
>--- a/misc/cronjobs/advance_notices.pl
>+++ b/misc/cronjobs/advance_notices.pl
>@@ -214,7 +214,7 @@ GetOptions(
>             'help|?'         => \$help,
>             'man'            => \$man,
>             'library=s'      => \@branchcodes,
>-            'frombranch'     => \$frombranch,
>+            'frombranch=s'   => \$frombranch,
>             'c'              => \$confirm,
>             'n'              => \$nomail,
>             'm:i'            => \$maxdays,
>@@ -242,7 +242,7 @@ unless ($confirm) {
> }
> cronlogaction();
> 
>-my %branches = {};
>+my %branches = ();
> if (@branchcodes) {
>     %branches = map { $_ => 1 } @branchcodes;
> }
>@@ -304,14 +304,14 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) {
>                 $due_digest->{ $upcoming->{borrowernumber} }->{count}++;
>             }
>         } else {
>-        my $branchcode;
>-        if($owning_library) {
>-        $branchcode = $upcoming->{'homebranch'};
>-        } else {
>-        $branchcode = $upcoming->{'branchcode'};
>-        }
>-        # Skip this DUE if we specify list of libraries and this one is not part of it
>-        next if (@branchcodes && !$branches{$branchcode});
>+            my $branchcode;
>+            if($owning_library) {
>+                $branchcode = $upcoming->{'homebranch'};
>+            } else {
>+                $branchcode = $upcoming->{'branchcode'};
>+            }
>+            # Skip this DUE if we specify list of libraries and this one is not part of it
>+            next if (@branchcodes && !$branches{$branchcode});
> 
>             my $item = Koha::Items->find( $upcoming->{itemnumber} );
>             my $letter_type = 'DUE';
>@@ -351,14 +351,14 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) {
>                 $upcoming_digest->{ $upcoming->{borrowernumber} }->{count}++;
>             }
>         } else {
>-        my $branchcode;
>-        if($owning_library) {
>-        $branchcode = $upcoming->{'homebranch'};
>-        } else {
>-        $branchcode = $upcoming->{'branchcode'};
>-        }
>-        # Skip this PREDUE if we specify list of libraries and this one is not part of it
>-        next if (@branchcodes && !$branches{$branchcode});
>+            my $branchcode;
>+            if($owning_library) {
>+            $branchcode = $upcoming->{'homebranch'};
>+            } else {
>+            $branchcode = $upcoming->{'branchcode'};
>+            }
>+            # Skip this PREDUE if we specify list of libraries and this one is not part of it
>+            next if (@branchcodes && !$branches{$branchcode});
> 
>             my $item = Koha::Items->find( $upcoming->{itemnumber} );
>             my $letter_type = 'PREDUE';
>@@ -389,7 +389,7 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) {
>       if ($nomail) {
>         for my $letter ( @letters ) {
>             local $, = "\f";
>-            print $letter->{'content'};
>+            print $letter->{'content'}."\n";
>         }
>       }
>       else {
>diff --git a/misc/cronjobs/overdue_notices.pl b/misc/cronjobs/overdue_notices.pl
>index e9e9ac3d34..0a5c721681 100755
>--- a/misc/cronjobs/overdue_notices.pl
>+++ b/misc/cronjobs/overdue_notices.pl
>@@ -75,7 +75,7 @@ overdue_notices.pl
>    -list-all                      list all overdues
>    -date         <yyyy-mm-dd>     emulate overdues run for this date
>    -email        <email_type>     type of email that will be used. Can be 'email', 'emailpro' or 'B_email'. Repeatable.
>-   --owning                        Send notices from item homebranch library instead of issuing library
>+   --frombranch                   Set the from address for the notice to one of 'item-homebranch' or 'item-issuebranch'.
> 
> =head1 OPTIONS
> 
>@@ -184,10 +184,12 @@ use it in order to send overdues on a specific date and not Now. Format: YYYY-MM
> 
> Allows to specify which type of email will be used. Can be email, emailpro or B_email. Repeatable.
> 
>-=item B<--owning>
>+=item B<--frombranch>
> 
> Use the address information from the item homebranch library instead of the issuing library.
> 
>+Defaults to 'item-issuebranch'
>+
> =back
> 
> =head1 DESCRIPTION
>@@ -297,7 +299,7 @@ my $verbose = 0;
> my $nomail  = 0;
> my $MAX     = 90;
> my $test_mode = 0;
>-my $owning_library = 0;
>+my $frombranch = 'item-issuebranch';
> my @branchcodes; # Branch(es) passed as parameter
> my @emails_to_use;    # Emails to use for messaging
> my @emails;           # Emails given in command-line parameters
>@@ -329,7 +331,7 @@ GetOptions(
>     'borcat=s'       => \@myborcat,
>     'borcatout=s'    => \@myborcatout,
>     'email=s'        => \@emails,
>-    'owning'         => \$owning_library,
>+    'frombranch=s'   => \$frombranch,
> ) or pod2usage(2);
> pod2usage(1) if $help;
> pod2usage( -verbose => 2 ) if $man;
>@@ -339,6 +341,11 @@ if ( defined $csvfilename && $csvfilename =~ /^-/ ) {
>     warn qq(using "$csvfilename" as filename, that seems odd);
> }
> 
>+die "--frombranch takes item-homebranch or item-issuebranch only"
>+    unless ( $frombranch eq 'item-issuebranch'
>+        || $frombranch eq 'item-homebranch' );
>+my $owning_library = ( $frombranch eq 'item-homebranch' ) ? 1 : 0;
>+
> my @overduebranches    = C4::Overdues::GetBranchcodesWithOverdueRules();    # Branches with overdue rules
> my @branches;                                    # Branches passed as parameter with overdue rules
> my $branchcount = scalar(@overduebranches);
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 21886: 82668 | 86513 | 111581 | 111582 | 111679 | 111711 | 111828 | 111872 | 111873 | 112990 | 114068 | 114069 | 114070 | 114071