Bug 29113

Summary: New "code" field for additional contents is not useful for the end users
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: ToolsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: jonathan.druart, m.de.rooy, martin.renvoize, nick, nugged
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29515
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31509
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00
Bug Depends on: 24387    
Bug Blocks: 29693    
Attachments: Bug 29113: Hide code for additional contents and generate it
Bug 29113: Hide code for additional contents and generate it
Bug 29113: Fix random assignation
Bug 29113: Fix random assignation
Bug 29113: Hide code for additional contents and generate it
Bug 29113: Fix random assignation
Bug 29113: Hide code for additional contents and generate it
Bug 29113: Fix random assignation

Description Katrin Fischer 2021-09-26 13:26:59 UTC
When adding a HTML customization there is now a new required "Code" field that has to be filled in.

Is this really necessary?

* The old news editor didn't require this additional field.
* None of the HTML customizations is repeatable.
Comment 1 Katrin Fischer 2021-09-26 13:27:37 UTC
* The code doesn't show in the list of entries.
Comment 2 Katrin Fischer 2021-10-02 11:55:10 UTC
... and why don't we use the internal id? :)
Comment 3 Jonathan Druart 2021-10-02 13:26:25 UTC
It groups the entries together. Like you have news_1 that is translated in different languages, the code will be the same for the different rows and the system will know they represent the same content.
Comment 4 Jonathan Druart 2021-10-02 13:26:54 UTC
It's the same behaviour we have for notice templates.
Comment 5 Katrin Fischer 2021-10-02 13:32:38 UTC
I wonder if we could not do better solving this with an additional ID or generated code instead of relying on user entry here if that's the only use case. But if we keep it, should we not make it visible in the table too?

For the notices the code is also used in the code, so we have to rely on the codes being right for activating/deactivating the functionality.
Comment 6 Jonathan Druart 2021-10-04 14:31:32 UTC
Created attachment 125695 [details] [review]
Bug 29113: Hide code for additional contents and generate it

additional_contents.code is used to group DB rows together. Each row
represent one content in a given language, and the code is used to know
they are translation of the lang='default' one.

It's not really useful for the end user and we could hide it and
generate it.

Test plan:
Create/Edit/Delete additional contents (news and HTML customizations)
and confirm that they are correctly grouped together.
You need several languages installed to test this patch correctly.
Comment 7 Jonathan Druart 2021-10-04 14:49:23 UTC
Created attachment 125696 [details] [review]
Bug 29113: Hide code for additional contents and generate it

additional_contents.code is used to group DB rows together. Each row
represent one content in a given language, and the code is used to know
they are translation of the lang='default' one.

It's not really useful for the end user and we could hide it and
generate it.

Test plan:
Create/Edit/Delete additional contents (news and HTML customizations)
and confirm that they are correctly grouped together.
You need several languages installed to test this patch correctly.
Comment 8 Nick Clemens (kidclamp) 2021-10-04 16:51:50 UTC
If i add a new item, defining several languages on the initial save then the code is set as NULL for all entries

