Feature should be enabled even for nil/false recipient if it is deployed to 100% #29

Closed
grosser opened this Issue Dec 6, 2012 · 13 comments

Comments

Projects
None yet
3 participants
Owner

grosser commented Dec 6, 2012

would be useful for a case like

Arturo.feature_enabled_for?(:foo, current_user)

where it would be slowly rolled out and then when it hits 100% is also rolled out for nil users,
otherwise it would be at 100% but still be not fully rolled out

https://github.com/jamesarosen/arturo/blob/master/app/models/arturo/feature.rb#L42

if you agree I'd make a pull-request

Collaborator

plukevdh commented Dec 6, 2012

While I understand the need for this, I don't think this is how I'd want
to address it. What I would suggest for now in your own code is create
something like

  def feature_enabled?(feature)
    feature.deployed_percentage == 100
  end

in your own code instead, since really feature_enabled_for? is phrased
specifically for checking specific users. Allowing for no providing a
user could add weird edge cases. If you like, add this helper method to
Arturo and PR so we can discuss.

Michael Grosser wrote:

would be useful for a case like

|Arturo.feature_enabled_for?(:foo, current_user)
|

where it would be slowly rolled out and then when it hits 100% is also
rolled out for nil users,
otherwise it would be at 100% but still be not fully rolled out

https://github.com/jamesarosen/arturo/blob/master/app/models/arturo/feature.rb#L42

if you agree I'd make a pull-request


Reply to this email directly or view it on GitHub
jamesarosen#29.

Owner

grosser commented Dec 6, 2012

My issue is that I have a mix of existing uses and nil users, I still want the slow rollout, but I also want the feature to be enabled for everyone once it hits 100%, atm I'm doing this by hand, user ? Arturo.feature_enabled_for?(:foo, user) : check_100_percent(:foo)

Collaborator

plukevdh commented Dec 6, 2012

I think that is a very specific use case for you then. I don't
understand the concept of nil users and I don't see adding an edge case
like that to a general purpose tool like arturo being useful for most
people, especially when someone might want it to fail for the opposite
reason.

Michael Grosser wrote:

My issue is that I have a mix of existing uses and nil users, I still
want the slow rollout, but I also want the feature to be enabled for
everyone once it hits 100%, atm I'm doing this by hand, |user ?
check_100_percent(:foo) : Arturo.feature_enabled_for?(:foo, user)|

Owner

grosser commented Dec 6, 2012

my logic was along the lines of: if something is enabled for everyone/100% it should be enabled no matter what I pass to feature_enabled_for?

Collaborator

plukevdh commented Dec 6, 2012

I can understand the reasoning, I'm just hesitant to change the current
implementation since the method explicitly requires a user
(feature_enabled_for implies it's checking for a user) and if a user is
not found, I agree that it should fail.

I'll let others weigh in though first.

Michael Grosser wrote:

my logic was along the lines of: if something is enabled for
everyone/100% it should be enabled no matter what I pass to
|feature_enabled_for?|


Reply to this email directly or view it on GitHub
jamesarosen#29 (comment).

Owner

grosser commented Dec 6, 2012

FYI: My current solution, still ugly but a little better...

Arturo.feature_enabled_for?(:foo, current_user || User.new { |u| u.id = 1 })
Contributor

jamesarosen commented Mar 3, 2013

@grosser What about a default_recipient that itself defaults to nil (current behavior) but you can set to User.new { |u| u.id = 1 }?

Owner

grosser commented Mar 3, 2013

I'd rather support passing new objects with id=nil for code that uses new users as anonymous users

Contributor

jamesarosen commented Mar 4, 2013

Feature#enabled_for? returns false if the recipient is nil, and Feature#passes_threshold? returns true if the deployment_percentage is 100, regardless of the recipient's id. You can pass User.new to a fully rolled-out feature and it will return true.

See https://github.com/jamesarosen/arturo/blob/master/app/models/arturo/feature.rb#L45 and https://github.com/jamesarosen/arturo/blob/master/app/models/arturo/feature.rb#L72

Owner

grosser commented Mar 4, 2013

Ups, my bad had that confused with the other issue...
Basically my reasoning was that the feature is on / available for all, so it should also be available for nil/false/everything. Because if it is not and I remove the "if enabled" code things will start to blow up, because the code never ran with nil even though it was on 100%

Contributor

jamesarosen commented Mar 4, 2013

The problem is one of semantics. I read "deployment percentage: 100%" as "deployed to all users," not "deployed to all users and non-users."

Owner

grosser commented Mar 4, 2013

I think of Arturo mainly as a test for new features and I always want to
be certain that nothing unexpected happens when I go from a 100% deployed
feature to removing the feature flag (the "if" around my new code),
to achieve this deployed should be true for whatever I pass when I reach
100%

On Sun, Mar 3, 2013 at 4:29 PM, James Rosen notifications@github.comwrote:

The problem is one of semantics. I read "deployment percentage: 100%" as
"deployed to all users," not "deployed to all users and non-users."


Reply to this email directly or view it on GitHubhttps://github.com/jamesarosen/arturo/issues/29#issuecomment-14359255
.

Contributor

jamesarosen commented Mar 4, 2013

The fundamental problem with your proposed change is that there's no way to get the old behavior back, whereas it's relatively easy to get the behavior you want now:

Arturo.feature_recipient do
  current_user || User.new { |u| u.id = 0 }
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment