Bug 24295

Summary: C4::Circulation::GetTransfers should be removed, use Koha::Item->get_transfer instead
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, joonas.kylmala, lucas, m.de.rooy, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Bug Depends on: 12362, 16686    
Bug Blocks: 17677, 22160, 33761    
Attachments: Bug 24295: Remove GetTransfer from Z3950Responder
Bug 24295: Remove GetTransfers from opac-reserve.pl
Bug 24295: Remove GetTransfers from opac-detail.pl
Bug 24295: Remove GetTransfers from opac-basket.pl
Bug 24295: Remove GetTransfers from reserve.pl
Bug 24295: Remove GetTransfers from detail.pl
Bug 24295: Remove GetTransfers from svc/holds
Bug 24295: Replace mock in t/db_dependent/Search.t
Bug 24295: Replace mock in t/db_dependent/Circulation/transferbook.t
Bug 24295: Replace GetTransfers in t/db_dependent/Circulation.t
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm
Bug 27281: Update LostItem to use Koha::Item[::Transfer] methods
Bug 27281: Add unit test for Koha::Item->get_transfers
Bug 27281: (follow-up) Clean up LostItem test
Bug 27896: Remove DeleteTransfer from circ/return.pl
BUg 27896: Remove DeleteTransfer
Bug 24295: Remove GetTransfer from Z3950Responder
Bug 24295: Remove GetTransfers from opac-reserve.pl
Bug 24295: Remove GetTransfers from opac-detail.pl
Bug 24295: Remove GetTransfers from opac-basket.pl
Bug 24295: Remove GetTransfers from reserve.pl
Bug 24295: Remove GetTransfers from detail.pl
Bug 24295: Remove GetTransfers from svc/holds
Bug 24295: Replace mock in t/db_dependent/Search.t
Bug 24295: Replace mock in t/db_dependent/Circulation/transferbook.t
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm
Bug 24295: Remove GetTransfer from Z3950Responder
Bug 24295: Remove GetTransfers from opac-reserve.pl
Bug 24295: Remove GetTransfers from opac-detail.pl
Bug 24295: Remove GetTransfers from opac-basket.pl
Bug 24295: Remove GetTransfers from reserve.pl
Bug 24295: Remove GetTransfers from detail.pl
Bug 24295: Remove GetTransfers from svc/holds
Bug 24295: Replace mock in t/db_dependent/Search.t
Bug 24295: Replace mock in t/db_dependent/Circulation/transferbook.t
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm
Bug 24295: Remove GetTransfers from C4/Search.pm
Bug 24295: Remove GetTransfers from C4/Search.pm
Bug 24295: Finally remove GetTransfers from C4/Circulation
Bug 24295: Remove GetTransfer from Z3950Responder
Bug 24295: Remove GetTransfers from opac-reserve.pl
Bug 24295: Remove GetTransfers from opac-detail.pl
Bug 24295: Remove GetTransfers from opac-basket.pl
Bug 24295: Remove GetTransfers from request.pl
Bug 24295: Remove GetTransfers from detail.pl
Bug 24295: Remove GetTransfers from svc/holds
Bug 24295: Replace mock in t/db_dependent/Search.t
Bug 24295: Replace mock in t/db_dependent/Circulation/transferbook.t
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm
Bug 24295: Remove GetTransfers from C4/Search.pm
Bug 24295: Finally remove GetTransfers from C4/Circulation
Bug 24295: Remove GetTransfers from circ/transferstoreceive.pl
Bug 24295: Remove GetTransfer from Z3950Responder
Bug 24295: Remove GetTransfers from opac-reserve.pl
Bug 24295: Remove GetTransfers from opac-detail.pl
Bug 24295: Remove GetTransfers from opac-basket.pl
Bug 24295: Remove GetTransfers from request.pl
Bug 24295: Remove GetTransfers from detail.pl
Bug 24295: Remove GetTransfers from svc/holds
Bug 24295: Replace mock in t/db_dependent/Search.t
Bug 24295: Replace mock in t/db_dependent/Circulation/transferbook.t
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm
Bug 24295: Remove GetTransfers from C4/Search.pm
Bug 24295: Remove GetTransfers from circ/transferstoreceive.pl
Bug 24295: Finally remove GetTransfers from C4/Circulation
Bug 24295: Remove GetTransfer from Z3950Responder
Bug 24295: Remove GetTransfers from opac-reserve.pl
Bug 24295: Remove GetTransfers from opac-detail.pl
Bug 24295: Remove GetTransfers from opac-basket.pl
Bug 24295: Remove GetTransfers from request.pl
Bug 24295: Remove GetTransfers from detail.pl
Bug 24295: Remove GetTransfers from svc/holds
Bug 24295: Replace mock in t/db_dependent/Search.t
Bug 24295: Replace mock in t/db_dependent/Circulation/transferbook.t
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm
Bug 24295: Remove GetTransfers from C4/Search.pm
Bug 24295: Remove GetTransfers from circ/transferstoreceive.pl
Bug 24295: Finally remove GetTransfers from C4/Circulation
Bug 24295: Remove GetTransfer from Z3950Responder
Bug 24295: Remove GetTransfers from opac-reserve.pl
Bug 24295: Remove GetTransfers from opac-detail.pl
Bug 24295: Remove GetTransfers from opac-basket.pl
Bug 24295: Remove GetTransfers from request.pl
Bug 24295: Remove GetTransfers from detail.pl
Bug 24295: Remove GetTransfers from svc/holds
Bug 24295: Replace mock in t/db_dependent/Search.t
Bug 24295: Replace mock in t/db_dependent/Circulation/transferbook.t
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm
Bug 24295: Remove GetTransfers from C4/Search.pm
Bug 24295: Remove GetTransfers from circ/transferstoreceive.pl
Bug 24295: Finally remove GetTransfers from C4/Circulation

