Bug 32560

Summary: Mark expired borrowers as having lost their card
Product: Koha Reporter: Alex Buckley <alexbuckley>
Component: Command-line UtilitiesAssignee: Alex Buckley <alexbuckley>
Status: RESOLVED WORKSFORME QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david, kyle, m.de.rooy, robin
Version: Main   
Hardware: All   
OS: All   
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
Bug 32560: Cronjob to mark expired borrowers as lost their card
Bug 32560: (follow-up) Altered wording of output
Bug 32560: Cronjob to mark expired borrowers as lost their card
Bug 32560: (follow-up) Altered wording of output
Bug 32560: (follow-up) Replace raw SQL queries with Koha::Patrons search and update
Bug 32560: Cronjob to mark expired borrowers as lost their card
Bug 32560: (follow-up) Altered wording of output
Bug 32560: (follow-up) Replace raw SQL queries with Koha::Patrons search and update

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