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.
Created attachment 23713 [details] [review] Bug 11430: DB changes: Add the primary key for search_history Adds a primary key search_history.id.
Created attachment 23714 [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
Created attachment 23715 [details] [review] Bug 11430: UT: add unit tests to delete only selected lines
Created attachment 23716 [details] [review] Bug 11430: OPAC changes
Created attachment 23717 [details] [review] Bug 11430: OPAC changes -- prog theme
Created attachment 23718 [details] [review] Bug 11430: Intranet changes
Created attachment 25499 [details] [review] Bug 11430: DB changes: Add the primary key for search_history Adds a primary key search_history.id.
Created attachment 25500 [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
Created attachment 25501 [details] [review] Bug 11430: UT: add unit tests to delete only selected lines
Created attachment 25502 [details] [review] Bug 11430: OPAC changes
Created attachment 25503 [details] [review] Bug 11430: OPAC changes -- prog theme
Created attachment 25504 [details] [review] Bug 11430: Intranet changes
Rebased patches
Created attachment 25618 [details] [review] Bug 11430: Fix paths to the checkboxes jquery plugin This patch fixes the path for the checkboxes jquery plugin (opac theme prog and intranet).
Patch "delete search history by id - API changes" does not apply
Did you apply dependencies before applying these ones?
(In reply to Jonathan Druart from comment #16) > Did you apply dependencies before applying these ones? Ah, no, sorry! Marc
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
Test in symposiuem Koha. Not see "depends on"
Created attachment 28636 [details] [review] Bug 11430: DB changes: Add the primary key for search_history Adds a primary key search_history.id.
Created attachment 28637 [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
Created attachment 28638 [details] [review] Bug 11430: UT: add unit tests to delete only selected lines
Created attachment 28639 [details] [review] Bug 11430: OPAC changes
Created attachment 28640 [details] [review] Bug 11430: OPAC changes -- prog theme
Created attachment 28641 [details] [review] Bug 11430: Intranet changes
Created attachment 28642 [details] [review] Bug 11430: Fix paths to the checkboxes jquery plugin This patch fixes the path for the checkboxes jquery plugin (opac theme prog and intranet).
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"
Patch tested with a sandbox, by sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 29042 [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>
Created attachment 29043 [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>
Created attachment 29044 [details] [review] Bug 11430: UT: add unit tests to delete only selected lines Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 29045 [details] [review] Bug 11430: OPAC changes Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 29046 [details] [review] Bug 11430: OPAC changes -- prog theme Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 29047 [details] [review] Bug 11430: Intranet changes Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 29048 [details] [review] Bug 11430: Fix paths to the checkboxes jquery plugin This patch fixes the path for the checkboxes jquery plugin (opac theme prog and intranet). Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
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 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.
Created attachment 29059 [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>
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 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?
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.
Created attachment 29081 [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>
Created attachment 29082 [details] [review] Bug 11430: UT: add unit tests to delete only selected lines Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 29083 [details] [review] Bug 11430: OPAC changes Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 29084 [details] [review] Bug 11430: Intranet changes Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 29085 [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>
Created attachment 29106 [details] [review] [PASSED QA] 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>
Created attachment 29107 [details] [review] [PASSED QA] 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>
Created attachment 29108 [details] [review] [PASSED QA] Bug 11430: OPAC changes Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 29109 [details] [review] [PASSED QA] Bug 11430: Intranet changes Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 29110 [details] [review] [PASSED QA] 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>
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.
Created attachment 29592 [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.
(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.
(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.
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...
(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?
Created attachment 29635 [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>
Created attachment 29636 [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>
Created attachment 29637 [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>
Created attachment 29638 [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>
Created attachment 29639 [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>
Created attachment 29640 [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>
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.
(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.
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.
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>
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>
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>
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>
Created attachment 33835 [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>
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>
Patches rebased.
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>
patch rebased.
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>
Created attachment 36821 [details] [review] Bug 11430: (RM followup) DBIx schema update
Created attachment 36822 [details] [review] Bug 11430: (QA followup) we test for warnings, always Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Patches pushed to master. Thanks Jonathan!