Description Martin Renvoize 2019-12-23 08:38:41 UTC
We've had a get_transfer method in the Koha::Item object which replicates this function. 

We should convert all uses of GetTransfers to use the newer Koha::Object based method and remove the original method.
Comment 1 Martin Renvoize 2021-03-15 16:26:14 UTC Comment hidden (obsolete)
Comment 2 Martin Renvoize 2021-03-15 16:26:17 UTC Comment hidden (obsolete)
Comment 3 Martin Renvoize 2021-03-15 16:26:21 UTC Comment hidden (obsolete)
Comment 4 Martin Renvoize 2021-03-15 16:26:25 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2021-03-15 16:26:28 UTC Comment hidden (obsolete)
Comment 6 Martin Renvoize 2021-03-15 16:26:31 UTC Comment hidden (obsolete)
Comment 7 Martin Renvoize 2021-03-15 16:26:35 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize 2021-03-15 16:37:23 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2021-03-15 16:43:25 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2021-03-16 08:57:29 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2021-03-16 10:56:36 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2021-03-16 11:01:17 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize 2021-03-16 11:01:20 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize 2021-03-16 11:01:23 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize 2021-03-16 11:01:26 UTC Comment hidden (obsolete)
Comment 16 Martin Renvoize 2021-03-16 11:01:29 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize 2021-03-16 11:01:32 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2021-03-16 11:01:35 UTC Comment hidden (obsolete)
Comment 19 Martin Renvoize 2021-03-16 11:01:38 UTC Comment hidden (obsolete)
Comment 20 Martin Renvoize 2021-03-16 11:01:42 UTC Comment hidden (obsolete)
Comment 21 Martin Renvoize 2021-03-16 11:01:45 UTC Comment hidden (obsolete)
Comment 22 Martin Renvoize 2021-03-16 11:01:48 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2021-03-16 11:01:51 UTC Comment hidden (obsolete)
Comment 24 Martin Renvoize 2021-03-16 11:01:54 UTC Comment hidden (obsolete)
Comment 25 Martin Renvoize 2021-03-16 11:01:57 UTC Comment hidden (obsolete)
Comment 26 Martin Renvoize 2021-03-16 11:02:00 UTC
Created attachment 118310 [details] [review]
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm

This patch removes the GetTransfers call from
GetItemsAvailableToFillHoldRequestsForBib instead replacing it with an
inline JOIN in the initial query.

Test plan
1/ Run the holds queue
2/ Check the results
3/ Put one of the items in the holds queue into transit
4/ Run the holds queue again
5/ Check that the results do not contain the item that is in transit
6/ Apply the patch
7/ Run the holds queue again
8/ Check that the results still do not contain the item that is in
transit
Comment 27 Martin Renvoize 2021-04-19 14:11:52 UTC
Moving the below comment from bug 24295:

