On Thu, Oct 04, 2012 at 09:30:47AM -0400, [email protected] wrote: > From: Tzu-Mainn Chen <[email protected]> > > --- > src/app/models/event.rb | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/src/app/models/event.rb b/src/app/models/event.rb > index 5d17380..22a0c4a 100644 > --- a/src/app/models/event.rb > +++ b/src/app/models/event.rb > @@ -47,6 +47,14 @@ class Event < ActiveRecord::Base > > scope :lifetime, where(:status_code => [:first_running, :all_running, > :some_running, :all_stopped]) > > + def source > + case source_type > + when "Deployment" then Deployment.unscoped.find(source_id) > + when "Instance" then Instance.unscoped.find(source_id) > + else super > + end > + end
While your code is just fine, I really dislike that ".unscoped" is used to mean "with deleted". It's not the least bit intuitive. This appears to be how paranoia does it, so I'm certainly not pointing a finger at you. Beyond the immediate readability issues, I worry that down the road, someone well-meaning is going to delete this method, pointing out that the "source" association works out of the box without us having to write custom code. They're not going to remember that "unscoped" overrides a default scope of only non-deleted items. (This person may very well be me a couple of months down the road...) I'm going to send a quick patch for consideration, which gives Paranoid objects a "with_deleted" method. Thus you could rewrite your code to be: ... when "Deployment" then Deployment.with_deleted.find(source_id) ... And then it should be abundantly clear what it does and why it's there. Your code does work fine and all tests pass, so if you don't like the patch I send, can we just add a comment to your method, even just the text of your commit message? -- Matt
