Bug 25251

Summary: When a record has no items click delete all does not need an alert
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: CatalogingAssignee: Nick Clemens (kidclamp) <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, kyle, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.05
Bug Depends on: 11821    
Bug Blocks:    
Attachments: Bug 25251: Remove alert from disabled buttons
Bug 25251: Remove alert from disabled buttons
Bug 25251: Remove alert from disabled buttons
Bug 25251: (follow-up) Remove alertNoItems
Bug 25251: Remove alert from disabled buttons
Bug 25251: (follow-up) Remove alertNoItems

Description Nick Clemens (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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