Bug 29792 - Transfers created from 'wrong transfer' checkin are not sent if modal is dismissed
Summary: Transfers created from 'wrong transfer' checkin are not sent if modal is dism...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 23092 24434 31663
Blocks: 33300
  Show dependency treegraph
 
Reported: 2022-01-04 18:15 UTC by Andrew Fuerste-Henry
Modified: 2023-12-28 20:43 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/669
Text to go in the release notes:
Version(s) released in:
22.11.00,22.05.10, 21.11.18


Attachments
Bug 29792: Automatically send wrong transfers (4.38 KB, patch)
2022-01-04 19:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29792: Automatically send wrong transfers (4.45 KB, patch)
2022-01-04 19:35 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29792: Automatically send wrong transfers (5.67 KB, patch)
2022-09-30 11:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29792: Automatically send wrong transfers (5.74 KB, patch)
2022-10-10 15:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29792: Automatically send wrong transfers (5.79 KB, patch)
2022-11-02 22:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29792: [22.05.x] Automatically send wrong transfers (5.87 KB, patch)
2023-02-16 12:14 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2022-01-04 18:15:58 UTC
To recreate:
1 - have an item in transit from Branch A to Branch B
2 - check the item in at Branch A
3 - get a Wrong Transfer message, click Print Slip or OK
4 - query the branchtransfers table, see that your original transfer was cancelled and a new one was made, confirm that new transfer has a datesent value
5 - repeat steps 1-3, but instead of clicking one of the buttons click outside of the modal to make it go away
6 - query the branchtransfers table, see that your original transfer was cancelled and a new one was made, confirm that new transfer DOES NOT HAVE a datesent value

So that transfer is just hanging out as requested and the item needs to get checked in again to get it sent. This is a change in behavior, as before bug 23092 the second transfer would be sent immediately even if one clicked away from the modal without selecting anything.
Comment 1 Andrew Fuerste-Henry 2022-01-04 18:49:05 UTC
I should have included that one must have TransfersBlockCirc turned off in order to click away from the modal without selecting something. Also, scanning in another barcode without addressing the modal has the same effect as clicking away from it.
Comment 2 Jason Robb 2022-01-04 18:58:31 UTC
This is causing disruption in our consortial setting.

It is not uncommon to check an item in multiple times. It is also not uncommon for a library to overlook prompts without clicking the buttons on those prompts.

Before, if a library checked in an item returning home twice consecutively, it remained in transit.

Now, if a library checks in an item returning home twice consecutively, it gets taken out of transit and the item appears Available at the wrong library.

As the behavior has changed, this has led to a non-trivial amount of items in our catalog showing Available when they were thought to be in transit.

It would be great if the previous behavior were restored, where any number of check-ins on a returning item kept it in transit to its home library (unless something else derails it like a newly placed hold). Otherwise, this creates a large amount of previously unnecessary work to track and retrigger transfers that were mistakenly canceled due to multiple check-ins.
Comment 3 Nick Clemens 2022-01-04 19:26:53 UTC
Created attachment 129008 [details] [review]
Bug 29792: Automatically send wrong transfers

Bug 24434 reinstated the 'updateWrongTransfer' call here, but made the assertion that
the transfer should not be sent until 'Ok' was clicked.

I see how unsent transfers are useful for rotation, but I believe for wrong trasnfer we shouldn't require a confirm or print. If a library does want to, the 'transfersblockcirc' system preference will enforce clicking the button
to continue

To test:
1 - have an item in transit from Branch A to Branch B
2 - check the item in at Branch A
3 - get a Wrong Transfer message, click Print Slip or OK
4 - query the branchtransfers table, see that your original transfer was cancelled and a new one was made, confirm that new transfer has a datesent value
5 - repeat steps 1-3, but instead of clicking one of the buttons click outside of the modal to make it go away
6 - query the branchtransfers table, see that your original transfer was cancelled and a new one was made, confirm that new transfer DOES NOT HAVE a datesent value
7 - Apply patch
8 - Repeat the check in
9 - New transfer is generated and has 'datesent' set
Comment 4 AspenCat Team 2022-01-04 19:30:31 UTC
We have not seen reports of this yet, but I can definitely see this being an issue for our consortium.  There will be times that items do get checked in multiple times, and that would create some of the same havoc that Jason is talking about - Bob Bennhoff
Comment 5 Andrew Fuerste-Henry 2022-01-04 19:35:41 UTC
Created attachment 129009 [details] [review]
Bug 29792: Automatically send wrong transfers

