From 3151b35171a9d31b9a8a5f290df9f8e2a04bceb7 Mon Sep 17 00:00:00 2001 From: Jonathan Druart 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. --- 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 . 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.20.1