Support for expansion of variables with params in parts of amp-analytics. #4855

avimehta · 2016-09-08T00:41:44Z

Specifically, the vars and extraUrlParams` section encoded the
variables with params before they could be expanded.

avimehta · 2016-09-08T00:42:10Z

third_party/babel/custom-babel-helpers.js

@@ -35,8 +35,58 @@
    };
  })();

-  babelHelpers.slice = Array.prototype.slice;
-  babelHelpers.bind = Function.prototype.bind;
+  babelHelpers.slicedToArray = (function () {


These changes were autogenerated by gulp babel-helpers.

dvoytenko · 2016-09-24T00:37:00Z

extensions/amp-analytics/0.1/amp-analytics.js

+   */
+  getNameArgs_(key) {
+    if (!key) {
+      return ['', ''];


This is not an object...

:) Now it is.

Still isn't...

dvoytenko · 2016-09-24T00:37:09Z

extensions/amp-analytics/0.1/amp-analytics.js

+   * Returns an array containing two values: name and args parsed from the key.
+   *
+   * @param {string} key The key to be parsed.
+   * @return {Object<string>}


dvoytenko · 2016-09-24T00:37:32Z

extensions/amp-analytics/0.1/amp-analytics.js

+    if (!key) {
+      return ['', ''];
+    }
+    const match = key.match(/([^(]*)(\([^)]*\))?/);


Can match be null?

It shouldn't be. If it is, that is an error that we would like to surface. Added a user.error.

Added a user.error.

Not yet....

dvoytenko · 2016-09-24T00:38:08Z

extensions/amp-analytics/0.1/amp-analytics.js

-    return encodeURIComponent(raw);
+    // Separate out names and arguments from the value and encode the value.
+    const {name, argList} = this.getNameArgs_(String(raw));
+    return encodeURIComponent(name) + argList;


What if name is '' here?

that will be encoded to '' and passed along. It shouldn't be empty though. That is like saying you are trying to expand an empty string.

dvoytenko · 2016-09-24T00:39:24Z

Added @jridgewell to review for a second pair of eyes and since he was already on this review earlier.

jridgewell

Code LGTM. One request.

jridgewell · 2016-09-26T17:53:14Z

extensions/amp-analytics/0.1/test/test-amp-analytics.js

      let actual = analytics.expandTemplate_('${1}', vars, {}, 3);
      expect(actual).to.equal('1234%25252524%2525257B1%2525257D');

      actual = analytics.expandTemplate_('${1}', vars, {}, 5);
      expect(actual).to.equal('123412%252525252524%25252525257B3%25252525257D');
    });
+
+    it('works with params', () => {


Can we get a test that shows it grabbing the actually query param?

I wasn't sure if you wanted me to add a unittest for getNameArgs() or something else. I added two tests. the one that goes through the whole code flow is not a unittest as it mocks parts of url replacement.

(getting actual query params is difficult here because the win.loc is about:srcdoc or something - no query params.)

avimehta · 2016-09-28T21:49:28Z

ptal.

dvoytenko · 2016-09-30T18:28:47Z

src/url.js

@@ -135,7 +135,7 @@ export function parseUrl(url, opt_nocache) {
 * @param {boolean=} opt_addToFront
 * @return {string}
 */
-function appendParamStringToUrl(url, paramString, opt_addToFront) {
+export function appendParamStringToUrl(url, paramString, opt_addToFront) {


I have relatively big concerns about exposing this method given potential for XSS. At the very least, if we want to do this, could you please massage the naming to something like:

appendEncodedParamStringToUrl

encodedParamString

?

Used the first name. If you would prefer, I can copy this method into amp-analytics and keep it private.

…ics config. Specifically, the `vars` and extraUrlParams` section encoded the variables with params before they could be expanded. Fixes ampproject#4825

avimehta · 2016-10-03T22:54:49Z

ptal.

jridgewell · 2016-10-03T22:58:15Z

extensions/amp-analytics/0.1/amp-analytics.js

