Bug 11450 - Hold Request Confirm Deletion
Summary: Hold Request Confirm Deletion
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Luke Honiss
QA Contact: Testopia
URL:
Whiteboard:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2013-12-26 23:49 UTC by Christopher Brannon
Modified: 2017-02-22 23:10 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 11450: Hold Request Confirm Deletion (5.16 KB, patch)
2017-01-19 03:02 UTC, Luke Honiss
Details | Diff | Splinter Review
Bug 11450: Hold Request Confirm Deletion (5.08 KB, patch)
2017-01-19 20:06 UTC, Luke Honiss
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11450: Hold Request Confirm Deletion (5.23 KB, patch)
2017-01-24 15:15 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 11450: Hold Request Confirm Deletion (3.52 KB, patch)
2017-01-25 23:07 UTC, Luke Honiss
Details | Diff | Splinter Review
Bug 11450: Hold Request Confirm Deletion (2.02 KB, patch)
2017-02-07 04:16 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 11450: Hold Request Confirm Deletion (2.07 KB, patch)
2017-02-07 04:44 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11450: Hold Request Confirm Deletion (2.15 KB, patch)
2017-02-08 19:28 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 11450: Hold Request Confirm Deletion (2.19 KB, patch)
2017-02-09 12:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Screen shot of how this patch looks. (9.01 KB, image/png)
2017-02-10 18:04 UTC, Christopher Brannon
Details
Screenshot in Firefox (12.38 KB, image/jpeg)
2017-02-14 19:24 UTC, Katrin Fischer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2013-12-26 23:49:52 UTC
Would like to see a confirmation screen before deleting hold requests (request.pl).  As it is now, if you purposely or accidently click on the delete button, the hold request is deleted instantly.

Christopher
Comment 1 Marc Véron 2015-12-20 17:34:26 UTC
Still valid?
Comment 2 Owen Leonard 2015-12-21 13:43:47 UTC
Still valid in master as of 21 Dec 2015
Comment 3 Aleisha Amohia 2016-03-24 00:50:41 UTC
I don't think this is valid anymore. Can you please check?
Comment 4 Christopher Brannon 2016-03-24 17:22:07 UTC
Still valid in master as of 3/24/16.
Comment 5 Jonathan Druart 2016-09-13 13:25:59 UTC
Still valid, when you click on the red X on request.pl, you do not get a confirmation message.
Comment 6 Luke Honiss 2017-01-19 03:02:42 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2017-01-19 19:55:44 UTC
This looks fine, it  just needs one small change. You don't need to define MSG_CONFIRM_CANCEL_HOLD in your patch because MSG_CONFIRM_DELETE_HOLD is already defined at the top. You can use MSG_CONFIRM_DELETE_HOLD instead.
Comment 8 Luke Honiss 2017-01-19 20:06:18 UTC
Created attachment 59276 [details] [review]
Bug 11450: Hold Request Confirm Deletion

==TEST PLAN==
1) Go to an item with a hold and click on the holds tab on the
left
2) Click the red 'X'
3) The hold will be deleted immediately
4) Apply patch
5) Return to an item with a hold and click the 'X'
6) There will now be a confirmation dialog
7) Click cancel and the dialog will disappear and the hold will not be
deleted
8) Click OK and the hold will be deleted
Comment 9 Owen Leonard 2017-01-24 15:15:04 UTC
Created attachment 59505 [details] [review]
[SIGNED-OFF] Bug 11450: Hold Request Confirm Deletion

==TEST PLAN==
1) Go to an item with a hold and click on the holds tab on the
left
2) Click the red 'X'
3) The hold will be deleted immediately
4) Apply patch
5) Return to an item with a hold and click the 'X'
6) There will now be a confirmation dialog
7) Click cancel and the dialog will disappear and the hold will not be
deleted
8) Click OK and the hold will be deleted

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

Made one minor correction while signing off: Removed some stray spaces
from inside a link.
Comment 10 Jonathan Druart 2017-01-25 12:52:03 UTC
This patch adds a lot of indentation changes but does not improve it.
Could you provide a patch without the indentation changes?
Comment 11 Luke Honiss 2017-01-25 23:07:57 UTC
Created attachment 59563 [details] [review]
Bug 11450: Hold Request Confirm Deletion

==TEST PLAN==
1) Go to an item with a hold and click on the holds tab on the
left
2) Click the red 'X'
3) The hold will be deleted immediately
4) Apply patch
5) Return to an item with a hold and click the 'X'
6) There will now be a confirmation dialog
7) Click cancel and the dialog will disappear and the hold will not be
deleted
8) Click OK and the hold will be deleted

Fixed some indentation
Comment 12 Chris Cormack 2017-01-26 00:38:01 UTC
Well done Luke working on your patches after the Academy is finished, very good to see!

Don't forget to obsolete any old patches if you add one to replace
Comment 13 M. Tompsett 2017-02-07 04:16:35 UTC
Created attachment 59967 [details] [review]
Bug 11450: Hold Request Confirm Deletion

