Defect #5700
closed
TimelogController#destroy assumes success
Added by Eric Davis over 14 years ago.
Updated over 14 years ago.
Category:
Code cleanup/refactoring
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
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'.
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".)
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.
(patch also adds check for successful flash message to the positive test)
as Eric points out, a failing callback will rather return false than raise an error, adapted the patch
- 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.
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by