Bug 11127

Summary: OPAC suggestion: the delete link is active when no suggestion selected
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: OPACAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, m.de.rooy, oleonard, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11090
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 11127: OPAC suggestion: the delete link is active when no suggestion selected
Bug 11127: OPAC suggestion: the delete link is active when no suggestion selected
[Signed-off] Bug 11127: OPAC suggestion: the delete link is active when no suggestion selected
Bug 11127: Define a specific message for suggestion
Bug 11127: Define a specific message for suggestion
Bug 11127: Define a specific message for suggestion
[SIGNED OFF] Bug 11127: Define a specific message for suggestion
[PASSED QA] Bug 11127: OPAC suggestion: the delete link is active when no suggestion selected
[PASSED QA] Bug 11127: Define a specific message for suggestion

Description Jonathan Druart 2013-10-24 09:40:31 UTC

    
Comment 1 Jonathan Druart 2013-10-24 09:44:28 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-10-24 09:46:06 UTC
Should I produce the same fix for others themes (bootstrap and ccsr)?
Comment 3 Owen Leonard 2013-10-24 13:59:49 UTC
Instead of adding the "disabled" attribute to an element which isn't really supposed to have it (according to the specification) I think it would be better to check for the existence of a checked checkbox:

  var checkedBoxes = $("input:checked");
  if ($(checkedBoxes).size() == 0) {
     alert(MSG_NO_RECORD_SELECTED);
     return false;
  }
Comment 4 Marcel de Rooy 2013-10-31 11:23:03 UTC
Jonathan:
If there are still suggestions, there is also the Delete selected button below.
If you click on that button without a selection, it still says: Selected sugg have been deleted.

For consistency, I would recommend adjusting that too.
Comment 5 Jonathan Druart 2013-10-31 13:27:51 UTC
Owen,
Why are there 2 ways to delete selected suggestions? Should not we keep only the button on the bottom?
Comment 6 Jonathan Druart 2013-11-13 14:42:01 UTC
ping Owen
Comment 7 Owen Leonard 2013-11-14 14:43:04 UTC
(In reply to Jonathan Druart from comment #6)

> ping Owen

Sorry, here I am!

(In reply to Jonathan Druart from comment #5)

> Why are there 2 ways to delete selected suggestions? Should not we keep only
> the button on the bottom?

I don't see any reason not to offer both options. We do the same on the lists page and the on the user account page with renewal options.
Comment 8 Jonathan Druart 2014-03-04 15:31:55 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2014-03-04 16:53:18 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2014-03-11 10:14:05 UTC
Hi Jonathan,

I like the idea and it works nicely :) The only thing I am a bit concerned about is reusing the message from the cart. I think right now they are always in the template even if the cart is turned off, but that might change at some point. Also the message might read a bit nicer as "No suggestion has been selected" - as item is also a bit confusing for translations.
Comment 11 Jonathan Druart 2014-03-11 11:07:43 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2014-03-11 11:44:49 UTC
Hi Jonathan, the alert doesn't come up for me now and I can't see the new message in the template. Maybe we could just add the string directly into the alert? There seems to be an example for this a bit below. I think the MSG.* mostly exist because the Javascript for those is in a separate .js file.
Comment 13 Owen Leonard 2014-03-11 13:00:10 UTC
The alert isn't coming up now because the MSG is defined in the prog include, but the template is only modified for the bootstrap theme.

I would probably just put the MSG definition in the template itself since it's only relevant to that one page.
Comment 14 Jonathan Druart 2014-03-11 15:35:06 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-03-11 15:35:39 UTC
(In reply to Owen Leonard from comment #13)
> The alert isn't coming up now because the MSG is defined in the prog
> include, but the template is only modified for the bootstrap theme.

Arg stupid mistake, sorry Katrin!
Comment 16 Katrin Fischer 2014-03-12 09:29:11 UTC
Hi Jonathan,

I am sorry, but after takig a look at the code, I think we need to move the line with the message out of the IF statement, or it will depend on cart and lists being turned on:
    [% IF Koha.Preference( 'opacbookbag' ) == 1 or Koha.Preference( 'virtualshelves' ) == 1 %]
Comment 17 Jonathan Druart 2014-03-12 12:10:18 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize (ashimema) 2014-03-13 15:17:48 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2014-03-13 15:20:41 UTC
Created attachment 26308 [details] [review]
[PASSED QA] Bug 11127: OPAC suggestion: the delete link is active when no suggestion selected

To reproduce:
1/ go on opac-suggestions.pl
2/ create a suggestion
3/ click on the delete link without checking any checkbox
The form is submitted with a message "The selected suggestions have been
deleted.".

Test plan:
0/ enable the bootstrap theme
1/ after applying the patch, click on delete without any checkbox
checked, a js alert should occur.
2/ check at least a cb and click on the delete link. The deletion should
work.

Followed test plan. Patch behaves as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script, works as described.
Comment 20 Katrin Fischer 2014-03-13 15:20:51 UTC
Created attachment 26309 [details] [review]
[PASSED QA] Bug 11127: Define a specific message for suggestion

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 21 Galen Charlton 2014-03-13 16:51:11 UTC
Pushed to master.  Thanks, Jonathan!
Comment 22 Fridolin Somers 2014-06-12 08:10:28 UTC
Pushed to 3.14.x, will be in 3.14.08
Comment 23 Fridolin Somers 2014-06-12 08:13:40 UTC
Oups, the title of this bug was the one of Bug 11934 : "Replace given by if-else statements"
Comment 24 Fridolin Somers 2014-06-12 08:18:21 UTC
Sorry, I've add a odd behavior with my Lazarus Plugin.
I restore this bug from history.