Reproducible after Bug 33497

I do not yet understand why but something changed after prefetching current_branchtransfers. It returns a transfer even if it shouldn't when the transfer contains a datetime in datearrived or datecancelled.

Koha/Schema/Result/Item.pm
...
__PACKAGE__->has_many(
  "current_branchtransfers",
  "Koha::Schema::Result::Branchtransfer",
  { 'foreign.itemnumber' => 'self.itemnumber' },
  {
      where => { datearrived => undef, datecancelled => undef },
      order_by => [ { -desc => 'datesent' }, { -asc => 'daterequested' } ]
  }
);

current_branchtransfers by itself works fine but together with the prefetch in detail.pl something gets broken.

The WHERE condition in "current_branchtransfers" is completely ignored when prefetching current transfers.

Tracing the query reveals this:

  $schema->storage->debug(1);
  my $items = Koha::Items->search( { 'me.itemnumber' => $transfer_item->itemnumber }, { prefetch => ['current_branchtransfers'] } );
  warn $items->next;


SELECT `me`.`itemnumber`, `me`.`biblionumber`, `me`.`biblioitemnumber`, `me`.`barcode`, `me`.`dateaccessioned`, `me`.`booksellerid`, `me`.`homebranch`, `me`.`price`, `me`.`replacementprice`, `me`.`replacementpricedate`, `me`.`datelastborrowed`, `me`.`datelastseen`, `me`.`stack`, `me`.`notforloan`, `me`.`damaged`, `me`.`damaged_on`, `me`.`itemlost`, `me`.`itemlost_on`, `me`.`withdrawn`, `me`.`withdrawn_on`, `me`.`itemcallnumber`, `me`.`coded_location_qualifier`, `me`.`issues`, `me`.`renewals`, `me`.`reserves`, `me`.`restricted`, `me`.`itemnotes`, `me`.`itemnotes_nonpublic`, `me`.`holdingbranch`, `me`.`timestamp`, `me`.`deleted_on`, `me`.`location`, `me`.`permanent_location`, `me`.`onloan`, `me`.`cn_source`, `me`.`cn_sort`, `me`.`ccode`, `me`.`materials`, `me`.`uri`, `me`.`itype`, `me`.`more_subfields_xml`, `me`.`enumchron`, `me`.`copynumber`, `me`.`stocknumber`, `me`.`new_status`, `me`.`exclude_from_local_holds_priority`, `current_branchtransfers`.`branchtransfer_id`, `current_branchtransfers`.`itemnumber`, `current_branchtransfers`.`daterequested`, `current_branchtransfers`.`datesent`, `current_branchtransfers`.`frombranch`, `current_branchtransfers`.`datearrived`, `current_branchtransfers`.`datecancelled`, `current_branchtransfers`.`tobranch`, `current_branchtransfers`.`comments`, `current_branchtransfers`.`reason`, `current_branchtransfers`.`cancellation_reason` FROM `items` `me` LEFT JOIN `branchtransfers` `current_branchtransfers` ON `current_branchtransfers`.`itemnumber` = `me`.`itemnumber` WHERE ( `me`.`itemnumber` = ? ) ORDER BY `me`.`itemnumber`: '1857'

So all transfers are fetched instead regardless of their status.

