Bug 23632: Remove C4::Logs::GetLogs

Koha::ActionLogs->search must be used instead.
There is no call to this subroutine in our code, it should be removed.

Test plan:
Make sure the 3 test files still return green and that there is no more
occurrences of GetLogs in the codebase.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

diff --git a/C4/Log.pm b/C4/Log.pm
index bad023d..dc3c63f 100644 (file)
--- a/C4/Log.pm
+++ b/C4/Log.pm
@@ -35,7 +35,7 @@ use vars qw(@ISA @EXPORT);
 BEGIN {
         require Exporter;
         @ISA = qw(Exporter);
-        @EXPORT = qw(&logaction &cronlogaction &GetLogs);
+        @EXPORT = qw(&logaction &cronlogaction);
 }
 
 =head1 NAME
@@ -120,81 +120,6 @@ sub cronlogaction {
     logaction( 'CRONJOBS', 'Run', undef, $loginfo ) if C4::Context->preference('CronjobLog');
 }
 
-=item GetLogs
-
-$logs = GetLogs($datefrom,$dateto,$user,\@modules,$action,$object,$info,$interfaces);
-
-Return:
-C<$logs> is a ref to a hash which contains all columns from action_logs
-
-=cut
-
-sub GetLogs {
-    my $datefrom = shift;
-    my $dateto   = shift;
-    my $user     = shift;
-    my $modules  = shift;
-    my $action   = shift;
-    my $object   = shift;
-    my $info     = shift;
-    my $interfaces = shift;
-
-    my $iso_datefrom = $datefrom ? output_pref({ dt => dt_from_string( $datefrom ), dateformat => 'iso', dateonly => 1 }) : undef;
-    my $iso_dateto = $dateto ? output_pref({ dt => dt_from_string( $dateto ), dateformat => 'iso', dateonly => 1 }) : undef;
-
-    $user ||= q{};
-
-    my $dbh   = C4::Context->dbh;
-    my $query = "
-        SELECT *
-        FROM   action_logs
-        WHERE 1
-    ";
-
-    my @parameters;
-    $query .=
-      " AND DATE_FORMAT(timestamp, '%Y-%m-%d') >= \"" . $iso_datefrom . "\" "
-      if $iso_datefrom;    #fix me - mysql specific
-    $query .=
-      " AND DATE_FORMAT(timestamp, '%Y-%m-%d') <= \"" . $iso_dateto . "\" "
-      if $iso_dateto;
-    if ( $user ne q{} ) {
-        $query .= " AND user = ? ";
-        push( @parameters, $user );
-    }
-    if ( $modules && scalar(@$modules) ) {
-        $query .=
-          " AND module IN (" . join( ",", map { "?" } @$modules ) . ") ";
-        push( @parameters, @$modules );
-    }
-    if ( $action && scalar(@$action) ) {
-        $query .= " AND action IN (" . join( ",", map { "?" } @$action ) . ") ";
-        push( @parameters, @$action );
-    }
-    if ($object) {
-        $query .= " AND object = ? ";
-        push( @parameters, $object );
-    }
-    if ($info) {
-        $query .= " AND info LIKE ? ";
-        push( @parameters, "%" . $info . "%" );
-    }
-    if ( $interfaces && scalar(@$interfaces) ) {
-        $query .=
-          " AND interface IN (" . join( ",", map { "?" } @$interfaces ) . ") ";
-        push( @parameters, @$interfaces );
-    }
-
-    my $sth = $dbh->prepare($query);
-    $sth->execute(@parameters);
-
-    my @logs;
-    while ( my $row = $sth->fetchrow_hashref ) {
-        push @logs, $row;
-    }
-    return \@logs;
-}
-
 1;
 __END__
 

diff --git a/t/db_dependent/Illrequest/Logger.t b/t/db_dependent/Illrequest/Logger.t
index e3b5670..07d4097 100644 (file)
--- a/t/db_dependent/Illrequest/Logger.t
+++ b/t/db_dependent/Illrequest/Logger.t
@@ -27,28 +27,9 @@ use t::lib::Mocks;
 
 my $schema = Koha::Database->new->schema;
 
