-
Notifications
You must be signed in to change notification settings - Fork 481
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
User based experiment: create end points and tests #22193
Conversation
|
||
# GET /experiments/set_single_user_experiment/:experiment_name | ||
def set_single_user_experiment | ||
valid_experiments = ['2018-teacher-experience'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a better place for this constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be more standard to define it on the class:
VALID_EXPERIMENTS = [...]
# ...
def set_single_user_experiment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
# GET /experiments/set_single_user_experiment/:experiment_name | ||
def set_single_user_experiment | ||
valid_experiments = ['2018-teacher-experience'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be more standard to define it on the class:
VALID_EXPERIMENTS = [...]
# ...
def set_single_user_experiment
def disable_single_user_experiment | ||
experiment_name = params[:experiment_name] | ||
|
||
unless Experiment.enabled?(experiment_name: experiment_name, user: current_user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: could be nice to first warn if the experiment is not valid again, to get a better error message in case of accidentally truncated urls
|
||
test_user_gets_response_for( | ||
:disable_single_user_experiment, | ||
name: 'single user cannot set disable experiment they are not in', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cannot set disable experiment
assert_nil flash[:alert] | ||
assert_includes flash[:notice], "You have successfully disabled the experiment" | ||
assert_nil Experiment.first | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
end | ||
|
||
experiment = SingleUserExperiment.where(min_user_id: current_user.id, name: experiment_name).first | ||
experiment.destroy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible that the user was in the experiment through another experiment type, in which case experiment
would be nil
here. If so, maybe show a "Unable to leave experiment X" message.
return | ||
end | ||
|
||
experiment = SingleUserExperiment.where(min_user_id: current_user.id, name: experiment_name).first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also where(...).first
can just be find_by(...)
Thanks for the feedback - all suggestions should be updated. |
Step 1 in 2018 teacher experience flag spec
Users will be able to join and leave an experiment via link (and experiment will be saved to their account)
Success message will be on studio home page and looks like this:
This sets us up to have a single flag for all the 2018 teacher experience work that will only need to be set once and will work on pegasus and dashboard.
cc @Erin007 @nkiruka