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.
* The code doesn't show in the list of entries.
... and why don't we use the internal id? :)
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.
It's the same behaviour we have for notice templates.
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.
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.
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.
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
Created attachment 125861 [details] [review] Bug 29113: Fix random assignation
Nick, I don't recreate what you describe. But I found a bug when testing (title and content were getting a random language).
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?
+ for my $lang ( sort {$a ne 'default'} @lang ) { # Process 'default' first Hmm. Nice coding pattern.
(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
If the title says that it is unnecessary, why not remove it? Or should you better adjust the title?
(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.
(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?
(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)
(In reply to Jonathan Druart from comment #17) > (In reply to Marcel de Rooy from comment #13) > > (In reply to Jonathan Druart from comment #9) > > > Created attachment 125861 [details] [review] [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) Done, commit message read good to me however.
(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.
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
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
Created attachment 127365 [details] [review] Bug 29113: Fix random assignation
(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?).
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...
Can we move forward?
(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.
(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.
+ code => $code || 'tmp_code', Please explain.
(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;
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>
Created attachment 127857 [details] [review] Bug 29113: Fix random assignation Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Works, but the 'code' could be nicer. Still time for another QAer today?
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>
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>
Pushed to master for 21.11, thanks to everybody involved!
> 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.