Bug 20942

Summary: Add route to get patron's account balance
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact:
Severity: new feature    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, josef.moravec, lari.taskula, m.de.rooy, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
URL: https://wiki.koha-community.org/wiki/Patrons_account_lines_endpoint_RFC
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15165
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Introduces API endpoints for dealing with patron accounts, a highly requested feature for third-party integrations.
Version(s) released in:
Bug Depends on: 17586, 20946, 20990    
Bug Blocks: 15165, 20944, 21786, 23607    
Attachments: Bug 20942: OpenAPI spec for /patrons/{patron_id}/account
Bug 20942: Unit tests for /patrons/{patron_id}/account
Bug 20942: Add route to get patron's account balance
Bug 20942: Split debit and credit lines
Bug 20942: OpenAPI spec for /patrons/{patron_id}/account
Bug 20942: Unit tests for /patrons/{patron_id}/account
Bug 20942: Add route to get patron's account balance
Bug 20942: Split debit and credit lines
Bug 20942: (QA follow-up) Use $lines->total_outstanding
Bug 20942: OpenAPI spec for /patrons/{patron_id}/account
Bug 20942: Unit tests for /patrons/{patron_id}/account
Bug 20942: Add route to get patron's account balance
Bug 20942: Split debit and credit lines
Bug 20942: (QA follow-up) Use $lines->total_outstanding

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.