From 01c5a47275adc325b53037fed3c51dd6389f6dca Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 23 Sep 2013 13:34:19 +0200 Subject: [PATCH] Bug 10933: The PurgeSearchHistory should be merge into the C4::Search::History module Since bug 10803 adds a C4::Search::History module, the PurgeSearchHistory routine should be moved. Test plan: - run misc/cronjobs/cleanup_database.pl with the searchhistory param and verify behavior is the same as before applying this patch. - run prove t/Search/History.t --- C4/Search.pm | 8 ---- C4/Search/History.pm | 13 ++++-- misc/cronjobs/cleanup_database.pl | 2 +- t/db_dependent/Search/History.t | 61 +++++++++++++++++++++++++++- t/db_dependent/Search_SearchHistory.t | 72 --------------------------------- 5 files changed, 71 insertions(+), 85 deletions(-) delete mode 100644 t/db_dependent/Search_SearchHistory.t diff --git a/C4/Search.pm b/C4/Search.pm index b45fa76..8134ae8 100644 --- a/C4/Search.pm +++ b/C4/Search.pm @@ -70,7 +70,6 @@ This module provides searching functions for Koha's bibliographic databases &buildQuery &GetDistinctValues &enabled_staff_search_views - &PurgeSearchHistory ); # make all your functions, whether exported or not; @@ -2189,13 +2188,6 @@ sub enabled_staff_search_views ); } -sub PurgeSearchHistory{ - my ($pSearchhistory)=@_; - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare("DELETE FROM search_history WHERE time < DATE_SUB( NOW(), INTERVAL ? DAY )"); - $sth->execute($pSearchhistory) or die $dbh->errstr; -} - =head2 z3950_search_args $arrayref = z3950_search_args($matchpoints) diff --git a/C4/Search/History.pm b/C4/Search/History.pm index b120ab5..1751453 100644 --- a/C4/Search/History.pm +++ b/C4/Search/History.pm @@ -17,6 +17,7 @@ sub add { my $query_cgi = $params->{query_cgi}; my $total = $params->{total} // 0; my $type = $params->{type} || 'biblio'; + my $time = $params->{time}; my $dbh = C4::Context->dbh; @@ -25,11 +26,11 @@ sub add { INSERT INTO search_history( userid, sessionid, query_desc, query_cgi, type, total, time ) VALUES( - ?, ?, ?, ?, ?, ?, NOW() + ?, ?, ?, ?, ?, ?, ? ) }; my $sth = $dbh->prepare($query); - $sth->execute($userid, $sessionid, $query_desc, $query_cgi, $type, $total); + $sth->execute($userid, $sessionid, $query_desc, $query_cgi, $type, $total, $time); } sub build_new_cookie_value { @@ -69,6 +70,7 @@ sub delete { my $sessionid = $params->{sessionid}; my $type = $params->{type} || q{}; my $previous = $params->{previous} || 0; + my $interval = $params->{interval} || 0; unless ( $userid ) { warn "ERROR: userid is required for history search"; @@ -90,11 +92,16 @@ sub delete { $query .= q{ AND type = ?} if $type; + # FIXME DATE_SUB is a Mysql-ism. Postgres uses: datefield - INTERVAL '6 months' + $query .= q{ AND time < DATE_SUB( NOW(), INTERVAL ? DAY )} + if $interval; + $dbh->do( $query, {}, $userid, ( $sessionid ? $sessionid : () ), - ( $type ? $type : () ) + ( $type ? $type : () ), + ( $interval ? $interval : () ) ); } diff --git a/misc/cronjobs/cleanup_database.pl b/misc/cronjobs/cleanup_database.pl index c190a80..3838237 100755 --- a/misc/cronjobs/cleanup_database.pl +++ b/misc/cronjobs/cleanup_database.pl @@ -181,7 +181,7 @@ if($pLogs) { if($pSearchhistory) { print "Purging records older than $pSearchhistory from search_history.\n" if $verbose; - PurgeSearchHistory($pSearchhistory); + C4::Search::History::delete({ interval => $pSearchhistory }); print "Done with purging search_history.\n" if $verbose; } diff --git a/t/db_dependent/Search/History.t b/t/db_dependent/Search/History.t index 33e834d..b6583f5 100644 --- a/t/db_dependent/Search/History.t +++ b/t/db_dependent/Search/History.t @@ -2,7 +2,7 @@ use Modern::Perl; -use Test::More tests => 16; +use Test::More tests => 22; use URI::Escape; use C4::Context; @@ -127,9 +127,59 @@ $all = C4::Search::History::get({userid => $userid}); is( scalar(@$all), 9, 'There are still 9 searches after calling delete without userid' ); delete_all( $userid ); +# Test delete with interval +add( $userid, $current_sessionid, $previous_sessionid, $total, $query_cgi_b, $query_cgi_a ); +C4::Search::History::delete({ + userid => $userid, + interval => 10, +}); +$all = C4::Search::History::get({userid => $userid}); +is( scalar(@$all), 9, 'There are still 9 searches after calling delete with an interval = 10 days' ); +C4::Search::History::delete({ + userid => $userid, + interval => 6, +}); +$all = C4::Search::History::get({userid => $userid}); +is( scalar(@$all), 8, 'There are still 8 searches after calling delete with an interval = 6 days' ); +C4::Search::History::delete({ + userid => $userid, + interval => 2, +}); +$all = C4::Search::History::get({userid => $userid}); +is( scalar(@$all), 2, 'There are still 2 searches after calling delete with an interval = 2 days' ); +delete_all( $userid ); + +add( $userid, $current_sessionid, $previous_sessionid, $total, $query_cgi_b, $query_cgi_a ); +C4::Search::History::delete({ + userid => $userid, + interval => 5, + type => 'biblio', +}); +$all = C4::Search::History::get({userid => $userid}); +is( scalar(@$all), 8, 'There are still 9 searches after calling delete with an interval = 5 days for biblio' ); +C4::Search::History::delete({ + userid => $userid, + interval => 5, + type => 'authority', +}); +$all = C4::Search::History::get({userid => $userid}); +is( scalar(@$all), 6, 'There are still 6 searches after calling delete with an interval = 5 days for authority' ); +C4::Search::History::delete({ + userid => $userid, + interval => -1, +}); +$all = C4::Search::History::get({userid => $userid}); +is( scalar(@$all), 0, 'There is no search after calling delete with an interval = -1 days' ); +delete_all( $userid ); + sub add { my ( $userid, $current_session_id, $previous_sessionid, $total, $query_cgi_b, $query_cgi_a ) = @_; + my $days_ago_2 = dt_from_string()->add_duration( DateTime::Duration->new( days => -2 ) ); + my $days_ago_4 = dt_from_string()->add_duration( DateTime::Duration->new( days => -4 ) ); + my $days_ago_6 = dt_from_string()->add_duration( DateTime::Duration->new( days => -6 ) ); + my $days_ago_8 = dt_from_string()->add_duration( DateTime::Duration->new( days => -8 ) ); + my $query_desc_b1_p = q{first previous biblio search}; my $first_previous_biblio_search = { userid => $userid, @@ -138,6 +188,7 @@ sub add { query_cgi => $query_cgi_b, total => $total, type => 'biblio', + time => $days_ago_2, }; my $query_desc_a1_p = q{first previous authority search}; @@ -148,6 +199,7 @@ sub add { query_cgi => $query_cgi_a, total => $total, type => 'authority', + time => $days_ago_2, }; my $query_desc_b2_p = q{second previous biblio search}; @@ -158,6 +210,7 @@ sub add { query_cgi => $query_cgi_b, total => $total, type => 'biblio', + time => $days_ago_4, }; my $query_desc_a2_p = q{second previous authority search}; @@ -168,6 +221,7 @@ sub add { query_cgi => $query_cgi_a, total => $total, type => 'authority', + time => $days_ago_4, }; @@ -180,6 +234,7 @@ sub add { query_cgi => $query_cgi_b, total => $total, type => 'biblio', + time => $days_ago_4, }; my $query_desc_a1_c = q{first current authority search}; @@ -190,6 +245,7 @@ sub add { query_cgi => $query_cgi_a, total => $total, type => 'authority', + time => $days_ago_4, }; my $query_desc_b2_c = q{second current biblio search}; @@ -200,6 +256,7 @@ sub add { query_cgi => $query_cgi_b, total => $total, type => 'biblio', + time => $days_ago_6, }; my $query_desc_a2_c = q{second current authority search}; @@ -210,6 +267,7 @@ sub add { query_cgi => $query_cgi_a, total => $total, type => 'authority', + time => $days_ago_6, }; my $query_desc_a3_c = q{third current authority search}; @@ -220,6 +278,7 @@ sub add { query_cgi => $query_cgi_a, total => $total, type => 'authority', + time => $days_ago_8, }; diff --git a/t/db_dependent/Search_SearchHistory.t b/t/db_dependent/Search_SearchHistory.t deleted file mode 100644 index 5f21e3e..0000000 --- a/t/db_dependent/Search_SearchHistory.t +++ /dev/null @@ -1,72 +0,0 @@ -#!/usr/bin/perl - -# This file is part of Koha. -# -# Copyright 2013 Equinox Software, Inc. -# -# Koha is free software; you can redistribute it and/or modify it under the -# terms of the GNU General Public License as published by the Free Software -# Foundation; either version 3 of the License, or (at your option) any later -# version. -# -# Koha is distributed in the hope that it will be useful, but WITHOUT ANY -# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR -# A PARTICULAR PURPOSE. See the GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License along -# with Koha; if not, see . - -use Modern::Perl; - -use Test::More tests => 6; - -use C4::Context; -use_ok('C4::Search', qw/PurgeSearchHistory/); # GetSearchHistory is not exported -use C4::Search::History; - -# Start transaction -my $dbh = C4::Context->dbh; -$dbh->{AutoCommit} = 0; -$dbh->{RaiseError} = 1; - -# start with a clean slate -$dbh->do('DELETE FROM search_history'); -is(_get_history_count(), 0, 'starting off with nothing in search_history'); - -# Add some history rows. Note that we're ignoring the return value; -# since the search_history table doesn't have an auto_increment -# column, it appears that the value of $dbh->last_insert_id() is -# useless for determining if the insert failed. -C4::Search::History::add({userid => 12345, sessionid => 'session_1', query_desc => 'query_desc_1', query_cgi => 'query_cgi_1', total => 5}); -C4::Search::History::add({userid => 12345, sessionid => 'session_1', query_desc => 'query_desc_2', query_cgi => 'query_cgi_3', total => 6}); -C4::Search::History::add({userid => 12345, sessionid => 'session_1', query_desc => 'query_desc_3', query_cgi => 'query_cgi_3', total => 7}); -C4::Search::History::add({userid => 56789, sessionid => 'session_2', query_desc => 'query_desc_4', query_cgi => 'query_cgi_4', total => 8}); -is(_get_history_count(), 4, 'successfully added four search_history rows'); - -# We're not testing GetSearchHistory at present because it is broken... -# see bug 10677 - -# munge some dates -my $sth = $dbh->prepare(' - UPDATE search_history - SET time = DATE_SUB(time, INTERVAL ? DAY) - WHERE query_desc = ? -'); -$sth->execute(46, 'query_desc_1'); -$sth->execute(31, 'query_desc_2'); - -PurgeSearchHistory(45); -is(_get_history_count(), 3, 'purged history older than 45 days'); -PurgeSearchHistory(30); -is(_get_history_count(), 2, 'purged history older than 30 days'); -PurgeSearchHistory(-1); -is(_get_history_count(), 0, 'purged all history'); - -sub _get_history_count { - my $count_sth = $dbh->prepare('SELECT COUNT(*) FROM search_history'); - $count_sth->execute(); - my $count = $count_sth->fetchall_arrayref(); - return $count->[0]->[0]; -} - -$dbh->rollback; -- 1.7.10.4