Bug 32560 - Mark expired borrowers as having lost their card
Summary: Mark expired borrowers as having lost their card
Status: RESOLVED WORKSFORME
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-01-03 23:22 UTC by Alex Buckley
Modified: 2023-10-23 17:08 UTC (History)
4 users (show)

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


Attachments
Bug 32560: Cronjob to mark expired borrowers as lost their card (1.76 KB, patch)
2023-01-04 01:27 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 32560: Cronjob to mark expired borrowers as lost their card (1.81 KB, patch)
2023-01-27 23:33 UTC, David Nind
Details | Diff | Splinter Review
Bug 32560: (follow-up) Altered wording of output (864 bytes, patch)
2023-02-02 04:19 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 32560: Cronjob to mark expired borrowers as lost their card (1.81 KB, patch)
2023-02-02 19:59 UTC, David Nind
Details | Diff | Splinter Review
Bug 32560: (follow-up) Altered wording of output (912 bytes, patch)
2023-02-02 19:59 UTC, David Nind
Details | Diff | Splinter Review
Bug 32560: (follow-up) Replace raw SQL queries with Koha::Patrons search and update (3.86 KB, patch)
2023-03-07 00:51 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 32560: Cronjob to mark expired borrowers as lost their card (1.85 KB, patch)
2023-08-03 15:49 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 32560: (follow-up) Altered wording of output (955 bytes, patch)
2023-08-03 15:49 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 32560: (follow-up) Replace raw SQL queries with Koha::Patrons search and update (3.90 KB, patch)
2023-08-03 15:49 UTC, Sam Lau
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2023-01-03 23:22:13 UTC
We have a library that marks expired borrowers as having lost their card. It would be nice to be able to upstream the cronjob they use to mark borrowers.lost = 1
Comment 1 Alex Buckley 2023-01-04 01:27:57 UTC Comment hidden (obsolete)
Comment 2 David Nind 2023-01-27 23:33:08 UTC Comment hidden (obsolete)
Comment 3 David Nind 2023-01-27 23:35:34 UTC
Just a minor suggestion - rather than the output being "1 borrowers to be changed", maybe change it to "1 borrower(s) changed".

The main reason is that when you run the script, the borrowers are changed.
Comment 4 Alex Buckley 2023-02-02 04:19:51 UTC Comment hidden (obsolete)
Comment 5 Alex Buckley 2023-02-02 04:21:00 UTC
(In reply to David Nind from comment #3)
> Just a minor suggestion - rather than the output being "1 borrowers to be
> changed", maybe change it to "1 borrower(s) changed".
> 
> The main reason is that when you run the script, the borrowers are changed.

Hi David, 

Great suggestion,  thank you, and thanks for testing too! I have attached a follow-up patch with that new wording. 

Cheers,
Alex
Comment 6 Alex Buckley 2023-02-02 04:23:28 UTC
Just setting this back to 'Needs signoff' so you can hopefully test and signoff the additional patch. Thanks so much
Comment 7 David Nind 2023-02-02 19:59:34 UTC Comment hidden (obsolete)
Comment 8 David Nind 2023-02-02 19:59:39 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2023-03-03 18:17:23 UTC
I would love to see the raw SQL replaces with a Koha::Patrons search and update.
Comment 10 Alex Buckley 2023-03-07 00:51:30 UTC Comment hidden (obsolete)
Comment 11 Alex Buckley 2023-03-07 00:53:41 UTC
(In reply to Kyle M Hall from comment #9)
> I would love to see the raw SQL replaces with a Koha::Patrons search and
> update.

Hi Kyle, 

I've just attached a follow-up patch with those changes and several other improvements. 

Thanks,
Alex
Comment 12 Sam Lau 2023-08-03 15:49:13 UTC
Created attachment 154222 [details] [review]
Bug 32560: Cronjob to mark expired borrowers as lost their card

Test plan:
1. Edit a borrower to have an expiration date in the past - also make
sure they have 'Lost card' set to 'No'

2. Run the set_expired_lost.pl script:
sudo koha-shell <instance>
cd misc/cronjobs
./set_expired_lost.pl

3. Notice you get output saying the number of borrowers are to be changed

4. View the borrower account from #1 and notice they have a 'Lost card'
value of 'Yes'

5. Run the set_expired_lost.pl script again and observe no output is
printed to the terminal - this is because all expired borrowers have
already been marked as lost.

Sponsored-by: Dalton McCaughey Library, Australia
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 13 Sam Lau 2023-08-03 15:49:16 UTC
Created attachment 154223 [details] [review]
Bug 32560: (follow-up) Altered wording of output

Sponsored-by: Dalton McCaughey Library, Australia
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 14 Sam Lau 2023-08-03 15:49:18 UTC
Created attachment 154224 [details] [review]
Bug 32560: (follow-up) Replace raw SQL queries with Koha::Patrons search and update

Also:
 - Add use Koha::Script -cron;
 - Add license
 - Add optional verbose flag
 - Add cronjob logging

Test plan:
1. Set 'CronjobLog' syspref = 'Log'

2. Edit a borrower to have an expiration date in the past - also make
sure they have 'Lost card' set to 'No'

3. Run the set_expired_lost.pl script:
sudo koha-shell <instance>
cd misc/cronjobs
./set_expired_lost.pl

4. Observe no output is displayed but viewing the borrower account from
step #1 they now have a 'Lost card' value of 'Yes'.

5. Change the borrower account 'Lost card' back to 'No'

6. Run the set_expired_lost.pl script again, this time with the verbose
flag:
sudo koha-shell <instance>
cd misc/cronjobs
./set_expired_lost.pl -v

7. Observe output is displayed to the commandline showing the number of
borrowers that have been changed. Check the borrower account again and
notice the 'Lost card' has been set back to 'Yes'.

8. Repeat step #6 and observe no output is
printed to the terminal - this is because all expired borrowers have
already been marked as lost.

9. Check there are log entries for set_expired_lost.pl cronjob in the action_logs table:
sudo koha-mysql <instance>
select * from action_logs where script='set_expired_lost.pl';

Sponsored-by: Dalton McCaughey Library, Australia
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 15 Katrin Fischer 2023-08-03 15:58:06 UTC
This feels like a workaround for a specific use case or to achieve a certain behavior. 

We do have other means to lock a patron account now that is expired, either by using system preferences to block OPAC actions (BlockExpiredPatronOpacActions) or using LockExpiredDelay and cleanup_database.pl. Maybe one of these would achieve the wanted behaviour as well?

You can now also create custom patron restrictions. I feel using one of those would also be a bit cleaner than using lost and give a less confusing message in the OPAC.
Comment 16 Marcel de Rooy 2023-09-29 09:40:01 UTC
Please give feedback on last comment.
Comment 17 Alex Buckley 2023-10-23 17:08:49 UTC
Hi Katrin and Marcel, 

My apologies for my late response, I am catching back up now I am back from leave in Italy.

Yes I agree with you Katrin the suggestions you have noted are a cleaner option than this enhancement. That being the case I am going to close this bug report. 

Thank you both for your help.
Alex