Bug 31642

Summary: Control locations for HTML customizations from new table or Authorised values
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: ToolsAssignee: Marcel de Rooy <m.de.rooy>
Status: RESOLVED WONTFIX QA Contact: Pedro Amorim <pedro.amorim>
Severity: enhancement    
Priority: P5 - low CC: blawlor, david, jonathan.druart, lisette, martin.renvoize, pedro.amorim, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 31642: Add sql entries for new AV categories and values
Bug 31642: Database revision for upgrades
Bug 31642: Use the AV categories to control locations for HTML customizations
Bug 31642: Add sql entries for new AV categories and values
Bug 31642: Database revision for upgrades
Bug 31642: Use the AV categories to control locations for HTML customizations
Bug 31642: (follow-up) Add descriptions for three new AV categories
Bug 31642: (QA follow-up) Hide Custom section when it has no children

Description Marcel de Rooy 2022-09-28 12:12:39 UTC
I ran into this limitation on e.g. bug 31503. I want to add locations that match authorised values for category CONSENT_TYPE. But Koha only allows a selected set of locations for html blocks.

If we could allow picking a field from the combo or typing free text, we provide more flexibility in the use of this feature.

Obviously this triggers a development discussion on field names. We have category, location and code. And their use becomes a bit unclear.
Category is the most clear one. Although html and pages are quite similar.
Location is opac or staff etc. for news and pages. But is used as a kind of block name for html_customizations.
Code is used to group the records for default and translated languages for an item in all categories. The name is rather obscure.
While describing this, bug 31383 comes to mind again..
Comment 1 Marcel de Rooy 2025-02-20 13:33:49 UTC
Coming back here after some time, I think that we could move this report in the following direction: Control locations for HTML customizations from new table or Authorised values.

[1] A new table could have volumns like id, name. And needs a nice little associated Administration form.
This table should also have a boolean field like is_system_location in order to make a distinction between custom locations and system locations. System entries should not be edited or deleted?
Also it might have a field that shows if it should be grouped/sorted under OPAC, Staff or ...? See also below current template code.

This would affect existing HTML template code like:

        [% SET opac_available_options = [ 'OpacNavRight', 'opacheader', 'OpacCustomSearch', 'OpacMainUserBlock', 'opaccredits', 'OpacLoginInstructions', 'OpacNav', 'OpacNavBottom', 'OpacSuggestionInstructions', 'ArticleRequestsDisclaimerText', 'OpacMoreSearches', 'OpacMySummaryNote', 'OpacLibraryInfo', 'OpacMaintenanceNotice', 'OPACResultsSidebar', 'OpacSuppressionMessage', 'SCOMainUserBlock', 'SelfCheckInMainUserBlock', 'SelfCheckHelpMessage', 'CatalogConcernHelp', 'CatalogConcernTemplate', 'CookieConsentBar', 'CookieConsentPopup', 'PatronSelfRegistrationAdditionalInstructions', 'ILLModuleCopyrightClearance' ] %]

        [% SET staff_available_options = [ 'IntranetmainUserblock', 'RoutingListNote', 'StaffAcquisitionsHome', 'StaffAuthoritiesHome', 'StaffCataloguingHome', 'StaffListsHome', 'StaffLoginInstructions', 'StaffPatronsHome', 'StaffPOSHome', 'StaffSerialsHome' ] %]


[2] Note that we could read them from Authorized values too but then we dont have additional fields like is_system, is_staff.
We could address that by reading from three AV categories: OPAC system, Staff system and Local customizations.
It would remove the need for yet another admin form.
Comment 2 Marcel de Rooy 2025-02-20 15:30:39 UTC
Will submit some patches following the (faster) second approach with AV.
Comment 3 Marcel de Rooy 2025-02-20 15:35:04 UTC
Created attachment 178424 [details] [review]
Bug 31642: Add sql entries for new AV categories and values

Test plan:
Run a new install. Verify that you have the new AV categories and
values like ADD_CONT_HTML_OPAC_SYSTEM.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Ran load_sql on the auth_values.yml.
Comment 4 Marcel de Rooy 2025-02-20 15:35:06 UTC
Created attachment 178425 [details] [review]
Bug 31642: Database revision for upgrades

Test plan:
Run updatedatabase.pl
Verify that the three new categories were added.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2025-02-20 15:35:08 UTC
Created attachment 178426 [details] [review]
Bug 31642: Use the AV categories to control locations for HTML customizations

Test plan:
Go to HTML customizations. Add new.
Check that you see the expected display locations.
Add a new location under AV category ADD_CONT_HTML_CUSTOM.
Add another HTML customization with that new location. Edit it.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2025-02-20 15:35:59 UTC
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt
   FAIL   tt_valid
                lines 402
