Bug 10937 - Advanced search: Hide itemtypes from search form & Group itemtypes together into one search option
Summary: Advanced search: Hide itemtypes from search form & Group itemtypes together i...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Maxime Beaulieu
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 17212 17531 15092 15093 17134
  Show dependency treegraph
 
Reported: 2013-09-24 14:01 UTC by Katrin Fischer
Modified: 2017-05-12 08:33 UTC (History)
19 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 10937 - Options to group or hide items in adv. search (19.11 KB, patch)
2013-10-15 18:47 UTC, Blou
Details | Diff | Splinter Review
Sponsor: Vanier college (10.29 KB, patch)
2013-10-22 14:36 UTC, Blou
Details | Diff | Splinter Review
Conflicts: installer/data/mysql/updatedatabase.pl (17.61 KB, patch)
2014-01-21 15:25 UTC, Blou
Details | Diff | Splinter Review
Bug 10937 - Option to hide and group itemtypes from advanced search (18.94 KB, patch)
2014-01-21 15:28 UTC, Blou
Details | Diff | Splinter Review
Bug 10937 - Option to hide and group itemtypes from advanced search (17.63 KB, patch)
2014-01-27 18:39 UTC, Blou
Details | Diff | Splinter Review
Fix tab issues (2.09 KB, patch)
2014-01-27 19:07 UTC, Blou
Details | Diff | Splinter Review
(Re?)added update message and set version to 3.15.XXX (935 bytes, patch)
2014-01-27 21:02 UTC, Blou
Details | Diff | Splinter Review
Fix and rebase following mtompset's comments (17.78 KB, patch)
2014-05-30 14:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Fix tab issues (1.69 KB, patch)
2014-05-30 14:03 UTC, M. Tompsett
Details | Diff | Splinter Review
(Re?)added update message and set version to 3.15.XXX (934 bytes, patch)
2014-05-30 14:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10937 - Option to hide and group itemtypes from advanced search (19.73 KB, patch)
2014-05-30 14:16 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10937 - Option to hide and group itemtypes from advanced search (20.51 KB, patch)
2014-05-30 15:01 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10937 - Option to hide and group itemtypes from advanced search (21.02 KB, patch)
2014-05-30 21:51 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10937 - Add unit tests. (2.79 KB, patch)
2014-06-02 20:13 UTC, Blou
Details | Diff | Splinter Review
Bug 10937 - Option to hide and group itemtypes from advanced search (24.25 KB, patch)
2014-06-02 22:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10947 - Follow-up to: 'Review of attachment 28626' (10.96 KB, patch)
2014-06-30 18:38 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master. (22.08 KB, patch)
2014-07-29 20:59 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master. (22.24 KB, patch)
2014-09-26 13:16 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master. (19.84 KB, patch)
2014-12-15 13:26 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 10937 - Grouped ItemTypes - Patch should apply properly on latest master. (20.00 KB, patch)
2014-12-22 14:12 UTC, simith.doliveira
Details | Diff | Splinter Review
[Signed-off] Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master. (20.21 KB, patch)
2014-12-24 10:10 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 10937 - Grouped ItemTypes - Patch should apply properly on latest master. (22.24 KB, patch)
2015-01-21 20:22 UTC, Ahmed Sfayhi
Details | Diff | Splinter Review
Bug 10937 - Grouped ItemTypes - Patch should apply properly on latest master. (22.22 KB, patch)
2015-02-26 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10937 - Grouped ItemTypes - Patch should apply properly on latest master. (22.28 KB, patch)
2015-02-27 22:46 UTC, Nick Clemens
Details | Diff | Splinter Review
[PASSED QA] Bug 10937 - Option to hide and group itemtypes from advanced search (22.34 KB, patch)
2015-03-13 13:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10937 - cleanup and rename DOCTYPECAT to ITEMTYPECAT (6.32 KB, patch)
2015-07-09 13:26 UTC, Blou
Details | Diff | Splinter Review
Bug 10937 - Fix Seach Type in Advanced Search (1.63 KB, patch)
2015-09-03 13:39 UTC, Genevieve Plantin
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-09-24 14:01:05 UTC
Our use case are item types used for ILL - it's possible to hide the records and the items, but the advanced search form is a bit trickier. It would be nice if we could specify which itemtypes are offered as search options in the OPAC advanced search form.
Comment 1 Eric Bégin 2013-10-03 17:38:49 UTC
Two new columns will be added in the item type.

1) Display in OPAC
Default : Yes
Set to NO to hide and itemtype in the opac's advanced search.

2) Search category
The search category allows library to group some « similar » itemtypes in a single checkbox in the opac.
The categories are defined in an Authorized value.

