Bug 19116 - Holds not set to waiting when "Confirm" is used
Summary: Holds not set to waiting when "Confirm" is used
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Josef Moravec
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 19020 19273 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-08-15 16:23 UTC by Katrin Fischer
Modified: 2019-10-14 19:57 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19116: Hold not set to waiting after transfer (1.60 KB, patch)
2017-08-22 09:40 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19116: Hold not set to waiting after transfer (1.66 KB, patch)
2017-08-27 11:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19116: Add tests (2.38 KB, patch)
2017-09-04 17:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19116: Add tests (2.73 KB, patch)
2017-09-07 16:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19116: Hold not set to waiting after transfer (1.72 KB, patch)
2017-09-12 12:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19116: Add tests (2.78 KB, patch)
2017-09-12 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19116: Hold not set to waiting after transfer (1.71 KB, patch)
2017-09-22 08:45 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19116: (followup) Add tests to highlight the problem in CheckReserves (1.54 KB, patch)
2017-09-22 08:45 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19116: Hold not set to waiting after transfer (1.71 KB, patch)
2017-09-22 08:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19116: Add tests (2.77 KB, patch)
2017-09-22 08:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19116: (followup) Add tests to highlight the problem in CheckReserves (1.54 KB, patch)
2017-09-22 08:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19116: Unit tests (2.83 KB, patch)
2017-09-27 18:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19116: (followup) Add tests to highlight the problem in CheckReserves (1.59 KB, patch)
2017-09-27 18:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19116: Hold not set to waiting after transfer (1.77 KB, patch)
2017-09-27 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2017-08-15 16:23:37 UTC
When a hold with status transfer is checked in, we offer "Confirm" and "Print and confirm", both should set the item to waiting and create a pick-up letter if set up. But only the second option has this effect.

To test:
- Place holds on 2 items from another branch
- Switch to that branch
- Check them in at the other branch to set them into transport status (T)
- Switch back to your homebranch
- Check items in again, use the different confirm buttons and compare
- Only "confirm and print" will be set to waiting, "confirm" remains in transport.


Might be related to bug 19020.
Comment 1 Josef Moravec 2017-08-18 19:38:12 UTC
Hm, same issue was fixed in 17940, but it is here again...
Comment 2 Josef Moravec 2017-08-18 19:53:03 UTC
And it is caused by bug 18438...
Comment 3 Josef Moravec 2017-08-18 20:06:36 UTC
So we have to options - in hidden.bs.modal handler call some api to mark the hold waiting, which means to extend api first, svc nor rest don't support it

or to get somehow the showed messages and and pass them to returns.pl too - because we don't won't the regression on 18438 funcionality I think...

any opinions? Other solution?
Comment 4 Katrin Fischer 2017-08-18 20:21:48 UTC
Can you explain a bit about the dependency to the other bug? 
Just a bit confused because for me it looks like one button works, the other does not... so why not make one behave like the other (missing the deeper understanding here clearly :) )
Comment 5 Katrin Fischer 2017-08-18 21:43:20 UTC
Retested, had totally forgotten we switched to a modal there. Read through the other bug too, but not totally understanding the problem yet.

What is strange: if the item is not from a transfer, the button seems to work correctly?
Comment 6 Katrin Fischer 2017-08-22 05:30:28 UTC
Found a fixed bug that reads similarly: bug 17940
Comment 7 Josef Moravec 2017-08-22 09:36:15 UTC
(In reply to Katrin Fischer from comment #5)
> Retested, had totally forgotten we switched to a modal there. Read through
> the other bug too, but not totally understanding the problem yet.
> 
> What is strange: if the item is not from a transfer, the button seems to
> work correctly?

Bug 18438 does make modal dissmiss without reloading whole page, but when the item is not transferred, the page normaly reloads and the the hold is marked waiting. These are two different modals in returns.tt:

#hold-found1 - if the hold is already waiting - just dismissing the modal is OK, the hold is already marked waiting

#hold-found2 - hold found, need to be marked waiting - the modal does not dismiss when confirm button clicked, but the form is confirmed and page reloads and mark the hold waiting. This modal is dismissed when ignore button is clicked - so hold is not marked as waiting

When hold found and need transfer the #hold-found2 modal is used (just with other modal-footer because different button labels)

When the hold is transferred the #hold-found1 is used, but should not, the hold is not waiting yet!

The Waiting or Reserved status comes originally from C4::Reserves::CheckReserves, the 'T' status is considered waiting here - which is not true

I tried to fix it, patch is comming


I remove dependency on 18438 - it does not come from here, patch from bug 18438 just highlighted the problem
Comment 8 Josef Moravec 2017-08-22 09:40:50 UTC
Created attachment 66307 [details] [review]
Bug 19116: Hold not set to waiting after transfer

Test plan:

0) Do not apply the patch
1) Place hold on item from another branch
2) Switch to that branch
3) Check them in at the other branch to set them into transport status (T)
4) Switch back to your homebranch
5) Check items in again, use the different confirm buttons and
    compare: Only "confirm and print" will be set to waiting, "confirm"
    remains in transport.
