Bug 8600 - Remove search terms in advanced search
Summary: Remove search terms in advanced search
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 3.12
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Maxime Pelletier
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-08 17:38 UTC by Maxime Pelletier
Modified: 2014-12-07 20:03 UTC (History)
8 users (show)

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


Attachments
patch to add link to remove a search term (1.54 KB, patch)
2012-08-08 17:44 UTC, Maxime Pelletier
Details | Diff | Splinter Review
Bug 8600 - Remove search terms in advanced search (2.33 KB, patch)
2012-09-20 18:07 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8600 - Remove search terms in advanced search (2.34 KB, patch)
2012-09-20 18:35 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 8600 - Remove search terms in advanced search (2.39 KB, patch)
2012-09-21 08:35 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Rewrote part of the patch based on previous feedback. (2.29 KB, patch)
2013-03-20 21:28 UTC, Maxime Pelletier
Details | Diff | Splinter Review
Bug 8600 - Remove search terms in advanced search (2.34 KB, patch)
2013-03-21 10:10 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8600 - Remove search terms in advanced search (2.40 KB, patch)
2013-06-05 01:13 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Maxime Pelletier 2012-08-08 17:38:54 UTC
Patch attached adds in [-] in the advanced search page before the [+] to allow to remove search terms.
To test click on the More options link in the advanced search page.