For Example, itemtypes DVD, VHS could use the Video category.
Searching for Video in the OPAC would returns the DVD and VHS items
Comment 2 Blou 2013-10-15 18:47:49 UTC Comment hidden (obsolete)
Comment 3 David Cook 2013-10-21 21:38:17 UTC
I'm glad that someone has worked on this issue as I was just talking about it a few hours ago!

However:

1) There is an issue with your code in regards to "Search Category". If an item type has a "search category" of none, it won't appear on the Advanced Search page.

That is...if "searchcategory" is null in the database and "hideinopac" is 0, the item type will appear.

However, if "searchcategory" is blank and "hideinopac" is 0, the item type will be hidden. 

"searchcategory" will be blank any time an item type is saved and "None" is selected as the Search Category. Definitely not a good thing!

There are a few other things that you will also want to consider in revising your patch:

2) Whitespace changes should be made in a separate patch so it's easier to separate code differences and format differences (like in kohastructure.sql).

3) You have a typo on the Add/Edit item type page (http://devonestaff/cgi-bin/koha/admin/itemtypes.pl?op=add_form).

It says "Advanded Search" rather than "Advanced Search".

--

You might want to also consider the following:

4) Your test plan assumes that there is an authorized value category of "DOCTYPECAT". You should mention that most users will probably have to create this authorized value category.

Also, your test plan says to choose "Hardware" in the staff client, but the authorized value will actually be "Anything" according to your plan. Of course, it should be the only search category available (other than none), but the test plan is a rather dense block of text, so I found it difficult to tell exactly what I was supposed to see.

It might be useful to add some whitespace between the test plan steps to make it easier to read. 

--

I'll be happy to look again once you have an updated patch! Thanks, Philippe.
Comment 4 Blou 2013-10-22 14:36:20 UTC Comment hidden (obsolete)
Comment 5 Blou 2013-10-22 14:40:18 UTC
The patch is additive.  The first one is required.
I rebased before doing it, so all should be good.

Feedbacks are welcomed.
Comment 6 Kyle M Hall 2013-10-25 14:43:39 UTC
Applying: Sponsor: Vanier college
/home/koha/kohaclone/.git/rebase-apply/patch:24: tab in indent.
            LEFT JOIN authorised_values ON searchcategory = authorised_value WHERE searchcategory > ''
/home/koha/kohaclone/.git/rebase-apply/patch:62: tab in indent.
        quantity SMALLINT(6) default NULL, -- suggested quantity to be purchased
/home/koha/kohaclone/.git/rebase-apply/patch:63: tab in indent.
        currency VARCHAR(3) default NULL, -- suggested currency for the suggested price
/home/koha/kohaclone/.git/rebase-apply/patch:64: tab in indent.
        price DECIMAL(28,6) default NULL, -- suggested price
/home/koha/kohaclone/.git/rebase-apply/patch:65: tab in indent.
        total DECIMAL(28,6) default NULL, -- suggested total cost (price*quantity updated for currency)
fatal: sha1 information is lacking or useless (C4/Koha.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Sponsor: Vanier college
Comment 7 mathieu saby 2013-12-25 18:39:37 UTC
Hello
Could you write a followup for the new default bootstrap theme?

Mathieu
Comment 8 Katrin Fischer 2013-12-29 21:44:32 UTC
Patches apply for me, for the second patch I think the subject should be a sponsor line :) (http://wiki.koha-community.org/wiki/Commit_messages#Sponsor_line)
Comment 9 Katrin Fischer 2013-12-29 21:48:04 UTC
Blou, could you take a look and provide a follow-up for the bootstrap theme? It's the default theme now and we shouldn't add a new feature that doesn't work with it.

Also, both patches seem almost identical - I am obsoleting the older patch.
Comment 10 Blou 2014-01-21 15:25:28 UTC Comment hidden (obsolete)
Comment 11 Blou 2014-01-21 15:28:17 UTC Comment hidden (obsolete)
Comment 12 M. Tompsett 2014-01-23 20:38:38 UTC
Comment on attachment 24597 [details] [review]
Bug 10937 - Option to hide and group itemtypes from advanced search

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

::: C4/Koha.pm
@@ +41,4 @@
>  		&slashifyDate
>  		&subfield_is_koha_internal_p
>  		&GetPrinters &GetPrinter
> +		&GetItemTypes &GetItemTypesCategorized &GetItemTypesByCategory &getitemtypeinfo

No problem, but I would have put the new functions on a different line, so as to reduce the chance of messing up the old ones.

@@ +270,4 @@
>      }
>  }
>  
> +=head2 GetItemTypes

GetItemTypesCategorized. Oops!

