Bug 7401 - Show shelving location facet instead of branch facet when only 1 branch configured
Summary: Show shelving location facet instead of branch facet when only 1 branch confi...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Mason James
URL:
Keywords:
Depends on:
Blocks: 10078
  Show dependency treegraph
 
Reported: 2012-01-04 15:58 UTC by Ian Walls
Modified: 2014-05-26 21:04 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (9.01 KB, patch)
2012-01-04 17:23 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7401 - Shelving Location facet instead of Branch facet when only 1 branch configured (7.49 KB, patch)
2012-02-16 17:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7401 - Shelving Location facet - Part II - Deprecate singelBranchMode (5.45 KB, patch)
2012-02-17 17:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7401 - Shelving Location facet instead of Branch facet when only 1 branch configured (8.45 KB, patch)
2012-03-12 18:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7401 - Shelving Location facet instead of Branch facet when only 1 branch configured (8.63 KB, patch)
2012-03-12 19:01 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7401 - Shelving Location facet instead of Branch facet when only 1 branch configured (8.60 KB, patch)
2012-06-11 08:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7401 - Shelving Location facet instead of Branch facet when only 1 branch configured (8.67 KB, patch)
2012-06-11 14:01 UTC, Michael Davis
Details | Diff | Splinter Review
Bug 7401 - Shelving Location facet instead of Branch facet when only 1 branch configured (8.55 KB, patch)
2012-07-10 18:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7401 - Shelving Location facet instead of Branch facet when only 1 branch configured (8.83 KB, patch)
2012-07-13 12:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7401 - Shelving Location facet (8.43 KB, patch)
2012-07-16 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Shelving location facet showing "*" link (3.06 KB, image/png)
2012-09-14 15:05 UTC, Owen Leonard
Details
Bug 7401 - Shelving Location facet (8.84 KB, patch)
2012-09-14 19:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7401 - Shelving Location facet (8.96 KB, patch)
2012-09-14 20:04 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Walls 2012-01-04 15:58:14 UTC
If there is only one branch configured in Koha, it doesn't make any sense to put a limiter on branchcode in search results.  Having a limiter on shelving locations within that branch, though, would be very handy.
Comment 1 Ian Walls 2012-01-04 17:23:25 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-02-16 17:16:12 UTC
It looks like this is already in Koha, but it checks the singleBranchMode, rather than counting the number of branches. So, if singleBranchMode is enabled, the branchcode limiter disappears, but a location limiter appears instead.
Comment 3 Kyle M Hall 2012-02-16 17:18:31 UTC
(In reply to comment #2)
It looks like I may be wrong. I'll do some more testing.

> It looks like this is already in Koha, but it checks the singleBranchMode,
> rather than counting the number of branches. So, if singleBranchMode is
> enabled, the branchcode limiter disappears, but a location limiter appears
> instead.
Comment 4 Kyle M Hall 2012-02-16 17:33:23 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-02-16 17:34:59 UTC
(In reply to comment #4)

I should also note that it didn't apply in its original state, but minor adjustments were all that were required to make it work.


> Created attachment 7700 [details] [review]
> Bug 7401 - Shelving Location facet instead of Branch facet when only 1 branch
> configured
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
> 
> Did some minor re-working so it uses the singleBranchMode
> system pref, instead of counting the number or branches.
Comment 6 Ian Walls 2012-02-17 14:48:35 UTC
Kyle,


SingleBranchMode is buggy, and should be phased out in favour of branch counting (in my opinion).  It seems unreasonable to me to have a system preference toggle when the number of configured branches would give you that information.  Turning on SingleBranchMode in a multi-branch context is just begging for problems.  Hence my usage of branch count instead of the system preference.

To the greater community:  opinions or thoughts on this?  I think we'd eliminate a fair number of problems by phasing SingleBranchMode out, but perhaps some of you know of a worthwhile use-case for keeping it (and keeping it in all it's currently used contexts).
Comment 7 Ian Walls 2012-02-17 14:49:51 UTC
Comment on attachment 7040 [details] [review]
Proposed Patch

Un-obsoleting patch due to debate between SingleBranchMode and counting @branchloop.
Comment 8 Kyle M Hall 2012-02-17 16:24:40 UTC
(In reply to comment #6)
Here are my thoughts: If you use branch counting instead of SingleBranchMode, then it limits the ability to use this. If a group if libraries use SingleBranchMode, why should they not be able to see the shelving facet? 

I understand the reasoning behind only using it for single libraries ( e.g. We don't want to show locations that don't exist in this library ). So, if SingleBranchMode is indeed to be deprecated, then you are right, and this should go back to branch counting.

As far as I can tell, SingleBranchMode is only used in two places, once in Search.pm and twice in Koha.pm, all instances involve facets.

> Kyle,
> 
> 
> SingleBranchMode is buggy, and should be phased out in favour of branch
> counting (in my opinion).  It seems unreasonable to me to have a system
> preference toggle when the number of configured branches would give you that
> information.  Turning on SingleBranchMode in a multi-branch context is just
> begging for problems.  Hence my usage of branch count instead of the system
> preference.
> 
> To the greater community:  opinions or thoughts on this?  I think we'd
> eliminate a fair number of problems by phasing SingleBranchMode out, but
> perhaps some of you know of a worthwhile use-case for keeping it (and keeping
> it in all it's currently used contexts).
Comment 9 Kyle M Hall 2012-02-17 16:29:05 UTC
Perhaps we should delete the SingleBranchMode system pref, and have C4::Context set it to enabled if the branch count is 1? That way we don't have to go counting the branches each time with GetBranchesLoop().
Comment 10 Kyle M Hall 2012-02-17 16:32:42 UTC
(In reply to comment #9)

I meant C4::Auth, not C4::Context.

> Perhaps we should delete the SingleBranchMode system pref, and have C4::Context
> set it to enabled if the branch count is 1? That way we don't have to go
> counting the branches each time with GetBranchesLoop().
Comment 11 Kyle M Hall 2012-02-17 17:38:11 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2012-03-12 18:55:45 UTC Comment hidden (obsolete)
Comment 13 Nicole C. Engard 2012-03-12 19:01:15 UTC Comment hidden (obsolete)
Comment 14 Paul Poulain 2012-03-21 16:10:49 UTC
QA comment:
* clean code
* I agree with the idea of removing SingleBranch mode, and the GetBranchCount is a 1st step in this direction
* a question though = why not having location facet even with multiple branches ?

marking passed QA, will push later, once i've the answer to question 3
Comment 15 Kyle M Hall 2012-03-21 16:30:40 UTC
(In reply to comment #14)

> * a question though = why not having location facet even with multiple
> branches ?

Reasoning is that a given location may not be used by all the libraries.
Comment 16 Paul Poulain 2012-03-22 09:03:47 UTC
sorry, but this patch does not apply correctly anymore, because of bug 3216, applied 2 weeks ago.
The bug 3216 changes the content of the array, from:
my $library_facet = {
   idx   => 'branch',
   label => 'Libraries',
   tags  => [ qw/ 995b / ],
   expanded => '1',
};

to
$library_facet = {
 link_value  => 'branch',
 label_value => 'Libraries',
 tags        => [ '995', ],
 subfield    => 'b',
 expanded    => '1',
};


solving this conflict is not trivial, I won't do it myself, sorry for the annoyance to do it again
Comment 17 Kyle M Hall 2012-06-11 08:55:23 UTC Comment hidden (obsolete)
Comment 18 Michael Davis 2012-06-11 14:01:05 UTC Comment hidden (obsolete)
Comment 19 Paul Poulain 2012-07-03 16:54:51 UTC
Comment on attachment 10154 [details] [review]
Bug 7401 - Shelving Location facet instead of Branch facet when only 1 branch configured

patch obsoleted, keeping the other signed-off one
Comment 20 Paul Poulain 2012-07-10 15:01:29 UTC
QA comment: some concern already expressed and solved, passed QA
Comment 21 Paul Poulain 2012-07-10 15:03:21 UTC
sorry, but does not apply once again. Please rebase and switch to passed QA again, and I'll take care of it promptly !
Comment 22 Kyle M Hall 2012-07-10 18:00:12 UTC Comment hidden (obsolete)
Comment 23 Paul Poulain 2012-07-13 08:22:30 UTC
Epic fail :\
Global symbol "$itemtypes" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Search.pm line 561.
Global symbol "$itemtypes" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Search.pm line 561.
Global symbol "$itemtypes" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Search.pm line 562.
Global symbol "$itemtypes" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Search.pm line 563.
Global symbol "$itemtypes" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Search.pm line 566.
BEGIN not safe after errors--compilation aborted at /home/paul/koha.dev/koha-community/C4/Search.pm line 1798.
Compilation failed in require at /home/paul/koha.dev/koha-community/opac/opac-search.pl line 47.
BEGIN failed--compilation aborted at /home/paul/koha.dev/koha-community/opac/opac-search.pl line 47.
Comment 24 Kyle M Hall 2012-07-13 12:20:12 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2012-07-13 12:23:50 UTC
My bad. I took a simple rebase for granted, and missed the fact that an additional variable and been added to GetRecords!
Comment 26 Paul Poulain 2012-07-16 13:50:17 UTC
Testing this before pushing, I can't have it working:
 * setup on UNIMARC
 * I realized that there is a mistake for location. in Unimarc settings, the proper field is 995$e, not 995$c. I've changed Koha.pm, but it does not change anything
 * I've set SingleBranch=ON (I've 10+ branches on my setup)
 * when I do a search, I see no "Location" facet on OPAC or on Staff interface
 * in my database, there are locations :
portable lecannetdesmaures>SELECT location,count(*) FROM items GROUP BY location;
+--------------------------+----------+
| location                 | count(*) |
+--------------------------+----------+
| NULL                     |      289 |
|                          |       18 |
| Ados                     |     1063 |
| Album                    |      933 |
| BD adulte                |     1610 |
| BD enfant                |      265 |
| Bureau                   |       15 |
| Cédéroms adulte          |       95 |
| Cédéroms enfant          |        2 |
| Contes                   |      348 |
| Doc adultes              |        3 |
| Doc jeunesse             |        5 |
...

Should I do something else to see the facets ?
Comment 27 Kyle M Hall 2012-07-16 16:31:03 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2012-07-16 16:32:28 UTC
Here is an updated patch. This version is modified enough from the original that I think it should be signed-off on again.
Comment 29 Owen Leonard 2012-09-14 15:05:18 UTC
Created attachment 12241 [details]
Shelving location facet showing "*" link

Is it to be expected that one of the facets is "*"--representing items with no shelving location I assume? Or is there a problem with my data? I don't think narrowing a search to items without shelving location is useful to patrons.
Comment 30 Kyle M Hall 2012-09-14 17:09:58 UTC
(In reply to comment #29)
> Created attachment 12241 [details]
> Shelving location facet showing "*" link
> 
> Is it to be expected that one of the facets is "*"--representing items with
> no shelving location I assume? Or is there a problem with my data? I don't
> think narrowing a search to items without shelving location is useful to
> patrons.

Your first assumption is correct. I don't know if we should alter it however, as that is how all the different facets work. If there are items with no value for the given facet, a "*" shows up to represent the 'no value' option.
Comment 31 Owen Leonard 2012-09-14 17:48:33 UTC
> If there are items
> with no value for the given facet, a "*" shows up to represent the 'no
> value' option.

In what situation is this the case?
Comment 32 Kyle M Hall 2012-09-14 17:58:50 UTC
(In reply to comment #31)
> > If there are items
> > with no value for the given facet, a "*" shows up to represent the 'no
> > value' option.
> 
> In what situation is this the case?

It happens for branches, as well. But those are the only two though. If a branch has no branchname, it get's a "*" instead.
Comment 33 Owen Leonard 2012-09-14 18:34:38 UTC
(In reply to comment #32)
> It happens for branches, as well. But those are the only two though. If a
> branch has no branchname, it get's a "*" instead.

By no branchname, do you mean the item has not homebranch or holdingbranch? I can't reproduce that output.

Even so, it seems like an awfully slim edge case. When would an item not have a branch name? Even in such a case, is it useful to the OPAC user to be able to refine their results to items which *don't* have a branch?
Comment 34 Kyle M Hall 2012-09-14 19:40:18 UTC
(In reply to comment #33)
> (In reply to comment #32)
> > It happens for branches, as well. But those are the only two though. If a
> > branch has no branchname, it get's a "*" instead.
> 
> By no branchname, do you mean the item has not homebranch or holdingbranch?
> I can't reproduce that output.
> 
> Even so, it seems like an awfully slim edge case. When would an item not
> have a branch name? Even in such a case, is it useful to the OPAC user to be
> able to refine their results to items which *don't* have a branch?

Actually, it appears to mean the branch exists, but doesn't have a name. I agree, it's not very useful. It's code Ian wrote, so I'm not sure if he had a particular reason for doing it that way. I'll post a new patch that should fix this issue.
Comment 35 Kyle M Hall 2012-09-14 19:40:36 UTC Comment hidden (obsolete)
Comment 36 Owen Leonard 2012-09-14 20:04:04 UTC
Created attachment 12254 [details] [review]
[SIGNED-OFF] Bug 7401 - Shelving Location facet

This patch enables the shelving location facet as an
alternative to the branches fact in two situations:
A) SingleBranchMode is enabled
B) There is only one branch in the branches table

Test Plan:
1) Catalog multiple items with different shelving locations.
2) Test enable by enabling SingleBranchMode
3) Test enable by deleting all but one branch

Based on initial patch by Ian Walls.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Tested cases 2) and 3) successfully in OPAC and staff client
Comment 37 Mason James 2012-09-16 14:20:42 UTC
(In reply to comment #36)
> Created attachment 12254 [details] [review]
> [SIGNED-OFF] Bug 7401 - Shelving Location facet
> Signed-off-by: Owen Leonard <oleonard@myacpl.org>

patch looks good, passing QA…

$ koha-qa.pl  

testing 1 commit(s) (applied to commit 3f31617)
 * 663f5e5 Bug 7401 - Shelving Location facet
      C4/Branch.pm
      C4/Koha.pm
      C4/Search.pm
      koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc
      koha-tmpl/opac-tmpl/prog/en/includes/opac-facets.inc
      opac/opac-search.pl

* C4/Branch.pm                                                             OK
* C4/Koha.pm                                                               OK
* C4/Search.pm                                                             OK
* opac/opac-search.pl                                                      OK
* koha-tmpl/intranet-tmpl/prog/en/includes/facets.inc                      OK
* koha-tmpl/opac-tmpl/prog/en/includes/opac-facets.inc                     OK
Comment 38 Paul Poulain 2012-09-19 12:28:34 UTC
Patch pushed to master