Bug 34668

This is really interesting, and seems to work well. I wonder if we should include an option to check a box in the modal that says "Don't warn me again" which would  suppress the warning for the current session?

I think what would happen is that the clerks would hit it automatically (and blindly) after a while, especially those that were targeted by this library request.

Considering each popup would be for a different user, with different books listed, this is always new information.  Turning it off seems counterintuitive, in my personnal opinion.

Humans, humans...

I think is going to be useful and it seems to work well. 

Can you check these QA errors:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
   FAIL	  filters
		missing_filter at line 967 (                    waiting_holds_barcodes.push("[% b %]");)


 WARN	circ/circulation.pl
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 240, now: 244)


For the perltidy warn maybe this is helpful: https://wiki.koha-community.org/wiki/Perltidy

QA errors should be fixed.

Created attachment 155188 [details] [review]
Bug 34668: Add popup warn librarians of waiting holds when checking out

When there are holds waiting for patrons, sometimes the librarian misses
the "Holds waiting here" display. This patch adds a modal popup warning
when checking out an item for a patron with waiting holds.

Test plan:
1) find a user (user1)
2) find a biblio (biblio1)
3) add a hold for biblio1 to user1
   (search biblio1 > Holds > find user1 > Place hold)
4) checkout biblio1 to another user if not already checked out
   and checkin through circulation page (not through the user page)
   > confirm hold
5) there should be a "Holds waiting here (1)" section added on user1 page

6) checkout any items that isn't the one on hold for user1
   -> notice it just checks out as normal
7) apply patch and update database
8) go to administration > system preferences > switch "WaitingNotifyAtCheckout" to "Notify"
9) refresh page and redo step 6
   -> notice there is now a modal to warn the librarian of a waiting hold
   click Ok to proceed with the checkout, click outside the modal to not do anything
10) additionnally, the popup shouldn't appear if we checkout any of
    user1's waiting hold

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>

Rather than unbinding/rebinding events, could we have an action bound to submit on the form, and then display the modal or not as needed? That would be more in line with current code I believe

(In reply to Nick Clemens from comment #9)
> Rather than unbinding/rebinding events, could we have an action bound to
> submit on the form, and then display the modal or not as needed? That would
> be more in line with current code I believe

>$('#circ-warnwaitingholds-modal .btn-primary').on('click',function() {
>    $('#mainform').submit();
>});

This is the action bound to (confirm) submit the usual checkout form from the modal.


The modal display logic depends on what is written in the barcode input, it needs to display the modal if the barcode about to be checked out is not among waiting holds, else it will submit the form as usual.

I currently do not see how this can be done without binding/unbinding events of the "Check out" button depending on the barcode input. Can I get more details on how to do what you suggest?

(In reply to Shi Yao Wang from comment #10)
> (In reply to Nick Clemens from comment #9)
> I currently do not see how this can be done without binding/unbinding events
> of the "Check out" button depending on the barcode input. Can I get more
> details on how to do what you suggest?

Something like this:

                $("#mainform").on('submit', function(){
                    if( $("#checkout_confirmed").length > 0 ){
                        return true;
                    }
                    if ( waiting_holds_barcodes.includes($('#barcode').val().trim()) ) {
                        return true;
                    } else {
                        $('#circ-warnwaitingholds-modal').modal();
                        return false;
                    }
                });
                $("#circ-warnwaitingholds-modal").on('hidden.bs.modal',function(){
                    $("#mainform").append('<input type="hidden" id="checkout_confirmed" value=1>').submit();
                });

I tried to signoff: Popup is showing but clicking Ok doesn't proceed the checkout, only popup disappears.

(In reply to Mia Kujala from comment #13)
> I tried to signoff: Popup is showing but clicking Ok doesn't proceed the
> checkout, only popup disappears.

Thank you for testing. It was indeed not proceeding with the checkout. It should be fixed now.

Tested again. Now the popup is not showing but it proceed the checkout

(In reply to Mia Kujala from comment #16)
> Tested again. Now the popup is not showing but it proceed the checkout

The popup shows up on my end... is the syspref active?
Also, the popup won't show if checking out an item waiting on hold.

Created attachment 157312 [details] [review]
Bug 34668: remove binding/unbinding event for one submit event

step 9) of previous patch is a bit different now, if we click outside of
the modal it will proceed with the checkout instead of not doing
anything

No new code changes, just changed the commit message.

Created attachment 157315 [details] [review]
Bug 34668: Add popup warn librarians of waiting holds when checking out

When there are holds waiting for patrons, sometimes the librarian misses
the "Holds waiting here" display. This patch adds a modal popup warning
when checking out an item for a patron with waiting holds.

Test plan:
1) find a user (user1)
2) find a biblio (biblio1)
3) add a hold for biblio1 to user1
   (search biblio1 > Holds > find user1 > Place hold)
4) checkout biblio1 to another user if not already checked out
   and checkin through circulation page (not through the user page)
   > confirm hold
