Bug 14385

Summary: Extend OpacHiddenItems to allow specifying exempt borrower categories
Product: Koha Reporter: Aleisha Amohia <aleisha>
Component: OPACAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: cbrannon, chris, claire_gravely, jkpattison, jonathan.druart, josef.moravec, katrin.fischer, kohadevinim, lisettepalouse+koha, liz, m.de.rooy, manueltamayao, martin.renvoize, mirko, mtompset, nick, severine.queune
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10589
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10895
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17829
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19040
Change sponsored?: --- Patch complexity: Large patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement allows for specifying specific borrower categories in the preference OpacHiddenItemsExceptions who, when signed in to the opac, will be able to see items hidden by the OpacHiddenItems system preference. This is intended to allow staff/privileged users to view records that the general public cannot.
Version(s) released in:
Bug Depends on: 19040    
Bug Blocks: 10589, 20664, 22420, 22766, 23530, 24121, 24198    
Attachments: Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385 Followup: C4::Borrowers not found
Bug 14385 Followup: C4::Borrowers not found
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385 Followup: C4::Borrowers not found
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385 Followup: C4::Borrowers not found
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Bug 14385: Added missing GetHiddenItems parameter change case
Bug 14385: Always use mocks, not set_preference
Bug 14385: Tweaks so t/db_dependent/00-strict.t passes
Bug 14385: Tweak t/db_dependent/Items.t to fully test changes
Bug 14385: Tweak borrower category code
Bug 14385: Search was returning URLS for wrong interface
Bug 14385: Corrected issues with category code.
Bug 14385: Properly trigger all changes in C4/Search.pm
Bug 14385: Fix QA Test tool failures
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385 Followup: C4::Borrowers not found
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Bug 14385: Added missing GetHiddenItems parameter change case
Bug 14385: Always use mocks, not set_preference
Bug 14385: Tweaks so t/db_dependent/00-strict.t passes
Bug 14385: Tweak t/db_dependent/Items.t to fully test changes
Bug 14385: Tweak borrower category code
Bug 14385: Search was returning URLS for wrong interface
Bug 14385: Corrected issues with category code.
Bug 14385: Properly trigger all changes in C4/Search.pm
Bug 14385: Fix QA Test tool failures
Bug 14385: Add some commenting to make sense of logic
Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385 Followup: C4::Borrowers not found
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Bug 14385: Added missing GetHiddenItems parameter change case
Bug 14385: Always use mocks, not set_preference
Bug 14385: Tweaks so t/db_dependent/00-strict.t passes
Bug 14385: Tweak t/db_dependent/Items.t to fully test changes
Bug 14385: Tweak borrower category code
Bug 14385: Search was returning URLS for wrong interface
Bug 14385: Corrected issues with category code.
Bug 14385: Properly trigger all changes in C4/Search.pm
Bug 14385: Fix QA Test tool failures
Bug 14385: Add some commenting to make sense of logic
Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385 Followup: C4::Borrowers not found
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Bug 14385: Added missing GetHiddenItems parameter change case
Bug 14385: Always use mocks, not set_preference
Bug 14385: Tweaks so t/db_dependent/00-strict.t passes
Bug 14385: Tweak t/db_dependent/Items.t to fully test changes
Bug 14385: Tweak borrower category code
Bug 14385: Search was returning URLS for wrong interface
Bug 14385: Corrected issues with category code.
Bug 14385: Properly trigger all changes in C4/Search.pm
Bug 14385: Fix QA Test tool failures
Bug 14385: Add some commenting to make sense of logic
Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Bug 14385: Add borrower category overrides
Bug 14385: Modify opac-ISBDdetail and opac-MARCdetail
Bug 14385: Fix hiding in opac-tags
Bug 14385: Trigger the C4::ILSDI::Services changes
Bug 14385: Added missing 'my'
Bug 14385: Test C4/Labels/Label.pm changes
Bug 14385: Improve C4::Record::marcrecord2csv test cases
Bug 14385: Corrected opac-details searchResult call
Bug 14385: Fix breaking issues constraint in ITerator test
Bug 14385: Fix ILSDI_Services test when clubs with branch exist
[SIGNED-OFF] Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
[SIGNED-OFF] Bug 14385 Followup: C4::Borrowers not found
[SIGNED-OFF] Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
[SIGNED-OFF] Bug 14385: Added missing GetHiddenItems parameter change case
[SIGNED-OFF] Bug 14385: Always use mocks, not set_preference
[SIGNED-OFF] Bug 14385: Tweaks so t/db_dependent/00-strict.t passes
[SIGNED-OFF] Bug 14385: Tweak t/db_dependent/Items.t to fully test changes
[SIGNED-OFF] Bug 14385: Tweak borrower category code
[SIGNED-OFF] Bug 14385: Search was returning URLS for wrong interface
[SIGNED-OFF] Bug 14385: Corrected issues with category code.
[SIGNED-OFF] Bug 14385: Properly trigger all changes in C4/Search.pm
[SIGNED-OFF] Bug 14385: Fix QA Test tool failures
[SIGNED-OFF] Bug 14385: Add some commenting to make sense of logic
[SIGNED-OFF] Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
[SIGNED-OFF] Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
[SIGNED-OFF] Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
[SIGNED-OFF] Bug 14385: Add borrower category overrides
[SIGNED-OFF] Bug 14385: Modify opac-ISBDdetail and opac-MARCdetail
[SIGNED-OFF] Bug 14385: Fix hiding in opac-tags
[SIGNED-OFF] Bug 14385: Trigger the C4::ILSDI::Services changes
[SIGNED-OFF] Bug 14385: Added missing 'my'
[SIGNED-OFF] Bug 14385: Test C4/Labels/Label.pm changes
[SIGNED-OFF] Bug 14385: Improve C4::Record::marcrecord2csv test cases
[SIGNED-OFF] Bug 14385: Corrected opac-details searchResult call
[SIGNED-OFF] Bug 14385: Fix breaking issues constraint in ITerator test
[SIGNED-OFF] Bug 14385: Fix ILSDI_Services test when clubs with branch exist
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385 Followup: C4::Borrowers not found
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Bug 14385: A lot of patches
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385 Followup: C4::Borrowers not found
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385 Followup: C4::Borrowers not found
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385 Followup: C4::Borrowers not found
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Squash of a lot of patches rebased
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Bug 14385: (follow-up) C4::Borrowers not found
Bug 14385: (follow-up) Changes to sysprefs.sql and atomicupdate
Bug 14385: Squash of a lot of patches rebased
Bug 14385: (QA follow-up) Additional changes and fixes
Bug 14385: (RM follow-up) Remove line from bad rebase
Bug 14385: (RM follow-up) Remove line from bad rebase

Description Aleisha Amohia 2015-06-15 04:54:26 UTC
Want to be able to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

