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'.
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 [U+1F984]