Bug 24434 - C4::Circulation::updateWrongTransfer is never called but should be
Summary: C4::Circulation::updateWrongTransfer is never called but should be
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 21346
Blocks: 28294 28520
  Show dependency treegraph
 
Reported: 2020-01-16 15:17 UTC by Martin Renvoize
Modified: 2021-06-21 13:05 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.01


Attachments
Bug 24434: Remove updateWrongTransfer (2.49 KB, patch)
2020-01-16 15:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Remove updateWrongTransfer (2.56 KB, patch)
2020-01-17 18:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24434: Remove updateWrongTransfer (2.60 KB, patch)
2020-02-29 20:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24434: Reinstate updateWrongTransfer (7.51 KB, patch)
2021-05-06 13:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Reinstate updateWrongTransfer (7.51 KB, patch)
2021-05-06 13:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Add 'WrongTransfer' to cancellation_reasons (3.04 KB, patch)
2021-05-06 13:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Reinstate updateWrongTransfer (6.64 KB, patch)
2021-05-07 06:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Add 'WrongTransfer' to cancellation_reasons (3.04 KB, patch)
2021-05-07 06:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Reinstate updateWrongTransfer (14.71 KB, patch)
2021-05-07 11:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Add 'WrongTransfer' to cancellation_reasons (3.04 KB, patch)
2021-05-07 11:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Add Unit Tests for relations (2.46 KB, patch)
2021-05-10 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Unit tests for updateWrongTransfer (2.76 KB, patch)
2021-05-10 09:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Reinstate updateWrongTransfer (14.71 KB, patch)
2021-05-17 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Add 'WrongTransfer' to cancellation_reasons (3.04 KB, patch)
2021-05-17 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Add Unit Tests for relations (2.46 KB, patch)
2021-05-17 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Unit tests for updateWrongTransfer (2.76 KB, patch)
2021-05-17 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Submit on print slip (2.51 KB, patch)
2021-05-17 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Submit on print slip (4.08 KB, patch)
2021-05-17 10:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Reinstate updateWrongTransfer (14.71 KB, patch)
2021-05-17 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Add 'WrongTransfer' to cancellation_reasons (3.04 KB, patch)
2021-05-17 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Add Unit Tests for relations (2.46 KB, patch)
2021-05-17 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Unit tests for updateWrongTransfer (2.78 KB, patch)
2021-05-17 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Submit on print slip (4.08 KB, patch)
2021-05-17 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Reinstate updateWrongTransfer (14.84 KB, patch)
2021-05-17 21:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24434: Add 'WrongTransfer' to cancellation_reasons (3.09 KB, patch)
2021-05-17 21:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24434: Add Unit Tests for relations (2.51 KB, patch)
2021-05-17 21:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24434: Unit tests for updateWrongTransfer (2.84 KB, patch)
2021-05-17 21:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24434: Submit on print slip (4.14 KB, patch)
2021-05-17 21:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24434: Add 'WrongTransfer' to cancellation_reasons (3.13 KB, patch)
2021-05-31 14:41 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24434: Reinstate updateWrongTransfer (14.84 KB, patch)
2021-06-08 08:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Add Unit Tests for relations (2.51 KB, patch)
2021-06-08 08:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Unit tests for updateWrongTransfer (2.84 KB, patch)
2021-06-08 08:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Submit on print slip (4.14 KB, patch)
2021-06-08 08:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Add 'WrongTransfer' to cancellation_reasons (3.13 KB, patch)
2021-06-08 08:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: DBIC Update (1.62 KB, patch)
2021-06-08 08:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24434: Reinstate updateWrongTransfer (14.92 KB, patch)
2021-06-10 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24434: Add Unit Tests for relations (2.58 KB, patch)
2021-06-10 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24434: Unit tests for updateWrongTransfer (2.91 KB, patch)
2021-06-10 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24434: Submit on print slip (4.21 KB, patch)
2021-06-10 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24434: Add 'WrongTransfer' to cancellation_reasons (3.20 KB, patch)
2021-06-10 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24434: DBIC Update (1.69 KB, patch)
2021-06-10 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24434: (QA follow-up) Remove tab character [squashed] (993 bytes, patch)
2021-06-10 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-01-16 15:17:54 UTC Comment hidden (obsolete)
Comment 1 Martin Renvoize 2020-01-16 15:33:40 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2020-01-17 18:21:05 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2020-01-23 11:19:07 UTC
WT-itemNumber, WT-waitingAt and WT-From have been removed from returns.tt by:
  commit 5ed171a45c46f63b18266ad3a3d0a978719ca285
  Bug 21346: Convert dialogs to modals.
