Bug 10857 - Add facets cancellation
Summary: Add facets cancellation
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: rel_3_16_candidate
: 11774 (view as bug list)
Depends on: 10952
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-10 11:53 UTC by Jonathan Druart
Modified: 2014-12-07 20:07 UTC (History)
11 users (show)

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


Attachments
Bug 10857: Add facets cancellation (8.15 KB, patch)
2013-09-10 12:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10857: Encode 'limit' parameter values (5.29 KB, patch)
2013-11-06 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10857: Add facets cancellation (8.15 KB, patch)
2014-01-09 13:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10857: Encode 'limit' parameter values (5.29 KB, patch)
2014-01-09 13:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10857: Add facets cancellation (8.20 KB, patch)
2014-01-10 13:36 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10857: Encode 'limit' parameter values (5.33 KB, patch)
2014-01-10 13:36 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10857: Add facets cancellation - bootstrap theme. (2.73 KB, patch)
2014-01-10 16:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10857: Add facets cancellation - bootstrap theme. (2.84 KB, patch)
2014-01-10 16:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 10857: Add facets cancellation (8.45 KB, patch)
2014-01-11 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10857: Encode 'limit' parameter values (5.39 KB, patch)
2014-01-11 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10857: Add facets cancellation - bootstrap theme. (2.91 KB, patch)
2014-01-11 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-09-10 11:53:29 UTC
When searching (at the OPAC or staff), facets can be enabled but never     disabled. So a user must relaunch the search.
Comment 1 mathieu saby 2013-09-10 12:11:08 UTC
Nice idea of improvement Jonathan.

Mathieu
Comment 2 Jonathan Druart 2013-09-10 12:25:55 UTC Comment hidden (obsolete)
Comment 3 mathieu saby 2013-09-13 21:24:38 UTC
I made  4 tests :
click on a Series facet, then remove it => OK
click on an Author facet, then remove it => OK
click on a Serial facet, then an Author facet, then remove the Author facet, then remove the Serial facet => OK
click on a Serial facet, then an Author facet, then remove the Serial facet, then remove the Author facet => KO

For me the last test failed : if the facets are removed in a different order than the order used to enable them, the [x] disappears, but Koha does not relaunch the search.

Do you think you can fix that? I hope this is not too complex.

Mathieu
Comment 4 Jonathan Druart 2013-09-16 08:10:28 UTC
(In reply to mathieu saby from comment #3)
> then remove the Author facet => KO
> 
> For me the last test failed : if the facets are removed in a different order
> than the order used to enable them, the [x] disappears, but Koha does not
> relaunch the search.

I cannot reproduce the issue. There is no importance on the order.
If a problem exists, it is not this one :)
Could you retest with others facets please?
Comment 5 mathieu saby 2013-09-16 08:12:01 UTC
Maybe a js problem in my browser! I will take an other look.

Mathieu
Comment 6 mathieu saby 2013-09-16 11:45:52 UTC
Sorry, still not working.
I made a little screencast. Better than long speeches...
https://www.youtube.com/watch?v=5ANbVtjpnNA

Mathieu
Comment 7 Jonathan Druart 2013-09-16 12:06:52 UTC
(In reply to mathieu saby from comment #6)
> Sorry, still not working.
> I made a little screencast. Better than long speeches...
> https://www.youtube.com/watch?v=5ANbVtjpnNA
> 
> Mathieu

How many results did you get with only the author facet selected?
Comment 8 mathieu saby 2013-09-16 12:33:14 UTC
5 results with one facet, 2 results with 2 facets.

Maybe my records are not very "clean" and that could explain the issue. Should I try on a sandbox?

Mathieu
Comment 9 Jonathan Druart 2013-09-16 12:39:28 UTC
(In reply to mathieu saby from comment #8)
> 5 results with one facet, 2 results with 2 facets.

On the screencast I see 5 results for the "Series" facets. So you got 5 results for the author facet too? If you got only 2, the behavior is normal. If you got 5, it is not.

> Maybe my records are not very "clean" and that could explain the issue.
> Should I try on a sandbox?

Yes, it will be easiest to debug an issue on a sandbox.
Comment 10 mathieu saby 2013-09-16 12:41:58 UTC
Request sent on sandbox 3 ;-)
Test results this afternoon.

Mathieu
Comment 11 Katrin Fischer 2013-09-22 17:30:09 UTC
Hi Mathieu, did you have a chance to test? I would really like to see this happen :)
Comment 12 mathieu saby 2013-09-22 17:47:09 UTC
I made some tests : I was not satisfied, and had probably not related problems (I crashed my browers 3 times because of too big http header or something like that). I doubt it is related to this bug.
So, I prefer someone else to test it.

