Bug 31383 - Additional contents: We need a parent and child table
Summary: Additional contents: We need a parent and child table
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 32245
Blocks: 35151 35152 35153 35154 34869 34889 34894 35048 35063 35065 35136
  Show dependency treegraph
 
Reported: 2022-08-17 11:59 UTC by Marcel de Rooy
Modified: 2023-11-02 12:38 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 31383: Create a parent-child DB relation for additional content (51.98 KB, patch)
2023-03-08 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Adjust and add tests (18.33 KB, patch)
2023-03-08 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DB changes (15.08 KB, patch)
2023-03-08 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DBIC changes (7.36 KB, patch)
2023-03-08 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DBIC specific (1.08 KB, patch)
2023-03-08 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Restore updated_on for individual content (8.08 KB, patch)
2023-03-08 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Create a parent-child DB relation for additional content (51.90 KB, patch)
2023-04-18 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Adjust and add tests (18.33 KB, patch)
2023-04-18 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DB changes (15.10 KB, patch)
2023-04-18 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DBIC changes (7.36 KB, patch)
2023-04-18 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DBIC specific (1.08 KB, patch)
2023-04-18 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Restore updated_on for individual content (8.08 KB, patch)
2023-04-18 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Create a parent-child DB relation for additional content (51.58 KB, patch)
2023-06-16 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Adjust and add tests (18.33 KB, patch)
2023-06-16 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DB changes (15.10 KB, patch)
2023-06-16 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DBIC changes (7.36 KB, patch)
2023-06-16 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DBIC specific (1.08 KB, patch)
2023-06-16 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Restore updated_on for individual content (8.08 KB, patch)
2023-06-16 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: (QA follow-up) Do not return 0 in ->get (945 bytes, patch)
2023-06-22 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Check lang and use right column in search_for_display (1.71 KB, patch)
2023-06-22 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Use one query in search_for_display instead of four (3.61 KB, patch)
2023-06-22 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Fix URLs on staff AC detail view (3.09 KB, patch)
2023-06-22 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Create a parent-child DB relation for additional content (51.61 KB, patch)
2023-06-26 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Adjust and add tests (18.36 KB, patch)
2023-06-26 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: DB changes (15.13 KB, patch)
2023-06-26 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: DBIC changes (7.39 KB, patch)
2023-06-26 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: DBIC specific (1.12 KB, patch)
2023-06-26 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Restore updated_on for individual content (8.10 KB, patch)
2023-06-26 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: (QA follow-up) Do not return 0 in ->get (945 bytes, patch)
2023-06-26 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Check lang and use right column in search_for_display (1.71 KB, patch)
2023-06-26 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Use one query in search_for_display instead of four (3.61 KB, patch)
2023-06-26 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Fix URLs on staff AC detail view (3.09 KB, patch)
2023-06-26 08:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Fix dbrevs (1.39 KB, patch)
2023-07-04 11:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Create a parent-child DB relation for additional content (51.73 KB, patch)
2023-07-19 14:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Adjust and add tests (18.42 KB, patch)
2023-07-19 14:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: DB changes (15.18 KB, patch)
2023-07-19 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: DBIC changes (7.45 KB, patch)
2023-07-19 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: DBIC specific (1.17 KB, patch)
2023-07-19 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Restore updated_on for individual content (8.16 KB, patch)
2023-07-19 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: (QA follow-up) Do not return 0 in ->get (1003 bytes, patch)
2023-07-19 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Check lang and use right column in search_for_display (1.77 KB, patch)
2023-07-19 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Use one query in search_for_display instead of four (3.66 KB, patch)
2023-07-19 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Fix URLs on staff AC detail view (3.15 KB, patch)
2023-07-19 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Fix dbrevs (1.48 KB, patch)
2023-07-19 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31383: Create a parent-child DB relation for additional content (51.79 KB, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Adjust and add tests (18.39 KB, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DB changes (15.15 KB, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DBIC changes (7.41 KB, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DBIC specific (1.14 KB, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Restore updated_on for individual content (8.13 KB, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: (QA follow-up) Do not return 0 in ->get (965 bytes, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Check lang and use right column in search_for_display (1.73 KB, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Use one query in search_for_display instead of four (3.62 KB, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Fix URLs on staff AC detail view (3.11 KB, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Fix dbrevs (1.44 KB, patch)
2023-09-22 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Create a parent-child DB relation for additional content (52.33 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: Adjust and add tests (18.45 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: DB changes (15.21 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: DBIC changes (7.48 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: DBIC specific (1.20 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: Restore updated_on for individual content (8.19 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: (QA follow-up) Do not return 0 in ->get (1.01 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: Check lang and use right column in search_for_display (1.80 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: Use one query in search_for_display instead of four (3.69 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: Fix URLs on staff AC detail view (3.17 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: Fix dbrevs (1.51 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: (QA follow-up) Tidy (20.04 KB, patch)
2023-09-27 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31383: Create a parent-child DB relation for additional content (52.24 KB, patch)
2023-10-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Adjust and add tests (18.45 KB, patch)
2023-10-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DB changes (15.21 KB, patch)
2023-10-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DBIC changes (6.84 KB, patch)
2023-10-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: DBIC specific (1.20 KB, patch)
2023-10-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Restore updated_on for individual content (8.19 KB, patch)
2023-10-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: (QA follow-up) Do not return 0 in ->get (1.01 KB, patch)
2023-10-20 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Check lang and use right column in search_for_display (1.80 KB, patch)
2023-10-20 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Use one query in search_for_display instead of four (3.69 KB, patch)
2023-10-20 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Fix URLs on staff AC detail view (3.17 KB, patch)
2023-10-20 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Fix dbrevs (1.51 KB, patch)
2023-10-20 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: (QA follow-up) Tidy (20.04 KB, patch)
2023-10-20 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: Fix kohastructure.sql (1.24 KB, patch)
2023-10-20 22:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31383: Fix the DB upgrade process (2.70 KB, patch)
2023-10-21 06:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: (bug 29691 follow-up) Remove get_opac_news_by_id (5.55 KB, patch)
2023-10-23 09:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: (follow-up) Remove get_opac_news_by_id (4.08 KB, patch)
2023-10-24 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31383: (follow-up) Remove get_opac_news_by_id (7.66 KB, patch)
2023-10-24 13:29 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-08-17 11:59:24 UTC
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.
Comment 1 Kyle M Hall 2022-08-17 12:56:38 UTC
For a less radical engineering change, we could just add a parent_id column to the existing table.
Comment 2 Katrin Fischer 2022-08-17 13:16:06 UTC
(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.
Comment 3 Katrin Fischer 2022-08-17 13:16:38 UTC
> 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?
Comment 4 Marcel de Rooy 2022-08-17 13:51:35 UTC
(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.
Comment 5 Kyle M Hall 2022-08-17 13:57:48 UTC
(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!
Comment 6 Jonathan Druart 2023-03-08 10:36:19 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2023-03-08 10:36:21 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2023-03-08 10:36:24 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2023-03-08 10:36:27 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2023-03-08 10:36:30 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2023-03-08 10:36:33 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2023-03-20 19:30:34 UTC
This is ready for testing!
Comment 13 Marcel de Rooy 2023-04-18 13:28:19 UTC
Looking here.

Rebased conflict with new WRAPPER in koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt
Comment 14 Marcel de Rooy 2023-04-18 13:30:31 UTC
 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
Comment 15 Marcel de Rooy 2023-04-18 13:37:21 UTC
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
Comment 16 Jonathan Druart 2023-04-18 14:43:08 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2023-04-18 14:43:11 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2023-04-18 14:43:14 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2023-04-18 14:43:17 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2023-04-18 14:43:20 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2023-04-18 14:43:23 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2023-04-18 14:43:38 UTC
(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!
Comment 23 Jonathan Druart 2023-04-18 14:45:23 UTC
(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.
Comment 24 Jonathan Druart 2023-06-16 15:07:57 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2023-06-16 15:08:01 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2023-06-16 15:08:05 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2023-06-16 15:08:08 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2023-06-16 15:08:11 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2023-06-16 15:08:15 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2023-06-16 15:08:40 UTC
Patches rebased against master.
Comment 31 Marcel de Rooy 2023-06-19 09:29:33 UTC
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.
Comment 32 Jonathan Druart 2023-06-19 10:32:33 UTC
(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.
Comment 33 Marcel de Rooy 2023-06-22 08:58:06 UTC
Working here. Moving it to BLOCKED
Comment 34 Marcel de Rooy 2023-06-22 09:01:10 UTC
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') ]
         },
     );
Comment 35 Marcel de Rooy 2023-06-22 09:03:06 UTC
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.
Comment 36 Marcel de Rooy 2023-06-22 10:06:38 UTC Comment hidden (obsolete)
Comment 37 Marcel de Rooy 2023-06-22 10:06:41 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2023-06-22 10:06:44 UTC Comment hidden (obsolete)
Comment 39 Marcel de Rooy 2023-06-22 10:07:46 UTC
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?
Comment 40 Marcel de Rooy 2023-06-22 14:20:27 UTC Comment hidden (obsolete)
Comment 41 Marcel de Rooy 2023-06-22 14:22:33 UTC
Main thing I still want to do, is test the dbrev a bit better than I did at first.
Comment 42 Marcel de Rooy 2023-06-26 08:21:12 UTC
(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
Comment 43 Marcel de Rooy 2023-06-26 08:28:04 UTC
(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.
Comment 44 Marcel de Rooy 2023-06-26 08:34:15 UTC
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.
Comment 45 Marcel de Rooy 2023-06-26 08:35:12 UTC Comment hidden (obsolete)
Comment 46 Marcel de Rooy 2023-06-26 08:35:15 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2023-06-26 08:35:18 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2023-06-26 08:35:21 UTC Comment hidden (obsolete)
Comment 49 Marcel de Rooy 2023-06-26 08:35:24 UTC Comment hidden (obsolete)
Comment 50 Marcel de Rooy 2023-06-26 08:35:27 UTC Comment hidden (obsolete)
Comment 51 Marcel de Rooy 2023-06-26 08:35:30 UTC Comment hidden (obsolete)
Comment 52 Marcel de Rooy 2023-06-26 08:35:33 UTC Comment hidden (obsolete)
Comment 53 Marcel de Rooy 2023-06-26 08:35:36 UTC Comment hidden (obsolete)
Comment 54 Marcel de Rooy 2023-06-26 08:35:39 UTC Comment hidden (obsolete)
Comment 55 Marcel de Rooy 2023-06-26 08:36:40 UTC
Rebased, few follow-ups. Dbrev needs attention.
Comment 56 Jonathan Druart 2023-07-04 11:58:01 UTC
(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.
Comment 57 Jonathan Druart 2023-07-04 11:58:57 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2023-07-04 12:00:59 UTC
(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).
Comment 59 Marcel de Rooy 2023-07-19 14:30:23 UTC
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'
Comment 60 Marcel de Rooy 2023-07-19 14:36:22 UTC
Needed small rebase on fa pencil stuff
Comment 61 Marcel de Rooy 2023-07-19 14:37:56 UTC Comment hidden (obsolete)
Comment 62 Marcel de Rooy 2023-07-19 14:37:59 UTC Comment hidden (obsolete)
Comment 63 Marcel de Rooy 2023-07-19 14:38:02 UTC Comment hidden (obsolete)
Comment 64 Marcel de Rooy 2023-07-19 14:38:05 UTC Comment hidden (obsolete)
Comment 65 Marcel de Rooy 2023-07-19 14:38:08 UTC Comment hidden (obsolete)
Comment 66 Marcel de Rooy 2023-07-19 14:38:10 UTC Comment hidden (obsolete)
Comment 67 Marcel de Rooy 2023-07-19 14:38:13 UTC Comment hidden (obsolete)
Comment 68 Marcel de Rooy 2023-07-19 14:38:16 UTC Comment hidden (obsolete)
Comment 69 Marcel de Rooy 2023-07-19 14:38:19 UTC Comment hidden (obsolete)
Comment 70 Marcel de Rooy 2023-07-19 14:38:22 UTC Comment hidden (obsolete)
Comment 71 Marcel de Rooy 2023-07-19 14:38:25 UTC Comment hidden (obsolete)
Comment 72 Jonathan Druart 2023-09-22 09:40:03 UTC Comment hidden (obsolete)
Comment 73 Jonathan Druart 2023-09-22 09:40:07 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2023-09-22 09:40:10 UTC Comment hidden (obsolete)
Comment 75 Jonathan Druart 2023-09-22 09:40:14 UTC Comment hidden (obsolete)
Comment 76 Jonathan Druart 2023-09-22 09:40:17 UTC Comment hidden (obsolete)
Comment 77 Jonathan Druart 2023-09-22 09:40:20 UTC Comment hidden (obsolete)
Comment 78 Jonathan Druart 2023-09-22 09:40:24 UTC Comment hidden (obsolete)
Comment 79 Jonathan Druart 2023-09-22 09:40:28 UTC Comment hidden (obsolete)
Comment 80 Jonathan Druart 2023-09-22 09:40:31 UTC Comment hidden (obsolete)
Comment 81 Jonathan Druart 2023-09-22 09:40:34 UTC Comment hidden (obsolete)
Comment 82 Jonathan Druart 2023-09-22 09:40:38 UTC Comment hidden (obsolete)
Comment 83 Jonathan Druart 2023-09-22 09:42:10 UTC
Trivial rebase.
Comment 84 Martin Renvoize 2023-09-26 15:49:40 UTC
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.
Comment 85 Jonathan Druart 2023-09-26 17:19:15 UTC
(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 :)
Comment 86 Martin Renvoize 2023-09-26 18:35:26 UTC
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
Comment 87 Martin Renvoize 2023-09-27 10:01:17 UTC Comment hidden (obsolete)
Comment 88 Martin Renvoize 2023-09-27 10:01:21 UTC Comment hidden (obsolete)
Comment 89 Martin Renvoize 2023-09-27 10:01:25 UTC Comment hidden (obsolete)
Comment 90 Martin Renvoize 2023-09-27 10:01:29 UTC Comment hidden (obsolete)
Comment 91 Martin Renvoize 2023-09-27 10:01:33 UTC Comment hidden (obsolete)
Comment 92 Martin Renvoize 2023-09-27 10:01:37 UTC Comment hidden (obsolete)
Comment 93 Martin Renvoize 2023-09-27 10:01:40 UTC Comment hidden (obsolete)
Comment 94 Martin Renvoize 2023-09-27 10:01:44 UTC Comment hidden (obsolete)
Comment 95 Martin Renvoize 2023-09-27 10:01:47 UTC Comment hidden (obsolete)
Comment 96 Martin Renvoize 2023-09-27 10:01:51 UTC Comment hidden (obsolete)
Comment 97 Martin Renvoize 2023-09-27 10:01:54 UTC Comment hidden (obsolete)
Comment 98 Martin Renvoize 2023-09-27 10:01:58 UTC Comment hidden (obsolete)
Comment 99 Martin Renvoize 2023-09-27 10:03:06 UTC
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
Comment 100 Tomás Cohen Arazi 2023-10-20 14:49:31 UTC
Small rebase required. Please do ASAP.
Comment 101 Jonathan Druart 2023-10-20 15:06:39 UTC
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>
Comment 102 Jonathan Druart 2023-10-20 15:06:43 UTC
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>
Comment 103 Jonathan Druart 2023-10-20 15:06:47 UTC
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>
Comment 104 Jonathan Druart 2023-10-20 15:06:50 UTC
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>
Comment 105 Jonathan Druart 2023-10-20 15:06:54 UTC
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>
Comment 106 Jonathan Druart 2023-10-20 15:06:57 UTC
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>
Comment 107 Jonathan Druart 2023-10-20 15:07:01 UTC
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>
Comment 108 Jonathan Druart 2023-10-20 15:07:04 UTC
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>
Comment 109 Jonathan Druart 2023-10-20 15:07:07 UTC
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>
Comment 110 Jonathan Druart 2023-10-20 15:07:11 UTC
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>
Comment 111 Jonathan Druart 2023-10-20 15:07:14 UTC
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>
Comment 112 Jonathan Druart 2023-10-20 15:07:18 UTC
Created attachment 157550 [details] [review]
Bug 31383: (QA follow-up) Tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 113 Tomás Cohen Arazi 2023-10-20 17:46:08 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 114 Tomás Cohen Arazi 2023-10-20 20:07:22 UTC
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?
Comment 115 Tomás Cohen Arazi 2023-10-20 22:14:19 UTC
Created attachment 157580 [details] [review]
Bug 31383: Fix kohastructure.sql

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 116 Jonathan Druart 2023-10-21 06:19:01 UTC
(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.
Comment 117 Jonathan Druart 2023-10-21 06:19:41 UTC
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.
Comment 118 Jonathan Druart 2023-10-21 06:21:30 UTC
(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.
Comment 119 Jonathan Druart 2023-10-23 09:20:37 UTC
Created attachment 157640 [details] [review]
Bug 31383: (bug 29691 follow-up) Remove get_opac_news_by_id

We can simply use get here
Comment 120 Jonathan Druart 2023-10-23 09:21:00 UTC
Nick, could you have a look at this patch please? Would it work for you (as the author of bug 29691)?
Comment 121 Owen Leonard 2023-10-23 11:08:54 UTC
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
Comment 122 Marcel de Rooy 2023-10-23 11:43:46 UTC
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'
Comment 123 Owen Leonard 2023-10-23 12:17:40 UTC
If I delete any notices which have a branchcode from the letters table before I run the database update the update runs without error.
Comment 124 Tomás Cohen Arazi 2023-10-23 13:01:33 UTC
(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.
Comment 125 Tomás Cohen Arazi 2023-10-23 13:04:04 UTC
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 ) %]
Comment 126 Owen Leonard 2023-10-23 13:31:35 UTC
(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.
Comment 127 Jonathan Druart 2023-10-23 13:38:43 UTC
(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!
Comment 128 Tomás Cohen Arazi 2023-10-23 13:47:19 UTC
(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!
Comment 129 Jonathan Druart 2023-10-24 13:14:08 UTC
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 );
Comment 130 Jonathan Druart 2023-10-24 13:14:35 UTC
Please wait for Nick's comment this time ;)
Comment 131 Nick Clemens 2023-10-24 13:29:46 UTC
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>
Comment 132 Tomás Cohen Arazi 2023-11-02 12:36:40 UTC
(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.