Comment 4 Jonathan Druart 2020-01-23 11:26:45 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2020-02-03 15:03:13 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2020-02-26 14:28:45 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2020-02-29 20:46:02 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2020-03-02 10:26:09 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2020-03-02 10:30:27 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2020-03-02 11:45:01 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2021-03-15 15:26:03 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2021-05-06 11:49:24 UTC
OK, I've now discovered the issue here.. or rather a customer whose recently upgraded has.

Test plan
1/ Check an item out from it's home library
2/ Check the item in at another library (with the return to home branch system preferences enabled)
2a/ Accept the transfer and note we now have a transfer present from the items check-in library to it's home library
3/ Check the item in at a third library
3a/ Note you are asked to return the item to it's home library (but the fact this is a WrongTransfer is not highlighted in any way)
4/ Go to the item record and note the holding library has been updated to reflect where the item was most recently checked in.
5/ Note, however, that the status continues to display that the item is in transit from the original check-in library to the home library.. this should have been updated to reflect that the item is now in transit from the new holding library to the home library.

I'll work on clarifying the actions here.
Comment 13 Martin Renvoize 2021-05-06 13:01:16 UTC
Created attachment 120625 [details] [review]
Bug 24434: Reinstate updateWrongTransfer

This patch re-instates the call to updateWrongTransfer to ensure the
transfer line it updated to reflect the new frombranch.

Test plan
1/ Check an item out from it's home library
2/ Check the item in at another library (with the return to home branch
system preferences enabled)
2a/ Accept the transfer and note we now have a transfer present from the
items check-in library to it's home library
3/ Check the item in at a third library
3a/ Note you are asked to return the item to it's home library.
3b/ With the patch applied, the modal title should highlight that a
'Wrong transfer' was detected.
4/ Go to the item record and note the holding library has been updated
to reflect where the item was most recently checked in.
4a/ With the patch applied the item status should reflect the last
checked in branch as the 'from' branch of the transfer.
Comment 14 Martin Renvoize 2021-05-06 13:17:29 UTC
Created attachment 120631 [details] [review]
Bug 24434: Reinstate updateWrongTransfer

This patch re-instates the call to updateWrongTransfer to ensure the
transfer line it updated to reflect the new frombranch.

Test plan
1/ Check an item out from it's home library
2/ Check the item in at another library (with the return to home branch
system preferences enabled)
2a/ Accept the transfer and note we now have a transfer present from the
items check-in library to it's home library
3/ Check the item in at a third library
3a/ Note you are asked to return the item to it's home library.
3b/ With the patch applied, the modal title should highlight that a
'Wrong transfer' was detected.
4/ Go to the item record and note the holding library has been updated
to reflect where the item was most recently checked in.
4a/ With the patch applied the item status should reflect the last
checked in branch as the 'from' branch of the transfer.
Comment 15 Martin Renvoize 2021-05-06 13:17:33 UTC
Created attachment 120632 [details] [review]
Bug 24434: Add 'WrongTransfer' to cancellation_reasons
Comment 16 Sally 2021-05-06 15:05:19 UTC
Hi Martin,

This works if I use the 'OK' button, but it fails if I use the 'Print transfer slip' button.

As the item is moving between libraries, we'd always print a transfer slip off for the book.
Comment 17 Martin Renvoize 2021-05-07 06:56:26 UTC
Created attachment 120657 [details] [review]
Bug 24434: Reinstate updateWrongTransfer

This patch re-instates the call to updateWrongTransfer to ensure the
transfer line it updated to reflect the new frombranch.

