Maybe put the number of days in a syspref, make the cron check the syspref when run?

(In reply to Andrew Fuerste-Henry from comment #1)
> Maybe put the number of days in a syspref, make the cron check the syspref
> when run?

+1

Created attachment 104611 [details] [review]
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days

Add option to cleanup_database script to removed 'resolved' return claims from the database after a specified number of days.

Test Plan:
1) Apply this patch
2) Test cleanup_database with the new --return-claims DAYS option

Created attachment 104612 [details] [review]
Bug 25429: Fix typos, correct spacing, add missing periods for script help

Hi Kyle, did you see comments #1 and #2? I think it would be really great if we could have this as a system preference - it would make things much easier with multiple instances on a server.

Created attachment 104798 [details] [review]
Bug 25429: Implement system preference, remove command line switch

Kyle, can you please rebase this? Thanks!

Created attachment 114601 [details] [review]
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days

Add option to cleanup_database script to removed 'resolved' return claims from the database after a specified number of days.

Test Plan:
1) Apply this patch
2) Test cleanup_database with the new --return-claims DAYS option

Created attachment 114602 [details] [review]
Bug 25429: Implement system preference, remove command line switch

After applying this patch, I get the error "unknown option: return-claims" when I run "perl misc/cronjobs/cleanup_database.pl --return-claims 40 --confirm"

Created attachment 115042 [details] [review]
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days

Add option to cleanup_database script to removed 'resolved' return claims from the database after a specified number of days.

Test Plan:
1) Apply this patch
2) Set the new syspref CleanUpDatabaseReturnClaims to a number of days
3) Run cleanup_database.pl
4) Note resolved claims older than the specified number of days are removed from the database

Bug 25429: Implement system preference, remove command line switch

Can we make the cron require a --claims flag or something comparable? It seems likely to create confusion down the line to have this cron perform an action based on a syspref value without some obvious indicator in the cron line itself. 

Additionally, as it is now I am unable to remove resolved claims without *also* doing something else with the cron. If I run this with just "--confirm," the cron just spits out the help text and wants me to try again with something actually set. Not that anyone's likely to be running this with no other flags, but it's a confusion easily avoided.

(In reply to Andrew Fuerste-Henry from comment #12)
> Can we make the cron require a --claims flag or something comparable? It
> seems likely to create confusion down the line to have this cron perform an
> action based on a syspref value without some obvious indicator in the cron
> line itself. 
> 
> Additionally, as it is now I am unable to remove resolved claims without
> *also* doing something else with the cron. If I run this with just
> "--confirm," the cron just spits out the help text and wants me to try again
> with something actually set. Not that anyone's likely to be running this
> with no other flags, but it's a confusion easily avoided.

I don't understand the purpose of moving the value to a syspref if it still requires a cli flag to run.

Created attachment 115086 [details] [review]
Bug 25429: Script should be able to run with only return claims

Created attachment 115087 [details] [review]
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days

Add option to cleanup_database script to removed 'resolved' return claims from the database after a specified number of days.

Test Plan:
1) Apply this patch
2) Set the new syspref CleanUpDatabaseReturnClaims to a number of days
3) Run cleanup_database.pl
4) Note resolved claims older than the specified number of days are removed from the database

Bug 25429: Implement system preference, remove command line switch

Created attachment 115088 [details] [review]
Bug 25429: Script should be able to run with only return claims

Created attachment 115089 [details] [review]
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days

Add option to cleanup_database script to removed 'resolved' return claims from the database after a specified number of days.

Test Plan:
1) Apply this patch
2) Set the new syspref CleanUpDatabaseReturnClaims to a number of days
3) Run cleanup_database.pl
4) Note resolved claims older than the specified number of days are removed from the database

Bug 25429: Implement system preference, remove command line switch

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 115090 [details] [review]
Bug 25429: Script should be able to run with only return claims

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 115907 [details] [review]
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days

Add option to cleanup_database script to removed 'resolved' return claims from the database after a specified number of days.

Test Plan:
1) Apply this patch
2) Set the new syspref CleanUpDatabaseReturnClaims to a number of days
3) Run cleanup_database.pl
4) Note resolved claims older than the specified number of days are removed from the database

Bug 25429: Implement system preference, remove command line switch

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>

Created attachment 115908 [details] [review]
Bug 25429: Script should be able to run with only return claims

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>

