Attachment 22058 Details for Bug 10933 – Bug 10933: The PurgeSearchHistory should be merge into the C4::Search::History module

[patch] Bug 10933: The PurgeSearchHistory should be merge into the C4::Search::History module

Bug-10933-The-PurgeSearchHistory-should-be-merge-i.patch (text/plain), 11.45 KB, created by Jonathan Druart on 2013-10-18 14:40:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2013-10-18 14:40:30 UTC

Size: 11.45 KB

patch

obsolete

>From 01c5a47275adc325b53037fed3c51dd6389f6dca Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@biblibre.com>
>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 <http://www.gnu.org/licenses>.
>-
>-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

Actions: View | Diff | Splinter Review

Attachments on bug 10933: 21364 | 22058 | 27452 | 28022 | 42114 | 42116 | 43786 | 43787 | 44095