From 404c88fe87b27e4b4578e0bb6df2252a0bad9ed1 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 23 Nov 2022 09:53:31 +0100 Subject: [PATCH] Bug 31969: Use filter_by_last_update This script has a pattern to delete rows depending on a given date/number of days, we should use the filter_by_last_update Koha::Objects method. No need for another method and tests, everything is already tested there. This patch also suggests to rename the reference to "background" and "bg" with "jobs", which seems more appropriate and not an abbreviation Signed-off-by: Tomas Cohen Arazi --- Koha/BackgroundJobs.pm | 45 ------------------ misc/cronjobs/cleanup_database.pl | 51 +++++++++++++-------- t/db_dependent/Koha/BackgroundJobs.t | 68 +--------------------------- 3 files changed, 32 insertions(+), 132 deletions(-) diff --git a/Koha/BackgroundJobs.pm b/Koha/BackgroundJobs.pm index 8d7ae64e0e..48afbf68fa 100644 --- a/Koha/BackgroundJobs.pm +++ b/Koha/BackgroundJobs.pm @@ -72,51 +72,6 @@ sub filter_by_current { ); } -=head3 purge - - my $params = { job_types => ('all') , # Arrayref of jobtypes to be purged - days => 1, # Age in days of jobs to be purged - confirm => 1, # Confirm deletion - }; - my $count = Koha::BackgroundJobs->purge( $params ); - -Deletes finished background jobs. Returns the number of jobs that was / would've been deleted. - -=cut - -sub purge { - my ( $self, $params) = @_; - - return 0 unless ( exists $params->{job_types} && scalar @{ $params->{job_types} } >= 1 ); - return 0 unless ( exists $params->{days} ); - - my $types = $params->{job_types}; - my $days = $params->{days}; - - my $confirm = exists $params->{confirm} ? $params->{confirm} : 0; - - my $rs; - if ( $types->[0] eq 'all' ){ - $rs = $self->search( - { - ended_on => { '<' => \[ 'date_sub(curdate(), INTERVAL ? DAY)', $days ] }, - status => 'finished', - }); - - } else { - $rs = $self->search( - { - ended_on => { '<' => \[ 'date_sub(curdate(), INTERVAL ? DAY)', $days ] }, - type => $types, - status => 'finished', - }); - } - my $count = $rs->count(); - $rs->delete if $confirm; - - return $count; -} - =head2 Internal methods =head3 _type diff --git a/misc/cronjobs/cleanup_database.pl b/misc/cronjobs/cleanup_database.pl index 96783c159f..0c32bc02ca 100755 --- a/misc/cronjobs/cleanup_database.pl +++ b/misc/cronjobs/cleanup_database.pl @@ -27,8 +27,8 @@ use constant DEFAULT_MESSAGES_PURGEDAYS => 365; use constant DEFAULT_SEARCHHISTORY_PURGEDAYS => 30; use constant DEFAULT_SHARE_INVITATION_EXPIRY_DAYS => 14; use constant DEFAULT_DEBARMENTS_PURGEDAYS => 30; -use constant DEFAULT_BGJOBS_PURGEDAYS => 1; -use constant DEFAULT_BGJOBS_PURGETYPES => qw{ update_elastic_index }; +use constant DEFAULT_JOBS_PURGEDAYS => 1; +use constant DEFAULT_JOBS_PURGETYPES => qw{ update_elastic_index }; use Koha::Script -cron; use C4::Context; @@ -110,8 +110,8 @@ Usage: $0 [-h|--help] [--confirm] [--sessions] [--sessdays DAYS] [-v|--verbose] --cards DAY Purge card creator batches last added to more than DAYS days ago. --return-claims Purge all resolved return claims older than the number of days specified in the system preference CleanUpDatabaseReturnClaims. - --bg-days DAYS Purge all finished background jobs this many days old. Defaults to 1 if no DAYS provided. - --bg-type TYPES What type of background job to purge. Defaults to "update_elastic_index" if omitted + --jobs-days DAYS Purge all finished background jobs this many days old. Defaults to 1 if no DAYS provided. + --jobs-type TYPES What type of background job to purge. Defaults to "update_elastic_index" if omitted Specifying "all" will purge all types. Repeatable. USAGE exit $_[0]; @@ -154,8 +154,8 @@ my $labels; my $cards; my @log_modules; my @preserve_logs; -my $background_days; -my @background_types; +my $jobs_days; +my @jobs_types; my $command_line_options = join(" ",@ARGV); @@ -198,8 +198,8 @@ GetOptions( 'labels' => \$labels, 'cards' => \$cards, 'return-claims' => \$return_claims, - 'bg-type:s' => \@background_types, - 'bg-days:i' => \$background_days, + 'jobs-type:s' => \@jobs_types, + 'jobs-days:i' => \$jobs_days, ) || usage(1); # Use default values @@ -212,8 +212,8 @@ $pSearchhistory = DEFAULT_SEARCHHISTORY_PURGEDAYS if defined($pSearchhis $pListShareInvites = DEFAULT_SHARE_INVITATION_EXPIRY_DAYS if defined($pListShareInvites) && $pListShareInvites == 0; $pDebarments = DEFAULT_DEBARMENTS_PURGEDAYS if defined($pDebarments) && $pDebarments == 0; $pMessages = DEFAULT_MESSAGES_PURGEDAYS if defined($pMessages) && $pMessages == 0; -$background_days = DEFAULT_BGJOBS_PURGEDAYS if defined($background_days) && $background_days == 0; -@background_types = (DEFAULT_BGJOBS_PURGETYPES) if $background_days && @background_types == 0; +$jobs_days = DEFAULT_JOBS_PURGEDAYS if defined($jobs_days) && $jobs_days == 0; +@jobs_types = (DEFAULT_JOBS_PURGETYPES) if $jobs_days && @jobs_types == 0; if ($help) { usage(0); @@ -251,7 +251,7 @@ unless ( $sessions || $labels || $cards || $return_claims - || $background_days + || $jobs_days ) { print "You did not specify any cleanup work for the script to do.\n\n"; usage(1); @@ -688,17 +688,28 @@ if ($cards) { } } -if ($background_days) { - print "Purging background jobs more than $background_days days ago.\n" if $verbose; - my $params = { job_types => \@background_types , - days => $background_days, - confirm => $confirm, - }; - my $count = Koha::BackgroundJobs->purge( $params ); +if ($jobs_days) { + print "Purging background jobs more than $jobs_days days ago.\n" + if $verbose; + my $jobs = Koha::BackgroundJobs->search( + { + status => 'finished', + ( $jobs_types[0] eq 'all' ? () : ( type => \@jobs_types ) ) + } + )->filter_by_last_update( + { + timestamp_column_name => 'ended_on', + days => $jobs_days, + } + ); + my $count = $jobs->count; + $jobs->delete if $confirm; if ($verbose) { say $confirm - ? sprintf "Done with purging %d background jobs of type(s): %s added more than %d days ago.\n", $count, join(',', @background_types), $background_days - : sprintf "%d background jobs of type(s): %s added more than %d days ago would have been purged.", $count, join(',', @background_types), $background_days; + ? sprintf "Done with purging %d background jobs of type(s): %s added more than %d days ago.\n", + $count, join( ',', @jobs_types ), $jobs_days + : sprintf "%d background jobs of type(s): %s added more than %d days ago would have been purged.", + $count, join( ',', @jobs_types ), $jobs_days; } } diff --git a/t/db_dependent/Koha/BackgroundJobs.t b/t/db_dependent/Koha/BackgroundJobs.t index c9cfdca132..c8089d4488 100755 --- a/t/db_dependent/Koha/BackgroundJobs.t +++ b/t/db_dependent/Koha/BackgroundJobs.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 15; +use Test::More tests => 14; use Test::MockModule; use List::MoreUtils qw(any); @@ -142,69 +142,3 @@ subtest 'search_limited' => sub { $schema->storage->txn_rollback; }; - -subtest 'purge' => sub { - plan tests => 9; - $schema->storage->txn_begin; - my $cnt_finished = Koha::BackgroundJobs->search({ status => 'finished' })->count; - - my $recent_date = dt_from_string; - my $old_date = dt_from_string->subtract({ days => 3 }); - my $job_recent_t1_new = $builder->build_object( { class => 'Koha::BackgroundJobs', value => { status => 'new', ended_on => $old_date, type => 'type1' } } ); - my $job_recent_t2_fin = $builder->build_object( { class => 'Koha::BackgroundJobs', value => { status => 'finished', ended_on => $recent_date, type => 'type2' } } ); - my $job_old_t1_fin = $builder->build_object( { class => 'Koha::BackgroundJobs', value => { status => 'finished', ended_on => $old_date, type => 'type1' } } ); - my $job_old_t2_fin = $builder->build_object( { class => 'Koha::BackgroundJobs', value => { status => 'finished', ended_on => $old_date, type => 'type2' } } ); - - my $params = { job_types => ['type1'] , # Arrayref of jobtypes to be purged - days => 1, # Age in days of jobs to be purged - confirm => 0, # Confirm deletion - }; - is( Koha::BackgroundJobs->purge($params), 1, 'Only the old finished type1 job would be purged' ); - - $params->{'job_types'} = ['all']; - is( Koha::BackgroundJobs->purge($params), $cnt_finished + 2, 'All finished old jobs would be purged with job_types = all' ); - - my $rs = Koha::BackgroundJobs->search( - { - id => [ $job_recent_t1_new->id, $job_recent_t2_fin->id, $job_old_t1_fin->id, $job_old_t2_fin->id ] - } - ); - is( $rs->count, 4, 'All jobs still left in queue'); - - $params->{'job_types'} = ['type1']; - $params->{'confirm'} = 1; - is( Koha::BackgroundJobs->purge($params), 1, 'Only the old finished type1 job is purged' ); - - $rs = Koha::BackgroundJobs->search( - { - id => [ $job_recent_t1_new->id, $job_recent_t2_fin->id, $job_old_t1_fin->id, $job_old_t2_fin->id ] - } - ); - is( $rs->count, 3, '3 jobs still left in queue'); - - $params->{'job_types'} = ['all']; - is( Koha::BackgroundJobs->purge($params), $cnt_finished + 1, 'The remaining old finished jobs is purged' ); - $rs = Koha::BackgroundJobs->search( - { - id => [ $job_recent_t1_new->id, $job_recent_t2_fin->id, $job_old_t1_fin->id, $job_old_t2_fin->id ] - } - ); - is( $rs->count, 2, '2 jobs still left in queue'); - - $rs = Koha::BackgroundJobs->search( - { - id => [ $job_recent_t1_new->id ] - } - ); - is( $rs->count, 1, 'Unfinished job still left in queue'); - - $rs = Koha::BackgroundJobs->search( - { - id => [ $job_recent_t2_fin->id ] - } - ); - is( $rs->count, 1, 'Recent finished job still left in queue'); - - $schema->storage->txn_rollback; - -}; -- 2.39.5