-# A mock response from C4::Log::GetLogs()
-my $logs = [
-    {
-        info      => '{"log_origin": "core"}',
-        action    => 'STATUS_CHANGE',
-        timestamp => '2018-10-02 11:12:22'
-    },
-    {
-        info      => '{"log_origin": "core"}',
-        action    => 'STATUS_CHANGE',
-        timestamp => '2018-10-02 11:12:12'
-    },
-    {
-        info      => '{"log_origin": "core"}',
-        action    => 'STATUS_CHANGE',
-        timestamp => '2018-10-02 11:12:32'
-    }
-];
 # Mock the modules we use
 my $c4_log = Test::MockModule->new('C4::Log');
 $c4_log->mock('logaction', sub { 1 });
-$c4_log->mock('GetLogs', sub { return $logs; });
 my $c4_tpl = Test::MockModule->new('C4::Templates');
 $c4_tpl->mock('_get_template_file',
     sub { return ('htdocs', 'theme', 'lang', 'base/'); });

diff --git a/t/db_dependent/Koha/Patron/Messages.t b/t/db_dependent/Koha/Patron/Messages.t
index 1260185..e1da753 100644 (file)
--- a/t/db_dependent/Koha/Patron/Messages.t
+++ b/t/db_dependent/Koha/Patron/Messages.t
@@ -22,7 +22,7 @@ use Modern::Perl;
 use Test::More tests => 12;
 
 use C4::Context;
-use C4::Log;
+use Koha::ActionLogs;
 use Koha::Patron::Message;
 use Koha::Patron::Messages;
 use Koha::Patrons;
