[PATCH] allow event to find source even if source has been soft deleted

Matt Wagner matt.wagner at redhat.com
Fri Oct 5 19:10:00 UTC 2012


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



More information about the aeolus-devel mailing list