>QA Query.. are 'get_transfer' and 'get_transfers' the right names for these >accessors..
>
>I feel like perhaps they would be better called
>
>`->next_transfer` and `->queued_transfers` or something along those lines.

When talking around the subject, we decided upon 

`get_active_transfer` and `get_all_transfers` as suitable names.
Comment 28 Martin Renvoize 2021-04-19 15:02:20 UTC Comment hidden (obsolete)
Comment 29 Martin Renvoize 2021-04-19 15:02:24 UTC Comment hidden (obsolete)
Comment 30 Martin Renvoize 2021-04-19 15:02:27 UTC Comment hidden (obsolete)
Comment 31 Martin Renvoize 2021-04-19 15:02:31 UTC Comment hidden (obsolete)
Comment 32 Martin Renvoize 2021-04-19 15:02:35 UTC Comment hidden (obsolete)
Comment 33 Martin Renvoize 2021-04-19 15:02:39 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize 2021-04-19 15:02:43 UTC Comment hidden (obsolete)
Comment 35 Martin Renvoize 2021-04-19 15:02:47 UTC Comment hidden (obsolete)
Comment 36 Martin Renvoize 2021-04-19 15:02:51 UTC Comment hidden (obsolete)
Comment 37 Martin Renvoize 2021-04-19 15:02:55 UTC Comment hidden (obsolete)
Comment 38 Martin Renvoize 2021-04-19 15:03:01 UTC Comment hidden (obsolete)
Comment 39 Martin Renvoize 2021-04-19 15:20:41 UTC Comment hidden (obsolete)
Comment 40 Martin Renvoize 2021-04-19 15:20:45 UTC Comment hidden (obsolete)
Comment 41 Tomás Cohen Arazi 2021-11-08 12:58:13 UTC
Nice work! Please re-do/rebase some of those.
Comment 42 Martin Renvoize 2021-11-09 11:52:43 UTC Comment hidden (obsolete)
Comment 43 Martin Renvoize 2021-11-09 11:52:47 UTC Comment hidden (obsolete)
Comment 44 Martin Renvoize 2021-11-09 11:52:51 UTC Comment hidden (obsolete)
Comment 45 Martin Renvoize 2021-11-09 11:52:55 UTC Comment hidden (obsolete)
Comment 46 Martin Renvoize 2021-11-09 11:52:59 UTC Comment hidden (obsolete)
Comment 47 Martin Renvoize 2021-11-09 11:53:03 UTC Comment hidden (obsolete)
Comment 48 Martin Renvoize 2021-11-09 11:53:07 UTC Comment hidden (obsolete)
Comment 49 Martin Renvoize 2021-11-09 11:53:11 UTC Comment hidden (obsolete)
Comment 50 Martin Renvoize 2021-11-09 11:53:15 UTC Comment hidden (obsolete)
Comment 51 Martin Renvoize 2021-11-09 11:53:19 UTC Comment hidden (obsolete)
Comment 52 Martin Renvoize 2021-11-09 11:53:23 UTC Comment hidden (obsolete)
Comment 53 Martin Renvoize 2021-11-09 11:53:27 UTC Comment hidden (obsolete)
Comment 54 Martin Renvoize 2021-11-09 11:53:32 UTC Comment hidden (obsolete)
Comment 55 Martin Renvoize 2021-11-09 11:55:08 UTC
Rebased.. remarkably, it wasn't too terrible a rebase.. just needed pretty much every patch touching to drop the GetTransfers import introduced with our EXPORT bug.

Back to NSO
Comment 56 Joonas Kylmälä 2021-11-13 15:43:50 UTC
1. Some of the patches are missing check if there is any transfer at all before proceeding to check in_transit status:

+        my $transfer = $item->get_transfer;
+        if ( $transfer->in_transit ) {

This results to such errors as this:

> Can't call method "in_transit" on an undefined value at /kohadevbox/koha/catalogue/detail.pl line 350

2.  The patch "Bug 24295: Finally remove GetTransfers from C4/Circulation" needs to come last in the patch series, so just reorder and reattach the patches.

3. Need to add "use Koha::DateUtils qw( output_pref );" to those patches that use the output_pref function here.

4. "Bug 24295: Remove GetTransfers from detail.pl" removes hours and minutes, please bring them back :)

