Bug 11430

Summary: Search history: Delete selected lines
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: SearchingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, koha, kyle, luce.barbey, matthias.meusburger, patrick.robitaille, sandboxes, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 10807, 10862    
Bug Blocks: 12460, 16593    
Attachments: Bug 11430: DB changes: Add the primary key for search_history
Bug 11430: delete search history by id - API changes
Bug 11430: UT: add unit tests to delete only selected lines
Bug 11430: OPAC changes
Bug 11430: OPAC changes -- prog theme
Bug 11430: Intranet changes
Bug 11430: DB changes: Add the primary key for search_history
Bug 11430: delete search history by id - API changes
Bug 11430: UT: add unit tests to delete only selected lines
Bug 11430: OPAC changes
Bug 11430: OPAC changes -- prog theme
Bug 11430: Intranet changes
Bug 11430: Fix paths to the checkboxes jquery plugin
Bug 11430: DB changes: Add the primary key for search_history
Bug 11430: delete search history by id - API changes
Bug 11430: UT: add unit tests to delete only selected lines
Bug 11430: OPAC changes
Bug 11430: OPAC changes -- prog theme
Bug 11430: Intranet changes
Bug 11430: Fix paths to the checkboxes jquery plugin
Bug 11430: DB changes: Add the primary key for search_history
Bug 11430: delete search history by id - API changes
Bug 11430: UT: add unit tests to delete only selected lines
Bug 11430: OPAC changes
Bug 11430: OPAC changes -- prog theme
Bug 11430: Intranet changes
Bug 11430: Fix paths to the checkboxes jquery plugin
Bug 11430: DB changes: Add the primary key for search_history
Bug 11430: delete search history by id - API changes
Bug 11430: UT: add unit tests to delete only selected lines
Bug 11430: OPAC changes
Bug 11430: Intranet changes
Bug 11430: DB changes: Add the primary key for search_history
[PASSED QA] Bug 11430: delete search history by id - API changes
[PASSED QA] Bug 11430: UT: add unit tests to delete only selected lines
[PASSED QA] Bug 11430: OPAC changes
[PASSED QA] Bug 11430: Intranet changes
[PASSED QA] Bug 11430: DB changes: Add the primary key for search_history
Bug 11430 [Follow-up] Search history: Delete selected lines
Bug 11430: delete search history by id - API changes
Bug 11430: UT: add unit tests to delete only selected lines
Bug 11430: OPAC changes
Bug 11430: Intranet changes
Bug 11430: DB changes: Add the primary key for search_history
Bug 11430 [Follow-up] Search history: Delete selected lines
Bug 11430: delete search history by id - API changes
Bug 11430: UT: add unit tests to delete only selected lines
Bug 11430: OPAC changes
Bug 11430: Intranet changes
Bug 11430: DB changes: Add the primary key for search_history
Bug 11430 [Follow-up] Search history: Delete selected lines
Bug 11430: DB changes: Add the primary key for search_history
Bug 11430: (QA followup) small typos in kohastructure.sql
Bug 11430: (RM followup) DBIx schema update
Bug 11430: (QA followup) we test for warnings, always

Description Jonathan Druart 2013-12-20 14:02:40 UTC
Bug 10807 adds a search history for authorities and bug 10862 adds the search history on the staff interface.

