Bug 30089 - Placing holds on OPAC broken
Summary: Placing holds on OPAC broken
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 29844
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-13 13:03 UTC by Katrin Fischer
Modified: 2022-12-12 21:23 UTC (History)
5 users (show)

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


Attachments
Bug 30089: Fix placing holds on OPAC after Bug 29844 (2.84 KB, patch)
2022-02-14 21:45 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30089: Fix placing holds on OPAC after Bug 29844 (2.85 KB, patch)
2022-02-14 22:50 UTC, Fridolin Somers
Details | Diff | Splinter Review
[ALTERNATIVE PATCH] Bug 30089: (bug 29844 follow-up) Fix placing holds - OPAC (1.15 KB, patch)
2022-02-15 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30089: (bug 29844 follow-up) Fix placing holds - OPAC (1.21 KB, patch)
2022-02-16 11:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30089: (bug 29844 follow-up) Fix placing holds - OPAC (1.26 KB, patch)
2022-02-18 15:17 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2022-02-13 13:03:36 UTC
When trying to place a hold in the OPAC, either multi hold or a single hold, the page explodes with:

The method Koha::AuthorisedValues->authorised_value is not covered by tests!

I am using ktd, the sample database and my search term is 'perl'.
Comment 1 Lucas Gass 2022-02-14 21:05:15 UTC
I recreate this also. Seems like it is caused by Bug 29844. 

When I revert commit 4ae2f6d9d8addd4e7fb8480ea0c41adfaa0febb0 I can again place a hold on the OPAC.
Comment 2 Fridolin Somers 2022-02-14 21:11:38 UTC
I'm on it
Comment 3 Fridolin Somers 2022-02-14 21:45:26 UTC
Created attachment 130581 [details] [review]
Bug 30089: Fix placing holds on OPAC after Bug 29844

When trying to place a hold in the OPAC, either multi hold or a single hold, the page explodes with:
The method Koha::AuthorisedValues->authorised_value is not covered by tests!

We could fix by adding ->as_list.
This patch fixes by using
Koha::AuthorisedValues->get_description_by_koha_field on each item
instead of Koha::AuthorisedValues->search_by_koha_field list.
Performance should be OK because this method as cache.
This is used a lot in staff interface.

Test plan :
1) In staff interface find a record with several items available
   For example in KTD biblionumer=126
2) Edit on item and change 'not for loan' to a non-zero value
   For example in KTD $7 = Staff Collection
3) Go to OPAC on this record
4) Place hold
5) Click on 'A specific item'
=> Check you see not for loan authorised value description
   For example in KTD '(Staff Collection)'
Comment 4 Fridolin Somers 2022-02-14 21:46:30 UTC
I'm not 100% sure of test plan.
Feel free to amend.
Comment 5 Katrin Fischer 2022-02-14 22:04:56 UTC
Comment on attachment 130581 [details] [review]
Bug 30089: Fix placing holds on OPAC after Bug 29844

Review of attachment 130581 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-reserve.pl
@@ +530,5 @@
> +                    kohafield        => 'items.notforloan',
> +                    authorised_value => $itemLoopIter->{notforloan}
> +                }
> +            );
> +            $itemLoopIter->{notforloanvalue} = $notforloan_av->{opac_description} // '';

I believe this line is not right - if OPAC description hasn't been set, the normal description should be used.
Comment 6 Fridolin Somers 2022-02-14 22:17:49 UTC
> 
> I believe this line is not right - if OPAC description hasn't been set, the
> normal description should be used.
Correct.

But this would change the current behavior no ?

Also isn't it a way to hide a value from OPAC an see it only on staff interface ?

TT plugin does the fallback in GetDescriptionByKohaField :
https://git.koha-community.org/Koha-community/Koha/src/commit/65cc721e9f90fea4d0eb3d2b85831b60f3046ce6/Koha/Template/Plugin/AuthorisedValues.pm#L76

