Bug 20145 - borrowers.datexpiry eq '0000-00-00' means expired?
Summary: borrowers.datexpiry eq '0000-00-00' means expired?
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 20331 20455
  Show dependency treegraph
 
Reported: 2018-02-06 19:47 UTC by Jonathan Druart
Modified: 2019-10-14 19:56 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20145: Do not insert 0000-00-00 in patron tests (and more) (6.00 KB, patch)
2018-02-14 18:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20145: Do not insert 0000-00-00 in patron tests (and more) (6.02 KB, patch)
2018-02-16 20:35 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 20145: Do not insert 0000-00-00 in patron tests (and more) (6.11 KB, patch)
2018-02-18 10:32 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-02-06 19:47:55 UTC
See bug http://lists.koha-community.org/pipermail/koha-devel/2018-February/044322.html

Koha::Patron->is_expired returns  0 if $self->dateexpiry eq '0000-00-00'
And there is a test I wrote for it on bug 17578 (commit 5a0a2ce584dec74808f471b0f92b2114c3d6029e , GetMemberDetails - Remove is_expired)

But then we also have, in C4::Circulation::CanBookBeIssued:
 752     if ( !defined $patron->dateexpiry || $patron->dateexpiry eq '0000-00-00') {
 753         $issuingimpossible{EXPIRED} = 1;
 754     } else {
With a test:
is( $issuingimpossible->{EXPIRED}, 1, 'The patron should be considered as expired if dateexpiry is 0000-00-00' );

So, my questions, do you have a very old Koha installations to search if there is borrowers.is_expired == "0000-00-00" in it?
Do you remember if it meant expired or not at some point?
Comment 1 Jonathan Druart 2018-02-14 18:19:18 UTC
Created attachment 71633 [details] [review]
Bug 20145: Do not insert 0000-00-00 in patron tests (and more)

We should call Koha::Patron->is_expired in CanBookBeIssued instead of
doing the same calculation.

Tests have been adapted to pass with new SQL modes.

We should not need to update the values in DB, we already have
  Bug 14717: Prevent 0000-00-00 dates in patron data (3.21.00.023)

Test plan:
  prove t/db_dependent/Circulation/dateexpiry.t
  prove t/db_dependent/Koha/Patrons.t
must return green
Comment 2 Roch D'Amour 2018-02-16 20:35:15 UTC
Created attachment 71859 [details] [review]
Bug 20145: Do not insert 0000-00-00 in patron tests (and more)

We should call Koha::Patron->is_expired in CanBookBeIssued instead of
doing the same calculation.

Tests have been adapted to pass with new SQL modes.

We should not need to update the values in DB, we already have
  Bug 14717: Prevent 0000-00-00 dates in patron data (3.21.00.023)

Test plan:
  prove t/db_dependent/Circulation/dateexpiry.t
  prove t/db_dependent/Koha/Patrons.t
must return green

Signed-off-by: Roch D'Amour <roch.damour@inlibro.com>
Comment 3 Katrin Fischer 2018-02-18 10:32:08 UTC
Created attachment 71897 [details] [review]
Bug 20145: Do not insert 0000-00-00 in patron tests (and more)

We should call Koha::Patron->is_expired in CanBookBeIssued instead of
doing the same calculation.

Tests have been adapted to pass with new SQL modes.

We should not need to update the values in DB, we already have
  Bug 14717: Prevent 0000-00-00 dates in patron data (3.21.00.023)

Test plan:
  prove t/db_dependent/Circulation/dateexpiry.t
  prove t/db_dependent/Koha/Patrons.t
must return green

Signed-off-by: Roch D'Amour <roch.damour@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 4 Jonathan Druart 2018-02-18 19:50:38 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 5 Nick Clemens 2018-03-23 12:47:54 UTC
Awsome work all! Pushed to stable for 17.11.04
Comment 6 Fridolin Somers 2018-03-27 08:45:41 UTC
Pushed to 17.05.x for v17.05.10

In order to apply I added in C4/Circulation.pm (exists in 17.11) :
     #
     # BORROWER STATUS
     #
+    my $patron = Koha::Patrons->find( $borrower->{borrowernumber} );