Description
Fridolin Somers
2017-07-31 14:27:34 UTC
Created attachment 65351 [details] [review] Bug 19008 - More database cleanups - statistics misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for statistics table. Test plan : - Count statistics : select count(*),date(datetime) from statistics group by date(datetime); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --statistics 30 - Recount statistics Created attachment 65352 [details] [review] Bug 19008 - More database cleanups - deleted catalog misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for deleted catalog tables. Note that deletedbiblio_metadata is managed by foreign key on biblionumber. Test plan : - Count : select count(*),year(timestamp) from deleteditems group by year(timestamp); select count(*),year(timestamp) from deletedbiblio group by year(timestamp); select count(*),year(timestamp) from deletedbiblioitems group by year(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --deleted-catalog 30 - Recount Created attachment 65353 [details] [review] Bug 19008 - More database cleanups - deleted patrons misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for deletedborrowers table. Test plan : - Count : select count(*),date(updated_on) from deletedborrowers group by date(updated_on); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --delete-patrons 30 - Recount Created attachment 65354 [details] [review] Bug 19008 - More database cleanups - old issues misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for old_issues table. Test plan : - Count : select count(*),date(datetime) from old_issues group by date(datetime); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --old-issues 30 - Recount Created attachment 65355 [details] [review] Bug 19008 - More database cleanups - old reserves misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for old_reserves table. Test plan : - Count : select count(*),date(datetime) from old_reserves group by date(datetime); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --old-reserves 30 - Recount Created attachment 65356 [details] [review] Bug 19008 - More database cleanups - item transfers misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for branchtransfers table. Test plan : - Count : select count(*),datearrived from branchtransfers group by datearrived; - Run cleanup : misc/cronjobs/cleanup_database.pl -v --transfers 30 - Recount Works as intended for the tables statistics, deleted catalogs, branch transfers and deleted patrons (there is a typo in your test plan for this one : "- Run cleanup : misc/cronjobs/cleanup_database.pl -v --delete-patrons 30" is missing a "d" at --deleted-patrons). AS for the tables old_issues and old_reserves, I couldn't get the test done on mysql, the count returns "ERROR 1054 (42S22): Unknown column 'datetime' in 'field list'" for both tables. The cleanup seems to be working, but I just can't verify it in mysql. (In reply to Simon Pouchol from comment #7) > Works as intended for the tables statistics, deleted catalogs, branch > transfers and deleted patrons (there is a typo in your test plan for this > one : "- Run cleanup : misc/cronjobs/cleanup_database.pl -v --delete-patrons > 30" is missing a "d" at --deleted-patrons). I am tweaking the test plan with this correction. > AS for the tables old_issues and old_reserves, I couldn't get the test done > on mysql, the count returns "ERROR 1054 (42S22): Unknown column 'datetime' > in 'field list'" for both tables. The cleanup seems to be working, but I > just can't verify it in mysql. All these were test plan typos, likely caused by cut-and-paste. Reading the code, they should have been timestamp. I am tweaking the test plan to match. The code reads well. Passes Koha QA Test tool. And while I had no data to actually test the purge, the code ran perfectly fine. I'm signing off. Created attachment 68491 [details] [review] Bug 19008 - More database cleanups - statistics misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for statistics table. Test plan : - Count statistics : select count(*),date(datetime) from statistics group by date(datetime); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --statistics 30 - Recount statistics Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 68492 [details] [review] Bug 19008 - More database cleanups - deleted catalog misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for deleted catalog tables. Note that deletedbiblio_metadata is managed by foreign key on biblionumber. Test plan : - Count : select count(*),year(timestamp) from deleteditems group by year(timestamp); select count(*),year(timestamp) from deletedbiblio group by year(timestamp); select count(*),year(timestamp) from deletedbiblioitems group by year(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --deleted-catalog 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 68493 [details] [review] Bug 19008 - More database cleanups - deleted patrons misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for deletedborrowers table. Test plan : - Count : select count(*),date(updated_on) from deletedborrowers group by date(updated_on); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --deleted-patrons 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 68494 [details] [review] Bug 19008 - More database cleanups - old issues misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for old_issues table. Test plan : - Count : select count(*),date(timestamp) from old_issues group by date(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --old-issues 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 68495 [details] [review] Bug 19008 - More database cleanups - old reserves misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for old_reserves table. Test plan : - Count : select count(*),date(timestamp) from old_reserves group by date(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --old-reserves 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 68496 [details] [review] Bug 19008 - More database cleanups - item transfers misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for branchtransfers table. Test plan : - Count : select count(*),datearrived from branchtransfers group by datearrived; - Run cleanup : misc/cronjobs/cleanup_database.pl -v --transfers 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 71771 [details] [review] Bug 19008 - More database cleanups - statistics misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for statistics table. Test plan : - Count statistics : select count(*),date(datetime) from statistics group by date(datetime); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --statistics 30 - Recount statistics Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 71772 [details] [review] Bug 19008 - More database cleanups - deleted catalog misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for deleted catalog tables. Note that deletedbiblio_metadata is managed by foreign key on biblionumber. Test plan : - Count : select count(*),year(timestamp) from deleteditems group by year(timestamp); select count(*),year(timestamp) from deletedbiblio group by year(timestamp); select count(*),year(timestamp) from deletedbiblioitems group by year(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --deleted-catalog 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 71773 [details] [review] Bug 19008 - More database cleanups - deleted patrons misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for deletedborrowers table. Test plan : - Count : select count(*),date(updated_on) from deletedborrowers group by date(updated_on); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --deleted-patrons 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 71774 [details] [review] Bug 19008 - More database cleanups - old issues misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for old_issues table. Test plan : - Count : select count(*),date(timestamp) from old_issues group by date(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --old-issues 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 71775 [details] [review] Bug 19008 - More database cleanups - old reserves misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for old_reserves table. Test plan : - Count : select count(*),date(timestamp) from old_reserves group by date(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --old-reserves 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 71776 [details] [review] Bug 19008 - More database cleanups - item transfers misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for branchtransfers table. Test plan : - Count : select count(*),datearrived from branchtransfers group by datearrived; - Run cleanup : misc/cronjobs/cleanup_database.pl -v --transfers 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Please improve the POD to explain what means "older than DAYS days" for each option you added. Should we add notes about the consequences? I think old_issues might be used in some reports and deleting this data will remove information like patron history etc. Maybe it should come with a warning and some hints. (In reply to Jonathan Druart from comment #21) > Please improve the POD to explain what means "older than DAYS days" for each > option you added. Sorry but I don't see how it could be explained. Look at sessdays arg : --sessdays DAYS purge only sessions older than DAYS days. (In reply to Katrin Fischer from comment #22) > Should we add notes about the consequences? I think old_issues might be used > in some reports and deleting this data will remove information like patron > history etc. Maybe it should come with a warning and some hints. Indeed, I will add a warn for statistics saying it is used in reports and for old_issues that is used in reading history. (In reply to Fridolin SOMERS from comment #23) > (In reply to Jonathan Druart from comment #21) > > Please improve the POD to explain what means "older than DAYS days" for each > > option you added. > > Sorry but I don't see how it could be explained. > Look at sessdays arg : > --sessdays DAYS purge only sessions older than DAYS days. Yes existing POD is bad, that is not a valid argument :) + --statistics DAYS Purge entries from statistics older than DAYS days. => this one may be ok + --deleted-catalog DAYS Purge deleted catalog older than DAYS => This can be improved: Remove deleted catalogue records that have been removed before DAYS + --deleted-patrons DAYS Purge deleted patrons older than DAYS days. => So if they are too old? Let remove all the retired! + --old-issues DAYS Purge old issues older than DAYS days. => Then we are going to "purge the checkins that have been checked out" [...] + --old-reserves DAYS Purge old reserves older than DAYS days. => I am expecting "holds" and "filled" in the description + --transfers DAYS Purge arrived items transfers older than DAYS days. => Hum, any native English speakers around? Should we keep "issues" and "reserves" in the parameter? At least not in the POD. (In reply to Jonathan Druart from comment #25) Maybe like: Purge statistics entries more than DAYS days old Purge catalog records deleted more then DAYS days ago Purge patrons deleted more than DAYS days ago Purge checkouts (old_issues) returned more than DAYS days ago Purge transfers completed more than DAYS day ago
> Indeed, I will add a warn for statistics saying it is used in reports and
> for old_issues that is used in reading history.
+1
(In reply to Nick Clemens from comment #26) > (In reply to Jonathan Druart from comment #25) > > Maybe like: > > Purge statistics entries more than DAYS days old > > Purge catalog records deleted more then DAYS days ago > > Purge patrons deleted more than DAYS days ago > > Purge checkouts (old_issues) returned more than DAYS days ago > > Purge transfers completed more than DAYS day ago Fridolin: Any chance you can find the time to update the POD with these suggestions? We really need a way to clean up the data that these patches clean up. :-) Created attachment 91922 [details] [review] Bug 19008 - More database cleanups - statistics misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for statistics table. Test plan : - Count statistics : select count(*),date(datetime) from statistics group by date(datetime); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --statistics 30 - Recount statistics Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91923 [details] [review] Bug 19008 - More database cleanups - deleted catalog misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for deleted catalog tables. Note that deletedbiblio_metadata is managed by foreign key on biblionumber. Test plan : - Count : select count(*),year(timestamp) from deleteditems group by year(timestamp); select count(*),year(timestamp) from deletedbiblio group by year(timestamp); select count(*),year(timestamp) from deletedbiblioitems group by year(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --deleted-catalog 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91924 [details] [review] Bug 19008 - More database cleanups - deleted patrons misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for deletedborrowers table. Test plan : - Count : select count(*),date(updated_on) from deletedborrowers group by date(updated_on); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --deleted-patrons 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91925 [details] [review] Bug 19008 - More database cleanups - old issues misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for old_issues table. Test plan : - Count : select count(*),date(timestamp) from old_issues group by date(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --old-issues 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91926 [details] [review] Bug 19008 - More database cleanups - old reserves misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for old_reserves table. Test plan : - Count : select count(*),date(timestamp) from old_reserves group by date(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --old-reserves 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91927 [details] [review] Bug 19008 - More database cleanups - item transfers misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for branchtransfers table. Test plan : - Count : select count(*),datearrived from branchtransfers group by datearrived; - Run cleanup : misc/cronjobs/cleanup_database.pl -v --transfers 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 91928 [details] [review] Bug 19008 - More database cleanups - QA followup This patch tries to change the POD as suggested in the QA process. I have: - Resolved a pretty trivial conflict: $ git bz apply 19008 Bug 19008 - More database cleanups 71771 - Bug 19008 - More database cleanups - statistics 71772 - Bug 19008 - More database cleanups - deleted catalog 71773 - Bug 19008 - More database cleanups - deleted patrons 71774 - Bug 19008 - More database cleanups - old issues 71775 - Bug 19008 - More database cleanups - old reserves 71776 - Bug 19008 - More database cleanups - item transfers Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 19008 - More database cleanups - statistics Using index info to reconstruct a base tree... M misc/cronjobs/cleanup_database.pl Falling back to patching base and 3-way merge... Auto-merging misc/cronjobs/cleanup_database.pl CONFLICT (content): Merge conflict in misc/cronjobs/cleanup_database.pl error: Failed to merge in the changes. Patch failed at 0001 Bug 19008 - More database cleanups - statistics Use 'git am --show-current-patch' to see the failed patch - Tried to add POD as suggested by the fine QA folks Not sure if this should be Needs signoff or Signed off, setting it to the former. @Magnus Merci beaucoup ;) = Thanks a lot So I did a small rebase and added a followup with some POD. Setting the status to "Signed off". Please change it back if my changes require another signoff. This is clearly a step in the right direction, the cleanups added by these patches are sorely needed. But I think the actions of the cleanup-script should be governed by sysprefs, not command line switches. There are several reasons for this: - On a package install with several instances this means we can have one line in the cron-setup to trigger the script, with koha-foreach, but all the sites can have their individual configurations - Libraries that do not have hands on access to their Koha server can change the setup without going through their provider - And most importantly, I think: Libraries can tell what they are cleaning up and anonymizing, by looking at the sysprefs, instead of having to figure out what the command line switches are and do. (I *think* it is a requirement of the GDPR that libraries should be able to tell what they are doing with their data. Having a few sysprefs to look at seems way better than having to ask a provider what the command line switches of the cronjob are set to.) But priority number 1 should be getting these patches in in the first place, then we can do the enhancement on another bug. Created attachment 98373 [details] [review] Bug 19008: More database cleanups - statistics misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for statistics table. Test plan : - Count statistics : select count(*),date(datetime) from statistics group by date(datetime); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --statistics 30 - Recount statistics Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 98374 [details] [review] Bug 19008: More database cleanups - deleted catalog misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for deleted catalog tables. Note that deletedbiblio_metadata is managed by foreign key on biblionumber. Test plan : - Count : select count(*),year(timestamp) from deleteditems group by year(timestamp); select count(*),year(timestamp) from deletedbiblio group by year(timestamp); select count(*),year(timestamp) from deletedbiblioitems group by year(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --deleted-catalog 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 98375 [details] [review] Bug 19008: More database cleanups - deleted patrons misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for deletedborrowers table. Test plan : - Count : select count(*),date(updated_on) from deletedborrowers group by date(updated_on); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --deleted-patrons 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 98376 [details] [review] Bug 19008: More database cleanups - old issues misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for old_issues table. Test plan : - Count : select count(*),date(timestamp) from old_issues group by date(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --old-issues 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 98377 [details] [review] Bug 19008: More database cleanups - old reserves misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for old_reserves table. Test plan : - Count : select count(*),date(timestamp) from old_reserves group by date(timestamp); - Run cleanup : misc/cronjobs/cleanup_database.pl -v --old-reserves 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 98378 [details] [review] Bug 19008: More database cleanups - item transfers misc/cronjobs/cleanup_database.pl provides some database cleanup. Yet some tables that could need cleanup are not in this script. This patch adds cleanup for branchtransfers table. Test plan : - Count : select count(*),datearrived from branchtransfers group by datearrived; - Run cleanup : misc/cronjobs/cleanup_database.pl -v --transfers 30 - Recount Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 98379 [details] [review] Bug 19008: More database cleanups - QA followup This patch tries to change the POD as suggested in the QA process. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 98380 [details] [review] Bug 19008: Replace issues with checkouts in print statement Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Nice work everyone! Pushed to master for 20.05 Enhancement not backported to 19.11.x *** Bug 13667 has been marked as a duplicate of this bug. *** (In reply to Joy Nelson from comment #49) > Enhancement not backported to 19.11.x Hi, can we reconsider this ? Its needed for GDPR cleanups like old_reserves. Requested this be pushed to 19.11.x for GDPR. Backported to 19.11.x for 19.11.05 assuming this is wanted in 19.05.x as well. backported to 19.05.x for 19.05.10 Enhancement not backported to 18.11.x series - let me know if you'd like me to reconsider |