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?

Adding our ILL topic expert :)

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?

(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.

I'll be looking into this when I get the chance, it's in my pile!

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>

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.

Adding others to this bug to have more eyes on it.
If someone else SOs feel free to convert my SO to PQA.

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>

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

You missed removing the system preference from sysprefs.sql. I'll provide a follow-up for that.

Pushed for 24.05!

Well done everyone, thank you!

We also missed removing the ILLModuleCopyrightClearance preference from the YAML files. Please fix!

(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.

Not backported to 23.11.x