On Thu, Oct 04, 2012 at 09:30:47AM -0400, tzumainn(a)redhat.com wrote:
From: Tzu-Mainn Chen <tzumainn(a)redhat.com>
---
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