Todo:
• C4::Items->GetHiddenItemnumbers:
** It must take an optional borrowercategory
** It must check the borrowercategory against the exception list, and if there's a match, then it doesn't mark that item as hidden.
** Update the test cases to check this.
• C4::Search->searchResults:
** $search_context should be modified to include a reference to the borrower details
** the places using $search_context will need to be updated to handle the new structure
** the borrower category will be passed on to GetHiddenItemnumbers
** update the test cases to account for this
• opac/opac-search.pl, opac/opac-detail.pl:
** Call Members::GetMember to get info on the current user, pass the category into searchResults as the search context.
• opac-MARCdetail.pl, opac/opac-detail.pl, opac/opac-ISBDdetail.pl:
** Call Members::GetMember to get info on the current user, pass the category into GetHiddenItemnumbers
*** Account for no one being logged in (i.e. don't pass the category.)
** Note: opac-MARCdetail.pl will need a bit of rearrangement to where it figures out the current user.
Comment 1 Aleisha Amohia 2015-06-16 05:25:19 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2016-01-04 07:32:50 UTC
Another one I can't sign off on, could someone please test this
Comment 3 Mark Tompsett 2016-04-22 01:02:03 UTC
This reminds me of bug 10589.
Comment 4 Claire Gravely 2016-06-04 09:42:48 UTC
CONFLICT (content): Merge conflict in C4/Search.pm
Comment 5 Mark Tompsett 2016-06-06 19:14:37 UTC
Also consider the refactor I'm doing on bug 16335. I just put up my filter work.
Comment 6 Aleisha Amohia 2016-07-19 03:43:48 UTC Comment hidden (obsolete)
Comment 7 Claire Gravely 2016-07-19 09:57:40 UTC
When testing I get the following Software error on the OPAC:

Can't locate C4/Borrowers.pm in @INC (you may need to install the C4::Borrowers module) (@INC contains: /home/claire/koha /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.20.2 /usr/local/share/perl/5.20.2 /usr/lib/x86_64-linux-gnu/perl5/5.20 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.20 /usr/share/perl/5.20 /usr/local/lib/site_perl .) at /home/claire/koha/opac/opac-search.pl line 56.
BEGIN failed--compilation aborted at /home/claire/koha/opac/opac-search.pl line 56.

Claire
Comment 8 Mirko Tietgen 2016-07-19 15:51:09 UTC Comment hidden (obsolete)
Comment 9 Mirko Tietgen 2016-07-19 15:53:49 UTC
I have not tested the functionality, but the followup makes the error go away to allow testing.
Comment 10 Claire Gravely 2016-07-20 08:10:31 UTC Comment hidden (obsolete)
Comment 11 Claire Gravely 2016-07-20 09:01:08 UTC
Created attachment 53506 [details] [review]
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>
Comment 12 Claire Gravely 2016-07-20 09:01:11 UTC
Created attachment 53507 [details] [review]
Bug 14385 Followup: C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>
Comment 13 Jonathan Druart 2016-08-01 11:33:10 UTC
Aleisha, as Mark told you in comment 3 and comment 5, your patch conflict with bug 10589 and bug 16335.

Moreover you forgot the changes to sysprefs.sql and an atomic update file.

Mark, bug 10589 is failed QA, do you plan to work on it?
Comment 14 Mark Tompsett 2016-08-01 11:43:51 UTC
*** Bug 10589 has been marked as a duplicate of this bug. ***
Comment 15 Aleisha Amohia 2016-08-02 03:30:48 UTC
(In reply to Jonathan Druart from comment #13)
> Aleisha, as Mark told you in comment 3 and comment 5, your patch conflict
> with bug 10589 and bug 16335.

I see 10589 has been closed will this one goes ahead. Mark, should we wait for you to finish your work with 16335 before finishing this one?
Comment 16 Mark Tompsett 2016-08-02 08:23:45 UTC
(In reply to Aleisha Amohia from comment #15)
> (In reply to Jonathan Druart from comment #13)
> > Aleisha, as Mark told you in comment 3 and comment 5, your patch conflict
> > with bug 10589 and bug 16335.
> 
> I see 10589 has been closed will this one goes ahead. Mark, should we wait
> for you to finish your work with 16335 before finishing this one?

Go ahead with this. 16335 is going to take a long time before I get it finished.
Comment 17 Mark Tompsett 2016-09-09 14:54:54 UTC
(In reply to Aleisha Amohia from comment #15)
> I see 10589 has been closed will this one goes ahead. Mark, should we wait
> for you to finish your work with 16335 before finishing this one?

Just fix the problems raised by JD in comment #13:
> Moreover you forgot the changes to sysprefs.sql and an atomic update file.

Awaiting this so I can sign off.
Comment 18 Aleisha Amohia 2016-09-16 02:43:33 UTC
Created attachment 55593 [details] [review]
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT
Comment 19 Christopher Brannon 2017-04-08 21:02:29 UTC
vagrant@kohadevbox:kohaclone(bug_14385)$ git bz apply 14385
Bug 14385 - Extend OpacHiddenItems to allow specifying exempt borrower categories

53506 - Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
53507 - Bug 14385 Followup: C4::Borrowers not found
55593 - Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Using index info to reconstruct a base tree...
M       C4/Items.pm
M       C4/Search.pm
M       catalogue/search.pl
M       cataloguing/addbooks.pl
M       koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
M       opac/opac-ISBDdetail.pl
M       opac/opac-MARCdetail.pl
M       opac/opac-detail.pl
M       opac/opac-search.pl
M       t/db_dependent/Items.t
M       t/db_dependent/Search.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Search.t
Auto-merging t/db_dependent/Items.t
Auto-merging opac/opac-search.pl
Auto-merging opac/opac-detail.pl
Auto-merging opac/opac-MARCdetail.pl
Auto-merging opac/opac-ISBDdetail.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging cataloguing/addbooks.pl
Auto-merging catalogue/search.pl
Auto-merging C4/Search.pm
Auto-merging C4/Items.pm
Applying: Bug 14385 Followup: C4::Borrowers not found
Using index info to reconstruct a base tree...
M       opac/opac-search.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-search.pl
Applying: Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Using index info to reconstruct a base tree...
M       installer/data/mysql/sysprefs.sql
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/sysprefs.sql
Comment 20 Christopher Brannon 2017-04-08 21:21:31 UTC
Not sure if the above means that the apply failed, but I did see changed files.  Tried this out, but the exception is not working.  Item is still hidden.
Comment 21 Mark Tompsett 2017-05-31 14:14:14 UTC
Is anyone going to try to resurrect this? If not, I'll take a stab at it, because this is the LAST set of patches, I need to free myself from the shackles of customized, unsupported code.
Comment 22 Mark Tompsett 2017-06-27 16:29:06 UTC
Found a missing C4/Biblio.pm modification... Still looking further.
Comment 23 Mark Tompsett 2017-06-28 00:03:01 UTC
Taking based on conversation with Aleisha on IRC. I think this is a cleaner start to the filtering that 10589 was trying to do. I'll be reopening that one after this.
Comment 24 Mark Tompsett 2017-06-28 02:56:50 UTC
Created attachment 64688 [details] [review]
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>
Comment 25 Mark Tompsett 2017-06-28 02:56:59 UTC
Created attachment 64689 [details] [review]
Bug 14385 Followup: C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>
Comment 26 Mark Tompsett 2017-06-28 02:57:05 UTC
Created attachment 64690 [details] [review]
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT
Comment 27 Mark Tompsett 2017-06-28 02:57:14 UTC
Created attachment 64691 [details] [review]
Bug 14385: Added missing GetHiddenItems parameter change case

Without this prove t had a failure.
Comment 28 Mark Tompsett 2017-06-28 02:57:21 UTC
Created attachment 64692 [details] [review]
Bug 14385: Always use mocks, not set_preference
Comment 29 Mark Tompsett 2017-06-28 02:57:31 UTC
Created attachment 64693 [details] [review]
Bug 14385: Tweaks so t/db_dependent/00-strict.t passes

There was a typo botcat vs borcat and borrowernumber was never
defined. Grabbing from userenv, like other code does.
Comment 30 Mark Tompsett 2017-07-14 02:09:40 UTC
Bug 17829 means the C4::Member::GetMember logic here doesn't work. Tweaking.
Comment 31 Mark Tompsett 2017-07-29 03:15:58 UTC
Perhaps when I have a more solid block of time, I'll be able to debug why the opac search results give links to the staff client. DOH! Still things to fix.
Comment 32 Mark Tompsett 2017-07-30 01:38:47 UTC
Good progress. Just need to finalize testing the C4/Search.pm changes.
Here is the test plan thus far.

prove -v t/db_dependent/Items.t
C4::Biblio changes (EmbedItemsInMarcBiblio) tested
C4::Items changes (GetHiddentItemnumbers) tested
the Items.t is tested itself too.

perldoc C4::Items
/PERLDOC
-- finds changes

installer/data/mysql/atomicupdate/bug_14385-add_OpacHiddenItemsExceptions_syspref.sql
koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
-- run an upgrade
-- log in to staff client
-- home -> koha administration -> global system preferences -> opac
-- should be an added OpacHiddenItemsExceptions under OpacHiddenItems
   in the Policy section.
-- should be able to save various values.

installer/data/mysql/sysprefs.sql
-- drop the db
-- create the db
-- fresh install
-- log in to staff client
-- home -> koha administration -> global system preferences -> opac
-- should be an added OpacHiddenItemsExceptions under OpacHiddenItems
   in the Policy section.
-- should be able to save various values.

opac/opac-search.pl
-- set the OpacHiddenItems to "itype: [BK]"
-- set the OpacHiddenItemsExceptions to "S"
-- make sure to catalogue at least two books, since you dropped everything.
-- Adding from Z39.50 is easiest.
-- do a full reindex
-- in the OPAC, search for "itype:BK"
-- there should be nothing found.
-- log in as the superlibrarian (which you probably set to S for staff)
-- in the OPAC, search for "itype:BK"
-- there should be two entries
-- change OpacHiddenItemsExceptions to "PT"
   (because your superlibrarian is probably S and not PT)
-- in the OPAC, search for "itype:BK"
-- there should be nothing found.

opac/opac-detail.pl
-- As you know there are two books, try a direct URL:
   {OPACBaseURL}/opac-detail.pl?biblionumber=1
-- it should return a 404 page
-- change OpacHiddenItemsExceptions back to "S"
-- try the direct URL again.
-- it should show up.

opac/opac-MARCdetail.pl
-- change the OpacHiddenItemsExceptions back to "PT"
-- try a direct URL:
   {OPACBaseURL}/opac-MARCdetail.pl?biblionumber=1
-- it should return a 404 page
-- change OpacHiddenItemsExceptions back to "S"
-- try the direct URL again.
-- it should show up.

opac/opac-ISBDdetail.pl
-- change the OpacHiddenItemsExceptions back to "PT"
-- try a direct URL:
   {OPACBaseURL}/opac-ISBDdetail.pl?biblionumber=1
-- it should return a 404 page
-- change OpacHiddenItemsExceptions back to "S"
-- try the direct URL again.
-- it should show up.

prove -v t/db_dependent/Search.t
the Search.t itself is proved.

catalogue/search.pl
-- Log in to the staff client
-- search for "itype:BK"
-- two results
-- change OpacHiddenItemsExceptions and OpacHiddenItems as desired
-- two results all the time, because this is the staff client!

cataloguing/addbooks.pl
-- Make sure you have records in the cataloguing reservoir
   This is done by staging at least one MARC record.
-- In the staff client, home -> Cataloguing -> Cataloguing search
-- Search for something that will find some records in the reservoir
-- Make sure results are expected regardless of hidden item
   system preferences.
Comment 33 Mark Tompsett 2017-07-30 01:43:11 UTC
Created attachment 65322 [details] [review]
Bug 14385: Tweak t/db_dependent/Items.t to fully test changes

This will test all the if structures fully in GetHiddenItemnumbers.
prove t/db_dependent/Items.t
Comment 34 Mark Tompsett 2017-07-30 01:43:15 UTC
Created attachment 65323 [details] [review]
Bug 14385: Tweak borrower category code

$borrower->{categorycode} on a Koha::Patron is not the
same as $borrower->categorycode. Fixed error.
Comment 35 Mark Tompsett 2017-07-30 01:43:21 UTC
Created attachment 65324 [details] [review]
Bug 14385: Search was returning URLS for wrong interface

There was one search context place wrong. Changed it to $is_opac
as the logic for setting $is_opac was modified correctly.
Comment 36 Mark Tompsett 2017-07-30 01:43:25 UTC
Created attachment 65325 [details] [review]
Bug 14385: Corrected issues with category code.

When a user isn't logged in, $borrower is undef and causes error
when determining category code. Added conditional check.
Comment 37 Mark Tompsett 2017-08-01 01:36:35 UTC
Created attachment 65376 [details] [review]
Bug 14385: Properly trigger all changes in C4/Search.pm

prove t/db_dependent/Search.t
Comment 38 Mark Tompsett 2017-08-01 01:40:10 UTC
Created attachment 65377 [details] [review]
Bug 14385: Fix QA Test tool failures

C4/Search.pm had some tabs.
Comment 39 Mark Tompsett 2017-08-01 01:43:20 UTC
Created attachment 65378 [details] [review]
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 40 Mark Tompsett 2017-08-01 01:43:31 UTC
Created attachment 65379 [details] [review]
Bug 14385 Followup: C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 41 Mark Tompsett 2017-08-01 01:43:40 UTC
Created attachment 65380 [details] [review]
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 42 Mark Tompsett 2017-08-01 01:43:50 UTC
Created attachment 65381 [details] [review]
Bug 14385: Added missing GetHiddenItems parameter change case

Without this prove t had a failure.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 43 Mark Tompsett 2017-08-01 01:43:59 UTC
Created attachment 65382 [details] [review]
Bug 14385: Always use mocks, not set_preference

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 44 Mark Tompsett 2017-08-01 01:44:07 UTC
Created attachment 65383 [details] [review]
Bug 14385: Tweaks so t/db_dependent/00-strict.t passes

There was a typo botcat vs borcat and borrowernumber was never
defined. Grabbing from userenv, like other code does.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 45 Mark Tompsett 2017-08-01 01:44:15 UTC
Created attachment 65384 [details] [review]
Bug 14385: Tweak t/db_dependent/Items.t to fully test changes

This will test all the if structures fully in GetHiddenItemnumbers.
prove t/db_dependent/Items.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 46 Mark Tompsett 2017-08-01 01:44:23 UTC
Created attachment 65385 [details] [review]
Bug 14385: Tweak borrower category code

$borrower->{categorycode} on a Koha::Patron is not the
same as $borrower->categorycode. Fixed error.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 47 Mark Tompsett 2017-08-01 01:44:38 UTC
Created attachment 65386 [details] [review]
Bug 14385: Search was returning URLS for wrong interface

There was one search context place wrong. Changed it to $is_opac
as the logic for setting $is_opac was modified correctly.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 48 Mark Tompsett 2017-08-01 01:44:46 UTC
Created attachment 65387 [details] [review]
Bug 14385: Corrected issues with category code.

When a user isn't logged in, $borrower is undef and causes error
when determining category code. Added conditional check.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 49 Mark Tompsett 2017-08-01 01:44:53 UTC
Created attachment 65388 [details] [review]
Bug 14385: Properly trigger all changes in C4/Search.pm

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 50 Mark Tompsett 2017-08-01 01:45:04 UTC
Created attachment 65389 [details] [review]
Bug 14385: Fix QA Test tool failures

C4/Search.pm had some tabs.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 51 Mark Tompsett 2017-08-01 01:50:09 UTC
Follow the test plan in comment #32 and add a prove t/db_dependent/Search.t to complete it. :)

Run QA Test tools, and all should be good.

Read the code, make sure all the searchResults() and GetHiddenItemnumbers() calls were tweaked with the new parameters.

Thanks to Aleisha Amohia for dealing with the searchResult() issues that I had failed to handle in my previous attempt on bug 10589, which will now become dependent on this patch, as I seek to add PatronEveryBranch (which this is), vs. PatronSingleBranch (which will be added).

I have signed off all the code (including Catalyst's), and so if anyone else could sign off all the code, I would consider this signed off.
Comment 52 Mark Tompsett 2017-08-01 17:33:55 UTC
*sigh* found an issue. EmbedItemsInMarcBiblio.
If $opac, we should be passing borcat.
Comment 53 Mark Tompsett 2017-08-01 23:16:20 UTC
Created attachment 65408 [details] [review]
Bug 14385: Add some commenting to make sense of logic
Comment 54 Mark Tompsett 2017-08-01 23:16:26 UTC
Created attachment 65409 [details] [review]
Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
Comment 55 Mark Tompsett 2017-08-01 23:16:32 UTC
Created attachment 65410 [details] [review]
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.

prove t/db_dependent/Items.t
Comment 56 Mark Tompsett 2017-08-01 23:16:39 UTC
Created attachment 65411 [details] [review]
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Comment 57 Mark Tompsett 2017-08-02 00:42:25 UTC
The problem with this, as it stands is the GetMarcBiblio call does not receive the patron category code, so it can't pass it to EmbedItemsInMarcBiblio which can't then pass it to GetHiddenItemnumbers. And while I'm tempted to hack in a user category code and branch lookup, that would defeat the niceness of the refactored parameter passing thus far.
Comment 58 Mark Tompsett 2017-08-04 02:15:42 UTC
To test all the EmbedItemsInMarcBiblio calls:
 prove t/db_dependent/Biblio.t
 prove t/db_dependent/Record/marcrecord2csv.t
 prove t/db_dependent/Koha/BiblioUtils/Iterator.t
 prove t/db_dependent/Exporter/Record.t
 ./misc/migration_tools/build_oai_sets.pl -i
 And catalog any new item in the staff client. I did a new from Z39.50.
Comment 59 Mark Tompsett 2017-08-11 01:44:35 UTC
(In reply to M. Tompsett from comment #57)
> The problem with this, as it stands is the GetMarcBiblio call does not
> receive the patron category code, so it can't pass it to
> EmbedItemsInMarcBiblio which can't then pass it to GetHiddenItemnumbers. And
> while I'm tempted to hack in a user category code and branch lookup, that
> would defeat the niceness of the refactored parameter passing thus far.

And my attempt to start the refactor on bug 19040 is proving painful for having a full trigger all the changes test plan.

Options:
1) Push 19040 seeing it eyeballs perfectly well, and I continue on fixing here.
2) Don't push 19040, but push this knowing this one flaw.
   -- it is basically the search results look weird, because they are semi-hidden.
3) Hack the lookup, which could really slow code down.
4) Give up.

I'd appreciate feedback.
Comment 60 Mark Tompsett 2017-08-16 15:10:10 UTC
Created attachment 66103 [details] [review]
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 61 Mark Tompsett 2017-08-16 15:10:15 UTC
Created attachment 66104 [details] [review]
Bug 14385 Followup: C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 62 Mark Tompsett 2017-08-16 15:10:19 UTC
Created attachment 66105 [details] [review]
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 63 Mark Tompsett 2017-08-16 15:10:24 UTC
Created attachment 66106 [details] [review]
Bug 14385: Added missing GetHiddenItems parameter change case

Without this prove t had a failure.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 64 Mark Tompsett 2017-08-16 15:10:28 UTC
Created attachment 66107 [details] [review]
Bug 14385: Always use mocks, not set_preference

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 65 Mark Tompsett 2017-08-16 15:10:32 UTC
Created attachment 66108 [details] [review]
Bug 14385: Tweaks so t/db_dependent/00-strict.t passes

There was a typo botcat vs borcat and borrowernumber was never
defined. Grabbing from userenv, like other code does.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 66 Mark Tompsett 2017-08-16 15:10:37 UTC
Created attachment 66109 [details] [review]
Bug 14385: Tweak t/db_dependent/Items.t to fully test changes

This will test all the if structures fully in GetHiddenItemnumbers.
prove t/db_dependent/Items.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 67 Mark Tompsett 2017-08-16 15:10:41 UTC
Created attachment 66110 [details] [review]
Bug 14385: Tweak borrower category code

$borrower->{categorycode} on a Koha::Patron is not the
same as $borrower->categorycode. Fixed error.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 68 Mark Tompsett 2017-08-16 15:10:45 UTC
Created attachment 66111 [details] [review]
Bug 14385: Search was returning URLS for wrong interface

There was one search context place wrong. Changed it to $is_opac
as the logic for setting $is_opac was modified correctly.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 69 Mark Tompsett 2017-08-16 15:10:50 UTC
Created attachment 66112 [details] [review]
Bug 14385: Corrected issues with category code.

When a user isn't logged in, $borrower is undef and causes error
when determining category code. Added conditional check.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 70 Mark Tompsett 2017-08-16 15:10:54 UTC
Created attachment 66113 [details] [review]
Bug 14385: Properly trigger all changes in C4/Search.pm

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 71 Mark Tompsett 2017-08-16 15:10:59 UTC
Created attachment 66114 [details] [review]
Bug 14385: Fix QA Test tool failures

C4/Search.pm had some tabs.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 72 Mark Tompsett 2017-08-16 15:11:03 UTC
Created attachment 66115 [details] [review]
Bug 14385: Add some commenting to make sense of logic
Comment 73 Mark Tompsett 2017-08-16 15:11:07 UTC
Created attachment 66116 [details] [review]
Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
Comment 74 Mark Tompsett 2017-08-16 15:11:12 UTC
Created attachment 66117 [details] [review]
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.

prove t/db_dependent/Items.t
Comment 75 Mark Tompsett 2017-08-16 15:11:16 UTC
Created attachment 66118 [details] [review]
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Comment 76 Mark Tompsett 2017-08-16 15:43:31 UTC
Created attachment 66125 [details] [review]
Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
Comment 77 Mark Tompsett 2017-08-16 15:43:35 UTC
Created attachment 66126 [details] [review]
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.

prove t/db_dependent/Items.t
Comment 78 Mark Tompsett 2017-08-16 15:43:40 UTC
Created attachment 66127 [details] [review]
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Comment 79 Mark Tompsett 2017-08-16 15:51:08 UTC
Created attachment 66128 [details] [review]
Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
Comment 80 Mark Tompsett 2017-08-16 15:51:12 UTC
Created attachment 66129 [details] [review]
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.

prove t/db_dependent/Items.t
Comment 81 Mark Tompsett 2017-08-16 15:51:16 UTC
Created attachment 66130 [details] [review]
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Comment 82 Mark Tompsett 2017-08-16 16:09:18 UTC
Created attachment 66131 [details] [review]
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.

prove t/db_dependent/Items.t
Comment 83 Mark Tompsett 2017-08-16 16:09:22 UTC
Created attachment 66132 [details] [review]
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Comment 84 Mark Tompsett 2017-08-18 21:56:14 UTC
Created attachment 66192 [details] [review]
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 85 Mark Tompsett 2017-08-18 21:56:20 UTC
Created attachment 66193 [details] [review]
Bug 14385 Followup: C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 86 Mark Tompsett 2017-08-18 21:56:26 UTC
Created attachment 66194 [details] [review]
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 87 Mark Tompsett 2017-08-18 21:56:33 UTC
Created attachment 66195 [details] [review]
Bug 14385: Added missing GetHiddenItems parameter change case

Without this prove t had a failure.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 88 Mark Tompsett 2017-08-18 21:56:38 UTC
Created attachment 66196 [details] [review]
Bug 14385: Always use mocks, not set_preference

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 89 Mark Tompsett 2017-08-18 21:56:42 UTC
Created attachment 66197 [details] [review]
Bug 14385: Tweaks so t/db_dependent/00-strict.t passes

There was a typo botcat vs borcat and borrowernumber was never
defined. Grabbing from userenv, like other code does.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 90 Mark Tompsett 2017-08-18 21:56:46 UTC
Created attachment 66198 [details] [review]
Bug 14385: Tweak t/db_dependent/Items.t to fully test changes

This will test all the if structures fully in GetHiddenItemnumbers.
prove t/db_dependent/Items.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 91 Mark Tompsett 2017-08-18 21:56:50 UTC
Created attachment 66199 [details] [review]
Bug 14385: Tweak borrower category code

$borrower->{categorycode} on a Koha::Patron is not the
same as $borrower->categorycode. Fixed error.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 92 Mark Tompsett 2017-08-18 21:56:56 UTC
Created attachment 66200 [details] [review]
Bug 14385: Search was returning URLS for wrong interface

There was one search context place wrong. Changed it to $is_opac
as the logic for setting $is_opac was modified correctly.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 93 Mark Tompsett 2017-08-18 21:57:00 UTC
Created attachment 66201 [details] [review]
Bug 14385: Corrected issues with category code.

When a user isn't logged in, $borrower is undef and causes error
when determining category code. Added conditional check.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 94 Mark Tompsett 2017-08-18 21:57:05 UTC
Created attachment 66202 [details] [review]
Bug 14385: Properly trigger all changes in C4/Search.pm

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 95 Mark Tompsett 2017-08-18 21:57:09 UTC
Created attachment 66203 [details] [review]
Bug 14385: Fix QA Test tool failures

C4/Search.pm had some tabs.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 96 Mark Tompsett 2017-08-18 21:57:13 UTC
Created attachment 66204 [details] [review]
Bug 14385: Add some commenting to make sense of logic
Comment 97 Mark Tompsett 2017-08-18 21:57:19 UTC
Created attachment 66205 [details] [review]
Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref
Comment 98 Mark Tompsett 2017-08-18 21:57:24 UTC
Created attachment 66206 [details] [review]
Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.

prove t/db_dependent/Items.t
Comment 99 Mark Tompsett 2017-08-18 21:57:29 UTC
Created attachment 66207 [details] [review]
Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change
Comment 100 Mark Tompsett 2017-08-18 21:57:34 UTC
Created attachment 66208 [details] [review]
Bug 14385: Add borrower category overrides

These files generally add borcat parameter to GetMarcBiblio.
Others might include correction of filtering of items
(opac-basket), or a comment as to why no changes were done
(opac-search).

In the case of opac-search, correcting the first FIXME will
likely correct the OpacHiddenItems issues on tags. As such,
that is beyond this bugs scope.

Some code had loop optimizations and fixes made, like a
'next unless $record' when the biblio shouldn't even be in
the list.
Comment 101 Mark Tompsett 2017-08-18 21:57:38 UTC
Created attachment 66209 [details] [review]
Bug 14385: Modify opac-ISBDdetail and opac-MARCdetail

Both files had similar logic. They were rearranged and
optimized, so that both files would have practically identical
initial blocks of code.

Optimizations were possible, because GetMarcBiblio
returns a filtered record, so that there is no double call
(once in the opac-### file and once in GetMarcBiblio) to
GetHiddenItemnumbers.
Comment 102 Mark Tompsett 2017-08-18 21:57:42 UTC
Created attachment 66210 [details] [review]
Bug 14385: Fix hiding in opac-tags

opac/opac-tags.pl was not properly hiding.

There is currently one known bug associated with tags left.
If you have two biblios tagged by different people with the
same tag, the opac-search will show the one you tagged that
is supposed to be hidden, because tag searches work differently
than regular searches. This is beyond the scope of this bug.
See the FIXME's in opac/opac-search.pl
Comment 103 Mark Tompsett 2017-08-18 22:09:18 UTC
Comment 51 and 58 constitute the most recent test plan. A more complete single one to come soon.
Comment 104 Mark Tompsett 2017-08-21 17:43:23 UTC
I would very much appreciate testing and feedback, while I attempt to make a more complete test plan.

Apply 19040 first.
Then this. Then test.
Comment 105 Mark Tompsett 2017-08-21 23:14:57 UTC
Created attachment 66302 [details] [review]
Bug 14385: Trigger the C4::ILSDI::Services changes

prove t/db_dependent/ILSDI_Services.t
Comment 106 Mark Tompsett 2017-08-22 00:29:48 UTC
Created attachment 66303 [details] [review]
Bug 14385: Added missing 'my'
Comment 107 Mark Tompsett 2017-08-22 14:50:59 UTC
Created attachment 66347 [details] [review]
Bug 14385: Test C4/Labels/Label.pm changes
Comment 108 Mark Tompsett 2017-08-22 16:47:17 UTC
Created attachment 66361 [details] [review]
Bug 14385: Improve C4::Record::marcrecord2csv test cases
Comment 109 Mark Tompsett 2017-08-22 22:39:37 UTC
Created attachment 66372 [details] [review]
Bug 14385: Corrected opac-details searchResult call
Comment 110 Mark Tompsett 2017-08-24 01:21:01 UTC
Created attachment 66401 [details] [review]
Bug 14385: Fix breaking issues constraint in ITerator test
Comment 111 Mark Tompsett 2017-08-24 01:35:47 UTC
Created attachment 66403 [details] [review]
Bug 14385: Fix ILSDI_Services test when clubs with branch exist
Comment 112 Mark Tompsett 2017-08-24 03:04:47 UTC
back up database
git bz apply 19040
git bz apply 14385
sudo koha-shell -c bash kohadev
./installer/data/misc/updatedatabase.pl
-- this proves the atomic update

drop the database
create an empty one
run a web install
-- this proves the sysprefs changes

restore database
./installer/data/misc/updatedatabase.pl
-- This gives non-fresh install DB to actually test on.

log into the staff client
go to the OPAC system preferences (Home -> Koha administration -> Global System Preferences -> OPAC)
scroll down to the policy section
-- there should be a new OpacHiddenItemsExpceptions system preference setting.
Set a value for the OpacHiddenItemsExceptions, and click save.
-- it should save.
Click 'Home' in the bread crumbs.
Navigate back to the OPAC system preferences
-- the value you set should be saved correctly.
-- This confirms the opac.pref file

sudo koha-shell -c bash kohadev
prove t/db_dependent/Authorities/Merge.t
-- This proves the C4/AuthoritiesMarc.pm change

prove t/db_dependent/HoldsQueue.t
-- This proves the C4/HoldsQueue.pm change

prove t/db_dependent/ILSDI_Services.t
-- This proves the C4/ILSDI/Services.pm change

prove t/db_dependent/Reserves.t t/db_dependent/Items.t
-- This prove the C4/Items.pm changes

prove t/db_dependent/Labels/t_Label.t
-- This proves the C4/Labels/Label.pm changes

prove t/db_dependent/Record/marcrecord2csv.t
-- This proves the C4/Record.pm changes

prove t/db_dependent/Serials.t
-- This proves the C4/Serials.pm change

prove t/db_dependent/ShelfBrowser.t
-- This proves the C4/ShelfBrowser.pm change

prove t/db_dependent/Koha/BiblioUtils.t
-- This proves the Koha/Biblio.pm change

prove t/db_dependent/Koha/BiblioUtils/Iterator.t
-- This proves the Koha/BiblioUtils/Iterator.pm change

prove t/db_dependent/Exporter/Record.t
-- This proves the Koha/Exporter/Record.pm changes

prove t/db_dependent/OAI/Server.t
-- This proves the Koha/OAI/Server/Repository.pm change

prove `git diff origin/master | grep ^diff | grep [.]t$ | cut -f4 -d' ' | sed -e "s#^..##"`
-- This proves all the test file changes.

./misc/migration_tools/build_oai_sets.pl -i
-- This proves this file change.

prove t/db_dependent/Search.t
-- This triggers searchResults tweaks in this file.

- Catalog any new item in the staff client. I did a new from Z39.50.
-- This will trigger any remaining EmbedItemsInMarcBiblio
   calls in the staff client.

1) Make two borrowers, one in a category that should see everything (ie Staff [S]), and another in a category that should only see certain things (ie Patron [PT])
2) Add the borrower that can see everything (the Staff) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Patron)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref.
   -- No records should show for this borrower as this item type is hidden to them.
6) Log out, and repeat the search
   -- No records should show