::: admin/itemtypes.pl
@@ +168,4 @@
>              $input->param('summary'),
>              $input->param('checkinmsg'),
>              $input->param('checkinmsgtype'),
> +            ( $input->param('hideinopac') ? 1 : 0 ),

This one is in parenthesis?

@@ +193,4 @@
>              $input->param('summary'),
>              $input->param('checkinmsg'),
>              $input->param('checkinmsgtype'),
> +            $input->param('hideinopac') ? 1 : 0,

This one is not? It might be best to avoid the inline expr?true:false in this list.

::: installer/data/mysql/kohastructure.sql
@@ +1241,5 @@
>    summary text, -- information from the summary field, may include HTML
>    checkinmsg VARCHAR(255), -- message that is displayed when an item with the given item type is checked in
>    checkinmsgtype CHAR(16) DEFAULT 'message' NOT NULL, -- type (CSS class) for the checkinmsg, can be "alert" or "message"
> +  hideinopac tinyint(1) NOT NULL DEFAULT 0, -- Hide the item type from the search options in OPAC
> +  searchcategory varchar(15) default NULL, -- Group this item type with others with the same value on OPAC search options

How did you decide on varchar(15)?

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-advsearch.tt
@@ +146,3 @@
>                                              [% itemtypeloo.description %]</label></div>
>                                              [% IF ( loop.last ) %]</div>[% ELSE %][% UNLESS ( loop.count % 4 ) %]</div><div class="row-fluid">[% END %][% END %]
> +                                            [% END %]

This END isn't even the area you were adding your IF around. Why change the indentation? Indentation issues should perhaps be a separate patch.
Comment 13 M. Tompsett 2014-01-23 20:41:58 UTC
Does not apply to master. Please rebase.
Comment 14 Blou 2014-01-27 18:39:03 UTC Comment hidden (obsolete)
Comment 15 M. Tompsett 2014-01-27 18:58:05 UTC
Applies fine.

mtompset@ubuntu:~/kohaclone$ koha-qa.pl -v 2 -c 1
testing 1 commit(s) (applied to bab96c3 'Bug 11571: fix breakage of -f option ')

 FAIL   C4/Koha.pm
   OK     pod
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 288)
                forbidden pattern: tab char (line 45)
   OK     valid
   OK     critic

...

 FAIL   opac/opac-search.pl
   OK     pod
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 411)
                forbidden pattern: tab char (line 228)
   OK     valid
   OK     critic

Please correct your tab issues.
Comment 16 Blou 2014-01-27 19:07:03 UTC Comment hidden (obsolete)
Comment 17 M. Tompsett 2014-01-27 19:22:40 UTC
Comment on attachment 24778 [details] [review]
Bug 10937 - Option to hide and group itemtypes from advanced search

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

::: installer/data/mysql/updatedatabase.pl
@@ +7950,5 @@
> +    $dbh->do(q{
> +        ALTER TABLE itemtypes
> +            ADD hideinopac TINYINT(1) NOT NULL DEFAULT 0,
> +            ADD searchcategory VARCHAR(20) DEFAULT NULL;
> +    });

You forgot to print out a meaningful message and call SetVersion, like the other ones. :)
Comment 18 M. Tompsett 2014-01-27 20:23:11 UTC
CURRENT TEST PLAN
------------------
0) Back up database, so you can reset and retest easily. ;)
1) Run Koha QA tool.
2) Apply the patch

SETUP
3) run ./installer/data/mysql/updatedatabase.pl to add the
    two columns to itemtypes
   -- Does a meaningful message get printed?
      Are the columns added?
       "DESCRIBE itemtypes;" should list hideinopac and searchcategory.
4) You need to add a category to group your item types:
   a) In Intranet/Koha Admin/Authorized values,
       select DOCTYPECAT in the 'Show category:' dropdown
   b) Click button "New authorized value for DOCTYPECAT"
   c) Enter
         Authorized value: HARDWARE
         Description : Hardware
         Description (OPAC): Hardware

NORMAL USAGE
2) Group your items under that new category
   a) In Intranet/Koha Admin/Item types, choose (at least) two item types and for each:
      - Click action/Edit on the right column
      - Third row (below Description) is the Search category list box, select Hardware
      - click Save changes at the bottom
3) Select at least one item to be hidden in the OPAC search
   a) In Intranet/Koha Admin/Item types (again), choose a different item type:
      -  Click action/Edit
      -  Click the checkbox "Hide in OPAC" below the list of icons.

FINAL TESTING
4) Go test your modifications
   a) Go to OPAC/Adv search.
   b) Validate that all items modified above (hidden or grouped) do not appear in Item type list
   c) Validate that new item type Hardware does appear instead.
   d) Select item Hardware, start Search.
   e) Validate returned items are the of the two types that were grouped into the Hardware category in step 4.