5) there should be a "Holds waiting here (1)" section added on user1 page

6) checkout any items that isn't the one on hold for user1
   -> notice it just checks out as normal
7) apply patch and update database
8) go to administration > system preferences > switch "WaitingNotifyAtCheckout" to "Notify"
9) refresh page and redo step 6
   -> notice there is now a modal to warn the librarian of a waiting hold
   click Ok to proceed with the checkout, click outside the modal to not do anything
10) additionnally, the popup shouldn't appear if we checkout any of
    user1's waiting hold

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Signed-off-by: Mia Kujala <mia.kujala@xamk.fi>

Created attachment 157316 [details] [review]
Bug 34668: remove binding/unbinding event for one submit event

Signed-off-by: Mia Kujala <mia.kujala@xamk.fi>

Works as designed and described in the test plan.

My sandbox is not sending the sign off through- but I have tested and it works as expected.

Comment on attachment 157315 [details] [review]
Bug 34668: Add popup warn librarians of waiting holds when checking out

Review of attachment 157315 [details] [review]:
-----------------------------------------------------------------

::: circ/circulation.pl
@@ +552,5 @@
> +        #Check for waiting holds
> +        my $waiting_holds          = $patron->holds->search( { found => 'W', branchcode => $branch } );
> +        my @waiting_holds_barcodes = ();
> +        while ( my $hold = $waiting_holds->next ) {
> +            push( @waiting_holds_barcodes, $hold->item->barcode );

There is no need to make a list of the barcodes, just pass the entire waiting_holds rs to the template and let it loop over them.

Created attachment 158359 [details] [review]
Bug 34668: pass whole waiting_holds rs

Created attachment 158528 [details] [review]
Bug 34668: Add popup warn librarians of waiting holds when checking out

When there are holds waiting for patrons, sometimes the librarian misses
the "Holds waiting here" display. This patch adds a modal popup warning
when checking out an item for a patron with waiting holds.

Test plan:
1) find a user (user1)
2) find a biblio (biblio1)
3) add a hold for biblio1 to user1
   (search biblio1 > Holds > find user1 > Place hold)
4) checkout biblio1 to another user if not already checked out
   and checkin through circulation page (not through the user page)
   > confirm hold
5) there should be a "Holds waiting here (1)" section added on user1 page

6) checkout any items that isn't the one on hold for user1
   -> notice it just checks out as normal
7) apply patch and update database
8) go to administration > system preferences > switch "WaitingNotifyAtCheckout" to "Notify"
9) refresh page and redo step 6
   -> notice there is now a modal to warn the librarian of a waiting hold
   click Ok to proceed with the checkout, click outside the modal to not do anything
10) additionnally, the popup shouldn't appear if we checkout any of
    user1's waiting hold

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Signed-off-by: Mia Kujala <mia.kujala@xamk.fi>
Signed-off-by: Kelly <kelly@bywatersolutions.com>