Test plan
1/ Check an item out from it's home library
2/ Check the item in at another library (with the return to home branch
system preferences enabled)
2a/ Accept the transfer and note we now have a transfer present from the
items check-in library to it's home library
3/ Check the item in at a third library
3a/ Note you are asked to return the item to it's home library.
3b/ With the patch applied, the modal title should highlight that a
'Wrong transfer' was detected.
4/ Go to the item record and note the holding library has been updated
to reflect where the item was most recently checked in.
4a/ With the patch applied the item status should reflect the last
checked in branch as the 'from' branch of the transfer.
5/ Work through the above again, but use the 'Print slip' option for
accepting the transfer and confirm that also works.
Comment 18 Martin Renvoize 2021-05-07 06:56:30 UTC
Created attachment 120658 [details] [review]
Bug 24434: Add 'WrongTransfer' to cancellation_reasons
Comment 19 Martin Renvoize 2021-05-07 06:57:35 UTC
(In reply to Sally from comment #16)
> Hi Martin,
> 
> This works if I use the 'OK' button, but it fails if I use the 'Print
> transfer slip' button.
> 
> As the item is moving between libraries, we'd always print a transfer slip
> off for the book.

Amended patch attached, thanks for catching that Sally. It should now work as expected for both the 'OK' and 'Print slip' options for accepting the transfer.
Comment 20 Martin Renvoize 2021-05-07 07:34:24 UTC
Hold off on testing.. I think I've found a bug in the process linked to the 'Cancel' option now... working on a followup
Comment 21 Martin Renvoize 2021-05-07 11:32:31 UTC
Created attachment 120683 [details] [review]
Bug 24434: Reinstate updateWrongTransfer

This patch re-instates the call to updateWrongTransfer to ensure the
transfer line is updated to reflect the new frombranch.

Test plan
1/ Check an item out from it's home library
2/ Check the item in at another library (with the return to home branch
system preferences enabled)
2a/ Accept the transfer and note we now have a transfer present from the
items check-in library to it's home library
3/ Check the item in at a third library
3a/ Note you are asked to return the item to it's home library.
3b/ With the patch applied, the modal title should highlight that a
'Wrong transfer' was detected.
4/ Go to the item record and note the holding library has been updated
to reflect where the item was most recently checked in.
4a/ With the patch applied the item status should reflect the last
checked in branch as the 'from' branch of the transfer.
5/ Work through the above again, but use the 'Print slip' option for
accepting the transfer and confirm that also works.
6/ Work through the above again, but use the 'Cancel' option and check
that this results in the item staying at it's current location and the
final transfer having been cancelled.
Comment 22 Martin Renvoize 2021-05-07 11:32:35 UTC
Created attachment 120684 [details] [review]
Bug 24434: Add 'WrongTransfer' to cancellation_reasons
Comment 23 Martin Renvoize 2021-05-07 11:33:14 UTC
Well.. that turned out to be rather involved!
Comment 24 Jonathan Druart 2021-05-07 12:27:24 UTC
trivial unit test is missing for ->to_branch ;)
Comment 25 Martin Renvoize 2021-05-10 08:14:31 UTC
Created attachment 120757 [details] [review]
Bug 24434: Add Unit Tests for relations

This patch adds unit tests for the newly introduced to_library (and also
add the missing test for the existing from_library).
Comment 26 Martin Renvoize 2021-05-10 09:12:41 UTC
Created attachment 120758 [details] [review]
Bug 24434: Unit tests for updateWrongTransfer

Add unit tests to cover updateWrongTransfer
Comment 27 Nick Clemens 2021-05-12 10:44:49 UTC
When I check the item in at the third library - if I select 'Print slip' there is no transfer generated