7) Log in as the borrower that should see everything (Staff)
8) Do the same search.
   -- There should be results from this search, as this borrower category has been specified as an exception to the hidden items

prove -v t/db_dependent/Items.t
C4::Biblio changes (EmbedItemsInMarcBiblio) tested
C4::Items changes (GetHiddentItemnumbers) tested
the Items.t is tested itself too.

perldoc C4::Items
/PERLDOC
-- finds changes

installer/data/mysql/atomicupdate/bug_14385-add_OpacHiddenItemsExceptions_syspref.sql
koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
-- run an upgrade
-- log in to staff client
-- home -> koha administration -> global system preferences -> opac
-- should be an added OpacHiddenItemsExceptions under OpacHiddenItems
   in the Policy section.
-- should be able to save various values.

installer/data/mysql/sysprefs.sql
-- drop the db
-- create the db
-- fresh install
-- log in to staff client
-- home -> koha administration -> global system preferences -> opac
-- should be an added OpacHiddenItemsExceptions under OpacHiddenItems
   in the Policy section.
-- should be able to save various values.

opac/opac-search.pl
-- set the OpacHiddenItems to "itype: [BK]"
-- set the OpacHiddenItemsExceptions to "S"
-- make sure to catalogue at least two books, since you dropped everything.
-- Adding from Z39.50 is easiest.
-- do a full reindex
-- in the OPAC, search for "itype:BK"
-- there should be nothing found.
-- log in as the superlibrarian (which you probably set to S for staff)
-- in the OPAC, search for "itype:BK"
-- there should be two entries
-- change OpacHiddenItemsExceptions to "PT"
   (because your superlibrarian is probably S and not PT)
