To recreate: 1 - Assign a charge to an item type 2 - Checkout an item of that type to a patron 3 - View the accountlines table for that patron SELECT * FROM accountlines WHERE accounttype='Rent' and borrowernumber=##; 4 - Note there is no issue_id
Created attachment 74019 [details] [review] Bug 20562: issue_id is not stored in accountlines for rental fees To recreate: 1 - Assign a charge to an item type 2 - Checkout an item of that type to a patron 3 - View the accountlines table for that patron SELECT * FROM accountlines WHERE accounttype='Rent' and borrowernumber=##; 4 - Note there is no issue_id Test Plan: 1) Apply this patch 2) prove t/db_dependent/Circulation/issue.t
Created attachment 74020 [details] [review] Bug 20562: issue_id is not stored in accountlines for rental fees To recreate: 1 - Assign a charge to an item type 2 - Checkout an item of that type to a patron 3 - View the accountlines table for that patron SELECT * FROM accountlines WHERE accounttype='Rent' and borrowernumber=##; 4 - Note there is no issue_id Test Plan: 1) Apply this patch 2) prove t/db_dependent/Circulation/issue.t
Created attachment 74021 [details] [review] Bug 20562: issue_id is not stored in accountlines for rental fees Test Plan: 1) Apply this patch 2) Assign a charge to an item type 3) Checkout an item of that type to a patron 4) View the accountlines table for that patron SELECT * FROM accountlines WHERE accounttype='Rent' and borrowernumber=##; 5) Note there is an issue_id Or 1) Apply this patch 2) prove t/db_dependent/Circulation/issue.t
Is it a regression of it never worked?
(In reply to Jonathan Druart from comment #4) > Is it a regression of it never worked? I would think not, but I suppose you could argue it either way philosophically.
Created attachment 74102 [details] [review] Bug 20562: issue_id is not stored in accountlines for rental fees Test Plan: 1) Apply this patch 2) Assign a charge to an item type 3) Checkout an item of that type to a patron 4) View the accountlines table for that patron SELECT * FROM accountlines WHERE accounttype='Rent' and borrowernumber=##; 5) Note there is an issue_id Or 1) Apply this patch 2) prove t/db_dependent/Circulation/issue.t Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 74240 [details] [review] Bug 20562: issue_id is not stored in accountlines for rental fees Test Plan: 1) Apply this patch 2) Assign a charge to an item type 3) Checkout an item of that type to a patron 4) View the accountlines table for that patron SELECT * FROM accountlines WHERE accounttype='Rent' and borrowernumber=##; 5) Note there is an issue_id Or 1) Apply this patch 2) prove t/db_dependent/Circulation/issue.t Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 74250 [details] [review] Bug 20562: Pass the Koha::Checkout object to AddIssuingCharge We do not need to pass all those parameters, just the checkout object is enough.
Could you take a look at the follow-up? It does not seem useful to add one more parameter when it's possible to remove one :) See bug 20591 to understand why I removed the DBI->last_insert_id call
Comment on attachment 74250 [details] [review] Bug 20562: Pass the Koha::Checkout object to AddIssuingCharge Review of attachment 74250 [details] [review]: ----------------------------------------------------------------- Reading the code - does make sense. Testing - does work. Thanks Jonathan! ::: C4/Circulation.pm @@ +3174,2 @@ > > + # FIXME What if checkout does not exist? Then we should throw an exception. I think this will need some refactoring during the move to Koha::Objects in future.
Created attachment 74253 [details] [review] Bug 20562: Pass the Koha::Checkout object to AddIssuingCharge We do not need to pass all those parameters, just the checkout object is enough. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Pushed to master for 18.05, thanks to everybody involved!
commit dc5d3b9abd34b0e80794fff7029809b7438bcefa Bug 20562: issue_id is not stored in accountlines for rental fees breaks t/db_dependent/Circulation/Returns.t # Failed test 'No account lines should exist for this issue_id, patron should not have been charged' # at t/db_dependent/Circulation/Returns.t line 330. # got: '1' # expected: '0' # Looks like you failed 1 test of 6. t/db_dependent/Circulation/Returns.t .. 4/5 # Failed test 'Handle ids duplication' # at t/db_dependent/Circulation/Returns.t line 333. # Looks like you failed 1 test of 5. Please fix ASAP
(In reply to Jonathan Druart from comment #13) > commit dc5d3b9abd34b0e80794fff7029809b7438bcefa > Bug 20562: issue_id is not stored in accountlines for rental fees > > breaks t/db_dependent/Circulation/Returns.t > > # Failed test 'No account lines should exist for this issue_id, patron > should not have been charged' > # at t/db_dependent/Circulation/Returns.t line 330. > # got: '1' > # expected: '0' > # Looks like you failed 1 test of 6. > t/db_dependent/Circulation/Returns.t .. 4/5 > # Failed test 'Handle ids duplication' > # at t/db_dependent/Circulation/Returns.t line 333. > # Looks like you failed 1 test of 5. > > > Please fix ASAP I'm trying to understand what this is actually testing. Can you give me any context? The unit tests aren't telling me much.
Ah, the unit test assumed that his was implemented already! That still doesn't explain the expected behavior of the test. It seems like that particular test should expect an accountline to exist and *not* for none to exist!
Created attachment 74584 [details] [review] Bug 20562: Fix unit test
Created attachment 74586 [details] [review] Bug 20562: Fix Returns.t test
Pushed to stable for 17.11.6
Patches does not apply on 17.05.x. Since its an important impact, I prefer not to rebase myself. Could you provide rebased patches ?