From c4d3002995c147244fffdff24f9ffbea484cfc48 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 MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Joonas Kylmälä Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi --- C4/Search.pm | 8 ---- C4/Search/History.pm | 13 +++-- misc/cronjobs/cleanup_database.pl | 2 +- t/db_dependent/Search/History.t | 63 +++++++++++++++++++++++- t/db_dependent/Search_SearchHistory.t | 69 --------------------------- 5 files changed, 73 insertions(+), 82 deletions(-) delete mode 100644 t/db_dependent/Search_SearchHistory.t diff --git a/C4/Search.pm b/C4/Search.pm index 1a8bba03ad..c9d2529dd3 100644 --- a/C4/Search.pm +++ b/C4/Search.pm @@ -69,7 +69,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; @@ -2427,13 +2426,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 c80310ec65..acb68e7d44 100644 --- a/C4/Search/History.pm +++ b/C4/Search/History.pm @@ -18,6 +18,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; @@ -26,12 +27,12 @@ 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 ); + $total, $time ); } sub add_to_session { @@ -66,6 +67,7 @@ sub delete { my $sessionid = $params->{sessionid}; my $type = $params->{type} || q{}; my $previous = $params->{previous} || 0; + my $interval = $params->{interval} || 0; unless ( ref( $id ) ) { $id = $id ? [ $id ] : []; @@ -99,12 +101,17 @@ 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, {}, ( @$id ? ( @$id ) : () ), ( $userid ? $userid : () ), ( $sessionid ? $sessionid : () ), - ( $type ? $type : () ) + ( $type ? $type : () ), + ( $interval ? $interval : () ), ); } diff --git a/misc/cronjobs/cleanup_database.pl b/misc/cronjobs/cleanup_database.pl index 0766141026..41a41ae724 100755 --- a/misc/cronjobs/cleanup_database.pl +++ b/misc/cronjobs/cleanup_database.pl @@ -243,7 +243,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 80c2b9909d..6c067b3fee 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 => 19; +use Test::More tests => 25; use Test::Warn; use URI::Escape; use List::Util qw( shuffle ); @@ -150,11 +150,63 @@ $ids = [ shuffle map { $_->{id} } @$all ]; C4::Search::History::delete({ id => [ @$ids[0..4] ] }); $all = C4::Search::History::get({ userid => $userid }); is( scalar(@$all), 4, 'There are 4 searches after calling delete with 5 ids' ); + +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, @@ -163,6 +215,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}; @@ -173,6 +226,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}; @@ -183,6 +237,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}; @@ -193,6 +248,7 @@ sub add { query_cgi => $query_cgi_a, total => $total, type => 'authority', + time => $days_ago_4, }; @@ -205,6 +261,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}; @@ -215,6 +272,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}; @@ -225,6 +283,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}; @@ -235,6 +294,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}; @@ -245,6 +305,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 e328502e52..0000000000 --- a/t/db_dependent/Search_SearchHistory.t +++ /dev/null @@ -1,69 +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'); - -# 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; -- 2.39.5