Attachment 42114 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.64 KB, created by Joonas Kylmälä on 2015-08-31 11:45:22 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Joonas Kylmälä

Created: 2015-08-31 11:45:22 UTC

Size: 11.64 KB

patch

obsolete

>From ce12c9cc75f75b210a8d90e331969f7e4ff85944 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
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>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Ã¤ <j.kylmala@gmail.com>
>---
> 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 e0f34bf..277ba6c 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;
>@@ -2428,13 +2427,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 c80310e..acb68e7 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 0766141..41a41ae 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 80c2b99..6c067b3 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 e328502..0000000
>--- 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 <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');
>-
>-# 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.9.1

Actions: View | Diff | Splinter Review

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