|
Description
Matt Blenkinsop
2023-07-14 09:23:57 UTC
You want a filter on /patrons? And an embed? Martin and I have discussed refactoring the patron accounts API - I've got a patch incoming to add this endpoint until we can get round to doing that work. I think the end goal will be to use /patrons with an embed and query parameters to fetch account data Created attachment 153477 [details] [review] Bug 34277: Add a patrons with outstanding charges endpoint This patch adds an endpoint to return all patrons with outstanding debits/credits on their account. Currently this can't be achieved through the API without looping through all patrons using /patrons/{patron_id}/account Test plan: 1) Apply patch 2) Add some charges/credits to some patron accounts 3) Call /api/v1/patrons/get_balances 4) An array should be returned with all the patrons that you added charges to Created attachment 153478 [details] [review] Bug 34277: Add unit test prove t/db_dependent/api/v1/patrons_accounts Unit tests failed:
# Failed test 'exact match for JSON Pointer ""'
# at t/db_dependent/api/v1/patrons_accounts.t line 510.
# Structures begin differing at:
# $got->[0] = HASH(0xaaaae46bfd98)
# $expected->[0] = Does not exist
# Failed test 'exact match for JSON Pointer ""'
# at t/db_dependent/api/v1/patrons_accounts.t line 525.
# Structures begin differing at:
# $got->[0]{outstanding_debits}{total} = '0.15'
# $expected->[0]{outstanding_debits}{total} = '10'
# Failed test 'exact match for JSON Pointer ""'
# at t/db_dependent/api/v1/patrons_accounts.t line 552.
# Structures begin differing at:
# $got->[0]{outstanding_debits}{lines}[0]{interface} = 'intranet'
# $expected->[0]{outstanding_debits}{lines}[0]{interface} = 'test'
# Failed test 'exact match for JSON Pointer ""'
# at t/db_dependent/api/v1/patrons_accounts.t line 590.
# Structures begin differing at:
# $got->[0]{balance} = '0.15'
# $expected->[0]{balance} = '0'
# Failed test 'exact match for JSON Pointer ""'
# at t/db_dependent/api/v1/patrons_accounts.t line 628.
# Structures begin differing at:
# $got->[0]{outstanding_credits}{total} = '0'
# $expected->[0]{outstanding_credits}{total} = '-10'
# Looks like you failed 5 tests of 15.
t/db_dependent/api/v1/patrons_accounts.t .. 6/6
# Failed test 'get_patron_balances() tests'
# at t/db_dependent/api/v1/patrons_accounts.t line 645.
# Looks like you failed 1 test of 6.
t/db_dependent/api/v1/patrons_accounts.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/6 subtests
Test Summary Report
-------------------
t/db_dependent/api/v1/patrons_accounts.t (Wstat: 256 Tests: 6 Failed: 1)
Failed test: 6
Non-zero exit status: 1
Files=1, Tests=6, 11 wallclock secs ( 0.02 usr 0.02 sys + 4.81 cusr 0.70 csys = 5.55 CPU)
Result: FAIL
Hi Laura, The tests are working fine for me and for one of my team as well who applied it from bugzilla - did you restart plack before running the tests? Tests fail for me when I add a manual credit to a patron accout:
# Failed test 'exact match for JSON Pointer ""'
# at /kohadevbox/koha/t/db_dependent/api/v1/patrons_accounts.t line 510.
# Structures begin differing at:
# $got->[0] = HASH(0x56200b91c9c8)
# $expected->[0] = Does not exist
ok 4 - GET //aMgjvtk9LM4Tcta89qLDALIsozh9yWEECy4Au6B:thePassword123@/api/v1/patrons/get_balances
ok 5 - 200 OK
not ok 6 - exact match for JSON Pointer ""
# Failed test 'exact match for JSON Pointer ""'
# at /kohadevbox/koha/t/db_dependent/api/v1/patrons_accounts.t line 525.
# Structures begin differing at:
# $got->[0]{outstanding_credits}{lines}[0] = HASH(0x56200b4754d0)
# $expected->[0]{outstanding_credits}{lines}[0] = Does not exist
ok 7 - GET //aMgjvtk9LM4Tcta89qLDALIsozh9yWEECy4Au6B:thePassword123@/api/v1/patrons/get_balances
ok 8 - 200 OK
not ok 9 - exact match for JSON Pointer ""
# Failed test 'exact match for JSON Pointer ""'
# at /kohadevbox/koha/t/db_dependent/api/v1/patrons_accounts.t line 552.
# Structures begin differing at:
# $got->[0]{outstanding_credits}{lines}[0] = HASH(0x56200b9414b8)
# $expected->[0]{outstanding_credits}{lines}[0] = Does not exist
ok 10 - GET //aMgjvtk9LM4Tcta89qLDALIsozh9yWEECy4Au6B:thePassword123@/api/v1/patrons/get_balances
ok 11 - 200 OK
not ok 12 - exact match for JSON Pointer ""
# Failed test 'exact match for JSON Pointer ""'
# at /kohadevbox/koha/t/db_dependent/api/v1/patrons_accounts.t line 590.
# Structures begin differing at:
# $got->[0]{outstanding_credits}{lines}[0]{credit_type} = 'CREDIT'
# $expected->[0]{outstanding_credits}{lines}[0]{credit_type} = 'PAYMENT'
ok 13 - GET //aMgjvtk9LM4Tcta89qLDALIsozh9yWEECy4Au6B:thePassword123@/api/v1/patrons/get_balances
ok 14 - 200 OK
not ok 15 - exact match for JSON Pointer ""
# Failed test 'exact match for JSON Pointer ""'
# at /kohadevbox/koha/t/db_dependent/api/v1/patrons_accounts.t line 628.
# Structures begin differing at:
# $got->[0]{borrowernumber} = '51'
# $expected->[0]{borrowernumber} = '66'
# Looks like you failed 5 tests of 15.
Created attachment 154516 [details] [review] Bug 34277: Add unit test prove t/db_dependent/api/v1/patrons_accounts.t Tests should now be ignoring existing lines to isolate the test environment Comment on attachment 153477 [details] [review] Bug 34277: Add a patrons with outstanding charges endpoint Review of attachment 153477 [details] [review]: ----------------------------------------------------------------- ::: api/v1/swagger/definitions/patron_balance.yaml @@ +1,4 @@ > --- > type: object > properties: > + borrowernumber: This should be `patron_id`. Can't we simplify it like GET /patrons?with_outstanding_debts=1 x-koha-embed: balance This way you will benefit from things like pagination, etc. Your current approach just returns the whole resultset, which could still be done with GET /patrons?with_outstanding_debts=1&_per_page=-1 x-koha-embed: balance My two cents. (In reply to Tomás Cohen Arazi from comment #11) > Can't we simplify it like > > GET /patrons?with_outstanding_debts=1 > x-koha-embed: balance For implementing this, you could: - Use Koha::Patrons->filter_by_amount_owed (or a simplified version, > 0, on top of it) - You would need to implement Koha::Patron->balance, for embedding purposes. Created attachment 156185 [details] [review] [ALTERNATE] Bug 34277: Add a way to filter patrons by their outstanding balance This patch adds to new `query parameters` to the `GET /patrons` route: * owes_less_than * owes_more_than This parameters will be used to build a query, based on Koha::Patrons->filter_by_amount_owed, to return only patrons matching the criteria. FIXME: Tests missing yet Created attachment 156186 [details] [review] [ALTERNATE] Bug 34277: Add option to embed balance details on GET /patrons Created attachment 156187 [details] [review] [ALTERNATE] Bug 34277: (follow-up) Use Koha::Patron->balance_details in GET /patrons/{patron_id}/account This patch makes the existing route reuse the introduced method. To verify no behavior change: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/patrons_accounts.t => SUCCESS: Tests pass! 2. Sign off :-D Hi, this are my 2 cents. Another angle for the problem. Keep in mind that: - I haven't written tests - I haven't dug into options for the weird 'outstanding' attribute that gets introduced by ->filter_by_amount_owed The test doesn't pass. error : Test Summary Report ------------------- t/db_dependent/api/v1/patrons_accounts.t (Wstat: 512 Tests: 1 Failed: 1) Failed test: 1 Non-zero exit status: 2 Parse errors: Bad plan. You planned 5 tests but ran 1. Files=1, Tests=1, 3 wallclock secs ( 0.01 usr 0.00 sys + 3.07 cusr 0.13 csys = 3.21 CPU) Result: FAIL |