See also bug 24430 and bug 35994. Current should be: not cancelled. Active should be: not cancelled and not completed. At least this should be the meaning in both modules and their methods. Since active and current is still quite close, perhaps we should even think about an alternative for current. We mean not cancelled, and they could be complete.
Hmm. On the other hand we quite often use filter_by_current and there is just one place where we use filter_by_active. So we could go the other way around..
git grep -P 'filter.*_out(?!side)' Clubs.pm:=head3 filter_out_empty Clubs.pm: my $filtered_rs = $clubs_rs->filter_out_empty; Clubs.pm:sub filter_out_empty { Holds.pm:=head3 filter_out_has_cancellation_requests Holds.pm: my $holds_without_cancellation_requests = $holds->filter_out_has_cancellation_requests; Holds.pm:sub filter_out_has_cancellation_requests { Items.pm: $result = $result->filter_out_lost; Items.pm:=head3 filter_out_lost Items.pm: my $filered_items = $items->filter_out_lost; Items.pm:sub filter_out_lost {
If filter_out_ is a pattern with consensus, then we need filter_by_current for current orders (not cancelled or completed) and filter_out_cancelled to get the current ones plus the completed ones.
Created attachment 162318 [details] [review] Bug 36018: Rename orders->filter_by_current Current is a bit misleading here. It means the ones that are not cancelled. We already use the filter_out_etc naming schema. So in harmony with that, we switch it to filter_out_cancelled. Test plan: Run t/db_dependent/Koha/Acquisition/Order.t Run git grep -l '::Orders' | xargs grep filter_by_current * All occurrences should be related to Recalls, not Orders. Go to acqui/basket.pl. Delete a whole basket who has some active and some cancelled lines. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Hmm. We still have an inconsistency in Koha::Biblio. It also has active_orders but it defines them as not cancelled..
Created attachment 162320 [details] [review] Bug 36018: biblio->active_orders should be ->uncancelled_orders Active orders are more than just not cancelled. See filter_by_active in Koha::Acquisition::Orders. They are still in the acq process; we still need to receive items on those orders. When we cancel and want to delete a biblio, we should check for not cancelled orders (broader than active orders as in Orders.pm). Test plan: Git grep active_orders. Run t/db_dependent/Koha/Biblio.t Run t/db_dependent/Koha/Acquisition/Order.t Add two order lines in a basket referring to same biblio. Try to cancel with delete biblio. Should not be possible. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #5) > Hmm. We still have an inconsistency in Koha::Biblio. > It also has active_orders but it defines them as not cancelled.. Main take-away for second patch: Biblio->active_orders is misleading. We are not just looking at order still in acq process, but at all not-cancelled orders. We will only delete a biblio if there is not another uncancelled order in the system for that biblio. So we adhere to the active definition of Koha::Acquisition::Orders->filter_by_active. Active is more than not cancelled. Look at the status and if you received all items. The third patch will make that more explicit even.
Created attachment 162321 [details] [review] Bug 36018: $orders->filter_by_active should check quantityreceived An active order that has no items to receive any more is actually not very active anymore :) Test plan: Run t/db_dependent/Koha/Acquisition/Orders.t Enable OPACAcquisitionDetails. Add order on basket for new biblio. Check that opac-detail does not yet count the item as on order (since status is still new). Close basket. Check opac-detail again. Should count it now. Reopen basket. Cancel line. Check opac-detail again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
The patch no longer applies (sha1/fake ancestor error): git bz apply 36018 Bug 36018 - Improve consistency in Acquisition/Order(s) regarding active/current orders 162318 - Bug 36018: Rename orders->filter_by_current 162320 - Bug 36018: biblio->active_orders should be ->uncancelled_orders 162321 - Bug 36018: $orders->filter_by_active should check quantityreceived Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 36018: Rename orders->filter_by_current Applying: Bug 36018: biblio->active_orders should be ->uncancelled_orders error: sha1 information is lacking or useless (acqui/basket.pl). error: could not build fake ancestor Patch failed at 0001 Bug 36018: biblio->active_orders should be ->uncancelled_orders
Created attachment 163769 [details] [review] Bug 36018: Rename orders->filter_by_current Current is a bit misleading here. It means the ones that are not cancelled. We already use the filter_out_etc naming schema. So in harmony with that, we switch it to filter_out_cancelled. Test plan: Run t/db_dependent/Koha/Acquisition/Order.t Run git grep -l '::Orders' | xargs grep filter_by_current * All occurrences should be related to Recalls, not Orders. Go to acqui/basket.pl. Delete a whole basket who has some active and some cancelled lines. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 163770 [details] [review] Bug 36018: biblio->active_orders should be ->uncancelled_orders Active orders are more than just not cancelled. See filter_by_active in Koha::Acquisition::Orders. They are still in the acq process; we still need to receive items on those orders. When we cancel and want to delete a biblio, we should check for not cancelled orders (broader than active orders as in Orders.pm). Test plan: Git grep active_orders. Run t/db_dependent/Koha/Biblio.t Run t/db_dependent/Koha/Acquisition/Order.t Add two order lines in a basket referring to same biblio. Try to cancel with delete biblio. Should not be possible. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 163771 [details] [review] Bug 36018: $orders->filter_by_active should check quantityreceived An active order that has no items to receive any more is actually not very active anymore :) Test plan: Run t/db_dependent/Koha/Acquisition/Orders.t Enable OPACAcquisitionDetails. Add order on basket for new biblio. Check that opac-detail does not yet count the item as on order (since status is still new). Close basket. Check opac-detail again. Should count it now. Reopen basket. Cancel line. Check opac-detail again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 163772 [details] [review] Bug 36018: (follow-up) Do not rely on assumed correct quantities Now filter_by_active checks quantities, we should ascertain that our test data is correct, not leaving it to random values from TestBuilder. Test plan: Run t/db_dependent/Koha/Acquisition/Order.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to David Nind from comment #9) > The patch no longer applies (sha1/fake ancestor error): Hi David, Thx for testing. I managed them to apply them on current master with auto merge. And still found an issue in a unit test. So added a follow-up as well. Could you retry please? Thanks
Created attachment 164953 [details] [review] Bug 36018: Rename orders->filter_by_current Current is a bit misleading here. It means the ones that are not cancelled. We already use the filter_out_etc naming schema. So in harmony with that, we switch it to filter_out_cancelled. Test plan: Run t/db_dependent/Koha/Acquisition/Order.t Run git grep -l '::Orders' | xargs grep filter_by_current * All occurrences should be related to Recalls, not Orders. Go to acqui/basket.pl. Delete a whole basket who has some active and some cancelled lines. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Janusz Kaczmarek <januszop@gmail.com>
Created attachment 164954 [details] [review] Bug 36018: biblio->active_orders should be ->uncancelled_orders Active orders are more than just not cancelled. See filter_by_active in Koha::Acquisition::Orders. They are still in the acq process; we still need to receive items on those orders. When we cancel and want to delete a biblio, we should check for not cancelled orders (broader than active orders as in Orders.pm). Test plan: Git grep active_orders. Run t/db_dependent/Koha/Biblio.t Run t/db_dependent/Koha/Acquisition/Order.t Add two order lines in a basket referring to same biblio. Try to cancel with delete biblio. Should not be possible. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Janusz Kaczmarek <januszop@gmail.com>
Created attachment 164955 [details] [review] Bug 36018: $orders->filter_by_active should check quantityreceived An active order that has no items to receive any more is actually not very active anymore :) Test plan: Run t/db_dependent/Koha/Acquisition/Orders.t Enable OPACAcquisitionDetails. Add order on basket for new biblio. Check that opac-detail does not yet count the item as on order (since status is still new). Close basket. Check opac-detail again. Should count it now. Reopen basket. Cancel line. Check opac-detail again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Janusz Kaczmarek <januszop@gmail.com>
Created attachment 164956 [details] [review] Bug 36018: (follow-up) Do not rely on assumed correct quantities Now filter_by_active checks quantities, we should ascertain that our test data is correct, not leaving it to random values from TestBuilder. Test plan: Run t/db_dependent/Koha/Acquisition/Order.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Janusz Kaczmarek <januszop@gmail.com>
I tried to test this, but when I try to add an order to my basket, I have an error message To recreate: 1. Go to Acquisitions 2. Click on the Search button next to the Vendor search 3. Click Add to basket next to 'My basket' 4. In the From an existing record search box, type shakespeare and submit 5. Click 'Add order' under the first result 6. In the item field y - Koha item type, choose Books 7. Click Add item 8. In Fund, choose Secondary fund 9. Click 'Save' --> Error message The method Koha::Biblio->uncancelled_orders is not covered by tests! Followed by a lot of text. I was trying to test this part of the first patch > Go to acqui/basket.pl. Delete a whole basket who has some active and some cancelled lines.
(In reply to Caroline Cyr La Rose from comment #19) > I tried to test this, but when I try to add an order to my basket, I have an > error message > > To recreate: > 1. Go to Acquisitions > 2. Click on the Search button next to the Vendor search > 3. Click Add to basket next to 'My basket' > 4. In the From an existing record search box, type shakespeare and submit > 5. Click 'Add order' under the first result > 6. In the item field y - Koha item type, choose Books > 7. Click Add item > 8. In Fund, choose Secondary fund > 9. Click 'Save' > --> Error message > > The method Koha::Biblio->uncancelled_orders is not covered by tests! > > Followed by a lot of text. > > I was trying to test this part of the first patch > > Go to acqui/basket.pl. Delete a whole basket who has some active and some cancelled lines. Thx Caroline. Can replicate this. Will have a look now.
(In reply to Marcel de Rooy from comment #20) > (In reply to Caroline Cyr La Rose from comment #19) > Thx Caroline. Can replicate this. Will have a look now. Actually, I replicated it when I did not refresh plack. And after that, I couldnt anymore. Could you retry? Restart all, incl refreshing plack and try again? Back to SO
(In reply to Janusz Kaczmarek from comment #18) > Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Thanks Janusz
(In reply to Marcel de Rooy from comment #21) > (In reply to Marcel de Rooy from comment #20) > > (In reply to Caroline Cyr La Rose from comment #19) > > Thx Caroline. Can replicate this. Will have a look now. > > Actually, I replicated it when I did not refresh plack. And after that, I > couldnt anymore. Could you retry? Restart all, incl refreshing plack and try > again? > > Back to SO You're right, after restart_all I don't have the error anymore. Sorry about that! I never know when to use reset_all or restart_all (unless it's specified in the test plan) and sometimes (most of the time!) avoid them because they take a while :)
I'm getting an error in the Receive Orders workflow with this patch applied: 1. Have at least one closed basket with an order line (When I reproduced it, I created an order from a new biblio record in KTD's sample basket, then closed the basket) 2. Go to Receive Shipments and add an invoice 3. Proceed to the next step of selecting order(s) to receive When the Pending Orders table tries to load on parcel.pl, a pop-up appears saying there was a 500 error with loading the table. It also produces this error in the API error log: [ERROR] GET /api/v1/acquisitions/orders: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Column 'quantity' in where clause is ambiguous at /kohadevbox/koha/Koha/Objects.pm line 399>> Tested on a totally fresh KTD instance with just this patch applied, because chasing false alarms because we forgot to restart something happens to the best of us :)
Created attachment 165153 [details] [review] Bug 36018: (follow-up) Disambiguate quantity One of the embeds refers to suggestions. This table also has a quantity column. Test plan: Verify that Pending Orders table does load now instead of triggering an internal server error on ambiguous column. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Emily Lamancusa from comment #24) > I'm getting an error in the Receive Orders workflow with this patch applied: > > 1. Have at least one closed basket with an order line > (When I reproduced it, I created an order from a new biblio record in > KTD's sample basket, then closed the basket) > 2. Go to Receive Shipments and add an invoice > 3. Proceed to the next step of selecting order(s) to receive > When the Pending Orders table tries to load on parcel.pl, a pop-up appears > saying there was a 500 error with loading the table. > > It also produces this error in the API error log: > [ERROR] GET /api/v1/acquisitions/orders: unhandled exception > (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI > Exception: DBD::mysql::st execute failed: Column 'quantity' in where clause > is ambiguous at /kohadevbox/koha/Koha/Objects.pm line 399>> > > Tested on a totally fresh KTD instance with just this patch applied, because > chasing false alarms because we forgot to restart something happens to the > best of us :) Good catch! Only visible when the API is pulling additional tables..
Created attachment 166503 [details] [review] Bug 36018: Rename orders->filter_by_current Current is a bit misleading here. It means the ones that are not cancelled. We already use the filter_out_etc naming schema. So in harmony with that, we switch it to filter_out_cancelled. Test plan: Run t/db_dependent/Koha/Acquisition/Order.t Run git grep -l '::Orders' | xargs grep filter_by_current * All occurrences should be related to Recalls, not Orders. Go to acqui/basket.pl. Delete a whole basket who has some active and some cancelled lines. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166504 [details] [review] Bug 36018: biblio->active_orders should be ->uncancelled_orders Active orders are more than just not cancelled. See filter_by_active in Koha::Acquisition::Orders. They are still in the acq process; we still need to receive items on those orders. When we cancel and want to delete a biblio, we should check for not cancelled orders (broader than active orders as in Orders.pm). Test plan: Git grep active_orders. Run t/db_dependent/Koha/Biblio.t Run t/db_dependent/Koha/Acquisition/Order.t Add two order lines in a basket referring to same biblio. Try to cancel with delete biblio. Should not be possible. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166505 [details] [review] Bug 36018: $orders->filter_by_active should check quantityreceived An active order that has no items to receive any more is actually not very active anymore :) Test plan: Run t/db_dependent/Koha/Acquisition/Orders.t Enable OPACAcquisitionDetails. Add order on basket for new biblio. Check that opac-detail does not yet count the item as on order (since status is still new). Close basket. Check opac-detail again. Should count it now. Reopen basket. Cancel line. Check opac-detail again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166506 [details] [review] Bug 36018: (follow-up) Do not rely on assumed correct quantities Now filter_by_active checks quantities, we should ascertain that our test data is correct, not leaving it to random values from TestBuilder. Test plan: Run t/db_dependent/Koha/Acquisition/Order.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166507 [details] [review] Bug 36018: (follow-up) Disambiguate quantity One of the embeds refers to suggestions. This table also has a quantity column. Test plan: Verify that Pending Orders table does load now instead of triggering an internal server error on ambiguous column. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All working well and a clear clarification and improvement. QA scripts passing, tests passing and coverage increased. Passing QA
Pushed for 24.05! Well done everyone, thank you!
t/db_dependent/api/v1/acquisitions_orders.t .. # Failed test 'Only one order is active' # at t/db_dependent/api/v1/acquisitions_orders.t line 137. # got: '0' # expected: '1' # Looks like you failed 1 test of 24. t/db_dependent/api/v1/acquisitions_orders.t .. 1/5 # Failed test 'list() tests' # at t/db_dependent/api/v1/acquisitions_orders.t line 225.
Might be a random failure, it's failing on Koha_Master but passing on Koha_Master_D11 it seems. Easy to recreate locally.
Created attachment 166677 [details] [review] Bug 36018: Prevent random failure from api/v1/acquisitions_orders.t
(In reply to Jonathan Druart from comment #36) > Created attachment 166677 [details] [review] [review] > Bug 36018: Prevent random failure from api/v1/acquisitions_orders.t Thx
Picked the follow-up for aquisition_orders.t for main.
Not backported to 23.11.x
I don't think this changes anything in the manual. Please reopen with more details if I'm wrong!