More revisions to follow once I actually start testing this fully. :)
Comment 19 Blou 2014-01-27 21:02:59 UTC Comment hidden (obsolete)
Comment 20 M. Tompsett 2014-01-28 03:10:41 UTC
Okay... that fresh install works. the update works.
the editing the itemtype stuff all seems to work.
But I can't get it to search properly.

ARTCL 	Article
AUD 	Audio CD
BK 	Books
CF 	Computer Files
CR 	Continuing Resources
EBK 	Ebooks, Online Resources
IM 	In house Material
MP 	Maps
MF 	Microfiche
MX 	Mixed Materials
MU 	Music
REF 	Reference
RPR 	Reprint, VFile
TH 	Thesis, Dissertation
VCD 	Video CD
VM 	Visual Materials

These are my item types. I took the first 8 and did a pretty 0-7 binary combination with HARDWARE, Hide in OPAC, and Not for Loan.

I then went to the OPAC interface, clicked on Advanced Search and tried to make sense of the Item type box. 10 things plus hardware were displayed. 16-4 (hidden in OPAC) = 12, 12-2 (not hidden in OPAC, but HARDWARE) = 10. plus hardware. Display looks good.

I checked hardware, and then clicked the Search button at the bottom of the page.

I got a bunch of results, but this is where it doesn't make sense. My first result was a journal (Continuing Resources), which I had marked as no category, not for loan, hide in OPAC. The fact that the search returned a no category, and not the hardware category makes it seem broken.