-- in the OPAC, search for "itype:BK"
-- there should be nothing found.

opac/opac-detail.pl
-- As you know there are two books, try a direct URL:
   {OPACBaseURL}/opac-detail.pl?biblionumber=1
-- it should return a 404 page
-- change OpacHiddenItemsExceptions back to "S"
-- try the direct URL again.
-- it should show up.

opac/opac-MARCdetail.pl
-- change the OpacHiddenItemsExceptions back to "PT"
-- try a direct URL:
   {OPACBaseURL}/opac-MARCdetail.pl?biblionumber=1
-- it should return a 404 page
-- change OpacHiddenItemsExceptions back to "S"
-- try the direct URL again.
-- it should show up.

opac/opac-ISBDdetail.pl
-- change the OpacHiddenItemsExceptions back to "PT"
-- try a direct URL:
   {OPACBaseURL}/opac-ISBDdetail.pl?biblionumber=1
-- it should return a 404 page
-- change OpacHiddenItemsExceptions back to "S"
-- try the direct URL again.
-- it should show up.

prove -v t/db_dependent/Search.t
the Search.t itself is proved.

catalogue/search.pl
-- Log in to the staff client
-- search for "itype:BK"
-- two results
-- change OpacHiddenItemsExceptions and OpacHiddenItems as desired
-- two results all the time, because this is the staff client!

