|
Description
Pedro Amorim
2025-11-11 15:02:32 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. Created attachment 189491 [details] [review] Bug 41237: Update fetch of patron's branchcode to search additional_contents by 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. 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> 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> 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> 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?
(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. 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 :)
|