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 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 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.
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
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
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.
(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.
I get test errors after trying to resolve merge conflicts when applying this.. requesting a rebase.
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
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>
Works well for me: Signing off
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 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.
Changing from Signed off to In Discussion so that questions will be addressed.
There is only 1 call, from C4::Circulation::LostItem, the occurrence in C4::Overdues is part of a comment
(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.
Working on an alternative patch.
Created attachment 81823 [details] [review] Bug 18677: Make the tests pass
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
(In reply to Jonathan Druart from comment #18) > Working on an alternative patch. I have attached 2 follow-ups, back to Needs Signoff.
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>
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>
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>
QA: Looking here
Bit distracted by unac_string. See 21848
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>
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>
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>
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
Awesome work all! Pushed to master for 18.11
Back ported to 18.05. Will be in 18.05.07 release.
Pushed to 17.11.x for 17.11.13