6) Apply the patch
7) Repeat 1-5 - now should work as expected - the hold is marked waiting
on "confirm" button too
8) Check the hold from the same branch, to make sure this doesn't add
regression
Comment 9 Katrin Fischer 2017-08-27 11:48:52 UTC
Testing now - I think we might need a unit test for QA.
Comment 10 Katrin Fischer 2017-08-27 11:57:17 UTC
Created attachment 66511 [details] [review]
Bug 19116: Hold not set to waiting after transfer

Test plan:

0) Do not apply the patch
1) Place hold on item from another branch
2) Switch to that branch
3) Check them in at the other branch to set them into transport status (T)
4) Switch back to your homebranch
5) Check items in again, use the different confirm buttons and
    compare: Only "confirm and print" will be set to waiting, "confirm"
    remains in transport.
6) Apply the patch
7) Repeat 1-5 - now should work as expected - the hold is marked waiting
on "confirm" button too
8) Check the hold from the same branch, to make sure this doesn't add
regression

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2017-08-27 11:57:47 UTC
Thx a lot, Josef! Works perfectly!
Comment 12 Jonathan Druart 2017-08-29 13:20:47 UTC
I would suggest to invert the condition:
  If found eq 'T' then return "Reserved"
  Otherwise return "Waiting"

We will need tests indeed.

I have to admit that this subroutine (CheckReserves) always scared me, hard to say we are not going to break something.
Comment 13 Tomás Cohen Arazi 2017-08-31 12:11:00 UTC
Can we have tests for this one please?
Comment 14 Katrin Fischer 2017-09-04 15:18:28 UTC
Can someone help out here and write the tests necessary please?
Comment 15 Jonathan Druart 2017-09-04 17:15:17 UTC
Created attachment 66824 [details] [review]
Bug 19116: Add tests
Comment 16 Jonathan Druart 2017-09-04 17:15:48 UTC
I do not recreate the issue with tests, someone else?
Comment 17 Katrin Fischer 2017-09-04 19:11:33 UTC
If I am reading the test correctly, you miss returning in the library the item is 'sourced at' in order to create the transport status.
Comment 18 Katrin Fischer 2017-09-04 19:45:42 UTC
The problem is that the item is not in transit, but I haven't figured out how to fix the tests.
Comment 19 Katrin Fischer 2017-09-04 20:29:28 UTC
is( $hold->found, "T", 'Hold is in transit' );
before you return it again at the pickup library. No matter what I try, I only get 'undef'.
Comment 20 Jonathan Druart 2017-09-07 16:20:07 UTC
Created attachment 66947 [details] [review]
Bug 19116: Add tests
Comment 21 Jonathan Druart 2017-09-07 16:20:39 UTC
Another try, the tests still pass with and without the fix.
Comment 22 Jonathan Druart 2017-09-07 16:23:15 UTC
(In reply to Josef Moravec from comment #2)
> And it is caused by bug 18438...

If this is caused by template changes, why do we need to fix it at package level?
Comment 23 Katrin Fischer 2017-09-07 17:36:22 UTC
*** Bug 19273 has been marked as a duplicate of this bug. ***
Comment 24 Katrin Fischer 2017-09-08 06:13:40 UTC
There is a template level fix/patch from Kyle on the linked bug 19273 - not sure what makes more sense.
Comment 25 Kyle M Hall 2017-09-08 14:46:00 UTC
(In reply to Jonathan Druart from comment #22)
> (In reply to Josef Moravec from comment #2)
> > And it is caused by bug 18438...
> 
> If this is caused by template changes, why do we need to fix it at package
> level?

Which is why I'd suggest my fix on bug 19273 which is a one line template change.
Comment 26 Josef Moravec 2017-09-11 05:52:40 UTC
(In reply to Kyle M Hall from comment #25)
> (In reply to Jonathan Druart from comment #22)
> > (In reply to Josef Moravec from comment #2)
> > > And it is caused by bug 18438...
> > 
> > If this is caused by template changes, why do we need to fix it at package
> > level?

My fault, it is not caused by bug 18438, this bug just highlited the problem.

> 
> Which is why I'd suggest my fix on bug 19273 which is a one line template
> change.

The patch on bug 19273 just reverts the functionality provided by bug 18438...
Comment 27 Katrin Fischer 2017-09-11 15:14:59 UTC
So better to fix the tests on this bug then?
Comment 28 Kyle M Hall 2017-09-12 12:42:58 UTC
Created attachment 67079 [details] [review]
Bug 19116: Hold not set to waiting after transfer

Test plan:

0) Do not apply the patch
1) Place hold on item from another branch
2) Switch to that branch
3) Check them in at the other branch to set them into transport status (T)
4) Switch back to your homebranch
5) Check items in again, use the different confirm buttons and
    compare: Only "confirm and print" will be set to waiting, "confirm"
    remains in transport.
