Description
Martin Renvoize (ashimema)
2019-12-23 08:38:41 UTC
Created attachment 118242 [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 Created attachment 118243 [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 Created attachment 118244 [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 Created attachment 118245 [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 Created attachment 118246 [details] [review] Bug 24295: Remove GetTransfers from reserve.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 Created attachment 118247 [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 Created attachment 118248 [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. Created attachment 118249 [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 Created attachment 118250 [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 Created attachment 118287 [details] [review] Bug 24295: Replace GetTransfers in t/db_dependent/Circulation.t Replace the call to GetTransfers with the get_transfer method in Koha::Item in Ciculation.t Test plan 1/ Run t/db_dependent/Circulation.t 2/ Confirm it passes 3/ Apply patch 4/ Repeat steps 1-2 Created attachment 118295 [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 Created attachment 118296 [details] [review] Bug 27281: Update LostItem to use Koha::Item[::Transfer] methods This patch updates C4::Circulation::LostItem to use the Koha::Item and Koha::Item::Transfer methods to cancel transfers when an item is marked as lost. Test plan 1/ Confirm t/db_dependant/Circulation.t passes prior to applying the patches 2/ Apply the patch and run updatedatabase.pl 3/ Confirm that t/db_dependant/Circulation.t still passes 4/ Signoff Created attachment 118297 [details] [review] Bug 27281: Add unit test for Koha::Item->get_transfers In the previous patch we introduced the new get_transfers method to the Koha::Item object. This patch adds proper unit tests for that addition. Test plan 1/ Apply patches and confirm t/db_dependent/Koha/Item.t passes. 2/ Signoff Created attachment 118298 [details] [review] Bug 27281: (follow-up) Clean up LostItem test The LostItem test was rather messy, and wasn't actually testing much in reality. This patch cleans it up and adds testing for transfer queue handling of ListItem cancellations. Test plan 1/ Confirm t/db_dependent/Circulation.t still passes 2/ Read the tests before and after and confirm that they test what is required. 3/ Signoff Created attachment 118299 [details] [review] Bug 27896: Remove DeleteTransfer from circ/return.pl This patch replaces the DeleteTransfer call in circ/returns.pl with a call to Koha::Item::Transfer->cancel. Test plan 1/ Check an item out 2/ Add a transfer request for the item to a second library 3/ Attempt to check the item in at the first library 4/ Note that you should be given a 'WrongTransfer' modal and have to option to cancel. 5/ Cancel the transfer 6/ Check in the database that the transfer now has 'datecancelled' set. 7/ Add a transfer for the item again 8/ From the transferstoreceive page cancel the transfer 9/ Click cancel and again check that datecancelled is set in the database for your transfer Created attachment 118300 [details] [review] BUg 27896: Remove DeleteTransfer This patch removes the last mentions of DeleteTransfer Test plan 1/ Grep the codebase for DeleteTransfer 2/ It should no longer exist Created attachment 118301 [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 Created attachment 118302 [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 Created attachment 118303 [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 Created attachment 118304 [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 Created attachment 118305 [details] [review] Bug 24295: Remove GetTransfers from reserve.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 Created attachment 118306 [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 Created attachment 118307 [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. Created attachment 118308 [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 Created attachment 118309 [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 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 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. Created attachment 119854 [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 Created attachment 119855 [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 Created attachment 119856 [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 Created attachment 119857 [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 Created attachment 119858 [details] [review] Bug 24295: Remove GetTransfers from reserve.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 Created attachment 119859 [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 Created attachment 119860 [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. Created attachment 119861 [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 Created attachment 119862 [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 Created attachment 119863 [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 Created attachment 119864 [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 Created attachment 119866 [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 Created attachment 119867 [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 Nice work! Please re-do/rebase some of those. Created attachment 127465 [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 Created attachment 127466 [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 Created attachment 127467 [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 Created attachment 127468 [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 Created attachment 127469 [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 Created attachment 127470 [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 Created attachment 127471 [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. Created attachment 127472 [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 Created attachment 127473 [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 Created attachment 127474 [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 Created attachment 127475 [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 Created attachment 127476 [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 Created attachment 127477 [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.. 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 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. 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 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 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 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 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 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 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. 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 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 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 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 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. 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 (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. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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. *** Bug 30580 has been marked as a duplicate of this bug. *** 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Thanks for the rebase Joonas.. just confirming I'm happy with your updates and my testing shows it's all good. Please QA this one or otherwise I will do it ;) FYI, patch "Bug 24295: Remove GetTransfers from opac-detail.pl" might become obsolete if bug 31313 follow-ups get pushed. Discussed with Tomas and he's happy for it to go PQA given it's had both Joonas's and my eyes on it. :) (In reply to Marcel de Rooy from comment #100) > Please QA this one or otherwise I will do it ;) Miracles happen :) Pushed to master for 22.11. Nice work everyone, thanks! Enhancement won't be backported to the 22.05.x series |