Bug 21641

Summary: Software error when checking out an item with a charge associated with it
Product: Koha Reporter: Alex Buckley <alexbuckley>
Component: CirculationAssignee: Alex Buckley <alexbuckley>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: fridolin.somers, gmcharlt, josef.moravec, kyle.m.hall, martin.renvoize, mtompset, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14825
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21683
Change sponsored?: Sponsored Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 21641: Added C4::Accounts to getnextacctno() call
Bug 21641: Added C4::Accounts to getnextacctno() call
Bug 21641: Added C4::Accounts to getnextacctno() call
Bug 21641: (follow-up) Address additional call
Bug 21641: (follow-up) Address wrong call method
Bug 21641: (follow-up) Address additional call
Bug 21641: (follow-up) Address wrong call method
Bug 21641: Added C4::Accounts to getnextacctno() call
Bug 21641: (follow-up) Address additional call
Bug 21641: (follow-up) Address wrong call method

Description Alex Buckley 2018-10-24 03:41:02 UTC
When you try to check out an item with a charge associated with it the following error is thrown as the C4::Circulation->AddIssuingCharge() is executed:

Undefined subroutine &C4::Circulation::getnextacctno called at /home/vagrant/kohaclone/C4/Circulation.pm line 3194


The getnextacctno function is in C4::Accounts. This module is in a use pragma at the top of C4/Circulation.pm
Comment 1 Alex Buckley 2018-10-24 03:44:20 UTC
Created attachment 81046 [details] [review]
Bug 21641: Added C4::Accounts to getnextacctno() call

Test plan:
1. Fetch and checkout the master branch
2. Checkout an item with a rental charge associated with it to a user
and notice an error is thrown
3. Apply patch
4. Restart memcached and plack
5. Repeat steps 1 and 2 and notice the checkout happens successfully

Sponsored-By: Catalyst IT
Comment 2 Andrew Isherwood 2018-10-25 12:34:09 UTC
Created attachment 81138 [details] [review]
Bug 21641: Added C4::Accounts to getnextacctno() call

Test plan:
1. Fetch and checkout the master branch
2. Checkout an item with a rental charge associated with it to a user
and notice an error is thrown
3. Apply patch
4. Restart memcached and plack
5. Repeat steps 1 and 2 and notice the checkout happens successfully

Sponsored-By: Catalyst IT

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Comment 3 Andrew Isherwood 2018-10-25 12:35:20 UTC
Created attachment 81139 [details] [review]
Bug 21641: Added C4::Accounts to getnextacctno() call

Test plan:
1. Fetch and checkout the master branch
2. Checkout an item with a rental charge associated with it to a user
and notice an error is thrown
3. Apply patch
4. Restart memcached and plack
5. Repeat steps 1 and 2 and notice the checkout happens successfully

Sponsored-By: Catalyst IT

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Comment 4 Andrew Isherwood 2018-10-25 12:35:23 UTC
Created attachment 81140 [details] [review]
Bug 21641: (follow-up) Address additional call

This patch makes the same change as the main patch, just for an
additional occurance of getnextacctno
Comment 5 Andrew Isherwood 2018-10-25 12:35:34 UTC
Hi Alex

I applied the patch and received the error:

Undefined subroutine &C4::Circulation::getnextacctno called at /usr/share/koha/C4/Circulation.pm line 2854.

I've added a patch to address this.

I've signed off your patch, if you sign off mine we should be good to go.
Comment 6 Mark Tompsett 2018-10-25 12:45:31 UTC
NOTE: This is why I asked about the the order of 'use' on the developers mailing list. I'm pretty sure this could be solved with an order change.
Comment 7 Mark Tompsett 2018-10-25 12:49:49 UTC
Also, I've been pondering... shouldn't this getnextacctno be in Koha::Account::Line(s)? It is just using the AccountLines table no relationships. I think it is a perfect candidate for moving.

AND... when is getnextacctno called? Almost always in the midst of generating a new account line. Why not remove the necessity for accountno being passed in a ->new() and determine the account line number id at save time? This means some code clean up, but I think it is better in the long run than this short term patch.
Comment 8 Mark Tompsett 2018-10-25 13:01:07 UTC
Jonathan Druart thinks this is sufficient for now. Failing, because wrong call method.

