Bug 18677

Summary: issue_id is not added to accountlines for lost item fees
Product: Koha Reporter: Nick Clemens <nick>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: cbrannon, fridolin.somers, gmcharlt, jesse, jonathan.druart, kyle.m.hall, m.de.rooy, maksim.sen, martin.renvoize, mtompset, veron
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21848
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 18677: issue_id is not added to accountlines for lost item fees
Bug 18677: issue_id is not added to accountlines for lost item fees
Bug 18677: issue_id is not added to accountlines for lost item fees
Bug 18677: issue_id is not added to accountlines for lost item fees
Bug 18677: Make the tests pass
Bug 18677: Remove new issue_id param from charlostitem
Bug 18677: issue_id is not added to accountlines for lost item fees
Bug 18677: Make the tests pass
Bug 18677: Remove new issue_id param from charlostitem
Bug 18677: issue_id is not added to accountlines for lost item fees
Bug 18677: Make the tests pass
Bug 18677: Remove new issue_id param from charlostitem
Bug 18677: (QA follow-up) Trivial fixes to Chargelostitem.t

Description Nick Clemens 2017-05-26 10:39:38 UTC

    
Comment 1 Kyle M Hall 2018-04-11 18:10:01 UTC
Created attachment 74022 [details] [review]
Bug 18677: issue_id is not added to accountlines for lost item fees

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t
Comment 2 Mark Tompsett 2018-04-12 13:58:34 UTC
Comment on attachment 74022 [details] [review]
Bug 18677: issue_id is not added to accountlines for lost item fees

Review of attachment 74022 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Accounts.t
@@ -528,2 @@
>      $lostfine = Koha::Account::Lines->find({ borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber1, accounttype => 'L' });
> -    $procfee  = Koha::Account::Lines->find({ borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber1, accounttype => 'PF' });

This seems like an accidental deletion.

@@ -599,3 @@
>      $lostfine = Koha::Account::Lines->find({ borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber2, accounttype => 'L' });
>      $procfee  = Koha::Account::Lines->find({ borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber2, accounttype => 'PF' });
> -    is( $lostfine->amount, "6.120000" , "Lost fine equals replacementcost when pref on and default set");

This seems like an accidental deletion.
Also, I prefer the more format agnostic +0 forced typecast.
Comment 3 Mark Tompsett 2018-04-13 04:48:35 UTC
Comment on attachment 74022 [details] [review]
Bug 18677: issue_id is not added to accountlines for lost item fees

