|
Description
Lari Taskula
2017-06-13 11:48:26 UTC
Created attachment 64255 [details] [review] Bug 18795: DELETE (anonymize) checkout history Anonymize patron's checkout history via REST API DELETE /checkouts/history?borrowernumber=123 To test: 1. prove t/db_dependent/api/v1/checkoutshistory.t 2. Manually send DELETE request to /api/v1/checkouts/history?borrowernumber=123 where 123 is your patron's borrowernumber Created attachment 94533 [details] [review] Bug 18795: DELETE (anonymize) checkout history Anonymize patron's checkout history via REST API DELETE /checkouts/history?patron_id=123 To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Manually send DELETE request to /api/v1/checkouts/history?patron_id=123 where 123 is your patron's borrowernumber Sponsored-by: Koha-Suomi Oy Created attachment 94547 [details] [review] Bug 18795: DELETE (anonymize) checkout history Anonymize patron's checkout history via REST API DELETE /checkouts/history?patron_id=123 To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Manually send DELETE request to /api/v1/checkouts/history?patron_id=123 where 123 is your patron's borrowernumber Sponsored-by: Koha-Suomi Oy Created attachment 94548 [details] [review] Bug 18795: (follow-up) Check whether patrons exists before deleting history Running $patrons->anonymise_issue_history after checking patrons exists is more logical even though $patrons->anonymise_issue_history() can handle 0 patrons returned in the search. Also this way we don't have to run anonymise_issue_history() code unless needed, thus making the code finish faster. Created attachment 94549 [details] [review] Bug 18795: (follow-up) Unify error message to be same across codebase In Koha/REST/V1/Patrons.pm the error message for not finding patron is "Patron not found" so let's use the same here. Created attachment 94552 [details] [review] Bug 18795: (follow-up) Check whether patrons exists before deleting history Running $patrons->anonymise_issue_history after checking patrons exists is more logical even though $patrons->anonymise_issue_history() can handle 0 patrons returned in the search. Also this way we don't have to run anonymise_issue_history() code unless needed, thus making the code finish faster. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Created attachment 94555 [details] [review] Bug 18795: (follow-up) Unify error message to be same across codebase In Koha/REST/V1/Patrons.pm the error message for not finding patron is "Patron not found" so let's use the same here. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> (In reply to Johanna Räisä from comment #8) > Created attachment 94555 [details] [review] [review] > Bug 18795: (follow-up) Unify error message to be same across codebase > > In Koha/REST/V1/Patrons.pm the error message for not finding patron is > "Patron not found" so let's use the same here. > > Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> You added the error message change here also in the test (which I had forgotten) so thanks for that little change. Hi, Please rebase this patch. Bug 18795 - REST API: DELETE (anonymize) checkout history 94547 - Bug 18795: DELETE (anonymize) checkout history 94552 - Bug 18795: (follow-up) Check whether patrons exists before deleting history 94555 - Bug 18795: (follow-up) Unify error message to be same across codebase Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 18795: DELETE (anonymize) checkout history Using index info to reconstruct a base tree... M Koha/REST/V1/Checkouts.pm M api/v1/swagger/paths.json Falling back to patching base and 3-way merge... Auto-merging api/v1/swagger/paths.json CONFLICT (content): Merge conflict in api/v1/swagger/paths.json Auto-merging Koha/REST/V1/Checkouts.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 18795: DELETE (anonymize) checkout history The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-18795-DELETE-anonymize-checkout-history-G3iEhh.patch Created attachment 95246 [details] [review] Bug 18795: DELETE (anonymize) checkout history Rebased patch. Created attachment 95247 [details] [review] Bug 18795: (follow-up) Check whether patrons exists before deleting history Rebased patch. Created attachment 95248 [details] [review] Bug 18795: (follow-up) Unify error message to be same across codebase Rebased patch Created attachment 95265 [details] [review] Bug 18795: DELETE (anonymize) checkout history Anonymize patron's checkout history via REST API DELETE /checkouts/history?patron_id=123 To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Manually send DELETE request to /api/v1/checkouts/history?patron_id=123 where 123 is your patron's borrowernumber Sponsored-by: Koha-Suomi Oy Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 95266 [details] [review] Bug 18795: (follow-up) Check whether patrons exists before deleting history Running $patrons->anonymise_issue_history after checking patrons exists is more logical even though $patrons->anonymise_issue_history() can handle 0 patrons returned in the search. Also this way we don't have to run anonymise_issue_history() code unless needed, thus making the code finish faster. Created attachment 95267 [details] [review] Bug 18795: (follow-up) Unify error message to be same across codebase In Koha/REST/V1/Patrons.pm the error message for not finding patron is "Patron not found" so let's use the same here. Created attachment 95463 [details] [review] Bug 18795: DELETE (anonymize) checkout history Anonymize patron's checkout history via REST API DELETE /checkouts/history?patron_id=123 To test: 1. prove t/db_dependent/api/v1/checkouts.t 2. Manually send DELETE request to /api/v1/checkouts/history?patron_id=123 where 123 is your patron's borrowernumber Sponsored-by: Koha-Suomi Oy Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com> Created attachment 95464 [details] [review] Bug 18795: (follow-up) Check whether patrons exists before deleting history Running $patrons->anonymise_issue_history after checking patrons exists is more logical even though $patrons->anonymise_issue_history() can handle 0 patrons returned in the search. Also this way we don't have to run anonymise_issue_history() code unless needed, thus making the code finish faster. Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com> Created attachment 95465 [details] [review] Bug 18795: (follow-up) Unify error message to be same across codebase In Koha/REST/V1/Patrons.pm the error message for not finding patron is "Patron not found" so let's use the same here. Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com> Hi Johanna. Nice and clean work!
A few remarks from the QA POV:
- Our API design mojo is: 'As RESTful as we can'. In this context, we should consider the 'checkout history of a patron' a resource, and as such it feels more appropriate to put the patron id on the path, like in:
DELETE /patrons/{patron_id}/checkouts/history
Then we can reserve this route you were using for batch anonymizing :-D like in
DELETE /checkouts/history?q=more_complex_query_on_patrons
- There are some response definitions missing on the spec, (503 for maintenance mode, 500 for internal server error, and so on).
- Put the tests on a separate subtest please to avoid interference if some changes are made to the previous tests/data.
(In reply to Tomás Cohen Arazi from comment #20) > Hi Johanna. Nice and clean work! > > A few remarks from the QA POV: > - Our API design mojo is: 'As RESTful as we can'. In this context, we should > consider the 'checkout history of a patron' a resource, and as such it feels > more appropriate to put the patron id on the path, like in: > > DELETE /patrons/{patron_id}/checkouts/history > > Then we can reserve this route you were using for batch anonymizing :-D like > in > > DELETE /checkouts/history?q=more_complex_query_on_patrons > > - There are some response definitions missing on the spec, (503 for > maintenance mode, 500 for internal server error, and so on). > > - Put the tests on a separate subtest please to avoid interference if some > changes are made to the previous tests/data. Hi Tomás, now that I'm back in business I will look those things as you suggested. |