Bug 18884 - Advanced search on staff client, Availability limit not properly limiting
Summary: Advanced search on staff client, Availability limit not properly limiting
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: David Cook
QA Contact: Kyle M Hall
URL:
Whiteboard:
Keywords:
Depends on: 15758
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-30 02:54 UTC by Michael Cabus
Modified: 2017-11-20 21:38 UTC (History)
7 users (show)

See Also:
Change 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
limit here is limit=BN, not limit=holdingbranch%3ABN, which causes an item not from this library (Bison) showing in search results (1.41 MB, image/png)
2017-06-30 02:54 UTC, Michael Cabus
Details
Bug 18884 - Advanced search on staff client, Availability limit not properly limiting (3.04 KB, patch)
2017-10-10 01:09 UTC, David Cook
Details | Diff | Splinter Review
Bug 18884 - Advanced search on staff client, Availability limit not properly limiting (2.42 KB, patch)
2017-10-12 00:17 UTC, Dilan Johnpullé
Details | Diff | Splinter Review
Bug 18884 - Advanced search on staff client, Availability limit not properly limiting (2.47 KB, patch)
2017-10-27 12:22 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Cabus 2017-06-30 02:54:15 UTC
Created attachment 64741 [details]
limit here is limit=BN, not limit=holdingbranch%3ABN, which causes an item not from this library (Bison) showing in search results

On the advanced search area, when you limit to a specific library in the availability limits, the resultant search string is:

/cgi-bin/koha/catalogue/search.pl?idx=au&q=janet&op=and&idx=kw&op=and&idx=kw&limit=E&sort_by=relevance

where the limit is: limit=E for holding branch...the limit should be

/cgi-bin/koha/catalogue/search.pl?idx=au&q=evanovich+janet&op=and&idx=kw&op=and&idx=kw&limit=holdingbranch%3ABN&sort_by=relevance

limit=holdingbranch%3ABN

This means that currently a limit for a specific library will return books not in that library.
Comment 1 Mark Alexander 2017-08-18 19:10:43 UTC
I ran into this problem today on 17.05.01.

The workaround I came up with was to create a library group, and add to this group the specific library I wanted to search.  Then in the search form, I selected that library group.

I'm not familiar with the code, but I did take a quick look, and couldn't see a way to fix this just in advsearch.tt.  The language limit is handled correctly there through the use of a FOREACH loop that inserts "ln,rtrn:" in front of the language name in the option value.  But the branch limit is handled by a call to options_for_libraries.  That function, in html_helpers.inc, doesn't seem to allow a prefix to be inserted in front of the option value.

So it looks like the branch selector in advsearch.tt should have a name other than "limit", and in search.pl this param should be handled separately from the other "limit" params, by inserting the prefix "limit=holdingbranch:" in front of the branch name.

Of course, due to my ignorance, I could be entirely wrong about all this :-) .
Comment 2 David Cook 2017-10-10 00:25:42 UTC
(In reply to Mark Alexander from comment #1)
> I ran into this problem today on 17.05.01.
> 
> The workaround I came up with was to create a library group, and add to this
> group the specific library I wanted to search.  Then in the search form, I
> selected that library group.
> 
> I'm not familiar with the code, but I did take a quick look, and couldn't
> see a way to fix this just in advsearch.tt.  The language limit is handled
> correctly there through the use of a FOREACH loop that inserts "ln,rtrn:" in
> front of the language name in the option value.  But the branch limit is
> handled by a call to options_for_libraries.  That function, in
> html_helpers.inc, doesn't seem to allow a prefix to be inserted in front of
> the option value.
> 
> So it looks like the branch selector in advsearch.tt should have a name
> other than "limit", and in search.pl this param should be handled separately
> from the other "limit" params, by inserting the prefix
> "limit=holdingbranch:" in front of the branch name.
> 
> Of course, due to my ignorance, I could be entirely wrong about all this :-)
> .

It's definitely a regression introduced by Bug 15758:

-        [% FOREACH branchloo IN branchloop %]
-        [% IF ( branchloo.selected ) %]
-        <option value="branch:[% branchloo.value %]" selected="selected">[% branchloo.branchname %]</option>
-        [% ELSE %]
-        <option value="branch:[% branchloo.value %]">[% branchloo.branchname %]</option>
-        [% END %]
-        [% END %]
+        [% PROCESS options_for_libraries libraries => Branches.all() %]

I think I'll just write a patch to add an optional prefix to html_helpers.inc to restore the original syntax.
Comment 3 David Cook 2017-10-10 01:09:47 UTC
Created attachment 67858 [details] [review]
Bug 18884 - Advanced search on staff client, Availability limit not properly limiting

Bug 15758 accidentally removed the branch: prefix off the
individual library limits on the advanced search in the staff client.

This patch adds an optional prefix to the html_helpers.inc file,
so that advsearch.tt can provide the required "branch:" prefix.

_TEST PLAN_

Before applying:
0) Make sure you have branches in your Koha (e.g. select all sample data
via web installer initially)
1) View the source on the staff client advanced search, and note that
the "branch:" prefix is missing from the individual library limit:

<option value="CPL">Centerville</option>

3) Apply the patch
4) Refresh the advanced search page
5) View the source on the staff client advanced search, and note that
the "branch:" prefix now appears for the individual library limit:

<option value="branch:CPL">Centerville</option>
Comment 4 David Cook 2017-10-10 01:16:24 UTC
Note that the fix I've done isn't exactly the one that the OP asked for.

"branch" is actually mapped to "homebranch" in ccl.properties, and the "Individual libraries" limit in the staff client advanced search has always used "branch:". 

If one wanted to differentiate between holding branch and home branch, I think that would be an enhancement, whilst my patch simply fixes a regression.
Comment 5 Dilan Johnpullé 2017-10-12 00:17:32 UTC
Created attachment 67985 [details] [review]
Bug 18884 - Advanced search on staff client, Availability limit not properly limiting

Patch applies and functions as described.
Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Comment 6 Jonathan Druart 2017-10-13 15:47:47 UTC
(In reply to Dilan Johnpullé from comment #5)
> Created attachment 67985 [details] [review] [review]
> Bug 18884 - Advanced search on staff client, Availability limit not properly
> limiting
> 
> Patch applies and functions as described.
> Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>

Dilan, you are not supposed to remove the test plan from the original patch :)
Thanks for your signoff!
Comment 7 Jonathan Druart 2017-10-13 15:48:11 UTC
Upping severity a bit, it impacts stable releases.
Comment 8 Kyle M Hall 2017-10-27 12:22:38 UTC
Created attachment 68683 [details] [review]
Bug 18884 - Advanced search on staff client, Availability limit not properly limiting

Patch applies and functions as described.

Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Jonathan Druart 2017-10-27 17:14:45 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 10 Fridolin SOMERS 2017-11-20 13:25:13 UTC
Pushed to 17.05.x, will be in 17.05.06
Comment 11 Katrin Fischer 2017-11-20 21:38:05 UTC
Patch was pushed to 16.11.x, will be in 16.11.14