feat(addon): #732 Create experiment for DeleteMenu #777

Merged
merged 1 commit into from May 31, 2016

Conversation

Projects
None yet
5 participants
@ncloudioj
Contributor

ncloudioj commented May 31, 2016

Fixes #732

This change is Reviewable

@ncloudioj

This comment has been minimized.

Show comment
Hide comment
Contributor

ncloudioj commented May 31, 2016

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 31, 2016

Coverage Status

Coverage increased (+0.06%) to 92.879% when pulling 216842e on experiment-on-block-menu into 0675ecb on master.

Coverage Status

Coverage increased (+0.06%) to 92.879% when pulling 216842e on experiment-on-block-menu into 0675ecb on master.

+ };
+
+ if (this.props.experimentData.reverseMenuOptions) {
+ payload.experiment_id = this.props.experimentData.id;

This comment has been minimized.

@k88hudson

k88hudson May 31, 2016

Member

This should always be included I think, not just for a truthy value

@k88hudson

k88hudson May 31, 2016

Member

This should always be included I think, not just for a truthy value

+ // Targeting 10% of users for this experiment
+ if (rng() <= 0.1) {
+ this._experimentData.id = "exp-001-delete-menu";
+ this._experimentData.reverseMenuOptions = true;

This comment has been minimized.

@k88hudson

k88hudson May 31, 2016

Member

I would recommend using string or integer based variant IDs such as "CONTROL" and "REVERSE". Also, you might want to call the experiment something neutral like deleteMenuOrder

@k88hudson

k88hudson May 31, 2016

Member

I would recommend using string or integer based variant IDs such as "CONTROL" and "REVERSE". Also, you might want to call the experiment something neutral like deleteMenuOrder

-module.exports = connect(justDispatch)(DeleteMenu);
+module.exports = connect(state => {
+ return {experimentData: state.Experiments.data};
+})(DeleteMenu);

This comment has been minimized.

@k88hudson

k88hudson May 31, 2016

Member

Could we have a test for this?

@k88hudson

k88hudson May 31, 2016

Member

Could we have a test for this?

@k88hudson k88hudson added PR / R+ and removed PR / Needs review labels May 31, 2016

@k88hudson

This comment has been minimized.

Show comment
Hide comment
@k88hudson

k88hudson May 31, 2016

Member

Ok, we are going to do a follow-up patch to refactor the randomizer stuff into its own module

Member

k88hudson commented May 31, 2016

Ok, we are going to do a follow-up patch to refactor the randomizer stuff into its own module

@ncloudioj ncloudioj merged commit 173b940 into master May 31, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 92.879%
Details

@ncloudioj ncloudioj deleted the experiment-on-block-menu branch May 31, 2016

+
+ this._experimentData = {};
+ let rng = seedrandom(options.clientID);
+ // Targeting 10% of users for this experiment

This comment has been minimized.

@yati-sagade

yati-sagade Dec 28, 2017

Hi! Just a curious Joe here. Did this code run on every new tab? If so, is this not putting 10% of new tab opens (as opposed to 10% of FF eligible FF installs) in the variant?

@yati-sagade

yati-sagade Dec 28, 2017

Hi! Just a curious Joe here. Did this code run on every new tab? If so, is this not putting 10% of new tab opens (as opposed to 10% of FF eligible FF installs) in the variant?

This comment has been minimized.

@yati-sagade

yati-sagade Dec 28, 2017

Just saw the seedrandom(), so that should be ok :)

@yati-sagade

yati-sagade Dec 28, 2017

Just saw the seedrandom(), so that should be ok :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment