Bug 37533 - Invalid query when receiving an order
Summary: Invalid query when receiving an order
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P3 blocker
Assignee: Andreas Jonsson
QA Contact: David Cook
URL:
Keywords:
: 37555 (view as bug list)
Depends on: 37018
Blocks:
  Show dependency treegraph
 
Reported: 2024-07-31 09:02 UTC by Andreas Jonsson
Modified: 2024-08-16 13:41 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.03,23.11.08,23.05.14
Circulation function:


Attachments
Bug 37533: fix query in orderreceive.tt (1.37 KB, patch)
2024-07-31 09:11 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 37533: fix query in orderreceive.tt (1.37 KB, patch)
2024-07-31 09:18 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 37533: fix query in orderreceive.tt (1.43 KB, patch)
2024-08-01 02:32 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37533: fix query in orderreceive.tt (1.48 KB, patch)
2024-08-01 04:29 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Jonsson 2024-07-31 09:02:58 UTC
There is an invalid query in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt

            var base_query = { "order_id": {"in": [[% multiple_orders | html %]]}};

It should be:

            var base_query = { "order_id": {"-in": [[% multiple_orders | html %]]}};


The validation introduced in bug 37018 will not allow the operator "in".
Comment 1 Andreas Jonsson 2024-07-31 09:11:23 UTC
Created attachment 169881 [details] [review]
Bug 37533: fix query in orderreceive.tt

The new validation in the REST API will no longer allow
the operator "in".  Consequently, it has to be replaced
with the allowed "-in".

Test plan:

 * Open an invoice and click "Go to receipt page" and
   on any basket click "receive" and make sure the dialog
   box appears.
Comment 2 Andreas Jonsson 2024-07-31 09:18:03 UTC
Created attachment 169882 [details] [review]
Bug 37533: fix query in orderreceive.tt

The new validation in the REST API will no longer allow
the operator "in".  Consequently, it has to be replaced
with the allowed "-in".

Test plan:

 * Open an invoice and click "Go to receipt page" and
   on any basket click "receive" and make sure the dialog
   box appears.
Comment 3 Aleisha Amohia 2024-08-01 02:32:30 UTC
Created attachment 169923 [details] [review]
Bug 37533: fix query in orderreceive.tt

The new validation in the REST API will no longer allow
the operator "in".  Consequently, it has to be replaced
with the allowed "-in".

Test plan:

 * Open an invoice and click "Go to receipt page" and
   on any basket click "receive" and make sure the dialog
   box appears.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 4 David Cook 2024-08-01 03:53:40 UTC
I'm going to need more information on this one...

Without the patch, I've gone to http://localhost:8081/cgi-bin/koha/acqui/parcel.pl?invoiceid=1 and I've received the item and had no problems. 

What's the bug that this is fixing?
Comment 5 Aleisha Amohia 2024-08-01 04:00:31 UTC
Here's how I reproduced the bug on 23.11.x:

1. Have a vendor and a basket
2. Add an order to the basket
3. Close the basket
4. Receive shipments
5. Add an invoice number and continue
6. On the receiving items page, click Receive in the row of your order
7. Instead of the usual pop-up window to receive my item, you get an error that says Something went wrong when loading the table. 400: Bad request.

This patch solved this error and I was able to receive my item
Comment 6 David Cook 2024-08-01 04:19:15 UTC
(In reply to Aleisha Amohia from comment #5)
> Here's how I reproduced the bug on 23.11.x:
> 
> 1. Have a vendor and a basket
> 2. Add an order to the basket
> 3. Close the basket
> 4. Receive shipments
> 5. Add an invoice number and continue
> 6. On the receiving items page, click Receive in the row of your order
> 7. Instead of the usual pop-up window to receive my item, you get an error
> that says Something went wrong when loading the table. 400: Bad request.
> 
> This patch solved this error and I was able to receive my item

Hmm... that's so interesting. That's the test plan I did on "main" and it worked fine. 

172.20.0.1 - - [01/Aug/2024:04:14:13 +0000] "GET /api/v1/app.pl/api/v1/acquisitions/orders?only_active=1&q=%7B%22order_id%22:%7B%22in%22:%5B2%5D%7D%7D&_page=1&_per_page=20&_match=contains&_order_by=%2Bme.order_id HTTP/1.1" 200 - "http://
localhost:8081/cgi-bin/koha/acqui/orderreceive.pl?multiple_orders=2&invoiceid=1" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36"

If bug 37018 were working, I would think that this should cause an error but doesn't. Curious. 

I suspected I needed to re-run "perl build-resources.PL" to update the API definitions, but even after doing that it is still working fine...
Comment 7 David Cook 2024-08-01 04:24:00 UTC
Yeah... something is rotten in Denmark.

Bug 37018 is in a state of "Passed QA", some of the patches are in "23.11.07", but some are missing from "main". It's a right mess at the moment.

I'll bring it up on bug 37018 and Mattermost...

--

For this one, it's a simple change, so I'll check that it still works with it (it should), and then stamp it.
Comment 8 David Cook 2024-08-01 04:29:19 UTC
Created attachment 169924 [details] [review]
Bug 37533: fix query in orderreceive.tt

The new validation in the REST API will no longer allow
the operator "in".  Consequently, it has to be replaced
with the allowed "-in".

Test plan:

 * Open an invoice and click "Go to receipt page" and
   on any basket click "receive" and make sure the dialog
   box appears.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 9 Katrin Fischer 2024-08-01 08:03:16 UTC
(In reply to David Cook from comment #7)
> Yeah... something is rotten in Denmark.
> 
> Bug 37018 is in a state of "Passed QA", some of the patches are in
> "23.11.07", but some are missing from "main". It's a right mess at the
> moment.
> 
> I'll bring it up on bug 37018 and Mattermost...
> 
> --
> 
> For this one, it's a simple change, so I'll check that it still works with
> it (it should), and then stamp it.

This was in the security release from Tuesday. The security patches will be the next ones I push, but currently stuck on with some conflicts/failing tests - see security channel if you want to help things along.

I can pick this up directly after they have been pushed. Assume the patches are there for testing maybe.
Comment 10 Katrin Fischer 2024-08-01 16:27:40 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 11 Katrin Fischer 2024-08-02 09:48:30 UTC
*** Bug 37555 has been marked as a duplicate of this bug. ***
Comment 12 Tomás Cohen Arazi 2024-08-12 18:38:20 UTC
Pushed to main for 24.11.

Nice work everyone, thanks!
Comment 13 Jonathan Druart 2024-08-14 09:40:20 UTC
Why don't we allow "in" then?
Comment 14 David Cook 2024-08-14 23:49:12 UTC
(In reply to Jonathan Druart from comment #13)
> Why don't we allow "in" then?

That's a good question. I imagine that "in" must've worked, although SQL::Abstract::Classic (and SQL::Abstract) seems to dictate "-in" in their docs. But the docs don't really explain it.

Looking at the SQL::Abstract source code, it looks like it's expecting modifiers to start with "-". 

It's a bit hard to tell at a glance, but it kind of seems that "-in" and "in" might yield different structures (or at least take different code paths to get to the same place).

Not sure if it's worth investigating or not, but probably good to require the "-" prefix.
Comment 15 Tomás Cohen Arazi 2024-08-14 23:52:37 UTC
(In reply to Jonathan Druart from comment #13)
> Why don't we allow "in" then?

I'm not sure how we've built the allow-list on bug 37018. But yes: another option would've been to just add it to the list.

I feel like it is not a bad thing to be able to identify more precisely this operands.