Description
Alex Buckley
2018-10-24 03:41:02 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 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> 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> 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 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. 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. 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. 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). 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 (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 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> 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> 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 (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. :) See bug 21683 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> 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> 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> Awesome work all! Pushed to master for 18.11 Pushed to 18.05.x for 18.05.06 Pushed to 17.11.x for 17.11.12 *** Bug 21679 has been marked as a duplicate of this bug. *** |