Add routes on RESTful API to: - list all accountlines, - list accountlines for a specific borrower, - edit accountlines, - pay accountlines.
Created attachment 44804 [details] [review] Bug 15165 - Add API route to list accountlines
Created attachment 44805 [details] [review] Bug 15165 - Add API route to edit accoulines
Created attachment 44806 [details] [review] Bug 15165 - Add API routes to pay accountlines
*** Bug 13935 has been marked as a duplicate of this bug. ***
Created attachment 45259 [details] [review] Bug 15163: Do not erase patron attributes if limited to another library The patron attributes displayed on editing a patron are not displayed if limited to another library. This is the easy but dirty way to fix this issue: It supposes that the data are not sensitive as they are now displayed in the html document. A better way to fix it would be to modify C4::Members::Attributes::SetBorrowerAttributes to delete only attributes we are editing. Test plan: 1/ Create 2 patron attributes, without branch limitations. 2/ Edit a patron and set a value for these attributes 3/ Limit a patron attributes to a library (one you are not logged in with). 4/ Edit again the patron. => You should not see the limited attributes 5/ Edit the patron attributes and remove the branch limitation => Without this patch, it has been removed from the database and is not displayed anymore. => With this patch, you should see it. Followed test plan, works as described. Signed-off-by: Marc Véron <veron@veron.ch>
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...
Created attachment 50189 [details] [review] Bug 15165 - Add API route to list accountlines Rebased on master Renamed Koha::Acountline(s)::type to _type
Created attachment 50190 [details] [review] Bug 15165 - Add API route to edit accoulines Rebased on master
Created attachment 50191 [details] [review] Bug 15165 - Add API routes to pay accountlines Rebased on master
Created attachment 50790 [details] [review] Bug 15165 - Add API routes to pay accountlines Signed-off-by: Andreas <andreas.hedstrom.mace@sub.su.se> Tested by Stockholm University Library. Works as intended in regards to accountlines (list, edit, pay). It is not possible to pay specified/partial amounts (for one or more accountlines). At least not that we have managed. I’m unsure if this is the intended purpose of this bug however, or if this is a possible future enhancement? Maybe Julian can comment on this? I’m going to set the status to Signed off as the basic functionality works, but if anyone disagrees feel free to set it back to Needs sign off!
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.
Created attachment 52681 [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
Created attachment 52682 [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
Created attachment 52683 [details] [review] Bug 15165 - Add API routes to pay accountlines PUT /accountlines/(:accountlines_id)/payment (pay towards accountline) PUT /accountlines/(:borrowernumber)/amountpayment (pay towards borrower) PUT /accountlines/(:accountlines_id)/partialpayment (pay accountline partially) Test plan: 1. Open a browser tab on Koha staff and log in (to create CGISESSID cookie). You should have permission updatecharges. 2. Create a fine to any patron and get the accountlines_id. 3. Send PUT 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 PUT request to http://yourlibrary/api/v1/accountlines/ZZZ/amountpayment with body defined in definitions/partialpayAccountlineBody.json. Replace ZZZ with the borrowernumber that you created two fines to. Set amount to 10. 7. Check that the two accountlines are paid. 8. Repeat step 2. 9. Send PUT request to http://yourlibrary/api/v1/accountlines/YYY/partialpayment with body defined in definitions/partialpayAccountlineBody.json. Replace YYY with the accountlines_id you created in step 8. Set amount to half of the amount of 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
(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".
(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 ]
(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.
(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 ?
(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?
Resetting status to ASSIGNED as this needs some work
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
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
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
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".
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
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
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
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.
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...
Because of the way the Koha::Account->pay is called in the patch, this report depends especially on bug 15897.
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
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
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
Created attachment 57846 [details] [review] Bug 15165: followup - fix definitions, and passing amount parameter in pay method
I rebased the patches on master and in followup fixed same small issues - we need this in our project.
because of 15897
Dependencies are pushed to master, marking back to need signoff
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
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
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
Created attachment 59242 [details] [review] Bug 15165: followup - fix definitions, and passing amount parameter in pay method
Just a rebase
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
Created attachment 59932 [details] [review] Bug 15165: Remove meansofpayment parameter Accountlines does not have such column in current master.
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
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
(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).
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)
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>
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>
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>
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>
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>
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>
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>
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.
See last comment. Moving it to Failed QA. At least we need feedback.
(In reply to Tomás Cohen Arazi from comment #56) > 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. Well Tomas, I see one use case for it. Imagine you'd like to create a bunch of very interesting statistics, which would then help you with improving your services. That could be done for example by observing correlations between fines cardinality. Imagine 70% of all users are having specific fee twice or more times in a month. That would be an alert signal for you to come up with some campaign to reduce users' fees (e.g. free month membership if you haven't any fee last year .. that's up to you) Moving this back to Signed Off - correct me, if you disagree :)
I think it could be interesting for a library to see all types of different fines and how much is owed. XXX ILL fees XXX overdue fees XXX lost item Often fines and fees make part of a libraries budget. There is currently no place in Koha for it, but I can imagine that there exist a range of reports libraries use. I know we got one.
Hm, does it make sense to have "access" to accountlines on both endpoint?
I am concerned about that ability to directly modify accountlines. I think it would ok as long as the changes trigger an addition to the account_offsets table, and are added to action_logs if the fines log is enabled.
I guess this patch doesn't apply anymore? I tried on 17.11 and on master but with no luck. Guess it need to be rebased/updated to incorporate Mojolicious::Plugin::OpenAPI (and JSON::Validator?)?
I am writing an RFC for both credits and debits endpoints for the API. Please comment about it either on the wiki or on the mailing list. I've contacted some of you already about it. https://wiki.koha-community.org/wiki/Patrons_debits_and_credits_endpoint_RFC
I'm writing a new RFC with alex_a, khall and other's help. Please feel free to add you 2 cents! https://wiki.koha-community.org/wiki/Patrons_account_lines_endpoint_RFC
Is this a duplicate of Bug 20942 ?
(In reply to Fridolin SOMERS from comment #65) > Is this a duplicate of Bug 20942 ? Somehow. It really depends on what you're planning to do with the API. I'd say we should complete it the endpoint to add debits (minor work required to write it).
It looks like we now have routes for listing and creating credits (including payment). So we might only miss creating debits?
(In reply to Elizabeth John from comment #68) Spam
(In reply to Katrin Fischer from comment #67) > It looks like we now have routes for listing and creating credits (including > payment). So we might only miss creating debits? This is also possible now. Can this be closed or is something still missing?
Closing as I believe these routes now all exist in the API. Please reopen if I missed something here!