Bug 10361 - Add Option to cleanup_database.pl to purge search_history entries
Summary: Add Option to cleanup_database.pl to purge search_history entries
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Mirko Tietgen
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-28 12:33 UTC by Mirko Tietgen
Modified: 2014-12-07 20:02 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10361 - Add Option to cleanup_database.pl to purge search_history entries (5.76 KB, patch)
2013-05-28 12:37 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 10361 Add Option to cleanup_database.pl to purge search_history entries (7.23 KB, patch)
2013-06-11 13:27 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 10361 Add Option to cleanup_database.pl to purge search_history entries (5.26 KB, patch)
2013-06-11 13:36 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 10361: (follow-up) add tests for search history functions (3.42 KB, patch)
2013-08-02 16:25 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10361: Add option to cleanup_database.pl to purge OPAC search history (5.24 KB, patch)
2013-08-02 16:26 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10361: (follow-up) add tests for search history functions (3.42 KB, patch)
2013-08-02 16:27 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10361: (follow-up) add tests for search history functions (3.49 KB, patch)
2013-08-09 23:31 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 10361: Add option to cleanup_database.pl to purge OPAC search history (5.29 KB, patch)
2013-08-30 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10361: (follow-up) add tests for search history functions (3.55 KB, patch)
2013-08-30 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mirko Tietgen 2013-05-28 12:33:31 UTC
Add an option to cleanup_database.pl to purge the search_history entries older than X days.
Comment 1 Mirko Tietgen 2013-05-28 12:37:40 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-06-03 04:26:29 UTC
"DELETE FROM search_history" query should go to C4::Search.pm, preferrably with a test (but not a show stopper)
Comment 3 Mirko Tietgen 2013-06-04 13:49:34 UTC
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.
Comment 4 Galen Charlton 2013-06-04 14:18:44 UTC
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
Comment 5 Mirko Tietgen 2013-06-11 13:27:47 UTC Comment hidden (obsolete)
Comment 6 Mirko Tietgen 2013-06-11 13:36:29 UTC Comment hidden (obsolete)
Comment 7 Mirko Tietgen 2013-06-11 13:42:31 UTC
(also removes a duplicate export of &SimpleSearch from Search.pm)
Comment 8 Galen Charlton 2013-08-02 16:25:36 UTC Comment hidden (obsolete)
Comment 9 Galen Charlton 2013-08-02 16:26:47 UTC Comment hidden (obsolete)
Comment 10 Galen Charlton 2013-08-02 16:27:16 UTC Comment hidden (obsolete)
Comment 11 Galen Charlton 2013-08-02 16:27:59 UTC
I've signed off on the patch and added a follow-up that adds some tests.
Comment 12 Mirko Tietgen 2013-08-09 23:31:23 UTC Comment hidden (obsolete)
Comment 13 Mirko Tietgen 2013-08-09 23:32:47 UTC
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!
Comment 14 Kyle M Hall 2013-08-30 13:31:07 UTC
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>
Comment 15 Kyle M Hall 2013-08-30 13:31:21 UTC
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>
Comment 16 Galen Charlton 2013-09-04 14:24:43 UTC
Pushed to master.  Thanks, Mirko!