Bug 33522 - Optionally skip (in)active patrons when sending membership expiry notices
Summary: Optionally skip (in)active patrons when sending membership expiry notices
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 33245
Blocks: 28688
  Show dependency treegraph
 
Reported: 2023-04-13 13:57 UTC by Marcel de Rooy
Modified: 2023-12-20 19:42 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/799
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 33522: Add active and inactive parameter to membership_expiry.pl (4.51 KB, patch)
2023-05-31 15:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33522: Add active and inactive parameter to membership_expiry.pl (4.51 KB, patch)
2023-06-02 15:14 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 33522: Add active and inactive parameter to membership_expiry.pl (4.50 KB, patch)
2023-09-02 07:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33522: (follow-up) Do not rely on fallback (2.59 KB, patch)
2023-09-02 07:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33522: Add active and inactive parameter to membership_expiry.pl (4.53 KB, patch)
2023-10-04 13:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33522: (follow-up) Do not rely on fallback (2.62 KB, patch)
2023-10-04 13:04 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2023-04-13 13:57:31 UTC
Could we use the patron->is_active method here? See bug 33245 (wip).
Comment 1 Marcel de Rooy 2023-05-30 14:32:39 UTC
(In reply to Marcel de Rooy from comment #0)
> Could we use the patron->is_active method here? See bug 33245 (wip).

Yes. Bug 33245 provides a base for this. You can do $patron->is_active({ months => 6 }) or something and continue.

The is_active method looks at datelastseen, enrollment date, expiry date, anonymized status, and recent holds, issues and article requests.
Comment 2 Marcel de Rooy 2023-05-31 15:19:03 UTC
Created attachment 151885 [details] [review]
Bug 33522: Add active and inactive parameter to membership_expiry.pl

Test plan:
Enable TrackLastPatronActivity.
Set MembershipExpiryDaysNotice to X days.
Pick a patron, make it expire in X days. Add email address.
Update borrowers.lastseen for this patron to make it active.
Run membership_expiry.pl -c -v -active
You should see at least one enqueued notice.
Note: Since we passed no number, we default to 6 months.
Run membership_expiry.pl -c -v -inactive
You should see that it skipped a number of active patrons (>=1).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2023-06-01 09:53:36 UTC
Note: bug 28688 will be based on top of this one.

And will handle the case where a patron might become inactive in the time between notification moment and expiry date.
If he is active at notification and if you would skip him, but becomes inactive, than he would have no notice but would not be renewed. We need an additional flag to view those patrons as inactive already at notify time.

The other way around could be a bit harder. He was not active but becomes active again just before expiry. If you would auto renew, no problem. If you sent a notice to inactive ones, no problem too. So only if you skipped inactive ones, he may silently expire without a notice.
Note that we also have the NotifyBorrowerDeparture warn. Since he became active again, he might have seen that one. It seems that SIP does not warn you for expiry though. Opened bug 33878 for that.
If you send two expiry notices (we send a second one 7 days before), the risk of this case is even smaller.
For now, I would consider this a side-effect of deciding to not inform inactive users about expiry given the other possible warning. And document it.
Comment 4 Sam Lau 2023-06-02 15:14:09 UTC
Created attachment 151986 [details] [review]
Bug 33522: Add active and inactive parameter to membership_expiry.pl

Test plan:
Enable TrackLastPatronActivity.
Set MembershipExpiryDaysNotice to X days.
Pick a patron, make it expire in X days. Add email address.
Update borrowers.lastseen for this patron to make it active.
Run membership_expiry.pl -c -v -active
You should see at least one enqueued notice.
Note: Since we passed no number, we default to 6 months.
Run membership_expiry.pl -c -v -inactive
You should see that it skipped a number of active patrons (>=1).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 5 Marcel de Rooy 2023-06-14 11:35:54 UTC
(In reply to Sam Lau from comment #4)

> Signed-off-by: Sam Lau <samalau@gmail.com>

Thanks Sam Lau for testing. Did you also have a look at the test plan of the base dependency (bug 33245)? It has not been signed off yet after all?
Comment 6 Marcel de Rooy 2023-09-02 07:23:12 UTC
Created attachment 155164 [details] [review]
Bug 33522: Add active and inactive parameter to membership_expiry.pl

Test plan:
Enable TrackLastPatronActivity.
Set MembershipExpiryDaysNotice to X days.
Pick a patron, make it expire in X days. Add email address.
Update borrowers.lastseen for this patron to make it active.
Run membership_expiry.pl -c -v -active 6
You should see at least one enqueued notice.
Run membership_expiry.pl -c -v -inactive 6
You should see that it skipped a number of active patrons (>=1).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 7 Marcel de Rooy 2023-09-02 07:23:14 UTC
Created attachment 155165 [details] [review]
Bug 33522: (follow-up) Do not rely on fallback

Bug 33245 has been adjusted; the fallback has been removed.
We should check now if the -active or -inactive parameter
actually has been passed.

Test plan:
See earlier test plan.
Run the script without active/inactive.
Run the script with active.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Jonathan Druart 2023-10-04 13:04:30 UTC
Created attachment 156531 [details] [review]
Bug 33522: Add active and inactive parameter to membership_expiry.pl

Test plan:
Enable TrackLastPatronActivity.
Set MembershipExpiryDaysNotice to X days.
Pick a patron, make it expire in X days. Add email address.
Update borrowers.lastseen for this patron to make it active.
Run membership_expiry.pl -c -v -active 6
You should see at least one enqueued notice.
Run membership_expiry.pl -c -v -inactive 6
You should see that it skipped a number of active patrons (>=1).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2023-10-04 13:04:33 UTC
Created attachment 156532 [details] [review]
Bug 33522: (follow-up) Do not rely on fallback

Bug 33245 has been adjusted; the fallback has been removed.
We should check now if the -active or -inactive parameter
actually has been passed.

Test plan:
See earlier test plan.
Run the script without active/inactive.
Run the script with active.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Tomás Cohen Arazi 2023-10-04 15:09:30 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 11 Fridolin Somers 2023-10-06 00:31:22 UTC
Enhancement not pushed to 23.05.x