Bugzilla – Attachment 156276 Details for
Bug 31383
Additional contents: We need a parent and child table
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 31383: (QA follow-up) Tidy
Bug-31383-QA-follow-up-Tidy.patch (text/plain), 20.04 KB, created by
Martin Renvoize (ashimema)
on 2023-09-27 10:01:58 UTC
(
hide
)
Description:
Bug 31383: (QA follow-up) Tidy
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2023-09-27 10:01:58 UTC
Size:
20.04 KB
patch
obsolete
>From 0e5b96f0a528003928bd51878781aa95381623b0 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@ptfs-europe.com> >Date: Thu, 21 Sep 2023 17:40:47 +0100 >Subject: [PATCH] Bug 31383: (QA follow-up) Tidy > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > .../Result/AdditionalContentsLocalization.pm | 13 +++ > .../data/mysql/atomicupdate/bug_31383.pl | 87 ++++++++++++------- > t/db_dependent/Koha/AdditionalContents.t | 74 ++++++++++------ > tools/additional-contents.pl | 43 ++++----- > 4 files changed, 141 insertions(+), 76 deletions(-) > >diff --git a/Koha/Schema/Result/AdditionalContentsLocalization.pm b/Koha/Schema/Result/AdditionalContentsLocalization.pm >index ed4fb504ab2..ea77d1b51a5 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 0b825813250..0aece4ae4ff 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 "%<news>%" >- }, { Slice => {} }); >- for my $template ( @$notice_templates ) { >+ }, { Slice => {} } >+ ); >+ for my $template (@$notice_templates) { > my $new_content = $template->{content}; > $new_content =~ s|<news>|[% FOR content IN additional_contents %]|g; > $new_content =~ s|</news>|[% 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 "%<<additional_content%" OR content LIKE "%<< additional_content%" >- }); >+ } >+ ); > >- if ( @$notice_templates ) { >+ if (@$notice_templates) { > say $out "WARNING - Some notice templates need manual adjustments!"; >- for my $template ( @$notice_templates ) { >+ for my $template (@$notice_templates) { > say $out sprintf "\t %s (id=%s)", $template->{code}, $template->{id}; > } > } >diff --git a/t/db_dependent/Koha/AdditionalContents.t b/t/db_dependent/Koha/AdditionalContents.t >index f3a05c67ad9..565b3fe3f8d 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 988089025ff..dfb96fcb00f 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.41.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 31383
:
147891
|
147892
|
147893
|
147894
|
147895
|
147896
|
149813
|
149814
|
149815
|
149816
|
149817
|
149818
|
152423
|
152424
|
152425
|
152426
|
152427
|
152428
|
152545
|
152546
|
152547
|
152566
|
152660
|
152661
|
152662
|
152663
|
152664
|
152665
|
152666
|
152667
|
152668
|
152669
|
153013
|
153666
|
153667
|
153668
|
153669
|
153670
|
153671
|
153672
|
153673
|
153674
|
153675
|
153676
|
156048
|
156049
|
156050
|
156051
|
156052
|
156053
|
156054
|
156055
|
156056
|
156057
|
156058
|
156265
|
156266
|
156267
|
156268
|
156269
|
156270
|
156271
|
156272
|
156273
|
156274
|
156275
|
156276
|
157539
|
157540
|
157541
|
157542
|
157543
|
157544
|
157545
|
157546
|
157547
|
157548
|
157549
|
157550
|
157580
|
157585
|
157640
|
157741
|
157742