Bug 41237

Summary: OPAC created requests ignore library selection, always default to patron's library
Product: Koha Reporter: Pedro Amorim <pedro.amorim>
Component: ILLAssignee: Pedro Amorim <pedro.amorim>
Status: Pushed to main --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: katrin.fischer, lisette, lucas, m.de.rooy, martin.renvoize, oleonard, pedro.amorim, s.graham4, tomascohen
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes a bug on the OPAC create ILL request form which was always setting the library to the patron's library, ignoring the library selection made on the form.
Version(s) released in:
26.05.00
Circulation function:
Bug Depends on: 35151    
Bug Blocks:    
Attachments: Bug 41237: Update fetch of patron's branchcode to search additional_contents by
Bug 41237: Add tests
Bug 41237: Update fetch of patron's branchcode to search additional_contents by
Bug 41237: Add tests
[24.11.x] Bug 41237: Update fetch of patron's branchcode to search additional_contents by
Bug 41237: (QA follow-up): Utilize available logged in user object
Bug 41237: Update fetch of patron's branchcode to search additional_contents by
Bug 41237: Add tests
Bug 41237: (QA follow-up): Utilize available logged in user object

Description Pedro Amorim 2025-11-11 15:02:32 UTC

    
Comment 1 Pedro Amorim 2025-11-11 15:09:59 UTC
I believe this regression was introduced by bug 35151, by setting the $params->{branchcode} to the patron's branchcode, overwriting the form selection.
The intent of that code was so that the patron's branchcode would then be used as a search parameter for the respective's library ILLModuleCopyrightClearance additional contents, but caused this unintended side effect. 

To reproduce:
1) Enable ILLModule
2) Logged in as koha:koha, create an OPAC request, visit:
<opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard
3) Pick any library not Centerville (the default library of the 'koha' user)
4) Notice the created request's library is CPL (Centerville). It ignored the library selection from the form.
Comment 2 Pedro Amorim 2025-11-11 15:27:52 UTC
Created attachment 189491 [details] [review]
Bug 41237: Update fetch of patron's branchcode to search additional_contents by
Comment 3 Pedro Amorim 2025-11-11 15:27:55 UTC
Created attachment 189492 [details] [review]
Bug 41237: Add tests

Test plan, k-t-d:
1) Apply tests patch, run:
prove t/db_dependent/Koha/ILL/Requests.t
2) Notice it fails. Apply the second patch. Repeat. Notice it passes.

Test failure explanation (before fix patch):
    #   Failed test 'Cardnumber not supplied and couldnt find additional_contents for all libraries. Dont show copyrightclearance'
    #   at t/db_dependent/Koha/ILL/Requests.t line 1067.
    #          got: 'copyrightclearance'
    #     expected: 'commit'

Its searching the copyrightclearance additional_contents for the provided branchcode param (illrq->patron->branchcode) (and finding one) when it shouldn't. That parameter is only to be utilized for the newly created ILL request's library. The branchcode filter of the additional_contents must the be logged in user's cardnumber, not params->{branchcode}. If no logged in user exists and this stage is reached (ILLOpacUnauthenticatedRequest), then only 'All libraries' ILLModuleCopyrightClearance are to be fetched, as expected.

Human test plan, before applying patches:
1) Enable ILLModule
2) Logged in as koha:koha, create an OPAC request, visit:
<opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard
3) Pick any library not Centerville (the default library of the 'koha' user)
4) Notice the created request's library is CPL (Centerville). It ignored the library selection from the form.
5) Apply patches. Repeat. Notice the request's library is now the one that was selected on the form.
Comment 4 Pedro Amorim 2025-11-11 17:04:07 UTC
Created attachment 189493 [details] [review]
Bug 41237: Update fetch of patron's branchcode to search additional_contents by

Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk>
Comment 5 Pedro Amorim 2025-11-11 17:04:10 UTC
Created attachment 189494 [details] [review]
Bug 41237: Add tests

Test plan, k-t-d:
1) Apply tests patch, run:
prove t/db_dependent/Koha/ILL/Requests.t
2) Notice it fails. Apply the second patch. Repeat. Notice it passes.

