Bug 15165 - REST API routes to list, edit and pay borrower's accountlines
Summary: REST API routes to list, edit and pay borrower's accountlines
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Arnaud
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 13935 (view as bug list)
Depends on: 15126 15894 15897
Blocks: 16652
  Show dependency treegraph
 
Reported: 2015-11-10 08:51 UTC by Alex Arnaud
Modified: 2017-05-03 17:31 UTC (History)
7 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:


Attachments
Bug 15165 - Add API route to list accountlines (11.05 KB, patch)
2015-11-12 13:49 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15165 - Add API route to edit accoulines (6.27 KB, patch)
2015-11-12 13:49 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15165 - Add API routes to pay accountlines (15.07 KB, patch)
2015-11-12 13:49 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15163: Do not erase patron attributes if limited to another library (5.28 KB, patch)
2015-12-01 12:55 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15165 - Add API route to list accountlines (11.05 KB, patch)
2016-04-13 12:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 15165 - Add API route to edit accoulines (6.26 KB, patch)
2016-04-13 12:04 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 15165 - Add API routes to pay accountlines (15.07 KB, patch)
2016-04-13 12:04 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 15165 - Add API routes to pay accountlines (15.72 KB, patch)
2016-04-27 09:30 UTC, Andreas Hedström Mace
Details | Diff | Splinter Review
Bug 15165 - Add API route to list accountlines (11.61 KB, patch)
2016-06-22 12:11 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15165 - Add API route to edit accountlines (6.73 KB, patch)
2016-06-22 12:12 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15165 - Add API routes to pay accountlines (16.58 KB, patch)
2016-06-22 12:12 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15165 - Add API route to list accountlines (80.90 KB, patch)
2016-08-30 14:40 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 15165 - Add API route to edit accountlines (84.11 KB, patch)
2016-08-30 14:40 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 15165 - Add API routes to pay accountlines (97.96 KB, patch)
2016-08-30 14:40 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 15165 - Add API route to list accountlines (9.74 KB, patch)
2016-10-20 13:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15165 - Add API route to edit accountlines (7.34 KB, patch)
2016-10-20 13:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15165 - Add API routes to pay accountlines (15.55 KB, patch)
2016-10-20 13:20 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15165 - Add API route to list accountlines (9.87 KB, patch)
2016-11-30 17:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15165 - Add API route to edit accountlines (7.34 KB, patch)
2016-11-30 17:24 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15165 - Add API routes to pay accountlines (15.55 KB, patch)
2016-11-30 17:24 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15165: followup - fix definitions, and passing amount parameter in pay method (1.98 KB, patch)
2016-11-30 17:24 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15165 - Add API route to list accountlines (9.87 KB, patch)
2017-01-19 13:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15165 - Add API route to edit accountlines (7.34 KB, patch)
2017-01-19 13:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15165 - Add API routes to pay accountlines (15.55 KB, patch)
2017-01-19 13:15 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15165: followup - fix definitions, and passing amount parameter in pay method (1.98 KB, patch)
2017-01-19 13:15 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15165: Remove meansofpayment parameter (1.31 KB, patch)
2017-02-06 14:39 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15165: Set amount parameter required, define its data type and confirm amount > 0 (10.23 KB, patch)
2017-02-06 14:40 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15165: Set amount parameter required, define its data type and confirm amount > 0 (9.80 KB, patch)
2017-02-06 14:42 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15165: Correct data types and fix some properties (2.51 KB, patch)
2017-02-14 10:25 UTC, Lari Taskula
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15165 - Add API route to list accountlines (9.94 KB, patch)
2017-04-27 11:53 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15165 - Add API route to edit accountlines (7.41 KB, patch)
2017-04-27 11:53 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15165 - Add API routes to pay accountlines (15.57 KB, patch)
2017-04-27 11:53 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15165: followup - fix definitions, and passing amount parameter in pay method (2.05 KB, patch)
2017-04-27 11:54 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15165: Remove meansofpayment parameter (1.38 KB, patch)
2017-04-27 11:54 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15165: Set amount parameter required, define its data type and confirm amount > 0 (9.87 KB, patch)
2017-04-27 11:54 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15165: Correct data types and fix some properties (2.58 KB, patch)
2017-04-27 11:55 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2015-11-10 08:51:35 UTC
Add routes on RESTful API to:
  - list all accountlines,
  - list accountlines for a specific borrower,
  - edit accountlines,
  - pay accountlines.