Created attachment 118880 [details] [review]
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days

Add option to cleanup_database script to removed 'resolved' return claims from the database after a specified number of days.

Test Plan:
1) Apply this patch
2) Set the new syspref CleanUpDatabaseReturnClaims to a number of days
3) Run cleanup_database.pl
4) Note resolved claims older than the specified number of days are removed from the database

Bug 25429: Implement system preference, remove command line switch

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 118881 [details] [review]
Bug 25429: Script should be able to run with only return claims

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

It works, tested with various values for edge cases.
Code looks good
QA script ok

Passed!

1.
Please use the same syntax as the other entries.
You need to use filter_by_last_update (inherited from Koha::Objects) to get the object set. It will offer a better verbose and confirm support.

See the "$pOldIssues" block for a good example.

2.
Why don't we add an option to the script, like the other flags? I would have expected a default number of days picked from the pref, and a flag passed to the script.

(In reply to Jonathan Druart from comment #24)
> 2.
> Why don't we add an option to the script, like the other flags? I would have
> expected a default number of days picked from the pref, and a flag passed to
> the script.

ok, I read the previous comments now. But... isn't how the whole script work for the other flags so far?

for instance:
190 $pDebarments       = DEFAULT_DEBARMENTS_PURGEDAYS         if defined($pDebarments)       && $pDebarments == 0;

Created attachment 119219 [details] [review]
Bug 25429: Use filter_by_last_update

(In reply to Jonathan Druart from comment #25)
> (In reply to Jonathan Druart from comment #24)
> > 2.
> > Why don't we add an option to the script, like the other flags? I would have
> > expected a default number of days picked from the pref, and a flag passed to
> > the script.
> 
> ok, I read the previous comments now. But... isn't how the whole script work
> for the other flags so far?
> 
> for instance:
> 190 $pDebarments       = DEFAULT_DEBARMENTS_PURGEDAYS         if
> defined($pDebarments)       && $pDebarments == 0;

This is what I was asked to implement, so I did. Koha is on a trajectory of moving away from cli paramters for cronjobs to sysprefs so that librarians can control how cronjobs behave without the need to have a sysadmin make changes for them. I fully expect over time all the parameters for this script will be moved to sysprefs. This is just the first step.

(In reply to Kyle M Hall from comment #27)
> (In reply to Jonathan Druart from comment #25)
> > (In reply to Jonathan Druart from comment #24)
> > > 2.
> > > Why don't we add an option to the script, like the other flags? I would have
> > > expected a default number of days picked from the pref, and a flag passed to
> > > the script.
> > 
> > ok, I read the previous comments now. But... isn't how the whole script work
> > for the other flags so far?
> > 
> > for instance:
> > 190 $pDebarments       = DEFAULT_DEBARMENTS_PURGEDAYS         if
> > defined($pDebarments)       && $pDebarments == 0;
> 
> This is what I was asked to implement, so I did. Koha is on a trajectory of
> moving away from cli paramters for cronjobs to sysprefs so that librarians
> can control how cronjobs behave without the need to have a sysadmin make
> changes for them. I fully expect over time all the parameters for this
> script will be moved to sysprefs. This is just the first step.

Hi, 

when I commented on wanting this to be a pref, I was thinking of making it work like the  PurgeSuggestionsOlderThan preference.

In this case a comment line switch is there, but the number of days and activation for the instance is handled by the pref.

I like this solution best, because:

- As an admin, you can turn off all deletions easily by removing the switch from the cron, without every library having to change their configuration. This might be handy sometimes (think COVID...)
- The libraries have the ability to turn the feature off indidvidually by leaving it empty and set it to the desirable number of days if they want deletions. I think this speaks against a default value of days and I expect that there is not a common default value anyway that would make sense to most libraries.

Adjusting status regarding the last comment.

Created attachment 120927 [details] [review]
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days

Add option to cleanup_database script to removed 'resolved' return claims from the database after a specified number of days.

Test Plan:
1) Apply this patch
2) Set the new syspref CleanUpDatabaseReturnClaims to a number of days
3) Run cleanup_database.pl
4) Note resolved claims older than the specified number of days are removed from the database

Bug 25429: Implement system preference, remove command line switch

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 120928 [details] [review]
Bug 25429: Script should be able to run with only return claims

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Created attachment 120929 [details] [review]
Bug 25429: Use filter_by_last_update

Created attachment 120930 [details] [review]
Bug 25429: Require command line switch to enable, control DAYS via syspref

Comment on attachment 120930 [details] [review]
Bug 25429: Require command line switch to enable, control DAYS via syspref

Review of attachment 120930 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/cleanup_database.pl
@@ +532,4 @@
>      }
>  }
>  
> +$return_claims = C4::Context->preference('CleanUpDatabaseReturnClaims') if $return_claims;

