Project

General

Profile

Actions

Defect #5700

closed

TimelogController#destroy assumes success

Added by Eric Davis almost 14 years ago. Updated over 13 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Code cleanup/refactoring
Target version:
Start date:
2010-06-16
Due date:
% Done:

100%

Estimated time:
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.


Files

5700.diff (3.35 KB) 5700.diff Jan from Planio www.plan.io, 2010-06-20 20:27
5700.diff (3.37 KB) 5700.diff Jan from Planio www.plan.io, 2010-06-20 20:47
Actions #1

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

Actions #2

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

Actions #3

Updated by Jan from Planio www.plan.io over 13 years ago

patch coming in 5min

Actions #4

Updated by Jan from Planio www.plan.io over 13 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.

Actions #5

Updated by Jan from Planio www.plan.io over 13 years ago

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

Actions #6

Updated by Jan from Planio www.plan.io over 13 years ago

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

Actions #7

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

Actions

Also available in: Atom PDF