Bug 18438

Summary: Check in: Modal about holds hides important check in messages
Product: Koha Reporter: Marc Véron <veron>
Component: CirculationAssignee: Alex Buckley <alexbuckley>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: alexbuckley, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, kyle, lisettepalouse+koha, mtj
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14224
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 11360    
Bug Blocks:    
Attachments: 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.
Bug 18438 - Removed unused js function call and if statement check for running js function
Bug 18438 - Removed onclick event on 'Confirm hold' button to adhere to coding guidelines following feedback
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.
Bug 18438 - Removed unused js function call and if statement check for running js function
Bug 18438 - Removed onclick event on 'Confirm hold' button to adhere to coding guidelines following feedback
Bug 18438 - Removed the data-dismiss="modal" from all confirm buttons except where an item is waiting.
Bug 18438 - Added focus on the checkin barcode input box after modal box is dismissed
Bug 18438 - Amended implementation of focus on barcode input box when the modal box is closed.
[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.
[SIGNED-OFF] Bug 18438 - Removed unused js function call and if statement check for running js function
[SIGNED-OFF] Bug 18438 - Removed onclick event on 'Confirm hold' button to adhere to coding guidelines following feedback
[SIGNED-OFF] Bug 18438 - Removed the data-dismiss="modal" from all confirm buttons except where an item is waiting.
[SIGNED-OFF] Bug 18438 - Amended implementation of focus on barcode input box when the modal box is closed.
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
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
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
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

Description Marc Véron 2017-04-17 07:20:50 UTC
To reproduce:
- Have a patron X who has an item checked out where an other patron has a hold on it
- Make sure patron X triggers a message while checking in (e.g. add a manual fine, add a manual restriction)
- Check the item in on Home > Circulation > Check in (/koha/circ/circulation.pl)
Result: A modal window appears that hides the messages about fines, restriction. 'Confirm hold' triggers the check-in; librarians have noch chance to see the other messages.

Introduced by Bug 11360.
See also comment #20 on Bug 11360 
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11360#c20
"If a checkin triggers the modal, any messages or alerts which were also triggered by the checkin will be hidden by the modal and will disappear after clearing the modal. For instance: A warning that the item was not checked out, or a checkin message which has been configured for that particular item type."
Comment 1 Alex Buckley 2017-04-19 02:44:20 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
Comment 2 Alex Buckley 2017-04-19 02:51:51 UTC
Created attachment 62350 [details] [review]
Bug 18438 - Removed unused js function call and if statement check for running js function
Comment 3 Owen Leonard 2017-04-19 12:52:37 UTC
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.
Comment 4 Alex Buckley 2017-04-19 21:10:10 UTC
Created attachment 62422 [details] [review]
Bug 18438 - Removed onclick event on 'Confirm hold' button to adhere to coding guidelines following feedback
Comment 5 Alex Buckley 2017-04-19 21:11:21 UTC
(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
Comment 6 Marc Véron 2017-04-20 07:34:40 UTC
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>
Comment 7 Marc Véron 2017-04-20 07:35:25 UTC
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>
Comment 8 Marc Véron 2017-04-20 07:36:14 UTC
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>
Comment 9 Jonathan Druart 2017-04-24 19:52:53 UTC
(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 :)
Comment 10 Alex Buckley 2017-04-26 08:57:21 UTC
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
Comment 11 Josef Moravec 2017-04-26 12:27:55 UTC
Works good, just small issue - I think that after dismissing modal, the focus should be on the input field for checking in...
Comment 12 Alex Buckley 2017-05-02 12:15:39 UTC
Created attachment 62955 [details] [review]
Bug 18438 - Added focus on the checkin barcode input box after modal box is dismissed
Comment 13 Josef Moravec 2017-05-03 05:35:30 UTC
(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.
Comment 14 Alex Buckley 2017-05-03 07:21:18 UTC
Created attachment 63002 [details] [review]
Bug 18438 - Amended implementation of focus on barcode input box when the modal box is closed.
Comment 15 Josef Moravec 2017-05-03 07:59:52 UTC
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>
Comment 16 Josef Moravec 2017-05-03 07:59:59 UTC
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>
Comment 17 Josef Moravec 2017-05-03 08:00:05 UTC
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>
Comment 18 Josef Moravec 2017-05-03 08:00:11 UTC
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>
Comment 19 Josef Moravec 2017-05-03 08:00:17 UTC
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>
Comment 20 Jonathan Druart 2017-05-08 15:18:12 UTC
I do not understand where we are with these patches.
What are the expected behaviour changes?
Comment 21 Josef Moravec 2017-05-09 08:22:03 UTC
(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...
Comment 22 Jonathan Druart 2017-05-09 13:08:26 UTC
(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?
Comment 23 Alex Buckley 2017-05-10 02:31:20 UTC
Hi Jonathan

Sure thing, I'll do that when I have a bit of time this evening
Comment 24 Alex Buckley 2017-05-10 21:18:33 UTC
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>
Comment 25 Alex Buckley 2017-05-10 21:19:56 UTC
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
Comment 26 Alex Buckley 2017-05-10 21:21:18 UTC
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
Comment 27 Marc Véron 2017-05-15 03:51:54 UTC
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>
Comment 28 Jonathan Druart 2017-05-18 20:54:26 UTC
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
Comment 29 Kyle M Hall 2017-05-19 14:25:17 UTC
Pushed to master for 17.05, thanks Alex!
Comment 30 Katrin Fischer 2017-05-21 21:12:11 UTC
This patch has been pushed to 16.11.x and will be in 16.11.08.
Comment 31 Mason James 2017-05-24 00:59:22 UTC
(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