Bug 26057 - Add Koha::Item::Transfer->cancel method
Summary: Add Koha::Item::Transfer->cancel method
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 26481
Blocks: 24446 27281
  Show dependency treegraph
 
Reported: 2020-07-24 10:31 UTC by Martin Renvoize
Modified: 2021-12-13 21:12 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds the `Koha::Item::Transfer->cancel` method to allow audited cancellation of item transfers. This patch introduces the `datecancelled` and `cancellation_reason` fields to branchtransfers for tracking when and why a transfer was cancelled.
Version(s) released in:
21.05.00


Attachments
Bug 26057: Add datecancelled field to branchtransfers (2.07 KB, patch)
2020-09-21 14:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26057: Add 'cancel' method to Koha::Item::Transfer (7.97 KB, patch)
2020-09-21 14:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
A screenshot of the branchtransfers table after some transfers are performed. (29.67 KB, image/png)
2020-10-06 09:55 UTC, Sally
Details
Bug 26057: Add datecancelled, cancellation_reason fields (3.40 KB, patch)
2021-02-09 11:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26057: Add 'cancel' method to Koha::Item::Transfer (10.92 KB, patch)
2021-02-09 11:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26057: (QA follow-up) Drop test for 'reason' (1.46 KB, patch)
2021-02-09 11:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26057: Add datecancelled, cancellation_reason fields (3.40 KB, patch)
2021-03-03 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26057: Add 'cancel' method to Koha::Item::Transfer (10.92 KB, patch)
2021-03-03 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26057: (QA follow-up) Drop test for 'reason' (1.46 KB, patch)
2021-03-03 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26057: (QA follow-up) Fix Indexer tests (2.67 KB, patch)
2021-03-03 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26057: (QA follow-up) Fix Biblios test for datecancellation (1.22 KB, patch)
2021-03-04 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26057: (QA follow-up) Fix Biblios and Reserves tests (1.65 KB, patch)
2021-03-04 08:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26057: (QA follow-up) Fix Biblios, Reserves, Z3950Responder and XSLT tests (4.32 KB, patch)
2021-03-04 08:45 UTC, Martin Renvoize
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-07-24 10:31:48 UTC
C4::Item::ModItemTransfer will silently cancel existing transfers by setting the datearrived and overwriting any data in the comment field with a cancellation note.

For full audit purposes, we should record the 'cancellation', prevent the datarrived from being set and prevent data loss by not overwriting the original comment.
Comment 1 Martin Renvoize 2020-09-21 14:59:21 UTC
Created attachment 110476 [details] [review]
Bug 26057: Add datecancelled field to branchtransfers
Comment 2 Martin Renvoize 2020-09-21 14:59:24 UTC
Created attachment 110477 [details] [review]
Bug 26057: Add 'cancel' method to Koha::Item::Transfer

This patch adds the 'cancel' method to Koha::Item::Transfer which sets
the transfer as cancelled by updating the datecancelled field. We also
update Koha::Item->get_transfer here to accomodate for the new
resolution available for a transfer.

