Description
Aleisha Amohia
2015-06-15 04:54:26 UTC
Created attachment 40158 [details] [review] Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories 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 Another one I can't sign off on, could someone please test this CONFLICT (content): Merge conflict in C4/Search.pm Also consider the refactor I'm doing on bug 16335. I just put up my filter work. Created attachment 53483 [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 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 Created attachment 53494 [details] [review] Bug 14385 Followup: C4::Borrowers not found GetMember is in C4::Members, not C4::Borrowers I have not tested the functionality, but the followup makes the error go away to allow testing. Created attachment 53505 [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> 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> 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> 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? *** Bug 10589 has been marked as a duplicate of this bug. *** (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? (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. (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. Created attachment 55593 [details] [review] Bug 14385: Followup: changes to sysprefs.sql and atomicupdate Run updatedatabase.pl when testing Sponsored-by: Catalyst IT 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 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. 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. Found a missing C4/Biblio.pm modification... Still looking further. 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. 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> 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> Created attachment 64690 [details] [review] Bug 14385: Followup: changes to sysprefs.sql and atomicupdate Run updatedatabase.pl when testing Sponsored-by: Catalyst IT Created attachment 64691 [details] [review] Bug 14385: Added missing GetHiddenItems parameter change case Without this prove t had a failure. Created attachment 64692 [details] [review] Bug 14385: Always use mocks, not set_preference 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. Bug 17829 means the C4::Member::GetMember logic here doesn't work. Tweaking. 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. 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. 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 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. 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. 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. Created attachment 65376 [details] [review] Bug 14385: Properly trigger all changes in C4/Search.pm prove t/db_dependent/Search.t Created attachment 65377 [details] [review] Bug 14385: Fix QA Test tool failures C4/Search.pm had some tabs. 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> 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> 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> 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> Created attachment 65382 [details] [review] Bug 14385: Always use mocks, not set_preference Signed-off-by: Mark Tompsett <mtompset@hotmail.com> 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> 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> 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> 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> 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> Created attachment 65388 [details] [review] Bug 14385: Properly trigger all changes in C4/Search.pm Signed-off-by: Mark Tompsett <mtompset@hotmail.com> 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> 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. *sigh* found an issue. EmbedItemsInMarcBiblio. If $opac, we should be passing borcat. Created attachment 65408 [details] [review] Bug 14385: Add some commenting to make sense of logic Created attachment 65409 [details] [review] Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref Created attachment 65410 [details] [review] Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call. prove t/db_dependent/Items.t Created attachment 65411 [details] [review] Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change 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. 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. (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. 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> 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> 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> 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> Created attachment 66107 [details] [review] Bug 14385: Always use mocks, not set_preference Signed-off-by: Mark Tompsett <mtompset@hotmail.com> 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> 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> 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> 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> 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> Created attachment 66113 [details] [review] Bug 14385: Properly trigger all changes in C4/Search.pm Signed-off-by: Mark Tompsett <mtompset@hotmail.com> 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> Created attachment 66115 [details] [review] Bug 14385: Add some commenting to make sense of logic Created attachment 66116 [details] [review] Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref Created attachment 66117 [details] [review] Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call. prove t/db_dependent/Items.t Created attachment 66118 [details] [review] Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change Created attachment 66125 [details] [review] Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref Created attachment 66126 [details] [review] Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call. prove t/db_dependent/Items.t Created attachment 66127 [details] [review] Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change Created attachment 66128 [details] [review] Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref Created attachment 66129 [details] [review] Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call. prove t/db_dependent/Items.t Created attachment 66130 [details] [review] Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change Created attachment 66131 [details] [review] Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call. prove t/db_dependent/Items.t Created attachment 66132 [details] [review] Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change 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> 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> 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> 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> Created attachment 66196 [details] [review] Bug 14385: Always use mocks, not set_preference Signed-off-by: Mark Tompsett <mtompset@hotmail.com> 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> 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> 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> 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> 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> Created attachment 66202 [details] [review] Bug 14385: Properly trigger all changes in C4/Search.pm Signed-off-by: Mark Tompsett <mtompset@hotmail.com> 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> Created attachment 66204 [details] [review] Bug 14385: Add some commenting to make sense of logic Created attachment 66205 [details] [review] Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref Created attachment 66206 [details] [review] Bug 14385: Trigger GetMarcBiblio's EmbedItemsInMarcBiblio call. prove t/db_dependent/Items.t Created attachment 66207 [details] [review] Bug 14385: Add missing test to trigger Koha/BiblioUtils/Iterator change 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. 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. 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 51 and 58 constitute the most recent test plan. A more complete single one to come soon. 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. Created attachment 66302 [details] [review] Bug 14385: Trigger the C4::ILSDI::Services changes prove t/db_dependent/ILSDI_Services.t Created attachment 66303 [details] [review] Bug 14385: Added missing 'my' Created attachment 66347 [details] [review] Bug 14385: Test C4/Labels/Label.pm changes Created attachment 66361 [details] [review] Bug 14385: Improve C4::Record::marcrecord2csv test cases Created attachment 66372 [details] [review] Bug 14385: Corrected opac-details searchResult call Created attachment 66401 [details] [review] Bug 14385: Fix breaking issues constraint in ITerator test Created attachment 66403 [details] [review] Bug 14385: Fix ILSDI_Services test when clubs with branch exist 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 #112 represents the latest test plan that I followed. I'm calling this done. Multiple eyes and sign offs on this would be appreciated. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 66533 [details] [review] [SIGNED-OFF] Bug 14385: Refactor EmbedItemsInMarcBiblio parameters to hashref Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> 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> 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> 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> 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> 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> 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> Created attachment 66540 [details] [review] [SIGNED-OFF] Bug 14385: Added missing 'my' Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Created attachment 66541 [details] [review] [SIGNED-OFF] Bug 14385: Test C4/Labels/Label.pm changes Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Created attachment 66542 [details] [review] [SIGNED-OFF] Bug 14385: Improve C4::Record::marcrecord2csv test cases Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Created attachment 66543 [details] [review] [SIGNED-OFF] Bug 14385: Corrected opac-details searchResult call Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Created attachment 66544 [details] [review] [SIGNED-OFF] Bug 14385: Fix breaking issues constraint in ITerator test Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> 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> Sorry for the spam! Signing off on all patches (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. 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 ;) (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. a+ preserving attribution. :) 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> 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> 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> 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. 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. Currently does not apply, reworking. 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. :) 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> 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> 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> 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. 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. 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. 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. 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. 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. 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. 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. 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. 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. 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> 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> 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> 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. 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> 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> 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> 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. 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 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. (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 ? (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! QA: Looking here now 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> 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> 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> 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> 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 :) 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 ? 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 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? (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. Awesome work all! Pushed to master for 18.11 Enhancement will not be backported to 18.05.x series. 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. Note: The routine C4::Biblio::get_biblio_authorised_values does not longer exist! And the 2 prefs (AuthorisedValueImages and StaffAuthorisedValueImages) have been removed by bug 16167, that's why we never call the non-existing routine. Created attachment 89963 [details] [review] Bug 14385: (RM follow-up) Remove line from bad rebase (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? 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> Followup looks solid, go forth and push :) Last follow-up pushed to master 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. (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. |