Bug 36018

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 {

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!