At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button.
Created attachment 23364 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Place a hold on an item 2) Scan that item's barcode though returns.pl 3) Note the hold trap 4) Scan another barcode 5) Note you were able to bypass interacting with the trapped hold form 6) Apply this patch 7) Repeat steps 2-4 8) Note you must now click one of the buttons. Clicking ignore will allow you to continue scanning items as before.
Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 23367 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Place a hold on an item 2) Scan that item's barcode though returns.pl 3) Note the hold trap 4) Scan another barcode 5) Note you were able to bypass interacting with the trapped hold form 6) Apply this patch 7) Repeat steps 2-4 8) Note you must now click one of the buttons. Clicking ignore will allow you to continue scanning items as before. Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Looks good. I tested in a sandbox and it did what was expected. Hold trap failed before the patch applied, and then the hold trap worked after. I like that the barcode field is completely deactivated, rather than the hold trap just taking the focus off. This forces you to push a button. Our current jquery that addresses this just takes the focus off. We tossed around the idea of deactivating the barcode field altogether, but didn't have the nerve to do that much. I think the behavior is acceptable and expected. Christopher
Sorry to come late to this patch, but I want to jump in: Is there any reason to show the input field at all if it's disabled? We are inconsistent on the checkout page. Sometimes the input is displayed as disabled, sometimes it is hidden. I think we should stick with hiding the input (and the "options" checkboxes too in this case) if the user cannot interact with them.
Owen, Can you provide any examples of when the field is hidden? I was unaware it did this. Maybe I have not been in a circumstance that it has. Christopher
putting the hold notification in a modal window might be nice here as well - something that greys the screen and puts the hold notice on top (with a non-js fallback, of course)
(In reply to Owen Leonard from comment #5) > Is there any reason to show the input field at all if it's disabled? We are > inconsistent on the checkout page. Sometimes the input is displayed as > disabled, sometimes it is hidden. I think we should stick with hiding the > input (and the "options" checkboxes too in this case) if the user cannot > interact with them. Owen, could you explain this point please? This patch impacts the check-in page and I don't know where there is hidden fields. To me it is consistent to disable the input and the submit button here. (In reply to Liz Rea from comment #7) > putting the hold notification in a modal window might be nice here as well - > something that greys the screen and puts the hold notice on top (with a > non-js fallback, of course) It is out-of-the-scope of this bug. Maybe could you open a new report for that? Marked as Passed QA.
Created attachment 23511 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Place a hold on an item 2) Scan that item's barcode though returns.pl 3) Note the hold trap 4) Scan another barcode 5) Note you were able to bypass interacting with the trapped hold form 6) Apply this patch 7) Repeat steps 2-4 8) Note you must now click one of the buttons. Clicking ignore will allow you to continue scanning items as before. Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
This patch can lead to a situation where the barcode form and button is disabled without providing the staff member any way of enabling it (short of use the back button or the like). Specifically, this occurs if an item that does /not/ fill a hold and which does not already have an active branchtransfer record is checked in a library other than its home. The staff member can print a transfer slip, but doing so does not enable the barcode input form. Marking as failed QA.
By the way, my gut feeling is that taking up Liz's suggestion using modal dialog windows for checkin conditions that require staff acknowledgement would help avoid the sort of regression I found.
There are a couple approaches you could go from here. Currently we have a jquery setup that simply takes the focus off the input box. It doesn't disable anything. This has been a successful adjustment for us. You might think that this is a loose fix, as you can click on the text box and continue on, but it is really no different than clicking the ignore button. The other approach is to fix the patch to re-enable the input box and button for transfers. I don't know if it is worth going to a modal dialog window in this one function. It isn't a consistent workflow throughout Koha. When you start to do things differently in different parts of the software, it really breaks down workflow. I think modal dialog windows would be great for many alert messages throughout Koha, but it needs to be consistent throughout, otherwise workflow is disrupted, and you might look for these alerts one way and not the other. Christopher
(In reply to Christopher Brannon from comment #12) > I don't know if it is worth going to a modal dialog window in this one > function. It isn't a consistent workflow throughout Koha. When you start > to do things differently in different parts of the software, it really > breaks down workflow. I think modal dialog windows would be great for many > alert messages throughout Koha, but it needs to be consistent throughout, > otherwise workflow is disrupted, and you might look for these alerts one way > and not the other. I agree about consistency, but from the other direction: namely, Bootstrap modals are only used in a couple places currently, but if folks generally think that they're a good direction to go in, then circulation is a good place to get started with adding more use of them: it's faster for a circ operator to dismiss a modal than to wait for a round-trip to the server with the current approach for dealing with blocking alerts during circ operations.
> I agree about consistency, but from the other direction: namely, Bootstrap > modals are only used in a couple places currently, but if folks generally > think that they're a good direction to go in, then circulation is a good > place to get started with adding more use of them: it's faster for a circ > operator to dismiss a modal than to wait for a round-trip to the server with > the current approach for dealing with blocking alerts during circ operations. I concur!
Created attachment 44967 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Apply this patch 2) Trap a hold, note the new modal 3) Check the hold in a second time 4) Note the new modal 5) For each modal, test each button
So, I don't have a barcode scanner at my desk, but if I simulate just typing in a barcode and pressing enter while the modal is up I can still just check in the next item
To be more clear: Failing QA because the input box is still selected and active while the modal is up. Typing a barcode and pressing enter (i.e. scanning the next book) will check in the next item without taking any action on the hold which is the same behavior as before patch. The modal does look very nice though and buttons seem to work as expected
Created attachment 45038 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Apply this patch 2) Trap a hold, note the new modal 3) Check the hold in a second time 4) Note the new modal 5) For each modal, test each button
(In reply to Nick Clemens from comment #17) > To be more clear: > > Failing QA because the input box is still selected and active while the > modal is up. > > Typing a barcode and pressing enter (i.e. scanning the next book) will check > in the next item without taking any action on the hold which is the same > behavior as before patch. > > The modal does look very nice though and buttons seem to work as expected Good catch! This new version should take care of that issue!
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. If libraries are concerned about seeing the "not checked out" message this is problematic since Bug 14821 now blocks those transactions from appearing in the table of scanned items. And while I don't personally know what the checkin message option is used for, I don't think it's acceptable that it disappears. Perhaps those messages should not be cleared when the modal is dismissed?
Created attachment 45122 [details] Screenshot of layout issues This screenshot highlights a few markup/layout issues: - The list which displays the patron information is missing the enclosing <ul></ul> - The list shouldn't be centered. There is CSS at line 1225 of staff-global.css which aligns lists which are displayed in a message div. It should be expanded to include '.modal-body ul' - The layout of the buttons doesn't work, but I'm not sure what to do about that.
(In reply to Owen Leonard from comment #21) > Created attachment 45122 [details] > Screenshot of layout issues > > This screenshot highlights a few markup/layout issues: > > - The list which displays the patron information is missing the enclosing > <ul></ul> > - The list shouldn't be centered. There is CSS at line 1225 of > staff-global.css which aligns lists which are displayed in a message div. It > should be expanded to include '.modal-body ul' I removed the ul tags to make it readable. The ul styling was 'odd'. Would you be able to style the data? My css-fu isn't all that great ; ) > - The layout of the buttons doesn't work, but I'm not sure what to do about > that. That's really odd, because they look great on my system! Are you using any styling beyond stock Koha on your test system?
Kyle I would like to look at this again. Can you please rebase?
Created attachment 47594 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Apply this patch 2) Trap a hold, note the new modal 3) Check the hold in a second time 4) Note the new modal 5) For each modal, test each button
Sorry, there is a merge conflict in returns.tt
Created attachment 48749 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Apply this patch 2) Trap a hold, note the new modal 3) Check the hold in a second time 4) Note the new modal 5) For each modal, test each button
(In reply to Owen Leonard from comment #20) > 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. This is still the case, and I still think it's a problem. The modal hides any other messages on the page and the messages are gone when you dismiss the modal. Regression: This undoes Bug 15670.
I personally find the default Bootstrap colored button styles to be garish and I don't think we should use them. I prefer a standard button with color added to the icons. This is another case where we need to get some consensus before introducing a wholly new style to the interface.
(In reply to Owen Leonard from comment #28) > I personally find the default Bootstrap colored button styles to be garish > and I don't think we should use them. I prefer a standard button with color > added to the icons. > > This is another case where we need to get some consensus before introducing > a wholly new style to the interface. Our library actually uses jquery to color some buttons to make them stand out. Frankly, I think the buttons all blend together, and I don't think colored icons wouldn't stand out enough. I think on the staff side, if garish improves workflow and reduces errors, then bring on the garish. Christopher
(In reply to Owen Leonard from comment #27) > (In reply to Owen Leonard from comment #20) > > > 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. > > This is still the case, and I still think it's a problem. The modal hides > any other messages on the page and the messages are gone when you dismiss > the modal. > Any suggestions for how to resolve this issue?
(In reply to Christopher Brannon from comment #29) > (In reply to Owen Leonard from comment #28) > > I personally find the default Bootstrap colored button styles to be garish > > and I don't think we should use them. I prefer a standard button with color > > added to the icons. > > > > This is another case where we need to get some consensus before introducing > > a wholly new style to the interface. > > Our library actually uses jquery to color some buttons to make them stand > out. Frankly, I think the buttons all blend together, and I don't think > colored icons wouldn't stand out enough. I think on the staff side, if > garish improves workflow and reduces errors, then bring on the garish. > > Christopher I think a good compromise is to use the bootstrap buttons with the standard gray color. Garishness can easily be added via css/jQuery ; )
I think it's best not to introduce new global style changes in patches which have a non-global scope. Changing the buttons just in this interface would make it break with the design of all similar interfaces. That's not to say that we should continue to update the way Koha looks. I just think we need to do it in a more deliberate way. If folks want to develop a new standard for how we style buttons, make that proposal as a separate bug, a post to koha-devel, a wiki page, etc. Post screenshots and say, "Let's start doing things this way!" and propose a plan for updating the interface across the board.
Consistency++ That said, we can still try to make it really easy to change things using ids and classes, so that libraries can make things stand out if they choose to :)
Lest all this talk of style suggest otherwise, it was the issues in comment #27 that prompted me to mark this as Failed QA.
(In reply to Kyle M Hall from comment #31) > (In reply to Christopher Brannon from comment #29) > > (In reply to Owen Leonard from comment #28) > > > I personally find the default Bootstrap colored button styles to be garish > > > and I don't think we should use them. I prefer a standard button with color > > > added to the icons. > > > > > > This is another case where we need to get some consensus before introducing > > > a wholly new style to the interface. > > > > Our library actually uses jquery to color some buttons to make them stand > > out. Frankly, I think the buttons all blend together, and I don't think > > colored icons wouldn't stand out enough. I think on the staff side, if > > garish improves workflow and reduces errors, then bring on the garish. > > > > Christopher > > I think a good compromise is to use the bootstrap buttons with the standard > gray color. Garishness can easily be added via css/jQuery ; ) As long as the buttons have distinctive IDs. :)
(In reply to Owen Leonard from comment #34) > Lest all this talk of style suggest otherwise, it was the issues in comment > #27 that prompted me to mark this as Failed QA. Which part of comment 27?
(In reply to Kyle M Hall from comment #36) > Which part of comment 27? The whole thing! 1. The dialog obscures any other messages which might be displayed during checkin, then hides those messages when the dialog is dismissed. 2. The patch reverts the changes made by Bug 15670.
(In reply to Owen Leonard from comment #37) > (In reply to Kyle M Hall from comment #36) > > Which part of comment 27? > > The whole thing! > > 1. The dialog obscures any other messages which might be displayed during > checkin, then hides those messages when the dialog is dismissed. > > 2. The patch reverts the changes made by Bug 15670. Let's focus on point 1 for now. What would you suggest as a fix for that?
Unless there is a good case for hiding all messages, why not simply leave them there?
Created attachment 48790 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Apply this patch 2) Trap a hold, note the new modal 3) Check the hold in a second time 4) Note the new modal 5) For each modal, test each button
This latest patch should fix both of your issues Owen!
(In reply to Kyle M Hall from comment #41) > This latest patch should fix both of your issues Owen! Does it mean it's ready to be tested?
Created attachment 50343 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Apply this patch 2) Trap a hold, note the new modal 3) Check the hold in a second time 4) Note the new modal 5) For each modal, test each button
(In reply to Jonathan Druart from comment #42) > (In reply to Kyle M Hall from comment #41) > > This latest patch should fix both of your issues Owen! > > Does it mean it's ready to be tested? I do believe so!
This looks good, and the test plan checks out. I do see some other instances of a <div class="dialog message"> left in the template which seem to be related to holds. Shouldn't these cases be covered too?
(In reply to Owen Leonard from comment #45) > This looks good, and the test plan checks out. I do see some other instances > of a <div class="dialog message"> left in the template which seem to be > related to holds. > > Shouldn't these cases be covered too? I don't believe it's necessary. I do see the "Hold needing transfer found" one, but I cannot figure out how to even trigger it. I wonder if it's just dead code. In any case, this patch won't impede the ability to see and act on those messages and we can follow this up with future bugs to take care of those as necessary. If there is a specific message you see needing taken care of let me know!
work as descibe
Does that mean you sign off on this patch? If so please go ahead and submit a sign off. Nicole
Created attachment 51964 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Apply this patch 2) Trap a hold, note the new modal 3) Check the hold in a second time 4) Note the new modal 5) For each modal, test each button Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
Created attachment 53205 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Apply this patch 2) Trap a hold, note the new modal 3) Check the hold in a second time 4) Note the new modal 5) For each modal, test each button Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Long read... and I have also tested this a bit: 1) Please make the buttons use the standard styling. - The current colors don't work with our green/grey/blue scheme. 2) I'd add a line break between the heading and the title - currently, the title can't be very long without breaking and it obscures the heading, which is what you should be able to focus on easily. Testing "Hold found (item is already waiting)" - 3) I don't think the "cancel hold" button should be the very first. What do you think? It's currently in the middle. My feeling goes towards: Confirm, Print and confirm, Cancel hold Testing "Hold found" 4) The buttons don't look nice breaking over 2 lines (ok, not a blocker... but...) I am changing this to enh, because I feel this is a bigger change now about the look and feel, that might be better in a stable version. Please let me know, if you disagree. I like the new address layout without the bullet points! BTW - do we need to repeat all that code for those 2 similar cases? (sep bug..)
Created attachment 53531 [details] [review] Bug 11360 [QA Followup] - Fix styles and button order
(In reply to Katrin Fischer from comment #53) > Long read... and I have also tested this a bit: I believe this followup should take care of your concerns! We can't do much about the buttons breaking over two lines. I believe that's just the way Bootstrap deals with it.
(In reply to Katrin Fischer from comment #53) Everything looks good to me (you will need bug 16942 to test this patch). > BTW - do we need to repeat all that code for those 2 similar cases? (sep > bug..) Existed before this patch, but yes it would be nice to refactor stuffs.
Created attachment 53685 [details] [review] Bug 11360 - Disable barcode field and submit button when a hold is found At the moment when scanning in returns, it can be easy to miss a hold trap due to the fact that the barcode and submit are still active and the barcode field is focused on. A librarian who is focused on scanning can easily scan another item and pass over the form, which is equivalent to clicking the "ignore" button. Test Plan: 1) Apply this patch 2) Trap a hold, note the new modal 3) Check the hold in a second time 4) Note the new modal 5) For each modal, test each button Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 53686 [details] [review] Bug 11360 [QA Followup] - Fix styles and button order Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Jonathan Druart from comment #58) > Created attachment 53686 [details] [review] [review] > Bug 11360 [QA Followup] - Fix styles and button order > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Jonathan did you intend to pass qa on this patch set, or did you just add an additional sign-off?
(In reply to Kyle M Hall from comment #59) > (In reply to Jonathan Druart from comment #58) > > Created attachment 53686 [details] [review] [review] [review] > > Bug 11360 [QA Followup] - Fix styles and button order > > > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> > > Jonathan did you intend to pass qa on this patch set, or did you just add an > additional sign-off? I wanted to let Katrin to have another look, but she told me it's ok if I PQA it.
Just took another look - agree with Jonathan!
Pushed to master for 16.11!
*** Bug 6392 has been marked as a duplicate of this bug. ***
Enhancement, skipping for 16.05.x
*** Bug 11899 has been marked as a duplicate of this bug. ***