This one allows the user to select the search history lines he wants to delete.
Comment 1 Jonathan Druart 2013-12-20 15:35:38 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-12-20 15:35:45 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-12-20 15:35:53 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-12-20 15:36:00 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2013-12-20 15:36:07 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2013-12-20 15:52:56 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-02-20 14:30:29 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-02-20 14:30:34 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-02-20 14:30:39 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-02-20 14:30:45 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-02-20 14:30:50 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-02-20 14:30:55 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-02-20 14:31:14 UTC
Rebased patches
Comment 14 Jonathan Druart 2014-02-25 14:11:03 UTC Comment hidden (obsolete)
Comment 15 Marc Véron 2014-03-06 16:00:22 UTC
Patch "delete search history by id - API changes" does not apply
Comment 16 Jonathan Druart 2014-03-06 16:32:47 UTC
Did you apply dependencies before applying these ones?
Comment 17 Marc Véron 2014-03-06 19:22:39 UTC
(In reply to Jonathan Druart from comment #16)
> Did you apply dependencies before applying these ones?

Ah, no, sorry!
Marc
Comment 18 LuceBarbey 2014-06-03 07:55:51 UTC
The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 11430:
<h1>Something went wrong !</h1>Applying: Bug 11430: DB changes: Add the primary key for search_history
Applying: Bug 11430: delete search history by id - API changes
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Search/History.pm
CONFLICT (content): Merge conflict in C4/Search/History.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 11430: delete search history by id - API changes
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 11430 - Search history: Delete selected lines

25499 - Bug 11430: DB changes: Add the primary key for search_history
25500 - Bug 11430: delete search history by id - API changes
25501 - Bug 11430: UT: add unit tests to delete only selected lines
25502 - Bug 11430: OPAC changes
25503 - Bug 11430: OPAC changes -- prog theme
25504 - Bug 11430: Intranet changes
25618 - Bug 11430: Fix paths to the checkboxes jquery plugin
Comment 19 LuceBarbey 2014-06-03 08:05:15 UTC
Test in symposiuem Koha. Not see "depends on"
Comment 20 Jonathan Druart 2014-06-03 08:49:36 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-06-03 08:49:42 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2014-06-03 08:49:47 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-06-03 08:49:59 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2014-06-03 08:50:04 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2014-06-03 08:50:11 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2014-06-03 08:50:17 UTC Comment hidden (obsolete)
Comment 27 Koha Team University Lyon 3 2014-06-19 15:05:57 UTC
Ok, I've tested the intranet, the opac on bootstrap and on prog themes.
At each time, I can delete research in the history search of catalog  and the history search of authority. I can delete research from the current session or from the former sessions.
If logout and return to the history search : the lines are still deleted and I can delete the other ones.

The only thing that bothers me is that after deleting lines of search autority, I go to the tab of the catalog search history. This isn't blocking for me. 
So I switch to "signed off"
Comment 28 Biblibre Sandboxes 2014-06-19 15:07:47 UTC
Patch tested with a sandbox, by sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Comment 29 Biblibre Sandboxes 2014-06-19 15:08:54 UTC Comment hidden (obsolete)
Comment 30 Biblibre Sandboxes 2014-06-19 15:08:57 UTC Comment hidden (obsolete)
Comment 31 Biblibre Sandboxes 2014-06-19 15:09:01 UTC Comment hidden (obsolete)
Comment 32 Biblibre Sandboxes 2014-06-19 15:09:05 UTC Comment hidden (obsolete)
Comment 33 Biblibre Sandboxes 2014-06-19 15:09:09 UTC Comment hidden (obsolete)
Comment 34 Biblibre Sandboxes 2014-06-19 15:09:12 UTC Comment hidden (obsolete)
Comment 35 Biblibre Sandboxes 2014-06-19 15:09:15 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2014-06-19 16:07:02 UTC
Comment on attachment 29042 [details] [review]
Bug 11430: DB changes: Add the primary key for search_history

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

Hi Jonathan, taking a look at the patches with splinter first.

::: installer/data/mysql/updatedatabase.pl
@@ +8574,5 @@
> +    |);
> +    print "Upgrade to $DBversion done (Bug 11430 - Add primary key for search_history)\n";
> +    SetVersion($DBversion);
> +}
> +

In kohastructure.sql id will be the first column, it will be the last when running updatedatabase.
Comment 37 Katrin Fischer 2014-06-19 16:16:33 UTC
Comment on attachment 29046 [details] [review]
Bug 11430: OPAC changes -- prog theme

Obsoleting patch for the prog theme, as the theme has been officially deprecated and no new features will be added.
Comment 38 Jonathan Druart 2014-06-19 16:31:21 UTC Comment hidden (obsolete)
Comment 39 Katrin Fischer 2014-06-19 18:45:57 UTC
Comment on attachment 29048 [details] [review]
Bug 11430: Fix paths to the checkboxes jquery plugin

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

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-search-history.tt
@@ +8,1 @@
>  <script type="text/javascript">

Hi Jonathan, 

I think we need to get the prog template change removed from this patch.

I also get a conflict here, could you check?
Comment 40 Katrin Fischer 2014-06-19 18:46:12 UTC
Comment on attachment 29045 [details] [review]
Bug 11430: OPAC changes

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

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-search-history.tt
@@ +215,3 @@
>  <script type="text/javascript">
>  //<![CDATA[
> +    var MSG_CONFIRM_DELETE_HISTORY = _("Are you sure you want to delete selected search history?");