Also, I ran through the test plan without the patches, left the last item with the cancelled transfer, but it seemed to have generated an additional transfer:
MariaDB [koha_kohadev]> select * from branchtransfers where itemnumber=975;
+-------------------+------------+---------------------+---------------------+------------+-------------+---------------------+----------+----------+----------------------+---------------------+
| branchtransfer_id | itemnumber | daterequested       | datesent            | frombranch | datearrived | datecancelled       | tobranch | comments | reason               | cancellation_reason |
+-------------------+------------+---------------------+---------------------+------------+-------------+---------------------+----------+----------+----------------------+---------------------+
|                 4 |        975 | 2021-05-12 10:31:32 | 2021-05-12 10:31:32 | FFL        | NULL        | 2021-05-12 10:31:45 | CPL      | NULL     | ReturnToHome         | Manual              |
|                 5 |        975 | 2021-05-12 10:31:45 | NULL                | FPL        | NULL        | NULL                | FFL      | NULL     | TransferCancellation | NULL                |
+-------------------+------------+---------------------+---------------------+------------+-------------+---------------------+----------+----------+----------------------+---------------------+


This doesn't happen after the patches, but the only way I could clear that second transfer was to check the item in at FFL - checking in at CPL didn't offer option to cancel
Comment 28 Martin Renvoize 2021-05-17 09:44:32 UTC
Created attachment 121029 [details] [review]
Bug 24434: Reinstate updateWrongTransfer

This patch re-instates the call to updateWrongTransfer to ensure the
transfer line is updated to reflect the new frombranch.

Test plan
1/ Check an item out from it's home library
2/ Check the item in at another library (with the return to home branch
system preferences enabled)
2a/ Accept the transfer and note we now have a transfer present from the
items check-in library to it's home library
3/ Check the item in at a third library
3a/ Note you are asked to return the item to it's home library.
3b/ With the patch applied, the modal title should highlight that a
'Wrong transfer' was detected.
4/ Go to the item record and note the holding library has been updated
to reflect where the item was most recently checked in.
4a/ With the patch applied the item status should reflect the last
checked in branch as the 'from' branch of the transfer.
5/ Work through the above again, but use the 'Print slip' option for
accepting the transfer and confirm that also works.
6/ Work through the above again, but use the 'Cancel' option and check
that this results in the item staying at it's current location and the
final transfer having been cancelled.
Comment 29 Martin Renvoize 2021-05-17 09:44:37 UTC
Created attachment 121030 [details] [review]
Bug 24434: Add 'WrongTransfer' to cancellation_reasons
Comment 30 Martin Renvoize 2021-05-17 09:44:41 UTC
Created attachment 121031 [details] [review]
Bug 24434: Add Unit Tests for relations

This patch adds unit tests for the newly introduced to_library (and also
add the missing test for the existing from_library).
Comment 31 Martin Renvoize 2021-05-17 09:44:45 UTC
Created attachment 121032 [details] [review]
Bug 24434: Unit tests for updateWrongTransfer

Add unit tests to cover updateWrongTransfer
Comment 32 Martin Renvoize 2021-05-17 09:44:49 UTC
Created attachment 121033 [details] [review]
Bug 24434: Submit on print slip

This patch ensures that the print slip option on the WrongTransfer modal
also submits the form and thus triggers the actual transfer of the item.
Comment 33 Martin Renvoize 2021-05-17 09:45:38 UTC
Seems I had the last piece that wasn't committed.. I've added it as a final patch.
Comment 34 Martin Renvoize 2021-05-17 10:03:35 UTC
Created attachment 121034 [details] [review]
Bug 24434: Submit on print slip

This patch ensures that the print slip option on the WrongTransfer modal
also submits the form and thus triggers the actual transfer of the item.

We also prevent a double transfer submission (and thus a cancellation
line appearing in the branchtransfers table) for the case where a
transfer back home is triggered.
Comment 35 Martin Renvoize 2021-05-17 16:20:10 UTC
Created attachment 121057 [details] [review]
Bug 24434: Reinstate updateWrongTransfer

This patch re-instates the call to updateWrongTransfer to ensure the
transfer line is updated to reflect the new frombranch.