5. "Bug 24295: Remove GetTransfer from Z3950Responder" now only lists the in_transit transfers, it used to return all transfers previously, even those not yet sent (so in queue). Could you double check on this? I think it is okay and the previous behaviour was buggy as it says they are IN_TRANSIT transfers, not queued ones.
Comment 57 Martin Renvoize 2022-02-10 12:00:52 UTC
Created attachment 130430 [details] [review]
Bug 24295: Remove GetTransfer from Z3950Responder

Replace the GetTrasfer call in Z3950Responder/Session.pm with a call to
the Koha::Item method 'get_transfer'.

Test plan
1/ Configure your system to use the Z3950 responder
2/ Trigger an item transfer for an item
3/ Search for the item via Z3950 and varify the transfer status is set
4/ Apply the patch
5/ Repeat step 3
Comment 58 Martin Renvoize 2022-02-10 12:00:57 UTC
Created attachment 130431 [details] [review]
Bug 24295: Remove GetTransfers from opac-reserve.pl

This patch replaces the call to GetTransfers in opac-reserve with the
get_transfer method available from the Koha::Item object

Test plan
1/ Place a reserve on an item at a different branch
2/ Trigger the transfer of the item
3/ Check the opac-reserves page for the display of the current transfer
status
4/ Apply patch
5/ Repeat step 3
Comment 59 Martin Renvoize 2022-02-10 12:01:01 UTC
Created attachment 130432 [details] [review]
Bug 24295: Remove GetTransfers from opac-detail.pl

This patch replaces the call to GetTransfers in opac-detail with the
get_transfer method available from the Koha::Item object

Test plan
1/ Trigger the transfer of the item
2/ Check the opac-detail page for the display of the current transfer
   status
3/ Apply patch
4/ Repeat step 2
Comment 60 Martin Renvoize 2022-02-10 12:01:06 UTC
Created attachment 130433 [details] [review]
Bug 24295: Remove GetTransfers from opac-basket.pl

This patch replaces the call to GetTransfers in opac-basket with the
get_transfer method available from the Koha::Item object

Test plan
1/ Trigger the transfer of the item
2/ Check the opac-basket page for the display of the current transfer
   status
3/ Apply patch
4/ Repeat step 2
Comment 61 Martin Renvoize 2022-02-10 12:01:10 UTC
Created attachment 130434 [details] [review]
Bug 24295: Remove GetTransfers from request.pl

This patch replaces the call to GetTransfers in reserve.pl with the
get_transfer method available from the Koha::Item object

Test plan
1/ Place a reserve on an item at a different branch
2/ Trigger the transfer of the item
3/ Check the reserves page for the display of the current transfer
   status
4/ Apply patch
5/ Repeat step 3
Comment 62 Martin Renvoize 2022-02-10 12:01:15 UTC
Created attachment 130435 [details] [review]
Bug 24295: Remove GetTransfers from detail.pl

This patch replaces the call to GetTransfers in detail.pl with the
get_transfer method available from the Koha::Item object

Test plan
1/ Trigger the transfer of the item
2/ Check the detail page for the display of the current transfer
   status
3/ Apply patch
4/ Repeat step 2
Comment 63 Martin Renvoize 2022-02-10 12:01:19 UTC
Created attachment 130436 [details] [review]
Bug 24295: Remove GetTransfers from svc/holds

This patch replaces the call to C4::GetTransfers in svc/holds with a
call to the get_transfer method in the Koha::Item object.
Comment 64 Martin Renvoize 2022-02-10 12:01:23 UTC
Created attachment 130437 [details] [review]
Bug 24295: Replace mock in t/db_dependent/Search.t

Replace the mock of GetTransfers in this test with a mock of transfers
data

Test plan
1/ Run t/db_dependent/Search.t
2/ Confirm it passes
3/ Apply patch
4/ Repeat steps 1-2
Comment 65 Martin Renvoize 2022-02-10 12:01:27 UTC
Created attachment 130438 [details] [review]
Bug 24295: Replace mock in t/db_dependent/Circulation/transferbook.t

Replace the call to GetTransfers with the get_transfer method in
Koha::Item in transferbook.t

Test plan
1/ Run t/db_dependent/Circulation/transferbook.t
2/ Confirm it passes
3/ Apply patch
4/ Repeat steps 1-2
Comment 66 Martin Renvoize 2022-02-10 12:01:31 UTC
Created attachment 130439 [details] [review]
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm

This patch removes the GetTransfers call from
GetItemsAvailableToFillHoldRequestsForBib instead replacing it with an
inline JOIN in the initial query.

Test plan
1/ Run the holds queue
2/ Check the results
3/ Put one of the items in the holds queue into transit
4/ Run the holds queue again
5/ Check that the results do not contain the item that is in transit
6/ Apply the patch
7/ Run the holds queue again
8/ Check that the results still do not contain the item that is in
transit
Comment 67 Martin Renvoize 2022-02-10 12:01:36 UTC
Created attachment 130440 [details] [review]
Bug 24295: Remove GetTransfers from C4/Search.pm

This patch removes the final use of GetTransfers from C4::Search.

Test plan
1/ Perform a search that will include results for some items that have
transfers of various states assigned to them
2/ Check the results match expectations (before and after applying the
patch should look the same)
3/ Signoff
Comment 68 Martin Renvoize 2022-02-10 12:01:40 UTC
Created attachment 130441 [details] [review]
Bug 24295: Remove GetTransfers from circ/transferstoreceive.pl

Just drop the import of GetTransfers from circ/transferstoreceive.pl is
it's never actually used.
Comment 69 Martin Renvoize 2022-02-10 12:01:44 UTC
Created attachment 130442 [details] [review]
Bug 24295: Finally remove GetTransfers from C4/Circulation

This patch handles the final removal of GetTransfers from
C4::Circulation.

