ckdake / setler

Setler lets you use the 'Feature Flags' pattern or add settings to models in Rails
MIT License
84 stars 21 forks source link

Setting defaults for ActiveRecord model don't take #16

Closed stevenharman closed 10 years ago

stevenharman commented 12 years ago
class User < ActiveRecord::Base
  has_setler :preferences
end

class Preferences < Setler::Settings
end

# in an initializer
Preferences.defaults[:beta_notice] = true

After creating a new user:

> User.last.preferences.beta_notice
 Setler::ScopedSettings Load (0.7ms)  SELECT "preferences".* FROM "preferences" WHERE "preferences"."var" = 'bid_notification' AND (("preferences"."thing_type" = 'User' AND "preferences"."thing_id" = 7)) LIMIT 1
=> nil
>
ckdake commented 12 years ago

Hmm. Might this be an implication of https://github.com/ckdake/setler/commit/b07b09dc15c3635b24456231d067b68df8fc08a4 ? Could you try 0.0.7 and see if this issue shows up for you there?

stevenharman commented 12 years ago

Downgrading does bring back defaults. Of course, now a ton of deprecation warnings show up, but that's another story.

ckdake commented 12 years ago

Doh. @coreyti, want to take a look at this?

coreyti commented 12 years ago

Ack. will do.

ckdake commented 12 years ago

Added a failing test for this in e8ffb5ad9abef93f9462bec94d9e4aa886b40230

ckdake commented 12 years ago

Did some tinkering around with class instance variables and a different way of setting defaults, but neither one does the trick. Someone with more ruby-foo than I may see exactly the right thing to do. Take a look at https://github.com/ckdake/setler/tree/fixing_defaults, currently it is just 0164867d7480b2617b8dd9f2871f34a28c078ba1 and 70c71a52f6529b0a21556336c076824f519f8202

ckdake commented 12 years ago

No fix on this, so I pushed a new version that restores the old behavior because people were counting on it. The fixing_defaults branch remains, I'd love some feedback from anyone on how to make that happen.

rheaton commented 12 years ago

Hi Chris-- I'm working on this issue for @coreyti (we are working on the same project). I am looking at the failing test you added. Would you like the setting of scoped preferences to behave like the most recent test you added? I am going to work from there!

  def test_user_preferences_has_defaults
    user = User.create name: 'user 1'
    assert_equal User.Preferences.all, user.preferences.all
    User.Preferences.defaults[:foo] = true
    assert_equal User.Preferences.all, user.preferences.all
    assert user.preferences.foo
  end
rheaton commented 12 years ago

Hi @ckdake I actually am going to have it default to the top-level scope.

e.g.

::Preferences.defaults[:foo] = "default"
user = User.create name: 'user 1'
assert user.preferences.foo == ::Preferences.defaults[:foo]

user.preferences.foo = "override"
assert user.preferences.foo == "override"
assert user.preferences.foo != ::Preferences.defaults[:foo]
rheaton commented 12 years ago

I have another question... should top-level settings override the defaults for scoped settings?

For example:

class User
  has_setler :settings
end

Settings.defaults[:foo] = "bar"
u = User.create
assert_equal u.settings.foo, "bar"
Settings.foo = "baz"

u.settings.foo = ???? 

I was thinking it would equal "bar" and not "baz" as I think it is what people would expect, but I'm not sure.

ckdake commented 12 years ago

Everything here works as expected, except for that failing test. I think this is going to be a pretty tricky thing to solve elegantly, but look forward to seeing what you come up with.

Your second comment looks inline with what I have in mind. Your 3rd comment is a bit of a grey area and I could see it going either way, but it seems like having it be "baz" would be the way to go.

rheaton commented 12 years ago

Alright, I will make the user setting be "baz" I will add something to the readme so people can easily understand the precedence.