Attachment 103336 Details for Bug 22393 – Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice

[patch] Bug 22393: Remove last remaining use of C4::Accounts::manualinvoice

Bug-22393-Remove-last-remaining-use-of-C4Accountsm.patch (text/plain), 13.01 KB, created by Martin Renvoize (ashimema) on 2020-04-21 08:56:21 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2020-04-21 08:56:21 UTC

Size: 13.01 KB

patch

obsolete

>From bfc74458218e73bb9d83d11aa2d9ca8672ed687c Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Date: Mon, 10 Feb 2020 14:55:13 +0000
>Subject: [PATCH] Bug 22393: Remove last remaining use of
> C4::Accounts::manualinvoice
>
>This patch re-arranges the manualinvoice controller script to clarify
>code flow, replaces the last call to C4::Accounts::manualinvoice with a
>call to Koha::Accounts->add_debit wrapped in a try catch block and also
>adds a check on passed barcodes when the invoice type is 'LOST' so it
>can link the subsequently created accountline to the item and issue.
>
>Test plan
>1/ Add a manual invoice (without entering a barcode)
>2/ Add a manual invoice with a valid barcode (Not a LOST type)
>3/ Add a manual invoice with a valid, but old, barcode (Not a LOST type)
>4/ Add a manual invoice with an invalid barcode, not an error is
>displayed
>5/ Add a manual invoice with type 'LOST' and a valid barcode for a
>checkout your user has had checked out
>6/ Add a manual invoice with type 'LOST' and a valid barcode, but not
>one that will match a checkout for your user. Note an error is displayed
>7/ When errors are displayed, note the form contains data from the
>previous submission so you can just correct the error rather than
>re-enter all data.
>8/ Signoff
>---
> .../prog/en/modules/members/maninvoice.tt     |  52 ++---
> members/maninvoice.pl                         | 186 ++++++++++++------
> 2 files changed, 153 insertions(+), 85 deletions(-)
>
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/maninvoice.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/maninvoice.tt
>index 864429e425..dfbc5e2338 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/maninvoice.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/maninvoice.tt
>@@ -36,29 +36,38 @@
>                 </ul>
>                 <div class="tabs-container">
> 
>-                [% IF ( ERROR ) %]
>-                [% IF ( ITEMNUMBER ) %]
>-                  ERROR an invalid itemnumber was entered, please hit back and try again
>+                [% IF error == 'itemnumber' %]
>+                    <div id="error_message" class="dialog alert">
>+                        Error: Invalid barcode entered, please try again
>+                    </div>
>+                [% ELSIF error %]
>+                    <div id="error_message" class="dialog alert">
>+                        An error occured, please try again: [% error %]
>+                    </div>
>                 [% END %]
>-                [% ELSE %]
>-                <form action="/cgi-bin/koha/members/maninvoice.pl" method="post" id="maninvoice"><input type="hidden" name="borrowernumber" id="borrowernumber" value="[% patron.borrowernumber | html %]" />
>+                <form action="/cgi-bin/koha/members/maninvoice.pl" method="post" id="maninvoice">
>+                    <input type="hidden" name="borrowernumber" id="borrowernumber" value="[% patron.borrowernumber | html %]" />
>                     <input type="hidden" name="csrf_token" value="[% csrf_token | html %]" />
>-            <fieldset class="rows">
>-            <legend>Manual invoice</legend>
>-            <ol>
>-                      <li>
>-                        <label for="type">Type: </label>
>-                        <select name="type" id="type">
>-                          [% FOREACH debit_type IN debit_types %]
>-                            <option value="[% debit_type.code | html %]">[% debit_type.description | html %]</option>
>-                          [% END %]
>-                        </select>
>-                      </li>
>-              <li><label for="barcode">Barcode: </label><input type="text" name="barcode" id="barcode" /></li>
>-                      <li><label for="desc">Description: </label><input type="text" name="desc" id="desc" size="50" /></li>
>-                      <li><label for="note">Note: </label><input type="text" name="note" size="50" id="note" /></li>
>-                      <li><label for="amount">Amount: </label><input type="number" name="amount" id="amount" required="required" value="" step="any" min="0" /> Example: 5.00</li>
>-            </ol>
>+                    <fieldset class="rows">
>+                        <legend>Manual invoice</legend>
>+                        <ol>
>+                            <li>
>+                                <label for="type">Type: </label>
>+                                <select name="type" id="type">
>+                                    [% FOREACH debit_type IN debit_types %]
>+                                    [% IF debit_type.code == type %]
>+                                    <option value="[% debit_type.code | html %]" selected="selected">[% debit_type.description | html %]</option>
>+                                    [% ELSE %]
>+                                    <option value="[% debit_type.code | html %]">[% debit_type.description | html %]</option>
>+                                    [% END %]
>+                                    [% END %]
>+                                </select>
>+                            </li>
>+                            <li><label for="barcode">Barcode: </label><input type="text" name="barcode" id="barcode" value="[% barcode %]" /></li>
>+                            <li><label for="desc">Description: </label><input type="text" name="desc" id="desc" size="50" value="[% desc %]" /></li>
>+                            <li><label for="note">Note: </label><input type="text" name="note" size="50" id="note" value="[% note %]" /></li>
>+                            <li><label for="amount">Amount: </label><input type="number" name="amount" id="amount" required="required" step="any" min="0" value="[% amount %]" /> Example: 5.00</li>
>+                        </ol>
>                     </fieldset>
>                     <fieldset class="action">
>                         <button type="submit" name="add" value="save">Save</button>
>@@ -67,7 +76,6 @@
>                     </fieldset>
>                 </form>
> 
>-                [% END %]
>                 </div>
>             </div>
> 
>diff --git a/members/maninvoice.pl b/members/maninvoice.pl
>index c39a323cae..3b4736d8d3 100755
>--- a/members/maninvoice.pl
>+++ b/members/maninvoice.pl
>@@ -23,6 +23,7 @@
> # along with Koha; if not, see <http://www.gnu.org/licenses>.
> 
> use Modern::Perl;
>+use Try::Tiny;
> 
> use C4::Auth;
> use C4::Output;
>@@ -32,8 +33,11 @@ use C4::Accounts;
> use C4::Items;
> use Koha::Token;
> 
>-use Koha::Items;
> use Koha::Patrons;
>+use Koha::Items;
>+use Koha::Old::Items;
>+use Koha::Checkouts;
>+use Koha::Old::Checkouts;
> 
> use Koha::Patron::Categories;
> use Koha::Account::DebitTypes;
>@@ -60,7 +64,30 @@ unless ($patron) {
>     exit;
> }
> 
>+my $logged_in_user = Koha::Patrons->find($loggedinuser);
>+output_and_exit_if_error(
>+    $input, $cookie,
>+    $template,
>+    {
>+        module         => 'members',
>+        logged_in_user => $logged_in_user,
>+        current_patron => $patron
>+    }
>+);
>+
> my $library_id = C4::Context->userenv->{'branch'};
>+my $desc       = $input->param('desc');
>+my $amount     = $input->param('amount');
>+my $note       = $input->param('note');
>+my $debit_type = $input->param('type');
>+my $barcode    = $input->param('barcode');
>+$template->param(
>+    desc    => $desc,
>+    amount  => $amount,
>+    note    => $note,
>+    type    => $debit_type,
>+    barcode => $barcode
>+);
> 
> my $add = $input->param('add');
> if ($add) {
>@@ -72,76 +99,109 @@ if ($add) {
>         }
>       );
> 
>-# Note: If the logged in user is not allowed to see this patron an invoice can be forced
>-# Here we are trusting librarians not to hack the system
>-    my $barcode = $input->param('barcode');
>-    my $itemnum;
>+    # Note: If the logged in user is not allowed to see this patron an invoice can be forced
>+    # Here we are trusting librarians not to hack the system
>+    my $desc       = $input->param('desc');
>+    my $amount     = $input->param('amount');
>+    my $note       = $input->param('note');
>+    my $debit_type = $input->param('type');
>+
>+    # If barcode is passed, attempt to find the associated item
>+    my $failed;
>+    my $item_id;
>+    my $issue_id;
>     if ($barcode) {
>         my $item = Koha::Items->find( { barcode => $barcode } );
>-        $itemnum = $item->itemnumber if $item;
>-    }
>-    my $desc   = $input->param('desc');
>-    my $amount = $input->param('amount');
>-    my $type   = $input->param('type');
>-    my $note   = $input->param('note');
>-    my $error =
>-      C4::Accounts::manualinvoice( $borrowernumber, $itemnum, $desc, $type,
>-        $amount, $note );
>-    if ($error) {
>-        if ( $error =~ /FOREIGN KEY/ && $error =~ /itemnumber/ ) {
>-            $template->param( 'ITEMNUMBER' => 1 );
>+        if ($item) {
>+            $item_id = $item->itemnumber;
>         }
>-        $template->param(
>-            csrf_token => Koha::Token->new->generate_csrf(
>-                { session_id => scalar $input->cookie('CGISESSID') }
>-            )
>-        );
>-        $template->param( 'ERROR' => $error );
>-        output_html_with_http_headers $input, $cookie, $template->output;
>-    }
>-    else {
>-
>-        if ( C4::Context->preference('AccountAutoReconcile') ) {
>-            $patron->account->reconcile_balance;
>+        else {
>+            $item = Koha::Old::Items->find( { barcode => $barcode } );
>+            if ($item) {
>+                $item_id = $item->itemnumber;
>+            }
>+            else {
>+                $template->param( error => 'itemnumber' );
>+                $failed = 1;
>+            }
>         }
> 
>-        if ($add eq 'save and pay') {
>-            print $input->redirect(
>-                "/cgi-bin/koha/members/pay.pl?borrowernumber=$borrowernumber"
>-            );
>-        } else {
>-            print $input->redirect(
>-                "/cgi-bin/koha/members/boraccount.pl?borrowernumber=$borrowernumber"
>+        if ( ( $debit_type eq 'LOST' ) && $item_id ) {
>+            my $checkouts = Koha::Checkouts->search(
>+                {
>+                    itemnumber     => $item_id,
>+                    borrowernumber => $borrowernumber
>+                }
>             );
>+            my $checkout =
>+                $checkouts->count
>+              ? $checkouts->next
>+              : Koha::Old::Checkouts->search(
>+                {
>+                    itemnumber     => $item_id,
>+                    borrowernumber => $borrowernumber
>+                },
>+                { order_by => { -desc => 'returndate' }, rows => 1 }
>+            )->next;
>+            $issue_id = $checkout ? $checkout->issue_id : undef;
>         }
>-
>-        exit;
>     }
>-}
>-else {
> 
>-    my $logged_in_user = Koha::Patrons->find($loggedinuser)
>-      or die "Not logged in";
>-    output_and_exit_if_error(
>-        $input, $cookie,
>-        $template,
>-        {
>-            module         => 'members',
>-            logged_in_user => $logged_in_user,
>-            current_patron => $patron
>+    unless ($failed) {
>+        try {
>+            $patron->account->add_debit(
>+                {
>+                    amount      => $amount,
>+                    description => $desc,
>+                    note        => $note,
>+                    user_id     => $logged_in_user->borrowernumber,
>+                    interface   => 'intranet',
>+                    library_id  => $library_id,
>+                    type        => $debit_type,
>+                    item_id     => $item_id,
>+                    issue_id    => $issue_id
>+                }
>+            );
>+
>+            if ( C4::Context->preference('AccountAutoReconcile') ) {
>+                $patron->account->reconcile_balance;
>+            }
>+
>+            if ( $add eq 'save and pay' ) {
>+                print $input->redirect(
>+                    "/cgi-bin/koha/members/pay.pl?borrowernumber=$borrowernumber"
>+                );
>+            }
>+            else {
>+                print $input->redirect(
>+                    "/cgi-bin/koha/members/boraccount.pl?borrowernumber=$borrowernumber"
>+                );
>+            }
>+
>+            exit;
>+        }
>+        catch {
>+            my $error = $_;
>+            if ( ref($error) eq 'Koha::Exceptions::Object::FKConstraint' ) {
>+                $template->param( error => $error->broken_fk );
>+            }
>+            else {
>+                $template->param( error => $error );
>+            }
>         }
>-    );
>-
>-    my @debit_types = Koha::Account::DebitTypes->search_with_library_limits(
>-        { can_be_invoiced => 1, archived => 0 },
>-        {}, $library_id );
>-    $template->param( debit_types => \@debit_types );
>-    $template->param(
>-        csrf_token => Koha::Token->new->generate_csrf(
>-            { session_id => scalar $input->cookie('CGISESSID') }
>-        ),
>-        patron    => $patron,
>-        finesview => 1,
>-    );
>-    output_html_with_http_headers $input, $cookie, $template->output;
>+    }
> }
>+
>+my @debit_types = Koha::Account::DebitTypes->search_with_library_limits(
>+  { can_be_invoiced => 1, archived => 0 },
>+  {}, $library_id );
>+
>+$template->param(
>+  debit_types => \@debit_types,
>+  csrf_token  => Koha::Token->new->generate_csrf(
>+      { session_id => scalar $input->cookie('CGISESSID') }
>+  ),
>+  patron    => $patron,
>+  finesview => 1,
>+  );
>+output_html_with_http_headers $input, $cookie, $template->output;
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 22393: 85516 | 95298 | 95303 | 95308 | 95309 | 95325 | 95326 | 98662 | 98663 | 103336 | 103382 | 103897 | 103898 | 106773 | 108330 | 108331 | 108332