Hm, maybe that would be a bit better as:
Are you sure you want to delete the selected search history entries?
Comment 41 Katrin Fischer 2014-06-19 18:50:56 UTC
3) I think it would be nice if there was a bit more padding between the 'Select all' 'Unselect all' links in staff and OPAC - but that's just a note, not a blocker.


4) I also have problems with the select/unselect links in the search history in staff, they are not working and reporting an error:

TypeError: $(...).unCheckCheckboxes is not a function
$(form).unCheckCheckboxes();


Overall this looks good! I think we are pretty close.
Comment 42 Jonathan Druart 2014-06-20 07:45:33 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2014-06-20 07:45:38 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2014-06-20 07:45:50 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2014-06-20 07:45:54 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2014-06-20 07:45:59 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2014-06-20 15:43:33 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2014-06-20 15:43:47 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2014-06-20 15:43:51 UTC Comment hidden (obsolete)
Comment 50 Kyle M Hall 2014-06-20 15:43:55 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2014-06-20 15:43:58 UTC Comment hidden (obsolete)
Comment 52 Owen Leonard 2014-06-20 16:20:09 UTC
I would like to add a follow-up to this before it is considered for pushing to master. The JavaScript-dependent features (selection links, delete link, etc) should not be visible if JavaScript is turned off.
Comment 53 Owen Leonard 2014-07-09 20:06:34 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2014-07-10 14:45:21 UTC
(In reply to Owen Leonard from comment #53)
> Created attachment 29592 [details] [review] [review]
> Note: This patch contains whitespace changes. Please diff accordingly.

Hi Owen,
What is the problem with the space alignment? There already are 2-space indentation in templates (and actually I have been using 2 spaces for at least 2 years).
I had the discussion with Kyle and he uses 2 spaces too.
I am not sure we had this king of requirement in the coding guidelines for template.
Since we regularly have a big amount of level in template, I think 2 spaces will avoid long lines.
Comment 55 Owen Leonard 2014-07-10 14:51:37 UTC
(In reply to Jonathan Druart from comment #54)

> What is the problem with the space alignment? There already are 2-space
> indentation in templates

I am certainly willing to be overruled, but I thought 4 spaces was a rule across Perl scripts and templates. It was in the Coding Guidelines for Perl scripts and I added it to the template guidelines yesterday because I thought the rule applied to both.

The Bootstrap theme templates were all created with 4-space indentation so I thought it was worth changing for consistency's sake.

As for long lines, I think consistent indentation (whether it follow a 2-space or 4-space rule) should be preferred over concerns about long lines. I consider long lines an issue that one's preferred editor should handle for you.
Comment 56 Jonathan Druart 2014-07-11 07:42:40 UTC
Owen,
Your patch adds a new button "delete selected", do you think it's a good idea to have 2 links for the same action? Maybe should we remove the one in the toolbar?

I searched for "#selections-toolbar" and I found opac-basket.pl. On the basket page, the links in the toolbar are displayed (but disabled), and there is no action button at the bottom. Moreover the following links "More details Send Download Print Empty and close" are clickable but don't work.

I am not sure what we should do here...
Comment 57 Owen Leonard 2014-07-11 12:49:21 UTC
(In reply to Jonathan Druart from comment #56)

> Your patch adds a new button "delete selected", do you think it's a good
> idea to have 2 links for the same action?

The primary reason for its inclusion is to make it usable with JavaScript off. The secondary benefit is that if you have a large number of results you can perform the delete action after scrolling down the page. Otherwise you have to return to the top. It looks a bit redundant with a small number of results but in my opinion is worth it. You can see the same thing on the lists page (opac-shelves.pl).

> I searched for "#selections-toolbar" and I found opac-basket.pl. On the
> basket page, the links in the toolbar are displayed (but disabled), and
> there is no action button at the bottom.

The cart doesn't work at all without JavaScript, so the cart page doesn't need to accommodate users without JavaScript.

> Moreover the following links "More
> details Send Download Print Empty and close" are clickable but don't work.

They work for me. Can you test again and file a separate bug if you can reproduce the problem?
Comment 58 Jonathan Druart 2014-07-11 13:19:44 UTC Comment hidden (obsolete)
Comment 59 Jonathan Druart 2014-07-11 13:19:49 UTC Comment hidden (obsolete)
Comment 60 Jonathan Druart 2014-07-11 13:19:54 UTC Comment hidden (obsolete)
Comment 61 Jonathan Druart 2014-07-11 13:20:04 UTC Comment hidden (obsolete)
Comment 62 Jonathan Druart 2014-07-11 13:20:09 UTC Comment hidden (obsolete)
Comment 63 Jonathan Druart 2014-07-11 13:20:15 UTC Comment hidden (obsolete)
Comment 64 Jonathan Druart 2014-07-11 13:22:11 UTC
Last patch set fixes conflict on opac.css file.

I added my SO on the last patch.
I send this enhancement back in the RM queue.
Comment 65 Jonathan Druart 2014-07-11 13:23:36 UTC
(In reply to Owen Leonard from comment #57)
> > Moreover the following links "More
> > details Send Download Print Empty and close" are clickable but don't work.
> 
> They work for me. Can you test again and file a separate bug if you can
> reproduce the problem?

It does not work with JS disabled.
All is fine with JS enabled.
Comment 66 Katrin Fischer 2014-11-21 18:41:18 UTC
I tried to rebase this, but I killed the new RSS link in the search history on first try... if someone else wants to give it a go, please do.
Comment 67 Jonathan Druart 2014-11-24 10:25:07 UTC
Created attachment 33831 [details] [review]
Bug 11430: delete search history by id - API changes

Bug 10807 adds a search history for authorities and bug 10862 adds the
search history on the staff interface.

This one allows the user to select the search history lines he wants to
delete.

This change is done for the OPAC and intranet interface. The user is now
allow to delete one or more lines of his/her search history.

Test plan (for intranet, opac: bootstrap and prog themes):
1/ launch some search (catalogue and authority)
2/ verify the lines is added to your search history
3/ delete one or more lines of the history and verify they have been
deleted
4/ at the OPAC: logout and do again steps 1-3.
5/ prove t/db_dependent/Search/History.t

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 68 Jonathan Druart 2014-11-24 10:25:12 UTC
Created attachment 33832 [details] [review]
Bug 11430: UT: add unit tests to delete only selected lines

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 69 Jonathan Druart 2014-11-24 10:25:19 UTC
Created attachment 33833 [details] [review]
Bug 11430: OPAC changes

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 70 Jonathan Druart 2014-11-24 10:25:24 UTC
Created attachment 33834 [details] [review]
Bug 11430: Intranet changes

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 71 Jonathan Druart 2014-11-24 10:25:29 UTC Comment hidden (obsolete)
Comment 72 Jonathan Druart 2014-11-24 10:25:34 UTC
Created attachment 33836 [details] [review]
Bug 11430 [Follow-up] Search history: Delete selected lines

This follow-up for Bug 11430 makes some changes to the template to make
it more useful when JavaScript is turned off:

- Hide the toolbar containing select all/clear all links if JS is off
- Add buttons to submit the forms if the delete link in the toolbar is
  unavailable (or if the user has scrolled all the way to the bottom of
  a long list)

Also changed: Corrected indentation from 2-space to 4-space, added
comments to the markup to help document page structure; changed the
class of the delete link to match other similar interfaces.

Note: This patch contains whitespace changes. Please diff
accordingly.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 73 Jonathan Druart 2014-11-24 10:26:15 UTC
Patches rebased.
Comment 74 Matthias Meusburger 2015-03-04 10:33:19 UTC
Created attachment 36444 [details] [review]
Bug 11430: DB changes: Add the primary key for search_history

Adds a primary key search_history.id.

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Matthias Meusburger 2015-03-04 10:33:38 UTC
patch rebased.
Comment 76 Tomás Cohen Arazi 2015-03-11 18:04:22 UTC
Created attachment 36820 [details] [review]
Bug 11430: (QA followup) small typos in kohastructure.sql

ìd (note the accent) is not a good column name :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 77 Tomás Cohen Arazi 2015-03-11 18:04:55 UTC
Created attachment 36821 [details] [review]
Bug 11430: (RM followup) DBIx schema update
Comment 78 Tomás Cohen Arazi 2015-03-11 18:11:31 UTC
Created attachment 36822 [details] [review]
Bug 11430: (QA followup) we test for warnings, always

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 79 Tomás Cohen Arazi 2015-03-11 18:18:38 UTC
Patches pushed to master.

Thanks Jonathan!