Bug 17583 - Use Koha::Patron->is_expired from circulation.pl
Summary: Use Koha::Patron->is_expired from circulation.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 17579
Blocks: 17585
  Show dependency treegraph
 
Reported: 2016-11-08 14:48 UTC by Jonathan Druart
Modified: 2017-06-14 22:11 UTC (History)
5 users (show)

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


Attachments
Bug 17583: Use Koha::Patron->is_expired from the circulation page (1.61 KB, patch)
2016-11-08 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Add the Koha::Patron->is_going_to_expired method (4.74 KB, patch)
2016-11-08 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Add the Koha::Patron->is_going_to_expired method (4.74 KB, patch)
2016-11-08 15:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Add test for is_going_to _expired to be true (1.36 KB, patch)
2016-11-09 10:02 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17583: Fix tests (4.04 KB, patch)
2016-11-09 11:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Fix tests (3.69 KB, patch)
2016-11-09 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Make sure we are comparing 2 dates (4.09 KB, patch)
2016-11-09 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Mock the pref before tests (1.37 KB, patch)
2016-11-15 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17583: Use Koha::Patron->is_expired from the circulation page (1.68 KB, patch)
2016-11-16 08:05 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17583: Add the Koha::Patron->is_going_to_expired method (4.81 KB, patch)
2016-11-16 08:05 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17583: Add test for is_going_to _expired to be true (1.43 KB, patch)
2016-11-16 08:06 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17583: Fix tests (3.76 KB, patch)
2016-11-16 08:06 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17583: Make sure we are comparing 2 dates (4.16 KB, patch)
2016-11-16 08:06 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17583: Mock the pref before tests (1.44 KB, patch)
2016-11-16 08:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17583: Use Koha::Patron->is_expired from the circulation page (1.66 KB, patch)
2016-12-02 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Add the Koha::Patron->is_going_to_expired method (4.66 KB, patch)
2016-12-02 08:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Add test for is_going_to _expired to be true (1.42 KB, patch)
2016-12-02 08:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Fix tests (3.75 KB, patch)
2016-12-02 08:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Make sure we are comparing 2 dates (4.15 KB, patch)
2016-12-02 08:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Mock the pref before tests (1.42 KB, patch)
2016-12-02 08:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17583: Use Koha::Patron->is_expired from the circulation page (1.76 KB, patch)
2016-12-02 10:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17583: Add the Koha::Patron->is_going_to_expired method (4.76 KB, patch)
2016-12-02 10:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17583: Add test for is_going_to _expired to be true (1.51 KB, patch)
2016-12-02 10:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17583: Fix tests (3.84 KB, patch)
2016-12-02 10:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17583: Make sure we are comparing 2 dates (4.24 KB, patch)
2016-12-02 10:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17583: Mock the pref before tests (1.52 KB, patch)
2016-12-02 10:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17583: [QA Follow-up] Final polishing (6.23 KB, patch)
2016-12-02 10:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-11-08 14:48:54 UTC
Bug 17579 introduces a Koha::Patron->is_expired method.
We should use it from the circulation page
Comment 1 Jonathan Druart 2016-11-08 15:16:19 UTC
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
Comment 2 Jonathan Druart 2016-11-08 15:16:22 UTC
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
Comment 3 Jonathan Druart 2016-11-08 15:46:16 UTC
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
Comment 4 Josef Moravec 2016-11-09 10:02:51 UTC
Created attachment 57364 [details] [review]
Bug 17583: Add test for is_going_to _expired to be true
Comment 5 Josef Moravec 2016-11-09 10:05:12 UTC
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.
Comment 6 Jonathan Druart 2016-11-09 11:25:47 UTC
Created attachment 57380 [details] [review]
Bug 17583: Fix tests
Comment 7 Josef Moravec 2016-11-09 11:46:30 UTC
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...
Comment 8 Jonathan Druart 2016-11-09 12:19:58 UTC
Created attachment 57384 [details] [review]
Bug 17583: Fix tests
Comment 9 Jonathan Druart 2016-11-09 12:21:41 UTC
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.
Comment 10 Jonathan Druart 2016-11-09 12:58:41 UTC
There is something wrong again.
Comment 11 Jonathan Druart 2016-11-09 13:30:40 UTC
Created attachment 57387 [details] [review]
Bug 17583: Make sure we are comparing 2 dates
Comment 12 Josef Moravec 2016-11-15 10:48:21 UTC
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
Comment 13 Jonathan Druart 2016-11-15 11:16:29 UTC
Created attachment 57488 [details] [review]
Bug 17583: Mock the pref before tests
Comment 14 Josef Moravec 2016-11-16 08:05:48 UTC
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>
Comment 15 Josef Moravec 2016-11-16 08:05:55 UTC
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>
Comment 16 Josef Moravec 2016-11-16 08:06:00 UTC
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>
Comment 17 Josef Moravec 2016-11-16 08:06:06 UTC
Created attachment 57526 [details] [review]
[SIGNED-OFF] Bug 17583: Fix tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 18 Josef Moravec 2016-11-16 08:06:11 UTC
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>
Comment 19 Josef Moravec 2016-11-16 08:06:16 UTC
Created attachment 57528 [details] [review]
[SIGNED-OFF] Bug 17583: Mock the pref before tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 20 Marcel de Rooy 2016-12-02 08:08:44 UTC
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.
Comment 21 Jonathan Druart 2016-12-02 08:30:56 UTC
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>
Comment 22 Jonathan Druart 2016-12-02 08:31:05 UTC
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>
Comment 23 Jonathan Druart 2016-12-02 08:31:15 UTC
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>
Comment 24 Jonathan Druart 2016-12-02 08:31:24 UTC
Created attachment 57891 [details] [review]
Bug 17583: Fix tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 25 Jonathan Druart 2016-12-02 08:31:34 UTC
Created attachment 57892 [details] [review]
Bug 17583: Make sure we are comparing 2 dates

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 26 Jonathan Druart 2016-12-02 08:31:44 UTC
Created attachment 57893 [details] [review]
Bug 17583: Mock the pref before tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 27 Marcel de Rooy 2016-12-02 10:58:58 UTC
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>
Comment 28 Marcel de Rooy 2016-12-02 10:59:02 UTC
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>
Comment 29 Marcel de Rooy 2016-12-02 10:59:06 UTC
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>
Comment 30 Marcel de Rooy 2016-12-02 10:59:10 UTC
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>
Comment 31 Marcel de Rooy 2016-12-02 10:59:14 UTC
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>
Comment 32 Marcel de Rooy 2016-12-02 10:59:18 UTC
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>
Comment 33 Marcel de Rooy 2016-12-02 10:59:22 UTC
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>
Comment 34 Kyle M Hall 2016-12-02 19:15:14 UTC
Pushed to master for 17,05, thanks Jonathan, Josef, Marcel!
Comment 35 Katrin Fischer 2016-12-04 21:44:47 UTC
This won't get backported to 16.11.x as it is an enhancement.