Adding a second does the same and they are all grouped as a single entry
Comment 9 Jonathan Druart 2021-10-07 08:06:32 UTC
Created attachment 125861 [details] [review]
Bug 29113: Fix random assignation
Comment 10 Jonathan Druart 2021-10-07 08:07:34 UTC
Nick, I don't recreate what you describe. But I found a bug when testing (title and content were getting a random language).
Comment 11 Marcel de Rooy 2021-10-07 11:12:34 UTC
Could this code field be used too as a sort of alias for passing the internal identifier (idnew?) to opac-main ?
See bug 29190
Or should that better be another (new) field?
Comment 12 Marcel de Rooy 2021-11-05 08:51:07 UTC
+    for my $lang ( sort {$a ne 'default'} @lang ) { # Process 'default' first

Hmm. Nice coding pattern.
Comment 13 Marcel de Rooy 2021-11-05 08:56:37 UTC
(In reply to Jonathan Druart from comment #9)
> Created attachment 125861 [details] [review] [review]
> Bug 29113: Fix random assignation

Unclear commit message, no test plan
Comment 14 Marcel de Rooy 2021-11-05 08:57:32 UTC
If the title says that it is unnecessary, why not remove it? Or should you better adjust the title?
Comment 15 Jonathan Druart 2021-11-05 09:24:23 UTC
(In reply to Marcel de Rooy from comment #14)
> If the title says that it is unnecessary, why not remove it? Or should you
> better adjust the title?

We need it, to group rows together (rows have different languages, but represent the same content). However the end users don't need it.
Comment 16 Marcel de Rooy 2021-11-05 09:26:26 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Marcel de Rooy from comment #14)
> > If the title says that it is unnecessary, why not remove it? Or should you
> > better adjust the title?
> 
> We need it, to group rows together (rows have different languages, but
> represent the same content). However the end users don't need it.

Adjust the title then?
If title and commit messages are unclear, getting it further may be more complicated?
Comment 17 Jonathan Druart 2021-11-05 09:27:28 UTC
(In reply to Marcel de Rooy from comment #13)
> (In reply to Jonathan Druart from comment #9)
> > Created attachment 125861 [details] [review] [review] [review]
> > Bug 29113: Fix random assignation
> 
> Unclear commit message, no test plan

Because of the sort we cannot shift. So we need to specify and retrieve explicitly what we need (using $lang)
Comment 18 Jonathan Druart 2021-11-05 09:28:40 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2021-11-05 09:28:52 UTC
(In reply to Marcel de Rooy from comment #16)
> (In reply to Jonathan Druart from comment #15)
> > (In reply to Marcel de Rooy from comment #14)
> > > If the title says that it is unnecessary, why not remove it? Or should you
> > > better adjust the title?
> > 
> > We need it, to group rows together (rows have different languages, but
> > represent the same content). However the end users don't need it.
> 
> Adjust the title then?
> If title and commit messages are unclear, getting it further may be more
> complicated?

Done, commit message read good to me however.
Comment 20 Martin Renvoize 2021-11-05 13:08:57 UTC
This is one of the reasons I was keen to have distinct tables for these features rather than combine them I think..  https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24387#c30
Comment 21 Martin Renvoize 2021-11-05 13:12:41 UTC
Sorry, sha1 errors, I can't get this one to apply.

My feeling is.. this hides an issue nicely as an interim measure.. I still feel that we should split the features to allow them to develop independently and at that point we can drop the superfluous 'code' field for this particular use case.

I'm still keen on 'News', 'Blocks' and 'Pages' at the end of the day
Comment 22 Jonathan Druart 2021-11-05 13:28:49 UTC
Created attachment 127365 [details] [review]
Bug 29113: Fix random assignation
Comment 23 Jonathan Druart 2021-11-05 13:30:14 UTC
(In reply to Martin Renvoize from comment #21)
> Sorry, sha1 errors, I can't get this one to apply.
> 
> My feeling is.. this hides an issue nicely as an interim measure.. I still
> feel that we should split the features to allow them to develop
> independently and at that point we can drop the superfluous 'code' field for
> this particular use case.
> 
> I'm still keen on 'News', 'Blocks' and 'Pages' at the end of the day

Which issue is hidden? We need this column, even if we split the features (what would we gain?).
Comment 24 Jonathan Druart 2021-11-05 14:22:51 UTC
To go further:
1. We should add "pages" to this table (and so don't split the table or we will end up with 3 identical tables). This is related to bug 27527.
2. I actually think we should put the notice templates into this table (merge them at least)
3. What we need to avoid this code is to normalize and have 2 tables, linked together.

additional_contents
- id
- category
- published_on
- updated_on
- expirationdate
- number

Another table for translations:
- fk
- lang (or eventually fk to a lang table)
- title
- content

And, this is similar to the itemtypes, av, etc. translation story. Translations in the DB must be reconsidered, but it's another topic...
Comment 25 Jonathan Druart 2021-11-16 09:00:54 UTC
Can we move forward?
Comment 26 Marcel de Rooy 2021-11-16 13:19:33 UTC
(In reply to Jonathan Druart from comment #24)

> additional_contents
> - id
> - category
> - published_on
> - updated_on
> - expirationdate
> - number
> 
> Another table for translations:
> - fk
> - lang (or eventually fk to a lang table)
> - title
> - content

This looks better.
Comment 27 Jonathan Druart 2021-11-16 13:27:55 UTC
(In reply to Marcel de Rooy from comment #26)
> (In reply to Jonathan Druart from comment #24)
> 
> > additional_contents
> > - id
> > - category
> > - published_on
> > - updated_on
> > - expirationdate
> > - number
> > 
> > Another table for translations:
> > - fk
> > - lang (or eventually fk to a lang table)
> > - title
> > - content
> 
> This looks better.

But this is not for now :D

However I still think we should hide the code for 21.11.
Comment 28 Marcel de Rooy 2021-11-19 10:25:41 UTC
+                    code           => $code || 'tmp_code',

Please explain.
Comment 29 Jonathan Druart 2021-11-19 10:38:32 UTC
(In reply to Marcel de Rooy from comment #28)
> +                    code           => $code || 'tmp_code',
> 
> Please explain.
The code is mandatory at DB level so we must add a temporary code during the insert, then update it after

I feel like it should actually be 'tmp_code' always, at this point we are creating a new entry and $code should never be set.

We could add the following change

diff --git a/tools/additional-contents.pl b/tools/additional-contents.pl
index e63a44a809a..59721e24b2a 100755
--- a/tools/additional-contents.pl
+++ b/tools/additional-contents.pl
@@ -159,7 +159,7 @@ elsif ( $op eq 'add_validate' ) {
             my $additional_content = Koha::AdditionalContent->new(
                 {
                     category       => $category,
-                    code           => $code || 'tmp_code',
+                    code           => 'tmp_code',
                     location       => $location,
                     branchcode     => $branchcode,
                     title          => $title,
@@ -173,13 +173,11 @@ elsif ( $op eq 'add_validate' ) {
             )->store;
             eval {
                 $additional_content->store;
-                unless ($code) {
-                    $additional_content->discard_changes;
-                    $code = $category eq 'news'
-                      ? 'News_' . $additional_content->idnew
-                      : $location . '_' . $additional_content->idnew;
-                    $additional_content->code($code)->store;
-                }
+                $additional_content->discard_changes;
+                $code = $category eq 'news'
+                  ? 'News_' . $additional_content->idnew
+                  : $location . '_' . $additional_content->idnew;
+                $additional_content->code($code)->store;
             };
             if ($@) {
                 $success = 0;
Comment 30 Marcel de Rooy 2021-11-19 10:53:13 UTC
Created attachment 127856 [details] [review]
Bug 29113: Hide code for additional contents and generate it

additional_contents.code is used to group DB rows together. Each row
represent one content in a given language, and the code is used to know
they are translation of the lang='default' one.

It's not really useful for the end user and we could hide it and
generate it.

Test plan:
Create/Edit/Delete additional contents (news and HTML customizations)
and confirm that they are correctly grouped together.
You need several languages installed to test this patch correctly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2021-11-19 10:53:17 UTC
Created attachment 127857 [details] [review]
Bug 29113: Fix random assignation

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2021-11-19 10:54:04 UTC
Works, but the 'code' could be nicer. Still time for another QAer today?
Comment 33 Martin Renvoize 2021-11-25 05:49:19 UTC
Created attachment 127989 [details] [review]
Bug 29113: Hide code for additional contents and generate it

additional_contents.code is used to group DB rows together. Each row
represent one content in a given language, and the code is used to know
they are translation of the lang='default' one.

It's not really useful for the end user and we could hide it and
generate it.

Test plan:
Create/Edit/Delete additional contents (news and HTML customizations)
and confirm that they are correctly grouped together.
You need several languages installed to test this patch correctly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2021-11-25 05:49:23 UTC
Created attachment 127990 [details] [review]
Bug 29113: Fix random assignation

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Jonathan Druart 2021-11-25 09:05:09 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 36 Marcel de Rooy 2022-09-02 08:10:38 UTC
> additional_contents.code is used to group DB rows together. Each row
> represent one content in a given language, and the code is used to know
> they are translation of the lang='default' one.
> 
> It's not really useful for the end user and we could hide it and
> generate it.

Opening a new bug to allow configuring this code somehow. I need some more flexibility on e.g. bug 31503.
It adds a location PatronConsent but I need codes like PatronConsent_[suffix] where suffix points to a specific patron consent type.