6) Apply the patch
7) Repeat 1-5 - now should work as expected - the hold is marked waiting
on "confirm" button too
8) Check the hold from the same branch, to make sure this doesn't add
regression

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2017-09-12 12:43:05 UTC
Created attachment 67080 [details] [review]
Bug 19116: Add tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall 2017-09-12 12:44:10 UTC
(In reply to Katrin Fischer from comment #27)
> So better to fix the tests on this bug then?

Katrin, I ran the unit tests on multiple data sets and they passed every time for me. I'm not willing to Pass QA without your approval though ; )
Comment 31 Jonathan Druart 2017-09-12 13:17:32 UTC
(In reply to Kyle M Hall from comment #30)
> (In reply to Katrin Fischer from comment #27)
> > So better to fix the tests on this bug then?
> 
> Katrin, I ran the unit tests on multiple data sets and they passed every
> time for me. I'm not willing to Pass QA without your approval though ; )

The tests do not highlight the problem, they pass with and without the patch.
Comment 32 Kyle M Hall 2017-09-12 14:43:42 UTC
(In reply to Jonathan Druart from comment #31)
> (In reply to Kyle M Hall from comment #30)
> > (In reply to Katrin Fischer from comment #27)
> > > So better to fix the tests on this bug then?
> > 
> > Katrin, I ran the unit tests on multiple data sets and they passed every
> > time for me. I'm not willing to Pass QA without your approval though ; )
> 
> The tests do not highlight the problem, they pass with and without the patch.

Ah, thanks for the clarity.
Comment 33 Josef Moravec 2017-09-22 08:45:19 UTC
Created attachment 67306 [details] [review]
Bug 19116: Hold not set to waiting after transfer

Test plan:

0) Do not apply the patch
1) Place hold on item from another branch
2) Switch to that branch
3) Check them in at the other branch to set them into transport status (T)
4) Switch back to your homebranch
5) Check items in again, use the different confirm buttons and
    compare: Only "confirm and print" will be set to waiting, "confirm"
    remains in transport.
6) Apply the patch
7) Repeat 1-5 - now should work as expected - the hold is marked waiting
on "confirm" button too
8) Check the hold from the same branch, to make sure this doesn't add
regression

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Josef Moravec 2017-09-22 08:45:55 UTC
Created attachment 67307 [details] [review]
Bug 19116: (followup) Add tests to highlight the problem in CheckReserves
Comment 35 Josef Moravec 2017-09-22 08:46:34 UTC
Created attachment 67308 [details] [review]
Bug 19116: Hold not set to waiting after transfer

Test plan:

