Bug 25251 - When a record has no items click delete all does not need an alert
Summary: When a record has no items click delete all does not need an alert
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 11821
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-22 13:49 UTC by Nick Clemens
Modified: 2022-03-25 14:37 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.05


Attachments
Bug 25251: Remove alert from disabled buttons (1.32 KB, patch)
2022-03-14 17:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25251: Remove alert from disabled buttons (1.46 KB, patch)
2022-03-14 18:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25251: Remove alert from disabled buttons (1.51 KB, patch)
2022-03-14 18:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25251: (follow-up) Remove alertNoItems (1.01 KB, patch)
2022-03-22 11:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25251: Remove alert from disabled buttons (1.59 KB, patch)
2022-03-22 15:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25251: (follow-up) Remove alertNoItems (1.09 KB, patch)
2022-03-22 15:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-04-22 13:49:52 UTC
It seems to be overkill, the delete all button is grayed out, has a tooltip, but click it generates an alert that must be dismissed. I think the first two are sufficient

To recreate:
1 - Find a record with items in the staff interface
2 - Click edit
3 - Note 'Delete all items' is grayed out
4 - Note hovering displays a note that there are no items
5 - Click on it anyway, an alert is generated
Comment 1 Jonathan Druart 2020-04-29 10:47:13 UTC
It's a feature, from:

commit 5c1691a8013bd38d6bae8b00a8950e1417695176
Author: Owen Leonard <oleonard@myacpl.org>
Date:   Fri Feb 21 15:27:41 2014 -0500

    Bug 11821: disable catalog edit menu items and provide explanations when actions cannot be done
    
    Biblio edit menu items which are disabled should trigger a tooltip
    on hover and an alert on click with explanations. This patch implements
    this for the relevent menu items.
Comment 2 Nick Clemens 2020-04-30 12:24:21 UTC
<oleonard> kidclamp: But seriously, I think removing the alert is fine. Voiceover in OSX reads the tooltip so I think we're okay there.
Comment 3 Katrin Fischer 2022-02-19 18:40:28 UTC
The alert is still there, if we wanted to remove it.
Comment 4 Nick Clemens 2022-03-14 17:36:37 UTC
Created attachment 131669 [details] [review]
Bug 25251: Remove alert from disabled buttons

To test:
1 - Find a record with items in the staff interface
2 - Click edit
3 - Note 'Edit items in a batch, 'Delete items in a batch', and 'Delete all items' are grayed out
4 - Note hovering displays a note that there are no items
5 - Click on each anyway, an alert is generated
6 - Apply patch
7 - Reload page
8 - Confirm no more alert
9 - Confirm tooltip is displayed
Comment 5 Owen Leonard 2022-03-14 18:18:34 UTC
Nick what would you think of this:

     $("#batchedit-disabled,#batchdelete-disabled,#deleteallitems-disabled")
         .on("click",function(e){
-            e.preventDefault();
-            alertNoItems();
+            e.stopPropagation();
         })
         .tooltip();

This would remove the alert, but it would also prevent the menu from closing when the user clicks on a disabled link. This makes sense to me because a user clicking on a disabled link probably means they're not paying attention. In that case having nothing happen is better than having something happen (even if it's just the menu closing).
Comment 6 Nick Clemens 2022-03-14 18:20:33 UTC
(In reply to Owen Leonard from comment #5)
> Nick what would you think of this:
> 
>      $("#batchedit-disabled,#batchdelete-disabled,#deleteallitems-disabled")
>          .on("click",function(e){
> -            e.preventDefault();
> -            alertNoItems();
> +            e.stopPropagation();
>          })
>          .tooltip();
> 
> This would remove the alert, but it would also prevent the menu from closing
> when the user clicks on a disabled link. This makes sense to me because a
> user clicking on a disabled link probably means they're not paying
> attention. In that case having nothing happen is better than having
> something happen (even if it's just the menu closing).

I would like that :-) I wanted it, but didn't know how to attain it - would you like to add the followup?
Comment 7 Nick Clemens 2022-03-14 18:29:18 UTC
Created attachment 131674 [details] [review]
Bug 25251: Remove alert from disabled buttons

This patch removes the alert, but adds a stopPropagation call
in order to not close the menu when the disabled buttons are clicked

To test:
1 - Find a record with items in the staff interface
2 - Click edit
3 - Note 'Edit items in a batch, 'Delete items in a batch', and 'Delete all items' are grayed out
4 - Note hovering displays a note that there are no items
5 - Click on each anyway, an alert is generated, menu is closed
6 - Apply patch
7 - Reload page
8 - Click on each disabled option and confirm
    - there is no more alert
    - the menu does not close
9 - Confirm tooltip is displayed
Comment 8 Owen Leonard 2022-03-14 18:35:24 UTC
Created attachment 131675 [details] [review]
Bug 25251: Remove alert from disabled buttons

This patch removes the alert, but adds a stopPropagation call
in order to not close the menu when the disabled buttons are clicked

To test:
1 - Find a record with items in the staff interface
2 - Click edit
3 - Note 'Edit items in a batch, 'Delete items in a batch', and 'Delete
    all items' are grayed out
4 - Note hovering displays a note that there are no items
5 - Click on each anyway, an alert is generated, menu is closed
6 - Apply patch
7 - Reload page
8 - Click on each disabled option and confirm
    - there is no more alert
    - the menu does not close
9 - Confirm tooltip is displayed

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Jonathan Druart 2022-03-21 14:02:25 UTC
Shouldn't we tweak confirm_items_deletion a bit and remove alertNoItems?
We wwill never reach it anymore.
Comment 10 Nick Clemens 2022-03-22 11:41:22 UTC
Created attachment 132012 [details] [review]
Bug 25251: (follow-up) Remove alertNoItems

We should not need this function any longer

I left the else conditional just in case
Comment 11 Jonathan Druart 2022-03-22 15:48:09 UTC
Created attachment 132029 [details] [review]
Bug 25251: Remove alert from disabled buttons

This patch removes the alert, but adds a stopPropagation call
in order to not close the menu when the disabled buttons are clicked

To test:
1 - Find a record with items in the staff interface
2 - Click edit
3 - Note 'Edit items in a batch, 'Delete items in a batch', and 'Delete
    all items' are grayed out
4 - Note hovering displays a note that there are no items
5 - Click on each anyway, an alert is generated, menu is closed
6 - Apply patch
7 - Reload page
8 - Click on each disabled option and confirm
    - there is no more alert
    - the menu does not close
9 - Confirm tooltip is displayed

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2022-03-22 15:48:13 UTC
Created attachment 132030 [details] [review]
Bug 25251: (follow-up) Remove alertNoItems

We should not need this function any longer

I left the else conditional just in case

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Fridolin Somers 2022-03-24 23:53:50 UTC
Totally makes sens
Comment 14 Fridolin Somers 2022-03-25 00:25:07 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 15 Kyle M Hall 2022-03-25 14:37:16 UTC
Pushed to 21.11.x for 21.11.05