Here is the URL (the name doesn't exist): https://demo.library.kohasystem.ca/cgi-bin/koha/opac-search.pl?idx=kw&q=&idx=kw&q=&idx=kw&q=&limit-yr=&searchcat=HARDWARE&limit=&limit=&multibranchlimit=&sort_by=relevance&do=Search

Hmmm... I changed the sort by drop down to title A-Z and the first one was BK, so that was expected. The result count is strangely high too, but perhaps I am mistaken in that account. I'll check manually later. Until then, I'm leaving this as Failed QA, because the search by relevance gave me a bad result, and I don't know if it is this patch, or just the by relevance search in general.
Comment 21 Blou 2014-01-28 18:08:29 UTC
Results hard to reproduce, I'll wait on your further testing results.

If you get "unwanted results", like the "no category" you listed below, could you examine the search string in the header, something like 

Results of search  with limit(s): 'mc-itype,phr:CDR or mc-itype,phr:VG or mc-itype,phr:VT' 

That is what is generated when HARDWARE is exploded in my latest test with URL:
cgi-bin/koha/opac-search.pl?idx=kw&q=&idx=kw&q=&idx=kw&q=&do=Search&searchcat=HARDWARE&limit-yr=&limit=&limit=&sort_by=relevance

Thanks
Comment 22 M. Tompsett 2014-05-30 14:02:59 UTC Comment hidden (obsolete)
Comment 23 M. Tompsett 2014-05-30 14:03:05 UTC Comment hidden (obsolete)
Comment 24 M. Tompsett 2014-05-30 14:03:13 UTC Comment hidden (obsolete)
Comment 25 M. Tompsett 2014-05-30 14:16:56 UTC Comment hidden (obsolete)
Comment 26 M. Tompsett 2014-05-30 15:01:39 UTC Comment hidden (obsolete)
Comment 27 M. Tompsett 2014-05-30 15:05:38 UTC
I think there is some inclusion/exclusion logic going awry here. If we don't show an item in the item types available in the advanced search, we likely do not want the user to actually get results with those in them. However, the results will still return them. If I hide everything except one item type, then if I don't select that item type, then everything is returned. If I do select just that item type, then the results will only be limited in that item type. Is this really what is desired?
Comment 28 M. Tompsett 2014-05-30 15:07:18 UTC
Also, rather than just dump this to failed qa, because there is a lack of C4/Koha test for the new routines, I'd like to figure out this discussion first.
Comment 29 M. Tompsett 2014-05-30 17:43:13 UTC
Okay, if OpacHiddenItems is set in a corresponding manner to the item types being marked as hidden in OPAC, then I think this should suffice. That is assuming that you only want the individual items not listed if they are grouped together. However, this is lacking testing for the C4/Koha routines that were added. Marking as Failed QA.
Comment 30 M. Tompsett 2014-05-30 21:51:36 UTC Comment hidden (obsolete)
Comment 31 Blou 2014-06-02 20:13:18 UTC Comment hidden (obsolete)
Comment 32 M. Tompsett 2014-06-02 22:47:57 UTC Comment hidden (obsolete)
Comment 33 M. Tompsett 2014-06-02 23:03:09 UTC
Comment on attachment 28626 [details] [review]
Bug 10937 - Option to hide and group itemtypes from advanced search

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

::: C4/Koha.pm
@@ +278,5 @@
>  
> +=head2 GetItemTypesCategorized
> +
> +Returns item types but grouped by category if available.
> +The categories must be part of Authorized Values (DOCTYPECAT)

The perldoc on these new functions is lacking.

@@ +320,5 @@
> +=head2 GetItemTypesByCategory
> +
> +    $category = category filter
> +
> +Returns the itemtypes that are grouped into the category.

The perldoc is valid, but horrible.

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt
@@ +127,5 @@
> +                [% END %]
> +          </select>
> +          (Options are defined as the authorized values for the DOCTYPECAT category)
> +      </li>
> +      [% ELSE %]

Can you explain why there is an IF/ELSE case?

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-advsearch.tt
@@ +141,5 @@
>                                      <legend>Limit to any of the following:</legend>
>                                      <div class="row-fluid">
>                                          [% FOREACH itemtypeloo IN advsearchloo.code_loop %]
> +                                            [% IF ((!itemtypeloo.searchcategory) AND (itemtypeloo.cat == 0)) OR (itemtypeloo.cat == 1) %]
> +                                            <div class="span3"><input type="checkbox" id="[% itemtypeloo.ccl FILTER remove(',') %]-[% itemtypeloo.number %]" name="[% IF ( itemtypeloo.cat == 1 ) %]searchcat[% ELSE %]limit[% END %]" value="[% IF ( itemtypeloo.cat == 1 ) %][% itemtypeloo.code %][% ELSE %]mc-[% itemtypeloo.ccl %]:[% itemtypeloo.code %][% END %]"/><label for="[% itemtypeloo.ccl FILTER remove(',') %]-[% itemtypeloo.number %]">[% UNLESS ( noItemTypeImages ) %][% IF ( itemtypeloo.imageurl ) %]<img border="0" src="[% itemtypeloo.imageurl %]" alt="[% itemtypeloo.description %]" />[% END %]&nbsp;[% END %]

This be some ugly code. Improve readability, please.

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-advsearch.tt
@@ +187,5 @@
>      <table>
>          <tr>
>      [% FOREACH itemtypeloo IN advsearchloo.code_loop %]
> +            [% IF ((!itemtypeloo.searchcategory) AND (itemtypeloo.cat == 0)) OR (itemtypeloo.cat == 1) %]
> +                <td><input type="checkbox" id="[% itemtypeloo.ccl FILTER remove(',') %]-[% itemtypeloo.number %]" name="[% IF ( itemtypeloo.cat == 1 ) %]searchcat[% ELSE %]limit[% END %]" value="[% IF ( itemtypeloo.cat == 1 ) %][% itemtypeloo.code %][% ELSE %]mc-[% itemtypeloo.ccl %]:[% itemtypeloo.code %][% END %]"/><label for="[% itemtypeloo.ccl FILTER remove(',') %]-[% itemtypeloo.number %]">[% UNLESS ( noItemTypeImages ) %][% IF ( itemtypeloo.imageurl ) %]<img border="0" src="[% itemtypeloo.imageurl %]" alt="[% itemtypeloo.description %]" />[% END %]&nbsp;[% END %]

You should looking into Tool Template variables. This code is ugly to read.
http://www.template-toolkit.org/docs/manual/Variables.html
Comment 34 Maxime Beaulieu 2014-06-30 18:38:10 UTC Comment hidden (obsolete)
Comment 35 M. Tompsett 2014-07-29 03:18:44 UTC
Sorry, doesn't apply cleanly. Please rebase.
Comment 36 Maxime Beaulieu 2014-07-29 20:59:41 UTC Comment hidden (obsolete)
Comment 37 PTFS Europe Sandboxes 2014-09-26 13:16:31 UTC Comment hidden (obsolete)
Comment 38 Martin Renvoize 2014-09-26 13:19:36 UTC
The above rebase was I, just the db update conflict to allow the sandbox to work ;)
Comment 39 M. Tompsett 2014-09-26 14:58:02 UTC
Comment on attachment 31896 [details] [review]
Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master.

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

::: t/db_dependent/Koha.t
@@ +7,4 @@
>  use warnings;
>  use C4::Context;
>  use Koha::DateUtils qw(dt_from_string);
> +use Data::Dumper;