(In reply to Lari Taskula from comment #5)
> The WHERE condition in "current_branchtransfers" is completely ignored when
> prefetching current transfers.

Ah this is surely intentional in detail.pl and we are meant to fetch all items and transfers here.

detail.tt then eventually calls Koha::Item->get_transfer() which should limit to current/active transfers. I still do not understand why it doesn't.

(In reply to Nick Clemens from comment #7)
> Created attachment 154900 [details] [review] [review]
> Bug 34639: Use coderef for releationship
> This does not include an order_by, so I am not sure if it is sufficient

prove t/db_dependent/Koha/Item.t fails with

# Subtest: get_transfers
    1..16
    ok 1 - Koha::Item->get_transfer should return a Koha::Item::Transfers object
    ok 2 - When no transfers exist, the Koha::Item:Transfers object should be empty
    ok 3 - When one transfer has been requested, the Koha::Item:Transfers object should contain one result
    ok 4 - When there are multiple open transfer requests, the Koha::Item::Transfers object contains them all
    ok 5 - Koha::Item->get_transfers returns the oldest transfer request first
    ok 6 - Koha::Item->get_transfers returns the newer transfer request second
    ok 7 - Koha::Item->get_transfers returns the newest transfer request last
    ok 8 - When one transfer is set to in_transit, the Koha::Item::Transfers object still contains them all
    not ok 9 - Koha::Item->get_transfers returns the active transfer request first
    #   Failed test 'Koha::Item->get_transfers returns the active transfer request first'
    #   at t/db_dependent/Koha/Item.t line 1158.
    #          got: '369'
    #     expected: '370'
    not ok 10 - Koha::Item->get_transfers returns the other transfers oldest to newest
    #   Failed test 'Koha::Item->get_transfers returns the other transfers oldest to newest'
    #   at t/db_dependent/Koha/Item.t line 1159.
    #          got: '370'
    #     expected: '369'
    ok 11 - Koha::Item->get_transfers returns the other transfers oldest to newest
    ok 12 - Once a transfer is received, it no longer appears in the list from ->get_transfers()
    ok 13 - Koha::Item->get_transfers returns the other transfers oldest to newest
    ok 14 - Koha::Item->get_transfers returns the other transfers oldest to newest
    ok 15 - Once a transfer is cancelled, it no longer appears in the list from ->get_transfers()
    ok 16 - Koha::Item->get_transfers returns the only transfer that remains
    # Looks like you failed 2 tests of 16.
not ok 18 - get_transfers

Other than that your patch seems to fix the original issue. Awesome!

Adding order_by back the way it was before works just fine. I confirmed this with dumping the SQL trace and it indeed still performs the ORDER BY

SELECT `me`.`branchtransfer_id`, `me`.`itemnumber`, `me`.`daterequested`, `me`.`datesent`, `me`.`frombranch`, `me`.`datearrived`, `me`.`datecancelled`, `me`.`tobranch`, `me`.`comments`, `me`.`reason`, `me`.`cancellation_reason` FROM `items` `item__row`  JOIN `branchtransfers` `me` ON ( `me`.`datearrived` IS NULL AND `me`.`datecancelled` IS NULL AND `me`.`itemnumber` = `item__row`.`itemnumber` ) WHERE ( `item__row`.`itemnumber` = ? ) ORDER BY `datesent` DESC, `daterequested` ASC: '2299

It now looks okay to me.

*** Bug 34554 has been marked as a duplicate of this bug. ***

This looks like it's working well but the QA script warns about tidiness. Can you please take a look?

Created attachment 155127 [details] [review]
Bug 34639: Add tests

To reproduce:

1. Add an item to library A
2. Go to Circulation -> Transfer
3. Transfer the item from library A to another library B
4. Set your currently logged in library to library B
5. Check-in the item
6. Observe message "Item received from A"
7. View the bibliographic record of the item (catalogue/detail.pl)
8. Observe item in "In transit from A to B since xx/xx/xxxx Available"

To test:
1. prove t/db_dependent/Koha/Item.t

Created attachment 155128 [details] [review]
Bug 34639: Use coderef for releationship

This patch switches from a simple relationship to a custom join as
illustrated in the DBIx::Class docs

Reference:
https://metacpan.org/pod/DBIx::Class::Relationship::Base#Custom-join-conditions

This does not include an order_by, so I am not sure if it is sufficient

To test:
1 - On command line:
    export DBIC_TRACE=1;
2 - Save simple script:
use Modern::Perl;
use Koha::Items;

my $items = Koha::Items->search( {'me.itemnumber'=>27}, {prefetch => 'current_branchtransfers'} );
my $item = $items->next;

warn $item->_result->current_branchtransfers->count();
warn $item->get_transfer();
3 - run it
4 - Note that the query doesn't include conditions on the transfers
5 - Apply patch
6 - Repeat
7 - Note that query is correct

Test in staf client:
1. Add an item to library A
2. Go to Circulation -> Transfer
3. Transfer the item from library A to another library B
4. Set your currently logged in library to library B
5. Check-in the item
6. Observe message "Item received from A"
7. View the bibliographic record of the item (catalogue/detail.pl)
8. Observe item in "In transit from A to B since xx/xx/xxxx Available"
9. Apply patch
10. Restart all
11. Reload details - item no longer in transit

Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>

Created attachment 155129 [details] [review]
Bug 34639: Restore order_by

To test:
1. prove t/db_dependent/Koha/Item.t
2. Observe success

(In reply to Owen Leonard from comment #17)
> This looks like it's working well but the QA script warns about tidiness.
> Can you please take a look?

Thanks, wasn't aware of such change. I've now tidied all patches.

(Nick, I took the freedom to squash perltidy fixes to your patch.)

I have tested - for the staff interface it fixes things for the display, and the tests pass.

However, I don't know enough about SQL queries to verify whether the query is correct (step 7 for the command line tests).

So I will leave for someone else to sign off.

Testing notes (using KTD):

1. Saved script in step 2 as test.pl, rang script using: perl test.pl.

Created attachment 155211 [details] [review]
Bug 34639: Add tests

To reproduce:

1. Add an item to library A
2. Go to Circulation -> Transfer
3. Transfer the item from library A to another library B
4. Set your currently logged in library to library B
5. Check-in the item
6. Observe message "Item received from A"
7. View the bibliographic record of the item (catalogue/detail.pl)
8. Observe item in "In transit from A to B since xx/xx/xxxx Available"

To test:
1. prove t/db_dependent/Koha/Item.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 155212 [details] [review]
Bug 34639: Use coderef for releationship

This patch switches from a simple relationship to a custom join as
illustrated in the DBIx::Class docs

Reference:
https://metacpan.org/pod/DBIx::Class::Relationship::Base#Custom-join-conditions

This does not include an order_by, so I am not sure if it is sufficient

To test:
1 - On command line:
    export DBIC_TRACE=1;
2 - Save simple script:
use Modern::Perl;
use Koha::Items;

my $items = Koha::Items->search( {'me.itemnumber'=>27}, {prefetch => 'current_branchtransfers'} );
my $item = $items->next;

warn $item->_result->current_branchtransfers->count();
warn $item->get_transfer();
3 - run it
4 - Note that the query doesn't include conditions on the transfers
5 - Apply patch
6 - Repeat
7 - Note that query is correct

Test in staf client:
1. Add an item to library A
2. Go to Circulation -> Transfer
3. Transfer the item from library A to another library B
4. Set your currently logged in library to library B
5. Check-in the item
6. Observe message "Item received from A"
7. View the bibliographic record of the item (catalogue/detail.pl)
8. Observe item in "In transit from A to B since xx/xx/xxxx Available"
9. Apply patch
10. Restart all
11. Reload details - item no longer in transit

Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 155213 [details] [review]
Bug 34639: Restore order_by

To test:
1. prove t/db_dependent/Koha/Item.t
2. Observe success

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 155236 [details] [review]
Bug 34639: Add tests

To reproduce:

1. Add an item to library A
2. Go to Circulation -> Transfer
3. Transfer the item from library A to another library B
4. Set your currently logged in library to library B
5. Check-in the item
6. Observe message "Item received from A"
7. View the bibliographic record of the item (catalogue/detail.pl)
8. Observe item in "In transit from A to B since xx/xx/xxxx Available"

To test:
1. prove t/db_dependent/Koha/Item.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Created attachment 155237 [details] [review]
Bug 34639: Use coderef for releationship

This patch switches from a simple relationship to a custom join as
illustrated in the DBIx::Class docs

Reference:
https://metacpan.org/pod/DBIx::Class::Relationship::Base#Custom-join-conditions

This does not include an order_by, so I am not sure if it is sufficient

To test:
1 - On command line:
    export DBIC_TRACE=1;
2 - Save simple script:
use Modern::Perl;
use Koha::Items;

my $items = Koha::Items->search( {'me.itemnumber'=>27}, {prefetch => 'current_branchtransfers'} );
my $item = $items->next;

warn $item->_result->current_branchtransfers->count();
warn $item->get_transfer();
3 - run it
4 - Note that the query doesn't include conditions on the transfers
5 - Apply patch
6 - Repeat
7 - Note that query is correct

Test in staf client:
1. Add an item to library A
2. Go to Circulation -> Transfer
3. Transfer the item from library A to another library B
4. Set your currently logged in library to library B
5. Check-in the item
6. Observe message "Item received from A"
7. View the bibliographic record of the item (catalogue/detail.pl)
8. Observe item in "In transit from A to B since xx/xx/xxxx Available"
9. Apply patch
10. Restart all
11. Reload details - item no longer in transit

Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Created attachment 155238 [details] [review]
Bug 34639: Restore order_by

To test:
1. prove t/db_dependent/Koha/Item.t
2. Observe success

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Pushed to master for 23.11.

Nice work everyone, thanks!

Pushed to 23.05.x for 23.05.04

Missing dependency for 22.11.x - not backporting

Nice work everyone!