Test plan
1/ Check an item out from it's home library
2/ Check the item in at another library (with the return to home branch
system preferences enabled)
2a/ Accept the transfer and note we now have a transfer present from the
items check-in library to it's home library
3/ Check the item in at a third library
3a/ Note you are asked to return the item to it's home library.
3b/ With the patch applied, the modal title should highlight that a
'Wrong transfer' was detected.
4/ Go to the item record and note the holding library has been updated
to reflect where the item was most recently checked in.
4a/ With the patch applied the item status should reflect the last
checked in branch as the 'from' branch of the transfer.
5/ Work through the above again, but use the 'Print slip' option for
accepting the transfer and confirm that also works.
6/ Work through the above again, but use the 'Cancel' option and check
that this results in the item staying at it's current location and the
final transfer having been cancelled.
Comment 36 Martin Renvoize 2021-05-17 16:20:14 UTC
Created attachment 121058 [details] [review]
Bug 24434: Add 'WrongTransfer' to cancellation_reasons
Comment 37 Martin Renvoize 2021-05-17 16:20:18 UTC
Created attachment 121059 [details] [review]
Bug 24434: Add Unit Tests for relations

This patch adds unit tests for the newly introduced to_library (and also
add the missing test for the existing from_library).
Comment 38 Martin Renvoize 2021-05-17 16:20:23 UTC
Created attachment 121060 [details] [review]
Bug 24434: Unit tests for updateWrongTransfer

Add unit tests to cover updateWrongTransfer
Comment 39 Martin Renvoize 2021-05-17 16:20:27 UTC
Created attachment 121061 [details] [review]
Bug 24434: Submit on print slip

This patch ensures that the print slip option on the WrongTransfer modal
also submits the form and thus triggers the actual transfer of the item.

We also prevent a double transfer submission (and thus a cancellation
line appearing in the branchtransfers table) for the case where a
transfer back home is triggered.
Comment 40 Victor Grousset/tuxayo 2021-05-17 21:33:25 UTC
Created attachment 121071 [details] [review]
Bug 24434: Reinstate updateWrongTransfer

This patch re-instates the call to updateWrongTransfer to ensure the
transfer line is updated to reflect the new frombranch.

Test plan
note: when applying the patches, also update the database

1/ Check an item out from it's home library
2/ Check the item in at another library
   (with AutomaticItemReturn system preferences enabled)
2a/ Accept the transfer and note we now have a transfer present from the
    items check-in library to it's home library
3/ Check the item in at a third library
3a/ Note you are asked to return the item to it's home library.
3b/ With the patch applied, the modal title should highlight that a
    'Wrong transfer' was detected.
4/ Go to the item record and note the holding library has been updated
   to reflect where the item was most recently checked in.
4a/ With the patch applied the item status should reflect the last
    checked in branch as the 'from' branch of the transfer.
5/ check-in from a 4th library, but use the 'Print slip' option for
   accepting the transfer and confirm that also works.
6/ check-in from a 5th library, but use the 'Cancel' option and check
   that this results in the item staying at it's current location and the
   final transfer having been cancelled.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 41 Victor Grousset/tuxayo 2021-05-17 21:33:32 UTC
Created attachment 121072 [details] [review]
Bug 24434: Add 'WrongTransfer' to cancellation_reasons

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 42 Victor Grousset/tuxayo 2021-05-17 21:33:40 UTC
Created attachment 121073 [details] [review]
Bug 24434: Add Unit Tests for relations

This patch adds unit tests for the newly introduced to_library (and also
add the missing test for the existing from_library).

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 43 Victor Grousset/tuxayo 2021-05-17 21:33:46 UTC
Created attachment 121074 [details] [review]
Bug 24434: Unit tests for updateWrongTransfer

Add unit tests to cover updateWrongTransfer

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 44 Victor Grousset/tuxayo 2021-05-17 21:33:50 UTC
Created attachment 121075 [details] [review]
Bug 24434: Submit on print slip

This patch ensures that the print slip option on the WrongTransfer modal
also submits the form and thus triggers the actual transfer of the item.

We also prevent a double transfer submission (and thus a cancellation
line appearing in the branchtransfers table) for the case where a
transfer back home is triggered.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 45 Victor Grousset/tuxayo 2021-05-17 21:36:18 UTC
It work! :D

Amended the test plan, here are the changes (listing in case I interpreted something wrong)
the return to home branch system preferences => AutomaticItemReturn