0) Do not apply the patch
1) Place hold on item from another branch
2) Switch to that branch
3) Check them in at the other branch to set them into transport status (T)
4) Switch back to your homebranch
5) Check items in again, use the different confirm buttons and
    compare: Only "confirm and print" will be set to waiting, "confirm"
    remains in transport.
6) Apply the patch
7) Repeat 1-5 - now should work as expected - the hold is marked waiting
on "confirm" button too
8) Check the hold from the same branch, to make sure this doesn't add
regression

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Josef Moravec 2017-09-22 08:46:38 UTC
Created attachment 67309 [details] [review]
Bug 19116: Add tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 37 Josef Moravec 2017-09-22 08:46:42 UTC
Created attachment 67310 [details] [review]
Bug 19116: (followup) Add tests to highlight the problem in CheckReserves
Comment 38 Julian Maurice 2017-09-26 07:14:01 UTC
*** Bug 19020 has been marked as a duplicate of this bug. ***
Comment 39 Tomás Cohen Arazi 2017-09-27 18:23:41 UTC
Created attachment 67409 [details] [review]
Bug 19116: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2017-09-27 18:23:50 UTC
Created attachment 67410 [details] [review]
Bug 19116: (followup) Add tests to highlight the problem in CheckReserves

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2017-09-27 18:24:00 UTC
Created attachment 67411 [details] [review]
Bug 19116: Hold not set to waiting after transfer

Test plan:

0) Do not apply the patch
1) Place hold on item from another branch
2) Switch to that branch
3) Check them in at the other branch to set them into transport status (T)
4) Switch back to your homebranch
5) Check items in again, use the different confirm buttons and
    compare: Only "confirm and print" will be set to waiting, "confirm"
    remains in transport.
6) Apply the patch
7) Repeat 1-5 - now should work as expected - the hold is marked waiting
on "confirm" button too
8) Check the hold from the same branch, to make sure this doesn't add
regression

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 42 Jonathan Druart 2017-09-29 15:25:28 UTC
I am not confident with this patch, I am expecting side-effects.
But I do not have anything else to suggest, let's give a try.
Comment 43 Jonathan Druart 2017-09-29 15:47:03 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 44 Fridolin Somers 2017-10-10 07:08:39 UTC
Pushed to 17.05.x, will be in 17.05.05.

Really dirty bug ;) thanks all for your job.
Comment 45 Katrin Fischer 2017-10-14 11:54:02 UTC
These patches have been pushed to 16.11.x and will be in 16.11.13.
Comment 46 David Cook 2018-05-07 05:24:44 UTC
I'm confused... does this actually work?

I see the following code in master:

<div class="modal-footer">
    <input type="hidden" name="cancel_reserve" value="0" />

    <button type="submit" class="btn btn-default approve" data-dismiss="modal">
        <i class="fa fa-check"></i> Confirm
    </button>

    <input type="hidden" name="print_slip" value="0" />
    <button type="submit" class="btn btn-default print" onclick="this.form.print_slip.value = 1; this.form.submit();">
        <i class="fa fa-print"></i> Print and confirm
    </button>

    <button type="submit" class="btn btn-default deny" onclick="this.form.cancel_reserve.value = 1; this.form.submit();">
        <i class="fa fa-times"></i> Cancel hold
    </button>
</div>

"Print and confirm" and "Cancel hold" both work in the 17.05.03 but "Confirm" doesn't. If I add onclick="this.form.submit();" to the Confirm button, then it works. 

As per Kyle's patch on #19273, I think the problem is due to that data-dismiss attribute?
Comment 47 David Cook 2018-05-07 05:27:30 UTC
Especially when I don't see a data-dismiss attribute on any other button on that page.

If you're not transferring, then this is what the code looks like:

<button type="submit" class="btn btn-default approve">
    <i class="fa fa-check"></i> Confirm hold
</button>
<button type="submit" class="btn btn-default print" onclick="this.form.print_slip.value = 1; this.form.submit();">
    <i class="fa fa-print"></i> Print slip and confirm
</button>

So I think the answer is probably what Kyle did on the other bug report?

Although I admit I haven't tested the current stable code or master... this is just analysis based on an older version.