Bug 10872 - C4::Items GetHiddenItems fix and optimization
Summary: C4::Items GetHiddenItems fix and optimization
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 10584
  Show dependency treegraph
 
Reported: 2013-09-10 20:13 UTC by Mark Tompsett
Modified: 2014-05-26 21:04 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10872 - C4::Items.pm has YAML::Load in GetHiddenItems but lacks use or require (960 bytes, patch)
2013-09-10 20:22 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10872 - C4::Items GetHiddenItems fix and optimization (1.38 KB, patch)
2013-09-10 21:14 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10872 - C4::Items GetHiddenItems fix and optimization (1.46 KB, patch)
2013-09-13 03:49 UTC, Mason James
Details | Diff | Splinter Review
Bug 10872 - C4::Items GetHiddenItems fix and optimization (1.59 KB, patch)
2013-09-13 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10872: UT for GetHiddenItemnumbers and POD fix (7.78 KB, patch)
2013-09-16 18:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2013-09-10 20:13:21 UTC
I believe the description covers it all. There is no testing, except does anything using C4::Items still work?

There should be a:
"require YAML;"
or
"use YAML qw/Load/;"

as the GetHiddenItems routine has a reference to YAML::Load.

This was discovered while adding GetHiddenItems() calls into opac/opac-MARCdetail.pl. I believe this problem dates back to bug 6488 or bug 5984.
Comment 1 Mark Tompsett 2013-09-10 20:22:30 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2013-09-10 20:48:52 UTC
It's not really an enhancement, as it should have been optimized in the first place. Changed to a "minor" bug as a result, since part of this wasn't even visible without another bug fix which is yet to come.
Comment 3 Mark Tompsett 2013-09-10 21:14:58 UTC Comment hidden (obsolete)
Comment 4 Mason James 2013-09-13 03:49:26 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2013-09-13 09:18:27 UTC
Created attachment 21075 [details] [review]
Bug 10872 - C4::Items GetHiddenItems fix and optimization

There should be a:
"require YAML;"
or
"use YAML qw/Load/;"

as the GetHiddenItems routine has a reference to YAML::Load.

This was discovered while adding a GetHiddenItems() call into
opac/opac-MARCdetail.pl. I believe this problem dates back to
bug 6488 or bug 5984.

I also added an optimization to GetHiddenItems to prevent
processing if there is nothing in the system preference. Test
by searching for a biblio which has some or all of its items
hidden.

Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Maybe
  return () unless $yaml =~ /\S/;
or
  return () if $yaml =~ /^\s*$/;
would have been easier to read.
Comment 6 Tomás Cohen Arazi 2013-09-16 18:39:08 UTC
Created attachment 21125 [details] [review]
Bug 10872: UT for GetHiddenItemnumbers and POD fix

This patch adds several unit tests for GetHiddenItemnumbers and fixes the POD for it.
It also wraps the tests for rollback, modernizes and adds a license text to it.

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Comment 7 Galen Charlton 2013-09-18 15:59:40 UTC
Reviewed and passed QA on this, as it is part of a patch series that I want to push as a unit.
Comment 8 Galen Charlton 2013-09-18 16:08:15 UTC
Pushed to master.  Thanks, Mark!
Comment 9 Tomás Cohen Arazi 2013-11-11 15:13:12 UTC
This patch has been pushed to 3.12.x, will be in 3.12.7.

Thanks Mark!