Defect #5700

TimelogController#destroy assumes success

Added by Eric Davis over 7 years ago. Updated over 7 years ago.

Status:ClosedStart date:2010-06-16
Priority:NormalDue date:
Assignee:Eric Davis% Done:

100%

Category:Code cleanup/refactoring
Target version:1.0.0 (RC)
Resolution:Fixed Affected version:

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.

5700.diff Magnifier (3.35 KB) Jan from Planio www.plan.io, 2010-06-20 20:27

5700.diff Magnifier (3.37 KB) Jan from Planio www.plan.io, 2010-06-20 20:47

Associated revisions

Revision 3805
Added by Eric Davis over 7 years ago

Handle unsuccessful destroys in TimelogController. #5700

Contributed by Jan

History

#1 Updated by Gregor Schmidt over 7 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'.

#2 Updated by Eric Davis over 7 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".)

#3 Updated by Jan from Planio www.plan.io over 7 years ago

patch coming in 5min

#4 Updated by Jan from Planio www.plan.io over 7 years ago

  • File 5700.diffMagnifier added
  • Assignee set to Eric Davis
  • % Done changed from 0 to 90

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.

#5 Updated by Jan from Planio www.plan.io over 7 years ago

(patch also adds check for successful flash message to the positive test)

#6 Updated by Jan from Planio www.plan.io over 7 years ago

as Eric points out, a failing callback will rather return false than raise an error, adapted the patch

#7 Updated by Eric Davis over 7 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.

Also available in: Atom PDF