Bug 20562

Summary: issue_id is not stored in accountlines for rental fees
Product: Koha Reporter: Nick Clemens <nick>
Component: Fines and feesAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: major    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, josef.moravec, kyle
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20591
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 20572    
Attachments: Bug 20562: issue_id is not stored in accountlines for rental fees
Bug 20562: issue_id is not stored in accountlines for rental fees
Bug 20562: issue_id is not stored in accountlines for rental fees
Bug 20562: issue_id is not stored in accountlines for rental fees
Bug 20562: issue_id is not stored in accountlines for rental fees
Bug 20562: Pass the Koha::Checkout object to AddIssuingCharge
Bug 20562: Pass the Koha::Checkout object to AddIssuingCharge
Bug 20562: Fix unit test
Bug 20562: Fix Returns.t test

Description Nick Clemens 2018-04-11 12:37:53 UTC
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
Comment 1 Kyle M Hall 2018-04-11 17:36:56 UTC
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
Comment 2 Kyle M Hall 2018-04-11 17:42:03 UTC
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
Comment 3 Kyle M Hall 2018-04-11 17:43:46 UTC
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
Comment 4 Jonathan Druart 2018-04-11 17:46:30 UTC
Is it a regression of it never worked?
Comment 5 Kyle M Hall 2018-04-11 18:44:31 UTC
(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.
Comment 6 Mark Tompsett 2018-04-12 13:47:03 UTC
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>
Comment 7 Josef Moravec 2018-04-16 14:20:20 UTC
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>
Comment 8 Jonathan Druart 2018-04-16 16:50:05 UTC
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.
Comment 9 Jonathan Druart 2018-04-16 16:51:15 UTC
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 10 Josef Moravec 2018-04-16 18:46:52 UTC
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.
Comment 11 Josef Moravec 2018-04-16 18:47:03 UTC
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>
Comment 12 Jonathan Druart 2018-04-19 15:42:58 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 13 Jonathan Druart 2018-04-19 16:21:39 UTC
  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
Comment 14 Kyle M Hall 2018-04-19 18:23:26 UTC
(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.
Comment 15 Kyle M Hall 2018-04-19 18:30:07 UTC
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!
Comment 16 Kyle M Hall 2018-04-19 18:36:26 UTC
Created attachment 74584 [details] [review]
Bug 20562: Fix unit test
Comment 17 Jonathan Druart 2018-04-19 19:22:19 UTC
Created attachment 74586 [details] [review]
Bug 20562: Fix Returns.t test
Comment 18 Nick Clemens 2018-05-22 11:06:57 UTC
Pushed to stable for 17.11.6
Comment 19 Fridolin Somers 2018-05-23 05:19:58 UTC
Patches does not apply on 17.05.x.
Since its an important impact, I prefer not to rebase myself.
Could you provide rebased patches ?