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
Still valid?
Still valid in master as of 21 Dec 2015
I don't think this is valid anymore. Can you please check?
Still valid in master as of 3/24/16.
Still valid, when you click on the red X on request.pl, you do not get a confirmation message.
Created attachment 59203 [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
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.
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
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.
This patch adds a lot of indentation changes but does not improve it. Could you provide a patch without the indentation changes?
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
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
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
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>
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>
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
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>
(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.
(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.
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.
Created attachment 60122 [details] Screen shot of how this patch looks.
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?
Pushed to master for 17.05, thanks Luke!
Created attachment 60220 [details] Screenshot in Firefox Just for comparison - it looks nicer in Firefox.
This patch has been pushed to 16.11.x and will be in 16.11.04.
Pushed to 3.22.x for 3.22.17
Pushed to 16.05.x, for 16.05.10 release