Attachment 107914 Details for Bug 23632 – Bug 23632: Remove C4::Logs::GetLogs

[patch] Bug 23632: Remove C4::Logs::GetLogs

Bug-23632-Remove-C4LogsGetLogs.patch (text/plain), 13.75 KB, created by Josef Moravec on 2020-08-06 19:07:32 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Josef Moravec

Created: 2020-08-06 19:07:32 UTC

Size: 13.75 KB

patch

obsolete

>From c5e71e177172656ec1c693c96b09a7a4bb4ded05 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Thu, 6 Aug 2020 11:28:53 +0200
>Subject: [PATCH] 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>
>---
> C4/Log.pm                             |  77 +------------------
> t/db_dependent/Illrequest/Logger.t    |  19 -----
> t/db_dependent/Koha/Patron/Messages.t |   4 +-
> t/db_dependent/Log.t                  | 134 ++++++++++++----------------------
> 4 files changed, 49 insertions(+), 185 deletions(-)
>
>diff --git a/C4/Log.pm b/C4/Log.pm
>index bad023d15d..dc3c63fa80 100644
>--- 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 e3b5670221..07d4097573 100644
>--- 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 1260185ebf..e1da75324f 100644
>--- 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 8376b61969..3ce974bc69 100644
>--- 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;
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 23632: 107871 | 107914 | 108189