Bug 20145

Summary: borrowers.datexpiry eq '0000-00-00' means expired?
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: fridolin.somers, nick, roch.damour
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20144
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 20331, 20455    
Attachments: Bug 20145: Do not insert 0000-00-00 in patron tests (and more)
Bug 20145: Do not insert 0000-00-00 in patron tests (and more)
Bug 20145: Do not insert 0000-00-00 in patron tests (and more)

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 (kidclamp) 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} );