Summary: | Placing holds on OPAC broken | ||
---|---|---|---|
Product: | Koha | Reporter: | Katrin Fischer <katrin.fischer> |
Component: | OPAC | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | blocker | ||
Priority: | P5 - low | CC: | fridolin.somers, jonathan.druart, kyle, lucas, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
22.05.00
|
|
Bug Depends on: | 29844 | ||
Bug Blocks: | |||
Attachments: |
Bug 30089: Fix placing holds on OPAC after Bug 29844
Bug 30089: Fix placing holds on OPAC after Bug 29844 [ALTERNATIVE PATCH] Bug 30089: (bug 29844 follow-up) Fix placing holds - OPAC Bug 30089: (bug 29844 follow-up) Fix placing holds - OPAC Bug 30089: (bug 29844 follow-up) Fix placing holds - OPAC |
Description
Katrin Fischer
2022-02-13 13:03:36 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. I'm on it 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)' I'm not 100% sure of test plan. Feel free to amend. 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. > > 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 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)' (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' 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! 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. 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> (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. I agree 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> Pushed to master for 22.05, thanks to everybody involved 🦄 |