Cleanup mojito #37

dapperdrop · 2019-12-18T06:02:44Z

Ref: #35

Removed unused code: veil, etc
Tidy up + make commenting consistent
Added TODOs: optimise and/or refactor certain blocks, please take a look at these especially @allmywant

sync with original repo

kingo55

Everything is checking out from my end.

I added some new unit tests on a local branch I'm working on and the cleaned up lib is passing with flying colours.

The other comments about refactoring, I have added new issues around those so we can keep more PRs a bit tighter.

kingo55 · 2019-12-18T11:10:08Z

lib/mojito.js

                if (inTest)
                {
                    this.setInTest(1);

+                    // TODO: optimise logic?


I think something like this could work, but @allmywant would probably know a better way to handle this.

var chosenRecipe = this.getRecipe() || this.chooseRecipe(); this.setRecipe(chosenRecipe); // Exclude users from test if their chosen recipe no longer exists var chosenRecipeObject = this.options.recipes[chosenRecipe]; if (!chosenRecipeObject) { this.setInTest(0); return success; } else this.setInTest(1);

Addressed: 95e244c

Ref: f20f1f3

I moved this.setInTest(1) back to the start of this block as it was breaking a unit test.

setInTest(1) does some cookie testing which this.getRecipe() relies on. This caused a chicken and egg bug.

@allmywant - should address this when refactoring get/set recipe/test functions in #40

kingo55 · 2019-12-18T11:16:48Z

lib/mojito.js

            {
                this.chosenRecipe = recipe;
                var success = false;
-                // assign an empty function if 'onChosen' is not defined
+
+                // TODO: refactor following block, see comment below for recipe.js(this)
+                // Assign an empty function if 'onChosen' is not defined
                if (!recipe.js)
                {
                    recipe.js = function () {};
                }

-                // Check if unveil is needed
-                if (this.needUnveil())
-                {
-                    // hide or shift body out of screen
-                    this.hideBodyElement(recipe);
-                }
-
-                // for Unveil cases, timeout reached before onChosen get called.
-                if (this.options.executed)
-                {
-                    return success;
-                }
-
                try
                {
-                    // test level css
+                    // Inject test level css
                    if (this.options.css)
                    {
                        this.injectCSS(this.options.css);
                    }
-                    // recipe level css
+                    // Inject recipe level css
                    if (recipe.css)
                    {
                        this.injectCSS(recipe.css);
                    }

+                    // TODO: refactor? if(receipe.js) recipe.js(this);
                    recipe.js(this);


We don't strictly need to add an empty function do we?:

this.chosenRecipe = recipe; var success = false; try { // Test level css if (this.options.css) { this.injectCSS(this.options.css); } // Recipe level css if (recipe.css) { this.injectCSS(recipe.css); } // Recipe level js if (recipe.js) { recipe.js(this); } success = true; this.log("Test Object [" + this.options.name + "][" + this.options.id + "] recipe onChosen [" + recipe.name + "][" + recipe.id + "] run."); }

@allmywant - would you mind commenting on this? If we don't strictly need:

if (!recipe.js) { recipe.js = function () {}; }

Then I will remove it in another commit. This should be the last item to resolve for this PR, unless you guys see something else should be addressed.

Addressed: f20f1f3

lib/mojito.js

dapperdrop · 2019-12-19T03:10:46Z

Maybe make this comment a bit clearer to reflect the logic:

"Assign recipes to users in test, else exclude them by test's sample rate"

Done: 062d8e3

allmywant · 2019-12-19T23:46:20Z

@dapperdrop

if (!recipe.js)
{
recipe.js = function () {};
}
Then I will remove it in another commit. This should be the last item to resolve for this PR, unless you guys see something else should be addressed.

Yes, we don't need it.
I also left some comments in this pr but it seems not visible to you guys. e.g. :

Move var seededRNGSeed = (new Date()).getTime(); here means the initial seed will be created every time we call getSeededRNGRandom function, seems not correct. This the only question I'd like to confirm before approving.

Please let me know if you have seen it, maybe I did something wrong when I submit review comments.

dapperdrop · 2019-12-19T23:52:34Z

Thanks @allmywant - I didn't see your other comments, thanks for letting us know.

Move var seededRNGSeed = (new Date()).getTime(); here means the initial seed will be created every time we call getSeededRNGRandom function, seems not correct. This the only question I'd like to confirm before approving.

Good spot there. I'm not sure either, @kingo55 do you know?

kingo55 · 2019-12-20T00:00:34Z

@allmywant - did you "Submit" the code review? I had that issue the other day where I thought I added questions to a PR, but they weren't visible until I hit the "Submit review" section. Github code review submission is not so clear IMO.

allmywant

@kingo55
Maybe I just left comments but didn't click the "Submit review" button.

allmywant · 2019-12-18T06:28:11Z

lib/mojito.js

             * @private
             */
            getSeededRNGRandom: function ()
            {
+                // initial seed
+                var seededRNGSeed = (new Date()).getTime();


@dapperdrop
Move var seededRNGSeed = (new Date()).getTime(); here means the initial seed will be created every time we call getSeededRNGRandom function, seems not correct.

@kingo55 What do you think?

@dapperdrop @kingo55
What do you guys think on this one? This the only question I'd like to confirm before approving.

I'll move this back

Addressed: f20f1f3

allmywant · 2019-12-18T06:32:38Z

lib/mojito.js

@@ -220,13 +197,12 @@ Mojito = (function ()
                    }
                }

-                this.options.newToThisTest = newToThisTest;


@dapperdrop
If this line is removed, we are not to able access newToThisTest in tracking code.

I think this was only used as part of veil functionality, so it should be safe to remove if we removed veil.

Thanks @dapperdrop , that make sense.

I was leaning towards not including this because we can infer this from the existance of decisionIdx or seed/hash on the test object.

It looks clearer having a name newToTest on the test object though. So if users only want to fire the tracking once per test, that would save data.

Up to @dapperdrop whether we keep this or not.

I've re-instated this in: f20f1f3

allmywant · 2019-12-18T06:34:06Z

lib/mojito.js

                    if (recipe.css)
                    {
                        this.injectCSS(recipe.css);
                    }

+                    // TODO: refactor? if(receipe.js) recipe.js(this);


Good point!

kingo55 · 2019-12-20T00:04:00Z

Move var seededRNGSeed = (new Date()).getTime(); here means the initial seed will be created every time we call getSeededRNGRandom function, seems not correct. This the only question I'd like to confirm before approving.

Good spot there. I'm not sure either, @kingo55 do you know?

I'm not sure. Perhaps it's safer for us to move it back so we're not unintentionally changing the logic of this old function at all.

dapperdrop and others added 4 commits October 23, 2019 09:42

Merge pull request #1 from mint-metrics/master

c0e2ff1

sync with original repo

Merge remote-tracking branch 'upstream/master'

0902d66

Merge remote-tracking branch 'upstream/master'

96519aa

tidy up comments, trim unused code, add todo's

aeda748

dapperdrop requested review from allmywant and kingo55 December 18, 2019 06:02

kingo55 approved these changes Dec 19, 2019

View reviewed changes

kingo55 added the enhancement New feature or request label Dec 19, 2019

cleaning up unusused variables, refactor !inTest

c8d679f

dapperdrop mentioned this pull request Dec 19, 2019

Refactor chosenRecipe setting inside activate() function for inTest users #39

Closed

dapperdrop added 2 commits December 19, 2019 13:32

refactor chosenrecipe / intest logic

95e244c

update commenting

062d8e3

kingo55 assigned dapperdrop Dec 19, 2019

kingo55 mentioned this pull request Dec 19, 2019

Refactor this.inTest() and this.getRecipe() functions so we only read storage once #40

Closed

allmywant reviewed Dec 20, 2019

View reviewed changes

allmywant approved these changes Dec 20, 2019

View reviewed changes

allmywant mentioned this pull request Dec 20, 2019

Refactoring on how to choose recipe with sampleRate values #33

Closed

revert seededRNGSeed, address setInTest(1) bug

f20f1f3

dapperdrop merged commit 44932ec into mint-metrics:master Dec 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup mojito #37

Cleanup mojito #37

dapperdrop commented Dec 18, 2019

kingo55 left a comment

kingo55 Dec 18, 2019

dapperdrop Dec 19, 2019

dapperdrop Dec 20, 2019

kingo55 Dec 20, 2019

kingo55 Dec 18, 2019

dapperdrop Dec 19, 2019

dapperdrop Dec 20, 2019

dapperdrop commented Dec 19, 2019

allmywant commented Dec 19, 2019

dapperdrop commented Dec 19, 2019

kingo55 commented Dec 20, 2019

allmywant left a comment

allmywant Dec 18, 2019

allmywant Dec 19, 2019

dapperdrop Dec 20, 2019

dapperdrop Dec 20, 2019

allmywant Dec 18, 2019

dapperdrop Dec 20, 2019

allmywant Dec 20, 2019

kingo55 Dec 20, 2019

dapperdrop Dec 20, 2019

allmywant Dec 18, 2019

kingo55 commented Dec 20, 2019

Cleanup mojito #37

Cleanup mojito #37

Conversation

dapperdrop commented Dec 18, 2019

kingo55 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapperdrop commented Dec 19, 2019

allmywant commented Dec 19, 2019

dapperdrop commented Dec 19, 2019

kingo55 commented Dec 20, 2019

allmywant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingo55 commented Dec 20, 2019