|
Description
Marc Véron
2017-04-17 07:20:50 UTC
Created attachment 62349 [details] [review] Bug 18438 - Implemented data-dismiss="modal" attribute on 'Confirm hold' and 'confirm' buttons so when they are cliked the modal box dissapears and any warning messages that were hidden by the box are now visible. The form is submitted via the js onclick function when the 'Comfirm holds' button is clicked because the checkin needs to be triggered by clicking this button. However when the 'Confirm' button is displayed this means that the item was not checked out when the user is trying to check it back in. In which case the check in does not need to be triggered and so in this patch the 'Confirm' button does not submt the form it simply closes the modal box to show any warning messages behind. Test plan: 1. Check out an item to one patron whilst having that item also being on hold to another patron 2. Check the item back in 3. Notice a modal box appears greying out the background with three buttons 'Confirm hold', 'Print and confirm' and 'Ignore'. Click confirm and notice that the page refreshes and no yellow warning messages are able to be displayed 4. Now that you have checked the item in. Try checking it in a second time by clicking on the Checkin tab and writing in the barcode. 5. The modal box will appear again, this time with three buttons 'Confirm', 'Print and confirm' and 'Cancel hold' 6. Click the 'Confirm' button and the page refreshes again and the yellow warning message hidden by the modal box is not properly displayed to the user 7. Apply patch 8. Try checking in the item again, and this time after clicking the 'confirm' button on the modal box notice that the yellow warning message is displayed telling the user the item was "Not checked out" 9. Drop the hold on the item and make sure it is not checked out. 10. Repeat steps 1 and 2 and notice after clicking the 'Confirm hold' button the page refreshes and the item is successfully checked back in Created attachment 62350 [details] [review] Bug 18438 - Removed unused js function call and if statement check for running js function This appears to be working really well, but I don't like that it introduces a new instance of "onclick." https://wiki.koha-community.org/wiki/Coding_Guidelines#JS9:_Avoid_the_use_of_event_attributes_like_.22onclick.22_to_attach_events I realize it's a pretty trivial use of it, but I would prefer that it be done according to coding guidelines. Created attachment 62422 [details] [review] Bug 18438 - Removed onclick event on 'Confirm hold' button to adhere to coding guidelines following feedback (In response to Owen Leonard in comment 3) Hi Owen As that onclick event isn't necessary I have removed it in the patch I just attached. Thanks Created attachment 62444 [details] [review] Bug 18438 - Implemented data-dismiss="modal" attribute on 'Confirm hold' and 'confirm' buttons so when they are cliked the modal box dissapears and any warning messages that were hidden by the box are now visible. The form is submitted via the js onclick function when the 'Comfirm holds' button is clicked because the checkin needs to be triggered by clicking this button. However when the 'Confirm' button is displayed this means that the item was not checked out when the user is trying to check it back in. In which case the check in does not need to be triggered and so in this patch the 'Confirm' button does not submt the form it simply closes the modal box to show any warning messages behind. Test plan: 1. Check out an item to one patron whilst having that item also being on hold to another patron 2. Check the item back in 3. Notice a modal box appears greying out the background with three buttons 'Confirm hold', 'Print and confirm' and 'Ignore'. Click confirm and notice that the page refreshes and no yellow warning messages are able to be displayed 4. Now that you have checked the item in. Try checking it in a second time by clicking on the Checkin tab and writing in the barcode. 5. The modal box will appear again, this time with three buttons 'Confirm', 'Print and confirm' and 'Cancel hold' 6. Click the 'Confirm' button and the page refreshes again and the yellow warning message hidden by the modal box is not properly displayed to the user 7. Apply patch 8. Try checking in the item again, and this time after clicking the 'confirm' button on the modal box notice that the yellow warning message is displayed telling the user the item was "Not checked out" 9. Drop the hold on the item and make sure it is not checked out. 10. Repeat steps 1 and 2 and notice after clicking the 'Confirm hold' button the page refreshes and the item is successfully checked back in Tested 3 patches together, followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 62445 [details] [review] Bug 18438 - Removed unused js function call and if statement check for running js function Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 62446 [details] [review] Bug 18438 - Removed onclick event on 'Confirm hold' button to adhere to coding guidelines following feedback Signed-off-by: Marc Véron <veron@veron.ch> (In reply to Marc Véron from comment #6) > 8. Try checking in the item again, and this time after clicking the > 'confirm' button on the modal box notice that the yellow warning message > is displayed telling the user the item was "Not checked out" Yes, but the hold is not confirmed! The form submission (and so the page reload) does something: it confirms the hold, which is why we expect here :) Created attachment 62708 [details] [review] Bug 18438 - Removed the data-dismiss="modal" from all confirm buttons except where an item is waiting. This means the hold is confirmed for a item by the form submitting and page reloading in all cases except where the item is being attempted to be checked in again where it is actually waiting in the library for pickuop by user with a hold on it. The confirm button in this case does not require the page reloading because the hidden input 'cancel_reserve' value of it is 0 Same test plan as comment 1 Works good, just small issue - I think that after dismissing modal, the focus should be on the input field for checking in... Created attachment 62955 [details] [review] Bug 18438 - Added focus on the checkin barcode input box after modal box is dismissed (In reply to Alex Buckley from comment #12) > Created attachment 62955 [details] [review] [review] > Bug 18438 - Added focus on the checkin barcode input box after modal box is > dismissed Alex, you should avoid to use onclick parameter on submit button. Created attachment 63002 [details] [review] Bug 18438 - Amended implementation of focus on barcode input box when the modal box is closed. Created attachment 63003 [details] [review] [SIGNED-OFF] Bug 18438 - Implemented data-dismiss="modal" attribute on 'Confirm hold' and 'confirm' buttons so when they are cliked the modal box dissapears and any warning messages that were hidden by the box are now visible. The form is submitted via the js onclick function when the 'Comfirm holds' button is clicked because the checkin needs to be triggered by clicking this button. However when the 'Confirm' button is displayed this means that the item was not checked out when the user is trying to check it back in. In which case the check in does not need to be triggered and so in this patch the 'Confirm' button does not submt the form it simply closes the modal box to show any warning messages behind. Test plan: 1. Check out an item to one patron whilst having that item also being on hold to another patron 2. Check the item back in 3. Notice a modal box appears greying out the background with three buttons 'Confirm hold', 'Print and confirm' and 'Ignore'. Click confirm and notice that the page refreshes and no yellow warning messages are able to be displayed 4. Now that you have checked the item in. Try checking it in a second time by clicking on the Checkin tab and writing in the barcode. 5. The modal box will appear again, this time with three buttons 'Confirm', 'Print and confirm' and 'Cancel hold' 6. Click the 'Confirm' button and the page refreshes again and the yellow warning message hidden by the modal box is not properly displayed to the user 7. Apply patch 8. Try checking in the item again, and this time after clicking the 'confirm' button on the modal box notice that the yellow warning message is displayed telling the user the item was "Not checked out" 9. Drop the hold on the item and make sure it is not checked out. 10. Repeat steps 1 and 2 and notice after clicking the 'Confirm hold' button the page refreshes and the item is successfully checked back in Tested 3 patches together, followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 63004 [details] [review] [SIGNED-OFF] Bug 18438 - Removed unused js function call and if statement check for running js function Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 63005 [details] [review] [SIGNED-OFF] Bug 18438 - Removed onclick event on 'Confirm hold' button to adhere to coding guidelines following feedback Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 63006 [details] [review] [SIGNED-OFF] Bug 18438 - Removed the data-dismiss="modal" from all confirm buttons except where an item is waiting. This means the hold is confirmed for a item by the form submitting and page reloading in all cases except where the item is being attempted to be checked in again where it is actually waiting in the library for pickuop by user with a hold on it. The confirm button in this case does not require the page reloading because the hidden input 'cancel_reserve' value of it is 0 Same test plan as comment 1 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 63007 [details] [review] [SIGNED-OFF] Bug 18438 - Amended implementation of focus on barcode input box when the modal box is closed. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> I do not understand where we are with these patches. What are the expected behaviour changes? (In reply to Jonathan Druart from comment #20) > I do not understand where we are with these patches. > What are the expected behaviour changes? As I understand, the change is, that the page doesn't reload after dismissing hold modal window, so the message under the window does not disappear and user haven't wait for another reload... (In reply to Josef Moravec from comment #21) > (In reply to Jonathan Druart from comment #20) > > I do not understand where we are with these patches. > > What are the expected behaviour changes? > > As I understand, the change is, that the page doesn't reload after > dismissing hold modal window, so the message under the window does not > disappear and user haven't wait for another reload... Yes, it is what I understood too, but it is the behaviour I already get against master. Alex, can you squash the patches and provide a simple test plan please? Hi Jonathan Sure thing, I'll do that when I have a bit of time this evening Created attachment 63371 [details] [review] Bug 18438 - Implemented data-dismiss="modal" in returns.tt so that any warning messages hidden by a hold modal are displayed after it is dismissed Test plan: 1. Check out an item to one patron whilst having that item also being on hold to another patron 2. Check the item back in 3. Notice a modal box appears greying out the background with three buttons 'Confirm hold', 'Print and confirm' and 'Ignore'. Click confirm and notice that the page refreshes and no yellow warning messages are able to be displayed 4. Now that you have checked the item in. Try checking it in a second time by clicking on the Checkin tab and writing in the barcode. 5. The modal box will appear again, this time with three buttons 'Confirm', 'Print and confirm' and 'Cancel hold' 6. Click the 'Confirm' button and the page refreshes again and the yellow warning message hidden by the modal box is not properly displayed to the user. Notice that the focus is on the barcode input box. 7. Apply patch 8. Try checking in the item again, and this time after clicking the 'confirm' button on the modal box notice that the yellow warning message is displayed telling the user the item was "Not checked out". Also notice the focus is in the barcode inputbox. 9. Drop the hold on the item and make sure it is not checked out. 10. Repeat steps 1 and 2 and notice after clicking the 'Confirm hold' button the page refreshes and the item is successfully checked back in. With the focus on the barcode input. 11. View koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt and notice that the button on line 345 does not use an onclick parameter Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 63372 [details] [review] Bug 18438 - Implemented data-dismiss="modal" in returns.tt so that any warning messages hidden by a hold modal are displayed after it is dismissed Test plan: 1. Check out an item to one patron whilst having that item also being on hold to another patron 2. Check the item back in 3. Notice a modal box appears greying out the background with three buttons 'Confirm hold', 'Print and confirm' and 'Ignore'. Click confirm and notice that the page refreshes and no yellow warning messages are able to be displayed 4. Now that you have checked the item in. Try checking it in a second time by clicking on the Checkin tab and writing in the barcode. 5. The modal box will appear again, this time with three buttons 'Confirm', 'Print and confirm' and 'Cancel hold' 6. Click the 'Confirm' button and the page refreshes again and the yellow warning message hidden by the modal box is not properly displayed to the user. Notice that the focus is on the barcode input box. 7. Apply patch 8. Try checking in the item again, and this time after clicking the 'confirm' button on the modal box notice that the yellow warning message is displayed telling the user the item was "Not checked out". Also notice the focus is in the barcode inputbox. 9. Drop the hold on the item and make sure it is not checked out. 10. Repeat steps 1 and 2 and notice after clicking the 'Confirm hold' button the page refreshes and the item is successfully checked back in. With the focus on the barcode input. 11. View koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt and notice that the button on line 345 does not use an onclick parameter This patch is just the previous 5 patches squashed down to one with an up to date test plan, the patch functionality itself has not been changed Created attachment 63458 [details] [review] Bug 18438 - Implemented data-dismiss="modal" in returns.tt so that any warning messages hidden by a hold modal are displayed after it is dismissed Test plan: 1. Check out an item to one patron whilst having that item also being on hold to another patron 2. Check the item back in 3. Notice a modal box appears greying out the background with three buttons 'Confirm hold', 'Print and confirm' and 'Ignore'. Click confirm and notice that the page refreshes and no yellow warning messages are able to be displayed 4. Now that you have checked the item in. Try checking it in a second time by clicking on the Checkin tab and writing in the barcode. 5. The modal box will appear again, this time with three buttons 'Confirm', 'Print and confirm' and 'Cancel hold' 6. Click the 'Confirm' button and the page refreshes again and the yellow warning message hidden by the modal box is not properly displayed to the user. Notice that the focus is on the barcode input box. 7. Apply patch 8. Try checking in the item again, and this time after clicking the 'confirm' button on the modal box notice that the yellow warning message is displayed telling the user the item was "Not checked out". Also notice the focus is in the barcode inputbox. 9. Drop the hold on the item and make sure it is not checked out. 10. Repeat steps 1 and 2 and notice after clicking the 'Confirm hold' button the page refreshes and the item is successfully checked back in. With the focus on the barcode input. 11. View koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt and notice that the button on line 345 does not use an onclick parameter Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 63549 [details] [review] Bug 18438 - Implemented data-dismiss="modal" in returns.tt so that any warning messages hidden by a hold modal are displayed after it is dismissed Test plan: 1. Check out an item to one patron whilst having that item also being on hold to another patron 2. Check the item back in 3. Notice a modal box appears greying out the background with three buttons 'Confirm hold', 'Print and confirm' and 'Ignore'. Click confirm and notice that the page refreshes and no yellow warning messages are able to be displayed 4. Now that you have checked the item in. Try checking it in a second time by clicking on the Checkin tab and writing in the barcode. 5. The modal box will appear again, this time with three buttons 'Confirm', 'Print and confirm' and 'Cancel hold' 6. Click the 'Confirm' button and the page refreshes again and the yellow warning message hidden by the modal box is not properly displayed to the user. Notice that the focus is on the barcode input box. 7. Apply patch 8. Try checking in the item again, and this time after clicking the 'confirm' button on the modal box notice that the yellow warning message is displayed telling the user the item was "Not checked out". Also notice the focus is in the barcode inputbox. 9. Drop the hold on the item and make sure it is not checked out. 10. Repeat steps 1 and 2 and notice after clicking the 'Confirm hold' button the page refreshes and the item is successfully checked back in. With the focus on the barcode input. 11. View koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt and notice that the button on line 345 does not use an onclick parameter Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Amended-patch: remove spaces Pushed to master for 17.05, thanks Alex! This patch has been pushed to 16.11.x and will be in 16.11.08. (In reply to Katrin Fischer from comment #30) > This patch has been pushed to 16.11.x and will be in 16.11.08. Blocked by Enhancement (bz11360), skipping for 16.05.x |