Test plan
1/ Check that there is no mention of the GetTransfers method codebase
wide now
2/ Run the circulation and transfers tests and check nothing fails..
perhaps even run the full test suit in k-t-d
3/ Signoff
Comment 70 Martin Renvoize 2022-02-10 12:04:31 UTC
(In reply to Joonas Kylmälä from comment #56)

Thanks for the review Joonas, it was really helpful!

> 1. Some of the patches are missing check if there is any transfer at all
> before proceeding to check in_transit status:
> 
> +        my $transfer = $item->get_transfer;
> +        if ( $transfer->in_transit ) {
> 

Great catch, fixed inline in all patches now.
 
> 2.  The patch "Bug 24295: Finally remove GetTransfers from C4/Circulation"
> needs to come last in the patch series, so just reorder and reattach the
> patches.

Done

> 3. Need to add "use Koha::DateUtils qw( output_pref );" to those patches
> that use the output_pref function here.

Another good catch, done inline in all patches now.
 
> 4. "Bug 24295: Remove GetTransfers from detail.pl" removes hours and
> minutes, please bring them back :)

Doh, sorted now.

> 5. "Bug 24295: Remove GetTransfer from Z3950Responder" now only lists the
> in_transit transfers, it used to return all transfers previously, even those
> not yet sent (so in queue). Could you double-check on this? I think it is
> okay and the previous behaviour was buggy as it says they are IN_TRANSIT
> transfers, not queued ones.

I agree with your conclusion here. It took a bit of digging first time around to come to that conclusion but I did the change by design here.
Comment 71 Joonas Kylmälä 2022-08-21 19:27:01 UTC
Created attachment 139546 [details] [review]
Bug 24295: Remove GetTransfer from Z3950Responder

Replace the GetTrasfer call in Z3950Responder/Session.pm with a call to
the Koha::Item method 'get_transfer'.

Test plan
1/ Configure your system to use the Z3950 responder
2/ Trigger an item transfer for an item
3/ Search for the item via Z3950 and varify the transfer status is set
4/ Apply the patch
5/ Repeat step 3

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 72 Joonas Kylmälä 2022-08-21 19:27:06 UTC
Created attachment 139547 [details] [review]
Bug 24295: Remove GetTransfers from opac-reserve.pl

This patch replaces the call to GetTransfers in opac-reserve with the
get_transfer method available from the Koha::Item object

Test plan
1/ Place a reserve on an item at a different branch
2/ Trigger the transfer of the item
3/ Check the opac-reserves page for the display of the current transfer
status
4/ Apply patch
5/ Repeat step 3

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 73 Joonas Kylmälä 2022-08-21 19:27:10 UTC
Created attachment 139548 [details] [review]
Bug 24295: Remove GetTransfers from opac-detail.pl

This patch replaces the call to GetTransfers in opac-detail with the
get_transfer method available from the Koha::Item object

Test plan
1/ Trigger the transfer of the item
2/ Check the opac-detail page for the display of the current transfer
   status
3/ Apply patch
4/ Repeat step 2

JK: Remove unneeded string formatting for transfertwhen as it's done
    on the template toolkit side already with $KohaDates

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 74 Joonas Kylmälä 2022-08-21 19:27:15 UTC
Created attachment 139549 [details] [review]
Bug 24295: Remove GetTransfers from opac-basket.pl

This patch replaces the call to GetTransfers in opac-basket with the
get_transfer method available from the Koha::Item object

Test plan
1/ Trigger the transfer of the item
2/ Check the opac-basket page for the display of the current transfer
   status
3/ Apply patch
4/ Repeat step 2

JK: Remove unneeded string formatting for transfertwhen as it's done
    on the template toolkit side already with $KohaDates. Add missing
    Koha::Items module import.

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 75 Joonas Kylmälä 2022-08-21 19:27:20 UTC
Created attachment 139550 [details] [review]
Bug 24295: Remove GetTransfers from request.pl

This patch replaces the call to GetTransfers in reserve.pl with the
get_transfer method available from the Koha::Item object

Test plan
1/ Place a reserve on an item at a different branch
2/ Trigger the transfer of the item
3/ Check the reserves page for the display of the current transfer
   status
4/ Apply patch
5/ Repeat step 3

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 76 Joonas Kylmälä 2022-08-21 19:27:24 UTC
Created attachment 139551 [details] [review]
Bug 24295: Remove GetTransfers from detail.pl

This patch replaces the call to GetTransfers in detail.pl with the
get_transfer method available from the Koha::Item object

Test plan
1/ Trigger the transfer of the item
2/ Check the detail page for the display of the current transfer
   status
3/ Apply patch
4/ Repeat step 2

JK: Remove unneeded string formatting for transfertwhen as it's done
    on the template toolkit side already with $KohaDates

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 77 Joonas Kylmälä 2022-08-21 19:27:29 UTC
Created attachment 139552 [details] [review]
Bug 24295: Remove GetTransfers from svc/holds

This patch replaces the call to C4::GetTransfers in svc/holds with a
call to the get_transfer method in the Koha::Item object.

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 78 Joonas Kylmälä 2022-08-21 19:27:33 UTC
Created attachment 139553 [details] [review]
Bug 24295: Replace mock in t/db_dependent/Search.t

Replace the mock of GetTransfers in this test with a mock of transfers
data

Test plan
1/ Run t/db_dependent/Search.t
2/ Confirm it passes
3/ Apply patch
4/ Repeat steps 1-2

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 79 Joonas Kylmälä 2022-08-21 19:27:38 UTC
Created attachment 139554 [details] [review]
Bug 24295: Replace mock in t/db_dependent/Circulation/transferbook.t

Replace the call to GetTransfers with the get_transfer method in
Koha::Item in transferbook.t

Test plan
1/ Run t/db_dependent/Circulation/transferbook.t
2/ Confirm it passes
3/ Apply patch
4/ Repeat steps 1-2

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 80 Joonas Kylmälä 2022-08-21 19:27:42 UTC
Created attachment 139555 [details] [review]
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm

This patch removes the GetTransfers call from
GetItemsAvailableToFillHoldRequestsForBib instead replacing it with an
inline JOIN in the initial query.

Test plan
1/ Run the holds queue
2/ Check the results
3/ Put one of the items in the holds queue into transit
4/ Run the holds queue again
5/ Check that the results do not contain the item that is in transit
6/ Apply the patch
7/ Run the holds queue again
8/ Check that the results still do not contain the item that is in
transit

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 81 Joonas Kylmälä 2022-08-21 19:27:48 UTC
Created attachment 139556 [details] [review]
Bug 24295: Remove GetTransfers from C4/Search.pm

This patch removes the final use of GetTransfers from C4::Search.

Test plan
1/ Perform a search that will include results for some items that have
transfers of various states assigned to them
2/ Check the results match expectations (before and after applying the
patch should look the same)
3/ Signoff

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 82 Joonas Kylmälä 2022-08-21 19:27:54 UTC
Created attachment 139557 [details] [review]
Bug 24295: Remove GetTransfers from circ/transferstoreceive.pl

Just drop the import of GetTransfers from circ/transferstoreceive.pl is
it's never actually used.

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 83 Joonas Kylmälä 2022-08-21 19:27:59 UTC
Created attachment 139558 [details] [review]
Bug 24295: Finally remove GetTransfers from C4/Circulation

This patch handles the final removal of GetTransfers from
C4::Circulation.

Test plan
1/ Check that there is no mention of the GetTransfers method codebase
wide now
2/ Run the circulation and transfers tests and check nothing fails..
perhaps even run the full test suit in k-t-d
3/ Signoff

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 84 Joonas Kylmälä 2022-08-21 19:39:19 UTC
Rebased the patches. In addition to the rebase I amended a couple minor fixes (left a comment on those patches): I noticed you had added output_pref() function call in some places but I didn't see it as needed as output_pref() is already called on the Template side via the $KohaDates filter, in fact some of the patches now were conflicting due to output_pref() calls being removed in the scripts in origin/master. So I removed the output_pref calls. Then there was a missing Koha::Items module import which I added.

Signing off.

Note to other testers: currently there is a regression in opac-detail.pl caused by bug 31313, see comment 16 there. This makes it harder to test the code change here as you would have to modify the item-status.inc code to fix the regression in order to be able to see the transfer status this patch touches on.
Comment 85 Joonas Kylmälä 2022-08-21 20:02:53 UTC
*** Bug 30580 has been marked as a duplicate of this bug. ***
Comment 86 Martin Renvoize 2022-08-23 11:22:51 UTC
Created attachment 139655 [details] [review]
Bug 24295: Remove GetTransfer from Z3950Responder

Replace the GetTrasfer call in Z3950Responder/Session.pm with a call to
the Koha::Item method 'get_transfer'.

Test plan
1/ Configure your system to use the Z3950 responder
2/ Trigger an item transfer for an item
3/ Search for the item via Z3950 and varify the transfer status is set
4/ Apply the patch
5/ Repeat step 3

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 87 Martin Renvoize 2022-08-23 11:22:56 UTC
Created attachment 139656 [details] [review]
Bug 24295: Remove GetTransfers from opac-reserve.pl

This patch replaces the call to GetTransfers in opac-reserve with the
get_transfer method available from the Koha::Item object

Test plan
1/ Place a reserve on an item at a different branch
2/ Trigger the transfer of the item
3/ Check the opac-reserves page for the display of the current transfer
status
4/ Apply patch
5/ Repeat step 3

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 88 Martin Renvoize 2022-08-23 11:23:01 UTC
Created attachment 139657 [details] [review]
Bug 24295: Remove GetTransfers from opac-detail.pl

This patch replaces the call to GetTransfers in opac-detail with the
get_transfer method available from the Koha::Item object

Test plan
1/ Trigger the transfer of the item
2/ Check the opac-detail page for the display of the current transfer
   status
3/ Apply patch
4/ Repeat step 2

JK: Remove unneeded string formatting for transfertwhen as it's done
    on the template toolkit side already with $KohaDates

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 89 Martin Renvoize 2022-08-23 11:23:06 UTC
Created attachment 139658 [details] [review]
Bug 24295: Remove GetTransfers from opac-basket.pl

This patch replaces the call to GetTransfers in opac-basket with the
get_transfer method available from the Koha::Item object

Test plan
1/ Trigger the transfer of the item
2/ Check the opac-basket page for the display of the current transfer
   status
3/ Apply patch
4/ Repeat step 2

JK: Remove unneeded string formatting for transfertwhen as it's done
    on the template toolkit side already with $KohaDates. Add missing
    Koha::Items module import.

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 90 Martin Renvoize 2022-08-23 11:23:10 UTC
Created attachment 139659 [details] [review]
Bug 24295: Remove GetTransfers from request.pl

This patch replaces the call to GetTransfers in reserve.pl with the
get_transfer method available from the Koha::Item object

Test plan
1/ Place a reserve on an item at a different branch
2/ Trigger the transfer of the item
3/ Check the reserves page for the display of the current transfer
   status
4/ Apply patch
5/ Repeat step 3

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 91 Martin Renvoize 2022-08-23 11:23:15 UTC
Created attachment 139660 [details] [review]
Bug 24295: Remove GetTransfers from detail.pl

This patch replaces the call to GetTransfers in detail.pl with the
get_transfer method available from the Koha::Item object

Test plan
1/ Trigger the transfer of the item
2/ Check the detail page for the display of the current transfer
   status
3/ Apply patch
4/ Repeat step 2

JK: Remove unneeded string formatting for transfertwhen as it's done
    on the template toolkit side already with $KohaDates

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 92 Martin Renvoize 2022-08-23 11:23:20 UTC
Created attachment 139661 [details] [review]
Bug 24295: Remove GetTransfers from svc/holds

This patch replaces the call to C4::GetTransfers in svc/holds with a
call to the get_transfer method in the Koha::Item object.

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 93 Martin Renvoize 2022-08-23 11:23:26 UTC
Created attachment 139662 [details] [review]
Bug 24295: Replace mock in t/db_dependent/Search.t

Replace the mock of GetTransfers in this test with a mock of transfers
data

Test plan
1/ Run t/db_dependent/Search.t
2/ Confirm it passes
3/ Apply patch
4/ Repeat steps 1-2

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 94 Martin Renvoize 2022-08-23 11:23:30 UTC
Created attachment 139663 [details] [review]
Bug 24295: Replace mock in t/db_dependent/Circulation/transferbook.t

Replace the call to GetTransfers with the get_transfer method in
Koha::Item in transferbook.t

Test plan
1/ Run t/db_dependent/Circulation/transferbook.t
2/ Confirm it passes
3/ Apply patch
4/ Repeat steps 1-2

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 95 Martin Renvoize 2022-08-23 11:23:35 UTC
Created attachment 139664 [details] [review]
Bug 24295: Remove GetTransfers call from C4/HoldsQueue.pm

This patch removes the GetTransfers call from
GetItemsAvailableToFillHoldRequestsForBib instead replacing it with an
inline JOIN in the initial query.

Test plan
1/ Run the holds queue
2/ Check the results
3/ Put one of the items in the holds queue into transit
4/ Run the holds queue again
5/ Check that the results do not contain the item that is in transit
6/ Apply the patch
7/ Run the holds queue again
8/ Check that the results still do not contain the item that is in
transit

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 96 Martin Renvoize 2022-08-23 11:23:39 UTC
Created attachment 139665 [details] [review]
Bug 24295: Remove GetTransfers from C4/Search.pm

This patch removes the final use of GetTransfers from C4::Search.

Test plan
1/ Perform a search that will include results for some items that have
transfers of various states assigned to them
2/ Check the results match expectations (before and after applying the
patch should look the same)
3/ Signoff

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 97 Martin Renvoize 2022-08-23 11:23:44 UTC
Created attachment 139666 [details] [review]
Bug 24295: Remove GetTransfers from circ/transferstoreceive.pl

Just drop the import of GetTransfers from circ/transferstoreceive.pl is
it's never actually used.

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 98 Martin Renvoize 2022-08-23 11:23:51 UTC
Created attachment 139667 [details] [review]
Bug 24295: Finally remove GetTransfers from C4/Circulation

This patch handles the final removal of GetTransfers from
C4::Circulation.

Test plan
1/ Check that there is no mention of the GetTransfers method codebase
wide now
2/ Run the circulation and transfers tests and check nothing fails..
perhaps even run the full test suit in k-t-d
3/ Signoff

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 99 Martin Renvoize 2022-08-23 11:25:19 UTC
Thanks for the rebase Joonas.. just confirming I'm happy with your updates and my testing shows it's all good.
Comment 100 Marcel de Rooy 2022-08-23 14:55:44 UTC
Please QA this one or otherwise I will do it ;)
Comment 101 Joonas Kylmälä 2022-08-23 20:29:28 UTC
FYI, patch "Bug 24295: Remove GetTransfers from opac-detail.pl" might become obsolete if bug 31313 follow-ups get pushed.
Comment 102 Martin Renvoize 2022-08-24 13:22:42 UTC
Discussed with Tomas and he's happy for it to go PQA given it's had both Joonas's and my eyes on it. :)
Comment 103 Marcel de Rooy 2022-08-24 13:35:22 UTC
(In reply to Marcel de Rooy from comment #100)
> Please QA this one or otherwise I will do it ;)

Miracles happen :)
Comment 104 Tomás Cohen Arazi 2022-08-26 19:05:19 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 105 Lucas Gass 2022-10-14 22:18:15 UTC
Enhancement won't be backported to the 22.05.x series