5/ Work through the above again, but use the 'Print slip' option for
accepting the transfer and confirm that also works.
6/ Work through the above again, but use the 'Cancel' option and check
that this results in the item staying at it's current location and the
final transfer having been cancelled.
↓↓↓↓↓↓↓↓↓↓↓↓
5/ check-in from a 4th library, but use the 'Print slip' option for
   accepting the transfer and confirm that also works.
6/ check-in from a 5th library, but use the 'Cancel' option and check
   that this results in the item staying at it's current location and the
   final transfer having been cancelled.

Mentioned the need to updatedatabase when applying patches
Comment 46 Victor Grousset/tuxayo 2021-05-17 22:09:18 UTC
Test notes: run the tests
- apply only the test patches
- prove t/db_dependent/Koha/Item/Transfer.t t/db_dependent/Circulation.t
- it should fail
- apply the other patches
- updatedatabase
- run the tests

Just tried and it works :D
Comment 47 Kyle M Hall 2021-05-28 11:31:24 UTC
Test Plan:
1) Apply these patches
2) Place a hold for pickup at Library B
3) Check in the item to create the transfer
4) Check in at Library C
5) Get the error "The reason parameter is mandatory at /usr/share/perl5/Exception/Class/Base.pm line 88"
Comment 48 Victor Grousset/tuxayo 2021-05-31 14:41:06 UTC
Created attachment 121515 [details] [review]
Bug 24434: Add 'WrongTransfer' to cancellation_reasons

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 49 Victor Grousset/tuxayo 2021-05-31 14:42:18 UTC
Rebased a patch that had a small conflict.
Comment 50 Victor Grousset/tuxayo 2021-05-31 14:42:50 UTC
(In reply to Kyle M Hall from comment #47)
> 5) Get the error "The reason parameter is mandatory at
> /usr/share/perl5/Exception/Class/Base.pm line 88"

I can't reproduce the error. I suspect that's because in 3) I was in the wrong library. Should I be in library B? Or another?
Maybe it's about the holding library of the item. Since it has to trigger and transfer.
Comment 51 Martin Renvoize 2021-06-08 08:26:40 UTC
I can't reproduce this error either..
Comment 52 Martin Renvoize 2021-06-08 08:28:27 UTC
Created attachment 121702 [details] [review]
Bug 24434: Reinstate updateWrongTransfer

This patch re-instates the call to updateWrongTransfer to ensure the
transfer line is updated to reflect the new frombranch.

Test plan
note: when applying the patches, also update the database

1/ Check an item out from it's home library
2/ Check the item in at another library
   (with AutomaticItemReturn system preferences enabled)
2a/ Accept the transfer and note we now have a transfer present from the
    items check-in library to it's home library
3/ Check the item in at a third library
3a/ Note you are asked to return the item to it's home library.
3b/ With the patch applied, the modal title should highlight that a
    'Wrong transfer' was detected.
4/ Go to the item record and note the holding library has been updated
   to reflect where the item was most recently checked in.
4a/ With the patch applied the item status should reflect the last
    checked in branch as the 'from' branch of the transfer.
5/ check-in from a 4th library, but use the 'Print slip' option for
   accepting the transfer and confirm that also works.
6/ check-in from a 5th library, but use the 'Cancel' option and check
   that this results in the item staying at it's current location and the
   final transfer having been cancelled.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 53 Martin Renvoize 2021-06-08 08:28:31 UTC
Created attachment 121703 [details] [review]
Bug 24434: Add Unit Tests for relations

This patch adds unit tests for the newly introduced to_library (and also
add the missing test for the existing from_library).

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 54 Martin Renvoize 2021-06-08 08:28:35 UTC
Created attachment 121704 [details] [review]
Bug 24434: Unit tests for updateWrongTransfer

Add unit tests to cover updateWrongTransfer

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 55 Martin Renvoize 2021-06-08 08:28:40 UTC
Created attachment 121705 [details] [review]
Bug 24434: Submit on print slip

This patch ensures that the print slip option on the WrongTransfer modal
also submits the form and thus triggers the actual transfer of the item.

