Bug 21641 - Software error when checking out an item with a charge associated with it
Summary: Software error when checking out an item with a charge associated with it
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-24 03:41 UTC by Alex Buckley
Modified: 2018-11-28 10:36 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 21641: Added C4::Accounts to getnextacctno() call (1.06 KB, patch)
2018-10-24 03:44 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21641: Added C4::Accounts to getnextacctno() call (1.13 KB, patch)
2018-10-25 12:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21641: Added C4::Accounts to getnextacctno() call (1.13 KB, patch)
2018-10-25 12:35 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21641: (follow-up) Address additional call (1023 bytes, patch)
2018-10-25 12:35 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21641: (follow-up) Address wrong call method (1.42 KB, patch)
2018-10-25 13:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21641: (follow-up) Address additional call (1.05 KB, patch)
2018-10-25 20:24 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21641: (follow-up) Address wrong call method (1.47 KB, patch)
2018-10-25 20:24 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21641: Added C4::Accounts to getnextacctno() call (1.18 KB, patch)
2018-10-28 13:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21641: (follow-up) Address additional call (1.11 KB, patch)
2018-10-28 13:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21641: (follow-up) Address wrong call method (1.53 KB, patch)
2018-10-28 13:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 M. 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 M. 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 M. 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 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 M. 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 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 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 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 2018-10-29 11:39:15 UTC
Awesome work all!

Pushed to master for 18.11
Comment 20 Martin Renvoize 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