Bug 27145

Summary: Patron deletion via intranet doesn't handle exceptions well
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: PatronsAssignee: Kyle M Hall (khall) <kyle>
Status: CLOSED FIXED QA Contact: Joonas Kylmälä <joonas.kylmala>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, joonas.kylmala, kyle.m.hall, kyle, martin.renvoize, victor
Version: Main   
Hardware: All   
OS: All   
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
Circulation function:
Bug Depends on: 14708    
Bug Blocks:    
Attachments: Bug 27145: Rethrow all other exceptions
Bug 27145: Wrap delete and move_to_deleted in a transaction
Bug 27145: Rethrow all other exceptions
Bug 27145: Wrap delete and move_to_deleted in a transaction
Bug 27145: Rethrow all other exceptions
Bug 27145: Wrap delete and move_to_deleted in a transaction

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 (khall) 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 (khall) 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 (ashimema) 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 (ashimema) 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 (ashimema) 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 (khall) 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.