We also prevent a double transfer submission (and thus a cancellation
line appearing in the branchtransfers table) for the case where a
transfer back home is triggered.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 56 Martin Renvoize 2021-06-08 08:28:44 UTC
Created attachment 121706 [details] [review]
Bug 24434: Add 'WrongTransfer' to cancellation_reasons

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 57 Martin Renvoize 2021-06-08 08:28:49 UTC
Created attachment 121707 [details] [review]
Bug 24434: DBIC Update
Comment 58 Martin Renvoize 2021-06-08 08:29:15 UTC
Minor rebase, and added the DBIC update patch in case that's what Kyle was missing ;)
Comment 59 Kyle M Hall 2021-06-10 16:19:43 UTC
Created attachment 121798 [details] [review]
Bug 24434: Reinstate updateWrongTransfer

This patch re-instates the call to updateWrongTransfer to ensure the
transfer line is updated to reflect the new frombranch.

Test plan
note: when applying the patches, also update the database

1/ Check an item out from it's home library
2/ Check the item in at another library
   (with AutomaticItemReturn system preferences enabled)
2a/ Accept the transfer and note we now have a transfer present from the
    items check-in library to it's home library
3/ Check the item in at a third library
3a/ Note you are asked to return the item to it's home library.
3b/ With the patch applied, the modal title should highlight that a
    'Wrong transfer' was detected.
4/ Go to the item record and note the holding library has been updated
   to reflect where the item was most recently checked in.
4a/ With the patch applied the item status should reflect the last
    checked in branch as the 'from' branch of the transfer.
5/ check-in from a 4th library, but use the 'Print slip' option for
   accepting the transfer and confirm that also works.
6/ check-in from a 5th library, but use the 'Cancel' option and check
   that this results in the item staying at it's current location and the
   final transfer having been cancelled.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 60 Kyle M Hall 2021-06-10 16:20:15 UTC
Created attachment 121799 [details] [review]
Bug 24434: Add Unit Tests for relations

This patch adds unit tests for the newly introduced to_library (and also
add the missing test for the existing from_library).

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 61 Kyle M Hall 2021-06-10 16:20:20 UTC
Created attachment 121800 [details] [review]
Bug 24434: Unit tests for updateWrongTransfer

Add unit tests to cover updateWrongTransfer

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 62 Kyle M Hall 2021-06-10 16:20:25 UTC
Created attachment 121801 [details] [review]
Bug 24434: Submit on print slip

This patch ensures that the print slip option on the WrongTransfer modal
also submits the form and thus triggers the actual transfer of the item.

We also prevent a double transfer submission (and thus a cancellation
line appearing in the branchtransfers table) for the case where a
transfer back home is triggered.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 63 Kyle M Hall 2021-06-10 16:20:30 UTC
Created attachment 121802 [details] [review]
Bug 24434: Add 'WrongTransfer' to cancellation_reasons

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 64 Kyle M Hall 2021-06-10 16:20:34 UTC
Created attachment 121803 [details] [review]
Bug 24434: DBIC Update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 65 Kyle M Hall 2021-06-10 16:20:39 UTC
Created attachment 121804 [details] [review]
Bug 24434: (QA follow-up) Remove tab character [squashed]

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 66 Jonathan Druart 2021-06-14 14:15:18 UTC
This patchset conflicts with bug 28520, I cannot apply both of them.
Can you add a dependency and fix the conflict please?
(adding the same comment there)
Comment 67 Jonathan Druart 2021-06-15 10:00:04 UTC Comment hidden (obsolete)
Comment 68 Jonathan Druart 2021-06-15 13:49:38 UTC
(In reply to Kyle M Hall from comment #65)
> Created attachment 121804 [details] [review] [review]
> Bug 24434: (QA follow-up) Remove tab character
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Patch will be squashed when pushed.
Comment 69 Jonathan Druart 2021-06-15 14:28:40 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 70 Kyle M Hall 2021-06-18 14:44:42 UTC
Pushed to 21.05.x for 21.05.01
Comment 71 Fridolin Somers 2021-06-21 13:05:19 UTC
Does not apply on 20.11.x, maybe because of Bug 26481 or its children.

Please provide rebased patches if needed