Review of attachment 74022 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +3633,5 @@
>              defined($fix) or warn "_FixOverduesOnReturn($borrowernumber, $itemnumber...) failed!";  # zero is OK, check defined
>          }
>          if (C4::Context->preference('WhenLostChargeReplacementFee')){
> +            my $checkout = Koha::Checkouts->find({ itemnumber => $itemnumber });
> +            C4::Accounts::chargelostitem($borrowernumber, $itemnumber, $checkout->id, $issues->{'replacementprice'}, "Lost Item $issues->{'title'} $issues->{'barcode'}");

There is no test code which triggers this change as far as I can tell. Please include a test which does.
Comment 4 Kyle M Hall 2018-04-13 13:23:23 UTC
Created attachment 74165 [details] [review]
Bug 18677: issue_id is not added to accountlines for lost item fees

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t
Comment 5 Maksim Sen 2018-04-13 18:44:02 UTC
I got this error while trying to do the test:

t/db_dependent/Accounts.t .. 18/25 DBD::mysql::st execute failed: Duplicate entry '2147483647' for key 'PRIMARY' [for Statement "INSERT INTO `biblio` ( `abstract`, `author`, `copyrightdate`, `datecreated`, `frameworkcode`, `notes`, `serial`, `seriestitle`, `timestamp`, `title`, `unititle`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='SqTkFqA', 1='Hl8u8G9qy', 2=26992, 3='2018-04-13', 4='A1mS', 5='MrVzEx', 6=9, 7='ya33U7dw6_', 8='2018-04-13 18:42:47', 9='cyTtFLH', 10='twGKLqjb'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.
    # No tests run!

#   Failed test 'No tests run for subtest "More Koha::Account::pay tests"'
#   at t/db_dependent/Accounts.t line 400.
DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '2147483647' for key 'PRIMARY' at /inlibro/git/koha-master-dev-inlibro/t/lib/TestBuilder.pm line 288
# Looks like your test exited with 255 just after 20.
t/db_dependent/Accounts.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 6/25 subtests 

Test Summary Report
-------------------
t/db_dependent/Accounts.t (Wstat: 65280 Tests: 20 Failed: 1)
  Failed test:  20
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 25 tests but ran 20.
Files=1, Tests=20,  6 wallclock secs ( 0.04 usr  0.00 sys +  2.54 cusr  0.16 csys =  2.74 CPU)
Result: FAIL
Comment 6 Mark Tompsett 2018-04-18 02:01:39 UTC
While I think Maksim Sim's error is bad data, I'll mark this Failed QA due to comments #2 and #3. #3 is a request for test coverage of a code change in a C4 library. Given how tests make code changes easier to check and our current RM, please provide them.
Comment 7 Kyle M Hall 2018-04-18 14:06:06 UTC
(In reply to M. Tompsett from comment #6)
> While I think Maksim Sim's error is bad data, I'll mark this Failed QA due
> to comments #2 and #3. #3 is a request for test coverage of a code change in
> a C4 library. Given how tests make code changes easier to check and our
> current RM, please provide them.

#2 was fixed with the last patch uploaded.

For #3 there is already a unit test added in Circulation.t in this patch.
Comment 8 Kyle M Hall 2018-04-18 14:06:17 UTC
(In reply to M. Tompsett from comment #6)
> While I think Maksim Sim's error is bad data, I'll mark this Failed QA due
> to comments #2 and #3. #3 is a request for test coverage of a code change in
> a C4 library. Given how tests make code changes easier to check and our
> current RM, please provide them.

#2 was fixed with the last patch uploaded.

For #3 there is already a unit test added in Circulation.t in this patch.
Comment 9 Martin Renvoize 2018-08-13 10:54:36 UTC
I get test errors after trying to resolve merge conflicts when applying this.. requesting a rebase.
Comment 10 Kyle M Hall 2018-10-25 12:18:16 UTC
Created attachment 81133 [details] [review]
Bug 18677: issue_id is not added to accountlines for lost item fees

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t
Comment 11 Martin Renvoize 2018-10-31 14:55:39 UTC
Created attachment 81732 [details] [review]
Bug 18677: issue_id is not added to accountlines for lost item fees

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2018-10-31 14:56:07 UTC
Works well for me: Signing off
Comment 13 Jonathan Druart 2018-10-31 18:27:38 UTC
I think there is something wrong. Why do you pass the issue_id to chargelostitem when we can guess it from the itemnumber given?
Comment 14 Mark Tompsett 2018-10-31 19:20:23 UTC
Comment on attachment 81732 [details] [review]
Bug 18677: issue_id is not added to accountlines for lost item fees

Review of attachment 81732 [details] [review]:
-----------------------------------------------------------------

::: C4/Accounts.pm
@@ +99,4 @@
>  
>  sub chargelostitem{
>      my $dbh = C4::Context->dbh();
> +    my ($borrowernumber, $itemnumber, $issue_id, $amount, $description) = @_;

change of parameters needs to make sure all places are changed. I can't get my Windows VM up, and I don't have time to boot my Ubuntu machine. Could someone please check C4::Overdues? I think this change might break there, which is why JD's comment about not passing it, and looking it up here makes sense to me.
Comment 15 Mark Tompsett 2018-10-31 19:20:51 UTC
Changing from Signed off to In Discussion so that questions will be addressed.
Comment 16 Jonathan Druart 2018-10-31 19:33:19 UTC
There is only 1 call, from C4::Circulation::LostItem, the occurrence in C4::Overdues is part of a comment
Comment 17 Mark Tompsett 2018-11-01 02:57:03 UTC
(In reply to Jonathan Druart from comment #16)
> There is only 1 call, from C4::Circulation::LostItem, the occurrence in
> C4::Overdues is part of a comment

So it is. Still, I thought the general thrust of development was to move towards the Koha namespace, and if not, perhaps hashref the parameter list, so that at least it gets closer to our coding guidelines.
https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL16:_Hashrefs_should_be_used_as_arguments

Also, that comment in C4::Overdues makes me wonder about a tiny refactor / move into the Koha name space. But perhaps that is beyond the scope of this.

Changing the parameter to a hashref should be a simple enough fix. Though, I do think putting the lookup in the function, rather than in the calling function, and thus avoid the need to change the parameter list, is probably better. Less files affected. Less is more.

It's not like chargelostitems is called for the same thing multiple times, is it? If it is, the external look up and hashref parameter is probably preferred. After all, we don't want to slow things down either.

To summarize: if there are no speed issues caused by a double call, then moving the look up into the function is the preferred fix. If there is a speed issue, then hashref'ing the parameter list and leaving it external is preferred.

Just my take on it.
Comment 18 Jonathan Druart 2018-11-01 14:30:45 UTC
Working on an alternative patch.
Comment 19 Jonathan Druart 2018-11-01 14:48:57 UTC
Created attachment 81823 [details] [review]
Bug 18677: Make the tests pass
Comment 20 Jonathan Druart 2018-11-01 14:49:11 UTC
Created attachment 81824 [details] [review]
Bug 18677: Remove new issue_id param from charlostitem

We have the itemnumber no need to pass the issue_id, we can retrieve it
from chargelostitem
Comment 21 Jonathan Druart 2018-11-01 14:50:20 UTC
(In reply to Jonathan Druart from comment #18)
> Working on an alternative patch.

I have attached 2 follow-ups, back to Needs Signoff.
Comment 22 Michal Denar 2018-11-06 07:02:04 UTC
Created attachment 81976 [details] [review]
Bug 18677: issue_id is not added to accountlines for lost item fees

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michal Denar <black23@gmail.com>
Comment 23 Michal Denar 2018-11-06 07:02:11 UTC
Created attachment 81977 [details] [review]
Bug 18677: Make the tests pass

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 24 Michal Denar 2018-11-06 07:02:18 UTC
Created attachment 81978 [details] [review]
Bug 18677: Remove new issue_id param from charlostitem

We have the itemnumber no need to pass the issue_id, we can retrieve it
from chargelostitem

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 25 Marcel de Rooy 2018-11-16 07:34:00 UTC
QA: Looking here
Comment 26 Marcel de Rooy 2018-11-16 08:50:45 UTC
Bit distracted by unac_string. See 21848
Comment 27 Marcel de Rooy 2018-11-16 09:10:27 UTC
Created attachment 82386 [details] [review]
Bug 18677: issue_id is not added to accountlines for lost item fees

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michal Denar <black23@gmail.com>

[EDIT:]
Patch should have increased the number of tests obviously.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2018-11-16 09:10:32 UTC
Created attachment 82387 [details] [review]
Bug 18677: Make the tests pass

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2018-11-16 09:10:38 UTC
Created attachment 82388 [details] [review]
Bug 18677: Remove new issue_id param from charlostitem

We have the itemnumber no need to pass the issue_id, we can retrieve it
from chargelostitem

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2018-11-16 09:10:44 UTC
Created attachment 82389 [details] [review]
Bug 18677: (QA follow-up) Trivial fixes to Chargelostitem.t

Kind of funny that we did not touch this test here. But it passed!
Trivial fixes:
[1] Typo precessfee
[2] Typo the linked
[3] Add rollback

Test plan:
Run t/db_dependent/Circulation/Chargelostitem.t
Comment 31 Nick Clemens 2018-11-16 13:08:31 UTC
Awesome work all!

Pushed to master for 18.11
Comment 32 Jesse Maseto 2018-12-03 19:27:27 UTC
Back ported to 18.05.

Will be in 18.05.07 release.
Comment 33 Fridolin Somers 2018-12-05 08:02:40 UTC
Pushed to 17.11.x for 17.11.13