🐛 [Story analytics] Fix prerender analytics not firing (#37975)

* Added tasts * Undo * Removed timeout of 10s * Reverted past change and added visibility wait * Fixed unit tests * Remove unused import in test * Test analytics not sent if not visible * Fixed test index value
ampproject · Mar 31, 2022 · bccc2a3 · bccc2a3
1 parent ba3e405
commit bccc2a3
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 17 deletions.
diff --git a/extensions/amp-story/1.0/story-analytics.js b/extensions/amp-story/1.0/story-analytics.js
@@ -8,7 +8,7 @@ import {triggerAnalyticsEvent} from '#utils/analytics';
 import {StateProperty, getStoreService} from './amp-story-store-service';
 import {getVariableService} from './variable-service';
 
-import {registerServiceBuilder} from '../../../src/service-helpers';
+import {getAmpdoc, registerServiceBuilder} from '../../../src/service-helpers';
 
 /** @const {string} */
 export const ANALYTICS_TAG_NAME = '__AMP_ANALYTICS_TAG_NAME__';
@@ -127,11 +127,15 @@ export class StoryAnalyticsService {
   triggerEvent(eventType, element = null) {
     this.incrementPageEventCount_(eventType);
 
-    triggerAnalyticsEvent(
-      this.element_,
-      eventType,
-      this.updateDetails(eventType, element)
-    );
+    getAmpdoc(this.element_)
+      .whenFirstVisible()
+      .then(() =>
+        triggerAnalyticsEvent(
+          this.element_,
+          eventType,
+          this.updateDetails(eventType, element)
+        )
+      );
   }
 
   /**

diff --git a/extensions/amp-story/1.0/test/test-amp-story-embedded-component.js b/extensions/amp-story/1.0/test/test-amp-story-embedded-component.js
@@ -5,7 +5,10 @@ import {LocalizationService} from '#service/localization';
 
 import * as analyticsApi from '#utils/analytics';
 
-import {registerServiceBuilder} from '../../../../src/service-helpers';
+import {
+  getAmpdoc,
+  registerServiceBuilder,
+} from '../../../../src/service-helpers';
 import {AmpStoryEmbeddedComponent} from '../amp-story-embedded-component';
 import {
   Action,
@@ -224,20 +227,22 @@ describes.realWin('amp-story-embedded-component', {amp: true}, (env) => {
     expect(tooltipTextEl.textContent).to.equal('google.com');
   });
 
-  it('should fire analytics event when entering a tooltip', () => {
+  it('should fire analytics event when entering a tooltip', async () => {
     fakePage.appendChild(clickableEl);
     storeService.dispatch(Action.TOGGLE_INTERACTIVE_COMPONENT, {
       element: clickableEl,
       state: EmbeddedComponentState.FOCUSED,
     });
 
+    await getAmpdoc(win.document).whenFirstVisible();
+
     expect(analyticsTriggerStub).to.be.calledWith(
       parentEl,
       StoryAnalyticsEvent.FOCUS
     );
   });
 
-  it('should send data-var specified by publisher in analytics event', () => {
+  it('should send data-var specified by publisher in analytics event', async () => {
     addAttributesToElement(clickableEl, {
       'data-vars-tooltip-id': '1234',
     });
@@ -248,6 +253,8 @@ describes.realWin('amp-story-embedded-component', {amp: true}, (env) => {
       state: EmbeddedComponentState.FOCUSED,
     });
 
+    await getAmpdoc(win.document).whenFirstVisible();
+
     expect(analyticsTriggerStub).to.be.calledWithMatch(
       parentEl,
       StoryAnalyticsEvent.FOCUS,
@@ -257,7 +264,7 @@ describes.realWin('amp-story-embedded-component', {amp: true}, (env) => {
     );
   });
 
-  it('should fire analytics event when clicking on the tooltip of a link', () => {
+  it('should fire analytics event when clicking on the tooltip of a link', async () => {
     fakePage.appendChild(clickableEl);
     storeService.dispatch(Action.TOGGLE_INTERACTIVE_COMPONENT, {
       element: clickableEl,
@@ -273,13 +280,15 @@ describes.realWin('amp-story-embedded-component', {amp: true}, (env) => {
 
     tooltip.click();
 
+    await getAmpdoc(win.document).whenFirstVisible();
+
     expect(analyticsTriggerStub).to.be.calledWith(
       parentEl,
       StoryAnalyticsEvent.CLICK_THROUGH
     );
   });
 
-  it('should fire analytics event when clicking on the tooltip of a tweet', () => {
+  it('should fire analytics event when clicking on the tooltip of a tweet', async () => {
     clickableEl = win.document.createElement('amp-twitter');
     addAttributesToElement(clickableEl, {
       'data-tweetid': '1166723359696130049',
@@ -300,6 +309,8 @@ describes.realWin('amp-story-embedded-component', {amp: true}, (env) => {
 
     tooltip.click();
 
+    await getAmpdoc(win.document).whenFirstVisible();
+
     expect(analyticsTriggerStub).to.be.calledWith(
       parentEl,
       StoryAnalyticsEvent.FOCUS

diff --git a/extensions/amp-story/1.0/test/test-amp-story-share.js b/extensions/amp-story/1.0/test/test-amp-story-share.js
@@ -4,7 +4,10 @@ import {Services} from '#service';
 
 import * as analyticsApi from '#utils/analytics';
 
-import {registerServiceBuilder} from '../../../../src/service-helpers';
+import {
+  getAmpdoc,
+  registerServiceBuilder,
+} from '../../../../src/service-helpers';
 import {AmpStoryShare} from '../amp-story-share';
 import {
   Action,
@@ -94,7 +97,7 @@ describes.realWin('amp-story-share', {amp: true}, (env) => {
     });
   });
 
-  it('should send correct analytics tagName and eventType when opening the share menu', () => {
+  it('should send correct analytics tagName and eventType when opening the share menu', async () => {
     analyticsTriggerStub = env.sandbox.stub(
       analyticsApi,
       'triggerAnalyticsEvent'
@@ -103,6 +106,8 @@ describes.realWin('amp-story-share', {amp: true}, (env) => {
 
     storeService.dispatch(Action.TOGGLE_SHARE_MENU, true);
 
+    await getAmpdoc(win.document).whenFirstVisible();
+
     // tagName should be amp-story-share-menu as per extensions/amp-story/amp-story-analytics.md
     expect(analyticsTriggerStub).to.be.calledWith(
       ampStory,

diff --git a/extensions/amp-story/1.0/test/test-story-analytics.js b/extensions/amp-story/1.0/test/test-story-analytics.js
@@ -1,7 +1,9 @@
 import * as analytics from '#utils/analytics';
 
+import {getAmpdoc} from 'src/service-helpers';
+
 import {Action, getStoreService} from '../amp-story-store-service';
-import {StoryAnalyticsService} from '../story-analytics';
+import {getAnalyticsService} from '../story-analytics';
 
 describes.realWin('amp-story-analytics', {amp: true}, (env) => {
   let el;
@@ -10,11 +12,12 @@ describes.realWin('amp-story-analytics', {amp: true}, (env) => {
   beforeEach(() => {
     const {win} = env;
     el = win.document.createElement('amp-story');
+    win.document.body.appendChild(el);
     storeService = getStoreService(win);
-    new StoryAnalyticsService(env.win, el);
+    getAnalyticsService(win, el);
   });
 
-  it('sends story-page-visible on current page change', () => {
+  it('sends story-page-visible on current page change', async () => {
     const triggerAnalyticsStub = env.sandbox.stub(
       analytics,
       'triggerAnalyticsEvent'
@@ -23,6 +26,9 @@ describes.realWin('amp-story-analytics', {amp: true}, (env) => {
       id: 'page-1',
       index: 0,
     });
+
+    await getAmpdoc(env.win.document).whenFirstVisible();
+
     expect(triggerAnalyticsStub).to.have.been.calledOnceWithExactly(
       el,
       'story-page-visible',
@@ -43,7 +49,7 @@ describes.realWin('amp-story-analytics', {amp: true}, (env) => {
     expect(triggerAnalyticsStub).not.to.be.called;
   });
 
-  it('sends story-page-visible on content page after ad page', () => {
+  it('does not send story-page-visible before document becomes visible', async () => {
     const triggerAnalyticsStub = env.sandbox.stub(
       analytics,
       'triggerAnalyticsEvent'
@@ -52,6 +58,29 @@ describes.realWin('amp-story-analytics', {amp: true}, (env) => {
       id: 'page-1',
       index: 0,
     });
+    expect(triggerAnalyticsStub).not.to.be.called;
+
+    await getAmpdoc(env.win.document).whenFirstVisible();
+
+    expect(triggerAnalyticsStub).to.have.been.calledOnceWithExactly(
+      el,
+      'story-page-visible',
+      env.sandbox.match({storyPageIndex: 0, storyPageId: 'page-1'})
+    );
+  });
+
+  it('sends story-page-visible on content page after ad page', async () => {
+    const triggerAnalyticsStub = env.sandbox.stub(
+      analytics,
+      'triggerAnalyticsEvent'
+    );
+    storeService.dispatch(Action.CHANGE_PAGE, {
+      id: 'page-1',
+      index: 0,
+    });
+
+    await getAmpdoc(env.win.document).whenFirstVisible();
+
     expect(triggerAnalyticsStub).to.have.been.calledOnceWithExactly(
       el,
       'story-page-visible',
@@ -68,6 +97,9 @@ describes.realWin('amp-story-analytics', {amp: true}, (env) => {
       id: 'page-2',
       index: 2,
     });
+
+    await getAmpdoc(env.win.document).whenFirstVisible();
+
     expect(triggerAnalyticsStub).to.have.been.calledTwice;
     expect(triggerAnalyticsStub.secondCall).to.have.been.calledWithExactly(
       el,