Oops! Forgot to remove it?
Comment 40 Koha Team Lyon 3 2014-12-09 15:28:06 UTC
There's a problem when applying this patch on a sandbox :
The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 10937:
<h1>Something went wrong !</h1>Applying: Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Koha.pm
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-advsearch.tt
CONFLICT (delete/modify): koha-tmpl/opac-tmpl/prog/en/modules/opac-advsearch.tt deleted in HEAD and modified in Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master.. Version Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master. of koha-tmpl/opac-tmpl/prog/en/modules/opac-advsearch.tt left in tree.
Auto-merging opac/opac-search.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master.
Comment 41 simith.doliveira 2014-12-15 13:26:41 UTC Comment hidden (obsolete)
Comment 42 Marc Véron 2014-12-20 18:17:44 UTC
Sorry, merge conflicts (again) with current master (3.19.00.002):

Applying: Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-search.pl
CONFLICT (content): Merge conflict in opac/opac-search.pl
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.

Oh, the patch title reads:
Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master. 

...but we are on Bug 10937?
Comment 43 simith.doliveira 2014-12-22 14:12:44 UTC Comment hidden (obsolete)
Comment 44 Marc Véron 2014-12-24 10:10:05 UTC Comment hidden (obsolete)
Comment 45 M. Tompsett 2014-12-24 17:48:27 UTC
Sorry, but modifying the table structure requires a patch to Schema files.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7567#c58
That was my first experience with them. Hope this helps.
Comment 46 Katrin Fischer 2014-12-26 09:13:57 UTC
I am not sure if the change to the schema files is mandatory yet, but if you do it, please put it in a separate patch to avoid conflicts.
http://wiki.koha-community.org/wiki/Database_updates
Comment 47 Jonathan Druart 2014-12-26 10:36:40 UTC
This patch needs a little bit more work to be readable by QAers.
There is not decent commit message, please read http://wiki.koha-community.org/wiki/Commit_messages

Marked as Failed QA.
Comment 48 Jonathan Druart 2014-12-26 10:43:18 UTC
Comment on attachment 34667 [details] [review]
[Signed-off] Bug 10947 - Grouped ItemTypes - Patch should apply properly on latest master.

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

Quick code review.