=> This is a false warning. I dont touch that line here. You can ignore it for now.
Comment 7 David Nind 2025-03-11 20:03:29 UTC
Created attachment 179183 [details] [review]
Bug 31642: Add sql entries for new AV categories and values

Test plan:
Run a new install. Verify that you have the new AV categories and
values like ADD_CONT_HTML_OPAC_SYSTEM.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Ran load_sql on the auth_values.yml.
Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2025-03-11 20:03:31 UTC
Created attachment 179184 [details] [review]
Bug 31642: Database revision for upgrades

Test plan:
Run updatedatabase.pl
Verify that the three new categories were added.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2025-03-11 20:03:34 UTC
Created attachment 179185 [details] [review]
Bug 31642: Use the AV categories to control locations for HTML customizations

Test plan:
Go to HTML customizations. Add new.
Check that you see the expected display locations.
Add a new location under AV category ADD_CONT_HTML_CUSTOM.
Add another HTML customization with that new location. Edit it.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2025-03-11 20:17:50 UTC
The only suggestion I would have is to add a description to the new authorized value categories:

- ADD_CONT_HTML_CUSTOM: Custom display locations for HTML customizations
- ADD_CONT_HTML_OPAC_SYSTEM: OPAC display locations for HTML customizations
- ADD_CONT_HTML_STAFF_SYSTEM: Staff interface display locations for HTML customizations
Comment 11 Marcel de Rooy 2025-03-12 12:30:32 UTC
(In reply to David Nind from comment #10)
> The only suggestion I would have is to add a description to the new
> authorized value categories:
> 
> - ADD_CONT_HTML_CUSTOM: Custom display locations for HTML customizations
> - ADD_CONT_HTML_OPAC_SYSTEM: OPAC display locations for HTML customizations
> - ADD_CONT_HTML_STAFF_SYSTEM: Staff interface display locations for HTML
> customizations

Thanks for your SO. Will add those descriptions.
Comment 12 Marcel de Rooy 2025-03-12 13:12:50 UTC
Created attachment 179205 [details] [review]
Bug 31642: (follow-up) Add descriptions for three new AV categories

Good suggestion from David Nind on BZ comment10 (thx).

Test plan:
Go to admin/authorised_values.pl and check category description on
main form.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Pedro Amorim 2025-05-30 11:26:32 UTC
Observations:
A) If you have an HTML customization on location e.g. 'CookieConsentPopup', and then delete the 'CookieConsentPopup' AV value from ADD_CONT_HTML_OPAC_SYSTEM, at least 2 things will happen:
1) Whenever you edit the existing HTML customization that's still pointing to the - now non-existent - 'CookieConsentPopup' location, the form will render 'ArticleRequestsDisclaimerText' (or whatever is the first alphabetical AV to be shown) as the current location. In short, the form will show false information.
2) The HTML customization is still displayed on whatever location it is still set to, because the code that fetches a HTML customization by a location is still looking for it. In other words, a staff member that deleted that AV from ADD_CONT_HTML_OPAC_SYSTEM potentially thinks that that location is not displayed anymore (because it no longer exists), but that's not the case.

B) 'Custom' is shown at the bottom of the dropdown as a parent level entry even if there are no children i.e. ADD_CONT_HTML_CUSTOM is empty.

C) Apologies for not understanding the original requirement, I'm certainly missing something obvious, but can you please clarify?
'I want to add locations that match authorised values for category CONSENT_TYPE.'
How would you use the work from this patchset? Would you be adding entries to ADD_CONT_HTML_CUSTOM and then how would those locations be rendered Through a plugin? In other words, how would entries from 'ADD_CONT_HTML_CUSTOM' be utilized?

