From 4ab7efb645f7dac89dce59a7e0f567bd8774e2ea Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Wed, 15 Feb 2023 14:47:45 +0000 Subject: [PATCH] Bug 32334: (QA follow-up) Remove fh and verbose parameter in favor of messages The module returns messages. The script can print them in verbose mode. Test script adjusted accordingly. Test plan: Run t/db_dependent/Koha/Database/Commenter.t Signed-off-by: Marcel de Rooy Signed-off-by: David Nind --- Koha/Database/Commenter.pm | 47 ++++++++++++------------ misc/maintenance/sync_db_comments.pl | 12 +++--- t/db_dependent/Koha/Database/Commenter.t | 24 ++++++------ 3 files changed, 41 insertions(+), 42 deletions(-) diff --git a/Koha/Database/Commenter.pm b/Koha/Database/Commenter.pm index 66d8d05ce9..9d4959e0ea 100644 --- a/Koha/Database/Commenter.pm +++ b/Koha/Database/Commenter.pm @@ -54,22 +54,21 @@ Koha::Database::Commenter - Manage column comments in database =head2 new $mgr = Koha::Database::Commenter->new({ - dbh => $dbh, database => $d, fh => $fh, schema_file => $s + dbh => $dbh, database => $d, schema_file => $s }); Object constructor. - Param dbh is mandatory. Params database, fh and schema_file are + Param dbh is mandatory. Params database and schema_file are optional. Param database can be used to move away from current database of db handle. - Param fh can be used to redirect output. Param schema_file is needed for resetting to schema. Falls back to the constant for Koha structure file. =cut sub new { - my ( $class, $params ) = @_; # params: database, dbh, fh, schema_file + my ( $class, $params ) = @_; # params: database, dbh, schema_file my $self = bless $params // {}, $class; Koha::Exceptions::MissingParameter->throw( parameter => 'dbh' ) unless $self->{dbh}; @@ -77,7 +76,6 @@ sub new { unless ref($self->{dbh}) eq DBI_HANDLE_CLASS; $self->{database} //= ( $self->{dbh}->selectrow_array('SELECT DATABASE()') )[0]; - $self->{fh} //= *STDOUT; $self->{schema_file} //= KOHA_STRUCTURE; $self->{schema_info} = {}; @@ -86,68 +84,68 @@ sub new { =head2 clear - $object->clear({ dry_run => 0, table => $table, verbose => 0 }); + $object->clear({ dry_run => 0, table => $table }, $messages ); Clears all current column comments in storage. If table is passed, only that table is changed. - Dry run only prints sql statements. + Dry run only returns sql statements in $messages (arrayref). =cut sub clear { - my ( $self, $params ) = @_; # dry_run, table, verbose + my ( $self, $params, $messages ) = @_; # dry_run, table my $cols = $self->_fetch_stored_comments($params); foreach my $col ( @$cols ) { next if !$col->{column_comment}; next if $params->{table} && $col->{table_name} ne $params->{table}; - $self->_change_column( $col->{table_name}, $col->{column_name}, undef, $params ); # undef clears + $self->_change_column( $col->{table_name}, $col->{column_name}, undef, $params, $messages ); # undef clears } } =head2 reset_to_schema - $object->reset_to_schema({ dry_run => 0, table => $table, verbose => 0 }); + $object->reset_to_schema({ dry_run => 0, table => $table }, $messages ); Resets column comments in storage to schema definition. Other column comments are cleared. When you pass table, only that table is changed. - Dry run only prints sql statements. + Dry run only returns sql statements in $messages (arrayref). =cut sub reset_to_schema { - my ( $self, $params ) = @_; # dry_run, table, verbose - $self->clear($params); + my ( $self, $params, $messages ) = @_; # dry_run, table + $self->clear( $params, $messages ); my $schema_comments = $self->_fetch_schema_comments; foreach my $table ( sort keys %$schema_comments ) { next if $params->{table} && $table ne $params->{table}; foreach my $col ( sort keys %{$schema_comments->{$table}} ) { - $self->_change_column( $table, $col, $schema_comments->{$table}->{$col}, $params ); + $self->_change_column( $table, $col, $schema_comments->{$table}->{$col}, $params, $messages ); } } } =head2 renumber - $object->renumber({ dry_run => 0, table => $table, verbose => 0 }); + $object->renumber({ dry_run => 0, table => $table }, $messages ); This is primarily meant for testing purposes (verifying results across whole database). It adds comments like Comment_1, Comment_2 etc. When you pass table, only that table is changed. Otherwise all tables are affected; note that the column counter does not reset by table. - Dry run only prints sql statements. + Dry run only returns sql statements in $messages (arrayref). =cut sub renumber { - my ( $self, $params ) = @_; # dry_run, table, verbose + my ( $self, $params, $messages ) = @_; # dry_run, table my $cols = $self->_fetch_stored_comments($params); my $i = 0; foreach my $col ( @$cols ) { next if $params->{table} && $col->{table_name} ne $params->{table}; $i++; - $self->_change_column( $col->{table_name}, $col->{column_name}, "Column_$i", $params ); + $self->_change_column( $col->{table_name}, $col->{column_name}, "Column_$i", $params, $messages ); } } @@ -203,7 +201,7 @@ ORDER BY table_name, column_name|; sub _change_column { # NOTE: We do not want to use DBIx schema here, but we use stored structure, # since we only want to change comment not actual table structure. - my ( $self, $table_name, $column_name, $comment, $params ) = @_; # params: dry_run, verbose + my ( $self, $table_name, $column_name, $comment, $params, $messages ) = @_; # params: dry_run $params //= {}; my $dbh = $self->{dbh}; @@ -239,18 +237,19 @@ sub _change_column { } $rv =~ s/\s+$//; # remove trailing spaces - # Print + # Dry run if( $params->{dry_run} ) { - print { $self->{fh} } "$rv;\n"; + push @$messages, "$rv;" if $messages; return; } + # Deploy eval { $dbh->do($rv) }; if( $@ ) { warn "Failure for $table_name:$column_name"; - print { $self->{fh} } "-- FAILED: $rv;\n"; - } elsif( $params->{verbose} ) { - print { $self->{fh} } "$rv;\n"; + push @$messages, "-- FAILED: $rv;" if $messages; + } else { + push @$messages, "$rv;" if $messages; } } diff --git a/misc/maintenance/sync_db_comments.pl b/misc/maintenance/sync_db_comments.pl index 3382467e5b..5014c4ac68 100755 --- a/misc/maintenance/sync_db_comments.pl +++ b/misc/maintenance/sync_db_comments.pl @@ -26,6 +26,8 @@ use Pod::Usage qw( pod2usage ); use C4::Context; use Koha::Database::Commenter; +sub alert_dry_run { print "-- DRY RUN\n" if $_[0]->{dry_run}; } + my $cmd_args = {}; GetOptions( 'clear' => \$cmd_args->{clear}, @@ -40,24 +42,24 @@ GetOptions( $cmd_args->{dry_run} = !$cmd_args->{commit}; my $commenter = Koha::Database::Commenter->new({ database => delete $cmd_args->{database}, dbh => C4::Context->dbh }); +my $messages = $cmd_args->{verbose} || $cmd_args->{dry_run} ? [] : undef; if( $cmd_args->{help} ) { pod2usage( -verbose => 2 ); } elsif( ($cmd_args->{clear}||0) + ($cmd_args->{renumber}||0) + ($cmd_args->{reset}||0) > 1 ) { print "You cannot pass the clear, renumber and reset flags together\n"; } elsif( delete $cmd_args->{clear} ) { alert_dry_run( $cmd_args ); - $commenter->clear( $cmd_args ); + $commenter->clear( $cmd_args, $messages ); } elsif( delete $cmd_args->{reset} ) { alert_dry_run( $cmd_args ); - $commenter->reset_to_schema( $cmd_args ); + $commenter->reset_to_schema( $cmd_args, $messages ); } elsif( delete $cmd_args->{renumber} ) { alert_dry_run( $cmd_args ); - $commenter->renumber( $cmd_args ); + $commenter->renumber( $cmd_args, $messages ); } else { pod2usage( -verbose => 1 ); } - -sub alert_dry_run { print "-- DRY RUN\n" if $_[0]->{dry_run}; } +print join("\n", @$messages), "\n" if $messages && @$messages; __END__ diff --git a/t/db_dependent/Koha/Database/Commenter.t b/t/db_dependent/Koha/Database/Commenter.t index cf1375311e..4ca9180ec3 100755 --- a/t/db_dependent/Koha/Database/Commenter.t +++ b/t/db_dependent/Koha/Database/Commenter.t @@ -24,31 +24,29 @@ subtest '->new, dry_run' => sub { # Another exception: delete schema file and reset should raise exception my $filename = create_schema_file(); - my $stdout; - open my $fh, '>', \$stdout; - $mgr = Koha::Database::Commenter->new({ dbh => $dbh, schema_file => $filename, fh => $fh }); + $mgr = Koha::Database::Commenter->new({ dbh => $dbh, schema_file => $filename }); unlink $filename; throws_ok { $mgr->reset_to_schema({ dry_run => 1, table => 'biblio' }) } 'Koha::Exceptions::FileNotFound', 'Schema deleted'; # Clear comments for article_requests in dry run mode - $stdout = q{}; - $mgr->clear({ table => 'article_requests', dry_run => 1 }); - like( $stdout, qr/COLUMN `toc_request`.*DEFAULT 0;$/m, 'No comment for toc_request' ); + my $messages = []; + $mgr->clear( { table => 'article_requests', dry_run => 1 }, $messages ); + is( grep( { /COLUMN `toc_request`.*DEFAULT 0;$/m } @$messages ), 1, 'No comment for toc_request' ); # Renumber this field in dry run mode - $stdout = q{}; - $mgr->renumber({ table => 'article_requests', dry_run => 1 }); - like( $stdout, qr/COLUMN `toc_request`.*COMMENT 'Column_\d+';$/m, 'Numbered comment for toc_request' ); + $messages = []; + $mgr->renumber( { table => 'article_requests', dry_run => 1 }, $messages ); + is( grep( { /COLUMN `toc_request`.*COMMENT 'Column_\d+';$/m } @$messages ), 1, 'Numbered comment for toc_request' ); # Reset in dry run mode, first fix schema file again # Our fake schema contains only one column for article_requests now. $filename = create_schema_file(); - $mgr = Koha::Database::Commenter->new({ dbh => $dbh, schema_file => $filename, fh => $fh }); - $stdout = q{}; - $mgr->reset_to_schema({ table => 'article_requests', dry_run => 1 }); + $mgr = Koha::Database::Commenter->new({ dbh => $dbh, schema_file => $filename }); + $messages = []; + $mgr->reset_to_schema( { table => 'article_requests', dry_run => 1 }, $messages ); # We expect an ALTER clearing toc_request first, followed by an ALTER adding comment. # Note: This is based on the fair assumption that toc_request had a comment! This test could fail if it had not. - like( $stdout, qr/ALTER.*toc_request.*DEFAULT 0;(.*\n)+ALTER.*toc_request.*COMMENT.*$/m, 'Reset for one-columned article_requests' ); + like( join("\n", @$messages), qr/ALTER.*toc_request.*DEFAULT 0;(.*\n)+ALTER.*toc_request.*COMMENT.*$/m, 'Reset for one-columned article_requests' ); $schema->storage->txn_rollback; }; -- 2.30.2