Test plan:
1/ Run the included unit tests additions (t/db_dependent/Koha/Items.t,
t/db_dependent/Koha/Item/Transfer.t
Comment 3 Sally 2020-10-06 09:53:19 UTC
I couldn't get this one to work - the patch applies, and I rebuilt the schema, CSS and restarted services.

I tried two methods:

1.
- Place a hold
- Find item and put it into transit
- Cancel hold and transfer

2.  
- Manually transfer an item to a library
- Manually transfer the same item to a different library

In the branchtransfers table, datecancelled as a field shows up - but it doesn't populate; the datearrived field still populates.

I'll add a screenshot of the branchtransfers table.
Comment 4 Sally 2020-10-06 09:55:07 UTC
Created attachment 111279 [details]
A screenshot of the branchtransfers table after some transfers are performed.
Comment 5 Martin Renvoize 2020-10-06 10:16:52 UTC
This is my mistake I'm afraid.. for not adding a test plan.

This patch only adds the underlying code to handle cancellations but does not update anywhere to call it as yet.

My apologies there Sally.. I'll get back to this tree shortly and add proper test plans to the bugs that expose the functionality properly.
Comment 6 Martin Renvoize 2020-10-06 10:46:12 UTC
Test plan
1/ Run the included tests
Comment 7 Martin Renvoize 2021-02-09 11:08:15 UTC
Created attachment 116554 [details] [review]
Bug 26057: Add datecancelled, cancellation_reason fields

This patch adds the datecancelled and cancellation_reason fields to the
branchtransfers table to record when and why transfers have been
cancelled.

To test
1/ Run the database update
2/ Check the database for the 'datecancelled' and 'cancellation_reason'
fields
3/ Signoff

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 8 Martin Renvoize 2021-02-09 11:08:18 UTC
Created attachment 116555 [details] [review]
Bug 26057: Add 'cancel' method to Koha::Item::Transfer

This patch adds the 'cancel' method to Koha::Item::Transfer which sets
the transfer as cancelled by updating the datecancelled field. We also
update Koha::Item->get_transfer here to accomodate for the new
resolution available for a transfer.

Test plan:
1/ Run the included unit tests additions (t/db_dependent/Koha/Items.t,
t/db_dependent/Koha/Item/Transfer.t

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 9 Martin Renvoize 2021-02-09 11:08:21 UTC
Created attachment 116556 [details] [review]
Bug 26057: (QA follow-up) Drop test for 'reason'

We decided this test was superflous to requirements and as such I've
removed it as requested.

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 10 Jonathan Druart 2021-03-03 14:31:06 UTC
Created attachment 117585 [details] [review]
Bug 26057: Add datecancelled, cancellation_reason fields

This patch adds the datecancelled and cancellation_reason fields to the
branchtransfers table to record when and why transfers have been
cancelled.

To test
1/ Run the database update
2/ Check the database for the 'datecancelled' and 'cancellation_reason'
fields
3/ Signoff

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 11 Jonathan Druart 2021-03-03 14:31:09 UTC
Created attachment 117586 [details] [review]
Bug 26057: Add 'cancel' method to Koha::Item::Transfer

This patch adds the 'cancel' method to Koha::Item::Transfer which sets
the transfer as cancelled by updating the datecancelled field. We also
update Koha::Item->get_transfer here to accomodate for the new
resolution available for a transfer.

Test plan:
1/ Run the included unit tests additions (t/db_dependent/Koha/Items.t,
t/db_dependent/Koha/Item/Transfer.t

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 12 Jonathan Druart 2021-03-03 14:31:12 UTC
Created attachment 117587 [details] [review]
Bug 26057: (QA follow-up) Drop test for 'reason'

We decided this test was superflous to requirements and as such I've
removed it as requested.

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 13 Jonathan Druart 2021-03-03 14:31:16 UTC
Created attachment 117588 [details] [review]
Bug 26057: (QA follow-up) Fix Indexer tests

The addition of cancellationdate meant that our test objects in the
Indexer tests now longer matched expectations and as such the tests
failed.
Comment 14 Jonathan Druart 2021-03-03 14:36:53 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 15 Martin Renvoize 2021-03-04 08:26:01 UTC
Created attachment 117632 [details] [review]
Bug 26057: (QA follow-up) Fix Biblios test for datecancellation

With the introduction of datecancellation we now have two fields to
check to varify if a transfer is 'complete' or not.  I forgot to update
this test at the time to ensure the datecancellation field was not set
when adding the mock transfer.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-03-04 08:34:41 UTC
Created attachment 117633 [details] [review]
Bug 26057: (QA follow-up) Fix Biblios and Reserves tests

With the introduction of datecancellation we now have two fields to
check to varify if a transfer is 'complete' or not.  I forgot to update
these two tests at the time to ensure the datecancellation field was not
set when adding the mock transfer.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2021-03-04 08:45:22 UTC
Created attachment 117635 [details] [review]
Bug 26057: (QA follow-up) Fix Biblios, Reserves, Z3950Responder and XSLT tests

With the introduction of datecancellation we now have two fields to
check to varify if a transfer is 'complete' or not.  I forgot to update
these tests at the time to ensure the datecancellation field was not set
when adding the mock transfer.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Jonathan Druart 2021-03-04 08:52:37 UTC
(In reply to Martin Renvoize from comment #17)
> Created attachment 117635 [details] [review] [review]
> Bug 26057: (QA follow-up) Fix Biblios, Reserves, Z3950Responder and XSLT
> tests
> 
> With the introduction of datecancellation we now have two fields to
> check to varify if a transfer is 'complete' or not.  I forgot to update
> these tests at the time to ensure the datecancellation field was not set
> when adding the mock transfer.
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

follow-up pushed to master
Comment 19 Fridolin Somers 2021-03-04 12:24:36 UTC
Enhancement not pushed to 20.11.x