@@ -102,6 +102,6 @@ is( get_nb_of_logactions(), $nb_of_logaction + 3, 'With BorrowersLog on, 1 new l
 $schema->storage->txn_rollback;
 
 sub get_nb_of_logactions {
-    return scalar( @{ C4::Log::GetLogs( undef, undef, undef, ['MEMBERS'] ) } );
+    return Koha::ActionLogs->search({ module => 'MEMBERS' })->count;
 }
 

diff --git a/t/db_dependent/Log.t b/t/db_dependent/Log.t
index 8376b61..3ce974b 100644 (file)
--- a/t/db_dependent/Log.t
+++ b/t/db_dependent/Log.t
@@ -15,13 +15,14 @@
 # with Koha; if not, see <http://www.gnu.org/licenses>.
 
 use Modern::Perl;
-use Test::More tests => 5;
+use Test::More tests => 3;
 
 use C4::Context;
 use C4::Log;
 use C4::Auth qw/checkpw/;
 use Koha::Database;
 use Koha::DateUtils;
+use Koha::ActionLogs;
 
 use t::lib::Mocks qw/mock_preference/; # to mock CronjobLog
 use t::lib::TestBuilder;
@@ -32,7 +33,7 @@ $schema->storage->txn_begin;
 our $dbh = C4::Context->dbh;
 
 subtest 'Existing tests' => sub {
-    plan tests => 6;
+    plan tests => 3;
 
     my $success;
     eval {
@@ -45,33 +46,6 @@ subtest 'Existing tests' => sub {
     };
     ok($success, "logaction seemed to work");
 
-    eval {
-        # FIXME: US formatted date hardcoded into test for now
-        $success = scalar(@{GetLogs("","","",undef,undef,"","")});
-    } or do {
-        diag($@);
-        $success = 0;
-    };
-    ok($success, "GetLogs returns results for an open search");
-
-    eval {
-        # FIXME: US formatted date hardcoded into test for now
-        my $date = output_pref( { dt => dt_from_string, dateonly => 1, dateformat => 'iso' } );
-        $success = scalar(@{GetLogs( $date, $date, "", undef, undef, "", "") } );
-    } or do {
-        diag($@);
-        $success = 0;
-    };
-    ok($success, "GetLogs accepts dates in an All-matching search");
-
-    eval {
-        $success = scalar(@{GetLogs("","","",["MEMBERS"],["MODIFY"],1,"")});
-    } or do {
-        diag($@);
-        $success = 0;
-    };
-    ok($success, "GetLogs seemed to find ".$success." like our test record in a tighter search");
-
     # We want numbers to be the same between runs.
     $dbh->do("DELETE FROM action_logs;");
 
@@ -86,22 +60,6 @@ subtest 'Existing tests' => sub {
     is($cronJobCount,1,"Cronjob logged as expected.");
 };
 
-subtest "GetLogs should return all logs if dates are not set" => sub {
-    plan tests => 2;
-    my $today = dt_from_string->add(minutes => -1);
-    my $yesterday = dt_from_string->add( days => -1 );
-    $dbh->do(q|
-        INSERT INTO action_logs (timestamp, user, module, action, object, info)
-        VALUES
-        (?, 42, 'CATALOGUING', 'MODIFY', 4242, 'Record 42 has been modified by patron 4242 yesterday'),
-        (?, 43, 'CATALOGUING', 'MODIFY', 4242, 'Record 43 has been modified by patron 4242 today')
-    |, undef, output_pref({dt =>$yesterday, dateformat => 'iso'}), output_pref({dt => $today, dateformat => 'iso'}));
-    my $logs = GetLogs( undef, undef, undef, ['CATALOGUING'], ['MODIFY'], 4242 );
-    is( scalar(@$logs), 2, 'GetLogs should return all logs regardless the dates' );
-    $logs = GetLogs( output_pref($today), undef, undef, ['CATALOGUING'], ['MODIFY'], 4242 );
-    is( scalar(@$logs), 1, 'GetLogs should return the logs for today' );
-};
-
 subtest 'logaction(): interface is correctly logged' => sub {
 
     plan tests => 4;
@@ -110,56 +68,29 @@ subtest 'logaction(): interface is correctly logged' => sub {
     $dbh->do("DELETE FROM action_logs;");
     C4::Context->interface( 'commandline' );
     logaction( "MEMBERS", "MODIFY", 1, "test operation");
-    my $logs = GetLogs();
-    is( @{$logs}[0]->{ interface }, 'commandline', 'Interface correctly deduced (commandline)');
+    my $log = Koha::ActionLogs->search->next;
+    is( $log->interface, 'commandline', 'Interface correctly deduced (commandline)');
 
     # No interface passed, using C4::Context->interface
     $dbh->do("DELETE FROM action_logs;");
     C4::Context->interface( 'opac' );
     logaction( "MEMBERS", "MODIFY", 1, "test operation");
-    $logs = GetLogs();
-    is( @{$logs}[0]->{ interface }, 'opac', 'Interface correctly deduced (opac)');
+    $log = Koha::ActionLogs->search->next;
+    is( $log->interface, 'opac', 'Interface correctly deduced (opac)');
 
     # Explicit interfaces
     $dbh->do("DELETE FROM action_logs;");
     C4::Context->interface( 'intranet' );
     logaction( "MEMBERS", "MODIFY", 1, 'test info', 'intranet');
-    $logs = GetLogs();
-    is( @{$logs}[0]->{ interface }, 'intranet', 'Passed interface is respected (intranet)');
+    $log = Koha::ActionLogs->search->next;
+    is( $log->interface, 'intranet', 'Passed interface is respected (intranet)');
 
     # Explicit interfaces
     $dbh->do("DELETE FROM action_logs;");
     C4::Context->interface( 'sip' );
     logaction( "MEMBERS", "MODIFY", 1, 'test info', 'sip');
-    $logs = GetLogs();
-    is( @{$logs}[0]->{ interface }, 'sip', 'Passed interface is respected (sip)');
-};
-
-subtest 'GetLogs() respects interface filters' => sub {
-
-    plan tests => 5;
-
-    $dbh->do("DELETE FROM action_logs;");
-
-    logaction( 'MEMBERS', 'MODIFY', 1, 'opac info',        'opac');
-    logaction( 'MEMBERS', 'MODIFY', 1, 'sip info',         'sip');
-    logaction( 'MEMBERS', 'MODIFY', 1, 'intranet info',    'intranet');
-    logaction( 'MEMBERS', 'MODIFY', 1, 'commandline info', 'commandline');
-
-    my $logs = scalar @{ GetLogs() };
-    is( $logs, 4, 'If no filter on interfaces is passed, all logs are returned');
-
-    $logs = GetLogs(undef,undef,undef,undef,undef,undef,undef,['opac']);
-    is( @{$logs}[0]->{ interface }, 'opac', 'Interface correctly filtered (opac)');
-
-    $logs = GetLogs(undef,undef,undef,undef,undef,undef,undef,['sip']);
-    is( @{$logs}[0]->{ interface }, 'sip', 'Interface correctly filtered (sip)');
-
-    $logs = GetLogs(undef,undef,undef,undef,undef,undef,undef,['intranet']);
-    is( @{$logs}[0]->{ interface }, 'intranet', 'Interface correctly filtered (intranet)');
-
-    $logs = GetLogs(undef,undef,undef,undef,undef,undef,undef,['commandline']);
-    is( @{$logs}[0]->{ interface }, 'commandline', 'Interface correctly filtered (commandline)');
+    $log = Koha::ActionLogs->search->next;
+    is( $log->interface, 'sip', 'Passed interface is respected (sip)');
 };
 
 subtest 'GDPR logging' => sub {
@@ -170,8 +101,15 @@ subtest 'GDPR logging' => sub {
 
     t::lib::Mocks::mock_userenv({ patron => $patron });
     logaction( 'AUTH', 'FAILURE', $patron->id, '', 'opac' );
-    my $logs = GetLogs( undef, undef, $patron->id, ['AUTH'], ['FAILURE'], $patron->id );
-    is( @$logs, 1, 'We should find one auth failure' );
+    my $logs = Koha::ActionLogs->search(
+        {
+            user   => $patron->id,
+            module => 'AUTH',
+            action => 'FAILURE',
+            object => $patron->id,
+        }
+    );
+    is( $logs->count, 1, 'We should find one auth failure' );
 
     t::lib::Mocks::mock_preference('AuthFailureLog', 1);
     my $strong_password = 'N0tStr0ngAnyM0reN0w:)';
@@ -179,18 +117,38 @@ subtest 'GDPR logging' => sub {
     my @ret = checkpw( $dbh, $patron->userid, 'WrongPassword', undef, undef, 1);
     is( $ret[0], 0, 'Authentication failed' );
     # Look for auth failure but NOT on patron id, pass userid in info parameter
-    $logs = GetLogs( undef, undef, 0, ['AUTH'], ['FAILURE'], undef, $patron->userid );
-    is( @$logs, 1, 'We should find one auth failure with this userid' );
+    $logs = Koha::ActionLogs->search(
+        {
+            module => 'AUTH',
+            action => 'FAILURE',
+            info   => { -like => '%'.$patron->userid.'%' },
+        }
+    );
+    is( $logs->count, 1, 'We should find one auth failure with this userid' );
     t::lib::Mocks::mock_preference('AuthFailureLog', 0);
     @ret = checkpw( $dbh, $patron->userid, 'WrongPassword', undef, undef, 1);
-    $logs = GetLogs( undef, undef, 0, ['AUTH'], ['FAILURE'], undef, $patron->userid );
-    is( @$logs, 1, 'Still only one failure with this userid' );
+    $logs = Koha::ActionLogs->search(
+        {
+            module => 'AUTH',
+            action => 'FAILURE',
+            info   => { -like => '%'.$patron->userid.'%' },
+        }
+    );
+    is( $logs->count, 1, 'Still only one failure with this userid' );
     t::lib::Mocks::mock_preference('AuthSuccessLog', 1);
     @ret = checkpw( $dbh, $patron->userid, $strong_password, undef, undef, 1);
     is( $ret[0], 1, 'Authentication succeeded' );
     # Now we can look for patron id
-    $logs = GetLogs( undef, undef, $patron->id, ['AUTH'], ['SUCCESS'], $patron->id );
-    is( @$logs, 1, 'We expect only one auth success line for this patron' );
+    $logs = Koha::ActionLogs->search(
+        {
+            user   => $patron->id,
+            module => 'AUTH',
+            action => 'SUCCESS',
+            object => $patron->id,
+        }
+    );
+
+    is( $logs->count, 1, 'We expect only one auth success line for this patron' );
 };
 
 $schema->storage->txn_rollback;