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

Introduce enable/disable experiment cohorting #615

Merged
merged 2 commits into from
May 27, 2020

Conversation

robin-phung
Copy link
Contributor

What problem does this solve?

Disabling Cohorting: Allow existing registered participants time to convert while no longer accepting new participants into the experiment. New participants will be given the control will be given the control and will not be recorded as being apart of the experiment.

Why is this useful?

  • Gives implementers the ability to soft close experiments, allowing shifting new participants into other experiments before concluding the result of the experiment (selecting a winner).
  • Allows for better accuracy of data, ~> allows participants the chance to convert if they engaged late before the end of the experiment.

How does this solve it?

  • Adds enable/disable button to the dashboard
  • Provides a brief alert description describing the resulting behaviour when performed.
  • Adds functions to the experiment object to check and mutate the flag stored in redis.
  • Deletion of the experiment will result in the deletion of the flag (resetting it back to its default state = cohorting enabled)
  • Continues to perform appropriate callbacks if the user is already an existing participant to the experiment.
  • Performs no callbacks if the participant is first entering the experiment for the first time when cohorting is disabled.
  • existing overrides and disables take precedence

Screen Shot 2020-04-23 at 10 20 26 PM

@andrehjr
Copy link
Member

andrehjr commented May 11, 2020

Thanks for the Pull Request @robin-phung ✨ I wanna do some testing just to make sure it's all good before merging.

The Trial#choose! method is a bit complex as the way it is right now(before your PR), I have some plans to refactor it.

@robin-phung
Copy link
Contributor Author

@andrehjr Let me know what your thoughts are on refactoring as I'm also interested (and may be able to assist/collab) in a refactor of the Helper#ab_test ~> Trial#choose!.

Copy link
Member

@andrehjr andrehjr left a comment

Choose a reason for hiding this comment

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

Just a few changes and we should be good to merge. 🚀 Sorry for taking so long.

Being able to 'soft close' an experiment does look useful to give more control over experiments.

lib/split/trial.rb Show resolved Hide resolved
lib/split/dashboard/views/_controls.erb Outdated Show resolved Hide resolved
lib/split/trial.rb Outdated Show resolved Hide resolved
@andrehjr
Copy link
Member

@andrehjr Let me know what your thoughts are on refactoring as I'm also interested (and may be able to assist/collab) in a refactor of the Helper#ab_test ~> Trial#choose!.

Sure! Help is always welcome. I've started the other day a few spikes starting with the ab_test method in #623 in order to start preparing things.

@robin-phung
Copy link
Contributor Author

Updated PR with the following changes:

  • Memoizes cohorting_disabled to prevent repeated calls to redis when calling #cohorting_disabled?
  • Places enable/disable cohorting buttons within the else condition of #has_winner? ~> only displays the buttons when the experiment does not have a winner.
  • Places the condition logic of new_participant && @experiment.cohorting_disabled? into a variable new_participant_and_cohorting_disabled

@robin-phung robin-phung requested a review from andrehjr May 27, 2020 02:14
@andrehjr
Copy link
Member

@robin-phung can you rebase these changes? 18c0514 is already on master 648576a

And that's it! 🚀

@robin-phung robin-phung force-pushed the master branch 2 times, most recently from f06b16b to f9fc878 Compare May 27, 2020 18:21
@robin-phung
Copy link
Contributor Author

My rebase went out of whack, it should be resolved now.

@andrehjr andrehjr merged commit 482e700 into splitrb:master May 27, 2020
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.

2 participants