Comment 1 Alex Arnaud 2015-11-12 13:49:34 UTC Comment hidden (obsolete)
Comment 2 Alex Arnaud 2015-11-12 13:49:38 UTC Comment hidden (obsolete)
Comment 3 Alex Arnaud 2015-11-12 13:49:42 UTC Comment hidden (obsolete)
Comment 4 Julian Maurice 2015-11-12 13:54:15 UTC
*** Bug 13935 has been marked as a duplicate of this bug. ***
Comment 5 Marc Véron 2015-12-01 12:55:30 UTC Comment hidden (obsolete)
Comment 6 Marc Véron 2015-12-01 12:57:33 UTC
Comment on attachment 45259 [details] [review]
Bug 15163: Do not erase patron attributes if limited to another library

Wrong bug numer, sorry for the noise...
Comment 7 Julian Maurice 2016-04-13 12:03:59 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2016-04-13 12:04:11 UTC Comment hidden (obsolete)
Comment 9 Julian Maurice 2016-04-13 12:04:19 UTC Comment hidden (obsolete)
Comment 10 Andreas Hedström Mace 2016-04-27 09:30:07 UTC Comment hidden (obsolete)
Comment 11 Lari Taskula 2016-06-22 12:11:15 UTC
Patch apply failed because of Bug 13903 has been pushed to master and changed specification files. Rebasing on master. Also fixing the following things in patches:
- "Bug 15165 - Add API route to edit accoulines" title typo:
  accoulines->accountlines
- definitions/amountpayed.json => definitions/amountpaid.json
  (also payed => paid where ever it existed)
- set amountpaid-object type from integer to number in order to make
  amountpayment with decimal value as well.
