Bug 33653 - Search for late orders can show received order lines
Summary: Search for late orders can show received order lines
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-05-02 16:07 UTC by Katrin Fischer
Modified: 2023-12-28 20:47 UTC (History)
5 users (show)

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


Attachments
Bug 33653: Never consider received orders as late (3.10 KB, patch)
2023-05-03 13:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33653: Never consider received orders as late (3.15 KB, patch)
2023-05-04 06:19 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 33653: Never consider received orders as late (3.21 KB, patch)
2023-05-04 14:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33653: Never consider received orders as late (3.21 KB, patch)
2023-05-04 18:15 UTC, Hinemoea Viault
Details | Diff | Splinter Review
Bug 33653: Use filter_by_active instead (5.22 KB, patch)
2023-05-09 07:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33653: Never consider received orders as late (3.21 KB, patch)
2023-05-09 09:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33653: Use filter_by_active instead (5.28 KB, patch)
2023-05-09 09:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33653: Use filter_by_active instead (1.79 KB, patch)
2023-05-09 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-05-02 16:07:02 UTC
In some circumstances the list of late orders will show orders that have already been completed/fully received. As their is no indication in the list that they have been received, they might get accidentally claimed. 
Late orders should never include order lines that have been received.

To test:
* Create a basket
* Create a normal order (not from a subscription, no standing order)
* Leave estimated delivery date empty
* Close the basket
* Create a new invoice and receive the order line
* Go to acq > late orders
* Verify the order is not in the list
* Set filter for 'To' date into the future, i.e. 01/01/2025
* Verify the order shows up now
Comment 1 Jonathan Druart 2023-05-03 13:20:09 UTC
Created attachment 150582 [details] [review]
Bug 33653: Never consider received orders as late

We should not list received orders on the late orders page.

Test plan:
* Create a basket
* Create a normal order (not from a subscription, no standing order)
* Leave estimated delivery date empty
* Close the basket
* Create a new invoice and receive the order line
* Go to acq > late orders
* Verify the order is not in the list
* Set filter for 'To' date into the future, i.e. 01/01/2025
=> Without this patch the order shows up
=> With this patch applied the received order does not show up
Comment 2 Jonathan Druart 2023-05-03 13:20:34 UTC
Not sure this is a regression, hence not sure it's a major.
Comment 3 Katrin Fischer 2023-05-03 21:21:24 UTC
Thanks Jonathan! Chose the severity according to embarassment this might cause the library. :)

Holding off for QA later.
Comment 4 Biblibre Sandboxes 2023-05-04 06:19:48 UTC
Created attachment 150624 [details] [review]
Bug 33653: Never consider received orders as late

We should not list received orders on the late orders page.

Test plan:
* Create a basket
* Create a normal order (not from a subscription, no standing order)
* Leave estimated delivery date empty
* Close the basket
* Create a new invoice and receive the order line
* Go to acq > late orders
* Verify the order is not in the list
* Set filter for 'To' date into the future, i.e. 01/01/2025
=> Without this patch the order shows up
=> With this patch applied the received order does not show up

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 5 Michaela Sieber 2023-05-04 06:20:51 UTC
Thanks Jonathan!
Comment 6 Katrin Fischer 2023-05-04 14:23:36 UTC
Created attachment 150669 [details] [review]
Bug 33653: Never consider received orders as late

We should not list received orders on the late orders page.

Test plan:
* Create a basket
* Create a normal order (not from a subscription, no standing order)
* Leave estimated delivery date empty
* Close the basket
* Create a new invoice and receive the order line
* Go to acq > late orders
* Verify the order is not in the list
* Set filter for 'To' date into the future, i.e. 01/01/2025
=> Without this patch the order shows up
=> With this patch applied the received order does not show up

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 7 Hinemoea Viault 2023-05-04 18:15:17 UTC
Created attachment 150686 [details] [review]
Bug 33653: Never consider received orders as late

We should not list received orders on the late orders page.

Test plan:
* Create a basket
* Create a normal order (not from a subscription, no standing order)
* Leave estimated delivery date empty
* Close the basket
* Create a new invoice and receive the order line
* Go to acq > late orders
* Verify the order is not in the list
* Set filter for 'To' date into the future, i.e. 01/01/2025
=> Without this patch the order shows up
=> With this patch applied the received order does not show up

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Hinemoea Viault <hinemoea.viault@inlibro.com>
Comment 8 Tomás Cohen Arazi 2023-05-05 13:11:31 UTC
This is correct but: shouldn't we chain ->filter_by_active instead?
Comment 9 Tomás Cohen Arazi 2023-05-08 12:27:43 UTC
Raising author's attention.
Comment 10 Jonathan Druart 2023-05-09 07:20:57 UTC
Created attachment 150821 [details] [review]
Bug 33653: Use filter_by_active instead

We have filter_by_active that is filtering by status already.
Comment 11 Jonathan Druart 2023-05-09 07:21:40 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> This is correct but: shouldn't we chain ->filter_by_active instead?

Yes, that makes sense. Katrin, do you agree? I have removed the existing condition on status in filter_by_lates.
Comment 12 Katrin Fischer 2023-05-09 08:00:12 UTC
If I am reading that right:

        {
            '-or' => [
                { 'basket.is_standing' => 1,
                  'orderstatus' => [ 'new', 'ordered', 'partial' ] },
                { 'orderstatus' => [ 'ordered', 'partial' ] }
            ]
        }

* It's a standing order with new, ordered or partial
* It's ordered or partial

That should work ok here.

I believe the standing orders could be an issue, but if I understand the code correctly, they are already showing up in the late orders:

-            ( orderstatus => { '!=' => 'cancelled' } ),
+            ( orderstatus => { '-not_in' => ['cancelled', 'complete'] } ),


If the delivery date is calculated from the order date, they will all be "late" for the 'new' order line and there is no way to tell that it is a standing order in the results list... probably something for a separate bug.
Comment 13 Katrin Fischer 2023-05-09 09:34:57 UTC
After talking to Joubu I realized we only include the new ones for standing orders now - this is another enhancement :)
Comment 14 Katrin Fischer 2023-05-09 09:52:09 UTC
Created attachment 150833 [details] [review]
Bug 33653: Never consider received orders as late

We should not list received orders on the late orders page.

Test plan:
* Create a basket
* Create a normal order (not from a subscription, no standing order)
* Leave estimated delivery date empty
* Close the basket
* Create a new invoice and receive the order line
* Go to acq > late orders
* Verify the order is not in the list
* Set filter for 'To' date into the future, i.e. 01/01/2025
=> Without this patch the order shows up
=> With this patch applied the received order does not show up

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 15 Katrin Fischer 2023-05-09 09:52:11 UTC
Created attachment 150834 [details] [review]
Bug 33653: Use filter_by_active instead

We have filter_by_active that is filtering by status already.

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 16 Tomás Cohen Arazi 2023-05-09 13:39:38 UTC
Created attachment 150865 [details] [review]
Bug 33653: Use filter_by_active instead

This patch makes `filter_by_lates` use the `filter_by_active` method
instead of an ad-hoc query filter. It has the advantage that it
considers standing orders too.

No behavior change, tests should still pass.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Tomás Cohen Arazi 2023-05-09 14:00:29 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 18 Matt Blenkinsop 2023-05-15 12:10:22 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 19 Lucas Gass 2023-06-15 16:50:28 UTC
Lots of merge conflicts with 22.05.x, cannot backport unless rebased.