Bug 20942 - Add route to get patron's account balance
Summary: Add route to get patron's account balance
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST api (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL: https://wiki.koha-community.org/wiki/...
Whiteboard:
Keywords:
Depends on: 20946 17586 20990
Blocks: 15165 20944
  Show dependency treegraph
 
Reported: 2018-06-14 13:47 UTC by Tomás Cohen Arazi
Modified: 2018-08-28 12:37 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Introduces API endpoints for dealing with patron accounts, a highly requested feature for third-party integrations.


Attachments
Bug 20942: OpenAPI spec for /patrons/{patron_id}/account (6.68 KB, patch)
2018-06-14 13:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20942: Unit tests for /patrons/{patron_id}/account (5.60 KB, patch)
2018-06-14 13:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20942: Add route to get patron's account balance (5.51 KB, patch)
2018-06-14 13:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20942: Split debit and credit lines (8.53 KB, patch)
2018-06-25 18:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20942: OpenAPI spec for /patrons/{patron_id}/account (6.73 KB, patch)
2018-06-25 19:18 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20942: Unit tests for /patrons/{patron_id}/account (5.65 KB, patch)
2018-06-25 19:18 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20942: Add route to get patron's account balance (5.57 KB, patch)
2018-06-25 19:18 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20942: Split debit and credit lines (8.57 KB, patch)
2018-06-25 19:18 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20942: (QA follow-up) Use $lines->total_outstanding (1.77 KB, patch)
2018-07-02 19:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20942: OpenAPI spec for /patrons/{patron_id}/account (6.81 KB, patch)
2018-07-10 17:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20942: Unit tests for /patrons/{patron_id}/account (5.71 KB, patch)
2018-07-10 17:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20942: Add route to get patron's account balance (5.63 KB, patch)
2018-07-10 17:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20942: Split debit and credit lines (8.65 KB, patch)
2018-07-10 17:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20942: (QA follow-up) Use $lines->total_outstanding (1.82 KB, patch)
2018-07-10 17:14 UTC, Kyle M Hall
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 2018-06-14 13:47:21 UTC

    
Comment 1 Tomás Cohen Arazi 2018-06-14 13:58:46 UTC
Created attachment 76053 [details] [review]
Bug 20942: OpenAPI spec for /patrons/{patron_id}/account

This patch adds the OpenAPI spec for the following paths:
- /patrons/{patron_id}/account

It also adds object definitions for:
- balance
- account line

Account line is to be used on both /account/lines (when implemented)
and for embeding the outstanding lines in the balance endpoint
(/patrons/{patron_id}/account).
Comment 2 Tomás Cohen Arazi 2018-06-14 13:58:51 UTC
Created attachment 76054 [details] [review]
Bug 20942: Unit tests for /patrons/{patron_id}/account

This patch adds tests for the /patrons/{patron_id}/account endpoint.
To test:

- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> FAIL: Tests should fail because the endpoint is not implemented.
Comment 3 Tomás Cohen Arazi 2018-06-14 13:58:56 UTC
Created attachment 76055 [details] [review]
Bug 20942: Add route to get patron's account balance

This patch implements the /patrons/{patron_id}/account endpoint. It can
be used to get the patron's balance information, including outstanding
lines.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> SUCCESS: tests pass!
- Make your favourite REST testing tool (RESTer on Firefox?) do:
  GET /api/v1/patrons/{patron_id}/account
to a valid patron_id. Play with Koha's UI to add credits/payments and
notice the endpoint returns the right info following the voted RFC. [1]
- Sign off :-D

[1] https://wiki.koha-community.org/wiki/Patrons_account_lines_endpoint_RFC
Comment 4 Tomás Cohen Arazi 2018-06-25 18:02:42 UTC
Created attachment 76393 [details] [review]
Bug 20942: Split debit and credit lines

This patch splits the balance to match this object schema:

{
    balance             => #,
    outstanding_credits => {
        total => #,
        lines => [ credit_line_1, ..., credit_line_n ]
    },
    outstanding_debits  => {
        total => #,
        lines => [ debit_line_1, ..., debit_line_m ]
    }
}

This change is made to ease usage from the UI. Also because the
outstanding credits need to be applied to outstanding debits in order to
the balance value to make sense. So we still need to have each total.

Tests are added for this change, and the schema files are adjusted as
well.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> SUCCESS: Tests pass!
- Sign off :-D

staff_id is changed into user_id as voted on the dev meeting the RFC got
approved.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Josef Moravec 2018-06-25 19:18:39 UTC
Created attachment 76409 [details] [review]
Bug 20942: OpenAPI spec for /patrons/{patron_id}/account

This patch adds the OpenAPI spec for the following paths:
- /patrons/{patron_id}/account

It also adds object definitions for:
- balance
- account line

Account line is to be used on both /account/lines (when implemented)
and for embeding the outstanding lines in the balance endpoint
(/patrons/{patron_id}/account).

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 6 Josef Moravec 2018-06-25 19:18:44 UTC
Created attachment 76410 [details] [review]
Bug 20942: Unit tests for /patrons/{patron_id}/account

This patch adds tests for the /patrons/{patron_id}/account endpoint.
To test:

- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> FAIL: Tests should fail because the endpoint is not implemented.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Josef Moravec 2018-06-25 19:18:49 UTC
Created attachment 76411 [details] [review]
Bug 20942: Add route to get patron's account balance

This patch implements the /patrons/{patron_id}/account endpoint. It can
be used to get the patron's balance information, including outstanding
lines.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> SUCCESS: tests pass!
- Make your favourite REST testing tool (RESTer on Firefox?) do:
  GET /api/v1/patrons/{patron_id}/account
to a valid patron_id. Play with Koha's UI to add credits/payments and
notice the endpoint returns the right info following the voted RFC. [1]
- Sign off :-D

[1] https://wiki.koha-community.org/wiki/Patrons_account_lines_endpoint_RFC

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Josef Moravec 2018-06-25 19:18:54 UTC
Created attachment 76412 [details] [review]
Bug 20942: Split debit and credit lines

This patch splits the balance to match this object schema:

{
    balance             => #,
    outstanding_credits => {
        total => #,
        lines => [ credit_line_1, ..., credit_line_n ]
    },
    outstanding_debits  => {
        total => #,
        lines => [ debit_line_1, ..., debit_line_m ]
    }
}

This change is made to ease usage from the UI. Also because the
outstanding credits need to be applied to outstanding debits in order to
the balance value to make sense. So we still need to have each total.

Tests are added for this change, and the schema files are adjusted as
well.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> SUCCESS: Tests pass!
- Sign off :-D

staff_id is changed into user_id as voted on the dev meeting the RFC got
approved.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 9 Jonathan Druart 2018-06-27 19:06:04 UTC
General question: cannot we have only one of $to_api_mapping and $to_model_mapping, and generate the other one? It seems that we are duplicating the information.
Comment 10 Tomás Cohen Arazi 2018-07-02 19:21:29 UTC
Created attachment 76641 [details] [review]
Bug 20942: (QA follow-up) Use $lines->total_outstanding

This patch makes the controller code use $lines->total_outstanding
instead of expecting ->outstanding_debits and ->outstanding_credits
return the total.

Tests should pass as before. No behaviour change is expected.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> SUCCESS: Tests still pass, no behaviour change.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2018-07-02 19:25:10 UTC
(In reply to Jonathan Druart from comment #9)
> General question: cannot we have only one of $to_api_mapping and
> $to_model_mapping, and generate the other one? It seems that we are
> duplicating the information.

I don't think this is urgent, but yes, we could have a hash of hashes, one of them generated by reverse.
Comment 12 Kyle M Hall 2018-07-10 17:13:46 UTC
Created attachment 76808 [details] [review]
Bug 20942: OpenAPI spec for /patrons/{patron_id}/account

This patch adds the OpenAPI spec for the following paths:
- /patrons/{patron_id}/account

It also adds object definitions for:
- balance
- account line

Account line is to be used on both /account/lines (when implemented)
and for embeding the outstanding lines in the balance endpoint
(/patrons/{patron_id}/account).

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2018-07-10 17:14:02 UTC
Created attachment 76809 [details] [review]
Bug 20942: Unit tests for /patrons/{patron_id}/account

This patch adds tests for the /patrons/{patron_id}/account endpoint.
To test:

- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> FAIL: Tests should fail because the endpoint is not implemented.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2018-07-10 17:14:06 UTC
Created attachment 76810 [details] [review]
Bug 20942: Add route to get patron's account balance

This patch implements the /patrons/{patron_id}/account endpoint. It can
be used to get the patron's balance information, including outstanding
lines.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> SUCCESS: tests pass!
- Make your favourite REST testing tool (RESTer on Firefox?) do:
  GET /api/v1/patrons/{patron_id}/account
to a valid patron_id. Play with Koha's UI to add credits/payments and
notice the endpoint returns the right info following the voted RFC. [1]
- Sign off :-D

[1] https://wiki.koha-community.org/wiki/Patrons_account_lines_endpoint_RFC

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2018-07-10 17:14:10 UTC
Created attachment 76811 [details] [review]
Bug 20942: Split debit and credit lines

This patch splits the balance to match this object schema:

{
    balance             => #,
    outstanding_credits => {
        total => #,
        lines => [ credit_line_1, ..., credit_line_n ]
    },
    outstanding_debits  => {
        total => #,
        lines => [ debit_line_1, ..., debit_line_m ]
    }
}

This change is made to ease usage from the UI. Also because the
outstanding credits need to be applied to outstanding debits in order to
the balance value to make sense. So we still need to have each total.

Tests are added for this change, and the schema files are adjusted as
well.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> SUCCESS: Tests pass!
- Sign off :-D

staff_id is changed into user_id as voted on the dev meeting the RFC got
approved.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2018-07-10 17:14:14 UTC
Created attachment 76812 [details] [review]
Bug 20942: (QA follow-up) Use $lines->total_outstanding

This patch makes the controller code use $lines->total_outstanding
instead of expecting ->outstanding_debits and ->outstanding_credits
return the total.

Tests should pass as before. No behaviour change is expected.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons_accounts.t
=> SUCCESS: Tests still pass, no behaviour change.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Nick Clemens 2018-07-18 19:18:37 UTC
Awesome work all!

Pushed to master for 18.11
Comment 18 Martin Renvoize 2018-08-02 11:39:34 UTC
Pushed to 18.05.x series for 18.05.03

I've made the decision to backport new api routes (one's that are a simple backport at least - where simple means there's not a lot of unported dependancies or the api route bug itself doesn't include lots of related refactoring work).

My justification here is that an API route isn't immediately user facing and as such most users will not see the change. However, it does allow for third-party integrations to be developed against a more stable code base earlier and allow for a wider range of Koha versions to be supported by third-party applications.. that can only be seen as a good thing in my opinion.
Comment 19 Tomás Cohen Arazi 2018-08-02 12:14:08 UTC
(In reply to Martin Renvoize from comment #18)
> Pushed to 18.05.x series for 18.05.03
> 
> I've made the decision to backport new api routes (one's that are a simple
> backport at least - where simple means there's not a lot of unported
> dependancies or the api route bug itself doesn't include lots of related
> refactoring work).
> 
> My justification here is that an API route isn't immediately user facing and
> as such most users will not see the change. However, it does allow for
> third-party integrations to be developed against a more stable code base
> earlier and allow for a wider range of Koha versions to be supported by
> third-party applications.. that can only be seen as a good thing in my
> opinion.

I agree! Thanks!
Comment 20 Fridolin SOMERS 2018-08-28 12:37:40 UTC
I prefer not to push to 17.11.x to focus on major bugs.