Bug 27145 - Patron deletion via intranet doesn't handle exceptions well
Summary: Patron deletion via intranet doesn't handle exceptions well
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 14708
Blocks:
  Show dependency treegraph
 
Reported: 2020-12-03 13:35 UTC by Joonas Kylmälä
Modified: 2022-12-12 21:23 UTC (History)
8 users (show)

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


Attachments
Bug 27145: Rethrow all other exceptions (1.59 KB, patch)
2020-12-03 14:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27145: Wrap delete and move_to_deleted in a transaction (1.28 KB, patch)
2020-12-04 11:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27145: Rethrow all other exceptions (1.64 KB, patch)
2021-11-11 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27145: Wrap delete and move_to_deleted in a transaction (1.33 KB, patch)
2021-11-11 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27145: Rethrow all other exceptions (1.78 KB, patch)
2021-11-13 14:17 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27145: Wrap delete and move_to_deleted in a transaction (1.48 KB, patch)
2021-11-13 14:17 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2020-12-03 13:35:48 UTC
Bug 14708 introduced a try catch around $patron->delete in commit "Bug 14708: (QA follow-up) Use try/catch blocks when calling
". However, in the catch block it only assumes the exception was from trying to delete anonymous patron when it can be anything else as well, the code should be modified so that it will handle the anonymous patron case and if it is anything else we log the other exception.

Kyle, can you have a go at this?
Comment 1 Kyle M Hall 2020-12-03 14:15:07 UTC
Created attachment 114149 [details] [review]
Bug 27145: Rethrow all other exceptions

Bug 14708 introduced a try catch around $patron->delete in commit "Bug 14708: (QA follow-up) Use try/catch blocks when calling
". However, in the catch block it only assumes the exception was from trying to delete anonymous patron when it can be anything else as well, the code should be modified so that it will handle the anonymous patron case and if it is anything else we log the other exception.
Comment 2 Jonathan Druart 2020-12-04 09:53:30 UTC
We should move $patron->move_to_deleted into the try and execute the two statements inside a transaction.
Comment 3 Kyle M Hall 2020-12-04 11:56:00 UTC
Created attachment 114183 [details] [review]
Bug 27145: Wrap delete and move_to_deleted in a transaction
Comment 4 Martin Renvoize 2021-11-11 15:13:52 UTC
Created attachment 127545 [details] [review]
Bug 27145: Rethrow all other exceptions

Bug 14708 introduced a try catch around $patron->delete in commit:
  "Bug 14708: (QA follow-up) Use try/catch blocks when calling"

However, in the catch block it only assumes the exception was from trying to
delete anonymous patron when it can be anything else as well, the code should
be modified so that it will handle the anonymous patron case and if it is
anything else we log the other exception.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-11-11 15:13:55 UTC
Created attachment 127546 [details] [review]
Bug 27145: Wrap delete and move_to_deleted in a transaction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-11-11 15:15:52 UTC
The change makes allot of sense, though it would be nice to do something with the other types of failures we're currently rethrowing.. but it's no bad thing to rethrow for now and collect data on what they are so we can handle them later (in new bug reports).. in my opinion.

Signing off.
Comment 7 Joonas Kylmälä 2021-11-13 14:17:10 UTC
Created attachment 127608 [details] [review]
Bug 27145: Rethrow all other exceptions

Bug 14708 introduced a try catch around $patron->delete in commit:
  "Bug 14708: (QA follow-up) Use try/catch blocks when calling"

However, in the catch block it only assumes the exception was from trying to
delete anonymous patron when it can be anything else as well, the code should
be modified so that it will handle the anonymous patron case and if it is
anything else we log the other exception.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 8 Joonas Kylmälä 2021-11-13 14:17:14 UTC
Created attachment 127609 [details] [review]
Bug 27145: Wrap delete and move_to_deleted in a transaction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 9 Jonathan Druart 2021-11-15 11:39:15 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 10 Kyle M Hall 2021-11-19 16:35:01 UTC
Pushed to 21.05.x for 21.05.05
Comment 11 Fridolin Somers 2021-11-20 00:12:37 UTC
Pushed to 20.11.x for 20.11.12
Comment 12 Victor Grousset/tuxayo 2021-11-22 01:41:18 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.