Test failure explanation (before fix patch):
    #   Failed test 'Cardnumber not supplied and couldnt find additional_contents for all libraries. Dont show copyrightclearance'
    #   at t/db_dependent/Koha/ILL/Requests.t line 1067.
    #          got: 'copyrightclearance'
    #     expected: 'commit'

Its searching the copyrightclearance additional_contents for the provided branchcode param (illrq->patron->branchcode) (and finding one) when it shouldn't. That parameter is only to be utilized for the newly created ILL request's library. The branchcode filter of the additional_contents must the be logged in user's cardnumber, not params->{branchcode}. If no logged in user exists and this stage is reached (ILLOpacUnauthenticatedRequest), then only 'All libraries' ILLModuleCopyrightClearance are to be fetched, as expected.

Human test plan, before applying patches:
1) Enable ILLModule
2) Logged in as koha:koha, create an OPAC request, visit:
<opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard
3) Pick any library not Centerville (the default library of the 'koha' user)
4) Notice the created request's library is CPL (Centerville). It ignored the library selection from the form.
5) Apply patches. Repeat. Notice the request's library is now the one that was selected on the form.

Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk>
Comment 6 Pedro Amorim 2025-11-12 14:15:05 UTC
Created attachment 189514 [details] [review]
[24.11.x] Bug 41237: Update fetch of patron's branchcode to search additional_contents by

Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk>
Comment 7 Marcel de Rooy 2025-12-05 07:46:42 UTC
Somehow this change is confusing.
Shouldnt we just check if we had a value in $params->{branchcode} instead of looking up a patron while we could simply pass him from the script.

If there was no value in $params->{branchcode}, you should not look for undef.
So
-                library_id => $params->{branchcode},
This should be something like
+                $params->{branchcode} ? ( library_id => $params->{branchcode} ) : (),

Am I missing something?
Comment 8 Pedro Amorim 2025-12-09 14:43:49 UTC
(In reply to Marcel de Rooy from comment #7)
> Somehow this change is confusing.
> Shouldnt we just check if we had a value in $params->{branchcode} instead of
> looking up a patron while we could simply pass him from the script.
> 
> If there was no value in $params->{branchcode}, you should not look for
> undef.
> So
> -                library_id => $params->{branchcode},
> This should be something like
> +                $params->{branchcode} ? ( library_id =>
> $params->{branchcode} ) : (),
> 
> Am I missing something?

Thanks for looking Marcel. I don't believe so as the correct ILLModuleCopyrightClearance AdditionalContents should be retrieved based on the patron's library, not the library selected from the form itself.

What I think should happen (and is happening with this patchset):
1) Patron from library A visits the ILL request form to create a new request at:
<opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard
2) Patron is presented the ILLModuleCopyrightClearance created for their library (if any)
3) If 2 is empty, patron is presented ILLModuleCopyrightClearance created for 'All libraries' (if any) [this is where search for undef is working as intended] 
4) If no ILLModuleCopyrightClearance retrieved from 2 or 3, skip it entirely and show form instead.
5) Patron is presented the form, they pick destination library B. Library B is correctly set to that ILL request's library.

This is my interpretation of the issue, apologies in advance if I misunderstood your comment.
Comment 9 Marcel de Rooy 2025-12-10 13:38:54 UTC
OLD 
If you had $patron, pass branchcode and cardnumber [WITHOUT using it btw] to backend_create. In that sub always look for AddCont on library_id => $params->{branchcode} so patron branch OR undefined (all libs).
NEW
Pass cardnumber but do NO LONGER pass branchcode.
ONLY look for AddCont with library_id if you got patron->branchcode via cardnumber. So you never look at library_id undefined (all libs); instead you accept copyright clearance from any branch when there is no patron.
QA COMMENT
It seems odd to look up cardnumber in create_backend if you can pass $patron from the caller.
Although I am not familiar with this workflow, the algorithm does not feel good.

Yeah I could be mistaken too :)
Comment 10 Pedro Amorim 2026-01-07 14:27:31 UTC
Created attachment 190990 [details] [review]
Bug 41237: (QA follow-up): Utilize available logged in user object

Instead of re-fetching the patron object.