==TEST PLAN==
1) Go to an item with a hold and click on the holds tab on the
left
2) Click the red 'X'
3) The hold will be deleted immediately
4) Apply patch
5) Return to an item with a hold and click the 'X'
6) There will now be a confirmation dialog
7) Click cancel and the dialog will disappear and the hold will not be
deleted
8) Click OK and the hold will be deleted

Restored indentations - Mark Tompsett
Comment 14 M. Tompsett 2017-02-07 04:44:26 UTC
Created attachment 59968 [details] [review]
Bug 11450: Hold Request Confirm Deletion

==TEST PLAN==
1) Go to an item with a hold and click on the holds tab on the
left
2) Click the red 'X'
3) The hold will be deleted immediately
4) Apply patch
5) Return to an item with a hold and click the 'X'
6) There will now be a confirmation dialog
7) Click cancel and the dialog will disappear and the hold will not be
deleted
8) Click OK and the hold will be deleted

NOTE: Restored indentations - Mark Tompsett
      New code change works great in staff!

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 15 Christopher Brannon 2017-02-08 19:28:20 UTC
Created attachment 60036 [details] [review]
[SIGNED OFF] Bug 11450: Hold Request Confirm Deletion

==TEST PLAN==
1) Go to an item with a hold and click on the holds tab on the
left
2) Click the red 'X'
3) The hold will be deleted immediately
4) Apply patch
5) Return to an item with a hold and click the 'X'
6) There will now be a confirmation dialog
7) Click cancel and the dialog will disappear and the hold will not be
deleted
8) Click OK and the hold will be deleted

Restored indentations - Mark Tompsett

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 16 Christopher Brannon 2017-02-08 19:31:06 UTC
Works as described, but it looks like a generic alert window is being used instead of a modal confirmation.  It has been standard for confirmations to be done through modal.  The alert includes information from the browser that could be confusing.

It would be preferred that this follow standard formatting before being pushed, but if all we are after here is the functionality, then it works.

Christopher
Comment 17 Jonathan Druart 2017-02-09 12:44:31 UTC
Created attachment 60068 [details] [review]
Bug 11450: Hold Request Confirm Deletion

==TEST PLAN==
1) Go to an item with a hold and click on the holds tab on the
left
2) Click the red 'X'
3) The hold will be deleted immediately
4) Apply patch
5) Return to an item with a hold and click the 'X'
6) There will now be a confirmation dialog
7) Click cancel and the dialog will disappear and the hold will not be
deleted
8) Click OK and the hold will be deleted

Restored indentations - Mark Tompsett

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 18 Jonathan Druart 2017-02-09 12:46:10 UTC
(In reply to Christopher Brannon from comment #16)
> Works as described, but it looks like a generic alert window is being used
> instead of a modal confirmation.  It has been standard for confirmations to
> be done through modal.  The alert includes information from the browser that
> could be confusing.
> 
> It would be preferred that this follow standard formatting before being
> pushed, but if all we are after here is the functionality, then it works.

I'd say we need it asap in stable release.
It will be easier to backport it as it is, rather than with a modal.
We could improve it later if needed.
Comment 19 Owen Leonard 2017-02-10 13:21:41 UTC
(In reply to Christopher Brannon from comment #16)

> a generic alert window is being used
> instead of a modal confirmation.  It has been standard for confirmations to
> be done through modal.

This is not true. Simple deletion confirmations are most often done via a JavaScript alert(). Some deletions, for instance in Administration, take the user to a separate page which displays a '<div class="dialog alert">' and asks for confirmation there. But these are not the norm.

I don't think any follow-up is required.
Comment 20 Christopher Brannon 2017-02-10 18:02:56 UTC
Well, it stood out to me.  I don't recall a lot, if any, of these alerts being used.  If they are, I think we should consider phasing them out with modals.  Like I mentioned earlier, the alert contains extra information that may be confusing.  It isn't consistent, and I am probably overly picky about keeping things consistent.  I've attached what I am seeing.  The alert just seems out of place, and it is typical to use message boxes or modals in Koha with particular fonts and colors to get attention.  This does not feel like part of the software.  It feels more like a shortcut to be honest.

Again, I am very grateful for the functionality, but I think we should maintain a more consistent look throughout Koha, and steer away from alerts whenever possible.
Comment 21 Christopher Brannon 2017-02-10 18:04:44 UTC
Created attachment 60122 [details]
Screen shot of how this patch looks.
Comment 22 Katrin Fischer 2017-02-12 23:03:30 UTC
As Owen pointed out, this pattern is still standard in Koha. As this is a bugfix, can we continue discussion about future style changes on a separate bug and have this go in?
Comment 23 Kyle M Hall 2017-02-14 13:46:08 UTC
Pushed to master for 17.05, thanks Luke!
Comment 24 Katrin Fischer 2017-02-14 19:24:26 UTC
Created attachment 60220 [details]
Screenshot in Firefox

Just for comparison - it looks nicer in Firefox.
Comment 25 Katrin Fischer 2017-02-14 19:25:33 UTC
This patch has been pushed to 16.11.x and will be in 16.11.04.
Comment 26 Julian Maurice 2017-02-20 09:07:30 UTC
Pushed to 3.22.x for 3.22.17
Comment 27 Mason James 2017-02-22 23:10:05 UTC
Pushed to 16.05.x, for 16.05.10 release