Mathieu
Comment 13 mathieu saby 2013-09-22 17:48:09 UTC
And sandbox 3 crashed to, but probably for another reason...

Mathieu
Comment 14 I'm just a bot 2013-09-24 05:36:14 UTC
Patch applied cleanly, go forth and signoff
Comment 15 Owen Leonard 2013-09-25 18:49:24 UTC
(In reply to mathieu saby from comment #12)
> I made some tests : I was not satisfied, and had probably not related
> problems (I crashed my browers 3 times because of too big http header or
> something like that).

Here's what I got after testing a few times:

Bad Request

Your browser sent a request that this server could not understand.
Size of a request header field exceeds server limit.

Cookie

/n
Comment 16 mathieu saby 2013-09-25 19:23:45 UTC
OH, exactly the same bug here!
But I thought it was not linked.

M. Saby
Comment 17 Julian Maurice 2013-09-26 10:56:38 UTC
Hi all,

I think this bug is only due to the cookies size limitation and the way Koha handle them (or maybe the problem comes from Apache or the web browser I don't know), but it is not specific to this patch.

So I wrote a patch to store anonymous search history into CGI::Session instead of cookies. Please see Bug 10952
Comment 18 Jonathan Druart 2013-09-26 14:20:19 UTC
As Julian said, I am not sure the problem is from this patch.
I think the issue already exists in master.
Switch back to needs signoff.
Comment 19 Owen Leonard 2013-09-30 19:03:59 UTC
I haven't run across the "Bad request" error again, but I did find this:

1. Perform a search
2. Click a library facet
3. Cancel the library facet
4. Click another library facet

At this point there is no "[x]" by the facet I clicked in step 4. Same results in the OPAC and staff client.
Comment 20 mathieu saby 2013-09-30 22:24:14 UTC
Hi Owen & Jonathan

I experienced the same bug during my tests.

Mathieu
Comment 21 Jonathan Druart 2013-10-01 12:54:54 UTC
(In reply to Owen Leonard from comment #19)
> I haven't run across the "Bad request" error again, but I did find this:
> 
> 1. Perform a search
> 2. Click a library facet
> 3. Cancel the library facet
> 4. Click another library facet
> 
> At this point there is no "[x]" by the facet I clicked in step 4. Same
> results in the OPAC and staff client.

I cannot reproduced, tested in the OPAC and staff.
The link appears and works.
What is the url generated in step 4?
Comment 22 Owen Leonard 2013-10-01 13:17:22 UTC
1. Keyword search from the main search bar: 
   /cgi-bin/koha/opac-search.pl?idx=&q=blue

2. Click library facet:
   /cgi-bin/koha/opac-search.pl?q=blue%2520%2523relevance_dsc&sort_by=relevance_dsc&limit=branch:APL

3. Cancel library facet:
   /cgi-bin/koha/opac-search.pl?idx=kw&q=blue%2520&limit=branch:APL&sort_by=relevance_dsc&nolimit=branch:APL

4. Click another library facet: 
   /cgi-bin/koha/opac-search.pl?q=kw%253Ablue%2520%2523relevance_dsc&sort_by=relevance_dsc&limit=branch:NPL
Comment 23 Jonathan Druart 2013-10-01 16:04:56 UTC
(In reply to Owen Leonard from comment #22)
> 1. Keyword search from the main search bar: 
>    /cgi-bin/koha/opac-search.pl?idx=&q=blue
> 
> 2. Click library facet:
>   
> /cgi-bin/koha/opac-search.
> pl?q=blue%2520%2523relevance_dsc&sort_by=relevance_dsc&limit=branch:APL
> 
> 3. Cancel library facet:
>   
> /cgi-bin/koha/opac-search.pl?idx=kw&q=blue%2520&limit=branch:
> APL&sort_by=relevance_dsc&nolimit=branch:APL
> 
> 4. Click another library facet: 
>   
> /cgi-bin/koha/opac-search.
> pl?q=kw%253Ablue%2520%2523relevance_dsc&sort_by=relevance_dsc&limit=branch:
> NPL

Here I get:
1/ opac-search.pl?q=d
2/ opac-search.pl?idx=kw&q=d&sort_by=author_asc&limit=branch:CPL
3/ opac-search.pl?idx=kw&q=d&limit=branch:CPL&sort_by=author_asc&nolimit=branch:CPL
4/ opac-search.pl?idx=kw&q=d&sort_by=author_asc&limit=branch:MPL

I don't understand why between your 3 and 4 the idx=kw&q=blue become kw:blue.
But in all cases, a facet is considered as active if a limit param is index:value.
Since it works with the first query, it is really odd that it does not work with the second one.

Mathieu, did you reproduce on a sandbox?
Comment 24 mathieu saby 2013-10-02 07:21:58 UTC
> 
> Mathieu, did you reproduce on a sandbox?

Hi
I tried this morning, but 
- Sandbox 1 did not set up
- Sandbox 8 (Marc21) did set up, but 0 results to any search on staff interface and opac (maybe zebra not running?)

Mathieu
Comment 25 Jonathan Druart 2013-10-02 10:25:26 UTC
(In reply to mathieu saby from comment #24)

> - Sandbox 1 did not set up

The sandbox is now available. I blocked it for tests.
Comment 26 mathieu saby 2013-10-02 10:44:37 UTC
Great. I spent a lot of time on your patch for invoice prices (prices give me headache...) but I will try this one on sandbox 1 this afternoon.

Mathieu
Comment 27 mathieu saby 2013-10-02 12:21:53 UTC
Note : it is easier to test by setting  maxRecordsForFacets to a big number (say 500 or 1000) and activate  displayFacetCount.

Mathieu
Comment 28 mathieu saby 2013-10-02 12:46:17 UTC
Test made on sandbox 1

Not easy at all to test, because (in Unimarc at least)
- Author facets with "," are broken
Ex :
http://catalogue.test1.biblibre.com/cgi-bin/koha/opac-search.pl?idx=kw&q=fr%252A&limit=au:Balzac,%20Honor%C3%A9%20de
=> 0 results

- Topic facets with "-" are broken
Ex : 
http://catalogue.test1.biblibre.com/cgi-bin/koha/opac-search.pl?idx=kw&q=fr%252A&limit=su-to:Nature%20-%20revue
=> 0 results
I did not check, but I think they are broken in master too as your patch is not supposed to change their behavior


Test 1 on OPAC:
maxRecordsForFacets = 1000
initial search ="a*"
1st click on series facet "Folio(39)" => [x] before facet Folio; 147 results
http://catalogue.test1.biblibre.com/cgi-bin/koha/opac-search.pl?idx=kw&q=a%252A&limit=se:Folio

2d click on topic facet "823 (9)" => [x] before facet "823" ; facet "Folio" still activated ; 2 results
http://catalogue.test1.biblibre.com/cgi-bin/koha/opac-search.pl?idx=kw&q=a%252A&limit=se:Folio&limit=su-to:823

3rd click on series facet "Folio" => [x] disapear before "Folio" ; 52 results
http://catalogue.test1.biblibre.com/cgi-bin/koha/opac-search.pl?idx=kw&q=a%252A&limit=se:Folio&limit=su-to:823&nolimit=se:Folio

Just to check : manually editing the url to limit only to "823" topic : 52 results
catalogue.test1.biblibre.com/cgi-bin/koha/opac-search.pl?idx=kw&q=a%252A&limit=su-to:823
Click on "Folio (2)" series facet => the 2 facets now have [x] ; 9 results are showing (why not 2?)
Click on "823" facet topic => the [x] disapear before "823" ; 147 results

=> it seems OK

Test 2 on staff interface : 
paying with 3 different facets, checking, unchecking in a different order 
=> it seems OK

I will try to reproduce Owen's issue
Mathieu
Comment 29 mathieu saby 2013-10-02 13:01:16 UTC
Test 3 : I confirm the problem if the 2 facets are in the same category. And I experienced again the crash server again, but I have no idea whether it is related to your patch.

- search "e*"
- click on library "Le Canet (1000)" => 
[x] before
 3950 result(s) found for 'kw,wrdl: e*' with limit(s): 'branch:Le Cannet' in Médiathèque municipale Le Cannet des Maures Catalog.
http://pro.test1.biblibre.com/cgi-bin/koha/catalogue/search.pl?idx=kw&q=e%252A&limit=branch:MAURES

- click on library "Médiathèque départementale de prêt (4)"
http://pro.test1.biblibre.com/cgi-bin/koha/catalogue/search.pl?idx=kw&q=e%252A&limit=branch:MAURES&limit=branch:BDP

BOUM!
FF Error "La connexion avec le serveur a été réinitialisée pendant le chargement de la page."
I broke sandbox staff interface!

I had made the same test 5 before, but I did not note the results at that time. And I did not experience the server error. Strange.

I try to test the same search on OPAC

- search "e*"
- click on library "Le Canet (1000)" => 
[x] before
http://catalogue.test1.biblibre.com/cgi-bin/koha/opac-search.pl?idx=kw&q=e%252A&limit=branch:MAURES
Your search returned 3950 results

- click on library "Médiathèque départementale de prêt"
[x] before the 2 facets
http://catalogue.test1.biblibre.com/cgi-bin/koha/opac-search.pl?idx=kw&q=e%252A&limit=branch:MAURES&limit=branch:BDP
Your search returned 21 results. Subscribe to this search 

- uncheck "Le Canet" => still 21 results!
http://catalogue.test1.biblibre.com/cgi-bin/koha/opac-search.pl?idx=kw&q=e%252A&limit=branch:MAURES&limit=branch:BDP&nolimit=branch:MAURES
Your search returned 21 results. 

Médiathèque Départementale de Prêt [x]
Le Cannet (21)



Mathieu
Comment 30 mathieu saby 2013-10-02 13:02:56 UTC
Hem, wait a second, in fact I think it working as expecteded...
Comment 31 mathieu saby 2013-10-02 13:07:14 UTC
In fact, there is only 21 documents in BDP
http://catalogue.test1.biblibre.com/cgi-bin/koha/opac-search.pl?idx=kw&q=e%252A&limit=branch:BDP
and all of these seems to be also in "Le Canet" (I cannot see "Le Canet" items on records, maybe they are hidden).

So my 3d test does not prove anything :(

And I cannot go back to staff interface so suppress some items, as I broke it...

Mathieu
Comment 32 mathieu saby 2013-10-02 15:42:23 UTC
So, still "need signoff"
Comment 33 I'm just a bot 2013-10-28 06:16:33 UTC
Patch applied cleanly, go forth and signoff
Comment 34 Jonathan Druart 2013-11-06 08:33:28 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2014-01-09 13:54:49 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2014-01-09 13:54:57 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2014-01-09 13:55:34 UTC
rebased patch.
Only available for the prog theme (for now).
Comment 38 Koha Team Lyon 3 2014-01-10 13:34:39 UTC
I made several searchs and tested several facets : authors - series - topics
and I have used several facets in the same time (in the same categries or not). When I remove a facet with [x], I have the logical number of results. 
I have the same results in OPAC or in pro interface.
It's ok for me.
Sonia
Comment 39 sandboxes@biblibre.com 2014-01-10 13:36:11 UTC
Patch tested with a sandbox, by sonia bouis <sonia.bouis@univ-lyon3.fr>
Comment 40 sandboxes@biblibre.com 2014-01-10 13:36:40 UTC Comment hidden (obsolete)
Comment 41 sandboxes@biblibre.com 2014-01-10 13:36:44 UTC Comment hidden (obsolete)
Comment 42 Kyle M Hall 2014-01-10 16:02:45 UTC
Needs updated to include bootstrap theme. Other than that, it looks great!
Comment 43 Jonathan Druart 2014-01-10 16:23:35 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2014-01-10 16:25:19 UTC Comment hidden (obsolete)
Comment 45 Katrin Fischer 2014-01-11 21:38:31 UTC
Created attachment 24201 [details] [review]
[PASSED QA] Bug 10857: Add facets cancellation

When searching (at the OPAC or pro), facets can be enabled but never
disabled. So a user is obliged to relaunch the search.

This patch adds a new link "[x]" at the right of each selected facet.
This link relaunch the search without this facet.

Test plan:
- Launch a search (OPAC and pro)
- Enable some facets
- Disable some facets

Signed-off-by: sonia bouis <sonia.bouis@univ-lyon3.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Patches pass all tests and QA script. Nice feature!

Tested in Boostrap and Prog, adding end removing multiple
facets in different sequences, adding and removing the
availability limit.
Comment 46 Katrin Fischer 2014-01-11 21:38:39 UTC
Created attachment 24202 [details] [review]
[PASSED QA] Bug 10857: Encode 'limit' parameter values

to allow facets with '&' character.

Signed-off-by: sonia bouis <sonia.bouis@univ-lyon3.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 47 Katrin Fischer 2014-01-11 21:38:44 UTC
Created attachment 24203 [details] [review]
[PASSED QA] Bug 10857: Add facets cancellation - bootstrap theme.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 48 Katrin Fischer 2014-01-11 21:41:06 UTC
I think maybe the little x could be formatted a bit differently to be more obvious, but that is not enough for failing QA on this :)

I discovered bug 11524 while testing this, which could be related to bug 10952 this depends on.
Comment 49 Katrin Fischer 2014-02-17 08:07:11 UTC
*** Bug 11774 has been marked as a duplicate of this bug. ***
Comment 50 Galen Charlton 2014-05-05 21:44:36 UTC
Pushed to master.  Thanks, Jonathan!