cataloguing/addbooks.pl
-- Make sure you have records in the cataloguing reservoir
   This is done by staging at least one MARC record.
-- In the staff client, home -> Cataloguing -> Cataloguing search
-- Search for something that will find some records in the reservoir
-- Make sure results are expected regardless of hidden item
   system preferences.

To test all the EmbedItemsInMarcBiblio calls:
 prove t/db_dependent/Biblio.t
 prove t/db_dependent/Record/marcrecord2csv.t
 prove t/db_dependent/Koha/BiblioUtils/Iterator.t
 prove t/db_dependent/Exporter/Record.t
 ./misc/migration_tools/build_oai_sets.pl -i
 And catalog any new item in the staff client. I did a new from Z39.50.

 
This part of the testing is reading code and pondering if it makes sense.

git grep searchResults
-- The lines are as follows:
C4/Search.pm
- Export line
- Perldoc 2 lines
- subroutine
catalogue/search.pl
- call has interface => 'intranet' and no borrower category
cataloguing/addbooks.pl
- call has interface => 'intranet' and no borrower category
opac/opac-detail.pl
- call as variable $search_context which is
  { 'interface' => 'opac', 'category' => $borcat }
  $borcat is defined at the beginning of the program
opac/opac-search.pl
- comment
- function calls use $search_context which is defined
  before the servers loop
t/db_dependent/Search.t
- Doesn't test hiding in searchResults.

git grep GetHiddenItemnumbers
-- the lines are as follows:
C4/Biblio.pm
- a single line triggered by prove t/db_dependent/Items.t
C4/Items.pm
- Export line
- Perldoc 2 lines
- subroutine

... More explicit to come later. searchResults, GetHiddenItemnumbers, EmbedItemsInMarcBiblio, GetMarcBiblio all had major prototype changes. Grep for these and confirm the changes reflect the use of a hashref either completely (GetMarcBiblio) or as a parameter (searchResults).
Comment 113 Mark Tompsett 2017-08-25 01:45:53 UTC
comment #112 represents the latest test plan that I followed. I'm calling this done. Multiple eyes and sign offs on this would be appreciated.
Comment 114 Aleisha Amohia 2017-08-27 23:40:43 UTC
Created attachment 66520 [details] [review]
[SIGNED-OFF] Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 115 Aleisha Amohia 2017-08-27 23:40:55 UTC
Created attachment 66521 [details] [review]
[SIGNED-OFF] Bug 14385 Followup: C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 116 Aleisha Amohia 2017-08-27 23:41:04 UTC
Created attachment 66522 [details] [review]
[SIGNED-OFF] Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 117 Aleisha Amohia 2017-08-27 23:41:14 UTC
Created attachment 66523 [details] [review]
[SIGNED-OFF] Bug 14385: Added missing GetHiddenItems parameter change case

Without this prove t had a failure.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 118 Aleisha Amohia 2017-08-27 23:41:22 UTC
Created attachment 66524 [details] [review]
[SIGNED-OFF] Bug 14385: Always use mocks, not set_preference

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 119 Aleisha Amohia 2017-08-27 23:41:32 UTC
Created attachment 66525 [details] [review]
[SIGNED-OFF] Bug 14385: Tweaks so t/db_dependent/00-strict.t passes

There was a typo botcat vs borcat and borrowernumber was never
defined. Grabbing from userenv, like other code does.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 120 Aleisha Amohia 2017-08-27 23:41:41 UTC
Created attachment 66526 [details] [review]
[SIGNED-OFF] Bug 14385: Tweak t/db_dependent/Items.t to fully test changes

This will test all the if structures fully in GetHiddenItemnumbers.
prove t/db_dependent/Items.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 121 Aleisha Amohia 2017-08-27 23:41:50 UTC
Created attachment 66527 [details] [review]
[SIGNED-OFF] Bug 14385: Tweak borrower category code

$borrower->{categorycode} on a Koha::Patron is not the
same as $borrower->categorycode. Fixed error.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 122 Aleisha Amohia 2017-08-27 23:42:00 UTC
Created attachment 66528 [details] [review]
[SIGNED-OFF] Bug 14385: Search was returning URLS for wrong interface

There was one search context place wrong. Changed it to $is_opac
as the logic for setting $is_opac was modified correctly.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 123 Aleisha Amohia 2017-08-27 23:42:09 UTC
Created attachment 66529 [details] [review]
[SIGNED-OFF] Bug 14385: Corrected issues with category code.

When a user isn't logged in, $borrower is undef and causes error
when determining category code. Added conditional check.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 124 Aleisha Amohia 2017-08-27 23:42:18 UTC
Created attachment 66530 [details] [review]
[SIGNED-OFF] Bug 14385: Properly trigger all changes in C4/Search.pm

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 125 Aleisha Amohia 2017-08-27 23:42:27 UTC
Created attachment 66531 [details] [review]
[SIGNED-OFF] Bug 14385: Fix QA Test tool failures

C4/Search.pm had some tabs.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 126 Aleisha Amohia 2017-08-27 23:42:35 UTC
Created attachment 66532 [details] [review]
[SIGNED-OFF] Bug 14385: Add some commenting to make sense of logic

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 127 Aleisha Amohia 2017-08-27 23:42:43 UTC
Created attachment 66533 [details] [review]
[SIGNED-OFF] Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 128 Aleisha Amohia 2017-08-27 23:42:51 UTC
Created attachment 66534 [details] [review]
[SIGNED-OFF] Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.