::: C4/Koha.pm
@@ +283,5 @@
> +The categories must be part of Authorized Values (DOCTYPECAT)
> +
> +=cut
> +
> +sub GetItemTypesCategorized {

Why don't you use C4::ItemType?

@@ +287,5 @@
> +sub GetItemTypesCategorized {
> +    my $dbh   = C4::Context->dbh;
> +    # Order is important, so that partially hidden (some items are not visible in OPAC) search
> +    # categories will be visible. hideinopac=0 must be last.
> +    my $query = qq|

There are no string interpolation here, limit to q||

@@ +312,5 @@
> +
> +    my %itemtypes;
> +    while ( my $IT = $sth->fetchrow_hashref ) {
> +        $itemtypes{ $IT->{'itemtype'} } = $IT;
> +    }

useless while, have a look at selectall_hashref.

@@ +324,5 @@
> +Returns the itemtype code of all itemtypes included in a searchcategory.
> +
> +=cut
> +
> +sub GetItemTypesByCategory {

Why don't you use C4::ItemType?

@@ +334,5 @@
> +    $sth->execute($category);
> +
> +    while ( my $data = $sth->fetchrow ) {
> +        push ( @results, $data );
> +    }

useless while, have a look at selectall_arrayref

::: installer/data/mysql/kohastructure.sql
@@ +1276,4 @@
>    checkinmsg VARCHAR(255), -- message that is displayed when an item with the given item type is checked in
>    checkinmsgtype CHAR(16) DEFAULT 'message' NOT NULL, -- type (CSS class) for the checkinmsg, can be "alert" or "message"
>    sip_media_type VARCHAR(3) DEFAULT NULL, -- SIP2 protocol media type for this itemtype
> +  hideinopac tinyint(1) NOT NULL DEFAULT 0, -- Hide the item type from the search options in OPAC

There are occurrences of hideatopac in the code, maybe it's better than hideinopac. Need to be confirmed by English native speaker.
Comment 49 M. Tompsett 2014-12-26 13:14:30 UTC
(In reply to Jonathan Druart from comment #48)
> There are occurrences of hideatopac in the code, maybe it's better than
> hideinopac. Need to be confirmed by English native speaker.

I can see how both might be used. The idea being that something is hidden at display time by OPAC vs. OPAC is being used an something is hidden in the output. Personally, hide in OPAC is clearer to Canadian, Anglophone me.
Comment 50 Ahmed Sfayhi 2015-01-21 20:22:28 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2015-02-26 15:20:53 UTC Comment hidden (obsolete)
Comment 52 Nick Clemens 2015-02-27 22:46:04 UTC Comment hidden (obsolete)
Comment 53 Nick Clemens 2015-02-27 22:52:01 UTC
I like this idea, but have a a small readability issue with the Hide In OPAC description on the Modify Item type page.  

It says: 
(if checked, items of this type will be hidden as filters in OPAC's advanced search)

I think more on the lines of:
If checked, the option to filter by items of this type will be hidden in OPAC advanced search (item of this type will not explicitly be hidden by this preference)
Comment 54 Kyle M Hall 2015-03-02 14:39:39 UTC
 FAIL	C4/Koha.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid


 FAIL	admin/authorised_values.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid


 FAIL	admin/itemtypes.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid


 OK	installer/data/mysql/updatedatabase.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   OK	  valid

 FAIL	opac/opac-search.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid


 FAIL	t/db_dependent/Koha.t
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   FAIL	  valid
Comment 55 simith.doliveira 2015-03-03 22:26:28 UTC
I got that when I ran QA :

 OK	C4/Koha.pm
   OK	  pod
   OK	  valid
   OK	  critic
   OK	  forbidden patterns

 OK	admin/authorised_values.pl
   OK	  pod
   OK	  valid
   OK	  critic
   OK	  forbidden patterns

 OK	admin/itemtypes.pl
   OK	  pod
   OK	  forbidden patterns
   OK	  valid
   OK	  critic

 OK	installer/data/mysql/updatedatabase.pl
   OK	  forbidden patterns
   OK	  valid
   OK	  critic
   OK	  pod

 OK	opac/opac-search.pl
   OK	  critic
   OK	  valid
   OK	  forbidden patterns
   OK	  pod

 OK	t/db_dependent/Koha.t
   OK	  valid
   OK	  critic
   OK	  forbidden patterns
   OK	  pod

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt
   OK	  valid_template
   OK	  forbidden patterns
   OK	  tt_valid

 OK	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-advsearch.tt
   OK	  tt_valid
   OK	  forbidden patterns
   OK	  valid_template


Can anyone else run QA?
Comment 56 Jonathan Druart 2015-03-04 11:12:20 UTC
Looks good to me too.
Kyle could you run perl -wc Koha.pm and tell us what is the error/warning?
It's certainly related to the perl version (5.20 here).
Comment 57 Magnus Enger 2015-03-05 11:00:14 UTC
It might be a good idea to QA bug 13787 at the same time as this bug.
Comment 58 Kyle M Hall 2015-03-13 12:58:03 UTC
(In reply to Jonathan Druart from comment #56)
> Looks good to me too.
> Kyle could you run perl -wc Koha.pm and tell us what is the error/warning?
> It's certainly related to the perl version (5.20 here).

Agreed, I'm running perl v5.10.1
Comment 59 Kyle M Hall 2015-03-13 13:01:23 UTC
Created attachment 36856 [details] [review]
[PASSED QA] Bug 10937 - Option to hide and group itemtypes from advanced search

This allows to group certain item types in a category, to be displayed (and searched) as such in OPAC's advanced search.  For example, you can group Reserve 2h and Reserve 12h into a Reserve category.  The 2 and 12h types won't appear anymore.
This also allows to simply prevent an item type from displaying as a search option.

TEST PLAN
------------------
0) Back up database, so you can reset and retest easily. ;)
1) Apply the patch
2) Run Koha QA tool.
3) prove -v t/db_dependent/Koha.t
  -- all tests should pass.
  4) run ./installer/data/mysql/updatedatabase.pl to add the
  two columns to itemtypes
  -- Does a meaningful message get printed?
  Are the columns added?
  "DESCRIBE itemtypes;" should list hideinopac and searchcategory.
  5) You need to add a category to group your item types:
  a) In Intranet/Koha Admin/Authorized values,
  select DOCTYPECAT in the 'Show category:' dropdown
  i) If you do not have a DOCTYPECAT category, create one.
  b) Click button "New authorized value for DOCTYPECAT"
  c) Enter
  Authorized value: HARDWARE
  Description : Hardware
  Description (OPAC): Hardware
  6) Group your items under that new category
  a) In Intranet/Koha Admin/Item types, choose (at least)
  two item types and for each:
  - Click action/Edit on the right column
  - Third row (below Description) is the Search category list box, select Hardware
  - click Save changes at the bottom
  7) Select at least one item to be hidden in the OPAC search
  a) In Intranet/Koha Admin/Item types (again), choose a different item type:
  -  Click action/Edit
  -  Click the checkbox "Hide in OPAC" below the list of icons.
  8) Go test your modifications
  a) Go to OPAC/Adv search.
  b) Validate that all items modified above (hidden or grouped) do not appear in Item type list
  c) Validate that new item type Hardware does appear instead.
  d) Select item Hardware, start Search.
  ) Validate returned items are the of the two types that were grouped into the Hardware category in step 4.

  Sponsored-by: Vanier college

