|
Description
Martin Renvoize (ashimema)
2018-04-23 15:47:38 UTC
Created attachment 74806 [details] [review] Bug 20639: Add ILLOpacbackends syspref This adds the ILLOpacbackends syspref, allowing users to refine the ill backends available to opac users for initiating ill requests Created attachment 74811 [details] [review] Bug 20639: Add ILLOpacbackends syspref This adds the ILLOpacbackends syspref, allowing users to refine the ill backends available to opac users for initiating ill requests Comment on attachment 74811 [details] [review] Bug 20639: Add ILLOpacbackends syspref Obsoleted due to updated patch Created attachment 75165 [details] [review] Bug 20639: Add ILLOpacbackends syspref This adds the ILLOpacbackends syspref, allowing users to refine the ill backends available to opac users for initiating ill requests Remove default assignment for backends We don't need a default assignment for the ILLOpacbackends assignment, if the pref isn't set, it returns undef anyway. Also, having this default assignment actually breaks the fetching of the preference Created attachment 78119 [details] [review] Bug 20639: Add ILLOpacbackends syspref This adds the ILLOpacbackends syspref, allowing users to refine the ill backends available to opac users for initiating ill requests Remove default assignment for backends We don't need a default assignment for the ILLOpacbackends assignment, if the pref isn't set, it returns undef anyway. Also, having this default assignment actually breaks the fetching of the preference Created attachment 79033 [details] [review] Bug 20639: (follow-up) Fix variable not passed available_backends expects a variable declaring the contents of the ILLopacbackends syspref. This wasn't being passed in all calls to it, so I've removed the requirement for it being passed, we now check the contents of the syspref in available_backends itselfBug To test: 1. Ensure you have at least two ILL backends installed 2. From the OPAC user's home page, choose "your interlibrary loan requests" 3. Click the "Create a new request" button 4. TEST: Observe that all backends are available 5. As a user with sufficient privileges, go to the System Preferences 6. Search the privileges for "interlibrary" 7. TEST: Observe that a "ILLOpacbackends" preference is available 8. Enter the name of *one* of the installed backends in the input box for "ILLOpacbackends", for example BLDSS 9. From the OPAC user's home page, choose "your interlibrary loan requests" 10. TEST: Observe that the "Create a new request" button is no longer a select box 11. TEST: Observe that clicking the "Create a new request" button takes you straight to the form for creating a request with the single available backend 12. As a user with sufficient privileges, go to the System Preferences 13. Search the privileges for "interlibrary" 14. Enter the name of *two* of the installed backends in the input box for "ILLOpacbackends", separated by a | character for example: BLDSS|Koha 15. From the OPAC user's home page, choose "your interlibrary loan requests" 16. TEST: Observe that the "Create a new request" button is now a select box 17. TEST: Observe that clicking the "Create a new request" button only offers the backends specified in step 14 Created attachment 79474 [details] [review] Bug 20639: (follow-up) Fix population of backends The OPAC view wasn't correctly restricting the display of backends when the ILLOpacbackends preference was set. See the test plan on comment 7 Created attachment 79477 [details] [review] Bug 20639: Add ILLOpacbackends syspref This adds the ILLOpacbackends syspref, allowing users to refine the ill backends available to opac users for initiating ill requests Remove default assignment for backends We don't need a default assignment for the ILLOpacbackends assignment, if the pref isn't set, it returns undef anyway. Also, having this default assignment actually breaks the fetching of the preference Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Created attachment 79478 [details] [review] Bug 20639: (follow-up) Fix population of backends The OPAC view wasn't correctly restricting the display of backends when the ILLOpacbackends preference was set. See the test plan on comment 7 Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Created attachment 84932 [details] [review] Bug 20639: Add ILLOpacbackends syspref This adds the ILLOpacbackends syspref, allowing users to refine the ill backends available to opac users for initiating ill requests Remove default assignment for backends We don't need a default assignment for the ILLOpacbackends assignment, if the pref isn't set, it returns undef anyway. Also, having this default assignment actually breaks the fetching of the preference Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 84933 [details] [review] Bug 20639: (follow-up) Fix population of backends The OPAC view wasn't correctly restricting the display of backends when the ILLOpacbackends preference was set. See the test plan on comment 7 Signed-off-by: Niamh.Walker-Headon@it-tallaght.ie Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 84934 [details] [review] Bug 20639: (QA follow-up) Rephrase system preference Rephrases system preference a tiny bit and explaining case of leaving the pref empty. This works as described but doesn't cover one use case: A library that doens't want to allow OPAC initiated requests. Something to think about later maybe? On second thought - Andrew, can we have some unit tests for testing the new behaviour of the method please? (In reply to Katrin Fischer from comment #14) > This works as described but doesn't cover one use case: A library that > doens't want to allow OPAC initiated requests. Something to think about > later maybe? Yes, indeed. Having ILL enabled only for the staff interface is definitely outside the scope of this bug I think. (In reply to Katrin Fischer from comment #14) > This works as described but doesn't cover one use case: A library that > doens't want to allow OPAC initiated requests. Something to think about > later maybe? I have now added Bug 22305 to record this (In reply to Katrin Fischer from comment #15) > On second thought - Andrew, can we have some unit tests for testing the new > behaviour of the method please? Hi Katrin - The method in question "available_backends" has never had tests due to it needing at least one backend installing in the filesystem in order to run, it looks at the directory specified in the config and enumerates the backends it finds. I think we should let the tests slip for this one for a couple of reasons. 1) It wouldn't be trivial to write the tests, adding and removing files as part of a mocking process which could be fraught with permissions issues. 2) We're still aiming to shift to Koha::Plugins infrastructure for backends in which case this call would likely be replaced by the use of GetPlugins or similar from that area of code. I will write a bug report for that piece and note/link it to this, but for now I certainly think we can get away without one and let the tree that's hanging of this continue to shrink. Awesome work all! Pushed to master for 19.05 Enhancement, will not be backported to 18.11.x series This should be in master, but it looks like it is not pushed! Oops! Actually , pushed to master for 19.05 Enhancement, will not be backported to 18.11.x series. |