prove t/db_dependent/Items.t

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 129 Aleisha Amohia 2017-08-27 23:43:03 UTC
Created attachment 66535 [details] [review]
[SIGNED-OFF] Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 130 Aleisha Amohia 2017-08-27 23:43:13 UTC
Created attachment 66536 [details] [review]
[SIGNED-OFF] Bug 14385: Add borrower category overrides

These files generally add borcat parameter to GetMarcBiblio.
Others might include correction of filtering of items
(opac-basket), or a comment as to why no changes were done
(opac-search).

In the case of opac-search, correcting the first FIXME will
likely correct the OpacHiddenItems issues on tags. As such,
that is beyond this bugs scope.

Some code had loop optimizations and fixes made, like a
'next unless $record' when the biblio shouldn't even be in
the list.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 131 Aleisha Amohia 2017-08-27 23:43:22 UTC
Created attachment 66537 [details] [review]
[SIGNED-OFF] Bug 14385: Modify opac-ISBDdetail and opac-MARCdetail

Both files had similar logic. They were rearranged and
optimized, so that both files would have practically identical
initial blocks of code.

Optimizations were possible, because GetMarcBiblio
returns a filtered record, so that there is no double call
(once in the opac-### file and once in GetMarcBiblio) to
GetHiddenItemnumbers.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 132 Aleisha Amohia 2017-08-27 23:43:31 UTC
Created attachment 66538 [details] [review]
[SIGNED-OFF] Bug 14385: Fix hiding in opac-tags

opac/opac-tags.pl was not properly hiding.

There is currently one known bug associated with tags left.
If you have two biblios tagged by different people with the
same tag, the opac-search will show the one you tagged that
is supposed to be hidden, because tag searches work differently
than regular searches. This is beyond the scope of this bug.
See the FIXME's in opac/opac-search.pl

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 133 Aleisha Amohia 2017-08-27 23:43:40 UTC
Created attachment 66539 [details] [review]
[SIGNED-OFF] Bug 14385: Trigger the C4::ILSDI::Services changes

prove t/db_dependent/ILSDI_Services.t

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 134 Aleisha Amohia 2017-08-27 23:43:49 UTC
Created attachment 66540 [details] [review]
[SIGNED-OFF] Bug 14385: Added missing 'my'

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 135 Aleisha Amohia 2017-08-27 23:43:57 UTC
Created attachment 66541 [details] [review]
[SIGNED-OFF] Bug 14385: Test C4/Labels/Label.pm changes

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 136 Aleisha Amohia 2017-08-27 23:44:07 UTC
Created attachment 66542 [details] [review]
[SIGNED-OFF] Bug 14385: Improve C4::Record::marcrecord2csv test cases

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 137 Aleisha Amohia 2017-08-27 23:44:16 UTC
Created attachment 66543 [details] [review]
[SIGNED-OFF] Bug 14385: Corrected opac-details searchResult call

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 138 Aleisha Amohia 2017-08-27 23:44:25 UTC
Created attachment 66544 [details] [review]
[SIGNED-OFF] Bug 14385: Fix breaking issues constraint in ITerator test

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 139 Aleisha Amohia 2017-08-27 23:44:34 UTC
Created attachment 66545 [details] [review]
[SIGNED-OFF] Bug 14385: Fix ILSDI_Services test when clubs with branch exist

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 140 Aleisha Amohia 2017-08-27 23:45:09 UTC
Sorry for the spam! Signing off on all patches
Comment 141 Mark Tompsett 2017-09-12 14:10:23 UTC
(In reply to Aleisha Amohia from comment #140)
> Sorry for the spam! Signing off on all patches

Thanks for the sign off. To the one that has the energy to QA this. The test plan is in comment #112.
Comment 142 Josef Moravec 2018-01-02 09:33:12 UTC
Hi,

these patchset doesn't apply. So just few comments from quick reading of the code:

You don't have to change signature of GetHiddenItemnumbers, you can ask for patron category here... And you can than leave the $search_context as is.

Also the "opac" param in GetMarcBiblio and EmbedItemsInMarcBiblio is not needed, you can use C4::Context->interface when you need to know the actual interface

And some squashing would be nice here ;)
Comment 143 Mark Tompsett 2018-01-03 00:21:05 UTC
(In reply to Josef Moravec from comment #142)
> these patchset doesn't apply.

Thanks for looking and noticing this.


> So just few comments from quick reading of the
> code:
> 
> You don't have to change signature of GetHiddenItemnumbers, you can ask for
> patron category here... And you can than leave the $search_context as is.

I can't recall the reason off the top of my head, and even after rebasing this locally -- I have yet to test and put it back up. But I've got a feeling it had something to so with the nesting of some search functions which were not easily determined to be opac or intranet otherwise.


> Also the "opac" param in GetMarcBiblio and EmbedItemsInMarcBiblio is not
> needed, you can use C4::Context->interface when you need to know the
> actual interface.

It is better to pass it in my opinion to refactor the parameters list, so that test functions don't require special mocking to get the C4::Context function to return the correct value in different cases (like shell script utilities). And this makes maintenance for other changes in the future easier for others, and brings the code up to preferred coding style (https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL16:_Hashrefs_should_be_used_as_arguments)


> And some squashing would be nice here ;)

I was tweaking, testing, fixing, repeating multiple times. Also, smaller patches are easier to test individually. I will squash as much as possible. I do, however, not want to lose the fact that Aleisha Amohia put in a massive effort which helped me see how to solve my C4::Search mess problem that I had hit while working on originally in bug 10589.
Comment 144 Liz Rea 2018-01-03 00:59:16 UTC
a+ preserving attribution. :)
Comment 145 Mark Tompsett 2018-01-04 01:03:28 UTC
Created attachment 70256 [details] [review]
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 146 Mark Tompsett 2018-01-04 01:03:34 UTC
Created attachment 70257 [details] [review]
Bug 14385 Followup: C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 147 Mark Tompsett 2018-01-04 01:03:40 UTC
Created attachment 70258 [details] [review]
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 148 Mark Tompsett 2018-01-04 01:03:47 UTC
Created attachment 70259 [details] [review]
Bug 14385: A lot of patches

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased!

The test plan is in comment #112 last I checked.
Comment 149 Mark Tompsett 2018-01-04 01:11:20 UTC
Still in the midst of retesting after squash. Once I'm happy, I'll post back up a full test plan, and change to needs sign off, just to make sure I didn't miss something.
Comment 150 Mark Tompsett 2018-03-05 16:22:26 UTC
Currently does not apply, reworking.
Comment 151 Mark Tompsett 2018-03-05 19:51:44 UTC
Comment on attachment 70259 [details] [review]
Bug 14385: A lot of patches

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