Created attachment 158529 [details] [review]
Bug 34668: remove binding/unbinding event for one submit event

Signed-off-by: Mia Kujala <mia.kujala@xamk.fi>
Signed-off-by: Kelly <kelly@bywatersolutions.com>

Created attachment 158530 [details] [review]
Bug 34668: pass whole waiting_holds rs

Signed-off-by: Kelly <kelly@bywatersolutions.com>

Working as described. QA script is mostly happy, 1 issue with file permissions that I am fixing in a follow-up. The rest of the code looks good. DBUpdate looks good.

Created attachment 160673 [details] [review]
Bug 34668: Add popup warn librarians of waiting holds when checking out

When there are holds waiting for patrons, sometimes the librarian misses
the "Holds waiting here" display. This patch adds a modal popup warning
when checking out an item for a patron with waiting holds.

Test plan:
1) find a user (user1)
2) find a biblio (biblio1)
3) add a hold for biblio1 to user1
   (search biblio1 > Holds > find user1 > Place hold)
4) checkout biblio1 to another user if not already checked out
   and checkin through circulation page (not through the user page)
   > confirm hold
5) there should be a "Holds waiting here (1)" section added on user1 page

6) checkout any items that isn't the one on hold for user1
   -> notice it just checks out as normal
7) apply patch and update database
8) go to administration > system preferences > switch "WaitingNotifyAtCheckout" to "Notify"
9) refresh page and redo step 6
   -> notice there is now a modal to warn the librarian of a waiting hold
   click Ok to proceed with the checkout, click outside the modal to not do anything
10) additionnally, the popup shouldn't appear if we checkout any of
    user1's waiting hold

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Signed-off-by: Mia Kujala <mia.kujala@xamk.fi>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Created attachment 160674 [details] [review]
Bug 34668: remove binding/unbinding event for one submit event

Signed-off-by: Mia Kujala <mia.kujala@xamk.fi>
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Created attachment 160675 [details] [review]
Bug 34668: pass whole waiting_holds rs

Signed-off-by: Kelly <kelly@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Created attachment 160676 [details] [review]
Bug 34668: (QA follow-up) Change file permission

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Pushed for 24.05!

Well done everyone, thank you!

Enhancement not pushed to 23.11.x

I feel like this could get very annoying if the pop-up appears after each scan...?
If I check out 5 items to a single patron, the pop-up appears 5 times and I have to click 'OK' after every checkout.

I guess it would force me to go get the fbook and check it out to get the pop-up out of my face, but it would definitely get on my nerves.

For now, I'll add a note in the manual to warn about this behaviour.

Tested this test plan: If there is item1 waiting for user A and other available item2 connected to same biblio is going to be checked out by user B, also user B gets this pop-up "this patron has waiting holds" even if the user B doesn't have any items to be picked up.  

So this setting is maybe not functioning as it should: so all users get the notification when checking out same book title (different items) until user A picks up and check outs the hold.

A new ticket and a fix for it Bug 37199

(In reply to Saija Pyhtilä from comment #37)
> Tested this test plan: If there is item1 waiting for user A and other
> available item2 connected to same biblio is going to be checked out by user
> B, also user B gets this pop-up "this patron has waiting holds" even if the
> user B doesn't have any items to be picked up.  
> 
> So this setting is maybe not functioning as it should: so all users get the
> notification when checking out same book title (different items) until user
> A picks up and check outs the hold.

(In reply to Slava Shishkin from comment #38)
> A new ticket and a fix for it Bug 37199

Thanks for reporting issues with this bug, and a link to the bug.

Normally, if a bug is pushed to main and there are follow-up issues, we open another bug and link to it - instead of reopening the existing bug.

A fix for this issue is in bug 37055, which is signed off and is waiting for quality assurance.

Thanks again!

David Nind