Bug 18795 - REST API: DELETE (anonymize) checkout history
Summary: REST API: DELETE (anonymize) checkout history
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Johanna Räisä
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 17005
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-13 11:48 UTC by Lari Taskula
Modified: 2021-01-07 12:55 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18795: DELETE (anonymize) checkout history (6.12 KB, patch)
2017-06-13 11:50 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18795: DELETE (anonymize) checkout history (4.21 KB, patch)
2019-10-22 11:47 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 18795: DELETE (anonymize) checkout history (4.20 KB, patch)
2019-10-22 12:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 18795: (follow-up) Check whether patrons exists before deleting history (1.21 KB, patch)
2019-10-22 12:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 18795: (follow-up) Unify error message to be same across codebase (892 bytes, patch)
2019-10-22 12:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 18795: (follow-up) Check whether patrons exists before deleting history (1.27 KB, patch)
2019-10-22 13:14 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 18795: (follow-up) Unify error message to be same across codebase (1.43 KB, patch)
2019-10-22 13:15 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 18795: DELETE (anonymize) checkout history (4.17 KB, patch)
2019-11-11 06:41 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 18795: (follow-up) Check whether patrons exists before deleting history (1.22 KB, patch)
2019-11-11 06:42 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 18795: (follow-up) Unify error message to be same across codebase (1.38 KB, patch)
2019-11-11 06:43 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 18795: DELETE (anonymize) checkout history (4.32 KB, patch)
2019-11-11 13:58 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 18795: (follow-up) Check whether patrons exists before deleting history (1.21 KB, patch)
2019-11-11 13:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 18795: (follow-up) Unify error message to be same across codebase (1.37 KB, patch)
2019-11-11 13:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 18795: DELETE (anonymize) checkout history (4.35 KB, patch)
2019-11-15 20:34 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 18795: (follow-up) Check whether patrons exists before deleting history (1.26 KB, patch)
2019-11-15 20:34 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 18795: (follow-up) Unify error message to be same across codebase (1.42 KB, patch)
2019-11-15 20:34 UTC, Bouzid Fergani
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2017-06-13 11:48:26 UTC
Anonymize patron's checkout history via REST API

DELETE /checkouts/history?borrowernumber=123
Comment 1 Lari Taskula 2017-06-13 11:50:32 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
Comment 2 Lari Taskula 2017-06-13 11:52:21 UTC
Doesn't apply at the moment and needs to be rebased once Bug 18137 and Bug 17005 move forward.
Comment 3 Johanna Räisä 2019-10-22 11:47:57 UTC
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
Comment 4 Joonas Kylmälä 2019-10-22 12:59:15 UTC
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
Comment 5 Joonas Kylmälä 2019-10-22 12:59:19 UTC
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.
Comment 6 Joonas Kylmälä 2019-10-22 12:59:24 UTC
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.
Comment 7 Johanna Räisä 2019-10-22 13:14:48 UTC
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>
Comment 8 Johanna Räisä 2019-10-22 13:15:59 UTC
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>
Comment 9 Joonas Kylmälä 2019-10-22 13:25:55 UTC
(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.
Comment 10 Bouzid Fergani 2019-11-08 21:38:37 UTC
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
Comment 11 Johanna Räisä 2019-11-11 06:41:17 UTC
Created attachment 95246 [details] [review]
Bug 18795: DELETE (anonymize) checkout history

Rebased patch.
Comment 12 Johanna Räisä 2019-11-11 06:42:19 UTC
Created attachment 95247 [details] [review]
Bug 18795: (follow-up) Check whether patrons exists  before deleting history

Rebased patch.
Comment 13 Johanna Räisä 2019-11-11 06:43:03 UTC
Created attachment 95248 [details] [review]
Bug 18795: (follow-up) Unify error message to be same  across codebase

Rebased patch
Comment 14 Joonas Kylmälä 2019-11-11 13:58:59 UTC
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>
Comment 15 Joonas Kylmälä 2019-11-11 13:59:02 UTC
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.
Comment 16 Joonas Kylmälä 2019-11-11 13:59:05 UTC
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.
Comment 17 Bouzid Fergani 2019-11-15 20:34:31 UTC
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>
Comment 18 Bouzid Fergani 2019-11-15 20:34:37 UTC
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>
Comment 19 Bouzid Fergani 2019-11-15 20:34:45 UTC
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>
Comment 20 Tomás Cohen Arazi 2020-03-20 14:50:41 UTC
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.
Comment 21 Johanna Räisä 2021-01-07 12:55:58 UTC
(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.