Add an option to cleanup_database.pl to purge the search_history entries older than X days.
Created attachment 18424 [details] [review] Bug 10361 - Add Option to cleanup_database.pl to purge search_history entries Add an option to cleanup_database.pl to purge the search_history entries older than X days. Test plan: - Apply patch - Check that your test DB has some entries a little older than 30 days and a few ones even older than that in search_history: SELECT * FROM search_history WHERE time < DATE_SUB( NOW(), INTERVAL 30 DAY ); If not, modify some existing entries. - Run cleanup_database with a fixed number of days (replace XX with something higher than 30) /misc/cronjobs/cleanup_database.pl --searchhistory XX - check that entries older than XX days got deleted from search_history - run without the day parameter /misc/cronjobs/cleanup_database.pl --searchhistory - check that entries older than 30 days got deleted from search_history - Signoff
"DELETE FROM search_history" query should go to C4::Search.pm, preferrably with a test (but not a show stopper)
Actually I was thinking it should go into some kind of privacy section in the future. There is more personal data that may have to be deleted on a regular basis to meet privacy/data protection requirements. I see the relation to Search.pm but it is not the place I would have been looking for this kind of feature. And Search.pm is bloated as it is. I'd welcome more comments on this.
From IRC: [06:54:14] <drojf> @later tell gmcharlt could you have a look at the comments on bug 10361 please? do you agree i should move the query to Search.pm? ... [07:16:14] <gmcharlt> to answer your question, if you don't want to spend too much effort at it, I agree with Srdjan that it should go into C4::Search -- that's where AddSearchHistory and GetSearchHistory live at the moment [07:17:05] <gmcharlt> if you want to take a little more time and refactor, maybe put it in a new module called Koha::Search::History on thel ike
Created attachment 18838 [details] [review] Bug 10361 Add Option to cleanup_database.pl to purge search_history entries Add an option to cleanup_database.pl to purge the search_history entries older than X days. Revised version, moved DELETE FROM search_history to Search.pm Test plan: - Apply patch - Check that your test DB has some entries a little older than 30 days and a few ones even older than that in search_history: SELECT * FROM search_history WHERE time < DATE_SUB( NOW(), INTERVAL 30 DAY ); If not, modify some existing entries. - Run cleanup_database with a fixed number of days (replace XX with something higher than 30) /misc/cronjobs/cleanup_database.pl --searchhistory XX - check that entries older than XX days got deleted from search_history - run without the day parameter /misc/cronjobs/cleanup_database.pl --searchhistory - check that entries older than 30 days got deleted from search_history - Signoff
Created attachment 18839 [details] [review] Bug 10361 Add Option to cleanup_database.pl to purge search_history entries Add an option to cleanup_database.pl to purge the search_history entries older than X days. Revised version, moved DELETE FROM search_history to Search.pm Test plan: - Apply patch - Check that your test DB has some entries a little older than 30 days and a few ones even older than that in search_history: SELECT * FROM search_history WHERE time < DATE_SUB( NOW(), INTERVAL 30 DAY ); If not, modify some existing entries. - Run cleanup_database with a fixed number of days (replace XX with something higher than 30) /misc/cronjobs/cleanup_database.pl --searchhistory XX - check that entries older than XX days got deleted from search_history - run without the day parameter /misc/cronjobs/cleanup_database.pl --searchhistory - check that entries older than 30 days got deleted from search_history - Signoff Rebased to master.
(also removes a duplicate export of &SimpleSearch from Search.pm)
Created attachment 20067 [details] [review] Bug 10361: (follow-up) add tests for search history functions This adds tests for the new PurgeSearchHistory function and AddSearchHistory. GetSearchHistory is not tested due to its current broken state (see bug 10667). To test: [1] Run prove -v t/db_dependent/Search_SearchHistory.t [2] Verify that all tests pass. Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 20068 [details] [review] Bug 10361: Add option to cleanup_database.pl to purge OPAC search history Add an option to cleanup_database.pl to purge the search_history entries older than X days. Test plan: - Apply patch - Check that your test DB has some entries a little older than 30 days and a few ones even older than that in search_history: SELECT * FROM search_history WHERE time < DATE_SUB( NOW(), INTERVAL 30 DAY ); If not, modify some existing entries. - Run cleanup_database with a fixed number of days (replace XX with something higher than 30) /misc/cronjobs/cleanup_database.pl --searchhistory XX - Check that entries older than XX days got deleted from search_history - Run without the day parameter /misc/cronjobs/cleanup_database.pl --searchhistory - Check that entries older than 30 days got deleted from search_history Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 20069 [details] [review] Bug 10361: (follow-up) add tests for search history functions This adds tests for the new PurgeSearchHistory function and AddSearchHistory. GetSearchHistory is not tested due to its current broken state (see bug 10667). To test: [1] Run prove -v t/db_dependent/Search_SearchHistory.t [2] Verify that all tests pass. Signed-off-by: Galen Charlton <gmc@esilibrary.com>
I've signed off on the patch and added a follow-up that adds some tests.
Created attachment 20258 [details] [review] Bug 10361: (follow-up) add tests for search history functions This adds tests for the new PurgeSearchHistory function and AddSearchHistory. GetSearchHistory is not tested due to its current broken state (see bug 10667). To test: [1] Run prove -v t/db_dependent/Search_SearchHistory.t [2] Verify that all tests pass. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
t/db_dependent/Search_SearchHistory.t .. 1..6 ok 1 - use C4::Search; ok 2 - starting off with nothing in search_history ok 3 - successfully added four search_history rows ok 4 - purged history older than 45 days ok 5 - purged history older than 30 days ok 6 - purged all history ok All tests successful. Thanks Galen for adding the tests!
Created attachment 20740 [details] [review] Bug 10361: Add option to cleanup_database.pl to purge OPAC search history Add an option to cleanup_database.pl to purge the search_history entries older than X days. Test plan: - Apply patch - Check that your test DB has some entries a little older than 30 days and a few ones even older than that in search_history: SELECT * FROM search_history WHERE time < DATE_SUB( NOW(), INTERVAL 30 DAY ); If not, modify some existing entries. - Run cleanup_database with a fixed number of days (replace XX with something higher than 30) /misc/cronjobs/cleanup_database.pl --searchhistory XX - Check that entries older than XX days got deleted from search_history - Run without the day parameter /misc/cronjobs/cleanup_database.pl --searchhistory - Check that entries older than 30 days got deleted from search_history Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 20741 [details] [review] Bug 10361: (follow-up) add tests for search history functions This adds tests for the new PurgeSearchHistory function and AddSearchHistory. GetSearchHistory is not tested due to its current broken state (see bug 10667). To test: [1] Run prove -v t/db_dependent/Search_SearchHistory.t [2] Verify that all tests pass. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master. Thanks, Mirko!