Bug 24611 - Wrong budget_id query parameter in /acquisitions/orders
Summary: Wrong budget_id query parameter in /acquisitions/orders
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 18731
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-07 12:36 UTC by Tomás Cohen Arazi
Modified: 2020-11-30 21:48 UTC (History)
4 users (show)

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


Attachments
Bug 24611: Regression test (1.17 KB, patch)
2020-02-07 12:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24611: Fix wrong budget_id query param in /acquisitions/orders (1.47 KB, patch)
2020-02-07 12:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24611: Regression test (1.22 KB, patch)
2020-02-07 21:01 UTC, David Nind
Details | Diff | Splinter Review
Bug 24611: Fix wrong budget_id query param in /acquisitions/orders (1.51 KB, patch)
2020-02-07 21:01 UTC, David Nind
Details | Diff | Splinter Review
Bug 24611: Regression test (1.29 KB, patch)
2020-02-13 13:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24611: Fix wrong budget_id query param in /acquisitions/orders (1.59 KB, patch)
2020-02-13 13:09 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-02-07 12:36:44 UTC
Between the writing time of the endpoint (2017) and the time it was refactored and pushed, attribute mappings were introduced, and the query parameters didn't get enough attention so it still refers to budget_id. We need to fix it!
Comment 1 Tomás Cohen Arazi 2020-02-07 12:54:31 UTC
Created attachment 98562 [details] [review]
Bug 24611: Regression test
Comment 2 Tomás Cohen Arazi 2020-02-07 12:54:34 UTC
Created attachment 98563 [details] [review]
Bug 24611: Fix wrong budget_id query param in /acquisitions/orders

This patch fixes the wrong query parameter that slipped on rewriting the
patches for the voted RFC.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests fail because fund_id is not a valid query parameter
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Nind 2020-02-07 21:01:44 UTC
Created attachment 98592 [details] [review]
Bug 24611: Regression test

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2020-02-07 21:01:47 UTC
Created attachment 98593 [details] [review]
Bug 24611: Fix wrong budget_id query param in /acquisitions/orders

This patch fixes the wrong query parameter that slipped on rewriting the
patches for the voted RFC.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests fail because fund_id is not a valid query parameter
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Katrin Fischer 2020-02-09 10:28:54 UTC
Test is not happy on my system - leaving this Signed Off as I think they passed for David and Tomas:

    #   Failed test 'SWAGGER3.2.2'
    #   at t/db_dependent/api/v1/acquisitions_orders.t line 65.
    #          got: '500'
    #     expected: '200'

    #   Failed test 'has value for JSON Pointer "/14/order_id"'
    #   at t/db_dependent/api/v1/acquisitions_orders.t line 65.
    # Looks like you failed 2 tests of 8.
t/db_dependent/api/v1/acquisitions_orders.t .. 1/5 
#   Failed test 'list() tests'
#   at t/db_dependent/api/v1/acquisitions_orders.t line 97.
t/db_dependent/api/v1/acquisitions_orders.t .. 5/5 # Looks like you failed 1 test of 5.
t/db_dependent/api/v1/acquisitions_orders.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/5 subtests 

Test Summary Report
-------------------
t/db_dependent/api/v1/acquisitions_orders.t (Wstat: 256 Tests: 5 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=5, 16 wallclock secs ( 0.02 usr  0.02 sys + 11.47 cusr  1.89 csys = 13.40 CPU)
Result: FAIL
Comment 6 Tomás Cohen Arazi 2020-02-09 14:55:49 UTC
Katirna, I don't manage to reproduce:

(base) ✔ ~/git/koha [qa_24611 ↑·2|⚑ 13]
11:52 $ git reset --hard origin/master
HEAD is now at 99e2fbd083 Bug 13193: (RM follow-up) Remove Cache::Memcached::Fast from PerlDependencies
(base) ✔ ~/git/koha [qa_24611|⚑ 13]
11:53 $ git bz apply 24611
Bug 24611 - Wrong budget_id query parameter in /acquisitions/orders

98592 - Bug 24611: Regression test
98593 - Bug 24611: Fix wrong budget_id query param in /acquisitions/orders

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 24611: Regression test
Applying: Bug 24611: Fix wrong budget_id query param in /acquisitions/orders
(base) ✔ ~/git/koha [qa_24611 ↑·2|⚑ 13]
11:53 $ kshell
root@d749cd762ad3:kohadevbox$ kshell
kohadev-koha@d749cd762ad3:/kohadevbox/koha$ prove t/db_dependent/api/v1/acquisitions_orders.t
t/db_dependent/api/v1/acquisitions_orders.t .. ok
All tests successful.
Files=1, Tests=5, 22 wallclock secs ( 0.05 usr  0.02 sys + 12.53 cusr  2.75 csys = 15.35 CPU)
Result: PASS
kohadev-koha@d749cd762ad3:/kohadevbox/koha$
Comment 7 Katrin Fischer 2020-02-09 15:44:32 UTC
Just what I thought - I think it's my environment.
Comment 8 Tomás Cohen Arazi 2020-02-09 17:01:12 UTC
(In reply to Katrin Fischer from comment #7)
> Just what I thought - I think it's my environment.

Katrina, it would be interesting to see the output of you add 

use Data::Printer colored => 1;
p($t->tx->res);

After the failed test. It might highlight something important (bad handling on error conditions).
Comment 9 Jonathan Druart 2020-02-13 13:09:31 UTC
Created attachment 98822 [details] [review]
Bug 24611: Regression test

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2020-02-13 13:09:34 UTC
Created attachment 98823 [details] [review]
Bug 24611: Fix wrong budget_id query param in /acquisitions/orders

This patch fixes the wrong query parameter that slipped on rewriting the
patches for the voted RFC.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests fail because fund_id is not a valid query parameter
3. Apply this patch
4. Repeat (2)
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Martin Renvoize 2020-02-13 16:02:04 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Katrin Fischer 2020-02-14 15:14:25 UTC
If you are still interested in me testing again, please ping me when you are back from vacation - but I believe it's probably fine!
Comment 13 Joy Nelson 2020-03-05 00:59:17 UTC
Does not apply to 19.11.x branch.  Please Rebase if needed.