Bug 25429 - `cleanup_database.pl` should remove resolved claims returned after X days
Summary: `cleanup_database.pl` should remove resolved claims returned after X days
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-08 16:57 UTC by Joy Nelson
Modified: 2022-12-12 21:23 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds the new `CleanUpDatabaseReturnClaims` system preference allowing administrators to specify how many days after resolution a claimed return record should be deleted from the database. For this functionality to be enabled, the `cleanup_database.pl` must be scheduled to run regularly with the new `--return-claims` parameter passed.
Version(s) released in:
21.11.00


Attachments
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days (2.86 KB, patch)
2020-05-08 18:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Fix typos, correct spacing, add missing periods for script help (6.67 KB, patch)
2020-05-08 18:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Implement system preference, remove command line switch (4.56 KB, patch)
2020-05-12 18:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days (2.83 KB, patch)
2020-12-22 12:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Implement system preference, remove command line switch (4.06 KB, patch)
2020-12-22 12:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days (5.30 KB, patch)
2021-01-11 16:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Script should be able to run with only return claims (783 bytes, patch)
2021-01-12 12:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days (5.30 KB, patch)
2021-01-12 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Script should be able to run with only return claims (783 bytes, patch)
2021-01-12 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days (5.38 KB, patch)
2021-01-12 13:08 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25429: Script should be able to run with only return claims (833 bytes, patch)
2021-01-12 13:08 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days (5.44 KB, patch)
2021-01-27 16:45 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25429: Script should be able to run with only return claims (887 bytes, patch)
2021-01-27 16:45 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days (5.46 KB, patch)
2021-03-26 20:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25429: Script should be able to run with only return claims (945 bytes, patch)
2021-03-26 20:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25429: Use filter_by_last_update (1.44 KB, patch)
2021-04-06 13:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days (5.48 KB, patch)
2021-05-13 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Script should be able to run with only return claims (957 bytes, patch)
2021-05-13 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Use filter_by_last_update (1.43 KB, patch)
2021-05-13 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Require command line switch to enable, control DAYS via syspref (1.89 KB, patch)
2021-05-13 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25429: Cleanup Database - remove resolved claims returned from db after X days (5.56 KB, patch)
2021-10-01 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 25429: Script should be able to run with only return claims (1.01 KB, patch)
2021-10-01 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 25429: Use filter_by_last_update (1.51 KB, patch)
2021-10-01 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 25429: Require command line switch to enable, control DAYS via syspref (1.97 KB, patch)
2021-10-01 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 25429: (QA follow-up) Add extra variable for days (1.65 KB, patch)
2021-10-01 09:02 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 Joy Nelson 2020-05-08 16:57:25 UTC
Add option to cleanup_database script to removed 'resolved' return claims from the database after a specified number of days.
Comment 1 Andrew Fuerste-Henry 2020-05-08 16:59:32 UTC
Maybe put the number of days in a syspref, make the cron check the syspref when run?
Comment 2 Katrin Fischer 2020-05-08 17:23:01 UTC
(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
Comment 3 Kyle M Hall 2020-05-08 18:09:20 UTC
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
Comment 4 Kyle M Hall 2020-05-08 18:09:29 UTC
Created attachment 104612 [details] [review]
Bug 25429: Fix typos, correct spacing, add missing periods for script help
Comment 5 Katrin Fischer 2020-05-09 17:49:49 UTC
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.
Comment 6 Kyle M Hall 2020-05-12 18:59:46 UTC
Created attachment 104798 [details] [review]
Bug 25429: Implement system preference, remove command line switch
Comment 7 Andrew Fuerste-Henry 2020-12-21 21:03:16 UTC
Kyle, can you please rebase this? Thanks!
Comment 8 Kyle M Hall 2020-12-22 12:30:41 UTC
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
Comment 9 Kyle M Hall 2020-12-22 12:30:51 UTC
Created attachment 114602 [details] [review]
Bug 25429: Implement system preference, remove command line switch
Comment 10 Andrew Fuerste-Henry 2020-12-23 15:52:38 UTC
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"
Comment 11 Kyle M Hall 2021-01-11 16:09:03 UTC
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
Comment 12 Andrew Fuerste-Henry 2021-01-11 20:04:19 UTC
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.
Comment 13 Kyle M Hall 2021-01-12 12:53:38 UTC
(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.
Comment 14 Kyle M Hall 2021-01-12 12:55:42 UTC
Created attachment 115086 [details] [review]
Bug 25429: Script should be able to run with only return claims
Comment 15 Kyle M Hall 2021-01-12 12:56:51 UTC
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
Comment 16 Kyle M Hall 2021-01-12 12:57:01 UTC
Created attachment 115088 [details] [review]
Bug 25429: Script should be able to run with only return claims
Comment 17 Andrew Fuerste-Henry 2021-01-12 13:08:39 UTC
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>
Comment 18 Andrew Fuerste-Henry 2021-01-12 13:08:42 UTC
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>
Comment 19 Andrew Fuerste-Henry 2021-01-27 16:45:42 UTC
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>
Comment 20 Andrew Fuerste-Henry 2021-01-27 16:45:45 UTC
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>
Comment 21 Victor Grousset/tuxayo 2021-03-26 20:36:48 UTC
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>
Comment 22 Victor Grousset/tuxayo 2021-03-26 20:36:52 UTC
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>
Comment 23 Victor Grousset/tuxayo 2021-03-26 20:37:46 UTC
It works, tested with various values for edge cases.
Code looks good
QA script ok

Passed!
Comment 24 Jonathan Druart 2021-04-06 10:13:19 UTC
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.
Comment 25 Jonathan Druart 2021-04-06 10:16:12 UTC
(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;
Comment 26 Kyle M Hall 2021-04-06 13:17:36 UTC
Created attachment 119219 [details] [review]
Bug 25429: Use filter_by_last_update
Comment 27 Kyle M Hall 2021-04-06 13:20:52 UTC
(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.
Comment 28 Katrin Fischer 2021-04-07 12:31:49 UTC
(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.
Comment 29 Jonathan Druart 2021-05-06 13:51:00 UTC
Adjusting status regarding the last comment.
Comment 30 Kyle M Hall 2021-05-13 13:33:17 UTC
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>
Comment 31 Kyle M Hall 2021-05-13 13:33:33 UTC
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>
Comment 32 Kyle M Hall 2021-05-13 13:33:37 UTC
Created attachment 120929 [details] [review]
Bug 25429: Use filter_by_last_update
Comment 33 Kyle M Hall 2021-05-13 13:33:41 UTC
Created attachment 120930 [details] [review]
Bug 25429: Require command line switch to enable, control DAYS via syspref
Comment 34 David Cook 2021-05-27 23:38:06 UTC
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".
Comment 35 David Cook 2021-05-27 23:40:14 UTC
(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.
Comment 36 Jonathan Druart 2021-05-28 06:12:16 UTC
David, basically you are asking for another switch in koha-conf that would be off by default, that's it?
Comment 37 Kyle M Hall 2021-05-28 10:57:26 UTC
(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.
Comment 38 Katrin Fischer 2021-05-29 13:48:57 UTC
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).
Comment 39 David Cook 2021-05-31 00:24:46 UTC
(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.
Comment 40 David Cook 2021-05-31 00:26:10 UTC
(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.
Comment 41 David Cook 2021-05-31 00:26:42 UTC
(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".
Comment 42 Katrin Fischer 2021-05-31 09:37:08 UTC
(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).
Comment 43 David Cook 2021-06-01 02:12:46 UTC
(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.
Comment 44 Katrin Fischer 2021-06-01 09:35:10 UTC
> 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.
Comment 45 Jonathan Druart 2021-06-02 14:33:42 UTC
(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.
Comment 46 Jonathan Druart 2021-06-02 14:34:45 UTC
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.
Comment 47 Marcel de Rooy 2021-10-01 07:59:15 UTC
QA: Looking here
Comment 48 Marcel de Rooy 2021-10-01 08:53:30 UTC
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.
Comment 49 Marcel de Rooy 2021-10-01 08:57:47 UTC
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.
Comment 50 Marcel de Rooy 2021-10-01 09:02:40 UTC
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>
Comment 51 Marcel de Rooy 2021-10-01 09:02:44 UTC
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>
Comment 52 Marcel de Rooy 2021-10-01 09:02:49 UTC
Created attachment 125586 [details] [review]
Bug 25429: Use filter_by_last_update

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2021-10-01 09:02:53 UTC
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>
Comment 54 Marcel de Rooy 2021-10-01 09:02:57 UTC
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.
Comment 55 Jonathan Druart 2021-10-06 12:56:42 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 56 Katrin Fischer 2021-11-22 22:26:45 UTC
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;
Comment 57 Jonathan Druart 2021-11-23 11:09:08 UTC
return-claims