Bug 24434 reinstated the 'updateWrongTransfer' call here, but made the assertion that
the transfer should not be sent until 'Ok' was clicked.

I see how unsent transfers are useful for rotation, but I believe for wrong trasnfer we shouldn't require a confirm or print. If a library does want to, the 'transfersblockcirc' system preference will enforce clicking the button
to continue

To test:
1 - have an item in transit from Branch A to Branch B
2 - check the item in at Branch A
3 - get a Wrong Transfer message, click Print Slip or OK
4 - query the branchtransfers table, see that your original transfer was cancelled and a new one was made, confirm that new transfer has a datesent value
5 - repeat steps 1-3, but instead of clicking one of the buttons click outside of the modal to make it go away
6 - query the branchtransfers table, see that your original transfer was cancelled and a new one was made, confirm that new transfer DOES NOT HAVE a datesent value
7 - Apply patch
8 - Repeat the check in
9 - New transfer is generated and has 'datesent' set

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 6 Martin Renvoize 2022-01-07 20:37:39 UTC
Leaving a note to self for when I QA properly next week.

Why are we changing the signature for request_transfer here?  The idea was to keep the methods singularly focused with defined exceptions.

In short, shouldn't we be chaining the returned trans ft er request with a call to transit...
Comment 7 Martin Renvoize 2022-01-31 15:35:07 UTC
Right.. so the idea of the transfers work was to try and remove the quasi-state of not knowing if an item is actually in transit or not... thus we have 'requested' and 'sent' dates to distinguish.  If we go back to defaulting to 'sent' in this case, we're going back to not actually knowing if the item has actually been sent or not.

Also, from an end-user perspective, the general rule for a modal is to 'fail-safe'.  i.e if you ignore a modal then the 'close' action is used by default.. in this case, that is 'do nothing' i.e. don't trigger a cancellation, but also don't trigger to say we've actually loaded the item into the van.

We could change the default action to for WrongTransfer to immediately call 'transit' to put the item into transit (set the sent date), we could do the same in circ/returns.pl or we could do it in the template with javascript to automatically trigger the 'ok' action of the modal if one clicks away.

But, in all those cases we're ended up with an unknown that some libraries would prefer to avoid.  Perhaps we could tie it to another system preference, 'AutomaticWrongTransfer' such that the default of the modal is 'OK' if closed without clicking a button?
Comment 8 Andrew Fuerste-Henry 2022-01-31 15:43:24 UTC
> But, in all those cases we're ended up with an unknown that some libraries
> would prefer to avoid.  Perhaps we could tie it to another system
> preference, 'AutomaticWrongTransfer' such that the default of the modal is
> 'OK' if closed without clicking a button?

A syspref seems like a good answer. We clearly have two strong preferences among users about what assumption Koha should make when this modal dismissed.
Comment 9 Martin Renvoize 2022-01-31 16:08:18 UTC
OK, I'll try to add that alternative patch later today.. got a couple of meetings first so bear with ;)
Comment 10 Katrin Fischer 2022-03-27 00:44:59 UTC
(In reply to Martin Renvoize from comment #9)
> OK, I'll try to add that alternative patch later today.. got a couple of
> meetings first so bear with ;)

Martin, do you still plan to provide an alternative patch or should we move on with what we have here?

As there seems to be a need for a pref, I'll set to failed QA for now.
Comment 11 Nick Clemens 2022-09-30 11:34:46 UTC
Created attachment 141124 [details] [review]
Bug 29792: Automatically send wrong transfers

Bug 24434 reinstated the 'updateWrongTransfer' call here, but made the assertion that
the transfer should not be sent until 'Ok' was clicked.

I see how unsent transfers are useful for rotation, but I believe for wrong transfer we shouldn't require a confirm or print. If a library does want to, the 'transfersblockcirc' system preference will enforce clicking the button
to continue

This patch also confirms a transfer when item already has a transfer and only options are Confirm/Print because the transfer already exists

To test:
1 - Disable TransfersBlockCirc system preference
2 - Manually transfer an item from branch A to branch B
3 - Check in the item at branch A
4 - Dismiss modal without confirming
5 - View item on biblio details page, item not in transit
6 - Check in item, confirm transfer
7 - Note item in transit on details page
8 - Check in the item at branch C
9 - Dismiss modal without confirm
10 - View item on biblio detail, not in transit
11 - Checkin at A, cancel transfer
12 - Apply patch
13 - repeat 1 - 4
14 - Confirm item appears in transit
15 - Check in at branch C, dismiss modal
16 - Confirm item in transit
Comment 12 Nick Clemens 2022-09-30 11:39:58 UTC
Amended test plan - after manually transferring, check-in and ignore wrong transfer modal - next check-in generates the 'item-transfer-modal'
Comment 13 Martin Renvoize 2022-10-10 15:27:36 UTC
Created attachment 141576 [details] [review]
Bug 29792: Automatically send wrong transfers

