Defect #5700
closedTimelogController#destroy assumes success
100%
Description
TimelogController#destroy assumes that deleting the TimeEntry always succeeds. It should check if @time_entry.destroy
was successful and change the flash message based on that.
Files
Updated by Gregor Schmidt over 14 years ago
The only way, this line could fail, is when you have lost the db connection. I'm not sure if we are able to handle that gracefully.
This is also reflected in the implementation of destroy The only thing that could bubble up is a general ActiveRecord error.
I think this is a 'won't fix'.
Updated by Eric Davis over 14 years ago
Gregor Schmidt wrote:
I think this is a 'won't fix'.
I disagree, it's easy for plugins to add callbacks to Models like before_delete. It's a UI inconsistency if Redmine is told to delete a record, it doesn't (for whatever reason), and the flash says it was deleted.
(Or alternatively) If Redmine itself ever did something in before_delete then someone will have to remember to change the controller because of the hard coded 'success' message. (e.g. if Redmine adds a permission for "Allowed to delete time entries".)
Updated by Jan from Planio www.plan.io over 14 years ago
patch which introduces an error message. includes a test that simulates a failing before_destroy
callback.
please check and commit if you think it's okay.
Updated by Jan from Planio www.plan.io over 14 years ago
(patch also adds check for successful flash message to the positive test)
Updated by Jan from Planio www.plan.io over 14 years ago
as Eric points out, a failing callback will rather return false than raise an error, adapted the patch
Updated by Eric Davis over 14 years ago
- Status changed from New to Closed
- Target version set to 1.0.0 (RC)
- % Done changed from 90 to 100
- Resolution set to Fixed
Applied in r3805, thank you for the contribution.