This doesn't make sense to me.

It means that you need to have a sysadmin change a cronjob for ALL instances if there's just 1 instance that wants to be able to use the system preference "CleanUpDatabaseReturnClaims".

(In reply to David Cook from comment #34)
> This doesn't make sense to me.
> 
> It means that you need to have a sysadmin change a cronjob for ALL instances
> if there's just 1 instance that wants to be able to use the system
> preference "CleanUpDatabaseReturnClaims".

To me, that means a custom crontab for vendors, which is not nice from a management perspective.

And it means that people who rely on volunteers or shared sysadmins (or no sysadmins) can't get the functionality.

David, basically you are asking for another switch in koha-conf that would be off by default, that's it?

(In reply to David Cook from comment #35)
> (In reply to David Cook from comment #34)
> > This doesn't make sense to me.
> > 
> > It means that you need to have a sysadmin change a cronjob for ALL instances
> > if there's just 1 instance that wants to be able to use the system
> > preference "CleanUpDatabaseReturnClaims".
> 
> To me, that means a custom crontab for vendors, which is not nice from a
> management perspective.
> 
> And it means that people who rely on volunteers or shared sysadmins (or no
> sysadmins) can't get the functionality.

(In reply to David Cook from comment #34)
> Comment on attachment 120930 [details] [review] [review]
> Bug 25429: Require command line switch to enable, control DAYS via syspref
> 
> Review of attachment 120930 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: misc/cronjobs/cleanup_database.pl
> @@ +532,4 @@
> >      }
> >  }
> >  
> > +$return_claims = C4::Context->preference('CleanUpDatabaseReturnClaims') if $return_claims;
> 
> This doesn't make sense to me.
> 
> It means that you need to have a sysadmin change a cronjob for ALL instances
> if there's just 1 instance that wants to be able to use the system
> preference "CleanUpDatabaseReturnClaims".

You are incorrect. It means that an administrator can disable that behavior for all instances. However, if the option is enabled on the command line, the system preference then takes over as the control. If the syspref is empty, the feature is disabled. If the syspref has a value, the feature is enabled.

This specific behavior was requested by a member of the QA team.

Guess that was me and I like the description of it - not sure I understand the problem pointed out by Jonathan. Maybe you can explain a bit more? We can add the switch to new installations by default and we can still deactivate/activate on demand. It would work like the self reg one now does too (point for consistency).

(In reply to Kyle M Hall from comment #37)
> You are incorrect. It means that an administrator can disable that behavior
> for all instances. However, if the option is enabled on the command line,
> the system preference then takes over as the control. If the syspref is
> empty, the feature is disabled. If the syspref has a value, the feature is
> enabled.
> 

The patch literally says 'Bug 25429: Require command line switch to enable, control DAYS via syspref" and the code supports that. 

That means that a sysadmin must add the CLI switch in order for the syspref to be usable, and this switch is not enabled out of the box. 

Can you show me how I'm incorrect because I'm not seeing it.

(In reply to Katrin Fischer from comment #38)
> Guess that was me and I like the description of it - not sure I understand
> the problem pointed out by Jonathan. Maybe you can explain a bit more? We
> can add the switch to new installations by default and we can still
> deactivate/activate on demand. It would work like the self reg one now does
> too (point for consistency).

I think the self-reg is a suboptimal implementation, which is why I've opened bug 28457.

We shouldn't require sysadmins to change crontabs in order to get cronjobs to work using system preferences. 

It defeats the whole point of Bug 24968.

(In reply to Jonathan Druart from comment #36)
> David, basically you are asking for another switch in koha-conf that would
> be off by default, that's it?

I'm not sure what you mean by "asking for another switch in koha-conf".

(In reply to David Cook from comment #40)
> (In reply to Katrin Fischer from comment #38)
> > Guess that was me and I like the description of it - not sure I understand
> > the problem pointed out by Jonathan. Maybe you can explain a bit more? We
> > can add the switch to new installations by default and we can still
> > deactivate/activate on demand. It would work like the self reg one now does
> > too (point for consistency).
> 
> I think the self-reg is a suboptimal implementation, which is why I've
> opened bug 28457.
> 
> We shouldn't require sysadmins to change crontabs in order to get cronjobs
> to work using system preferences. 
> 
> It defeats the whole point of Bug 24968.

Ok, I am not going to insist on the switch any longer. I've also added the topic to the dev meeting agenda a few days ago, because I think we need to reach a decision that will unlock the 2 waiting bugs and more to come for good.

But... from our own experience: especially the self reg ones are quite dangerous. If you put a 'wrong' patron category type into the pref, it will delete a big chunk of your users without batting an eye (and they are delete-deleted, not moved).

(In reply to Katrin Fischer from comment #42)
> But... from our own experience: especially the self reg ones are quite
> dangerous. If you put a 'wrong' patron category type into the pref, it will
> delete a big chunk of your users without batting an eye (and they are
> delete-deleted, not moved).

I think the "Delete unverified self registrations older than DAYS" should be safe either way?

But "Delete expired self registration accounts" does sound dangerous as you describe. Personally, I don't see the merit of having that in cleanup_database.pl at all. Even with a sysadmin adding that flag, you can't control the behaviour of library staff.

> But "Delete expired self registration accounts" does sound dangerous as you
> describe. Personally, I don't see the merit of having that in
> cleanup_database.pl at all. Even with a sysadmin adding that flag, you can't
> control the behaviour of library staff.

I think having all "delete" and clean-up things together is ok, especially when we move things into the GUI. But it requires good documentation for sure. We added a hint to the system preference, maybe we should add one to the script as well for now.

(In reply to David Cook from comment #41)
> (In reply to Jonathan Druart from comment #36)
> > David, basically you are asking for another switch in koha-conf that would
> > be off by default, that's it?
> 
> I'm not sure what you mean by "asking for another switch in koha-conf".

I don't want to add more confusion and I guess it wouldn't have answered the need anyway.
I thought you were asking for more flexibility and so adding a new switch in koha-conf.xml to allow the syspref to have precedence over the script flag.

I would like another QA stamp and clarification on what we need here. I won't have time to dedicate to this in the next couple of weeks.

QA: Looking here

It might be completely obvious for people using this feature daily how it works, but since not everybody does, a little bit more test plan would have been helpful.

I read the discussion. Yes, there is a trend to put values in the prefs and no longer on the cmdline. We are in a hybrid situation now (as we are with many things).
Asking to put something in koha-conf too would be rather tedious imo.
This looks good to me.
Adding just a small follow-up including my test plan.

Created attachment 125584 [details] [review]
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days

Add option to cleanup_database script to removed 'resolved' return claims from the database after a specified number of days.

Test Plan:
1) Apply this patch
2) Set the new syspref CleanUpDatabaseReturnClaims to a number of days
3) Run cleanup_database.pl
4) Note resolved claims older than the specified number of days are removed from the database

Bug 25429: Implement system preference, remove command line switch

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

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

Created attachment 125585 [details] [review]
Bug 25429: Script should be able to run with only return claims

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Rebecca Coert <rcoert@arlingtonva.us>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

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

Created attachment 125586 [details] [review]
Bug 25429: Use filter_by_last_update

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

Created attachment 125587 [details] [review]
Bug 25429: Require command line switch to enable, control DAYS via syspref

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

Created attachment 125588 [details] [review]
Bug 25429: (QA follow-up) Add extra variable for days

I think it is a bit more clear to use another variable for the
number of days from the preferences than overwriting the flag
variable.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested by adding an authval for LOST.
Filling prefs ClaimReturnedLostValue, CleanUpDatabaseReturnClaims.
Claiming a return, resolving it. Setting date back via SQL.
Running the script with -v --return-claims, toggling --confirm.
The claim is counted and deleted with confirm.

Pushed to master for 21.11, thanks to everybody involved!

Wondering: return-claims or return_claims?


+   --return-claims         Purge all resolved return claims older than the number of days specified in
+                           the system preference CleanUpDatabaseReturnClaims.

vs.

+$return_claims = C4::Context->preference('CleanUpDatabaseReturnClaims') if $return_claims;
 if ($return_claims) {
     print "Purging return claims older than $return_claims days.\n" if $verbose;

return-claims