+   */
+  getNameArgs_(key) {
+    if (!key) {
+      return ['', ''];


Still isn't...

jridgewell · 2016-10-03T22:58:40Z

extensions/amp-analytics/0.1/amp-analytics.js

+    if (!key) {
+      return ['', ''];
+    }
+    const match = key.match(/([^(]*)(\([^)]*\))?/);


Added a user.error.

Not yet....

jridgewell · 2016-10-03T23:00:42Z

Ha, apparently I reviewed a dated PR. LGTM.

dvoytenko · 2016-10-04T21:44:13Z

LGTM. A clear name should be enough to pick up issues in review.

…ics. (ampproject#4855) * Support for expansion of variables with params in parts of amp-analytics config. Specifically, the `vars` and extraUrlParams` section encoded the variables with params before they could be expanded. Fixes ampproject#4825

avimehta reviewed Sep 8, 2016
View reviewed changes

avimehta force-pushed the bug branch 2 times, most recently from 88e86ca to a6cbdba Compare September 14, 2016 20:33

avimehta force-pushed the bug branch from a6cbdba to 5b7ee72 Compare September 22, 2016 21:47

avimehta added NEEDS REVIEW WG: analytics labels Sep 22, 2016

avimehta assigned dvoytenko Sep 22, 2016

dvoytenko suggested changes Sep 24, 2016

View reviewed changes

dvoytenko assigned jridgewell Sep 24, 2016

jridgewell requested changes Sep 26, 2016

View reviewed changes

avimehta force-pushed the bug branch 3 times, most recently from 1f3d9b7 to b2da985 Compare September 28, 2016 22:33

jridgewell approved these changes Sep 30, 2016

View reviewed changes

dvoytenko reviewed Sep 30, 2016

View reviewed changes

avimehta added 3 commits October 3, 2016 15:46

Support for expansion of variables with params in parts of amp-analyt…

202946d

…ics config. Specifically, the `vars` and extraUrlParams` section encoded the variables with params before they could be expanded. Fixes ampproject#4825

Review feedback.

731457f

Review feedback. - Renamed method.

d3dce6f

avimehta force-pushed the bug branch from b2da985 to d3dce6f Compare October 3, 2016 22:53

jridgewell requested changes Oct 3, 2016

View reviewed changes

jridgewell approved these changes Oct 3, 2016

View reviewed changes

Fixed check-types errors.

ce13dbf

dvoytenko approved these changes Oct 4, 2016

View reviewed changes

avimehta merged commit c609e02 into ampproject:master Oct 5, 2016

avimehta deleted the bug branch October 5, 2016 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for expansion of variables with params in parts of amp-analytics. #4855

Support for expansion of variables with params in parts of amp-analytics. #4855

avimehta commented Sep 8, 2016

avimehta Sep 8, 2016

dvoytenko Sep 24, 2016

avimehta Sep 28, 2016

jridgewell Oct 3, 2016 •

edited

dvoytenko Sep 24, 2016

avimehta Sep 28, 2016

dvoytenko Sep 24, 2016

avimehta Sep 28, 2016

jridgewell Oct 3, 2016

dvoytenko Sep 24, 2016

avimehta Sep 28, 2016

dvoytenko commented Sep 24, 2016

jridgewell left a comment

jridgewell Sep 26, 2016

avimehta Sep 28, 2016

avimehta commented Sep 28, 2016

dvoytenko Sep 30, 2016

avimehta Oct 3, 2016

avimehta commented Oct 3, 2016

jridgewell Oct 3, 2016 •

edited

jridgewell Oct 3, 2016

jridgewell commented Oct 3, 2016

dvoytenko commented Oct 4, 2016

Support for expansion of variables with params in parts of amp-analytics. #4855

Support for expansion of variables with params in parts of amp-analytics. #4855

Conversation

avimehta commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell Oct 3, 2016 • edited

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

dvoytenko commented Sep 24, 2016

jridgewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avimehta commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avimehta commented Oct 3, 2016

jridgewell Oct 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Oct 3, 2016

dvoytenko commented Oct 4, 2016

jridgewell Oct 3, 2016 •

edited

jridgewell Oct 3, 2016 •

edited