Bug 20562 - issue_id is not stored in accountlines for rental fees
Summary: issue_id is not stored in accountlines for rental fees
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 20572
  Show dependency treegraph
 
Reported: 2018-04-11 12:37 UTC by Nick Clemens
Modified: 2018-05-23 05:19 UTC (History)
4 users (show)

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


Attachments
Bug 20562: issue_id is not stored in accountlines for rental fees (3.06 KB, patch)
2018-04-11 17:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20562: issue_id is not stored in accountlines for rental fees (3.22 KB, patch)
2018-04-11 17:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20562: issue_id is not stored in accountlines for rental fees (3.22 KB, patch)
2018-04-11 17:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20562: issue_id is not stored in accountlines for rental fees (3.25 KB, patch)
2018-04-12 13:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20562: issue_id is not stored in accountlines for rental fees (3.30 KB, patch)
2018-04-16 14:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20562: Pass the Koha::Checkout object to AddIssuingCharge (3.68 KB, patch)
2018-04-16 16:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20562: Pass the Koha::Checkout object to AddIssuingCharge (3.72 KB, patch)
2018-04-16 18:47 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20562: Fix unit test (1.18 KB, patch)
2018-04-19 18:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20562: Fix Returns.t test (2.34 KB, patch)
2018-04-19 19:22 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 M. 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 ?