Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add #flipper_enabled? to ActiveRecord models #755

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

bkeepers
Copy link
Collaborator

@bkeepers bkeepers commented Aug 30, 2023

Adds #flipper_enabled? to all ActiveRecord models as shorthand for Flipper.enabled?(:feature_name, model)…with a bonus…

One thing that has always felt awkward is that I'm never quite sure who/what to use as an actor when building a new feature. Is it user-centric? Or some other other model like Organization or Project? In #710 we added the ability to check multiple actors at once, which is great, but it's still awkward to always have to have Flipper.enabled?(:feature, current_user, *current_user&.organizations) sprinkled throughout the code.

With this change, you can define #flipper_actors on any model, and calling #flipper_enabled? will check the feature against the relevant actors.

class User < ApplicationRecord
  has_many :memberships
  has_many :organizations, through: :memberships

  def flipper_actors
    [self] + organizations
  end
end

So now current_user.flipper_enabled?(:new_feature) will check if it's enabled for a specific user or any organization that they are part of.

@bkeepers bkeepers changed the title Add #enabled? to ActiveRecord models Add #flipper_enabled? to ActiveRecord models Aug 30, 2023

# Check if a feature is enabled for this record or any associated actors
# returned by `#flipper_actors`
def flipper_enabled?(feature_name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One interesting thing with this, you could have user and org. In user you override flipper_actors but in org you can't because you don't know the specific user. I think that is part of my concern with adding flipper_actors and this method name (even though it is now scoped).

I want the convenience but also don't want it to be too sharp for users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One interesting thing with this, you could have user and org. In user you override flipper_actors but in org you can't because you don't know the specific user.

I'm not sure I understand. If you have access to a user, you would check against that instead of the organization.

current_user.flipper_enabled?(:thing)

If not, then you check against just the organization:

current_org.flipper_enabled?(:thing)

Either way, enabling the feature for the organization would make both checks pass:

Flipper.enable(:thing, current_org)

Every feature check would benefit from the user knowing it's related actors.

I want the convenience but also don't want it to be too sharp for users.

By default the behavior doesn't change. It's only if you explicitly overwrite #flipper_actors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the feature is enable for 1 of the users organizations, then wouldn't they see it enabled on all of their organizations? That doesn't seem correct.

@jnunemaker
Copy link
Collaborator

I'm not sure I understand.

I just meant if you have this:

class User < ApplicationRecord
  has_many :memberships
  has_many :organizations, through: :memberships

  def flipper_actors
    [self] + organizations
  end
end

class Organization < ApplicationRecord
  def flipper_actors
    [self] # no way to add user here so its the same multi actor as user
  end
end

user = User.find(...) # user is member of org
org = Org.find(...)
Flipper.enable(:foo, org)

user.feature_enabled?(:foo) # => true
org.feature_enabled?(:foo) # => true

Both of those work. But they are not symmetrical. If you enable user instead of org the result is different.

Flipper.disable(:foo, org)
Flipper.enable(:foo, user)

user.feature_enabled?(:foo) # => true
org.feature_enabled?(:foo) # => false

Looking at it, it only works as hierarchy and when using the lowest level. But if you check from higher levels you'll have to really know what's going on.

Maybe because users are a very common actor and usually the lowest level in a given hierarchy this will end up fine?

@bkeepers
Copy link
Collaborator Author

Looking at it, it only works as hierarchy and when using the lowest level. But if you check from higher levels you'll have to really know what's going on.

Gotcha, I guess I see that as desired behavior. I often want to enable a feature for aUser (myself or specific users) before other actor models like Project or Organization. I also expect that enabling a feature for an Organization will enable it for all users in that organization.

@jnunemaker
Copy link
Collaborator

Interesting! Let’s decide on it next week. Only other concern is one more thing to search for when finding flag usage.

@jnunemaker
Copy link
Collaborator

Seeing #783 made me think if we do this PR we could also add flipper_features to return all enabled features for an actor or a Hash of all features with key pointed at true/false.

@jnunemaker
Copy link
Collaborator

@bkeepers I was going through old PRs today. I say we merge this and see if people like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants