refactor: Clean up the Optimizely integration #481

nchilada · 2020-06-12T23:50:40Z

What does this PR do?

Product changes

Don't attempt to interface with Optimizely Classic Web, since Classic is finally dead
Prepare to support Optimizely Edge, an alternative to Optimizely Web
Drop all references to customCampaignProperties. It seems to have been documented here but it couldn't possibly have worked.

Bug fixes

Don't try to track future inactive campaigns. The listener configured by registerFutureActiveCampaigns collects null campaign decisions in addition to active campaign decisions; until this PR, such campaigns were being passed to newActiveCampaign where they were throwing (asynchronous and thus inconsequential) errors.
If there is an effective referrer for the current page load, include it when tracking future campaigns and not just when tracking current campaigns.

Other refactoring

Stop using dependency injection (the referrerOverride, sendExperimentData, and sendCampaignData hooks on the left-hand side of the diff. It looks like that pattern was copy-and-pasted from here or here; in any case, it's confusing and unnecessary.
Define setRedirectInfo as a stateful method.
Test initWebIntegration as an isolated unit instead of invoking it as part of analytics.initialize. Ideally we'd do the same for sendWebDecisionToSegment`, but I ran out of energy.
Explicitly install lodash, sinon, and chai as devDependencies and use them throughout the tests. The preexisting tests used things like analytics.stub, analytics.called, and analytics.deepEqual but those are undocumented and seem to be less full-featured. As someone who doesn't have access to internal Segment documentation, those functions are very difficult to work with.

Are there breaking changes in this PR?

Technically yes, but effectively no, since the removed functionality is relevant to Optimizely products that haven't been supported in a long time.

Any background context you want to provide?

Our Classic Web product, the predecessor to X Web, is long gone.
We recently launched Optimizely Performance Edge, which may be implemented instead of Optimizely Web on any given page. We'll extend analytics.js-integrations to add support for it in a followup pull request.

Is there parity with the server-side/android/iOS integration components (if applicable)?

N/A

Does this require a new integration setting? If so, please explain how the new setting works

No. On the contrary, the "Custom Experiment Properties" setting described here is obsolete now that Optimizely Classic is gone (and was also incorrectly documented or implemented), and the "Custom Campaign Properties" setting can also be un-documented because it too was incorrectly implemented and is now being removed.

Links to helpful docs and other external resources

I've linked to some API docs from JSDoc comments!

* Don't attempt to interface with Optimizely Classic Web, since Classic is finally dead * Prepare to support Optimizely Edge, an alternative to Optimizely Web * Drop all references to customCampaignProperties. It seems to have been documented [here](https://segment.com/docs/connections/destinations/catalog/optimizely-web/#settings) but it couldn't possibly have worked. * Generally refactor the code and tests.

nchilada · 2020-06-12T23:52:17Z

@patrickshih-optimizely you may want to rebase #479 on top of this. I'll leave it up to you! One of us will definitely have merge conflicts 😆

integrations/optimizely/lib/index.js

patrickshih-optimizely · 2020-06-13T00:48:49Z

integrations/optimizely/lib/index.js

-        // Segment added code: in case this is a redirect experiment
-        if (referrer) campaignState.experiment.referrer = referrer;
-        sendCampaignData(campaignState);
+  var registerFutureActiveCampaigns = function() {


we talked a little bit about potentially bringing these out as private functions? It would make this suite much easier to test IMO. What are your thoughts?

If not, it feels a little questionable to declare these as functions and not really re-use them. Maybe we can unfurl them and supplement with comments? WDYT?

Fair points! Of the three remaining nested functions (see the latest commit) I think some are going to be easily reusable and some aren't.

Maybe we can extract the reusable ones?

What would we do with remaining ones - inline them? One could argue that single-use nested functions are helpful for documenting what each block of code does, but I think code comments might suffice too.

patrickshih-optimizely

Thanks for doing all the cleanup code. I had a comment we can discuss!

Also, I'll rebase on top of yours... it'll makes my integration smoother

nchilada · 2020-06-17T23:11:33Z

integrations/optimizely/test/index.test.js


-  describe('after loading', function() {
-    beforeEach(function(done) {
-      analytics.once('ready', done);


I don't know what analytics.once('ready', callback); does or whether it's relevant here. I removed it because it seemed to cause the test suite to hang and skip the tests in this describe; though maybe I'm wrong, maybe I was just misreading the test output.

nchilada · 2020-06-17T23:12:23Z

integrations/optimizely/test/index.test.js

+    beforeEach(function() {
      analytics.initialize();
-      mockBothOptimizelyDataObjects();
-      analytics.page();


I'm not sure if this was doing anything. There may be some value in testing an analytics.page call without the optional name argument, but I suspect this is not the correct place for that.

nchilada · 2020-06-18T00:16:48Z

integrations/optimizely/HISTORY.md

+
+  * Don't attempt to interface with Optimizely Classic Web, since Classic is finally dead
+  * Prepare to support Optimizely Edge, an alternative to Optimizely Web
+  * Drop all references to customCampaignProperties. It seems to have been documented [here](https://segment.com/docs/connections/destinations/catalog/optimizely-web/#settings) but it couldn't possibly have worked.


Beyond this update to the integration code, Custom Campaign Properties and Custom Experiment Properties should also be removed from the docs and from the Segment app. (I didn't highlight Custom Experiment Properties in HISTORY.md since it was specific to the Optimizely Classic Web product, whose code is being removed entirely.)

Full explanation of why this functionality is surely broken:

The docs indicate that Custom Experiment Properties and Custom Campaign Properties could be used to capture experiment- and campaign-specific properties like experiment_color and campaign_name in Classic Web and X Web respectively. This makes sense, kinda.

Unfortunately, the integration code indicates that for Classic Web, the Custom Experiment Properties are being read off of the top-level window.optimizely.data object, rather than off of an experiment object

And for X Web, the Custom Campaign Properties are being read off of a non-existent window.optimizely.newMockData property. This property is mocked in index.test.js but does not appear in actual X Web snippets.

Thanks for the heads-up! I'll work on getting the documentation updated.

tysonmote · 2020-08-10T20:48:55Z

@nchilada Thanks for this! I'm going to work on releasing this shortly.

tysonmote · 2020-08-10T22:03:56Z

@nchilada It looks like this PR introduces some new lint warnings in our CI. Do you think you could fix these up? (Sorry that this isn't clear -- I'm not sure why our CircleCI setup isn't able to run successfully with external branches, yet.)

/workdir/integrations/optimizely/lib/index.js
--
  | 292:15  warning  'state' is already declared in the upper scope  no-shadow
  |  
  | /workdir/integrations/optimizely/test/index.test.js
  | 106:11  warning  for..in loops iterate over the entire prototype chain, which is virtually never what you want. Use Object.{keys,values,entries}, and iterate over the resulting array  no-restricted-syntax
  | 170:7   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  | 666:9   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  | 690:9   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  | 720:9   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  | 747:9   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  | 774:9   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  |  
  | ✖ 8 problems (0 errors, 8 warnings)

integrations/optimizely/lib/index.js

nchilada · 2020-08-13T02:41:22Z

@tysonmote @gpsamson thank you for the reviews and sorry for the late update! I've pushed some lint fixes; hopefully they help. I'm heading out of office through next week but @patrickshih-optimizely and I can check in to make sure you don't need anything to more for this PR.

BTW would you all be in charge of any necessary integration testing? I saw a link to this README but I don't have a lot of context (I've never directly used Segment myself).

jenskene · 2021-04-21T22:21:49Z

@patrickshih-optimizely and @nchilada — Thank you for your patience! We are planning to merge this PR and #483 on May 3, 2021. Let us know if anything has changed in the past (many) months.

patrickshih-optimizely · 2021-04-22T02:31:06Z

Hi @jenskene ! Thanks for getting back to us! @nchilada had since moved on from Optimizely, feel free to direct any questions about this PR to me.

I scanned this PR once again and this one looks good. Mainly, because most of the changes in this PR is removing old functionality that Optimizely does not support anymore (specifically, our Classic product and things w.r.t Classic)

nchilada · 2021-04-23T15:52:32Z

@patrickshih-optimizely thanks very much for taking over! @jenskene yup I left Optimizely last year and stopped getting notifications. Sorry for not posting an update in either PR!

patrickshih-optimizely reviewed Jun 13, 2020

View reviewed changes

integrations/optimizely/lib/index.js Show resolved Hide resolved

patrickshih-optimizely reviewed Jun 13, 2020

View reviewed changes

integrations/optimizely/lib/index.js Outdated Show resolved Hide resolved

patrickshih-optimizely reviewed Jun 13, 2020

View reviewed changes

nchilada marked this pull request as draft June 13, 2020 05:01

nchilada added 2 commits June 12, 2020 23:31

Way more changes

8e556d5

Finish fixing and refactoring tests

e5475be

nchilada marked this pull request as ready for review June 17, 2020 22:51

yarn add lodash --dev

099f98b

nchilada commented Jun 17, 2020

View reviewed changes

Update HISTORY.md

ed636c0

nchilada commented Jun 18, 2020

View reviewed changes

patrickshih-optimizely mentioned this pull request Jul 10, 2020

feat(optimizely): Expose Edge API to track Edge experiments #483

Open

tysonmote mentioned this pull request Aug 10, 2020

Release #481 (refactor: Clean up the Optimizely integration) #501

Closed

gpsamson reviewed Aug 11, 2020

View reviewed changes

integrations/optimizely/lib/index.js Show resolved Hide resolved

Fix lint

d2211d7

nchilada mentioned this pull request Aug 13, 2020

Support a better debugging mode for unit tests #502

Open

brennan mentioned this pull request Sep 21, 2020

feat(optimizely): Expose Edge API to track Edge experiments #515

Closed

patrickshih-optimizely approved these changes Apr 22, 2021

View reviewed changes

refactor: Clean up the Optimizely integration #481

Are you sure you want to change the base?

refactor: Clean up the Optimizely integration #481

Conversation

nchilada commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Product changes

Bug fixes

Other refactoring

Are there breaking changes in this PR?

Any background context you want to provide?

Is there parity with the server-side/android/iOS integration components (if applicable)?

Does this require a new integration setting? If so, please explain how the new setting works

Links to helpful docs and other external resources

Uh oh!

nchilada commented Jun 12, 2020

Uh oh!

Uh oh!

Uh oh!

patrickshih-optimizely Jun 13, 2020

Choose a reason for hiding this comment

Uh oh!

nchilada Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickshih-optimizely left a comment

Choose a reason for hiding this comment

Uh oh!

nchilada Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

nchilada Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tysonmote Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

tysonmote commented Aug 10, 2020

Uh oh!

tysonmote commented Aug 10, 2020

Uh oh!

Uh oh!

nchilada commented Aug 13, 2020

Uh oh!

jenskene commented Apr 21, 2021

Uh oh!

patrickshih-optimizely commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nchilada commented Apr 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nchilada commented Jun 12, 2020 •

edited

Loading

nchilada Jun 15, 2020 •

edited

Loading

nchilada Jun 17, 2020 •

edited

Loading

nchilada Jun 18, 2020 •

edited

Loading

patrickshih-optimizely commented Apr 22, 2021 •

edited

Loading