From de8eaf4a07fb0ab9a193a30ca96d23254f7ae0dd 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 --- Koha/BackgroundJobs.pm | 45 ------------------- misc/cronjobs/cleanup_database.pl | 51 ++++++++++++--------- t/db_dependent/Koha/BackgroundJobs.t | 67 +--------------------------- 3 files changed, 32 insertions(+), 131 deletions(-) diff --git a/Koha/BackgroundJobs.pm b/Koha/BackgroundJobs.pm index 8d7ae64e0e9..48afbf68fa8 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 96783c159fe..0c32bc02ca2 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 ed4fd8d9fb2..cfb9c28cc76 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); @@ -140,68 +140,3 @@ subtest 'search_limited' => sub { $schema->storage->txn_rollback; }; - -subtest 'purge' => sub { - plan tests => 9; - $schema->storage->txn_begin; - - 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), 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), 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.25.1