Signed-off-by: Nick <nick@quecheelibrary.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 60 Katrin Fischer 2015-07-09 02:05:54 UTC
Comment on attachment 36856 [details] [review]
[PASSED QA] Bug 10937 - Option to hide and group itemtypes from advanced search

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

I talked to Tomas about this a while ago and finally got around to put my concerns in writing. Leaving to Tomas for final decision as this is PQA right now.

::: C4/Koha.pm
@@ +33,2 @@
>  use DBI qw(:sql_types);
> + use Data::Dumper;

This here doesn't look quite right, first you have: 
use autouse 'Data::cselectall_arrayref' => qw(Dumper);
then an additional
use Data::Dumper;
Can you explain?

@@ +279,5 @@
> +    $categories = GetItemTypesCategorized();
> +
> +Returns a hashref containing search categories.
> +A search category will be put in the hash if at least one of its itemtypes is visible in OPAC.
> +The categories must be part of Authorized Values (DOCTYPECAT)

I am not happy with the name: DOCTYPECAT. If this is to help group itemtypes, why not name it accordingly? ITYPECAT or ITEMTYPECAT? Or a bit different: ITYPEGROUP?

::: installer/data/mysql/kohastructure.sql
@@ +1276,5 @@
>    checkinmsg VARCHAR(255), -- message that is displayed when an item with the given item type is checked in
>    checkinmsgtype CHAR(16) DEFAULT 'message' NOT NULL, -- type (CSS class) for the checkinmsg, can be "alert" or "message"
>    sip_media_type VARCHAR(3) DEFAULT NULL, -- SIP2 protocol media type for this itemtype
> +  hideinopac tinyint(1) NOT NULL DEFAULT 0, -- Hide the item type from the search options in OPAC
> +  searchcategory varchar(20) default NULL, -- Group this item type with others with the same value on OPAC search options

I think this is used to store the authorised value code picked from the authorized value DOCTYPECAT category. If I am not wrong, they should match in length to avoid problems:
authorised_values.authorised_value is varchar(80)

::: installer/data/mysql/updatedatabase.pl
@@ +9838,5 @@
> +    $dbh->do(q{
> +        ALTER TABLE itemtypes
> +            ADD hideinopac TINYINT(1) NOT NULL DEFAULT 0
> +              AFTER sip_media_type,
> +            ADD searchcategory VARCHAR(20) DEFAULT NULL

See above!

::: t/db_dependent/Koha.t
@@ +7,4 @@
>  use warnings;
>  use C4::Context;
>  use Koha::DateUtils qw(dt_from_string);
> +use Data::Dumper;

I am not sure if this is needed here.
Comment 61 Blou 2015-07-09 11:56:47 UTC
All those comments are perfectly valid.  I'll correct them all (the use Dumper are most probably something forgotten by the dev, no excuse).  

Regarding the name, which one would you prefer
"ITYPECAT or ITEMTYPECAT? Or a bit different: ITYPEGROUP"

If no response, I'll go with ITYPEGROUP

(of course, I'll do a patch's patch. I'm not messing with a passed qa)
Comment 62 Blou 2015-07-09 13:26:11 UTC
Created attachment 40884 [details] [review]
Bug 10937 - cleanup and rename DOCTYPECAT to ITEMTYPECAT

- "Item" and not "Document" is the word used throughout Koha.
- Moved the updates to an atomic update
- changed searchcategory from varchar(20) to (80)
- cleanup
Comment 63 Blou 2015-07-09 13:30:01 UTC
Since the word "category" was used throughout the patch, I kept it instead of using "group".

updatedatabase.pl doesn't apply anymore with the first patch, but since it's PassedQA, I'm not touching it.  The second patch puts it all in atomicupdate, to help in case it all falls apart and someone recreate a new squashed patch from 0.
Comment 64 Genevieve Plantin 2015-09-03 13:39:44 UTC
Created attachment 42323 [details] [review]
Bug 10937 - Fix Seach Type in Advanced Search

In order to see the checkboxes of the Collection and Shelving location of the advanced search, the if changed so that only the Itemtypes will go through the previous if
Comment 65 Tomás Cohen Arazi 2015-10-22 15:01:40 UTC
Patches pushed to master.

Thanks Ahmed and Philippe!
Comment 66 Marcel de Rooy 2017-05-12 08:33:20 UTC
Adding a see also to bug 18400.
The functionality around ITEMTYPECAT looks pretty weird to me btw.