Bug 35151 - Convert ILLModuleCopyrightClearance system preference to additional contents
Summary: Convert ILLModuleCopyrightClearance system preference to additional contents
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Pedro Amorim
URL:
Keywords:
Depends on: 31383 35065
Blocks: 26050 35152
  Show dependency treegraph
 
Reported: 2023-10-25 17:20 UTC by Owen Leonard
Modified: 2024-05-25 09:16 UTC (History)
8 users (show)

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


Attachments
Bug 35151: Convert ILLModuleCopyrightClearance system preference to additional contents (11.86 KB, patch)
2023-11-08 17:25 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35151: Convert ILLModuleCopyrightClearance system preference to additional contents (11.96 KB, patch)
2023-11-09 15:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35151: Convert ILLModuleCopyrightClearance system preference to additional contents (12.02 KB, patch)
2023-12-07 17:15 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35151: Convert ILLModuleCopyrightClearance system preference to additional contents (12.10 KB, patch)
2024-02-29 10:48 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2023-10-25 17:20:53 UTC
Building on 22318, the ILLModuleCopyrightClearance system preference should be converted to an HTML customization option.
Comment 1 Owen Leonard 2023-11-03 15:55:05 UTC
This bug is trickier than other similar ones because we don't just show the ILLModuleCopyrightClearance message or not based on whether it is present.

Instead, there is an additional step to the OPAC ILL request process if ILLModuleCopyrightClearance is present. This is easy to do with a system preference, but I don't think there are any instances of Koha checking for the existence of an html customization at the perl module level.

I brought up this issue on IRC:

https://irc.koha-community.org/koha/2023-10-26#i_2517477

There's not a consensus that this is the way to go, but this is what I got working in Illrequest.pm:

sub backend_create {
    my ( $self, $params ) = @_;

    # Establish whether we need to do a generic copyright clearance.
    if ( $params->{opac} ) {

        my $copyright_content = Koha::AdditionalContents->search_for_display(
            {
                category   => 'html_customizations',
                location   => ['ILLModuleCopyrightClearance'],
                lang       => $params->{lang},
                library_id => $params->{branchcode},
            }
        );

        if ( ( !$params->{stage} || $params->{stage} eq 'init' )
            && $copyright_content->count )
        {
...

Is this a terrible idea?
Comment 2 Katrin Fischer 2023-11-05 11:32:30 UTC
Adding our ILL topic expert :)
Comment 3 Pedro Amorim 2023-11-06 10:38:04 UTC
I think, ideally, this is what we'd do:
1) Update ILLModuleCopyrightClearance handling to be a ILL:Workflow stage like TypeDisclaimer or Availability, see bug 33716. The difference between CopyrightClearance and the other 2 is that CopyrightClearance only shows up in the OPAC and comes up before the create form, whereas the other 2 show up after the create form. I wanted to do this in bug 33716 but was out of scope for that specific bug.
2) sub backend_create should not handle ILLModuleCopyrightClearance directly, but instead have something like Koha::Illrequest::Workflow::CopyrightClearance->show_copyrightclearance and have that class do all the HTML customization logic handling.