But not in GetByCode :
https://git.koha-community.org/Koha-community/Koha/src/commit/65cc721e9f90fea4d0eb3d2b85831b60f3046ce6/Koha/Template/Plugin/AuthorisedValues.pm#L27
Comment 7 Fridolin Somers 2022-02-14 22:50:33 UTC
Created attachment 130583 [details] [review]
Bug 30089: Fix placing holds on OPAC after Bug 29844

When trying to place a hold in the OPAC, either multi hold or a single hold, the page explodes with:
The method Koha::AuthorisedValues->authorised_value is not covered by tests!

We could fix by adding ->as_list.
This patch fixes by using
Koha::AuthorisedValues->get_description_by_koha_field on each item
instead of Koha::AuthorisedValues->search_by_koha_field list.
Performance should be OK because this method as cache.
This is used a lot in staff interface.

Test plan :
1) In staff interface find a record with several items available
   For example in KTD biblionumer=126
2) Edit on item and change 'not for loan' to a non-zero value
   For example in KTD $7 = Staff Collection
3) Go to OPAC on this record
4) Place hold
5) Click on 'A specific item'
=> Check you see not for loan authorised value description
   For example in KTD '(Staff Collection)'
Comment 8 Fridolin Somers 2022-02-14 22:52:09 UTC
(In reply to Fridolin Somers from comment #6)
> > 
> > I believe this line is not right - if OPAC description hasn't been set, the
> > normal description should be used.
> Correct.
> 
> But this would change the current behavior no ?
> 
> Also isn't it a way to hide a value from OPAC an see it only on staff
> interface ?
> 
> TT plugin does the fallback in GetDescriptionByKohaField :
> https://git.koha-community.org/Koha-community/Koha/src/commit/
> 65cc721e9f90fea4d0eb3d2b85831b60f3046ce6/Koha/Template/Plugin/
> AuthorisedValues.pm#L76
> 
> But not in GetByCode :
> https://git.koha-community.org/Koha-community/Koha/src/commit/
> 65cc721e9f90fea4d0eb3d2b85831b60f3046ce6/Koha/Template/Plugin/
> AuthorisedValues.pm#L27

OK forget it
https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/AuthorisedValue.pm#L43
'opac_description' is a method it contains the fallback, db field is 'lib_opac'
Comment 9 Jonathan Druart 2022-02-15 07:43:43 UTC
Created attachment 130592 [details] [review]
[ALTERNATIVE PATCH] Bug 30089: (bug 29844 follow-up) Fix placing holds - OPAC

The method Koha::AuthorisedValues->authorised_value is not covered by tests!
Comment 10 Jonathan Druart 2022-02-15 07:44:20 UTC
Fridolin, I would prefer to fix it like we do with the other occurrences and make sure we won't introduce another regression. It could impact performance if there is a lot of items.
Comment 11 Tomás Cohen Arazi 2022-02-16 11:16:36 UTC
Created attachment 130659 [details] [review]
Bug 30089: (bug 29844 follow-up) Fix placing holds - OPAC

The method Koha::AuthorisedValues->authorised_value is not covered by tests!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2022-02-16 11:18:08 UTC
(In reply to Jonathan Druart from comment #10)
> Fridolin, I would prefer to fix it like we do with the other occurrences and
> make sure we won't introduce another regression. It could impact performance
> if there is a lot of items.

I agree with this. Further refactorings can be done on their own bugs so we can discuss implementation details.
Comment 13 Fridolin Somers 2022-02-16 20:24:17 UTC
I agree
Comment 14 Nick Clemens 2022-02-18 15:17:02 UTC
Created attachment 130863 [details] [review]
Bug 30089: (bug 29844 follow-up) Fix placing holds - OPAC

The method Koha::AuthorisedValues->authorised_value is not covered by tests!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Fridolin Somers 2022-02-22 01:15:00 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