Bug 24434 reinstated the 'updateWrongTransfer' call here, but made the assertion that
the transfer should not be sent until 'Ok' was clicked.

I see how unsent transfers are useful for rotation, but I believe for wrong transfer we shouldn't require a confirm or print. If a library does want to, the 'transfersblockcirc' system preference will enforce clicking the button
to continue

This patch also confirms a transfer when item already has a transfer and only options are Confirm/Print because the transfer already exists

To test:
1 - Disable TransfersBlockCirc system preference
2 - Manually transfer an item from branch A to branch B
3 - Check in the item at branch A
4 - Dismiss modal without confirming
5 - View item on biblio details page, item not in transit
6 - Check in item, confirm transfer
7 - Note item in transit on details page
8 - Check in the item at branch C
9 - Dismiss modal without confirm
10 - View item on biblio detail, not in transit
11 - Checkin at A, cancel transfer
12 - Apply patch
13 - repeat 1 - 4
14 - Confirm item appears in transit
15 - Check in at branch C, dismiss modal
16 - Confirm item in transit

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2022-10-10 15:28:19 UTC
Thanks for taking this over Nick.. looks great and works as described.. 

Signing off.
Comment 15 Katrin Fischer 2022-11-02 22:44:07 UTC
Created attachment 143049 [details] [review]
Bug 29792: Automatically send wrong transfers

Bug 24434 reinstated the 'updateWrongTransfer' call here, but made the assertion that
the transfer should not be sent until 'Ok' was clicked.

I see how unsent transfers are useful for rotation, but I believe for wrong transfer we shouldn't require a confirm or print. If a library does want to, the 'transfersblockcirc' system preference will enforce clicking the button
to continue

This patch also confirms a transfer when item already has a transfer and only options are Confirm/Print because the transfer already exists

To test:
1 - Disable TransfersBlockCirc system preference
2 - Manually transfer an item from branch A to branch B
3 - Check in the item at branch A
4 - Dismiss modal without confirming
5 - View item on biblio details page, item not in transit
6 - Check in item, confirm transfer
7 - Note item in transit on details page
8 - Check in the item at branch C
9 - Dismiss modal without confirm
10 - View item on biblio detail, not in transit
11 - Checkin at A, cancel transfer
12 - Apply patch
13 - repeat 1 - 4
14 - Confirm item appears in transit
15 - Check in at branch C, dismiss modal
16 - Confirm item in transit

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Tomás Cohen Arazi 2022-11-03 12:55:49 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 17 Lucas Gass 2023-02-15 21:10:09 UTC
I would be happy to backport this but it does not apply cleanly to 22.05.x.
Comment 18 Nick Clemens 2023-02-16 12:14:43 UTC
Created attachment 146726 [details] [review]
Bug 29792: [22.05.x] Automatically send wrong transfers

Bug 24434 reinstated the 'updateWrongTransfer' call here, but made the assertion that
the transfer should not be sent until 'Ok' was clicked.

I see how unsent transfers are useful for rotation, but I believe for wrong transfer we shouldn't require a confirm or print. If a library does want to, the 'transfersblockcirc' system preference will enforce clicking the button
to continue

This patch also confirms a transfer when item already has a transfer and only options are Confirm/Print because the transfer already exists

To test:
1 - Disable TransfersBlockCirc system preference
2 - Manually transfer an item from branch A to branch B
3 - Check in the item at branch A
4 - Dismiss modal without confirming
5 - View item on biblio details page, item not in transit
6 - Check in item, confirm transfer
7 - Note item in transit on details page
8 - Check in the item at branch C
9 - Dismiss modal without confirm
10 - View item on biblio detail, not in transit
11 - Checkin at A, cancel transfer
12 - Apply patch
13 - repeat 1 - 4
14 - Confirm item appears in transit
15 - Check in at branch C, dismiss modal
16 - Confirm item in transit

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Lucas Gass 2023-02-16 13:13:54 UTC
Backported to 22.05.x for upcoming 22.05.10
Comment 20 Arthur Suzuki 2023-02-22 22:39:37 UTC
The patch for 22.05.x also applies clean on 21.11.x.
Applied for 21.11.18!
Comment 21 wainuiwitikapark 2023-03-15 02:22:30 UTC
Not backported to 21.05.x