Move video_state macro (#26212)

* moved out and added tests * fixing tests * Adding warning * Move macro implementation into getAnalyticsDetail
ampproject · Feb 14, 2020 · 755ff40 · 755ff40
1 parent feaaddf
commit 755ff40
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 14 deletions.
diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js
@@ -626,14 +626,10 @@ export class GlobalVariableSource extends VariableSource {
     });
 
     this.setAsync('VIDEO_STATE', (id, property) => {
-      const root = this.ampdoc.getRootNode();
-      const video = user().assertElement(
-        root.getElementById(/** @type {string} */ (id)),
-        `Could not find an element with id="${id}" for VIDEO_STATE`
+      return Services.videoManagerForDoc(this.ampdoc).getVideoStateProperty(
+        id,
+        property
       );
-      return Services.videoManagerForDoc(this.ampdoc)
-        .getAnalyticsDetails(video)
-        .then(details => (details ? details[property] : ''));
     });
 
     this.setAsync('AMP_STATE', key => {

diff --git a/src/service/video-manager-impl.js b/src/service/video-manager-impl.js
@@ -317,14 +317,23 @@ export class VideoManager {
   }
 
   /**
-   * Gets the current analytics details for the given video.
+   * Gets the current analytics details property for the given video.
    * Fails silently if the video is not registered.
-   * @param {!AmpElement} videoElement
-   * @return {!Promise<!VideoAnalyticsDetailsDef|undefined>}
+   * @param {string} id
+   * @param {string} property
+   * @return {!Promise<string>}
    */
-  getAnalyticsDetails(videoElement) {
+  getVideoStateProperty(id, property) {
+    const root = this.ampdoc.getRootNode();
+    const videoElement = user().assertElement(
+      root.getElementById(/** @type {string} */ (id)),
+      `Could not find an element with id="${id}" for VIDEO_STATE`
+    );
     const entry = this.getEntryForElement_(videoElement);
-    return entry ? entry.getAnalyticsDetails() : Promise.resolve();
+    return (entry
+      ? entry.getAnalyticsDetails()
+      : Promise.resolve()
+    ).then(details => (details ? details[property] : ''));
   }
 
   /**

diff --git a/test/manual/amp-video.amp.html b/test/manual/amp-video.amp.html
@@ -109,6 +109,9 @@
   <script async src="https://cdn.ampproject.org/v0.js"></script>
 </head>
 <body>
+
+  <amp-pixel src="https://donot.worry/?ct=VIDEO_STATE(my-video,currentTime)&r=TOTAL_ENGAGED_TIME&b=BACKGROUND_STATE"></amp-pixel>
+
   <div style="display: flex; flex-direction: column; height: 100vh">
     <div>
       <h1>AMP Video Players</h1>
@@ -212,6 +215,9 @@ <h2>Analytics Events</h2>
             },
             "videoSpec": {
               "interval": 2
+            },
+            "extraUrlParams": {
+              "currentTime": "VIDEO_STATE(my-video,currentTime)"
             }
           },
           "myVideoSession": {

diff --git a/test/unit/test-url-replacements.js b/test/unit/test-url-replacements.js
@@ -903,8 +903,8 @@ describes.sandboxed('UrlReplacements', {}, env => {
       it('Should replace VIDEO_STATE(video,parameter) with video data', () => {
         const win = getFakeWindow();
         env.sandbox.stub(Services, 'videoManagerForDoc').returns({
-          getAnalyticsDetails() {
-            return Promise.resolve({currentTime: 1.5});
+          getVideoStateProperty() {
+            return Promise.resolve('1.5');
           },
         });
         env.sandbox