Added initial version of amp-anlaytics variable filters. #5621

avimehta · 2016-10-14T21:01:47Z

WIP. Tests are pending a preliminary design review.

avimehta · 2016-10-14T21:02:11Z

avimehta · 2016-10-14T21:03:34Z

extensions/amp-analytics/0.1/variables.js

+  }
+
+  hashFilter_() {
+    if (this.crypto_) {


Hash seems to be the only filter for now that is async. Should I make everything async? Otherwise, what are the options?

My guess is that, yes, it should be all async. Unless we decide to initially exclude SHA. But sooner or later something else will come up with async requirement.

/cc @cramforce

made everything async.

dvoytenko · 2016-10-18T21:39:08Z

extensions/amp-analytics/0.1/variables.js

+    this.register_('default', defaultFilter);
+    this.register_('hash', this.hashFilter_.bind(this));
+    this.register_('substr', substrFilter);
+    this.register_('trim', value => value.trim());


Value can be null, right? Here and everywhere.

should I make a lightweight wrapper or something? Or will assertString work?

I almost want to say that the callback should not be called for null or undefined. But we have default filter so that's not possible. We need to allow null and undefined though. So I see only two options: (a) always process null directly as in value => value != null ? filter(value) : null; (b) have two register ways registerNotNull_ and registerNull_: one will callback on nulls and the other won't.

i am not sure I understand option (a). How will default filter work in this case? option (b) sounds fine.

Is this still outstanding?

yes, this is outstanding. I don't understand option (a) and didn't want to implement it without knowing what it means..

Option (a) basically says, all functions should expect (and test) any value: be it an empty string or null or undefined or 0, etc. Option (b) means: if value is null it won't even call the filter (except in some cases such as default). The main question here is whether the value can ever be null or undefined.

dvoytenko · 2016-10-18T21:39:18Z

extensions/amp-analytics/0.1/variables.js

+
+export class VariableService {
+
+  constructor(window) {


dvoytenko · 2016-10-18T21:39:29Z

extensions/amp-analytics/0.1/variables.js

+ */
+let VariableFilterDef;
+
+function substrFilter() {


dvoytenko · 2016-10-18T21:39:47Z

extensions/amp-analytics/0.1/variables.js

+  return arguments[0].substr(start, length);
+}
+
+function defaultFilter() {


jsdoc. why use arguments vs positioned args?

Any ideas on how to handle check-types warnings like following:

extensions/amp-analytics/0.1/variables.js:69: ERROR - actual parameter 2 of VariableService$$module$extensions$amp_analytics$0_1$variables.prototype.register_ does not match formal parameter found : function (string, string, string): Promise<VariableFilterIODef$$module$extensions$amp_analytics$0_1$variables> required: function (...(Array<string>|string)): (Array<string>|string) this.register_('substr', substrFilter);

/cc @jridgewell

also /cc @erwinmombay

okay.. using positioned args now. Still need to figure out if there is a better way to do type checking.

dvoytenko · 2016-10-18T21:40:00Z

extensions/amp-analytics/0.1/variables.js

+  constructor(window) {
+    this.win_ = window;
+
+    this.filters_ = Object.create(null);


dvoytenko · 2016-10-18T21:40:44Z

extensions/amp-analytics/0.1/variables.js

+  return arguments[0] || arguments[1];
+}
+
+export class VariableService {


I guess a bigger question is whether this should be exclusively an analytics feature or url-replacements. Let's keep it here for now.

ack. One issue with url-replacements is that the format used there is plain strings for variables. Passing arguments and nesting becomes difficult.

dvoytenko · 2016-10-18T21:40:52Z

extensions/amp-analytics/0.1/variables.js

+    this.win_ = window;
+
+    this.filters_ = Object.create(null);
+    this.crypto_ = null;


dvoytenko · 2016-10-18T21:41:43Z

extensions/amp-analytics/0.1/variables.js

+
+  /**
+   * [register_ description]
+   * @param  {string} name [description]


nit: spacing

forgot to fix in this revision. Will fix with next commit.

dvoytenko · 2016-10-18T21:42:27Z

extensions/amp-analytics/0.1/variables.js

+    return encodeURIComponent(name) + argList;
+  }
+
+  hashFilter_() {


dvoytenko · 2016-10-18T21:43:36Z

extensions/amp-analytics/0.1/variables.js

+  }
+
+  hashFilter_() {
+    if (this.crypto_) {


My guess is that, yes, it should be all async. Unless we decide to initially exclude SHA. But sooner or later something else will come up with async requirement.

/cc @cramforce

jridgewell · 2016-10-18T22:09:28Z

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

-                /* arg*/ false);
+            trigger['selector'] = this.variableService_.expandTemplate(
+                trigger['selector'], trigger, this.config_,
+                 /* opt_event */ undefined, /* opt_iterations */ undefined,


Are these opt parameters required?

nope. I need to pass in the last argument. Any better way to do this?

jridgewell · 2016-10-18T22:14:25Z

extensions/amp-analytics/0.1/variables.js

+ */
+let VariableFilterDef;
+
+function substrFilter() {


Why not declare string, start, and length as parameters?

jridgewell · 2016-10-18T22:16:47Z

extensions/amp-analytics/0.1/variables.js

+    this.register_('toUpperCase', value => value.toUpperCase());
+    this.register_('not', value => String(!value));
+    this.register_('base64', value => base64Encode(String(value)));
+    this.register_('if', (value, thenValue, elseValue) => Boolean(value)


Boolean cast isn't necessary.

forgot in current commit. Will do it in next commit.

jridgewell · 2016-10-18T22:17:35Z

extensions/amp-analytics/0.1/variables.js

+   * @param  {VariableFilterDef} handler [description]
+   */
+  register_(name, handler) {
+    if (this.filters_[name]) {


Make this an assert, so it'll get DCEd.

jridgewell · 2016-10-18T22:30:18Z

extensions/amp-analytics/0.1/variables.js

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


This should be an assert, since the next line match[1] will throw an error anyways.

jridgewell · 2016-10-18T22:34:17Z

extensions/amp-analytics/0.1/variables.js

+    // Replace placeholders with URI encoded values.
+    // Precedence is opt_event.vars > trigger.vars > config.vars.
+    // Nested expansion not supported.
+    return expandTemplate(template, key => {


Can we rename the imported expandTemplate to something else?

removed the import.

jridgewell · 2016-10-18T22:42:20Z

extensions/amp-analytics/0.1/variables.js

+  expandTemplate(template, trigger, config, opt_event, opt_iterations,
+      opt_encode) {
+    opt_iterations = opt_iterations === undefined ? 2 : opt_iterations;
+    opt_encode = opt_encode === undefined ? true : opt_encode;


Calling this opt_encode make me think the default is to not encode. Can we call this opt_no_encode, which its default value being false (you'll have to swap any falsesyou passed before to true).

jridgewell · 2016-10-18T22:49:38Z

extensions/amp-analytics/0.1/variables.js

+    for (let i = 0; i < filters.length; i++) {
+      const parsedFilter = this.parseFilter_(filters[i].trim());
+      if (parsedFilter.fn) {
+        parsedFilter.args.splice(0, 0, value);


jridgewell · 2016-10-18T22:49:50Z

extensions/amp-analytics/0.1/variables.js

+      user().error(TAG, 'Invalid filter name: ' + tokens[0]);
+      return {};
+    }
+    return {fn, args: tokens.splice(1)};


jridgewell · 2016-10-18T22:51:53Z

extensions/amp-analytics/0.1/variables.js

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


I never understood why we turn ${default(a, b)} into encodeURIComponent('default') + "(a, b)".

still not addressed. I am aware of this and have not had the time to go through all the possible scenarios here.

jridgewell · 2016-11-22T20:39:22Z

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

    // Add any given extraUrlParams as query string param
    if (this.config_['extraUrlParams'] || trigger['extraUrlParams']) {
      const params = Object.create(null);
      Object.assign(params, this.config_['extraUrlParams'],
          trigger['extraUrlParams']);
      for (const k in params) {
        if (typeof params[k] == 'string') {
-          params[k] = this.expandTemplate_(params[k], trigger, event);
+          requestPromise = requestPromise


Can this be done in parallel?

jridgewell · 2016-11-22T20:40:19Z

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

        }
      }
-      request = this.addParamsToUrl_(request, params);
+      requestPromise.then(() => {
+        request = this.addParamsToUrl_(request, params);


We can return this instead, and consume the promises value.

jridgewell · 2016-11-22T20:41:09Z

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

+      this.config_['vars']['requestCount']++;
+      return this.variableService_.expandTemplate(
+          request, trigger, this.config_, event)
+        .then(request =>


These can be hoisted out of this then block.

thanks. This is much cleaner!

jridgewell · 2016-11-22T20:45:05Z

extensions/amp-analytics/0.1/variables.js

+function substrFilter(str, s, l) {
+  const start = Number(s);
+  let length = str.length;
+  user().assertNumber(start,


Note that this will allow NaN, which is a number. We have an isFinite that'll probably be better.

changed. Was thinking of putting range checks as well but since substr accepts arbitrary values, didn't add them here.

jridgewell · 2016-11-22T20:46:32Z

extensions/amp-analytics/0.1/variables.js

+}
+
+/**
+ * @param {*} value


Not sure what to put here. Suggestions?

No idea. 😬

jridgewell · 2016-11-22T20:54:51Z

extensions/amp-analytics/0.1/variables.js

+      user().error(TAG, 'Invalid filter name: ' + tokens[0]);
+      return {};
+    }
+    return {fn, args: tokens.slice(1)};


A stretch goal might be to validate the number of arguments matches what we expect.

This is not addressed. Not sure if this should be done at this level or inside the functions.

jridgewell · 2016-11-22T20:55:32Z

extensions/amp-analytics/0.1/variables.js

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


Can we fix #6027 while we're here?

Still not fixed. I have a rudimentary solution but looking more to find a better solution. Suggestions welcome.

jridgewell · 2016-11-22T20:59:53Z

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

-
+            return this.variableService_.expandTemplate(
+                trigger['selector'], trigger, this.config_,
+                /* opt_event */ undefined, /* opt_iterations */ undefined,


This is becoming complex enough to warrant an options object. Not to mention it'll take care of the special opt_iterations must equal undefined to get the default behavior.

technically, I was not changing any of the code here. just moving it from one file to another. If you think options object will be better, I'll add it. Let me know.

3 boolean flags is pushing my code smell button. 😬

opt_event is an object, opt_iterations is a number. opt_no_encode is the only boolean :). Making the change :)

jridgewell · 2016-11-23T01:29:24Z

extensions/amp-analytics/0.1/variables.js

+        return Promise.resolve('');
+      }
+
+      const {name, argList} = this.getNameArgs_(tokens[0].trim());


We can do another #shift to avoid the #slice below.

jridgewell · 2016-11-23T01:30:35Z

extensions/amp-analytics/0.1/variables.js

+
+      // Queue current replacement promise after the last replacement.
+      if (replacementPromise) {
+        replacementPromise = replacementPromise.then(() => p);


We could avoid a lot of promise allocations if we use an array and Promise.all instead.

done. Thanks!

dvoytenko · 2016-11-23T17:51:01Z

@avimehta Still looking for an answer re: sync vs async. It seems that at some point we'll run into an issue if we continue down the sync path. Indeed, we might already have with crypto.

avimehta · 2016-11-23T18:25:30Z

@dvoytenko I already made everything async. Does anything need changing?

dvoytenko · 2016-11-23T19:50:27Z

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

+          request, trigger, this.config_, event)
+        .then(request =>
+          // For consistency with amp-pixel we also expand any url replacements.
+          urlReplacementsForDoc(this.win.document).expandAsync(request))


I'm getting some concerns over the refactoring that @mkhatib is currently doing. You've seen that, right?

yes. Will work with him to resolve the conflicts.

Resolved conflicts. Rebased on top of his changes.

dvoytenko · 2016-11-23T19:53:17Z

@avimehta Sorry if I missed it. I looked at hashFilter_ and it continued to rely on risk-exposed crypto_ var. Is there some misconnect there?

avimehta · 2016-11-28T18:58:14Z

ptal. @mkhatib I was planning to wait for your PR to merge before merging this since your PR is much more complex and more urgent. Let me know if you think otherwise.

jridgewell · 2016-11-28T19:16:58Z

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

@@ -41,6 +42,7 @@ AMP.registerServiceForDoc(
 installActivityService(AMP.win);
 installCidService(AMP.win);
 installCryptoService(AMP.win);
+variableServiceFor(AMP.win);


@mkhatib: is this needed?

Ideally we wouldn't expose this as a service, but instead just create it as an instance var of Instrumentation class.

Instrumentation class is not really involved in this PR (or variable expansion). Why move it there?

@mkhatib Can you look at this PR wrt your decent changes to amp-analytics?

jridgewell · 2016-11-28T19:17:41Z

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

-
+            return this.variableService_.expandTemplate(
+                trigger['selector'], trigger, this.config_,
+                /* opt_event */ undefined, /* opt_iterations */ undefined,


3 boolean flags is pushing my code smell button. 😬

jridgewell · 2016-11-28T19:25:19Z

extensions/amp-analytics/0.1/variables.js

+    let result = Promise.resolve(value);
+    for (let i = 0; i < filters.length; i++) {
+      const parsedFilter = this.parseFilter_(filters[i].trim());
+      if (parsedFilter) {


parsedFilter will always be truthy.

jridgewell · 2016-11-28T19:29:45Z

extensions/amp-analytics/0.1/variables.js

+        user().assertString(value).toLowerCase());
+    this.register_('toUpperCase', value =>
+        user().assertString(value).toUpperCase());
+    this.register_('not', value => String(!value));


Is it useful to propagate the boolean value instead of the string?

Yes, I think it results in consistency. Otherwise falsey values give value: true whereas truthy values give a value: (empty string)

jridgewell · 2016-11-28T19:30:20Z

extensions/amp-analytics/0.1/variables.js

+}
+
+/**
+ * @param {*} value


No idea. 😬

dvoytenko · 2016-11-28T21:36:48Z

extensions/amp-analytics/0.1/variables.js

+let VariableFilterDef;
+
+/**
+ * @param {!string} str


So, in truth, these are always strings, right? If yes - let's declare them as strings; type conversions are already explicit in the code.

dvoytenko · 2016-11-28T21:38:09Z

extensions/amp-analytics/0.1/variables.js

+        'Length ' + length + ' in substr filter should be a number');
+  }
+
+  return str.substr(start, length);


Let's confirm on this example. Is it ever possible that str passed here is a null, aside the fact that it's declared non-nullable?

added allowNull property. I need to add tests but want you guys to go through the code once..

dvoytenko · 2016-11-28T21:39:46Z

extensions/amp-analytics/0.1/variables.js

+
+    this.register_('default', defaultFilter);
+    this.register_('substr', substrFilter);
+    this.register_('trim', value => user().assertString(value).trim());


Ditto: is this really possible that a value is null? If yes, is it really a user error? Is it an error at all? If we did not have a filter here, what value would be sent to the server? My strong suspicion is that it'd be an empty string. If that's the case, we should treat this value here similarly.

This applies to all of the filters below.

started checking for null.

Fixes ampproject#2198

avimehta · 2016-12-01T20:07:32Z

ptal

jridgewell · 2016-12-01T20:33:42Z

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

+      .then(() => this.addParamsToUrl_(request, params))
+      .then(request => {
+        this.config_['vars']['requestCount']++;
+        const expansionOptions = this.expansionOptions_(event, trigger);


Can we reuse the one above?

jridgewell · 2016-12-01T20:45:27Z

extensions/amp-analytics/0.1/variables.js

+      const {filter, args} = this.parseFilter_(filters[i].trim());
+      if (filter) {
+        result = result.then(value => {
+          if (value != null || filter.allowNull) {


We should just call the #filter, with the value and args, let it figure out if it should allow, then it'll call the filtering callback.

That results in most of the 1-line registrations into full fledged functions. Will try it out but thought you should know...

Didn't do this. The change adds bunch of verbosity for not many benefits.

dvoytenko · 2016-12-01T22:35:24Z

extensions/amp-analytics/0.1/variables.js

+ * @struct
+ * @const
+ */
+class FilterDef {


No need in Def - that only for typedefs, not classes.

dvoytenko · 2016-12-01T22:38:08Z

extensions/amp-analytics/0.1/variables.js

+
+/**
+ * @param {!string} str
+ * @param {Number} s


Commented before: since the real args are all strings and we are doing type conversion below - let's just go ahead and type them as strings.

made them all strings.

dvoytenko · 2016-12-01T22:38:57Z

extensions/amp-analytics/0.1/variables.js

+}
+
+
+export class VariableService {


dvoytenko · 2016-12-01T22:40:12Z

extensions/amp-analytics/0.1/variables.js

+    user().assert(fnName, 'Filter ' + name + ' is invalid.');
+    const filter = user().assert(this.filters_[fnName],
+        'Invalid filter name: ' + fnName);
+    return {filter, args: tokens};


Let's clarify: this is either a typedef or a class: it can't be either or, otherwise we may have problems with obfuscation.

dvoytenko · 2016-12-01T22:40:28Z

extensions/amp-analytics/0.1/variables.js

+
+  /**
+   * @param {string} filterStr
+   * @return {Object<string, FilterDef|Array<string>>}


The return type seems wrong?

seems right to me. which part do you think is wrong?

It looks like you return a typedef that looks like this:

@return {!{filter: !Filter, args: !Array<string>}}

Had to use @return {?{filter: !Filter, args: !Array<string>}} as I return null when the filter is not found.

dvoytenko · 2016-12-01T22:41:40Z

extensions/amp-analytics/0.1/variables.js

+
+      // Since the replacement will happen later, return the original template.
+      return match;
+


nit: remove empty line

dvoytenko · 2016-12-01T22:42:41Z

extensions/amp-analytics/0.1/variables.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>}


This looks like a typedef and not a map.

dvoytenko · 2016-12-01T22:44:03Z

extensions/amp-analytics/0.1/variables.js

+    /** @private {!Object<string, FilterDef>} */
+    this.filters_ = map();
+
+    this.register_('default', new FilterDef(defaultFilter, true));


/* allowNulls */ true

dvoytenko · 2016-12-01T22:45:04Z

extensions/amp-analytics/0.1/variables.js

+   */
+  hashFilter_(value) {
+    return cryptoFor(this.win_).then(crypto =>
+      crypto.sha384Base64(user().assertString(value)));


This filter is declared with allowNulls=false, thus value can no longer be null here, right? So we don't need assertString?

removed from here and above.

dvoytenko · 2016-12-01T22:46:09Z

extensions/amp-analytics/0.1/variables.js

+  }
+
+  /**
+   * @param {!string} template The template to expand


string is always !string already, so let's dump ! everywhere. The same is true for number and boolean. All other types need explicit !.

dvoytenko · 2016-12-01T22:47:23Z

@avimehta Yep. That looks right! Thanks for adding it. I left few more (mostly cleanup) comments, but otherwise I'm close to LGTM.

avimehta · 2016-12-02T21:10:28Z

I added all the filter work behind a flag. I want to work on this a bit more before releasing it. Hope that is okay..

dvoytenko · 2016-12-03T00:48:17Z

@avimehta Just one note from me on typedef for the return type in one of the functions. PTAL.

avimehta · 2016-12-06T18:57:40Z

Made some related changes and addressed the comment. ptal.

jridgewell · 2016-12-06T19:54:38Z

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

+      .then(() => {
+        request = this.addParamsToUrl_(request, params);
+        this.config_['vars']['requestCount']++;
+        const expansionOptions = this.expansionOptions_(event, trigger);


Can we reuse the expansionOptions from above?

It is not clear here but the requestCount variable is incremented for each request that is sent. If I reuse, the object, the requestCount doesn't change. Even if I increment it, each request increments the value from 0 to 1(do they have a separate copy in their closure?).

Perfect. Didn't realize the options depended on the request count.

hasOwn doesn't exist when the object is created using `map()`. Added a test as well.

…5621) * Added initial version of amp-anlaytics variable filters. Fixes ampproject#2198

Closes #5621.

…6560) Closes ampproject#5621.

avimehta added the WG: analytics label Oct 14, 2016

avimehta commented Oct 14, 2016

View reviewed changes

avimehta force-pushed the filters branch from e479159 to a1e612d Compare October 14, 2016 21:37

dvoytenko suggested changes Oct 18, 2016

View reviewed changes

jridgewell reviewed Oct 18, 2016

View reviewed changes

avimehta force-pushed the filters branch 2 times, most recently from 8a63990 to 27a9182 Compare November 22, 2016 01:50

jridgewell reviewed Nov 23, 2016

View reviewed changes

dvoytenko reviewed Nov 23, 2016

View reviewed changes

avimehta self-assigned this Nov 28, 2016

jridgewell reviewed Nov 28, 2016

View reviewed changes

dvoytenko suggested changes Nov 28, 2016

View reviewed changes

Added initial version of amp-anlaytics variable filters.

304536b

Fixes ampproject#2198

avimehta force-pushed the filters branch from 1e2db97 to 304536b Compare December 1, 2016 19:43

jridgewell reviewed Dec 1, 2016

View reviewed changes

dvoytenko suggested changes Dec 1, 2016

View reviewed changes

Review feedback.

29f060e

avimehta force-pushed the filters branch from e51b599 to 29f060e Compare December 2, 2016 21:08

gtrufitt mentioned this pull request Dec 5, 2016

AMP-analytics: Passing useragent is truncated due to getNameArgs method #6027

Closed

review feedback.

501bbcd

jridgewell approved these changes Dec 6, 2016

View reviewed changes

dvoytenko approved these changes Dec 7, 2016

View reviewed changes

avimehta merged commit 65177f8 into ampproject:master Dec 7, 2016

avimehta mentioned this pull request Dec 7, 2016

Revert "Added initial version of amp-anlaytics variable filters." #6542

Merged

avimehta added a commit to avimehta/amphtml that referenced this pull request Dec 8, 2016

Fixed a bug with my recent PR: ampproject#5621.

c241a43

hasOwn doesn't exist when the object is created using `map()`. Added a test as well.

avimehta mentioned this pull request Dec 8, 2016

Added initial version of amp-anlaytics variable filters. #6560

Merged

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Added initial version of amp-anlaytics variable filters. (ampproject#…

8f9f715

…5621) * Added initial version of amp-anlaytics variable filters. Fixes ampproject#2198

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Added initial version of amp-anlaytics variable filters. (ampproject#…

02f7b4c

…5621) * Added initial version of amp-anlaytics variable filters. Fixes ampproject#2198

avimehta added a commit that referenced this pull request Jan 10, 2017

Added initial version of amp-anlaytics variable filters. (#6560)

69c6baf

Closes #5621.

jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017

Added initial version of amp-anlaytics variable filters. (ampproject#…

cce80da

…6560) Closes ampproject#5621.


		// Since the replacement will happen later, return the original template.
		return match;

Added initial version of amp-anlaytics variable filters. #5621

Added initial version of amp-anlaytics variable filters. #5621

Conversation

avimehta commented Oct 14, 2016 • edited Loading

avimehta commented Oct 14, 2016

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

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

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

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

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

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

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

avimehta commented Oct 14, 2016 •

edited

Loading