(In reply to Owen Leonard from comment #1)
> sub backend_create {
>     my ( $self, $params ) = @_;
> 
>     # Establish whether we need to do a generic copyright clearance.
>     if ( $params->{opac} ) {
> 
>         my $copyright_content = Koha::AdditionalContents->search_for_display(
>             {
>                 category   => 'html_customizations',
>                 location   => ['ILLModuleCopyrightClearance'],
>                 lang       => $params->{lang},
>                 library_id => $params->{branchcode},
>             }
>         );
> 
>         if ( ( !$params->{stage} || $params->{stage} eq 'init' )
>             && $copyright_content->count )
>         {
> ...
> 
> Is this a terrible idea?

I think this works, but I'm not familiar with HTML customizations enough to see possible issues this may open. What happens if more than one HTML customization exists for 'ILLModuleCopyrightClearance' location?
Comment 4 Owen Leonard 2023-11-06 12:05:58 UTC
(In reply to Pedro Amorim from comment #3)

> What happens if more than one HTML
> customization exists for 'ILLModuleCopyrightClearance' location?

It will show them all, in the order specified.
Comment 5 Owen Leonard 2023-11-08 17:25:37 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2023-11-09 15:02:15 UTC Comment hidden (obsolete)
Comment 7 Pedro Amorim 2023-11-16 15:05:43 UTC
I'll be looking into this when I get the chance, it's in my pile!
Comment 8 Pedro Amorim 2023-12-07 17:15:54 UTC
Created attachment 159659 [details] [review]
Bug 35151: Convert ILLModuleCopyrightClearance system preference to additional contents

This patch moves the ILLModuleCopyrightClearance system preference into
HTML customizations, making it possible to have language- and
library-specific content.

To test you should have some content in the ILLModuleCopyrightClearance
system preference before applying the patch. Apply the patch, run the
database update process, and rebuild the OPAC CSS.

- In the staff client, go to Tools -> HTML customizations and verify
  that the content from ILLModuleCopyrightClearance is now stored there.
- The HTML customization entry form should offer
  ILLModuleCopyrightClearance as a choice under "Display location."
- Update and reinstall active translations (for instance fr-FR):
  - perl misc/translator/translate update fr-FR
  - perl misc/translator/translate install fr-FR
- Enable the translation if necessary under Administration -> System
  preferences -> language.
- Enable the "opaclanguagesdisplay" preference if necessary.
- Edit the ILLModuleCopyrightClearance HTML customization and add unique
  content to the "fr-FR" tab.

- Enable the "ILLModule" and "ILLModuleUnmediated" system preferences if
  necessary. You must have at least one ILL backend installed.
- Log into the OPAC and click the "Interlibrary loan requests" in the
  sidebar menu on the user summary page.
- Click "Create new request."
- You should be taken to a page with your ILLModuleCopyrightClearance
  content shown along with "Yes" and "No" buttons.
  - Clicking "Yes" should take you to the form for creating a new
    request.
- Switch to your updated translation and confirm that the content
  appears correctly.
- Remove all "ILLModuleCopyrightClearance" html customizations. Test the
  "Create new request" process in the OPAC again.
  - There should be no confirmation step before arriving at the form for
    creating a new ILL request.
- Go to Administration -> System preferences and search for
  "ILLModuleCopyrightClearance." It should return no results.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 9 Pedro Amorim 2023-12-07 17:20:33 UTC
This is great, not only the language specific but also library specific option.

My only minor concern (not about this patch) is that, before, ILL related options where somewhat exclusive to sys prefs, now it's sys prefs and html customizations (among others, like ILL batch statuses in the Koha admin) but this relates to a different problem, one that could maybe be addressed by having an ILL config dedicated admin page where all ILL related configs would be accessible through that.
Comment 10 Pedro Amorim 2024-02-21 16:03:43 UTC
Adding others to this bug to have more eyes on it.
If someone else SOs feel free to convert my SO to PQA.
Comment 11 Martin Renvoize 2024-02-29 10:48:39 UTC
Created attachment 162590 [details] [review]
Bug 35151: Convert ILLModuleCopyrightClearance system preference to additional contents

This patch moves the ILLModuleCopyrightClearance system preference into
HTML customizations, making it possible to have language- and
library-specific content.

To test you should have some content in the ILLModuleCopyrightClearance
system preference before applying the patch. Apply the patch, run the
database update process, and rebuild the OPAC CSS.

- In the staff client, go to Tools -> HTML customizations and verify
  that the content from ILLModuleCopyrightClearance is now stored there.
- The HTML customization entry form should offer
  ILLModuleCopyrightClearance as a choice under "Display location."
- Update and reinstall active translations (for instance fr-FR):
  - perl misc/translator/translate update fr-FR
  - perl misc/translator/translate install fr-FR
- Enable the translation if necessary under Administration -> System
  preferences -> language.
- Enable the "opaclanguagesdisplay" preference if necessary.
- Edit the ILLModuleCopyrightClearance HTML customization and add unique
  content to the "fr-FR" tab.

- Enable the "ILLModule" and "ILLModuleUnmediated" system preferences if
  necessary. You must have at least one ILL backend installed.
- Log into the OPAC and click the "Interlibrary loan requests" in the
  sidebar menu on the user summary page.
- Click "Create new request."
- You should be taken to a page with your ILLModuleCopyrightClearance
  content shown along with "Yes" and "No" buttons.
  - Clicking "Yes" should take you to the form for creating a new
    request.
- Switch to your updated translation and confirm that the content
  appears correctly.
- Remove all "ILLModuleCopyrightClearance" html customizations. Test the
  "Create new request" process in the OPAC again.
  - There should be no confirmation step before arriving at the form for
    creating a new ILL request.
- Go to Administration -> System preferences and search for
  "ILLModuleCopyrightClearance." It should return no results.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2024-02-29 10:49:52 UTC
Adding my own testing here and upgrading Pedro to a PQA.

I would love to have seen some unit tests, but alas I think that's a little more involved given where this touches. Current unit tests are all passing and the QA script is happy.

PQA
Comment 13 Katrin Fischer 2024-04-19 17:08:57 UTC
You missed removing the system preference from sysprefs.sql. I'll provide a follow-up for that.
Comment 14 Katrin Fischer 2024-04-19 17:55:57 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 15 Katrin Fischer 2024-04-19 20:04:00 UTC
We also missed removing the ILLModuleCopyrightClearance preference from the YAML files. Please fix!
Comment 16 Katrin Fischer 2024-04-22 06:41:25 UTC
(In reply to Katrin Fischer from comment #15)
> We also missed removing the ILLModuleCopyrightClearance preference from the
> YAML files. Please fix!

I've written the follow-up.
Comment 17 Fridolin Somers 2024-05-23 14:09:12 UTC
Not backported to 23.11.x