Refactor string eval for scope_condition

[brendon]

This code needs to be looked at:

define_method :scope_condition do
  eval "%{#{configuration[:scope]}}"
end

Some work has been done here previously by @rdvdijk: #215

Any thoughts @swanandp?

[brendon]

Personally I'm not sure how we could work around this. Supplying the scope as a string is very handy sometimes. The only way this is insecure is if the developer wants to shoot themselves in the foot and supply a silly scope condition. I don't think there's a way to have the scope condition interfered with from outside the app code since this is all evaluated at boot time.

[krisleech]

I not sure if this is related but I used acts_as_list scope: 'study_id' and found the scoping not work. I had to use a symbol instead of a string which I found surprising.

[brendon]

Yes, it is a bit counter-intuitive. The string/symbol thing has been around as long as the library as far as I know. One of my contractors is doing some refactor of the structure of acts_as_list and he's found this weird also. We might look at modifying the behaviour but it would break a lot of existing users code as they may be depending on the string functionality.

@ledestin what were your thoughts on this again?

[ledestin]

If an array is passed, it's treated differently from a scalar. You could code that strings aren't changed, but to a Symbol, _id could be added. But as noted above, it'd be confusing. A way out of this could be to look for a =, if present, it's a WHERE query, if not, it's a column.

[brendon]

That's an idea. It starting to get very subtle though. I just remembered the conversation we had about inspecting the models Reflections to determine wether or not to add an _id. In the case of a string, many use this to supply their own more complicated scopes. They then also override scope_changed? to ensure a change is picked up. Here's one example where I use it (though without passing a string):

  acts_as_list :add_new_at => :top

  def scope_condition
    ['notice_area_id = ? AND ? >= CURDATE()', notice_area_id, end_date.to_s(:db)]
  end

  # A custom acts_as_list scope requires a custom scoped_changed? method
  def scope_changed?
    changed.include?('notice_area_id') ||
    changed.include?('end_date') && (
      changes['end_date'][0] >= Time.zone.now.beginning_of_day &&
      changes['end_date'][1] < Time.zone.now.beginning_of_day ||
      changes['end_date'][1] >= Time.zone.now.beginning_of_day &&
      changes['end_date'][0] < Time.zone.now.beginning_of_day
    )
  end

[brendon]

Closing this for now.