In the design of additional contents the idea of a parent-child relation is implicitly present. You have a default page and translations. But we do this in one table coming from the old news items. Several reports show (imo) that we would be better off creating a parent table listing the main news items, CMS pages or HTML content. And a child table containing all language related stuff.
For a less radical engineering change, we could just add a parent_id column to the existing table.
(In reply to Kyle M Hall from comment #1) > For a less radical engineering change, we could just add a parent_id column > to the existing table. Reading the bug description, that was my first thought too, but I think Marcel didn't mean having a main page and parent pages, but something different. We'd have one main entry for the type of customization in the 'parent' table, like "OpacNav". Then the second table would be an 1:n relationship having the translations. We see a lot of bugs because the current structure requires a "default" entry to exist and things like that. I also think we might need to rethink the structure, but it will be a bigger change.
> Then the second table would be an 1:n relationship having the translations. ... and the default and basically all the content. That about right, Marcel?
(In reply to Katrin Fischer from comment #3) > > Then the second table would be an 1:n relationship having the translations. > > ... and the default and basically all the content. > > That about right, Marcel? Yes as mentioned in description. The tables change, the interface is not drastically changed.
(In reply to Katrin Fischer from comment #2) > (In reply to Kyle M Hall from comment #1) > > For a less radical engineering change, we could just add a parent_id column > > to the existing table. > > Reading the bug description, that was my first thought too, but I think > Marcel didn't mean having a main page and parent pages, but something > different. > > We'd have one main entry for the type of customization in the 'parent' > table, like "OpacNav". > > Then the second table would be an 1:n relationship having the translations. > > We see a lot of bugs because the current structure requires a "default" > entry to exist and things like that. I also think we might need to rethink > the structure, but it will be a bigger change. Ah, thanks for the clarity Katrin!
Created attachment 147891 [details] [review] Bug 31383: Create a parent-child DB relation for additional content In the design of additional contents the idea of a parent-child relation is implicitly present. You have a default page and translations. But we do this in one table coming from the old news items. Several reports show that we would be better off creating a parent table listing the main news items, CMS pages or HTML content. And a child table containing the title, content and lang. Note that this first step is a prelimenary step to clean this area and make it more robust and extensible. More enhancements to come. What is this patchset doing? * DB changes - Rename additional_contents.idnew with id - Create a new table additional_contents_localizations(id, additional_content_id, title, content, lang) that will contain the translated contents - Move the content to this new table - Remove title, content and lang columns from additional_contents - Replace the notice templates that are using ''<news>" (should only be ISSUESLIP) and remove support for this syntax. Also add a warning in case other occurrences of uses of the old syntax exist. * CRUD - We add a new Koha::AdditionalContentsLocalization[s] couple, and move some logic from Koha::AdditionalContent[s] to there. Note that, to prevent too much drastic changes in notice templates, and to make them easy to use, the different attributes of the content object is accessible from the translated content object (ie. Koha::AdditionalContentsLocatlization->library is available and return $self->additional_content->library). I think it's an elegant way to keep things simple. - No changes expected for "NewsLog" logging - Little behaviour changes for pages, see tools/page.pl changes. We are now passing the id of the content, and the desired language, instead of the mix of "page_id" or code and lang. Note that here we certainly need to rename "language" query param to not change the full interface language. Test plan: 0. Preparation steps, use master a. Create notice templates that are using "<< additional_contents.code >>". This won't be replaced, but we want the update process to alert us. b. Create several news, additional contents, pages. Some with translated contents, some without. c. Make sure ISSUESLIP has the "<news>" section. If you are using the sample data there is nothing to do here d. Turn on NewsLogs 1. Apply the patches, restart_all, updatedatabase => Confirm that the new table is created and filled with the contents you had prior to the update => Confirm that additional_contents_localizations.updated_on has been kept to the previous values => Confirm that ISSUESLIP has been replaced properly => Confirm that you get a warning about the additional_contents 2. Create, update, delete news, html customs, pages => Confirm that the additional_contents_localizations.updated_on is only adjusted when required => Confirm that the logs are correctly created when NewsLogs is on 3. Check some items out, generate a slip => Confirm that the news are displayed at the bottom of the slip, and that the publication date is correctly formatted 4. Have several HTML customizations (like OpacNav, opacheader), in translated in different languages => Confirm that the default values is displayed when you are using the interface in a language without translation => Confirm that the translated version is picked when it exists Notes for QA: * I am not sure we really need the alert during the update DB process about the additional_contents leftover. We should not have them outside of ISSUESLIP. Shouldn't it hurt? * There is something ugly in sample_news.yml, the id is hardcoded. But how do we prevent that and keep translatability? Sponsored-by: Rijksmuseum, Netherlands
Created attachment 147892 [details] [review] Bug 31383: Adjust and add tests Sponsored-by: Rijksmuseum, Netherlands
Created attachment 147893 [details] [review] Bug 31383: DB changes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 147894 [details] [review] Bug 31383: DBIC changes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 147895 [details] [review] Bug 31383: DBIC specific Sponsored-by: Rijksmuseum, Netherlands
Created attachment 147896 [details] [review] Bug 31383: Restore updated_on for individual content Sponsored-by: Rijksmuseum, Netherlands
This is ready for testing!
Looking here. Rebased conflict with new WRAPPER in koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt
FAIL Koha/Schema/Result/AdditionalContentsLocalization.pm FAIL pod coverage POD is missing for koha_object_class POD is missing for koha_objects_class FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt FAIL valid_template DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'me.id' in 'field list' at /usr/share/koha/Koha/Objects.pm line 421 FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-page.tt FAIL valid_template DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'me.id' in 'field list' at /usr/share/koha/Koha/Objects.pm line 421
perl installer/data/mysql/updatedatabase.pl DEV atomic update /usr/share/koha/installer/data/mysql/atomicupdate/bug_31383.pl [13:36:56]: Bug 31383 - Split the additional_contents table Renamed additional_contents.idnew with 'id' Added new table 'additional_contents_localizations' Removed 'title', 'content', 'lang' columns from additional_contents ERROR - {UNKNOWN}: Not a HASH reference at /usr/share/koha/installer/data/mysql/atomicupdate/bug_31383.pl line 71. at /usr/share/koha/C4/Installer.pm line 743
Created attachment 149813 [details] [review] Bug 31383: Create a parent-child DB relation for additional content In the design of additional contents the idea of a parent-child relation is implicitly present. You have a default page and translations. But we do this in one table coming from the old news items. Several reports show that we would be better off creating a parent table listing the main news items, CMS pages or HTML content. And a child table containing the title, content and lang. Note that this first step is a prelimenary step to clean this area and make it more robust and extensible. More enhancements to come. What is this patchset doing? * DB changes - Rename additional_contents.idnew with id - Create a new table additional_contents_localizations(id, additional_content_id, title, content, lang) that will contain the translated contents - Move the content to this new table - Remove title, content and lang columns from additional_contents - Replace the notice templates that are using ''<news>" (should only be ISSUESLIP) and remove support for this syntax. Also add a warning in case other occurrences of uses of the old syntax exist. * CRUD - We add a new Koha::AdditionalContentsLocalization[s] couple, and move some logic from Koha::AdditionalContent[s] to there. Note that, to prevent too much drastic changes in notice templates, and to make them easy to use, the different attributes of the content object is accessible from the translated content object (ie. Koha::AdditionalContentsLocatlization->library is available and return $self->additional_content->library). I think it's an elegant way to keep things simple. - No changes expected for "NewsLog" logging - Little behaviour changes for pages, see tools/page.pl changes. We are now passing the id of the content, and the desired language, instead of the mix of "page_id" or code and lang. Note that here we certainly need to rename "language" query param to not change the full interface language. Test plan: 0. Preparation steps, use master a. Create notice templates that are using "<< additional_contents.code >>". This won't be replaced, but we want the update process to alert us. b. Create several news, additional contents, pages. Some with translated contents, some without. c. Make sure ISSUESLIP has the "<news>" section. If you are using the sample data there is nothing to do here d. Turn on NewsLogs 1. Apply the patches, restart_all, updatedatabase => Confirm that the new table is created and filled with the contents you had prior to the update => Confirm that additional_contents_localizations.updated_on has been kept to the previous values => Confirm that ISSUESLIP has been replaced properly => Confirm that you get a warning about the additional_contents 2. Create, update, delete news, html customs, pages => Confirm that the additional_contents_localizations.updated_on is only adjusted when required => Confirm that the logs are correctly created when NewsLogs is on 3. Check some items out, generate a slip => Confirm that the news are displayed at the bottom of the slip, and that the publication date is correctly formatted 4. Have several HTML customizations (like OpacNav, opacheader), in translated in different languages => Confirm that the default values is displayed when you are using the interface in a language without translation => Confirm that the translated version is picked when it exists Notes for QA: * I am not sure we really need the alert during the update DB process about the additional_contents leftover. We should not have them outside of ISSUESLIP. Shouldn't it hurt? * There is something ugly in sample_news.yml, the id is hardcoded. But how do we prevent that and keep translatability? Sponsored-by: Rijksmuseum, Netherlands
Created attachment 149814 [details] [review] Bug 31383: Adjust and add tests Sponsored-by: Rijksmuseum, Netherlands
Created attachment 149815 [details] [review] Bug 31383: DB changes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 149816 [details] [review] Bug 31383: DBIC changes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 149817 [details] [review] Bug 31383: DBIC specific Sponsored-by: Rijksmuseum, Netherlands
Created attachment 149818 [details] [review] Bug 31383: Restore updated_on for individual content Sponsored-by: Rijksmuseum, Netherlands
(In reply to Marcel de Rooy from comment #13) > Looking here. > > Rebased conflict with new WRAPPER in > koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt Done. (In reply to Marcel de Rooy from comment #14) > FAIL Koha/Schema/Result/AdditionalContentsLocalization.pm > FAIL pod coverage > POD is missing for koha_object_class > POD is missing for koha_objects_class We don't require POD for those subroutines. > FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt > FAIL valid_template > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: > DBD::mysql::st execute failed: Unknown column 'me.id' in 'field list' at > /usr/share/koha/Koha/Objects.pm line 421 > > FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-page.tt > FAIL valid_template > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: > DBD::mysql::st execute failed: Unknown column 'me.id' in 'field list' at > /usr/share/koha/Koha/Objects.pm line 421 Once you run the atomic update this will be fixed. (In reply to Marcel de Rooy from comment #15) > perl installer/data/mysql/updatedatabase.pl > DEV atomic update > /usr/share/koha/installer/data/mysql/atomicupdate/bug_31383.pl [13:36:56]: > Bug 31383 - Split the additional_contents table > Renamed additional_contents.idnew with 'id' > Added new table 'additional_contents_localizations' > Removed 'title', 'content', 'lang' columns from additional_contents > ERROR - {UNKNOWN}: Not a HASH reference at > /usr/share/koha/installer/data/mysql/atomicupdate/bug_31383.pl line 71. at > /usr/share/koha/C4/Installer.pm line 743 Fixed, sorry about that!
(In reply to Jonathan Druart from comment #22) > (In reply to Marcel de Rooy from comment #13) > > FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt > > FAIL valid_template > > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: > > DBD::mysql::st execute failed: Unknown column 'me.id' in 'field list' at > > /usr/share/koha/Koha/Objects.pm line 421 > > > > FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-page.tt > > FAIL valid_template > > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: > > DBD::mysql::st execute failed: Unknown column 'me.id' in 'field list' at > > /usr/share/koha/Koha/Objects.pm line 421 > > Once you run the atomic update this will be fixed. Note that they are still failing with an other error: DBIx::Class::Schema::source(): Can't find source for AdditionalContentsLocalization at /kohadevbox/koha/Koha/Objects.pm line 524 I think it's a false positive, it happens occasionally with valid_template. t/db_dependent/TestBuilder.t is passing so I think we are safe.
Created attachment 152423 [details] [review] Bug 31383: Create a parent-child DB relation for additional content In the design of additional contents the idea of a parent-child relation is implicitly present. You have a default page and translations. But we do this in one table coming from the old news items. Several reports show that we would be better off creating a parent table listing the main news items, CMS pages or HTML content. And a child table containing the title, content and lang. Note that this first step is a prelimenary step to clean this area and make it more robust and extensible. More enhancements to come. What is this patchset doing? * DB changes - Rename additional_contents.idnew with id - Create a new table additional_contents_localizations(id, additional_content_id, title, content, lang) that will contain the translated contents - Move the content to this new table - Remove title, content and lang columns from additional_contents - Replace the notice templates that are using ''<news>" (should only be ISSUESLIP) and remove support for this syntax. Also add a warning in case other occurrences of uses of the old syntax exist. * CRUD - We add a new Koha::AdditionalContentsLocalization[s] couple, and move some logic from Koha::AdditionalContent[s] to there. Note that, to prevent too much drastic changes in notice templates, and to make them easy to use, the different attributes of the content object is accessible from the translated content object (ie. Koha::AdditionalContentsLocatlization->library is available and return $self->additional_content->library). I think it's an elegant way to keep things simple. - No changes expected for "NewsLog" logging - Little behaviour changes for pages, see tools/page.pl changes. We are now passing the id of the content, and the desired language, instead of the mix of "page_id" or code and lang. Note that here we certainly need to rename "language" query param to not change the full interface language. Test plan: 0. Preparation steps, use master a. Create notice templates that are using "<< additional_contents.code >>". This won't be replaced, but we want the update process to alert us. b. Create several news, additional contents, pages. Some with translated contents, some without. c. Make sure ISSUESLIP has the "<news>" section. If you are using the sample data there is nothing to do here d. Turn on NewsLogs 1. Apply the patches, restart_all, updatedatabase => Confirm that the new table is created and filled with the contents you had prior to the update => Confirm that additional_contents_localizations.updated_on has been kept to the previous values => Confirm that ISSUESLIP has been replaced properly => Confirm that you get a warning about the additional_contents 2. Create, update, delete news, html customs, pages => Confirm that the additional_contents_localizations.updated_on is only adjusted when required => Confirm that the logs are correctly created when NewsLogs is on 3. Check some items out, generate a slip => Confirm that the news are displayed at the bottom of the slip, and that the publication date is correctly formatted 4. Have several HTML customizations (like OpacNav, opacheader), in translated in different languages => Confirm that the default values is displayed when you are using the interface in a language without translation => Confirm that the translated version is picked when it exists Notes for QA: * I am not sure we really need the alert during the update DB process about the additional_contents leftover. We should not have them outside of ISSUESLIP. Shouldn't it hurt? * There is something ugly in sample_news.yml, the id is hardcoded. But how do we prevent that and keep translatability? Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152424 [details] [review] Bug 31383: Adjust and add tests Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152425 [details] [review] Bug 31383: DB changes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152426 [details] [review] Bug 31383: DBIC changes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152427 [details] [review] Bug 31383: DBIC specific Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152428 [details] [review] Bug 31383: Restore updated_on for individual content Sponsored-by: Rijksmuseum, Netherlands
Patches rebased against master.
FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt FAIL valid_template DBIx::Class::Schema::source(): Can't find source for AdditionalContentsLocalization at /usr/share/koha/Koha/Objects.pm line 524 FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-page.tt FAIL valid_template DBIx::Class::Schema::source(): Can't find source for AdditionalContentsLocalization at /usr/share/koha/Koha/Objects.pm line 524 Can't explain this warning. Should expect a wrong pointer somewhere to Koha/Schema/Result but everything seems fine. Note: Already ran dbrev.
(In reply to Marcel de Rooy from comment #31) > FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt > FAIL valid_template > DBIx::Class::Schema::source(): Can't find source for > AdditionalContentsLocalization at /usr/share/koha/Koha/Objects.pm line 524 > > FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-page.tt > FAIL valid_template > DBIx::Class::Schema::source(): Can't find source for > AdditionalContentsLocalization at /usr/share/koha/Koha/Objects.pm line 524 > > Can't explain this warning. Should expect a wrong pointer somewhere to > Koha/Schema/Result but everything seems fine. > Note: Already ran dbrev. See comment 23.
Working here. Moving it to BLOCKED
Will add a follow-up for this bug. We need to check $lang before searching translated content. And which was very bad, we do not need to search for additional_content_id in the wrong column. diff --git a/Koha/AdditionalContents.pm b/Koha/AdditionalContents.pm index 0555ddc6d1..7d5e877d0f 100644 --- a/Koha/AdditionalContents.pm +++ b/Koha/AdditionalContents.pm @@ -85,9 +85,9 @@ sub search_for_display { my $contents = $self->SUPER::search( $search_params, { order_by => 'number' } ); my @all_content_id = $contents->get_column('id'); - my @translated_content_id; - if ( $params->{lang} ) { - my $translated_contents = Koha::AdditionalContentsLocalizations->search( + my ( $translated_contents, @translated_content_id ); + if ( $params->{lang} && $params->{lang} ne 'default' ) { + $translated_contents = Koha::AdditionalContentsLocalizations->search( { additional_content_id => [$contents->get_column('id')], lang => $params->{lang}, @@ -105,7 +105,7 @@ sub search_for_display { return Koha::AdditionalContentsLocalizations->search( { - id => [@translated_content_id, $default_contents->get_column('id')] + id => [ $translated_contents ? $translated_contents->get_column('id') : (), $default_contents->get_column('id') ] }, );
I might have one query for the whole thing in search_for_display instead of four. But I am not sure if we really gain performance.
Created attachment 152545 [details] [review] Bug 31383: (QA follow-up) Do not return 0 in ->get The last statement evaluated is the if( $content->count ). If that evaluates to false, the false expression value is returned. In this case that is 0. We should just return undef or empty list (scalar/list context).
Created attachment 152546 [details] [review] Bug 31383: Check lang and use right column in search_for_display We should check if $lang equals default before looking for translated contents. And note that @translated_content_id contains additional_content_id not id, so cannot be used in the last query.
Created attachment 152547 [details] [review] Bug 31383: Use one query in search_for_display instead of four Benchmarking does however not show real performance gain. But it might be considered as a code improvement?
What do you think of the last patch, Jonathan? We are actually cheating in search_for_display since we return localizations in the main object? Should we move it?
Created attachment 152566 [details] [review] Bug 31383: Fix URLs on staff AC detail view No real need anymore for showing a Default and Current language link. They could be made with the language parameter (not lang), but just changing the language allows you to test easily. Would you really like to see Default when you added content for all enabled languages? You can still add language=default to the URL. Test plan: Test the page URLs. Experiment with changing language in the regular way and via URL parameter.
Main thing I still want to do, is test the dbrev a bit better than I did at first.
(In reply to Marcel de Rooy from comment #41) > Main thing I still want to do, is test the dbrev a bit better than I did at > first. First test crashes: Added new table 'additional_contents_localizations' ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate entry 'pages-staff_only_7-CPL' for key 'additional_contents_uniq' at /usr/share/koha/C4/Installer.pm line 741
(In reply to Marcel de Rooy from comment #42) > (In reply to Marcel de Rooy from comment #41) > > Main thing I still want to do, is test the dbrev a bit better than I did at > > first. > > First test crashes: > Added new table 'additional_contents_localizations' > ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate entry > 'pages-staff_only_7-CPL' for key 'additional_contents_uniq' at > /usr/share/koha/C4/Installer.pm line 741 I would suggest to put the logic in a txn_do because I ended up now with a half conversion of data.
This was my add contents test table: | news | News_1 | NULL | | news | News_2 | NULL | | html_customizations | CatalogConcernHelp_1 | NULL | | html_customizations | CatalogConcernTemplate_1 | NULL | | pages | opac_only_5 | NULL | | pages | opac_only_5 | NULL | | pages | staff_only_7 | CPL | | pages | staff_only_7 | CPL | | pages | staff_only_7 | CPL | | news | News_1 | NULL | | news | News_2 | NULL | | news | News_2 | NULL | | news | News_13 | CPL | | news | News_13 | CPL | | html_customizations | CatalogConcernTemplate_1 | NULL | After all contents has been copied to localization, you try to add UNIQUE constraint on the above data. Which obviously does not work. Since you did not delete the double entries in additional_contents.
Created attachment 152660 [details] [review] Bug 31383: Create a parent-child DB relation for additional content In the design of additional contents the idea of a parent-child relation is implicitly present. You have a default page and translations. But we do this in one table coming from the old news items. Several reports show that we would be better off creating a parent table listing the main news items, CMS pages or HTML content. And a child table containing the title, content and lang. Note that this first step is a prelimenary step to clean this area and make it more robust and extensible. More enhancements to come. What is this patchset doing? * DB changes - Rename additional_contents.idnew with id - Create a new table additional_contents_localizations(id, additional_content_id, title, content, lang) that will contain the translated contents - Move the content to this new table - Remove title, content and lang columns from additional_contents - Replace the notice templates that are using ''<news>" (should only be ISSUESLIP) and remove support for this syntax. Also add a warning in case other occurrences of uses of the old syntax exist. * CRUD - We add a new Koha::AdditionalContentsLocalization[s] couple, and move some logic from Koha::AdditionalContent[s] to there. Note that, to prevent too much drastic changes in notice templates, and to make them easy to use, the different attributes of the content object is accessible from the translated content object (ie. Koha::AdditionalContentsLocatlization->library is available and return $self->additional_content->library). I think it's an elegant way to keep things simple. - No changes expected for "NewsLog" logging - Little behaviour changes for pages, see tools/page.pl changes. We are now passing the id of the content, and the desired language, instead of the mix of "page_id" or code and lang. Note that here we certainly need to rename "language" query param to not change the full interface language. Test plan: 0. Preparation steps, use master a. Create notice templates that are using "<< additional_contents.code >>". This won't be replaced, but we want the update process to alert us. b. Create several news, additional contents, pages. Some with translated contents, some without. c. Make sure ISSUESLIP has the "<news>" section. If you are using the sample data there is nothing to do here d. Turn on NewsLogs 1. Apply the patches, restart_all, updatedatabase => Confirm that the new table is created and filled with the contents you had prior to the update => Confirm that additional_contents_localizations.updated_on has been kept to the previous values => Confirm that ISSUESLIP has been replaced properly => Confirm that you get a warning about the additional_contents 2. Create, update, delete news, html customs, pages => Confirm that the additional_contents_localizations.updated_on is only adjusted when required => Confirm that the logs are correctly created when NewsLogs is on 3. Check some items out, generate a slip => Confirm that the news are displayed at the bottom of the slip, and that the publication date is correctly formatted 4. Have several HTML customizations (like OpacNav, opacheader), in translated in different languages => Confirm that the default values is displayed when you are using the interface in a language without translation => Confirm that the translated version is picked when it exists Notes for QA: * I am not sure we really need the alert during the update DB process about the additional_contents leftover. We should not have them outside of ISSUESLIP. Shouldn't it hurt? * There is something ugly in sample_news.yml, the id is hardcoded. But how do we prevent that and keep translatability? Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152661 [details] [review] Bug 31383: Adjust and add tests Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152662 [details] [review] Bug 31383: DB changes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152663 [details] [review] Bug 31383: DBIC changes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152664 [details] [review] Bug 31383: DBIC specific Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152665 [details] [review] Bug 31383: Restore updated_on for individual content Sponsored-by: Rijksmuseum, Netherlands
Created attachment 152666 [details] [review] Bug 31383: (QA follow-up) Do not return 0 in ->get The last statement evaluated is the if( $content->count ). If that evaluates to false, the false expression value is returned. In this case that is 0. We should just return undef or empty list (scalar/list context).
Created attachment 152667 [details] [review] Bug 31383: Check lang and use right column in search_for_display We should check if $lang equals default before looking for translated contents. And note that @translated_content_id contains additional_content_id not id, so cannot be used in the last query.
Created attachment 152668 [details] [review] Bug 31383: Use one query in search_for_display instead of four Benchmarking does however not show real performance gain. But it might be considered as a code improvement?
Created attachment 152669 [details] [review] Bug 31383: Fix URLs on staff AC detail view No real need anymore for showing a Default and Current language link. They could be made with the language parameter (not lang), but just changing the language allows you to test easily. Would you really like to see Default when you added content for all enabled languages? You can still add language=default to the URL. Test plan: Test the page URLs. Experiment with changing language in the regular way and via URL parameter.
Rebased, few follow-ups. Dbrev needs attention.
(In reply to Marcel de Rooy from comment #53) > Created attachment 152668 [details] [review] [review] > Bug 31383: Use one query in search_for_display instead of four > > Benchmarking does however not show real performance gain. > But it might be considered as a code improvement? I am expecting faster results without this patch. If we want to keep it we need to remove the following statement: use Array::Utils qw( array_minus ); (In reply to Marcel de Rooy from comment #54) > Created attachment 152669 [details] [review] [review] > Bug 31383: Fix URLs on staff AC detail view IMO we should move this to its own bug report as it's a behaviour change.
Created attachment 153013 [details] [review] Bug 31383: Fix dbrevs We need to remove duplicate entries.
(In reply to Marcel de Rooy from comment #44) > This was my add contents test table: > > | news | News_1 | NULL | > | news | News_2 | NULL | > | html_customizations | CatalogConcernHelp_1 | NULL | > | html_customizations | CatalogConcernTemplate_1 | NULL | > | pages | opac_only_5 | NULL | > | pages | opac_only_5 | NULL | > | pages | staff_only_7 | CPL | > | pages | staff_only_7 | CPL | > | pages | staff_only_7 | CPL | > | news | News_1 | NULL | > | news | News_2 | NULL | > | news | News_2 | NULL | > | news | News_13 | CPL | > | news | News_13 | CPL | > | html_customizations | CatalogConcernTemplate_1 | NULL | > > After all contents has been copied to localization, you try to add UNIQUE > constraint on the above data. Which obviously does not work. Since you did > not delete the double entries in additional_contents. Should be fixed in the last patch. Didn't catch it because I was no using a specific branchcode I guess (NULL). (In reply to Marcel de Rooy from comment #43) > (In reply to Marcel de Rooy from comment #42) > > (In reply to Marcel de Rooy from comment #41) > > > Main thing I still want to do, is test the dbrev a bit better than I did at > > > first. > > > > First test crashes: > > Added new table 'additional_contents_localizations' > > ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate entry > > 'pages-staff_only_7-CPL' for key 'additional_contents_uniq' at > > /usr/share/koha/C4/Installer.pm line 741 > > I would suggest to put the logic in a txn_do because I ended up now with a > half conversion of data. We run dbrevs in a transaction already but the DBMS will implicitly commit for CREATE/ALTER/DROP/etc statements (yes, that's bad).
dbrev does no longer crash, still checking results DEV atomic update /usr/share/koha/installer/data/mysql/atomicupdate/bug_31383.pl [14:29:24]: Bug 31383 - Split the additional_contents table Renamed additional_contents.idnew with 'id' Added new table 'additional_contents_localizations' Removed 'title', 'content', 'lang' columns from additional_contents Adjusted notice template 'ISSUESLIP'
Needed small rebase on fa pencil stuff
Created attachment 153666 [details] [review] Bug 31383: Create a parent-child DB relation for additional content In the design of additional contents the idea of a parent-child relation is implicitly present. You have a default page and translations. But we do this in one table coming from the old news items. Several reports show that we would be better off creating a parent table listing the main news items, CMS pages or HTML content. And a child table containing the title, content and lang. Note that this first step is a prelimenary step to clean this area and make it more robust and extensible. More enhancements to come. What is this patchset doing? * DB changes - Rename additional_contents.idnew with id - Create a new table additional_contents_localizations(id, additional_content_id, title, content, lang) that will contain the translated contents - Move the content to this new table - Remove title, content and lang columns from additional_contents - Replace the notice templates that are using ''<news>" (should only be ISSUESLIP) and remove support for this syntax. Also add a warning in case other occurrences of uses of the old syntax exist. * CRUD - We add a new Koha::AdditionalContentsLocalization[s] couple, and move some logic from Koha::AdditionalContent[s] to there. Note that, to prevent too much drastic changes in notice templates, and to make them easy to use, the different attributes of the content object is accessible from the translated content object (ie. Koha::AdditionalContentsLocatlization->library is available and return $self->additional_content->library). I think it's an elegant way to keep things simple. - No changes expected for "NewsLog" logging - Little behaviour changes for pages, see tools/page.pl changes. We are now passing the id of the content, and the desired language, instead of the mix of "page_id" or code and lang. Note that here we certainly need to rename "language" query param to not change the full interface language. Test plan: 0. Preparation steps, use master a. Create notice templates that are using "<< additional_contents.code >>". This won't be replaced, but we want the update process to alert us. b. Create several news, additional contents, pages. Some with translated contents, some without. c. Make sure ISSUESLIP has the "<news>" section. If you are using the sample data there is nothing to do here d. Turn on NewsLogs 1. Apply the patches, restart_all, updatedatabase => Confirm that the new table is created and filled with the contents you had prior to the update => Confirm that additional_contents_localizations.updated_on has been kept to the previous values => Confirm that ISSUESLIP has been replaced properly => Confirm that you get a warning about the additional_contents 2. Create, update, delete news, html customs, pages => Confirm that the additional_contents_localizations.updated_on is only adjusted when required => Confirm that the logs are correctly created when NewsLogs is on 3. Check some items out, generate a slip => Confirm that the news are displayed at the bottom of the slip, and that the publication date is correctly formatted 4. Have several HTML customizations (like OpacNav, opacheader), in translated in different languages => Confirm that the default values is displayed when you are using the interface in a language without translation => Confirm that the translated version is picked when it exists Notes for QA: * I am not sure we really need the alert during the update DB process about the additional_contents leftover. We should not have them outside of ISSUESLIP. Shouldn't it hurt? * There is something ugly in sample_news.yml, the id is hardcoded. But how do we prevent that and keep translatability? Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153667 [details] [review] Bug 31383: Adjust and add tests Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153668 [details] [review] Bug 31383: DB changes Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153669 [details] [review] Bug 31383: DBIC changes Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153670 [details] [review] Bug 31383: DBIC specific Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153671 [details] [review] Bug 31383: Restore updated_on for individual content Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153672 [details] [review] Bug 31383: (QA follow-up) Do not return 0 in ->get The last statement evaluated is the if( $content->count ). If that evaluates to false, the false expression value is returned. In this case that is 0. We should just return undef or empty list (scalar/list context). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153673 [details] [review] Bug 31383: Check lang and use right column in search_for_display We should check if $lang equals default before looking for translated contents. And note that @translated_content_id contains additional_content_id not id, so cannot be used in the last query. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153674 [details] [review] Bug 31383: Use one query in search_for_display instead of four Benchmarking does however not show real performance gain. But it might be considered as a code improvement? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153675 [details] [review] Bug 31383: Fix URLs on staff AC detail view No real need anymore for showing a Default and Current language link. They could be made with the language parameter (not lang), but just changing the language allows you to test easily. Would you really like to see Default when you added content for all enabled languages? You can still add language=default to the URL. Test plan: Test the page URLs. Experiment with changing language in the regular way and via URL parameter. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153676 [details] [review] Bug 31383: Fix dbrevs We need to remove duplicate entries. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156048 [details] [review] Bug 31383: Create a parent-child DB relation for additional content In the design of additional contents the idea of a parent-child relation is implicitly present. You have a default page and translations. But we do this in one table coming from the old news items. Several reports show that we would be better off creating a parent table listing the main news items, CMS pages or HTML content. And a child table containing the title, content and lang. Note that this first step is a prelimenary step to clean this area and make it more robust and extensible. More enhancements to come. What is this patchset doing? * DB changes - Rename additional_contents.idnew with id - Create a new table additional_contents_localizations(id, additional_content_id, title, content, lang) that will contain the translated contents - Move the content to this new table - Remove title, content and lang columns from additional_contents - Replace the notice templates that are using ''<news>" (should only be ISSUESLIP) and remove support for this syntax. Also add a warning in case other occurrences of uses of the old syntax exist. * CRUD - We add a new Koha::AdditionalContentsLocalization[s] couple, and move some logic from Koha::AdditionalContent[s] to there. Note that, to prevent too much drastic changes in notice templates, and to make them easy to use, the different attributes of the content object is accessible from the translated content object (ie. Koha::AdditionalContentsLocatlization->library is available and return $self->additional_content->library). I think it's an elegant way to keep things simple. - No changes expected for "NewsLog" logging - Little behaviour changes for pages, see tools/page.pl changes. We are now passing the id of the content, and the desired language, instead of the mix of "page_id" or code and lang. Note that here we certainly need to rename "language" query param to not change the full interface language. Test plan: 0. Preparation steps, use master a. Create notice templates that are using "<< additional_contents.code >>". This won't be replaced, but we want the update process to alert us. b. Create several news, additional contents, pages. Some with translated contents, some without. c. Make sure ISSUESLIP has the "<news>" section. If you are using the sample data there is nothing to do here d. Turn on NewsLogs 1. Apply the patches, restart_all, updatedatabase => Confirm that the new table is created and filled with the contents you had prior to the update => Confirm that additional_contents_localizations.updated_on has been kept to the previous values => Confirm that ISSUESLIP has been replaced properly => Confirm that you get a warning about the additional_contents 2. Create, update, delete news, html customs, pages => Confirm that the additional_contents_localizations.updated_on is only adjusted when required => Confirm that the logs are correctly created when NewsLogs is on 3. Check some items out, generate a slip => Confirm that the news are displayed at the bottom of the slip, and that the publication date is correctly formatted 4. Have several HTML customizations (like OpacNav, opacheader), in translated in different languages => Confirm that the default values is displayed when you are using the interface in a language without translation => Confirm that the translated version is picked when it exists Notes for QA: * I am not sure we really need the alert during the update DB process about the additional_contents leftover. We should not have them outside of ISSUESLIP. Shouldn't it hurt? * There is something ugly in sample_news.yml, the id is hardcoded. But how do we prevent that and keep translatability? Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156049 [details] [review] Bug 31383: Adjust and add tests Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156050 [details] [review] Bug 31383: DB changes Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156051 [details] [review] Bug 31383: DBIC changes Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156052 [details] [review] Bug 31383: DBIC specific Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156053 [details] [review] Bug 31383: Restore updated_on for individual content Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156054 [details] [review] Bug 31383: (QA follow-up) Do not return 0 in ->get The last statement evaluated is the if( $content->count ). If that evaluates to false, the false expression value is returned. In this case that is 0. We should just return undef or empty list (scalar/list context). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156055 [details] [review] Bug 31383: Check lang and use right column in search_for_display We should check if $lang equals default before looking for translated contents. And note that @translated_content_id contains additional_content_id not id, so cannot be used in the last query. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156056 [details] [review] Bug 31383: Use one query in search_for_display instead of four Benchmarking does however not show real performance gain. But it might be considered as a code improvement? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156057 [details] [review] Bug 31383: Fix URLs on staff AC detail view No real need anymore for showing a Default and Current language link. They could be made with the language parameter (not lang), but just changing the language allows you to test easily. Would you really like to see Default when you added content for all enabled languages? You can still add language=default to the URL. Test plan: Test the page URLs. Experiment with changing language in the regular way and via URL parameter. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 156058 [details] [review] Bug 31383: Fix dbrevs We need to remove duplicate entries. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Trivial rebase.
Generally, this all looks like an improvement.. but the QA script throws up something a bit odd that I've not got to the bottom of yet. In the valid template tests I get a couple of occurrences of `DBIx::Class::Schema::source(): Can't find source for AdditionalContentsLocalization at /kohadevbox/koha/Koha/Objects.pm line 524` and a subsequent test fail.
(In reply to Martin Renvoize from comment #84) > Generally, this all looks like an improvement.. but the QA script throws up > something a bit odd that I've not got to the bottom of yet. > > In the valid template tests I get a couple of occurrences of > `DBIx::Class::Schema::source(): Can't find source for > AdditionalContentsLocalization at /kohadevbox/koha/Koha/Objects.pm line 524` > and a subsequent test fail. See comment 23 :)
Awesome, thanks Jonathan, if missed that comment. This all seems good to me, I could find any regressions. I'll upload the designed patches in the morning and amend to PQA. Thanks for the with here
Created attachment 156265 [details] [review] Bug 31383: Create a parent-child DB relation for additional content In the design of additional contents the idea of a parent-child relation is implicitly present. You have a default page and translations. But we do this in one table coming from the old news items. Several reports show that we would be better off creating a parent table listing the main news items, CMS pages or HTML content. And a child table containing the title, content and lang. Note that this first step is a prelimenary step to clean this area and make it more robust and extensible. More enhancements to come. What is this patchset doing? * DB changes - Rename additional_contents.idnew with id - Create a new table additional_contents_localizations(id, additional_content_id, title, content, lang) that will contain the translated contents - Move the content to this new table - Remove title, content and lang columns from additional_contents - Replace the notice templates that are using ''<news>" (should only be ISSUESLIP) and remove support for this syntax. Also add a warning in case other occurrences of uses of the old syntax exist. * CRUD - We add a new Koha::AdditionalContentsLocalization[s] couple, and move some logic from Koha::AdditionalContent[s] to there. Note that, to prevent too much drastic changes in notice templates, and to make them easy to use, the different attributes of the content object is accessible from the translated content object (ie. Koha::AdditionalContentsLocatlization->library is available and return $self->additional_content->library). I think it's an elegant way to keep things simple. - No changes expected for "NewsLog" logging - Little behaviour changes for pages, see tools/page.pl changes. We are now passing the id of the content, and the desired language, instead of the mix of "page_id" or code and lang. Note that here we certainly need to rename "language" query param to not change the full interface language. Test plan: 0. Preparation steps, use master a. Create notice templates that are using "<< additional_contents.code >>". This won't be replaced, but we want the update process to alert us. b. Create several news, additional contents, pages. Some with translated contents, some without. c. Make sure ISSUESLIP has the "<news>" section. If you are using the sample data there is nothing to do here d. Turn on NewsLogs 1. Apply the patches, restart_all, updatedatabase => Confirm that the new table is created and filled with the contents you had prior to the update => Confirm that additional_contents_localizations.updated_on has been kept to the previous values => Confirm that ISSUESLIP has been replaced properly => Confirm that you get a warning about the additional_contents 2. Create, update, delete news, html customs, pages => Confirm that the additional_contents_localizations.updated_on is only adjusted when required => Confirm that the logs are correctly created when NewsLogs is on 3. Check some items out, generate a slip => Confirm that the news are displayed at the bottom of the slip, and that the publication date is correctly formatted 4. Have several HTML customizations (like OpacNav, opacheader), in translated in different languages => Confirm that the default values is displayed when you are using the interface in a language without translation => Confirm that the translated version is picked when it exists Notes for QA: * I am not sure we really need the alert during the update DB process about the additional_contents leftover. We should not have them outside of ISSUESLIP. Shouldn't it hurt? * There is something ugly in sample_news.yml, the id is hardcoded. But how do we prevent that and keep translatability? Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156266 [details] [review] Bug 31383: Adjust and add tests Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156267 [details] [review] Bug 31383: DB changes Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156268 [details] [review] Bug 31383: DBIC changes Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156269 [details] [review] Bug 31383: DBIC specific Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156270 [details] [review] Bug 31383: Restore updated_on for individual content Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156271 [details] [review] Bug 31383: (QA follow-up) Do not return 0 in ->get The last statement evaluated is the if( $content->count ). If that evaluates to false, the false expression value is returned. In this case that is 0. We should just return undef or empty list (scalar/list context). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156272 [details] [review] Bug 31383: Check lang and use right column in search_for_display We should check if $lang equals default before looking for translated contents. And note that @translated_content_id contains additional_content_id not id, so cannot be used in the last query. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156273 [details] [review] Bug 31383: Use one query in search_for_display instead of four Benchmarking does however not show real performance gain. But it might be considered as a code improvement? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156274 [details] [review] Bug 31383: Fix URLs on staff AC detail view No real need anymore for showing a Default and Current language link. They could be made with the language parameter (not lang), but just changing the language allows you to test easily. Would you really like to see Default when you added content for all enabled languages? You can still add language=default to the URL. Test plan: Test the page URLs. Experiment with changing language in the regular way and via URL parameter. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156275 [details] [review] Bug 31383: Fix dbrevs We need to remove duplicate entries. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156276 [details] [review] Bug 31383: (QA follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
No regressions found, QA scripts pretty much happy and this is a solid improvement in our foundations, hopefully meaning we can move forward with further enhancements to this area of Koha. Great work all involved, thank you. Passing QA
Small rebase required. Please do ASAP.
Created attachment 157539 [details] [review] Bug 31383: Create a parent-child DB relation for additional content In the design of additional contents the idea of a parent-child relation is implicitly present. You have a default page and translations. But we do this in one table coming from the old news items. Several reports show that we would be better off creating a parent table listing the main news items, CMS pages or HTML content. And a child table containing the title, content and lang. Note that this first step is a prelimenary step to clean this area and make it more robust and extensible. More enhancements to come. What is this patchset doing? * DB changes - Rename additional_contents.idnew with id - Create a new table additional_contents_localizations(id, additional_content_id, title, content, lang) that will contain the translated contents - Move the content to this new table - Remove title, content and lang columns from additional_contents - Replace the notice templates that are using ''<news>" (should only be ISSUESLIP) and remove support for this syntax. Also add a warning in case other occurrences of uses of the old syntax exist. * CRUD - We add a new Koha::AdditionalContentsLocalization[s] couple, and move some logic from Koha::AdditionalContent[s] to there. Note that, to prevent too much drastic changes in notice templates, and to make them easy to use, the different attributes of the content object is accessible from the translated content object (ie. Koha::AdditionalContentsLocatlization->library is available and return $self->additional_content->library). I think it's an elegant way to keep things simple. - No changes expected for "NewsLog" logging - Little behaviour changes for pages, see tools/page.pl changes. We are now passing the id of the content, and the desired language, instead of the mix of "page_id" or code and lang. Note that here we certainly need to rename "language" query param to not change the full interface language. Test plan: 0. Preparation steps, use master a. Create notice templates that are using "<< additional_contents.code >>". This won't be replaced, but we want the update process to alert us. b. Create several news, additional contents, pages. Some with translated contents, some without. c. Make sure ISSUESLIP has the "<news>" section. If you are using the sample data there is nothing to do here d. Turn on NewsLogs 1. Apply the patches, restart_all, updatedatabase => Confirm that the new table is created and filled with the contents you had prior to the update => Confirm that additional_contents_localizations.updated_on has been kept to the previous values => Confirm that ISSUESLIP has been replaced properly => Confirm that you get a warning about the additional_contents 2. Create, update, delete news, html customs, pages => Confirm that the additional_contents_localizations.updated_on is only adjusted when required => Confirm that the logs are correctly created when NewsLogs is on 3. Check some items out, generate a slip => Confirm that the news are displayed at the bottom of the slip, and that the publication date is correctly formatted 4. Have several HTML customizations (like OpacNav, opacheader), in translated in different languages => Confirm that the default values is displayed when you are using the interface in a language without translation => Confirm that the translated version is picked when it exists Notes for QA: * I am not sure we really need the alert during the update DB process about the additional_contents leftover. We should not have them outside of ISSUESLIP. Shouldn't it hurt? * There is something ugly in sample_news.yml, the id is hardcoded. But how do we prevent that and keep translatability? Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157540 [details] [review] Bug 31383: Adjust and add tests Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157541 [details] [review] Bug 31383: DB changes Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157542 [details] [review] Bug 31383: DBIC changes Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157543 [details] [review] Bug 31383: DBIC specific Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157544 [details] [review] Bug 31383: Restore updated_on for individual content Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157545 [details] [review] Bug 31383: (QA follow-up) Do not return 0 in ->get The last statement evaluated is the if( $content->count ). If that evaluates to false, the false expression value is returned. In this case that is 0. We should just return undef or empty list (scalar/list context). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157546 [details] [review] Bug 31383: Check lang and use right column in search_for_display We should check if $lang equals default before looking for translated contents. And note that @translated_content_id contains additional_content_id not id, so cannot be used in the last query. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157547 [details] [review] Bug 31383: Use one query in search_for_display instead of four Benchmarking does however not show real performance gain. But it might be considered as a code improvement? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157548 [details] [review] Bug 31383: Fix URLs on staff AC detail view No real need anymore for showing a Default and Current language link. They could be made with the language parameter (not lang), but just changing the language allows you to test easily. Would you really like to see Default when you added content for all enabled languages? You can still add language=default to the URL. Test plan: Test the page URLs. Experiment with changing language in the regular way and via URL parameter. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157549 [details] [review] Bug 31383: Fix dbrevs We need to remove duplicate entries. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157550 [details] [review] Bug 31383: (QA follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pushed to master for 23.11. Nice work everyone, thanks!
This breaks Koha_Master, which tries to re-apply all the updates in an attempt to test db_revs to be idempotent. https://jenkins.koha-community.org/job/Koha_Master/2692/console We could add a check for the existence of the table, and use different queries. Do you agree? Is it worth the trouble?
Created attachment 157580 [details] [review] Bug 31383: Fix kohastructure.sql Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi from comment #114) > This breaks Koha_Master, which tries to re-apply all the updates in an > attempt to test db_revs to be idempotent. > > https://jenkins.koha-community.org/job/Koha_Master/2692/console > > We could add a check for the existence of the table, and use different > queries. Do you agree? Is it worth the trouble? No, I don't think so. I am about to provide a patch to make the test passes.
Created attachment 157585 [details] [review] Bug 31383: Fix the DB upgrade process This patch fixes misc4dev/run_tests.pl --run-db-upgrade-only We need to make sure the column exists before running the query.
(In reply to Jonathan Druart from comment #117) > Created attachment 157585 [details] [review] [review] > Bug 31383: Fix the DB upgrade process > > This patch fixes misc4dev/run_tests.pl --run-db-upgrade-only > > We need to make sure the column exists before running the query. Patch pushed to master.
Created attachment 157640 [details] [review] Bug 31383: (bug 29691 follow-up) Remove get_opac_news_by_id We can simply use get here
Nick, could you have a look at this patch please? Would it work for you (as the author of bug 29691)?
I'm getting an error from updatedatabase: ERROR - {UNKNOWN}: Not a HASH reference at /kohadevbox/koha/installer/data/mysql/db_revs/230600041.pl line 130. at /kohadevbox/koha/C4/Installer.pm line 741
Did not see it myself: Upgrade to 23.06.00.041 [11:42:55]: Bug 31383 - Split the additional_contents table Renamed additional_contents.idnew with 'id' Added new table 'additional_contents_localizations' Removed 'title', 'content', 'lang' columns from additional_contents Adjusted notice template 'ISSUESLIP'
If I delete any notices which have a branchcode from the letters table before I run the database update the update runs without error.
(In reply to Owen Leonard from comment #123) > If I delete any notices which have a branchcode from the letters table > before I run the database update the update runs without error. So, before upgrade, we need to add a couple branch-specific news.
This should not work either I guess? sub get_opac_news_by_id { my ( $self, $params ) = @_; my $news_id = $params->{news_id}; my $content = Koha::AdditionalContents->search( { location => ['opac_only', 'staff_and_opac'], idnew => $news_id, category => 'news', } ); because of `idnew`. Used in the OPAC's main page: $ git grep get_opac_news_by_id opac-tmpl/bootstrap/en/modules/opac-main.tt: [% SET koha_news = AdditionalContents.get_opac_news_by_id( news_id => news_id ) %]
(In reply to Owen Leonard from comment #123) > If I delete any notices which have a branchcode from the letters table > before I run the database update the update runs without error. I think I was testing this incorrectly, so ignore me.
(In reply to Tomás Cohen Arazi from comment #125) > This should not work either I guess? > > sub get_opac_news_by_id { > my ( $self, $params ) = @_; > > my $news_id = $params->{news_id}; > > my $content = Koha::AdditionalContents->search( > { > location => ['opac_only', 'staff_and_opac'], > idnew => $news_id, > category => 'news', > } > ); > > because of `idnew`. > > Used in the OPAC's main page: > $ git grep get_opac_news_by_id > opac-tmpl/bootstrap/en/modules/opac-main.tt: [% SET koha_news = > AdditionalContents.get_opac_news_by_id( news_id => news_id ) %] Please see last latch!
(In reply to Jonathan Druart from comment #119) > Created attachment 157640 [details] [review] [review] > Bug 31383: (bug 29691 follow-up) Remove get_opac_news_by_id > > We can simply use get here Pushed to master. Thanks!
Created attachment 157741 [details] [review] Bug 31383: (follow-up) Remove get_opac_news_by_id Keep the same behaviour, but wondering why we don't return 'content' when there is no match. ie. + is( $additional_contents, undef ); should certainly be + is( $additional_contents->{content}->count, 0 );
Please wait for Nick's comment this time ;)
Created attachment 157742 [details] [review] Bug 31383: (follow-up) Remove get_opac_news_by_id Keep the same behaviour, but wondering why we don't return 'content' when there is no match. ie. + is( $additional_contents, undef ); should certainly be + is( $additional_contents->{content}->count, 0 ); WNC amended patch - tidied Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Jonathan Druart from comment #130) > Please wait for Nick's comment this time ;) Please mark it as BLOCKED in such situations. Sometimes I already went through a bug and already have it on my local branch for a day or two. It would be better to have the koha-push script complain about wrong status on the bug instead of having to revisit if there are new comments on each already pushed bug.