Failing QA especially because of observation A). I think this gives the staff member the ability to potentially shoot themselves in the foot by deleting an AV entry that the code directly expects it to exist, and that'll lead to future problems. Personally, I agree with comment 1 regarding having a "is_system" column in the authorised_values table and believe that would be the solution here to prevent deleting/editing AV entries that the system expects to exist.
Comment 14 Marcel de Rooy 2025-06-02 10:05:52 UTC
(In reply to Pedro Amorim from comment #13)
Thx Pedro for qa. Will come back to those points on separate comments.
Comment 15 Marcel de Rooy 2025-06-02 11:46:47 UTC
Created attachment 182904 [details] [review]
Bug 31642: (QA follow-up) Hide Custom section when it has no children

Test plan:
Have no entries under AV category ADD_CONT_HTML_CUSTOM.
Verify that you do not see Custom at the bottom of the Display
location combo when adding a new html block.
Add one AV entry under ADD_CONT_HTML_CUSTOM.
Refresh and make sure that you see the Custom section now.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2025-06-02 11:47:05 UTC
Point [B]
Hiding Custom when there are no entries. Handled in QA follow-up.
Comment 17 Marcel de Rooy 2025-06-02 11:47:29 UTC
Point [C]
Actually two reasons:
[1] To illustrate the need for this change, please look at this part of opac-patron-consent.tt (see also bug 31503)
                            [% IF consent_type == 'GDPR_PROCESSING' %]
                                <legend><h2>Privacy policy consent</h2></legend>
                                [% rma_privacy_url = { en => 'https://www.rijksmuseum.nl/en/privacy-policy', 'nl-NL' => 'https://www.rijksmuseum.nl/nl/privacybeleid' } %]
                                <div class="alert alert-warning">
                                    <p>In order to keep you logged in, we need your consent to process personal data as specified in the privacy policy linked below.</p>
                                    <p>If you choose to withdraw your consent we will <strong>remove your account</strong> within a reasonable time and you should log out now.</p>
                                </div>
                                <p>I have read the <a target="_blank" href="[% rma_privacy_url.$lang | url %]">privacy policy</a> of Stichting Het Rijksmuseum and the <a target="_blank" href="[% Koha.Preference('PrivacyPolicyURL') %]">additional explanation</a> for the library catalog, and agree with your processing of my personal data as outlined therein.</p>
                            [% ELSIF consent_types.$consent_type %]
                                [% SET consent_title = ( consent_types.$consent_type.title.$lang || consent_types.$consent_type.title.en ) %]
                                [% SET consent_desc  = ( consent_types.$consent_type.description.$lang || consent_types.$consent_type.description.en )  %]
                                <legend><h2>[% consent_title | html %]</h2></legend>
                                <p>[% consent_desc | html %]</p>
                                <p>Do you agree?</p>
                            [% ELSE %]
                                <legend><h2>Consent for [% consent_type | html %]</h2></legend>
                                <p>Do you agree?</p>
                            [% END %]
Being fed by:
foreach my $consent_type ( sort keys %$consent_types) {
    push @consents, $patron->consent($consent_type);
}
=> We could move a lot of this to additional contents. Currently I provide those texts for a custom newsletter consent from the consent plugin itself.

[2] As second reason, an architectural one. Instead of keeping the growing list of names for available html blocks in the template, we control it via Authorised values. No need to adjust additional-contents.tt anymore for adding a new standard or custom html block.
Comment 18 Marcel de Rooy 2025-06-02 12:02:16 UTC
Point [A].
Yeah, thats a nice one. Actually that problem is wider than just what you mention here. From this general 'multi-purpose' table like authorised values, it is hard to define foreign key constraints to keep all data in shape. Which means that (in general) removing values from AV creates inconsistencies like those in the database. A lot of item values e.g. depend on AV and would be bad data when removing corresponding entries from AV, etc.
Will still think a little bit on how to handle this specific case for the display location (having extended use in mind).
Comment 19 Marcel de Rooy 2025-06-03 11:41:56 UTC
Point [A] follow-up.
I think that it is too hard to solve both issues mentioned under A on this report here. They have a much wider scope.

If you delete values from a category, the Koha combo boxes here and there will currently display a wrong entry too.

The second issue under A is a nice one. But resembles having the value still in the database for other AV entries. Finding the additional content for AV html block X looks different than say blocking an item checkout because it is still notforloan (while you may have removed that entry from AV, but the item still has it), but in essence it is the same problem.

The idea of using is_system on the value level would be nice but no complete solution for both issues. Deleting a custom value would still be a problem.

Writing a module for additional actions after an AV modify or delete crossed my mind, but looks like a larger exercise with pitfalls as to performance etc.

So in conclusion, how should we proceed here? Or just abandon it?
Comment 20 Pedro Amorim 2025-06-03 13:04:50 UTC
(In reply to Marcel de Rooy from comment #19) 
> The idea of using is_system on the value level would be nice but no complete
> solution for both issues. Deleting a custom value would still be a problem.

From my understanding, custom HTML customization locations are meant to only be used by plugins (e.g. core is not querying or using'em). If this is the case, isolating this possible issue only to installations that use said plugins would be better than not handling it at all. 'is_system' AV entries should, at the very least, not be deletable through the UI, imo.
Additionally, plugin code making use of 'custom' locations should know that, because they're 'custom', they may or may not exist, so can error handle accordingly. Whereas core Koha using 'is_system' should ideally never need to handle the case of them not existing.

> If you delete values from a category, the Koha combo boxes here and there will > currently display a wrong entry too.

True, but we should at least consider displaying "!" with some text like "current value doesn't match any of the options" or similar. I know this patchset doesn't introduce this pattern in Koha, but it's adding yet another possibility where it may happen.

> Writing a module for additional actions after an AV modify or delete crossed
> my mind, but looks like a larger exercise with pitfalls as to performance
> etc.

We should have action hooks like 'before/after_object_update', 'before/after_object_create' and 'before/after_object_delete' at Koha::Object level and then any plugin could do whatever they wanted. As to performance, every other OS framework out there does this, surely we could work around these eventual challenges. But this is way outside the scope of this bug.

> So in conclusion, how should we proceed here? Or just abandon it?

I don't know. I don't feel comfortable PQAing it as is because it adds the possibility for staff members to create problems for themselves in a place that they couldn't before, but I also understand it's a deeper/wider problem not exclusive to this patchset.

A possible alternative here would be to add a filter hook to get_html_customizations_options added in bug 39900. Needs some changes but would allow for plugins to hook into the list of available customization locations and add whatever they want. Though personally I think the 'is_system' approach for AV values would benefit Koha more as a whole.
Comment 21 Marcel de Rooy 2025-06-05 12:53:41 UTC
(In reply to Pedro Amorim from comment #20)
> From my understanding, custom HTML customization locations are meant to only
> be used by plugins (e.g. core is not querying or using'em). If this is the
> case, isolating this possible issue only to installations that use said
> plugins would be better than not handling it at all. 'is_system' AV entries
> should, at the very least, not be deletable through the UI, imo.
> Additionally, plugin code making use of 'custom' locations should know that,
> because they're 'custom', they may or may not exist, so can error handle
> accordingly. Whereas core Koha using 'is_system' should ideally never need
> to handle the case of them not existing.

Agreed. Will still try if I can get a first start on av.is_system on this report while postponing more changes to a separate one.

> True, but we should at least consider displaying "!" with some text like
> "current value doesn't match any of the options" or similar. I know this
> patchset doesn't introduce this pattern in Koha, but it's adding yet another
> possibility where it may happen.

Will try. Not sure if people will oppose that as a "new inconsistent pattern". But maybe an alert on the page could do as well? Coming back later.

> I don't know. I don't feel comfortable PQAing it as is because it adds the
> possibility for staff members to create problems for themselves in a place
> that they couldn't before, but I also understand it's a deeper/wider problem
> not exclusive to this patchset.
> 
> A possible alternative here would be to add a filter hook to
> get_html_customizations_options added in bug 39900. Needs some changes but
> would allow for plugins to hook into the list of available customization
> locations and add whatever they want. Though personally I think the
> 'is_system' approach for AV values would benefit Koha more as a whole.

Looking at the is_system approach.
Comment 22 Katrin Fischer 2025-06-10 06:28:13 UTC
Sorry for being late here, I had set this aside to have a closer look, once things have calmed down a bit more after release.

I am not sure if moving this into an AV is the best approach. It will allow to change values that are only used internally and for people to create issues for themselves, as Pedro noted. More so, it adds another thing we need to maintain, while we still need code in the templates for each value to have a translatable description.

Wouldn't it make more sense to add branch_limitations to the HTML customizations, if that was the ultimate goal? It would also allow to keep all settings in one place. For plugins it should be possible to add to the list as well without requiring a database entry in a "standard" table (also making things easier to maintain).

If we keep this approach, I don't think lib and lib_opac should show up in translations:

+  # Locations for HTML blocks
+  - authorised_values:
+      translatable: [ lib, lib_opac ]
+      multiline: []
+      rows:
+        # OPAC
+        - category: "ADD_CONT_HTML_OPAC_SYSTEM"
+          authorised_value: "OpacNavRight"
+          lib: "OpacNavRight"
+          lib_opac: "OpacNavRight"


I am also not sure there is a use case for custom HTML customizations yet and if that should be added as an empty category if you cannot use it for something out of the box.
Comment 23 Marcel de Rooy 2025-06-10 11:19:10 UTC
(In reply to Katrin Fischer from comment #22)
> I am also not sure there is a use case for custom HTML customizations yet
> and if that should be added as an empty category if you cannot use it for
> something out of the box.

See description.
Comment 24 Marcel de Rooy 2025-06-10 11:20:40 UTC
Thanks for the feedback, Pedro and Katrin. I think that it is best now to not put more time in this patch set.

Closing