From 5bab527644cf84f0515688f14ec34894ce245beb Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 21 Sep 2023 17:40:47 +0100 Subject: [PATCH] Bug 31383: (QA follow-up) Tidy Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi --- Koha/AdditionalContentsLocalization.pm | 2 +- .../Result/AdditionalContentsLocalization.pm | 13 +++ .../data/mysql/atomicupdate/bug_31383.pl | 87 ++++++++++++------- t/db_dependent/Koha/AdditionalContents.t | 74 ++++++++++------ tools/additional-contents.pl | 43 ++++----- 5 files changed, 142 insertions(+), 77 deletions(-) diff --git a/Koha/AdditionalContentsLocalization.pm b/Koha/AdditionalContentsLocalization.pm index f70def51c7..20302f76ca 100644 --- a/Koha/AdditionalContentsLocalization.pm +++ b/Koha/AdditionalContentsLocalization.pm @@ -41,7 +41,7 @@ Return the Koha::AdditionalContent for this translated content. =cut sub additional_content { - my ( $self ) = @_; + my ($self) = @_; my $rs = $self->_result->additional_content; return Koha::AdditionalContent->_new_from_dbic($rs); } diff --git a/Koha/Schema/Result/AdditionalContentsLocalization.pm b/Koha/Schema/Result/AdditionalContentsLocalization.pm index ed4fb504ab..ea77d1b51a 100644 --- a/Koha/Schema/Result/AdditionalContentsLocalization.pm +++ b/Koha/Schema/Result/AdditionalContentsLocalization.pm @@ -159,9 +159,22 @@ __PACKAGE__->belongs_to( # Created by DBIx::Class::Schema::Loader v0.07049 @ 2023-03-08 09:59:00 # DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:7N/mHMqyPJJsYaA8V968wQ +=head2 koha_object_class + + Koha Object class + +=cut + sub koha_object_class { 'Koha::AdditionalContentsLocalization'; } + +=head2 koha_objects_class + + Koha Objects class + +=cut + sub koha_objects_class { 'Koha::AdditionalContentsLocalizations'; } diff --git a/installer/data/mysql/atomicupdate/bug_31383.pl b/installer/data/mysql/atomicupdate/bug_31383.pl index 0b82581325..0aece4ae4f 100755 --- a/installer/data/mysql/atomicupdate/bug_31383.pl +++ b/installer/data/mysql/atomicupdate/bug_31383.pl @@ -1,19 +1,22 @@ use Modern::Perl; return { - bug_number => "31383", + bug_number => "31383", description => "Split the additional_contents table", - up => sub { + up => sub { my ($args) = @_; - my ($dbh, $out) = @$args{qw(dbh out)}; + my ( $dbh, $out ) = @$args{qw(dbh out)}; unless ( TableExists('additional_contents_localizations') ) { - $dbh->do(q{ + $dbh->do( + q{ ALTER TABLE additional_contents CHANGE COLUMN idnew `id` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'unique identifier for the additional content category' - }); + } + ); say $out "Renamed additional_contents.idnew with 'id'"; - $dbh->do(q{ + $dbh->do( + q{ CREATE TABLE `additional_contents_localizations` ( `id` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'unique identifier for the additional content', `additional_content_id` int(10) unsigned NOT NULL COMMENT 'link to the additional content', @@ -25,24 +28,36 @@ return { UNIQUE KEY `additional_contents_localizations_uniq` (`additional_content_id`,`lang`), CONSTRAINT `additional_contents_localizations_ibfk1` FOREIGN KEY (`additional_content_id`) REFERENCES `additional_contents` (`id`) ON DELETE CASCADE ON UPDATE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; - }); + } + ); say $out "Added new table 'additional_contents_localizations'"; - my $contents = $dbh->selectall_arrayref(q{SELECT MIN(id) AS id, category, code, branchcode FROM additional_contents GROUP BY category, code, branchcode}, { Slice => {} }); - my $sth_insert = $dbh->prepare(q{ + my $contents = $dbh->selectall_arrayref( + q{SELECT MIN(id) AS id, category, code, branchcode FROM additional_contents GROUP BY category, code, branchcode}, + { Slice => {} } + ); + my $sth_insert = $dbh->prepare( + q{ INSERT INTO additional_contents_localizations(additional_content_id, title, content, lang, updated_on) VALUES(?, ?, ?, ?, ?) - }); - for my $content ( @$contents ) { + } + ); + for my $content (@$contents) { my $q = q{ SELECT title, content, lang, branchcode, updated_on FROM additional_contents WHERE category=? AND code=? AND }; $q .= defined $content->{branchcode} ? " branchcode = ?" : " branchcode IS NULL"; - my $translated_contents = $dbh->selectall_arrayref($q, { Slice => {} }, $content->{category}, $content->{code}, defined $content->{branchcode} ? $content->{branchcode} : ()); - for my $translated_content ( @$translated_contents ) { - $sth_insert->execute($content->{id}, $translated_content->{title}, $translated_content->{content}, $translated_content->{lang}, $translated_content->{updated_on}); + my $translated_contents = $dbh->selectall_arrayref( + $q, { Slice => {} }, $content->{category}, $content->{code}, + defined $content->{branchcode} ? $content->{branchcode} : () + ); + for my $translated_content (@$translated_contents) { + $sth_insert->execute( + $content->{id}, $translated_content->{title}, $translated_content->{content}, + $translated_content->{lang}, $translated_content->{updated_on} + ); } # Delete duplicates @@ -51,55 +66,67 @@ return { WHERE category=? AND code=? AND id<>? AND }; $q .= defined $content->{branchcode} ? " branchcode = ?" : " branchcode IS NULL"; - $dbh->do($q, undef, $content->{category}, $content->{code}, $content->{id}, $content->{branchcode}); + $dbh->do( $q, undef, $content->{category}, $content->{code}, $content->{id}, $content->{branchcode} ); } - $dbh->do(q{ + $dbh->do( + q{ ALTER TABLE additional_contents DROP INDEX additional_contents_uniq - }); - $dbh->do(q{ + } + ); + $dbh->do( + q{ ALTER TABLE additional_contents ADD UNIQUE KEY `additional_contents_uniq` (`category`,`code`,`branchcode`) - }); - $dbh->do(q{ + } + ); + $dbh->do( + q{ ALTER TABLE additional_contents DROP COLUMN title, DROP COLUMN content, DROP COLUMN lang - }); + } + ); say $out "Removed 'title', 'content', 'lang' columns from additional_contents"; } - my $notice_templates = $dbh->selectall_arrayref(q{ + my $notice_templates = $dbh->selectall_arrayref( + q{ SELECT id, module, code, content FROM letter WHERE content LIKE "%%" - }, { Slice => {} }); - for my $template ( @$notice_templates ) { + }, { Slice => {} } + ); + for my $template (@$notice_templates) { my $new_content = $template->{content}; $new_content =~ s||[% FOR content IN additional_contents %]|g; $new_content =~ s||[% END %]|g; $new_content =~ s{<<\s*additional_contents\.title\s*>>}{[% content.title | html %]}g; $new_content =~ s{<<\s*additional_contents\.content\s*>>}{[% content.content | html %]}g; $new_content =~ s{<<\s*additional_contents\.published_on\s*>>}{[% content.published_on | \$KohaDates %]}g; - $dbh->do(q{ + $dbh->do( + q{ UPDATE letter SET content = ? WHERE id = ? - }, undef, $new_content, $template->{id}); + }, undef, $new_content, $template->{id} + ); say $out "Adjusted notice template '" . $template->{code} . "'"; } - $notice_templates = $dbh->selectall_arrayref(q{ + $notice_templates = $dbh->selectall_arrayref( + q{ SELECT id, code FROM letter WHERE content LIKE "%<{code}, $template->{id}; } } diff --git a/t/db_dependent/Koha/AdditionalContents.t b/t/db_dependent/Koha/AdditionalContents.t index f3a05c67ad..565b3fe3f8 100755 --- a/t/db_dependent/Koha/AdditionalContents.t +++ b/t/db_dependent/Koha/AdditionalContents.t @@ -52,7 +52,7 @@ subtest 'Koha::AdditionalContents basic test' => sub { content => 'content for news 1', lang => 'default', }; - $new_news_item_1->translated_contents([$content_1]); + $new_news_item_1->translated_contents( [$content_1] ); my $new_news_item_2 = Koha::AdditionalContent->new( { category => 'news', @@ -66,14 +66,17 @@ subtest 'Koha::AdditionalContents basic test' => sub { content => 'content for news 2', lang => 'default', }; - $new_news_item_2->translated_contents([$content_2]); + $new_news_item_2->translated_contents( [$content_2] ); - like( $new_news_item_1->id, qr|^\d+$|, 'Adding a new news_item should have set the id'); + like( $new_news_item_1->id, qr|^\d+$|, 'Adding a new news_item should have set the id' ); is( Koha::AdditionalContents->search->count, $nb_of_news + 2, 'The 2 news should have been added' ); my $retrieved_news_item_1 = Koha::AdditionalContents->find( $new_news_item_1->id )->translated_contents->next; - is( $retrieved_news_item_1->title, $content_1->{title}, 'Find a news_item by id should return the correct news_item' ); - is( $retrieved_news_item_1->content, $content_1->{content}, 'The content method return the content of the news'); + is( + $retrieved_news_item_1->title, $content_1->{title}, + 'Find a news_item by id should return the correct news_item' + ); + is( $retrieved_news_item_1->content, $content_1->{content}, 'The content method return the content of the news' ); my $default_content = $new_news_item_2->default_localization; is( $default_content->content, $content_2->{content}, 'default_localization return the default content' ); @@ -82,9 +85,15 @@ subtest 'Koha::AdditionalContents basic test' => sub { $default_content = $new_news_item_2->default_localization; is( $default_content->content, $content_2->{content}, 'default_localization still return the default content' ); my $retrieved_translated_content = $new_news_item_2->translated_content('en'); - is( $retrieved_translated_content->content, $content_2->{content}, 'default content is returned for non-existing translated interface' ); + is( + $retrieved_translated_content->content, $content_2->{content}, + 'default content is returned for non-existing translated interface' + ); $retrieved_translated_content = $new_news_item_2->translated_content('nl-NL'); - is( $retrieved_translated_content->content, $translated_content->{content}, 'translated content is returned if it existsOB' ); + is( + $retrieved_translated_content->content, $translated_content->{content}, + 'translated content is returned if it existsOB' + ); $new_news_item_1->delete; is( Koha::AdditionalContents->search->count, $nb_of_news + 1, 'Delete should have deleted the news_item' ); @@ -167,7 +176,7 @@ subtest '->author' => sub { $author->delete; - $news_item = Koha::AdditionalContents->find($news_item->id); + $news_item = Koha::AdditionalContents->find( $news_item->id ); is( ref($news_item), 'Koha::AdditionalContent', 'News are not deleted alongwith the author' ); is( $news_item->author, undef, '->author returns undef is the author has been deleted' ); @@ -199,7 +208,7 @@ subtest '->search_for_display' => sub { number => 1, } }); - $new_expired->translated_contents( [ { lang => 'default', content => ''} ] ); + $new_expired->translated_contents( [ { lang => 'default', content => '' } ] ); my $new_not_expired = $builder->build_object({ class => 'Koha::AdditionalContents', value => { @@ -283,27 +292,40 @@ subtest 'find_best_match' => sub { plan tests => 3; $schema->storage->txn_begin; - my $library01 = $builder->build_object({ class => 'Koha::Libraries' }); - my $html01 = $builder->build_object({ - class => 'Koha::AdditionalContents', - value => { category => 'html_customizations', location => 'test_best_match', branchcode => undef }, - }); - my ( $default_content ) = $html01->translated_contents( [ { lang => 'default', content => '' } ] )->as_list; + my $library01 = $builder->build_object( { class => 'Koha::Libraries' } ); + my $html01 = $builder->build_object( + { + class => 'Koha::AdditionalContents', + value => { category => 'html_customizations', location => 'test_best_match', branchcode => undef }, + } + ); + my ($default_content) = $html01->translated_contents( [ { lang => 'default', content => '' } ] )->as_list; my $params = { category => 'html_customizations', location => 'test_best_match', lang => 'nl-NL' }; - is( Koha::AdditionalContents->find_best_match($params)->id, $default_content->id, 'Found all branches, lang default' ); + is( + Koha::AdditionalContents->find_best_match($params)->id, $default_content->id, + 'Found all branches, lang default' + ); - my $html02 = $builder->build_object({ - class => 'Koha::AdditionalContents', - value => { category => 'html_customizations', location => 'test_best_match', branchcode => undef }, - }); - my ( $translated_content ) = $html02->translated_contents( [ { lang => 'nl-NL', content => '' } ] )->as_list; - is( Koha::AdditionalContents->find_best_match($params)->id, $translated_content->id, 'Found all branches, lang nl-NL' ); - - $params->{ library_id } = $library01->id; + my $html02 = $builder->build_object( + { + class => 'Koha::AdditionalContents', + value => { category => 'html_customizations', location => 'test_best_match', branchcode => undef }, + } + ); + my ($translated_content) = $html02->translated_contents( [ { lang => 'nl-NL', content => '' } ] )->as_list; + is( + Koha::AdditionalContents->find_best_match($params)->id, $translated_content->id, + 'Found all branches, lang nl-NL' + ); + + $params->{library_id} = $library01->id; $html02->branchcode( $library01->id )->store; - is( Koha::AdditionalContents->find_best_match($params)->id, $translated_content->id, 'Found library01, lang nl-NL' ); + is( + Koha::AdditionalContents->find_best_match($params)->id, $translated_content->id, + 'Found library01, lang nl-NL' + ); # Note: find_best_match is tested further via $libary->opac_info; see t/db_dependent/Koha/Library.t $schema->storage->txn_rollback; -} + } diff --git a/tools/additional-contents.pl b/tools/additional-contents.pl index 988089025f..dfb96fcb00 100755 --- a/tools/additional-contents.pl +++ b/tools/additional-contents.pl @@ -77,7 +77,7 @@ if ( $op eq 'add_form' ) { ); } elsif ( $op eq 'add_validate' ) { - output_and_exit_if_error($cgi, $cookie, $template, { check => 'csrf_token' }); + output_and_exit_if_error( $cgi, $cookie, $template, { check => 'csrf_token' } ); my $location = $cgi->param('location'); my $code = $cgi->param('code'); my $branchcode = $cgi->param('branchcode') || undef; @@ -93,15 +93,15 @@ elsif ( $op eq 'add_validate' ) { sub { my $additional_content; my $params = { - location => $location, - branchcode => $branchcode, - expirationdate => $expirationdate, - published_on => $published_on, - number => $number, - borrowernumber => $borrowernumber, - }; + location => $location, + branchcode => $branchcode, + expirationdate => $expirationdate, + published_on => $published_on, + number => $number, + borrowernumber => $borrowernumber, + }; - if ( $id ) { + if ($id) { $additional_content = Koha::AdditionalContents->find($id); $additional_content->set($params)->store; } else { @@ -117,9 +117,9 @@ elsif ( $op eq 'add_validate' ) { unless ($code) { $additional_content->discard_changes; $code = - $category eq 'news' - ? 'News_' . $additional_content->id - : $location . '_' . $additional_content->id; + $category eq 'news' + ? 'News_' . $additional_content->id + : $location . '_' . $additional_content->id; $additional_content->code($code)->store; $id = $additional_content->id; } @@ -142,16 +142,19 @@ elsif ( $op eq 'add_validate' ) { }; my $existing_content = $existing_contents->find($id); - if ( $existing_content ) { + if ($existing_content) { if ( $existing_content->title ne $title || $existing_content->content ne $content ) { if ( C4::Context->preference("NewsLog") ) { - logaction('NEWS', 'MODIFY' , undef, sprintf("%s|%s|%s|%s", $code, $title, $lang, $content)); + logaction( + 'NEWS', 'MODIFY', undef, + sprintf( "%s|%s|%s|%s", $code, $title, $lang, $content ) + ); } } else { $translated_content->{updated_on} = $existing_content->updated_on; } - } elsif( C4::Context->preference("NewsLog") ) { - logaction('NEWS', 'ADD' , undef, sprintf("%s|%s|%s|%s", $code, $title, $lang, $content)) + } elsif ( C4::Context->preference("NewsLog") ) { + logaction( 'NEWS', 'ADD', undef, sprintf( "%s|%s|%s|%s", $code, $title, $lang, $content ) ); } push @translated_contents, $translated_content; @@ -159,10 +162,10 @@ elsif ( $op eq 'add_validate' ) { if ( C4::Context->preference("NewsLog") ) { my @existing_ids = $existing_contents->get_column('id'); - my @deleted_ids = array_minus( @existing_ids, @seen_ids ); - for my $id ( @deleted_ids ) { + my @deleted_ids = array_minus( @existing_ids, @seen_ids ); + for my $id (@deleted_ids) { my $c = $existing_contents->find($id); - logaction('NEWS', 'DELETE' , undef, sprintf("%s|%s|%s", $code, $c->lang, $c->content)); + logaction( 'NEWS', 'DELETE', undef, sprintf( "%s|%s|%s", $code, $c->lang, $c->content ) ); } } @@ -171,7 +174,7 @@ elsif ( $op eq 'add_validate' ) { ); } catch { warn $_; - if ( $id ) { + if ($id) { push @messages, { type => 'error', code => 'error_on_update' }; } else { push @messages, { type => 'error', code => 'error_on_insert' }; -- 2.39.5