|
Description
Lari Taskula
2023-08-29 09:42:46 UTC
Created attachment 154895 [details] [review] Bug 34639: (DO NOT PUSH) 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/Transfer.t Created attachment 154896 [details] [review] Bug 34639: (DO NOT PUSH) 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/Transfer.t 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. Created attachment 154900 [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 Created attachment 154917 [details] [review] Bug 34639: Add tests To test: 1. prove t/db_dependent/Koha/Item.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! Created attachment 154918 [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 154919 [details] [review] Bug 34639: Restore order_by To test: 1. prove t/db_dependent/Koha/Item.t 2. Observe success 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. *** Created attachment 154939 [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 154940 [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 154941 [details] [review] Bug 34639: Restore order_by To test: 1. prove t/db_dependent/Koha/Item.t 2. Observe success 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! |