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

Implementing deactivate user #133

Closed
wants to merge 14 commits into from

Conversation

LikitSaeLee
Copy link

reference: #87

IMO, The rules should be evaluated by specificity.

  1. User specific rule.
  2. Group rule.
  3. Percentage rule

Example:

$rollout.activate_group(:chat, :all)
$rollout.deactivate_user(:chat, user)
$rollout.active?(:chat, user) # false, because user specific rule takes highest precedence
$rollout.activate_group(:chat, :group_including_user)
$rollout.deactivate_user(:chat, user)
$rollout.activate_group(:chat, :another_group_including_user)
$rollout.active?(:chat, user) # false, because user specific rule takes highest precedence
$rollout.deactivate_group(:chat, :group_including_user)
$rollout.activate_user(:chat, user)
$rollout.active?(:chat, user) # true, because user specific rule takes highest precedence

Changes:

  1. Denote deactivated user id in feature#users by appending x in user_id.
  2. A single user id can only be added (A user cannot be both active and deactivated)
  3. Proper implementing the rules above.
  4. Add Rollout#remove_user so that the user_id/user specific rule can be removed ( for reducing memory usage on redis ).

@cohendvir
Copy link

hey @LikitSaeLee, is this pr going somewhere? :)
we just had a bug in a feature we deployed for 10% of our users, and we had to roll it back completely because we couldn't turn it off for that one user..
This pr could have been really helpful right now!

@raghuvarmabh
Copy link

any update on this. I am also in the same boat.

@ErebusBat
Copy link

As @reneklacan stated in #87 (comment):

I'm kind of afraid this would introduce even more unexpected behaviour if it's not well thought out.

This PR has clearly defined rules; however this PR does change the well defined and understood behavior of existing functions. For that reason alone I would be hesitant to give this PR a 👍.

we just had a bug in a feature we deployed for 10% of our users, and we had to roll it back completely because we couldn't turn it off for that one user.

The way we have addressed this in the past has been to create a specific disabling flag rollout. While this can seem messy at some points it is actually quite nice because after all the bugs are worked out the disabling rollout can be 100% deleted. If you keep the original feature rollout then you can check the activation percentage and know, without a doubt, that it is enabled for everyone.

@reneklacan reneklacan closed this Jun 29, 2019
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.

None yet

5 participants