Description
Jonathan Druart
2016-11-25 11:59:23 UTC
Created attachment 57787 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls C4::Circulation::GetItemIssue returned all the issue and item informations for a given issue. Moveover it also did some date manipulations. Most of the time this subroutine was called, there additional information were useless as the caller usually just needed the basic issue's infos 'from the issue table). This first patch updates the simple calls, ie. the ones that just need the issue's infomations. Test plan: The following operations should success: - transfer a book - create a rule for on-site checkouts and confirm that a patron cannot check more items out that it's defined in the rule. - Renew an issue using ILSDI - Using SIP confirm that you are able to see your issues Created attachment 57788 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Created attachment 57789 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue This patch simply removes the C4::Circulation::GetItemIssue subroutine Test plan: At this point, `git grep GetItemIssue` should not return any occurrence in the codebase Patch does not apply: Applying: Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls fatal: sha1 information is lacking or useless (C4/Circulation.pm). (In reply to Aleisha Amohia from comment #4) > Patch does not apply: > > Applying: Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls > fatal: sha1 information is lacking or useless (C4/Circulation.pm). Did you apply the dependencies first? With dependencies, I was able apply these patches. Created attachment 57847 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue This patch simply removes the C4::Circulation::GetItemIssue subroutine Test plan: At this point, `git grep GetItemIssue` should not return any occurrence in the codebase Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> This patch set needs updated to use Koha::Checkouts Created attachment 58431 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls C4::Circulation::GetItemIssue returned all the issue and item informations for a given issue. Moveover it also did some date manipulations. Most of the time this subroutine was called, there additional information were useless as the caller usually just needed the basic issue's infos 'from the issue table). This first patch updates the simple calls, ie. the ones that just need the issue's infomations. Test plan: The following operations should success: - transfer a book - create a rule for on-site checkouts and confirm that a patron cannot check more items out that it's defined in the rule. - Renew an issue using ILSDI - Using SIP confirm that you are able to see your issues Created attachment 58432 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Created attachment 58433 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue This patch simply removes the C4::Circulation::GetItemIssue subroutine Test plan: At this point, `git grep GetItemIssue` should not return any occurrence in the codebase Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Created attachment 58501 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date could you rebase please? Created attachment 58918 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls C4::Circulation::GetItemIssue returned all the issue and item informations for a given issue. Moveover it also did some date manipulations. Most of the time this subroutine was called, there additional information were useless as the caller usually just needed the basic issue's infos 'from the issue table). This first patch updates the simple calls, ie. the ones that just need the issue's infomations. Test plan: The following operations should success: - transfer a book - create a rule for on-site checkouts and confirm that a patron cannot check more items out that it's defined in the rule. - Renew an issue using ILSDI - Using SIP confirm that you are able to see your issues Created attachment 58919 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue This patch simply removes the C4::Circulation::GetItemIssue subroutine Test plan: At this point, `git grep GetItemIssue` should not return any occurrence in the codebase Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Created attachment 58920 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Created attachment 61761 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls C4::Circulation::GetItemIssue returned all the issue and item informations for a given issue. Moveover it also did some date manipulations. Most of the time this subroutine was called, there additional information were useless as the caller usually just needed the basic issue's infos 'from the issue table). This first patch updates the simple calls, ie. the ones that just need the issue's infomations. Test plan: The following operations should success: - transfer a book - create a rule for on-site checkouts and confirm that a patron cannot check more items out that it's defined in the rule. - Renew an issue using ILSDI - Using SIP confirm that you are able to see your issues Created attachment 61762 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue This patch simply removes the C4::Circulation::GetItemIssue subroutine Test plan: At this point, `git grep GetItemIssue` should not return any occurrence in the codebase Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Created attachment 61763 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Just a rebase. First two patches apply cleanly, with 3rd patch (complex calls) I get: fatal: sha1 information is lacking or useless (C4/Circulation.pm). Created attachment 62034 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Typo in 3rd patch, circ/returns.pl , line 314: my $date_due_dt = dt_strom_string( $issue->date_due, 'sql' ); Created attachment 62086 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Worked through test plans from comments #1 - #3, everything OK, will sign off. Created attachment 62102 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls C4::Circulation::GetItemIssue returned all the issue and item informations for a given issue. Moveover it also did some date manipulations. Most of the time this subroutine was called, there additional information were useless as the caller usually just needed the basic issue's infos 'from the issue table). This first patch updates the simple calls, ie. the ones that just need the issue's infomations. Test plan: The following operations should success: - transfer a book - create a rule for on-site checkouts and confirm that a patron cannot check more items out that it's defined in the rule. - Renew an issue using ILSDI - Using SIP confirm that you are able to see your issues Followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 62103 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue This patch simply removes the C4::Circulation::GetItemIssue subroutine Test plan: At this point, `git grep GetItemIssue` should not return any occurrence in the codebase Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 62104 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 64257 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls C4::Circulation::GetItemIssue returned all the issue and item informations for a given issue. Moveover it also did some date manipulations. Most of the time this subroutine was called, there additional information were useless as the caller usually just needed the basic issue's infos 'from the issue table). This first patch updates the simple calls, ie. the ones that just need the issue's infomations. Test plan: The following operations should success: - transfer a book - create a rule for on-site checkouts and confirm that a patron cannot check more items out that it's defined in the rule. - Renew an issue using ILSDI - Using SIP confirm that you are able to see your issues Followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 64258 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue This patch simply removes the C4::Circulation::GetItemIssue subroutine Test plan: At this point, `git grep GetItemIssue` should not return any occurrence in the codebase Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 64259 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Signed-off-by: Marc Véron <veron@veron.ch> looking here - SIP item_information request failed for me in testing (worked on master) - will try again tomorrow SIP fails on the date, this needs to be converted to a string for returning Checking in an overdue item fails to calculate the fine Test fails: t/db_dependent/Circulation.t .. 44/95 # Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10' # at t/db_dependent/Circulation.t line 671. # got: 'auto_too_much_oweing' # expected: 'auto_renew' # Looks like you failed 1 test of 6. I think there are spots where the date isn't being converted from a DateTime object when needed Created attachment 64284 [details] [review] Bug 17680: Fix regression - is_overdue must be called is_overdue must be called even if there is not dropbox date (!) (In reply to Nick Clemens from comment #32) > SIP fails on the date, this needs to be converted to a string for returning Any chances you could try to fix this one? > Checking in an overdue item fails to calculate the fine Ha, terrible! Will be fixed with the last patch, thanks for catching that. > Test fails: > t/db_dependent/Circulation.t .. 44/95 > # Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10' > # at t/db_dependent/Circulation.t line 671. > # got: 'auto_too_much_oweing' > # expected: 'auto_renew' > # Looks like you failed 1 test of 6. I manage to recreate that with prove t/db_dependent/Circulation* but then the test failed on master too. Sounds like a master bug, depending on data. > I think there are spots where the date isn't being converted from a DateTime > object when needed Did you spot where? (In reply to Jonathan Druart from comment #34) > > Test fails: > > t/db_dependent/Circulation.t .. 44/95 > > # Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10' > > # at t/db_dependent/Circulation.t line 671. > > # got: 'auto_too_much_oweing' > > # expected: 'auto_renew' > > # Looks like you failed 1 test of 6. > > I manage to recreate that with prove t/db_dependent/Circulation* > but then the test failed on master too. Sounds like a master bug, depending > on data. It depends on the value of finesMode. I have opened bug 18802. Created attachment 64337 [details] [review] Bug 17680: Add few tests for AddReturn when overdue To make sure the last patch fixes the issue Code looks ok, but I cannot apply the 'complex calls' patch. Created attachment 64805 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 64806 [details] [review] Bug 17680: Fix regression - is_overdue must be called is_overdue must be called even if there is not dropbox date (!) Created attachment 64807 [details] [review] Bug 17680: Add few tests for AddReturn when overdue To make sure the last patch fixes the issue wihtout the patches: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/Returns.t .. ok All tests successful. Files=1, Tests=4, 2 wallclock secs ( 0.02 usr 0.01 sys + 2.09 cusr 0.35 csys = 2.47 CPU) Result: PASS with the patches: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Circulation/Returns.t t/db_dependent/Circulation/Returns.t .. 1/4 # Failed test 'No account lines should exist on old issue_id' # at t/db_dependent/Circulation/Returns.t line 311. # got: '1' # expected: '0' # Failed test 'Two account lines should exist on new issue_id' # at t/db_dependent/Circulation/Returns.t line 314. # got: '1' # expected: '2' # Looks like you failed 2 tests of 4. t/db_dependent/Circulation/Returns.t .. 4/4 # Failed test 'Handle ids duplication' # at t/db_dependent/Circulation/Returns.t line 318. # Looks like you failed 1 test of 4. t/db_dependent/Circulation/Returns.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/4 subtests Test Summary Report ------------------- t/db_dependent/Circulation/Returns.t (Wstat: 256 Tests: 4 Failed: 1) Failed test: 4 Non-zero exit status: 1 Files=1, Tests=4, 3 wallclock secs ( 0.02 usr 0.01 sys + 2.14 cusr 0.33 csys = 2.50 CPU) Result: FAIL kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ Created attachment 64833 [details] [review] Bug 17680: Fix conflict with bug 18651 $issue is now a Koha::Checkout, not a hashref (In reply to Tomás Cohen Arazi from comment #41) Thanks for catching that, fixed! Created attachment 64866 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls C4::Circulation::GetItemIssue returned all the issue and item informations for a given issue. Moveover it also did some date manipulations. Most of the time this subroutine was called, there additional information were useless as the caller usually just needed the basic issue's infos 'from the issue table). This first patch updates the simple calls, ie. the ones that just need the issue's infomations. Test plan: The following operations should success: - transfer a book - create a rule for on-site checkouts and confirm that a patron cannot check more items out that it's defined in the rule. - Renew an issue using ILSDI - Using SIP confirm that you are able to see your issues Followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 64867 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue This patch simply removes the C4::Circulation::GetItemIssue subroutine Test plan: At this point, `git grep GetItemIssue` should not return any occurrence in the codebase Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 64868 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 64869 [details] [review] Bug 17680: Fix regression - is_overdue must be called is_overdue must be called even if there is not dropbox date (!) Created attachment 64870 [details] [review] Bug 17680: Add few tests for AddReturn when overdue To make sure the last patch fixes the issue Created attachment 64871 [details] [review] Bug 17680: Fix conflict with bug 18651 $issue is now a Koha::Checkout, not a hashref Created attachment 64925 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls C4::Circulation::GetItemIssue returned all the issue and item informations for a given issue. Moveover it also did some date manipulations. Most of the time this subroutine was called, there additional information were useless as the caller usually just needed the basic issue's infos 'from the issue table). This first patch updates the simple calls, ie. the ones that just need the issue's infomations. Test plan: The following operations should success: - transfer a book - create a rule for on-site checkouts and confirm that a patron cannot check more items out that it's defined in the rule. - Renew an issue using ILSDI - Using SIP confirm that you are able to see your issues Followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 64926 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue This patch simply removes the C4::Circulation::GetItemIssue subroutine Test plan: At this point, `git grep GetItemIssue` should not return any occurrence in the codebase Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 64927 [details] [review] Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls There are a few calls to GetItemIssue where it's not as easy to make sure everything will be fine just replacing the calls with a Koha::Issues->find - In AddReturn the overdue flag is used (that's why this patch depends on bug 17689) - In CanBookBeRenewed, as well as the overdue flag the dates converted to DateTime were used. It's now our job to convert them when we need them. - Same in AddRenewal but we also call _CalculateAndUpdateFine, so we need to update the variables in this subroutine. Note that, prior to this patch, AddReturn returned the GetItemIssue hashref in the $iteminformation. Most of the time this variable is not used, I have found only 1 place where it's used: circ/returns.pl TODO: In this script we should call ->is_overdue instead of the DateTime->compare calls Test plan: All the circulation tests must pass (it's how I have caught the specific cases). Do some checkins/checkouts/renewal and focus on the due date Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 64928 [details] [review] Bug 17680: Fix regression - is_overdue must be called is_overdue must be called even if there is not dropbox date (!) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 64929 [details] [review] Bug 17680: Add few tests for AddReturn when overdue To make sure the last patch fixes the issue Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 64930 [details] [review] Bug 17680: Fix conflict with bug 18651 $issue is now a Koha::Checkout, not a hashref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 64931 [details] [review] Bug 17680: Remove Koha::Upload use incorrectly added Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I tested this patchset on the UI. It looks trivial in most of the cases. I haven't tested ILS-DI and SIP beyond the included unit tests, but the changes seems pretty straight-forward, and tests pass of course. The QA script is happy too. Pushed to master for 17.11, thanks to everybody involved! Enhancement not pushed to 17.05.x (In reply to Tomás Cohen Arazi from comment #57) > I tested this patchset on the UI. It looks trivial in most of the cases. I > haven't tested ILS-DI and SIP beyond the included unit tests, but the > changes seems pretty straight-forward, and tests pass of course. I think I found a problem in SIP/ILS/Item. It fails silently on dt_from_string when checking in an item. Created attachment 65302 [details] [review] Bug 17680: [QA Follow-up] dt_from_string failing silently This is a trivial fix, but finding the cause for a failing checkin in SIPServer surely was not. Without this simple change, I could not checkin an item while the SIP server just closed the connection and syslog contained nothing useful. The error can be easily reproduced without this patch by: use Modern::Perl; use C4::SIP::ILS::Item; my $item = C4::SIP::ILS::Item->new('some_barcode'); where some_barcode should refer to an item checked out. You should trigger the error (with your due date): 2017-07-28 23:59:00 is not an object at line 3. Why the ILS/Item.pm module still compiles without a warning, is not clear though. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Marcel de Rooy from comment #61) > Created attachment 65302 [details] [review] [review] > Bug 17680: [QA Follow-up] dt_from_string failing silently > > This is a trivial fix, but finding the cause for a failing checkin in > SIPServer surely was not. > Without this simple change, I could not checkin an item while the SIP server > just closed the connection and syslog contained nothing useful. > > The error can be easily reproduced without this patch by: > use Modern::Perl; > use C4::SIP::ILS::Item; > my $item = C4::SIP::ILS::Item->new('some_barcode'); > where some_barcode should refer to an item checked out. > You should trigger the error (with your due date): > 2017-07-28 23:59:00 is not an object at line 3. > > Why the ILS/Item.pm module still compiles without a warning, is not clear > though. > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Nice find - these are painful to locate. Suspect that the reason there is no compile time warning is because ILS::Item is using autoloading so that the routine gets resolved at runtime. Created attachment 65310 [details] [review] Bug 17680 [QA Follow-up] dt_from_string failing silently Added independent sign off to last patch Would be great to have the SIP tests moved to t Last patch pushed to master for 17.11. Thanks Marcel and Colin! (In reply to Colin Campbell from comment #62) > Suspect that the reason there is no > compile time warning is because ILS::Item is using autoloading so that the > routine gets resolved at runtime. Yes, nice effects of autoloading of course. We could improve that some day ;) (In reply to Jonathan Druart from comment #64) > Would be great to have the SIP tests moved to t Will try to add at least a test for checkin on 18996. Bit by bit extending what we have in t/db_dependent/SIP. I am not that comfortable with C4/SIP/t and suspect that it does not create its own data too. Enhancement not pushed to 17.05.x |