::: opac/opac-tags.pl
@@ +310,5 @@
> +        if (grep { $_->{term} eq $my_tag->{term} } @$results) {
> +            if (! $my_tag->{visible} ) {
> +                my $check_biblio = GetMarcBiblio({
> +                    biblionumber => $my_tag->{biblionumber},
> +                    embed_itesm  => 1,

Always good idea to re-check over and over. :)
Comment 152 Mark Tompsett 2018-03-05 20:36:13 UTC
Created attachment 72445 [details] [review]
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 153 Mark Tompsett 2018-03-05 20:36:18 UTC
Created attachment 72446 [details] [review]
Bug 14385 Followup: C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 154 Mark Tompsett 2018-03-05 20:36:23 UTC
Created attachment 72447 [details] [review]
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 155 Mark Tompsett 2018-03-05 20:36:29 UTC
Created attachment 72448 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!

The test plan is in comment #112 last I checked.
Comment 156 Mark Tompsett 2018-03-06 01:52:35 UTC
Created attachment 72449 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!

The test plan is in comment #112 last I checked.
Comment 157 Mark Tompsett 2018-03-06 01:55:50 UTC
Created attachment 72450 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!

The test plan is in comment #112 last I checked.
Comment 158 Mark Tompsett 2018-03-06 01:58:08 UTC
Created attachment 72451 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!

The test plan is in comment #112 last I checked.
Comment 159 Mark Tompsett 2018-03-06 02:01:51 UTC
Created attachment 72452 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!

The test plan is in comment #112 last I checked.
Comment 160 Mark Tompsett 2018-03-06 02:17:15 UTC
Created attachment 72453 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!

The test plan is in comment #112 last I checked.
Comment 161 Mark Tompsett 2018-03-06 02:19:59 UTC
Created attachment 72454 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!

The test plan is in comment #112 last I checked.
Comment 162 Mark Tompsett 2018-03-06 02:28:03 UTC
Today was massive rebase, and confirm changes. Still to do: test and confirm that comment #112 is the exhaustive test plan. Sorry for the noise.
Comment 163 Mark Tompsett 2018-04-11 13:09:05 UTC
Created attachment 74001 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again! Rebased again -- my fault for being slow on this.

The test plan is in comment #112 last I checked.
Comment 164 Mark Tompsett 2018-05-31 01:48:46 UTC
This is too huge, my mental energy keeps being too low, and I need this in sooner rather than later. Follow the test plan in comment #112.
Comment 165 Mark Tompsett 2018-09-14 13:19:12 UTC
Created attachment 78654 [details] [review]
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 166 Mark Tompsett 2018-09-14 13:19:18 UTC
Created attachment 78655 [details] [review]
Bug 14385 Followup: C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 167 Mark Tompsett 2018-09-14 13:19:24 UTC
Created attachment 78656 [details] [review]
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 168 Mark Tompsett 2018-09-14 13:19:31 UTC
Created attachment 78657 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!

The test plan is in comment #112 last I checked.
Comment 169 Joy Nelson 2018-09-14 22:51:25 UTC
Created attachment 78825 [details] [review]
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 170 Joy Nelson 2018-09-14 22:51:35 UTC
Created attachment 78826 [details] [review]
Bug 14385 Followup: C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 171 Joy Nelson 2018-09-14 22:51:43 UTC
Created attachment 78827 [details] [review]
Bug 14385: Followup: changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 172 Joy Nelson 2018-09-14 22:51:51 UTC
Created attachment 78828 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!

The test plan is in comment #112 last I checked.
Comment 173 Katrin Fischer 2018-10-11 06:08:02 UTC
Please rebase.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories
Applying: Bug 14385 Followup: C4::Borrowers not found
Applying: Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
Applying: Bug 14385: Squash of a lot of patches rebased
fatal: sha1 information is lacking or useless (C4/Search.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 14385: Squash of a lot of patches rebased
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-14385-Squash-of-a-lot-of-patches-rebased-PmRtSL.patch
Comment 174 Mark Tompsett 2018-10-11 13:53:32 UTC
Created attachment 80411 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!
- Rebased t/db_dependent/Items.t conflict.

The test plan is in comment #112 last I checked.
Comment 175 Marcel de Rooy 2018-10-11 13:59:15 UTC
(In reply to Katrin Fischer from comment #173)
> Please rebase.
> 
> Apply? [(y)es, (n)o, (i)nteractive] y
> Applying: Bug 14385: Extend OpacHiddenItems to allow specifying exempt
> borrower categories
> Applying: Bug 14385 Followup: C4::Borrowers not found
> Applying: Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
> Applying: Bug 14385: Squash of a lot of patches rebased
> fatal: sha1 information is lacking or useless (C4/Search.pm).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Bug 14385: Squash of a lot of patches rebased
> The copy of the patch that failed is found in:
>    /home/vagrant/kohaclone/.git/rebase-apply/patch
> When you have resolved this problem run "git bz apply --continue".
> If you would prefer to skip this patch, instead run "git bz apply --skip".
> To restore the original branch and stop patching run "git bz apply --abort".
> Patch left in /tmp/Bug-14385-Squash-of-a-lot-of-patches-rebased-PmRtSL.patch

cait: Are you working on this one ?
Comment 176 Katrin Fischer 2018-10-11 14:11:00 UTC
(In reply to Marcel de Rooy from comment #175)
> (In reply to Katrin Fischer from comment #173)
> > Please rebase.
> > 
> > Apply? [(y)es, (n)o, (i)nteractive] y
> > Applying: Bug 14385: Extend OpacHiddenItems to allow specifying exempt
> > borrower categories
> > Applying: Bug 14385 Followup: C4::Borrowers not found
> > Applying: Bug 14385: Followup: changes to sysprefs.sql and atomicupdate
> > Applying: Bug 14385: Squash of a lot of patches rebased
> > fatal: sha1 information is lacking or useless (C4/Search.pm).
> > Repository lacks necessary blobs to fall back on 3-way merge.
> > Cannot fall back to three-way merge.
> > Patch failed at 0001 Bug 14385: Squash of a lot of patches rebased
> > The copy of the patch that failed is found in:
> >    /home/vagrant/kohaclone/.git/rebase-apply/patch
> > When you have resolved this problem run "git bz apply --continue".
> > If you would prefer to skip this patch, instead run "git bz apply --skip".
> > To restore the original branch and stop patching run "git bz apply --abort".
> > Patch left in /tmp/Bug-14385-Squash-of-a-lot-of-patches-rebased-PmRtSL.patch
> 
> cait: Are you working on this one ?

Haven't started yet to look beyong applying, happy to hand over!
Comment 177 Marcel de Rooy 2018-10-12 05:29:49 UTC
QA: Looking here now
Comment 178 Marcel de Rooy 2018-10-12 09:35:36 UTC
Created attachment 80463 [details] [review]
Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

Edit: Fixing merge conflicts in
 - t/db_dependent/Items.t
 - t/db_dependent/Search.t
 - C4/Search.pm

Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in.
Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems

To test:

1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions)
2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions
3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library).
4) Log in as the borrower that should only see certain things (Adult - exception)
5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them.
6) Log in as the borrower that should see everything (Adult)
7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 179 Marcel de Rooy 2018-10-12 09:35:44 UTC
Created attachment 80464 [details] [review]
Bug 14385: (follow-up) C4::Borrowers not found

GetMember is in C4::Members, not C4::Borrowers

Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 180 Marcel de Rooy 2018-10-12 09:35:51 UTC
Created attachment 80465 [details] [review]
Bug 14385: (follow-up) Changes to sysprefs.sql and atomicupdate

Run updatedatabase.pl when testing

Sponsored-by: Catalyst IT

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 181 Marcel de Rooy 2018-10-12 09:36:00 UTC
Created attachment 80466 [details] [review]
Bug 14385: Squash of a lot of patches rebased

- Added missing GetHiddenItems parameter change case
     Without this prove t had a failure.

- Always use mocks, not set_preference

- Tweaks so t/db_dependent/00-strict.t passes
     There was a typo botcat vs borcat and borrowernumber was never
     defined. Grabbing from userenv, like other code does.

- Tweak t/db_dependent/Items.t to fully test changes
     This will test all the if structures fully in GetHiddenItemnumbers.
         prove t/db_dependent/Items.t

- Tweak borrower category code
     $borrower->{categorycode} on a Koha::Patron is not the
     same as $borrower->categorycode. Fixed error.

- Search was returning URLS for wrong interface
     There was one search context place wrong. Changed it to $is_opac
     as the logic for setting $is_opac was modified correctly.

- Corrected issues with category code.
     When a user isn't logged in, $borrower is undef and causes error
     when determining category code. Added conditional check.

- Properly trigger all changes in C4/Search.pm

- Fix QA Test tool failures
     C4/Search.pm had some tabs.

- Add some commenting to make sense of logic

- Refactor EmbedItemsInMarcBiblio parameters to hashref

- Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call.
     prove t/db_dependent/Items.t

- Add missing test to trigger Koha/BiblioUtils/Iterator change

- Add borrower category overrides
     These files generally add borcat parameter to GetMarcBiblio.
     Others might include correction of filtering of items
     (opac-basket), or a comment as to why no changes were done
     (opac-search).

     In the case of opac-search, correcting the first FIXME will
     likely correct the OpacHiddenItems issues on tags. As such,
     that is beyond this bugs scope.

     Some code had loop optimizations and fixes made, like a
     'next unless $record' when the biblio shouldn't even be in
     the list.

- Modify opac-ISBDdetail and opac-MARCdetail
     Both files had similar logic. They were rearranged and
     optimized, so that both files would have practically identical
     initial blocks of code.

     Optimizations were possible, because GetMarcBiblio
     returns a filtered record, so that there is no double call
     (once in the opac-### file and once in GetMarcBiblio) to
     GetHiddenItemnumbers.

- Fix hiding in opac-tags
     opac/opac-tags.pl was not properly hiding.

     There is currently one known bug associated with tags left.
     If you have two biblios tagged by different people with the
     same tag, the opac-search will show the one you tagged that
     is supposed to be hidden, because tag searches work differently
     than regular searches. This is beyond the scope of this bug.
     See the FIXME's in opac/opac-search.pl

- Trigger the C4::ILSDI::Services changes
     prove t/db_dependent/ILSDI_Services.t

- Added missing 'my'

- Test C4/Labels/Label.pm changes

- Improve C4::Record::marcrecord2csv test cases

- Corrected opac-details searchResult call

- Fix breaking issues constraint in ITerator test

- Fix ILSDI_Services test when clubs with branch exist

- Rebased again!
- Rebased t/db_dependent/Items.t conflict.

The test plan is in comment #112 last I checked.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 182 Marcel de Rooy 2018-10-12 09:36:09 UTC
Created attachment 80467 [details] [review]
Bug 14385: (QA follow-up) Additional changes and fixes

[1] searchResults: second my $interface can be removed: unused
[2] call of getitemtypeimagelocation on L2119 needs interface key
[3] ISBDdetail: No need to find patron again (line 182 vs 84)
[4] opac-search: No need to find patron twice (657 and 631)
[5] tabs on line 2220 of C4/Search.pm (qa tools warn)
[6] Ugly hack to overcome "Undefined subroutine &C4::Items::ModZebra"
    by loading C4::Items before C4::Biblio when running tests
    Koha/BiblioUtils/Iterator.t and Labels/t_Label.t.
    This is a more general problem that needs attention somewhere else.
    It seems that Biblio.pm is one of the suspects.
[7] This patch set makes Search.t crash/fail with me. Note that without
    these patches Search.t still passed! Why o why..
    A little debugging pointed me to a missing MPL branch (aarg).
    Adding the simple test on the result of Libraries->find in
    C4::Biblio::GetAuthorisedValueDesc made the test continue.
[8] Resolve: Variable "$borcat" is not available at opac-detail.pl line 246
    Lexical $borcat cannot be used in sub searchAgain in opac-detail.pl
    under Plack. Must be defined with our (or passed as argument).
[9] Resolve crash on TWO serious typos in opac-basket on ONE line:
        Koha::Patron->find({ borrowernumber -> $borrowernumber })
    Yeah: find is in Koha::Patrons and we need => !!
    No need to pass a hash to find method btw for a pk value.
[10] Serious bugfixing here. Add List::Util to opac-basket.
    Can't locate object method "none" via package "1".
    You can't test everything :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
After this longer list I renamed Final to Additional in the patch title :)
Comment 183 Marcel de Rooy 2018-10-12 09:38:42 UTC
Just a few minor question written down during code review without exploring them further:

TODO why change for $oldbiblio->{'authorised_value_images'}
TODO $search_context->{category} => ADD to POD
TODO Waarom addbooks: if ( not exists $line->{'size'} ) { $line->{'size'} = "" }
TODO Why add use Koha::Libraries; to opac-search ?
Comment 184 Marcel de Rooy 2018-10-12 09:40:30 UTC
QA Comment:

Great work. Spent whole morning on it so there will be comments (see also follow-up):

Since you adjust EmbedItemsInMarcBiblio more fundamentally and add a borcat parameter, I would have at least expected the borcat parameter in the subtest for this routine. Bad luck however.
Considering the history of this report, and the demonstrated attention for other tests here, I will not be too strict on that. Knowing that you will submit them on a new report :)

No blocker but something worth fixing / feature extending ?
Have a book with two items. One item must be hidden. In the OPAC I see:
Availability: Items available for loan: RMA [Call number: 200 P 1] (2). 
This line on opac-search or opac-shelves.pl shows me (in this case) the call number of the hidden item ! Also the number 2 reveals that there are more items than shown on opac-detail.
Could imagine that these "hidden books" may stand on a shelf number that you do not want to display either.. 
When I put the book in the cart and email the cart, I will see both items in the cart. How about hiding that one ?

Similar point.
Although you could argue that you have a special circ rule for these hidden items, without such a rule opac-reserve shows me the hidden items if I allow item holds. Which it should not imo.
Not a blocker for me now, but it feels even a bit more serious than the former point.

And thinking further: In the staff client the hidden item pops up even if I do not put the staff category in the new pref. OK, I understand that. But if I export a biblio with Save to marcxml or export data under tools etc. the item also pops up everywhere. Certainly not a blocker, but should the feature of hiding items not have some impact in such things too ? Same for SRU/Z39.50 output (item just there). Etc.

In conclusion: This patch set looks good enough to me. The feature has been developed (before) picking mostly low fruit. Looking a bit further, items are not that hidden after all..
Comment 185 Mark Tompsett 2018-10-12 14:27:23 UTC
Comment on attachment 80467 [details] [review]
Bug 14385: (QA follow-up) Additional changes and fixes

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

Thank you very much for a Passed QA. Just had one question about one optimization you made in opac/opac-search.pl as I eyeball'd through the patch. The other things are just comments as I was looking through it.

Regarding the leak via counting being off. This counting problem already leaks and is an issue. Fixing the counting problem is a complex problem, which has had recent comments on the mailing list. Given the size and scope of testing for this patch, I did not want to keep coding for perfection. Perfection is the enemy of good, or some variation thereof. :)

::: C4/Search.pm
@@ +2215,5 @@
>                  else {
>                      $can_place_holds = 1;
>                      $available_count++;
> +                    $available_items->{$prefix}->{count}++ if $item->{$hbranch};
> +                    foreach (qw(branchname itemcallnumber description)) {

White space fix.

::: opac/opac-ISBDdetail.pl
@@ +82,3 @@
>  my $borcat = q{};
> +if ( $patron && C4::Context->preference('OpacHiddenItemsExceptions') ) {
> +    $borcat = $patron->categorycode;

Nice optimization.

::: opac/opac-basket.pl
@@ -59,4 @@
>  my $borcat = q{};
>  if ( C4::Context->preference('OpacHiddenItemsExceptions') ) {
>      # we need to fetch the borrower info here, so we can pass the category
> -    my $borrower = Koha::Patron->find( { borrowernumber -> $borrowernumber } );

Oops... should have been => good catch.

::: opac/opac-detail.pl
@@ +85,4 @@
>  my @all_items = GetItemsInfo($biblionumber);
>  my @hiddenitems;
>  my $patron = Koha::Patrons->find( $borrowernumber );
> +our $borcat= q{};

I would have preferred passing, rather than changing to our.

::: opac/opac-search.pl
@@ +125,4 @@
>      authnotrequired => ( C4::Context->preference("OpacPublic") ? 1 : 0 ),
>      }
>  );
> +my $patron = Koha::Patrons->find( $borrowernumber );

Not an exact copy of 658 during the optimization. Will this affect things?
Comment 186 Marcel de Rooy 2018-10-15 07:17:36 UTC
(In reply to M. Tompsett from comment #185)
> Regarding the leak via counting being off. This counting problem already
> leaks and is an issue. Fixing the counting problem is a complex problem,
> which has had recent comments on the mailing list. Given the size and scope
> of testing for this patch, I did not want to keep coding for perfection.
> Perfection is the enemy of good, or some variation thereof. :)

Not sure if we talk about the same counting problem. There is the problem of leaving a search result out (having 19 hits on a page instead of 20 etc.) but I was referring to the listing of 2 items where one is hidden. At least we know that there is one hidden :) But no need to address that here.

> ::: opac/opac-search.pl
> @@ +125,4 @@
> >      authnotrequired => ( C4::Context->preference("OpacPublic") ? 1 : 0 ),
> >      }
> >  );
> > +my $patron = Koha::Patrons->find( $borrowernumber );
> 
> Not an exact copy of 658 during the optimization. Will this affect things?