Patch sponsored by the CCSR ( http://www.ccsr.qc.ca ).
Comment 1 Maxime Pelletier 2012-08-08 17:44:33 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2012-08-16 19:38:54 UTC
Don't forget to assign the bug to yourself when you submit a patch, and to mark it as "needs signoff."
Comment 3 Owen Leonard 2012-08-16 19:51:47 UTC
I'll get my petty comment out of the way first: Shouldn't it be [+][-] instead of [-][+]? I don't know why I think that.

More importantly: I think the [-] option should only be applied to fields which are dynamically added. If you remove a field which wasn't dynamically added you're using the operation as a defacto "clear" button, which we offer only in rare circumstances.

The other problem with being able to remove a "hard-coded" row with [-] is that it makes it possible to remove the last row which contains the [+]. If you remove that one you don't have the option anymore to add more rows. The final version of this enhancement should prevent/work around that problem.
Comment 4 Maxime Pelletier 2012-08-21 21:17:55 UTC
(In reply to comment #3)
> I'll get my petty comment out of the way first: Shouldn't it be [+][-]
> instead of [-][+]? I don't know why I think that.
> 
> More importantly: I think the [-] option should only be applied to fields
> which are dynamically added. If you remove a field which wasn't dynamically
> added you're using the operation as a defacto "clear" button, which we offer
> only in rare circumstances.
> 
> The other problem with being able to remove a "hard-coded" row with [-] is
> that it makes it possible to remove the last row which contains the [+]. If
> you remove that one you don't have the option anymore to add more rows. The
> final version of this enhancement should prevent/work around that problem.

I disagree with the +- order because I feel like the - should be aligned. But it doesn't matter.
I disagree about having the - only on dynamic lines. I do like it being some kind of clear. If I type words in 3 fields and change my mind on one I can remove it and add a new one.

I have a few possible solutions and ideas and I would like your opinion on the best one to take.

What I propose is to have a link "Add a new search term" at the top instead of the little [+]. That way it would always be in the same position and it would be clearer. In most app the + is to show something hidden, so a text link would help imo.
Also I will change my patch to have a small red x instead of a - for the same reasons.

Would that be alright with you ?
Comment 5 Katrin Fischer 2012-08-22 04:29:47 UTC
I think I like the +- buttons because they are very fast and flexible. I can add a search field whereever I want. I don't have to search for a button on top but can do so right at the end of the field I last typed in.
Comment 6 Marc Véron 2012-09-19 21:28:30 UTC
This is a nice enhancement I would like to see in the Opac.

I agree with Katrin, liking +- buttons as well.

Adding / removing options works well.

However I propose to change the wording of "More options"/"Fewer options"

Reason: If you remove all options with [-] and have only one left, "Fewer options" is kind of nonsense, because it means to restore the original (3) options, i.e. more, not fewer.

Better wording would be something like
"Manage options" (instead of "More optons") 
and 
"Default options" (instead of "Fewer options")

If someone removes the last line with [+] , he/she can "recover" by switching back to "Default options". ("Fewer opions" would be misleading.)

Marc
Comment 7 Katrin Fischer 2012-09-20 05:14:40 UTC
Hi Marc,

Hmm, I am not sure it works like this. You only see the +/- when you have the screen with "more" options. So if you accidently delete all your search fields and are left with only one, the fewer options will bring you back to 3 search fields, but you won't have +/- or the boolean pull downs available - so the screen will change and it will not be the same search form you have seen before. That will require switching back and forth.

I think maybe the javascript needs to be changed to not allow deleting of the lsecond line or allow cloning of the first with adding boolean search options on this step. I know it's much harder to do, but I don't feel comfortable with the feature as it is now :(
Comment 8 Marc Véron 2012-09-20 18:07:26 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2012-09-20 18:35:40 UTC Comment hidden (obsolete)
Comment 10 Mirko Tietgen 2012-09-21 08:35:46 UTC Comment hidden (obsolete)
Comment 11 Mirko Tietgen 2012-09-21 08:39:45 UTC
I like the feature! Two things:

- I would prefer the newly added fields to be empty, at the moment the contents (if any) of the field where you click [+] is duplicated. That does not make sense since you would most likely not want that in the duplicated field
- Also I would like a more fancy look of the buttons at some point :)

Anyway works for me as it is, signed off.
Comment 12 Mirko Tietgen 2012-09-21 08:57:38 UTC
More precisely,

- it would make sense to me to duplicate the state of operator and search index of the row where I hit [+], but that does not happen, it will always be "and" and "Keyword"
- it does not make much sense to me to clone the text field, which is done
Comment 13 Marc Véron 2012-09-21 11:21:50 UTC
Mirko,

The + button behaves as it did before (regarding the value and preserving the operator etc.). It simply makes a clone of the row.

My aim was to get the [-] button working for having it in the next release and defer the + to a follow up.

Marc
Comment 14 Jared Camins-Esakov 2012-10-06 04:23:52 UTC
Changing target version so this bug report will show up in the list of 3.12-targeted features and doesn't get lost in the shuffle during feature freeze.
Comment 15 Jonathan Druart 2012-11-09 14:51:47 UTC
QA Comment:

It works as expected.

But I don't know if
  <a href="javascript:;'>
is the best way to do that.
In Koha, we use
  <a href="#" onclick="function(); return false;">

But in an accessibility point of view, it is not good.

Owen, could you give your opinion on it please ?
Comment 16 Owen Leonard 2012-11-09 15:18:52 UTC
(In reply to comment #15)
> But I don't know if
>   <a href="javascript:;'>
> is the best way to do that.
> In Koha, we use
>   <a href="#" onclick="function(); return false;">

Quite right. Even better, add an id or class to the link and move the onclick handling to the $(document).ready() block. In my opinion this requires a correction.
Comment 17 Jonathan Druart 2012-11-09 15:54:34 UTC
(In reply to comment #16)
> Quite right. Even better, add an id or class to the link and move the
> onclick handling to the $(document).ready() block. In my opinion this
> requires a correction.

Thanks Owen. 
So I switch to failed qa
Comment 18 Maxime Pelletier 2013-03-20 21:28:52 UTC Comment hidden (obsolete)
Comment 19 Paul Poulain 2013-03-21 10:10:34 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 20 Paul Poulain 2013-03-21 10:10:53 UTC Comment hidden (obsolete)
Comment 21 Chris Cormack 2013-06-05 01:13:50 UTC
Created attachment 18649 [details] [review]
Bug 8600 - Remove search terms in advanced search

Rewrote parts of previous patch by Marc Veron to remove remove href and onclick of links and instead use jQuery

Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 22 Galen Charlton 2013-06-05 15:25:00 UTC
Pushed to master.  Thanks, Frédérick!

A couple notes:

- I reworded the commit description to be clearer
- I pushed a follow-up patch to remove the unneeded debug logging to the JavaScript console.
Comment 23 Jonathan Druart 2013-06-06 07:20:41 UTC
(In reply to comment #22)
> - I pushed a follow-up patch to remove the unneeded debug logging to the
> JavaScript console.

The console.log should be fail the qa tests. Chris, is your local repo up-to-date?
Comment 24 Tomás Cohen Arazi 2013-06-10 18:06:57 UTC
This patch has been pushed to 3.12.x, will be in 3.12.1.

Thanks Frédérick!