C4::Accounts::getnextacctno and C4::Accounts->getnextacctno are different.
The latter passes ($self, $param), the former is only ($param).
Comment 9 Andrew Isherwood 2018-10-25 13:19:38 UTC
Created attachment 81149 [details] [review]
Bug 21641: (follow-up) Address wrong call method

This patch substitutes C4::Accounts->getnextacctno for
C4::Accounts::getnextacctno since getnextacctno is only expecting to be
passed a borrowernumber
Comment 10 Tomás Cohen Arazi (tcohen) 2018-10-25 14:06:38 UTC
(In reply to M. Tompsett from comment #7)
> Also, I've been pondering... shouldn't this getnextacctno be in
> Koha::Account::Line(s)? It is just using the AccountLines table no
> relationships. I think it is a perfect candidate for moving.
> 
> AND... when is getnextacctno called? Almost always in the midst of
> generating a new account line. Why not remove the necessity for accountno
> being passed in a ->new() and determine the account line number id at save
> time? This means some code clean up, but I think it is better in the long
> run than this short term patch.

The accountno field and the associated functions should be removed. I don't think we should move that anywhere. :-D
Comment 11 Alex Buckley 2018-10-25 20:24:13 UTC
Created attachment 81200 [details] [review]
Bug 21641: (follow-up) Address additional call

This patch makes the same change as the main patch, just for an
additional occurance of getnextacctno

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 12 Alex Buckley 2018-10-25 20:24:34 UTC
Created attachment 81201 [details] [review]
Bug 21641: (follow-up) Address wrong call method

This patch substitutes C4::Accounts->getnextacctno for
C4::Accounts::getnextacctno since getnextacctno is only expecting to be
passed a borrowernumber

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 13 Alex Buckley 2018-10-25 20:25:54 UTC
Hi Andrew

Thanks for signing off on my patch and writing the followups addressing the incorrect call method. I have tested and everything is working correctly so have signed off. 

Cheers,
Alex
Comment 14 Mark Tompsett 2018-10-25 21:57:15 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> The accountno field and the associated functions should be removed. I don't
> think we should move that anywhere. :-D

Hmmm... we already have accountlines_id. You're probably right, cut and refactor anything that used it. :)
Comment 15 Josef Moravec 2018-10-26 11:28:07 UTC
See bug 21683
Comment 16 Tomás Cohen Arazi (tcohen) 2018-10-28 13:16:32 UTC
Created attachment 81408 [details] [review]
Bug 21641: Added C4::Accounts to getnextacctno() call

Test plan:
1. Fetch and checkout the master branch
2. Checkout an item with a rental charge associated with it to a user
and notice an error is thrown
3. Apply patch
4. Restart memcached and plack
5. Repeat steps 1 and 2 and notice the checkout happens successfully

Sponsored-By: Catalyst IT

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Tomás Cohen Arazi (tcohen) 2018-10-28 13:16:40 UTC
Created attachment 81409 [details] [review]
Bug 21641: (follow-up) Address additional call

This patch makes the same change as the main patch, just for an
additional occurance of getnextacctno

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi (tcohen) 2018-10-28 13:16:48 UTC
Created attachment 81410 [details] [review]
Bug 21641: (follow-up) Address wrong call method

This patch substitutes C4::Accounts->getnextacctno for
C4::Accounts::getnextacctno since getnextacctno is only expecting to be
passed a borrowernumber

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Nick Clemens (kidclamp) 2018-10-29 11:39:15 UTC
Awesome work all!

Pushed to master for 18.11
Comment 20 Martin Renvoize (ashimema) 2018-11-08 15:31:56 UTC
Pushed to 18.05.x for 18.05.06
Comment 21 Fridolin Somers 2018-11-26 06:42:51 UTC
Pushed to 17.11.x for 17.11.12
Comment 22 Katrin Fischer 2019-02-04 22:56:48 UTC
*** Bug 21679 has been marked as a duplicate of this bug. ***