Not a problem. We could test on borrowernumber again, but find(undef) will return nothing either and this is consistent with the common approach.
The line using $patron is testing on $patron. So should be fine.
Comment 187 Nick Clemens 2018-11-02 10:43:48 UTC
Awesome work all!

Pushed to master for 18.11
Comment 188 Martin Renvoize 2018-11-16 12:09:57 UTC
Enhancement will not be backported to 18.05.x series.
Comment 189 Jonathan Druart 2019-04-29 00:06:48 UTC
I suspect a wrong resolution conflict here, see
  commit 5e4e10c4ca558180137bf5a4ff5a68495efa0ec7
  Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories

The following line has been removed for years, and re-added by this patchset:

+        $oldbiblio->{'authorised_value_images'}  = ($search_context->{'interface'} eq 'opac' && C4::Context->preference('AuthorisedValueImages')) || ($search_context->{'interface'} eq 'intranet' && C4::Context->preference('StaffAuthorisedValueImages')) ? C4::Items::get_authorised_value_images( C4::Biblio::get_biblio_authorised_values( $oldbiblio->{'biblionumber'}, $marcrecord ) ) : [];

At first glance I'd say we just need a patch removing the line.
Comment 190 Jonathan Druart 2019-04-29 00:08:26 UTC
Note: The routine C4::Biblio::get_biblio_authorised_values does not longer exist!
Comment 191 Jonathan Druart 2019-04-29 00:13:10 UTC
And the 2 prefs (AuthorisedValueImages and StaffAuthorisedValueImages) have been removed by bug 16167, that's why we never call the non-existing routine.
Comment 192 Nick Clemens 2019-05-22 14:50:30 UTC
Created attachment 89963 [details] [review]
Bug 14385: (RM follow-up) Remove line from bad rebase
Comment 193 Nick Clemens 2019-05-22 14:51:12 UTC
(In reply to Jonathan Druart from comment #190)
> Note: The routine C4::Biblio::get_biblio_authorised_values does not longer
> exist!

Can you confirm the supplied patch?
Comment 194 Martin Renvoize 2019-05-23 15:27:13 UTC
Created attachment 90038 [details] [review]
Bug 14385: (RM follow-up) Remove line from bad rebase

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 195 Martin Renvoize 2019-05-23 15:28:02 UTC
Followup looks solid, go forth and push :)
Comment 196 Nick Clemens 2019-05-23 15:46:47 UTC
Last follow-up pushed to master
Comment 197 Jonathan Druart 2019-12-18 11:24:08 UTC
From this commit 70651422a7517338a68e22321d9918757746ef33
    Bug 14385: (QA follow-up) Additional changes and fixes

@ Biblio.pm:1551 @ sub GetAuthorisedValueDesc {

         #---- branch
         if ( $tagslib->{$tag}->{$subfield}->{'authorised_value'} eq "branches" ) {
-            return Koha::Libraries->find($value)->branchname;
+            my $branch = Koha::Libraries->find($value);
+            return $branch? $branch->branchname: q{};
         }


We are actually hiding a data inconsistency. I would recommend to revert that change and fix the failing test properly instead.
Comment 198 Katrin Fischer 2020-01-02 22:51:48 UTC
(In reply to Jonathan Druart from comment #197)
> From this commit 70651422a7517338a68e22321d9918757746ef33
>     Bug 14385: (QA follow-up) Additional changes and fixes
> 
> @ Biblio.pm:1551 @ sub GetAuthorisedValueDesc {
> 
>          #---- branch
>          if ( $tagslib->{$tag}->{$subfield}->{'authorised_value'} eq
> "branches" ) {
> -            return Koha::Libraries->find($value)->branchname;
> +            my $branch = Koha::Libraries->find($value);
> +            return $branch? $branch->branchname: q{};
>          }
> 
> 
> We are actually hiding a data inconsistency. I would recommend to revert
> that change and fix the failing test properly instead.

I think this got lost here - should we file a new bug report? Not sure how to phrase it.