Bug 34639 - Item shown in transit on detail.pl even if marked as arrived or cancelled
Summary: Item shown in transit on detail.pl even if marked as arrived or cancelled
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P1 - high major (vote)
Assignee: Lari Taskula
QA Contact: Aleisha Amohia
URL:
Keywords:
: 34554 (view as bug list)
Depends on: 33497
Blocks: 35100
  Show dependency treegraph
 
Reported: 2023-08-29 09:42 UTC by Lari Taskula
Modified: 2024-01-08 16:18 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04


Attachments
Bug 34639: (DO NOT PUSH) Add tests (2.55 KB, patch)
2023-08-29 09:45 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: (DO NOT PUSH) Add tests (2.69 KB, patch)
2023-08-29 10:00 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: Use coderef for releationship (2.32 KB, patch)
2023-08-29 12:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34639: Add tests (3.18 KB, patch)
2023-08-29 18:57 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: Add tests (3.20 KB, patch)
2023-08-29 19:06 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: Restore order_by (766 bytes, patch)
2023-08-29 19:16 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: Add tests (3.19 KB, patch)
2023-08-30 09:27 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: Use coderef for releationship (2.37 KB, patch)
2023-08-30 09:27 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: Restore order_by (766 bytes, patch)
2023-08-30 09:27 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: Add tests (3.28 KB, patch)
2023-09-01 15:24 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: Use coderef for releationship (2.39 KB, patch)
2023-09-01 15:24 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: Restore order_by (762 bytes, patch)
2023-09-01 15:24 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 34639: Add tests (3.36 KB, patch)
2023-09-05 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34639: Use coderef for releationship (2.47 KB, patch)
2023-09-05 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34639: Restore order_by (838 bytes, patch)
2023-09-05 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34639: Add tests (3.41 KB, patch)
2023-09-05 20:46 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 34639: Use coderef for releationship (2.52 KB, patch)
2023-09-05 20:46 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 34639: Restore order_by (896 bytes, patch)
2023-09-05 20:46 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2023-08-29 09:42:46 UTC
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"
Comment 1 Lari Taskula 2023-08-29 09:45:27 UTC Comment hidden (obsolete)
Comment 2 Lari Taskula 2023-08-29 10:00:10 UTC Comment hidden (obsolete)
Comment 3 Lari Taskula 2023-08-29 10:08:36 UTC
Reproducible after Bug 33497
Comment 4 Lari Taskula 2023-08-29 10:21:46 UTC
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.
Comment 5 Lari Taskula 2023-08-29 10:53:33 UTC
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.
Comment 6 Lari Taskula 2023-08-29 11:33:54 UTC
(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.
Comment 7 Nick Clemens 2023-08-29 12:25:03 UTC Comment hidden (obsolete)
Comment 8 Lari Taskula 2023-08-29 18:57:12 UTC Comment hidden (obsolete)
Comment 9 Lari Taskula 2023-08-29 18:59:22 UTC
(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!
Comment 10 Lari Taskula 2023-08-29 19:06:51 UTC Comment hidden (obsolete)
Comment 11 Lari Taskula 2023-08-29 19:16:14 UTC Comment hidden (obsolete)
Comment 12 Lari Taskula 2023-08-29 19:20:04 UTC
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.
Comment 13 Lari Taskula 2023-08-30 09:14:42 UTC
*** Bug 34554 has been marked as a duplicate of this bug. ***
Comment 14 Lari Taskula 2023-08-30 09:27:01 UTC Comment hidden (obsolete)
Comment 15 Lari Taskula 2023-08-30 09:27:03 UTC Comment hidden (obsolete)
Comment 16 Lari Taskula 2023-08-30 09:27:06 UTC Comment hidden (obsolete)
Comment 17 Owen Leonard 2023-08-31 15:44:40 UTC
This looks like it's working well but the QA script warns about tidiness. Can you please take a look?
Comment 18 Lari Taskula 2023-09-01 15:24:13 UTC
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
Comment 19 Lari Taskula 2023-09-01 15:24:16 UTC
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>
Comment 20 Lari Taskula 2023-09-01 15:24:19 UTC
Created attachment 155129 [details] [review]
Bug 34639: Restore order_by

To test:
1. prove t/db_dependent/Koha/Item.t
2. Observe success
Comment 21 Lari Taskula 2023-09-01 15:28:37 UTC
(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.)
Comment 22 David Nind 2023-09-02 20:14:07 UTC
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.
Comment 23 Jonathan Druart 2023-09-05 08:48:09 UTC
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>
Comment 24 Jonathan Druart 2023-09-05 08:48:11 UTC
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>
Comment 25 Jonathan Druart 2023-09-05 08:48:14 UTC
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>
Comment 26 Aleisha Amohia 2023-09-05 20:46:31 UTC
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>
Comment 27 Aleisha Amohia 2023-09-05 20:46:34 UTC
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>
Comment 28 Aleisha Amohia 2023-09-05 20:46:37 UTC
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>
Comment 29 Tomás Cohen Arazi 2023-09-08 14:54:31 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 30 Fridolin Somers 2023-09-08 20:00:26 UTC
Pushed to 23.05.x for 23.05.04
Comment 31 Matt Blenkinsop 2023-09-15 10:01:34 UTC
Missing dependency for 22.11.x - not backporting

Nice work everyone!