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.
Hm, same issue was fixed in 17940, but it is here again...
And it is caused by bug 18438...
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?
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 :) )
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?
Found a fixed bug that reads similarly: bug 17940
(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
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
Testing now - I think we might need a unit test for QA.
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>
Thx a lot, Josef! Works perfectly!
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.
Can we have tests for this one please?
Can someone help out here and write the tests necessary please?
Created attachment 66824 [details] [review] Bug 19116: Add tests
I do not recreate the issue with tests, someone else?
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.
The problem is that the item is not in transit, but I haven't figured out how to fix the tests.
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'.
Created attachment 66947 [details] [review] Bug 19116: Add tests
Another try, the tests still pass with and without the fix.
(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?
*** Bug 19273 has been marked as a duplicate of this bug. ***
There is a template level fix/patch from Kyle on the linked bug 19273 - not sure what makes more sense.
(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.
(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...
So better to fix the tests on this bug then?
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>
Created attachment 67080 [details] [review] Bug 19116: Add tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(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 ; )
(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.
(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.
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>
Created attachment 67307 [details] [review] Bug 19116: (followup) Add tests to highlight the problem in CheckReserves
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>
Created attachment 67309 [details] [review] Bug 19116: Add tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 67310 [details] [review] Bug 19116: (followup) Add tests to highlight the problem in CheckReserves
*** Bug 19020 has been marked as a duplicate of this bug. ***
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>
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>
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>
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.
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.05. Really dirty bug ;) thanks all for your job.
These patches have been pushed to 16.11.x and will be in 16.11.13.
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?
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.