- Test plans missing. Adding them.
Comment 12 Lari Taskula 2016-06-22 12:11:52 UTC Comment hidden (obsolete)
Comment 13 Lari Taskula 2016-06-22 12:12:13 UTC Comment hidden (obsolete)
Comment 14 Lari Taskula 2016-06-22 12:12:33 UTC Comment hidden (obsolete)
Comment 15 Lari Taskula 2016-06-22 12:14:42 UTC
(In reply to Andreas Hedström Mace from comment #10)
> Created attachment 50790 [details] [review] [review]
> It is not possible to pay specified/partial amounts (for one or more
> accountlines).
Maybe I misunderstood the problem, but doesn't this do it:
  PUT /accountlines/123/partialpayment
  {"amount": 5.00, "description": "payment description"} 
Or pay payment with specific amount to borrower:
  PUT /accountlines/:borrowernumber/amountpayment
  { "amount": 5, "description": "payment description" }

Anyway, comments to this bug. I think some fixes are needed.

- Payments should be POST instead of PUT (PUT is idempotent, but
  making multiple payments has different effect than sending just one)
- If patron has amountoustanding 0, payment is still possible. 
  Should we return an error instead? Also the same should be considered
  when making a payment bigger than current amountoutstanding.
- /accountlines/:borrowernumber/amountpayment is confusing because of
  :borrowernumber. How about /accountlines/amountpayment?borrowernumber=X
- Do you think it would be useful to return total and currency? e.g.
  GET /accountlines?borrowernumber=1
  {
  	"total": 100.00,
	"currency": "USD",
	"rows": [
	  {
		// accountline-object with amountoutstanding 25,
		// accountline-object with amountoutstanding 60,
		// accountline-object with amountoutstanding 15
	  }
	]
  }

I will switch this back to "In Discussion".
Comment 16 Lari Taskula 2016-06-22 12:21:04 UTC
(In reply to Lari Taskula from comment #15)
> 	"rows": [
> 	  {
> 		// accountline-object with amountoutstanding 25,
> 		// accountline-object with amountoutstanding 60,
> 		// accountline-object with amountoutstanding 15
> 	  }
> 	]
...should be of course array of accountline-objects, sorry for the typo!
"rows": [
 // accountline-object with amountoutstanding 25,
 // accountline-object with amountoutstanding 60,
 // accountline-object with amountoutstanding 15
]
Comment 17 Lari Taskula 2016-06-22 13:08:12 UTC
(In reply to Lari Taskula from comment #15)
> Anyway, comments to this bug. I think some fixes are needed.
One thing that I forgot to mention. I think it's a must to have just one endpoint for making a payment towards an accountline, otherwise it would be very confusing. I know this probably stems from C4::Accounts having multiple subroutines for making a payment, but Bug 15894 is working on unifying them as one.
Comment 18 Julian Maurice 2016-07-08 08:05:03 UTC
(In reply to Lari Taskula from comment #15)
> - Payments should be POST instead of PUT (PUT is idempotent, but
>   making multiple payments has different effect than sending just one)
I agree

> - If patron has amountoustanding 0, payment is still possible. 
>   Should we return an error instead?
Yes, probably

> - /accountlines/:borrowernumber/amountpayment is confusing because of
>   :borrowernumber. How about /accountlines/amountpayment?borrowernumber=X
I believe it's not a good idea to mix GET and POST parameters. What about POST /borrowers/:borrowernumber/payment ?

> - Do you think it would be useful to return total and currency? e.g.
>   GET /accountlines?borrowernumber=1
>   {
>   	"total": 100.00,
> 	"currency": "USD",
> 	"rows": [
> 	  {
> 		// accountline-object with amountoutstanding 25,
> 		// accountline-object with amountoutstanding 60,
> 		// accountline-object with amountoutstanding 15
> 	  }
> 	]
>   }
I think it couldn't hurt. But what currency should we return ? The default (active) currency ?
Comment 19 Lari Taskula 2016-07-08 09:49:28 UTC
(In reply to Julian Maurice from comment #18)
> > - /accountlines/:borrowernumber/amountpayment is confusing because of
> >   :borrowernumber. How about /accountlines/amountpayment?borrowernumber=X
> I believe it's not a good idea to mix GET and POST parameters. What about
> POST /borrowers/:borrowernumber/payment ?
> 
Good point. That makes more sense.

> > - Do you think it would be useful to return total and currency? e.g.
> >   GET /accountlines?borrowernumber=1
> >   {
> >   	"total": 100.00,
> > 	"currency": "USD",
> > 	"rows": [
> > 	  {
> > 		// accountline-object with amountoutstanding 25,
> > 		// accountline-object with amountoutstanding 60,
> > 		// accountline-object with amountoutstanding 15
> > 	  }
> > 	]
> >   }
> I think it couldn't hurt. But what currency should we return ? The default
> (active) currency ?
I think yes. Could there be a situation where active currency would not be the correct one, apart from misconfiguration of the active currency?
Comment 20 Julian Maurice 2016-08-22 12:28:47 UTC
Resetting status to ASSIGNED as this needs some work
Comment 21 Julian Maurice 2016-08-30 14:40:38 UTC
Created attachment 55033 [details] [review]
Bug 15165 - Add API route to list accountlines

GET /accountlines                  (list)
GET /accountlines?borrowernumber=X (list)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission updatecharges.
2. Go to http://yourlibrary/api/v1/accountlines?borrowernumber=XXX (replace
   XXX with a borrowernumber that has accountlines) and check you receive
   correct data
3. Go to http://yourlibrary/api/v1/accountlines and check that you receive
   all accountlines
4. Run unit tests in t/db_dependent/api/v1/accountlines.t
Comment 22 Julian Maurice 2016-08-30 14:40:43 UTC
Created attachment 55034 [details] [review]
Bug 15165 - Add API route to edit accountlines

PUT /accountlines/{accountlines_id} (edit)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission updatecharges.
2. Send PUT request to http://yourlibrary/api/v1/accountlines/YYY
   where YYY is one of your accountlines_id. See the body definition
   in definitions/editAccountlineBody.json and use this in your PUT request.
3. Run unit tests in t/db_dependent/api/v1/accountlines.t
Comment 23 Julian Maurice 2016-08-30 14:40:48 UTC
Created attachment 55035 [details] [review]
Bug 15165 - Add API routes to pay accountlines

POST /accountlines/(:accountlines_id)/payment  (pay towards accountline)
POST /patrons/(:borrowernumber)/payment        (pay towards borrower)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You must have permission updatecharges.
2. Create a fine to any patron and get the accountlines_id.
3. Send POST request to http://yourlibrary/api/v1/accountlines/YYY/payment
   without body where YYY is the accountlines_id you created in step 2.
4. Check that the accountline that you created in step 2 is paid.
5. Create two payments with amount 5.00 (with no other outstanding payments)
6. Send POST request to
   http://yourlibrary/api/v1/patrons/ZZZ/payment with body
   {"amount": 10}
   Replace ZZZ with the borrowernumber for which you have created two
   fines
7. Check that the two accountlines are paid.
8. Repeat step 2.
9. Send POST request to
   http://yourlibrary/api/v1/accountlines/YYY/payment with body
   {"amount": XXX}
   Replace YYY with the accountlines_id you created in step 8.
   Set amount (XXX) to the half of the amount of the fine you created in
   step 8.
10. Check that the fine is still outstanding with half of the original
    amount.
11. Run unit tests at t/db_dependent/api/v1/accountlines.t and
    t/db_dependent/api/v1/patrons.t
Comment 24 Julian Maurice 2016-08-30 14:45:27 UTC
Patches rewritten on top of bug 15894.

Routes /accountlines/{id}/payment and /accountlines/{id}/partialpayment have been merged.
Route /accountlines/{borrowernumber}/amountpayment has been moved to /patrons/{borrowernumber}/payment
These routes were previously "PUT routes", they are now "POST routes".
Comment 25 Lari Taskula 2016-10-20 13:19:35 UTC
Created attachment 56694 [details] [review]
Bug 15165 - Add API route to list accountlines

GET /accountlines                  (list)
GET /accountlines?borrowernumber=X (list)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission updatecharges.
2. Go to http://yourlibrary/api/v1/accountlines?borrowernumber=XXX (replace
   XXX with a borrowernumber that has accountlines) and check you receive
   correct data
3. Go to http://yourlibrary/api/v1/accountlines and check that you receive
   all accountlines
4. Run unit tests in t/db_dependent/api/v1/accountlines.t
Comment 26 Lari Taskula 2016-10-20 13:19:56 UTC
Created attachment 56695 [details] [review]
Bug 15165 - Add API route to edit accountlines

PUT /accountlines/{accountlines_id} (edit)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission updatecharges.
2. Send PUT request to http://yourlibrary/api/v1/accountlines/YYY
   where YYY is one of your accountlines_id. See the body definition
   in definitions/editAccountlineBody.json and use this in your PUT request.
3. Run unit tests in t/db_dependent/api/v1/accountlines.t
Comment 27 Lari Taskula 2016-10-20 13:20:11 UTC
Created attachment 56696 [details] [review]
Bug 15165 - Add API routes to pay accountlines

POST /accountlines/(:accountlines_id)/payment  (pay towards accountline)
POST /patrons/(:borrowernumber)/payment        (pay towards borrower)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You must have permission updatecharges.
2. Create a fine to any patron and get the accountlines_id.
3. Send POST request to http://yourlibrary/api/v1/accountlines/YYY/payment
   without body where YYY is the accountlines_id you created in step 2.
4. Check that the accountline that you created in step 2 is paid.
5. Create two payments with amount 5.00 (with no other outstanding payments)
6. Send POST request to
   http://yourlibrary/api/v1/patrons/ZZZ/payment with body
   {"amount": 10}
   Replace ZZZ with the borrowernumber for which you have created two
   fines
7. Check that the two accountlines are paid.
8. Repeat step 2.
9. Send POST request to
   http://yourlibrary/api/v1/accountlines/YYY/payment with body
   {"amount": XXX}
   Replace YYY with the accountlines_id you created in step 8.
   Set amount (XXX) to the half of the amount of the fine you created in
   step 8.
10. Check that the fine is still outstanding with half of the original
    amount.
11. Run unit tests at t/db_dependent/api/v1/accountlines.t and
    t/db_dependent/api/v1/patrons.t
Comment 28 Lari Taskula 2016-10-20 13:23:38 UTC
Moved permissions into Swagger and added tests for accessing own accountlines. Tests are failing for me for paying, but it is probably reasonable to wait until rest of the Koha::Account patches has been pushed before doing more for this.
Comment 29 Josef Moravec 2016-11-08 20:37:20 UTC
Getting accountlines by borrowernumber is impossible, as t/db_dependent/api/v1/accountlines.t also shows...

It is related to bug 17445

We should add borrowernumber to swagger definition of allowed parameters I think...
Comment 30 Josef Moravec 2016-11-30 17:10:50 UTC
Because of the way the Koha::Account->pay is called in the patch, this report depends especially on bug 15897.
Comment 31 Josef Moravec 2016-11-30 17:23:58 UTC
Created attachment 57843 [details] [review]
Bug 15165 - Add API route to list accountlines

GET /accountlines                  (list)
GET /accountlines?borrowernumber=X (list)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission updatecharges.
2. Go to http://yourlibrary/api/v1/accountlines?borrowernumber=XXX (replace
   XXX with a borrowernumber that has accountlines) and check you receive
   correct data
3. Go to http://yourlibrary/api/v1/accountlines and check that you receive
   all accountlines
4. Run unit tests in t/db_dependent/api/v1/accountlines.t
Comment 32 Josef Moravec 2016-11-30 17:24:04 UTC
Created attachment 57844 [details] [review]
Bug 15165 - Add API route to edit accountlines

PUT /accountlines/{accountlines_id} (edit)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission updatecharges.
2. Send PUT request to http://yourlibrary/api/v1/accountlines/YYY
   where YYY is one of your accountlines_id. See the body definition
   in definitions/editAccountlineBody.json and use this in your PUT request.
3. Run unit tests in t/db_dependent/api/v1/accountlines.t
Comment 33 Josef Moravec 2016-11-30 17:24:10 UTC
Created attachment 57845 [details] [review]
Bug 15165 - Add API routes to pay accountlines

POST /accountlines/(:accountlines_id)/payment  (pay towards accountline)
POST /patrons/(:borrowernumber)/payment        (pay towards borrower)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You must have permission updatecharges.
2. Create a fine to any patron and get the accountlines_id.
3. Send POST request to http://yourlibrary/api/v1/accountlines/YYY/payment
   without body where YYY is the accountlines_id you created in step 2.
4. Check that the accountline that you created in step 2 is paid.
5. Create two payments with amount 5.00 (with no other outstanding payments)
6. Send POST request to
   http://yourlibrary/api/v1/patrons/ZZZ/payment with body
   {"amount": 10}
   Replace ZZZ with the borrowernumber for which you have created two
   fines
7. Check that the two accountlines are paid.
8. Repeat step 2.
9. Send POST request to
   http://yourlibrary/api/v1/accountlines/YYY/payment with body
   {"amount": XXX}
   Replace YYY with the accountlines_id you created in step 8.
   Set amount (XXX) to the half of the amount of the fine you created in
   step 8.
10. Check that the fine is still outstanding with half of the original
    amount.
11. Run unit tests at t/db_dependent/api/v1/accountlines.t and
    t/db_dependent/api/v1/patrons.t
Comment 34 Josef Moravec 2016-11-30 17:24:15 UTC
Created attachment 57846 [details] [review]
Bug 15165: followup - fix definitions, and passing amount parameter in pay method
Comment 35 Josef Moravec 2016-11-30 17:25:37 UTC
I rebased the patches on master and in followup fixed same small issues - we need this in our project.
Comment 36 Josef Moravec 2016-12-20 07:55:30 UTC
because of 15897
Comment 37 Josef Moravec 2017-01-19 11:42:22 UTC
Dependencies are pushed to master, marking back to need signoff
Comment 38 Josef Moravec 2017-01-19 13:14:49 UTC
Created attachment 59239 [details] [review]
Bug 15165 - Add API route to list accountlines

GET /accountlines                  (list)
GET /accountlines?borrowernumber=X (list)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission updatecharges.
2. Go to http://yourlibrary/api/v1/accountlines?borrowernumber=XXX (replace
   XXX with a borrowernumber that has accountlines) and check you receive
   correct data
3. Go to http://yourlibrary/api/v1/accountlines and check that you receive
   all accountlines
4. Run unit tests in t/db_dependent/api/v1/accountlines.t
Comment 39 Josef Moravec 2017-01-19 13:14:55 UTC
Created attachment 59240 [details] [review]
Bug 15165 - Add API route to edit accountlines

PUT /accountlines/{accountlines_id} (edit)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission updatecharges.
2. Send PUT request to http://yourlibrary/api/v1/accountlines/YYY
   where YYY is one of your accountlines_id. See the body definition
   in definitions/editAccountlineBody.json and use this in your PUT request.
3. Run unit tests in t/db_dependent/api/v1/accountlines.t
Comment 40 Josef Moravec 2017-01-19 13:15:00 UTC
Created attachment 59241 [details] [review]
Bug 15165 - Add API routes to pay accountlines

POST /accountlines/(:accountlines_id)/payment  (pay towards accountline)
POST /patrons/(:borrowernumber)/payment        (pay towards borrower)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You must have permission updatecharges.
2. Create a fine to any patron and get the accountlines_id.
3. Send POST request to http://yourlibrary/api/v1/accountlines/YYY/payment
   without body where YYY is the accountlines_id you created in step 2.
4. Check that the accountline that you created in step 2 is paid.
5. Create two payments with amount 5.00 (with no other outstanding payments)
6. Send POST request to
   http://yourlibrary/api/v1/patrons/ZZZ/payment with body
   {"amount": 10}
   Replace ZZZ with the borrowernumber for which you have created two
   fines
7. Check that the two accountlines are paid.
8. Repeat step 2.
9. Send POST request to
   http://yourlibrary/api/v1/accountlines/YYY/payment with body
   {"amount": XXX}
   Replace YYY with the accountlines_id you created in step 8.
   Set amount (XXX) to the half of the amount of the fine you created in
   step 8.
10. Check that the fine is still outstanding with half of the original
    amount.
11. Run unit tests at t/db_dependent/api/v1/accountlines.t and
    t/db_dependent/api/v1/patrons.t
Comment 41 Josef Moravec 2017-01-19 13:15:07 UTC
Created attachment 59242 [details] [review]
Bug 15165: followup - fix definitions, and passing amount parameter in pay method
Comment 42 Josef Moravec 2017-01-19 13:15:19 UTC
Just a rebase
Comment 43 Josef Moravec 2017-01-20 08:11:40 UTC
Test t/db_dependent/api/v1/accountlines.t fails:

Use of uninitialized value $amount in subtraction (-) at /home/vagrant/kohaclone/Koha/Account.pm line 178.
Use of uninitialized value $amount in subtraction (-) at /home/vagrant/kohaclone/Koha/Account.pm line 205.

#   Failed test at t/db_dependent/api/v1/accountlines.t line 178.
# Looks like you failed 1 test of 37.


it's because the Koha::Account->pay always need an amount... so when api doesn't get an amount parameter, it can't have anything to pass - I think the API should return an error then
Comment 44 Lari Taskula 2017-02-06 14:39:49 UTC
Created attachment 59932 [details] [review]
Bug 15165: Remove meansofpayment parameter

Accountlines does not have such column in current master.
Comment 45 Lari Taskula 2017-02-06 14:40:05 UTC
Created attachment 59933 [details] [review]
Bug 15165: Set amount parameter required, define its data type and confirm amount > 0

This patch enforces "amount" to be required when paying towards accountline(s).
It also specifies the data type of amount, so that nonsense amounts like "strings"
cannot go through. Also, defines minimum amount as 0 and exclusiveMinimum true so
that the amount must be greater than 0 (and not equal).

This patch also wraps the operations in try/catch as now recommended. Possible
enhancements to Koha::Account->pay could also be done so that we would catch
Koha::Exceptions in the case of invalid input. However, this is probably outside
the scope of this bug and would require more testing on other components as well.
After all, we can handle the input nicely with Swagger alone.

To test:
1. prove t/db_dependent/api/v1/accountlines.t
2. prove t/db_dependent/api/v1/patrons.t
Comment 46 Lari Taskula 2017-02-06 14:42:41 UTC
Created attachment 59934 [details] [review]
Bug 15165: Set amount parameter required, define its data type and confirm amount > 0

This patch enforces "amount" to be required when paying towards accountline(s).
It also specifies the data type of amount, so that nonsense amounts like "strings"
cannot go through. Also, defines minimum amount as 0 and exclusiveMinimum true so
that the amount must be greater than 0 (and not equal).

This patch also wraps the operations in try/catch as now recommended. Possible
enhancements to Koha::Account->pay could also be done so that we would catch
Koha::Exceptions in the case of invalid input. However, this is probably outside
the scope of this bug and would require more testing on other components as well.
After all, we can handle the input nicely with Swagger alone.

To test:
1. prove t/db_dependent/api/v1/accountlines.t
2. prove t/db_dependent/api/v1/patrons.t
Comment 47 Lari Taskula 2017-02-06 14:47:39 UTC
(In reply to Josef Moravec from comment #43)
> Test t/db_dependent/api/v1/accountlines.t fails:
> 
> Use of uninitialized value $amount in subtraction (-) at
> /home/vagrant/kohaclone/Koha/Account.pm line 178.
> Use of uninitialized value $amount in subtraction (-) at
> /home/vagrant/kohaclone/Koha/Account.pm line 205.
> 
> #   Failed test at t/db_dependent/api/v1/accountlines.t line 178.
> # Looks like you failed 1 test of 37.
> 
> 
> it's because the Koha::Account->pay always need an amount... so when api
> doesn't get an amount parameter, it can't have anything to pass - I think
> the API should return an error then

I took a look at this and provided a patch to fix it. Initially I wanted to enhance Koha::Account->pay to throw Koha::Exceptions in case of invalid input for amount (non-numeric / not given / not more than zero) but I guess it could cause problems elsewhere and is probably out of this bug's scope. Swagger offers the tools to fix this nicely ("required" parameter and "minimum"/"exclusiveMinimum" to define min amount).
Comment 48 Lari Taskula 2017-02-14 10:25:25 UTC
Created attachment 60180 [details] [review]
Bug 15165: Correct data types and fix some properties

Removed properties:
- time

Added properties:
- issue_id
- dispute

(Note: this patch currently causes tests to fail because we do not numify numbers
before returning the data)
Comment 49 Josef Moravec 2017-04-27 11:53:42 UTC
Created attachment 62769 [details] [review]
[SIGNED-OFF] Bug 15165 - Add API route to list accountlines

GET /accountlines                  (list)
GET /accountlines?borrowernumber=X (list)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission updatecharges.
2. Go to http://yourlibrary/api/v1/accountlines?borrowernumber=XXX (replace
   XXX with a borrowernumber that has accountlines) and check you receive
   correct data
3. Go to http://yourlibrary/api/v1/accountlines and check that you receive
   all accountlines
4. Run unit tests in t/db_dependent/api/v1/accountlines.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 50 Josef Moravec 2017-04-27 11:53:49 UTC
Created attachment 62770 [details] [review]
[SIGNED-OFF] Bug 15165 - Add API route to edit accountlines

PUT /accountlines/{accountlines_id} (edit)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission updatecharges.
2. Send PUT request to http://yourlibrary/api/v1/accountlines/YYY
   where YYY is one of your accountlines_id. See the body definition
   in definitions/editAccountlineBody.json and use this in your PUT request.
3. Run unit tests in t/db_dependent/api/v1/accountlines.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 51 Josef Moravec 2017-04-27 11:53:56 UTC
Created attachment 62771 [details] [review]
[SIGNED-OFF] Bug 15165 - Add API routes to pay accountlines

POST /accountlines/(:accountlines_id)/payment  (pay towards accountline)
POST /patrons/(:borrowernumber)/payment        (pay towards borrower)

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You must have permission updatecharges.
2. Create a fine to any patron and get the accountlines_id.
3. Send POST request to http://yourlibrary/api/v1/accountlines/YYY/payment
   without body where YYY is the accountlines_id you created in step 2.
4. Check that the accountline that you created in step 2 is paid.
5. Create two payments with amount 5.00 (with no other outstanding payments)
6. Send POST request to
   http://yourlibrary/api/v1/patrons/ZZZ/payment with body
   {"amount": 10}
   Replace ZZZ with the borrowernumber for which you have created two
   fines
7. Check that the two accountlines are paid.
8. Repeat step 2.
9. Send POST request to
   http://yourlibrary/api/v1/accountlines/YYY/payment with body
   {"amount": XXX}
   Replace YYY with the accountlines_id you created in step 8.
   Set amount (XXX) to the half of the amount of the fine you created in
   step 8.
10. Check that the fine is still outstanding with half of the original
    amount.
11. Run unit tests at t/db_dependent/api/v1/accountlines.t and
    t/db_dependent/api/v1/patrons.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 52 Josef Moravec 2017-04-27 11:54:01 UTC
Created attachment 62772 [details] [review]
[SIGNED-OFF] Bug 15165: followup - fix definitions, and passing amount parameter in pay method

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 53 Josef Moravec 2017-04-27 11:54:10 UTC
Created attachment 62773 [details] [review]
[SIGNED-OFF] Bug 15165: Remove meansofpayment parameter

Accountlines does not have such column in current master.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 54 Josef Moravec 2017-04-27 11:54:20 UTC
Created attachment 62774 [details] [review]
[SIGNED-OFF] Bug 15165: Set amount parameter required, define its data type and confirm amount > 0

This patch enforces "amount" to be required when paying towards accountline(s).
It also specifies the data type of amount, so that nonsense amounts like "strings"
cannot go through. Also, defines minimum amount as 0 and exclusiveMinimum true so
that the amount must be greater than 0 (and not equal).

This patch also wraps the operations in try/catch as now recommended. Possible
enhancements to Koha::Account->pay could also be done so that we would catch
Koha::Exceptions in the case of invalid input. However, this is probably outside
the scope of this bug and would require more testing on other components as well.
After all, we can handle the input nicely with Swagger alone.

To test:
1. prove t/db_dependent/api/v1/accountlines.t
2. prove t/db_dependent/api/v1/patrons.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 55 Josef Moravec 2017-04-27 11:55:16 UTC
Created attachment 62775 [details] [review]
[SIGNED-OFF] Bug 15165: Correct data types and fix some properties

Removed properties:
- time

Added properties:
- issue_id
- dispute

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 56 Tomás Cohen Arazi 2017-05-03 17:31:05 UTC
Accountlines are tied to patrons. That means they might better be placed in /patrons/{patron_id}/accountlines. Is there a place in Koha where all accountlines are listed somehow that would require this kind of global endpoint? I don't see the use case for it.