|
Description
Mark Tompsett
2013-07-15 10:57:56 UTC
Created attachment 19647 [details] [review] Provide two system preferences to flexibly override OpacHiddenItems Test ==== 1) Add PTSB and PTEB as patron categories. 2) Create a dummy user set as regular patron (PT). 3) Make sure you have a biblio that has at least a single item in two branches. (e.g. Lord of the Rings in Manila and Davao) 4) Modify OpacHiddenItems to hide one of the two branches. 5) Enable OPAC -> Features -> opacuserlogin 6) Do an anonymous OPAC search for the item. It should only list one of the two branches. 7) Log in to OPAC. Search for item. It should only list one of the two branches. 8) Apply patch 9) Change dummy user to match the branch of the hidden item. 10) Change dummy user patron category to PTSB. 11) Log in to OPAC. Search for item. It should list all the two branches items. 12) Change dummy user to not match the branch of the hidden item. 13) Log in to OPAC. Search for item. It should list only one of the two branches. 14) Change the dummy user patron category to PTEB. 15) Log in to OPAC. Search for item. It should list all the two branches items. Created attachment 19708 [details] [review] Based on patron category unhide items hidden with OpacHiddenItems Ran into a test case that blew up, corrected for this case. Paste in a opac-detail url for a hidden item in a fresh browser. Created attachment 19931 [details] [review] Uses patron category to keep visible some things hidden by OPACHiddenItems This patch will: 1) Add two system preferences: PatronSingleBranch and PatronEveryBranch. 2) Items which would normally be hidden by OpacHiddenItems will be visible if: a) The patron's category code matches the PatronSingleBranch patron category code AND the home branch of the item is the same as the home branch of the patron. OR b) The patron's category code matches the PatronEveryBranch patron category code. The two system preferences default to PTSB and PTEB which are not added to the list of possible patron categories. This means OpacHiddenItems will continue to function as is normally. Having them as system preferences means the patron category code is not hard coded and can be changed. my $categorycode = C4::Members::GetBorrowerCategorycode()... should go in the if() block above, and be conditioned on the $borrowernumber as well. Conditioning on the borrowernumber seems reasonable, but it doesn't belong in the C4::Content->userenv check, because it looks it up in the DB. Revised and improved patch coming. Wizzyrea requested pipe delimited category codes, so that Teachers and Librarians could both seen hidden items, without having to group them into the same patron category code. Created attachment 19932 [details] [review] Using list of category codes OPACHiddenItems can be overridden This patch improvement overrides OPACHiddenItems based on potentially multiple patron category codes in the PatronSingleBranch and PatronEveryBranch system preferences which get added. Created attachment 19933 [details] [review] Using list of category codes OPACHiddenItems can be overridden Oops. Need to remember to test before posting patches. :) Sorry, now another one: grep( /^$categorycode$/, @SingleBranchCodes ) should be replaced with any() (and @EveryBranchCodes too) Created attachment 19949 [details] [review] Provide two system preferences to flexibly override OpacHiddenItems As per Srdjan Jankovic's suggestion to use any instead of grep. The patch has been tweaked accordingly. IT works and I will sign it of, however I need to raise two things (should have done earlier, but occurred to me when testing): 1. Not sure if it is appropriate to put default values for those sysprefs 2. This should really go to issuing rules. OpacHiddenItems is just horrible. I understand the grief, maybe create another bug request for moving those sysprefs to issuingrules. If done properly, it should obsolete OpacHiddenItems in most cases. Created attachment 19954 [details] [review] [SIGNED-OFF] Bug 10589 - Override OpacHiddenItems based on Patron Category Two system preferences, PatronSingleBranch and PatronEveryBranch, have been added to facilitate some granularity on the override. If the home branch of the item and the patron match and the catgegory code of the patron matches one of the pipe delimited values in the PatronSingleBranch patron category code, then an item which would normally be hidden will be visible. If the patron category matches one of the pipe delimited values in the PatronEveryBranch patron category, then every item hidden by OpacHiddenItems will be visible. Since the patron category codes used by default for these two system preferences are not added, OpacHiddenItems will continue function as expected until individual patrons and system preferences are configured. There is no need to provide this functionality to staff, as they have access to the staff client. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Comment on attachment 19949 [details] [review] Provide two system preferences to flexibly override OpacHiddenItems Obsoleted the patch which is not signed off. Created attachment 21063 [details] [review] Bug 10589 - Override OpacHiddenItems based on Patron Category Two system preferences, PatronSingleBranch and PatronEveryBranch, have been added to facilitate some granularity on the override. If the home branch of the item and the patron match and the catgegory code of the patron matches one of the pipe delimited values in the PatronSingleBranch patron category code, then an item which would normally be hidden will be visible. If the patron category matches one of the pipe delimited values in the PatronEveryBranch patron category, then every item hidden by OpacHiddenItems will be visible. Since the patron category codes used by default for these two system preferences are not added, OpacHiddenItems will continue function as expected until individual patrons and system preferences are configured. There is no need to provide this functionality to staff, as they have access to the staff client. Rebased to current master. Comment on attachment 19954 [details] [review] [SIGNED-OFF] Bug 10589 - Override OpacHiddenItems based on Patron Category Rebased because the signed off version didn't apply easily any more, and I really want this in 3.14 :) Created attachment 21066 [details] [review] Bug 10589 - Override OpacHiddenItems based on Patron Category Two system preferences, PatronSingleBranch and PatronEveryBranch, have been added to facilitate some granularity on the override. If the home branch of the item and the patron match and the catgegory code of the patron matches one of the pipe delimited values in the PatronSingleBranch patron category code, then an item which would normally be hidden will be visible. If the patron category matches one of the pipe delimited values in the PatronEveryBranch patron category, then every item hidden by OpacHiddenItems will be visible. Since the patron category codes used by default for these two system preferences are not added, OpacHiddenItems will continue function as expected until individual patrons and system preferences are configured. There is no need to provide this functionality to staff, as they have access to the staff client. Rebased to current master. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 21256 [details] [review] Bug 10589 - Override OpacHiddenItems based on Patron Category Two system preferences, PatronSingleBranch and PatronEveryBranch, have been added to facilitate some granularity on the override. If the home branch of the item and the patron match and the catgegory code of the patron matches one of the pipe delimited values in the PatronSingleBranch patron category code, then an item which would normally be hidden will be visible. If the patron category matches one of the pipe delimited values in the PatronEveryBranch patron category, then every item hidden by OpacHiddenItems will be visible. Since the patron category codes used by default for these two system preferences are not added, OpacHiddenItems will continue function as expected until individual patrons and system preferences are configured. There is no need to provide this functionality to staff, as they have access to the staff client. Rebased to current master (again). Patch applied cleanly, go forth and signoff I put the tests into a separate bug, because at the time, this was signed off. Sadly, I had to rebase. See bug 10895 for a test file, which depends on bug 10900. Created attachment 21620 [details] [review] Bug 10589 - Override OpacHiddenItems based on Patron Category Two system preferences, PatronSingleBranch and PatronEveryBranch, have been added to facilitate some granularity on the override. If the home branch of the item and the patron match and the catgegory code of the patron matches one of the pipe delimited values in the PatronSingleBranch patron category code, then an item which would normally be hidden will be visible. If the patron category matches one of the pipe delimited values in the PatronEveryBranch patron category, then every item hidden by OpacHiddenItems will be visible. Since the patron category codes used by default for these two system preferences are not added, OpacHiddenItems will continue function as expected until individual patrons and system preferences are configured. There is no need to provide this functionality to staff, as they have access to the staff client. Rebased to current master (again). Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 22587 [details] [review] Bug 10589 - Override OpacHiddenItems based on Patron Category Two system preferences, PatronSingleBranch and PatronEveryBranch, have been added to facilitate some granularity on the override. If the home branch of the item and the patron match and the catgegory code of the patron matches one of the pipe delimited values in the PatronSingleBranch patron category code, then an item which would normally be hidden will be visible. If the patron category matches one of the pipe delimited values in the PatronEveryBranch patron category, then every item hidden by OpacHiddenItems will be visible. Since the patron category codes used by default for these two system preferences are not added, OpacHiddenItems will continue function as expected until individual patrons and system preferences are configured. There is no need to provide this functionality to staff, as they have access to the staff client. Rebased to current master (again). Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Rebased again. Created attachment 22629 [details] [review] Bug 10589 - Override OpacHiddenItems based on Patron Category Two system preferences, PatronSingleBranch and PatronEveryBranch, have been added to facilitate some granularity on the override. If the home branch of the item and the patron match and the catgegory code of the patron matches one of the pipe delimited values in the PatronSingleBranch patron category code, then an item which would normally be hidden will be visible. If the patron category matches one of the pipe delimited values in the PatronEveryBranch patron category, then every item hidden by OpacHiddenItems will be visible. Since the patron category codes used by default for these two system preferences are not added, OpacHiddenItems will continue function as expected until individual patrons and system preferences are configured. There is no need to provide this functionality to staff, as they have access to the staff client. Rebased to current master (again). Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> QA Comment: Code looks good to me. Works as advertised. No complaints from qa tools. Passed QA Just some smaller points (no blockers imo): 1) No unit tests. Adding these makes the chance of a push considerably larger :) 2) The names of the prefs are not completely explanatory on itself. The name does not give a clue that this pref has to do with unhiding items for some patron categories. 3) The dbrev for install and upgrade contains empty string for the systempreferences fields options and type. This works, but imo it should be better (more consistent) to set type to Free and options to NULL. (Please have a look at other prefs.) 4) To make it easier for the user, a followup could update the description of OpacHiddenItems making the user aware that these two new prefs make it possible to fine-tune its behavior. (In reply to M. de Rooy from comment #23) > 1) No unit tests. Adding these makes the chance of a push considerably > larger :) Reading back, I happen to notice that there seem to be tests under another report. Sorry for that.. (In reply to M. de Rooy from comment #24) > (In reply to M. de Rooy from comment #23) > > 1) No unit tests. Adding these makes the chance of a push considerably > > larger :) > > Reading back, I happen to notice that there seem to be tests under another > report. Sorry for that.. Bug 10895 to be exact Punting this to the next release; my concerns about relying on this in your particular use case remain. This patch will work perfectly with us in terms of managing our patrons that can view hidden materials. We are a network of linguistic libraries located in different countries in South East Asia and we have materials that would need to be hidden from one library to another and we used OpacHiddenItems to manage this. It really works well with us in restricting general public and regular patrons in viewing hidden materials from the Opac. But since we have patrons who works in multiple countries that needs access to the hidden materials, once they logged in Opac they must have a privilege to view hidden materials. I second Manny's comments. Our situation with a variety of libraries spread throughout countries and yet connected in purpose is complicated. We have so appreciated Koha as a means to serve all these libraries. The upgrades and patches -- several headed up by Mark Tompsett (thanks Mark!) -- have been huge in our ability to serve our patrons. This particular patch would really help us in this unique situation in which we work. Created attachment 30796 [details] [review] Bug 10589 - Override OpacHiddenItems based on Patron Category Two system preferences, PatronSingleBranch and PatronEveryBranch, have been added to facilitate some granularity on the override. If the home branch of the item and the patron match and the catgegory code of the patron matches one of the pipe delimited values in the PatronSingleBranch patron category code, then an item which would normally be hidden will be visible. If the patron category matches one of the pipe delimited values in the PatronEveryBranch patron category, then every item hidden by OpacHiddenItems will be visible. Since the patron category codes used by default for these two system preferences are not added, OpacHiddenItems will continue function as expected until individual patrons and system preferences are configured. There is no need to provide this functionality to staff, as they have access to the staff client. Rebased to current master (again). Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased to current master (Marc) Created attachment 30806 [details] [review] [SIGNED-OFF] Bug 10589 - Override OpacHiddenItems based on Patron Category Two system preferences, PatronSingleBranch and PatronEveryBranch, have been added to facilitate some granularity on the override. If the home branch of the item and the patron match and the catgegory code of the patron matches one of the pipe delimited values in the PatronSingleBranch patron category code, then an item which would normally be hidden will be visible. If the patron category matches one of the pipe delimited values in the PatronEveryBranch patron category, then every item hidden by OpacHiddenItems will be visible. Since the patron category codes used by default for these two system preferences are not added, OpacHiddenItems will continue function as expected until individual patrons and system preferences are configured. There is no need to provide this functionality to staff, as they have access to the staff client. Rebased to current master (again). Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased to current master (Marc) Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Mark, could you try to provide unit tests for this change please? See Bugs 10900 and 10895. They kind of all go together. The unit tests are on another bug report. 10895 is in discussion, because Galen punted it to the next release. (In reply to M. Tompsett from comment #32) > See Bugs 10900 and 10895. They kind of all go together. The unit tests are > on another bug report. 10895 is in discussion, because Galen punted it to > the next release. Could you please re-explain (or point me a comment) how should I apply these patches, their goal and why there are dependent please? Mark, note it is not mandatory, but I can try to find a moment to have a look and it could make me win some minutes :) Okay, this patch needs to be AFTER 10900, and the test cases are in 10895, but those depend on 10900 as well. Bug 10589 is an ENHANCEMENT. It was signed off rather quickly, but at the time I didn't have tests for this. Since it was signed off, I figured I'd write the tests on another report, bug 10895. While trying to write the tests, I discovered the calling convention for set_userenv was broken. So rather than use the broken calling method, I made it uniform across the board to -> instead of ::. Bug 10900 was the result. 10900 is what corrects the set_userenv calls. To test it, I would pay attention to http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10900#c10 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10900#c32 Once done then apply bug 10589 and bug 10895. Kind of like what is talked about in http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10895#c15 Hope this clarifies this mess of code. This patch is the offending patch that causes the Circulation test to fail. One tiny line. As per Tomas' suggestion, I'm going to rework it to pass branch and categorycode, so that Members is not used in Items. Still valid? (In reply to Marc Véron from comment #36) > Still valid? This patch is not the correct way to proceed. I'm working on a rewrite with filters now. Okay... thoughts on a rework include: GetHiddenItems in C4::Items would be removed. This would lead to a refactor EmbedItemsInMarcBiblio to NOT have an fourth parameter $opac as all the filtering will be done by the OPAC modules hooking into the filtering logic created by bug 11592 and the use of a new OpacHiddenItems filter. This trigger removing tests from t/db_dependent/Items.t, but also new tests to test the OpacHiddenItems filter would be added to replace those. Additionally, the GetHiddenItems logic in the detail pages would be supplemented/replaced by logic in the OpacHiddenItems filter which would delete the biblio completely if all the items were hidden. Comparing filtered and unfiltered records could be used to determine what to do in the detail pages. Oops. Missed C4::Search::searchResults. That comment is right about it being messy. I'm working on a refactor in bug 16335, so I'll close this for bug 14385 to move ahead. *** This bug has been marked as a duplicate of bug 14385 *** The PatronEveryBranch is duplicate of bug 14385, but the PatronSingleBranch is a granularity which that bug lacks. (In reply to M. Tompsett from comment #39) > Oops. Missed C4::Search::searchResults. That comment is right about it being > messy. On a positive note, that is dealt with in bug 14385. This further patch is simpler as a result. :) Comment on attachment 30806 [details] [review] [SIGNED-OFF] Bug 10589 - Override OpacHiddenItems based on Patron Category Going to rework based on bug 14385. Created attachment 82521 [details] [review] Bug 10589: local exceptions overriding OpacHiddenItems This patch adds the functionality change to C4/Items.pm which allows to cross check the patron branch with the item branch in addition to the local exception patron category list. This patch is large because is also merely adds patron_branch to a series of function calls which OpacHiddenItems affects. Created attachment 82522 [details] [review] Bug 10589: Unit tests of changes The core changes were in C4/Items.pm with GetHiddenItemnumbers now processing logic against a patron branch vs. an item's home branch and does the patron have the local exception category. This adds 4 tests to confirm those changes. prove -v t/db_dependent/Items.t If you apply just this, the tests should fail. If you apply both patches, the tests should pass. Now that bug 14385 has been pushed to master, this patch will merely need to add a patron branch code to the passed parameter hash, so that logic can be added. A fuller test plan will come shortly. Please take a look at bug 22454 and bug 22455. The rationale is that if you have the biblio object, you can know all that is needed regarding hiding in OPAC without fetching MARC data or complex calculations. And if we were to add parameters like category and library, I would rather pass the Koha::Patron object altogether. This check, with bug 22457 in mind could be written: my $should_hide = ( $patron->category->overrides_opac_hidding_policy or $patron->library->overrides_opac_hidding_policy ) ? 0 : 1; while ( my $biblio = $biblios->next ) { next if $biblio->hidden_in_opac and $should_hide; do_your_things(); } This check should be written on the controller script. (In reply to Tomás Cohen Arazi from comment #47) > Please take a look at bug 22454 and bug 22455. The rationale is that if you > have the biblio object, you can know all that is needed regarding hiding in > OPAC without fetching MARC data or complex calculations. And if we were to > add parameters like category and library, I would rather pass the > Koha::Patron object altogether. > This check, with bug 22457 in mind could be written: > > my $should_hide = ( $patron->category->overrides_opac_hidding_policy or > $patron->library->overrides_opac_hidding_policy ) > ? 0 > : 1; > > while ( my $biblio = $biblios->next ) { > next if $biblio->hidden_in_opac and $should_hide; > do_your_things(); > } > > This check should be written on the controller script. Doesn't quite work. The idea is: if (! defined $patron) { $globally_override = 0; } elsif (! defined $patron->category) { $globally_override = 0; } else { $globally_override = $patron->category->globally_overrides_opac_hiding; } while ( my $biblio = $bilbios->next ) { if (! defined $patron) { $locally_override = 0; } else { $locally_override = ($patron->branchcode ne $biblio->homebranch) ? 0 : $patron->category->overrides_opac_hiding_policy_locally; } $should_show = $globally_override || $locally_override; $should_hide = ! $should_show; next if $biblio->hidden_in_opac && $should_hide; do_your_things(); } Doh! That doesn't quite work, because homebranch is on item. So, you want to pass patron to hidden_in_opac on items... Okay... Hmm... thinking.... |