This also adjusts tests:
prove t/db_dependent/Koha/ILL/Requests.t
Comment 11 Pedro Amorim 2026-01-07 14:31:55 UTC
(In reply to Marcel de Rooy from comment #9)
> OLD 
> If you had $patron, pass branchcode and cardnumber [WITHOUT using it btw] to
> backend_create.

backend_create does not use it directly, but delegates it to the specific backend being utilized:

    my $args = {
        request => $self,
        other   => $params,
    };
    my $result = $self->_backend->create($args);
    
The backend itself will then make use of it. Take a look at Koha/ILL/Backend/Standard::create for example, which then calls add_request that then does use it to effectively insert the request in the database.

> In that sub always look for AddCont on library_id =>
> $params->{branchcode} so patron branch OR undefined (all libs).

This is correct, and highlights the problem exactly. It worked as intended for that purpose, but had the side-effect of overwriting the existing $params->{branchcode} which is the branchcode that is selected on the form and may or may not be the logged in patron's branchcode.

> NEW
> Pass cardnumber but do NO LONGER pass branchcode.

branchcode is still passed, as was before. It's in $params->{branchcode}, but now is the one submitted in the form by the user, not being overwritten.

> ONLY look for AddCont with library_id if you got patron->branchcode via
> cardnumber. So you never look at library_id undefined (all libs); instead
> you accept copyright clearance from any branch when there is no patron.

I understand that the code reads as such, but the way AdditionalContents->search_for_display works is that it sets the search on library_id = undef if the parameter is omited. I believe this behavior was introduced in bug 29332.
Additionally, this potential regression would be caught by the tests written in this patchset, one way to prove it is by reverting the change from bug 29332 and running t/db_dependent/Koha/ILL/Requests.t (with patches applied - QA followup). It'll cause the tests to fail.

> QA COMMENT
> It seems odd to look up cardnumber in create_backend if you can pass $patron
> from the caller.

Agreed, follow-up patch submitted.
Comment 12 Marcel de Rooy 2026-01-16 10:46:29 UTC
Hi! Still on my list. Coming back soon.
Comment 13 Marcel de Rooy 2026-01-20 13:12:07 UTC
Created attachment 191691 [details] [review]
Bug 41237: Update fetch of patron's branchcode to search additional_contents by

Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2026-01-20 13:12:09 UTC
Created attachment 191692 [details] [review]
Bug 41237: Add tests

Test plan, k-t-d:
1) Apply tests patch, run:
prove t/db_dependent/Koha/ILL/Requests.t
2) Notice it fails. Apply the second patch. Repeat. Notice it passes.

Test failure explanation (before fix patch):
    #   Failed test 'Cardnumber not supplied and couldnt find additional_contents for all libraries. Dont show copyrightclearance'
    #   at t/db_dependent/Koha/ILL/Requests.t line 1067.
    #          got: 'copyrightclearance'
    #     expected: 'commit'

Its searching the copyrightclearance additional_contents for the provided branchcode param (illrq->patron->branchcode) (and finding one) when it shouldn't. That parameter is only to be utilized for the newly created ILL request's library. The branchcode filter of the additional_contents must the be logged in user's cardnumber, not params->{branchcode}. If no logged in user exists and this stage is reached (ILLOpacUnauthenticatedRequest), then only 'All libraries' ILLModuleCopyrightClearance are to be fetched, as expected.

Human test plan, before applying patches:
1) Enable ILLModule
2) Logged in as koha:koha, create an OPAC request, visit:
<opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard
3) Pick any library not Centerville (the default library of the 'koha' user)
4) Notice the created request's library is CPL (Centerville). It ignored the library selection from the form.
5) Apply patches. Repeat. Notice the request's library is now the one that was selected on the form.

Signed-off-by: Stephen Graham <s.graham4@herts.ac.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2026-01-20 13:12:11 UTC
Created attachment 191693 [details] [review]
Bug 41237: (QA follow-up): Utilize available logged in user object

Instead of re-fetching the patron object.

This also adjusts tests:
prove t/db_dependent/Koha/ILL/Requests.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Lucas Gass (lukeg) 2026-01-20 14:37:14 UTC
Nice work everyone!

Pushed to main for 26.05
Comment 17 Lucas Gass (lukeg) 2026-01-20 14:38:08 UTC
please add release notes