Bug 17579 introduces a Koha::Patron->is_expired method. We should use it from the circulation page
Created attachment 57329 [details] [review] Bug 17583: Use Koha::Patron->is_expired from the circulation page Bug 17579 introduces a Koha::Patron->is_expired method. Let's use it from this script. Test plan: Confirm that you see the message on the interface from the circulation page for an expired patron
Created attachment 57330 [details] [review] Bug 17583: Add the Koha::Patron->is_going_to_expired method In order to be consistent, we need to create this method as well. Test plan: Make sure the pref NotifyBorrowerDeparture works as expected
Created attachment 57334 [details] [review] Bug 17583: Add the Koha::Patron->is_going_to_expired method In order to be consistent, we need to create this method as well. Test plan: Make sure the pref NotifyBorrowerDeparture works as expected
Created attachment 57364 [details] [review] Bug 17583: Add test for is_going_to _expired to be true
I think you should substract, not add the number of days from NotifyBorrowerDeparture syspref in is_going_to_expired method. I tried to share a test to highlight it.
Created attachment 57380 [details] [review] Bug 17583: Fix tests
Thanks Jonathan, but there is another issue as I discovered now, let me describe it: It looks like Koha used to "think" that when dateexpiry = today, than membership has expired... That's bug from my point of view... Your code continue to behave the same way - that's the reason why 2 tests of subtest 'is_going_to_expired' are failing - at least on my machine... We just need to decide: Is it a bug? Should we fix it on this report or open the new bug report (probably better, but we'd have failing Patrons test on master...) What do you think? Sorry for complicating things...
Created attachment 57384 [details] [review] Bug 17583: Fix tests
Yes I have seen that. The current behavior is: If dateexpiry = today => not going to expired If pref is 0 => not going to expired If dateexpiry - pref = today => not going to expired. I am not sure this is the correct behavior, but I want to keep this behavior after this patch.
There is something wrong again.
Created attachment 57387 [details] [review] Bug 17583: Make sure we are comparing 2 dates
This doesn't works for me, the test returns: # Failed test 'Patron should not be considered going to expire if dateexpiry is today' # at t/db_dependent/Koha/Patrons.t line 195. # got: '1' # expected: '0' # Looks like you failed 1 test of 9. It's this test: 195 is( $patron->is_going_to_expired, 0, 'Patron should not be considered going to expire if dateexpiry is today'); Testing on top of Bug 17594 and Bug 17579
Created attachment 57488 [details] [review] Bug 17583: Mock the pref before tests
Created attachment 57523 [details] [review] [SIGNED-OFF] Bug 17583: Use Koha::Patron->is_expired from the circulation page Bug 17579 introduces a Koha::Patron->is_expired method. Let's use it from this script. Test plan: Confirm that you see the message on the interface from the circulation page for an expired patron Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57524 [details] [review] [SIGNED-OFF] Bug 17583: Add the Koha::Patron->is_going_to_expired method In order to be consistent, we need to create this method as well. Test plan: Make sure the pref NotifyBorrowerDeparture works as expected Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57525 [details] [review] [SIGNED-OFF] Bug 17583: Add test for is_going_to _expired to be true Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57526 [details] [review] [SIGNED-OFF] Bug 17583: Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57527 [details] [review] [SIGNED-OFF] Bug 17583: Make sure we are comparing 2 dates Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57528 [details] [review] [SIGNED-OFF] Bug 17583: Mock the pref before tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Applying: Bug 17583: Add the Koha::Patron->is_going_to_expired method fatal: sha1 information is lacking or useless (Koha/Patron.pm). Repository lacks necessary blobs to fall back on 3-way merge.
Created attachment 57888 [details] [review] Bug 17583: Use Koha::Patron->is_expired from the circulation page Bug 17579 introduces a Koha::Patron->is_expired method. Let's use it from this script. Test plan: Confirm that you see the message on the interface from the circulation page for an expired patron Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57889 [details] [review] Bug 17583: Add the Koha::Patron->is_going_to_expired method In order to be consistent, we need to create this method as well. Test plan: Make sure the pref NotifyBorrowerDeparture works as expected Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57890 [details] [review] Bug 17583: Add test for is_going_to _expired to be true Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57891 [details] [review] Bug 17583: Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57892 [details] [review] Bug 17583: Make sure we are comparing 2 dates Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57893 [details] [review] Bug 17583: Mock the pref before tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 57914 [details] [review] Bug 17583: Use Koha::Patron->is_expired from the circulation page Bug 17579 introduces a Koha::Patron->is_expired method. Let's use it from this script. Test plan: Confirm that you see the message on the interface from the circulation page for an expired patron Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 57915 [details] [review] Bug 17583: Add the Koha::Patron->is_going_to_expired method In order to be consistent, we need to create this method as well. Test plan: Make sure the pref NotifyBorrowerDeparture works as expected Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 57916 [details] [review] Bug 17583: Add test for is_going_to _expired to be true Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 57917 [details] [review] Bug 17583: Fix tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 57918 [details] [review] Bug 17583: Make sure we are comparing 2 dates Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 57919 [details] [review] Bug 17583: Mock the pref before tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 57920 [details] [review] Bug 17583: [QA Follow-up] Final polishing Number of tests in Patrons.t corrected. Method is_going_to_expired (no english!) renamed to is_going_to_expire. Adding a negative duration replaced by a subtract. Reads easier. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 17,05, thanks Jonathan, Josef, Marcel!
This won't get backported to 16.11.x as it is an enhancement.