Remove A4A Dependency on ads/_config.js #9462

bradfrizzell · 2017-05-22T14:34:21Z

bradfrizzell · 2017-05-22T14:35:01Z

This is still a WIP, I only made the changes for doubleclick to make sure this is what you meant, Hongfei. If this is what you intended, I'll go through and make the changes to all the other networks and the docs.

lannka

Overall looks good. Thanks for the change.

lannka · 2017-05-22T17:57:12Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

@@ -134,3 +134,16 @@ export function doubleclickIsA4AEnabled(win, element) {
  }
  return enableA4A;
 }
+
+export const adConfig = {


i guess not all fields are used in a4a. can you check what are necessary?

I tried removing them all one by one and no tests broke. @keithwrightbos do you know which ones we definitely don't need here?

we're using preconnect at least. you will need to take a look at the code.

renderStartImplemented gets used in XoriginIframeHandler, which gets called from within the A4A impl so we need that too. Everything else can go.

i took a look at the code, cid is also used, in utils.js

renderStart is not used when we do preferential rendering, but we can have a case where fast fetch is used, but it ends up failing client side validation and falling back to using XOriginIframeHandler, in which case renderStart does apply

Also, where are you seeing clientIdScope being used? When I search for it I don't get anything back related to FF.

for fast fetch fall back case, we still skip renderstart: https://github.com/ampproject/amphtml/blob/master/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js#L237

clientIdScope is used by ad-cid.js .

ah I see, my mistake. fixing

:-) now i start to think, why didn't we do prefetch?

keithwrightbos · 2017-05-23T14:39:23Z

extensions/amp-a4a/0.1/test/utils.js

@@ -16,6 +16,8 @@

 import {AmpA4A} from '../amp-a4a';
 import {base64UrlDecodeToBytes} from '../../../../src/utils/base64';
+import {adConfig,} from


nit: remove comma

keithwrightbos · 2017-05-23T15:11:04Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -363,8 +362,6 @@ export class AmpA4A extends AMP.BaseElement {

  /** @override */
  buildCallback() {
-    const adType = this.element.getAttribute('type');
-    this.config = adConfig[adType] || {};


If we're expecting that objects have this.config variable then I suggest we make it explicitly part of the constructor of AmpA4A. I'm not a fan of this expectation that the object just exist in implementations. Seems unclear to me and future implementors will likely not be aware of how to integrate.

I would instead propose a new, explicit, overrideable function:

/** @return {!Array} /
getPreconnectUrls() { return []; }
/* @return {!Array} */
getPrefetchUrls() { return []; }

lannka · 2017-05-23T22:40:20Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -403,6 +400,16 @@ export class AmpA4A extends AMP.BaseElement {
    return true;
  }

+  /** @return {!Array|!String} */
+  getPreconnectUrls() {
+    return false;


i'm sure this gives you a type check error.
why not just return []?

lannka · 2017-05-23T22:40:59Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+
+  /** @override */
+  getPreconnectUrls() {
+    return 'https://googleads.g.doubleclick.net';


return ['https://googleads.g.doubleclick.net'];

was there any reason specifically you want this to be an array instead of a string? the way it was previously implemented handled it either as a string or an array of strings.

simpler typing, less bug-prone.

to be specific, so i want the method to return {!Array<string>}. So the caller can to results.length freely

lannka · 2017-05-24T18:26:06Z

ads/google/a4a/utils.js

-    const slotNumber = window['ampAdGoogleIfiCounter']++;
-    const win = a4a.win;
-    const documentInfo = documentInfoForDoc(adElement);
+  return getClientScopedAdCid(a4a.getAmpDoc(), a4a.win, 'AMP_ECID_GOOGLE').then(


don't hard code here. we should let each adnetwork to config the CID scope

This is a util specifically for google though, so only used by adsense and doubleclick

ah, true. i thought it's a common logic should be shared. but fine. it's not the scope of this PR.

keithwrightbos · 2017-05-24T19:48:20Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -403,6 +400,16 @@ export class AmpA4A extends AMP.BaseElement {
    return true;
  }

+  /** @return {!Array|!string} */


Add some documentation indicating what these are for and when they should be overridden. Also type should be !Array<string>|string

Added docstring, let me know if it's ok.

keithwrightbos · 2017-05-24T19:49:50Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -423,10 +430,10 @@ export class AmpA4A extends AMP.BaseElement {
  preconnectCallback(unusedOnLayout) {
    this.preconnect.preload(this.getSafeframePath_());
    this.preconnect.preload(getDefaultBootstrapBaseUrl(this.win, 'nameframe'));
-    if (!this.config) {
+    const preconnect = this.getPreconnectUrls();
+    if (preconnect.length == 0) {


I would argue this is unnecessary. If its an empty array it would result in no preconnects

keithwrightbos · 2017-05-24T19:50:33Z

src/ad-cid.js

+ *     - the ad network does not request one or
+ *     - `amp-analytics` which provides the CID service was not installed.
+ */
+export function getClientScopedAdCid(doc, win, clientIdScope,


Indentation of parameters here looks a bit off but linter has the last word!

changed where I split the line

keithwrightbos · 2017-05-24T19:50:57Z

src/ad-cid.js

-      return;
-    }
+     return;
+   }


Same here, I would expect this to be one column to the right

keithwrightbos · 2017-05-24T20:36:57Z

extensions/amp-a4a/0.1/amp-a4a.js

+   * Returns preconnect urls for A4A. Ad network should overwrite in their
+   * Fast Fetch implementation and return an array of urls for the runtime to
+   * preconnect to.
+   * @return {!Array<string>|string}


So now one can be string while there other cannot? Let's be consistent (personally I would be fine with just an array every time)

keithwrightbos · 2017-05-24T20:38:33Z

src/ad-cid.js

+}
+
+/**
+ * @param {!AmpDoc$$module$src$service$ampdoc_impl} doc


Whoa, is the valid? I would expect it to be {!../service/ampdoc-impl.AmpDoc}

check-types didn't complain about it, but I'll change to the way you have it

keithwrightbos · 2017-05-24T20:39:05Z

src/ad-cid.js

+ * @param {!AmpDoc$$module$src$service$ampdoc_impl} doc
+ * @param {!Window} win
+ * @param {!string} clientIdScope
+ * @param {?string=} opt_clientIdCookieName


remove ? as it implies you could pass null

keithwrightbos · 2017-05-24T20:52:53Z

src/ad-cid.js

-      scope: dev().assertString(config.clientIdScope),
-      cookieName: config.clientIdCookieName,
+      scope: dev().assertString(clientIdScope),
+      cookieName: opt_clientIdCookieName || undefined,


Why do you need || undefined here? You're guarding against a null value for cookieName?

ah shoot meant to change that when I changed the type in the jsdoc

lannka · 2017-05-24T20:58:13Z

src/ad-cid.js

+ *     - `amp-analytics` which provides the CID service was not installed.
+ */
+export function getClientScopedAdCid(
+    doc, win, clientIdScope, opt_clientIdCookieName) {


doc -> ampdoc

win is not needed. can be ampdoc.win

lannka · 2017-05-24T20:59:12Z

src/ad-cid.js

+ *     - the ad network does not request one or
+ *     - `amp-analytics` which provides the CID service was not installed.
+ */
+export function getClientScopedAdCid(


naming suggestion: getOrCreateAdCid

In my opinion, that's not really an accurate name. This function can return undefined, it won't always actually make an AdCid.

ClientScopedAdCid does not make sense, since CID implies "client", and they're always scoped.

lannka · 2017-05-24T20:59:44Z

src/ad-cid.js

+ * @param {!Window} win
+ * @param {!string} clientIdScope
+ * @param {string=} opt_clientIdCookieName
+ * @return {!Promise<string|undefined>} A promise for a CID or undefined if


update the doc

Are you referring to that this and getAdCid have the same description of the return value? I don't think that's inaccurate

... or undefined if the ad network does not request one or

does not apply here, as there is no "ad network" involved here.

lannka · 2017-05-26T20:25:20Z

src/ad-cid.js

+ * @param {string=} opt_clientIdCookieName
+ * @return {!Promise<string|undefined>} A promise for a CID or undefined.
+ */
+export function getAdCidHelper(


I would expect a method named "getXXHelper" returns a util object for XX.

getOrCreateAdCid is actually a good name, it returns undefined only when there's an error (service does not exist, or timed out).

lannka · 2017-05-26T20:26:07Z

extensions/amp-a4a/0.1/test/utils.js

+
+  /** @override */
+  getPreconnectUrls() {
+    return 'https://googleads.g.doubleclick.net';


lannka · 2017-05-26T20:28:22Z

ads/google/a4a/utils.js

-    const slotNumber = window['ampAdGoogleIfiCounter']++;
-    const win = a4a.win;
-    const documentInfo = documentInfoForDoc(adElement);
+  return getAdCidHelper(a4a.getAmpDoc(), 'AMP_ECID_GOOGLE').then(


please add '_ga' as the 3rd param of the call, since we're migrating to GA cookie name. see #9591

lannka

LGTM, one more comment

lannka · 2017-05-30T18:26:47Z

src/ad-cid.js

+ * @return {!Promise<string|undefined>} A promise for a CID or undefined.
+ */
+export function getOrCreateAdCid(
+    ampDoc, win, clientIdScope, opt_clientIdCookieName) {


any reason why win is added back?

I added it back because ampDoc.win was not valid and was causing test failures.

can you fix the test instead?

sure, as long as ampDoc.win is something that should exist, I can fix that.

yes, it should exist: https://github.com/ampproject/amphtml/blob/master/src/service/ampdoc-impl.js#L186

Done, turns out the issue was just with the local testing behavior not working for no reason

bradfrizzell · 2017-06-01T16:35:53Z

Can you please merge, Hongfei? I don't have merge rights.

…

On Thu, Jun 1, 2017 at 12:21 PM, Hongfei Ding ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9462 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEFDACXy6lPE5tevM2pvWq5Yz_QHWaheks5r_uUUgaJpZM4NicXq> .

-- Brad Frizzell | Software Engineer | bradfrizzell@google.com | 201-421-5676

* Added doubleclick specific config * Modified other networks * Update MockA4AImpl * Remove unneeded config lines * Add cid, remove renderStartImplemented * Fix lint complaint * Changed from mandatory config to overrrideable method * Addressed feedback, modified getAdCid * Addressed additional feedback * Responded to feedback * Removed unnneeded guard against null value * Changed signature of function * Fixed lint issues * addressed comments * Addressed comments, fixed broken test * Change signature / update tests

* Bidtellect amp implementation… (ampproject#9518) * Bidtellect amp implementation… * removing renderStartImplemented… * spaces? * adding example * Fix test-amp-ad-network-doubleclick-impl (ampproject#9645) * Optimizations to speed up Travis PR builds (ampproject#9626) Travis builds have become slow during busy periods of the day due to various reasons. This PR does the following: Reorganizes build tasks across fewer build shards Reduces the number of VMs used by PR and master builds to 2 No longer does a full gulp dist for unit tests (Possible to do after ampproject#9404) Reduces the total CPU time used for PR builds to < 20 mins Fixes ampproject#9500 Fixes ampproject#9387 * Animations: full on-action API (ampproject#9641) * Animations: full on-action API * review fixes * Fix Validator tests on Travis, which now uses Node v4 (ampproject#9654) * Amp-imgur : Implement imgur embed (ampproject#9405) (ampproject#9434) * Amp-imgur : Implement imgur embed (ampproject#9405) * amp-imgur test code modified * amp-imgur validation code changed * fix amp-imgur lint check * Remove unused Layour variables * Test code modified * Ingegration amp-imgur extension * [amp-imgur] add resize logic * Remove 3p iframes and add resize message listener * Fix some code with reviews * change some syntax * change validation rules * add find event source * [amp-imgur] remove unnecessary code and add object check * remove spec_name from validation code * Remove unnecessary code and add owners file * Change required tag name to in validation file * data-imgurid -> data-imgur-id * Add whitelist for amp-imgur * remove unused AmpImgur import in test file * remove unused preconnect callback * Update amp-cors-requests.md (ampproject#9636) Added updates and corrections per Dima's feedback. * Expose login url building to amp access service (ampproject#9300) * Animations: support style keyframes (ampproject#9647) * Animations: support style keyframes * docs * review fixes * lints * Animations: subtargets format (ampproject#9655) * Animations: subtargets format * fix docs * Fix extern for integration test (ampproject#9657) * Remove whitelisted link for PR 9434 (ampproject#9656) * Add callouts for CORS + cleanup (ampproject#9591) * Add callouts for CORS + cleanup * Update amp-access.md * Doubleclick Fast Fetch: Send default safeframe version on ad request (ampproject#9613) * Send default safeframe version on ad request * fix test failure * amp-ima-video: Fixes some undefined variables in compiled extension (ampproject#9617) * Add experimental input-debounced to the actions and event doc (ampproject#9650) * Copy and rename compiled script directly for deprecated version (ampproject#9587) * Remove A4A Dependency on ads/_config.js (ampproject#9462) * Added doubleclick specific config * Modified other networks * Update MockA4AImpl * Remove unneeded config lines * Add cid, remove renderStartImplemented * Fix lint complaint * Changed from mandatory config to overrrideable method * Addressed feedback, modified getAdCid * Addressed additional feedback * Responded to feedback * Removed unnneeded guard against null value * Changed signature of function * Fixed lint issues * addressed comments * Addressed comments, fixed broken test * Change signature / update tests * Fix wrong contents and white spaces in amp-imgur.md (ampproject#9658) * Performance: separate ini-load from first visible ini-load (ampproject#9665) * amp-bind: Fix more integration tests (ampproject#9598) * more bind test fixes * include object.assign polyfill * move polyfills to separate file * replace use of map() from bind-validator with ownProperty() * fix types and test * Stop using IOS elastic scroll when slidescroll takes over animation (Only for the NON SNAP-POINTS flow) (ampproject#9668) * removing overflow touch on no-scroll for ios * Stop using IOS elastic scroll when slidescroll takes over animation (Only for the NON SNAP-POINTS flow) * make sure css is compiled before entry points (ampproject#9643) * update npm package version (ampproject#9671) * A4A: expose visibilityCsi (ampproject#9667) * A4A: expose visibilityCsi * tests * amp-bind: Fix embedding in FIF (ampproject#9541) * move element service changes from ampproject#9447 * store element service ids in extension struct * add unit test, fix other tests * Fix null value binding (ampproject#9674) * Add amp-form's submit action to the docs (ampproject#9675) * Write cookie for ad-cid. (ampproject#9594) * Write cookie for ad-cid. * fix tests * amp-access-laterpay fixes (ampproject#9633) * Add support for LaterPay subscriptions * Fix canonical link on laterpay example * Tweak default styling * Add an example locale message for the header * Fix indentation of JsDoc (ampproject#9677) It should be indented at the same level as the method it's documenting. * Add example to amp iframe docs (ampproject#9660) * Change image to static link hosted on ampproject.org * Update amp-iframe doc w example + gen cleanup * Adds SFG experiment branches to doubleclick-a4a-config.js. (ampproject#9662) * Added experiment branches corresponding to exp=a4a:(5|6). * Removed test file. * Removed changes to yarn.lock + minor fixes. * Fixed adsense-a4a-config.js. * Fixed return statement. * Removed reference to style tag for opacity (ampproject#9634) As style tag for `opacity` was replaced with `amp-boilerplate (back in this [commit](ampproject@0a056ca), updated the text to reflect those changes. * De-flake amp-bind integration tests (ampproject#9683) * split bind fixtures into one file per extension * fix width/height binding bug, avoid race condition w/ dynamic containers * actually, just skip the tests affected by race condition for now * Remove timer calls to prevent future flakiness (ampproject#9478) * Remove timer calls to prevent future flakiness * Revert the line order change * Poll instead of using stub callback constructor * Add jsdoc and rename variables * amp-animation: polyfill partial keyframes from CSS (ampproject#9689) * Animations: whitelist offset distance and regroup condition types (ampproject#9688) * Use Docker containers in Travis (ampproject#9666) * clean up web-worker experiment (ampproject#9706) * Load examiner.js when #development=2 (ampproject#9680) * Load examiner.js when #development=1 * address comments * Use development=2 * Fix presubmit * Implementation of amp-ad-exit (ampproject#9390) * Implementation of the amp-ad-exit component for managing ad navigation (ampproject#8515). Changes from the I2I: - Only RANDOM, CLICK_X, and CLICK_Y variables can be used, alongside custom vars. - It doesn't work well with <a> tags - the tap action doesn't trigger on middle clicks. Recommendation is to use other elements. - ClickLocationFilter for border click protection is not yet implemented. It will be added in a future change. The component does some lightweight validation of the JSON config when it's built. This may be overkill. Tested: gulp test --files 'extensions/amp-ad-exit/0.1/test/*' gulp check-types * Update JSDoc for some functions. Fix lint issue (ignore unused parameters in an interface method). * sinon.spy -> sandbox.spy * Add amp-ad-exit to the list of allowed extensions in A4A pages and fix validation rules. Address PR: add experiment flag; use camelCase for config properties address some comments Instantiate a new Filter for each spec. * Instantiate a new Filter for each spec. * remove duplicate doctype tag in example * Make ActionEventDef public for type annotations. * Add filter factory. * fix event var * update type annotation for Filter.filter * address PR comments * Move filter config validation to the ctor. Remove Filter.buildCallback(). * camelCase in documentation examples * Introducing "it.yield", a convenient way to test promise code (ampproject#9601) * Introduce yield. * Address comments. * fix done state error handling * Document amp-access as a special target (ampproject#9568) `amp-access` is a special target that's not mentioned in this document. It's special because you can't give an arbitrary ID to it, i.e., you can't just change the id of the `amp-access` script tag to `amp-access2` and expect `on="tap:amp-access2.login"` to work. Given that the "actions" for `amp-access` is a bit dynamic depending on the structure of the `amp-access` JSON, instead of listing out the actions in a tabular form, a reference to the `amp-access` documentation is added. * Run presubmit tests soon after building (ampproject#9716) * Revert "Use Docker containers in Travis (ampproject#9666)" (ampproject#9717) This reverts commit 98ecb9b. It turns out that while using Docker containers instead of VMs on Travis reduces VM startup time, it has significantly increased build and test time, increasing the overall PR check round trip from < 20 mins to > 30 mins. Compare https://travis-ci.org/ampproject/amphtml/jobs/238464404 (Docker) against https://travis-ci.org/ampproject/amphtml/jobs/238462066 (VM). Fixes ampproject#9651 * Validator Rollup (ampproject#9719) * Add validator tests for amp-imgur. * Disallow amp-embed as child of amp-app-banner. * Minor cleanup, Make sure our set numbers are consistent in javascript. * amp-state: Allow both `src` and script child (ampproject#9721) * adapt code from ampproject#8897 * also_requires_attr should be in trigger * Remove experiment for input-debounced. Update docs. (ampproject#9724) * Validator Rollup (ampproject#9727) * Minor cleanup of amp-ad-exit * Disallow amp-ad/amp-embed as children of amp ad containers when data-multi-size is present. * Revision bump * Enabling dynamic queryparam addition to anchour links (ampproject#9684) * Enabling queryparam addition to anchour links * Code review changes * Additional cache urls (ampproject#9733) * Significantly speed up gulp build --css-only (ampproject#9726) Prior to this PR, gulp build --css-only would take ~ 1 minute to run, and end up building a bunch of js files in addition to compiling css. After this PR, gulp build --css-only takes ~ 0.5 seconds to run (a 100x speed up), and the only output files in build/ after running it are .css, and .css.js files. Resulting speeds on my workstation: gulp build --css-only: ~500ms gulp build: ~1m 45s gulp dist --fortesting: ~3m 30s Fixes ampproject#9640 * add amp-analytics Facebook Pixel support (ampproject#9449) * add amp-analytics Facebook Pixel support * add amp-analytics Facebook Pixel support * fix test on extensions/amp-analytics/0.1/test/vendor-requests.json * add Facebook Pixel standard events https://developers.facebook.com/docs/ads-for-websites/pixel-events/v2.9#events * Update vendors.js fix conflicts * Update vendors.js fix conflicts * Update vendor-requests.json fix conflicts * Update vendor-requests.json fix conflicts * Update vendor-requests.json * Update analytics.amp.html * Update analytics-vendors.amp.html * Update vendors.js * Update analytics-vendors.amp.html * fix lint error * update * facebook pixel * Update vendor-requests.json * Update analytics.amp.html * fix lint error * fix lint error * fix alphabetical order * Update yarn.lock * AMP-ad supports Seznam Imedia ad network (ampproject#8893) * AMP-ad supports Seznam Imedia ad network * Fixed requested changes * Fixed requested changes * Render API implemented * Improved placing ads * Fixed requested changes * Render API fixed context * Renamed IM.cz to Imedia * Described JSON parameters

bradfrizzell requested a review from lannka May 22, 2017 14:34

googlebot added the cla: yes label May 22, 2017

bradfrizzell requested a review from keithwrightbos May 22, 2017 14:35

lannka requested changes May 22, 2017

View reviewed changes

keithwrightbos requested changes May 23, 2017

View reviewed changes

lannka reviewed May 23, 2017

View reviewed changes

lannka reviewed May 24, 2017

View reviewed changes

keithwrightbos requested changes May 24, 2017

View reviewed changes

keithwrightbos approved these changes May 24, 2017

View reviewed changes

lannka reviewed May 24, 2017

View reviewed changes

lannka reviewed May 26, 2017

View reviewed changes

lannka reviewed May 30, 2017

View reviewed changes

bradfrizzell added 15 commits May 31, 2017 14:02

Added doubleclick specific config

ead3fa8

Modified other networks

cad4a51

Update MockA4AImpl

f04515f

Remove unneeded config lines

e5bd193

Add cid, remove renderStartImplemented

e051e42

Fix lint complaint

2f67876

Changed from mandatory config to overrrideable method

b92cd7e

Addressed feedback, modified getAdCid

8a1a68b

Addressed additional feedback

ea717a9

Responded to feedback

c23f3bf

Removed unnneeded guard against null value

70680f2

Changed signature of function

3834881

Fixed lint issues

c98f0e1

addressed comments

ea752ed

Addressed comments, fixed broken test

1d47c0f

Change signature / update tests

8d9bac9

lannka approved these changes Jun 1, 2017

View reviewed changes

Merge branch 'master' into frizz-split-config-out

b8fd145

keithwrightbos merged commit 0d30ecf into ampproject:master Jun 1, 2017

keithwrightbos deleted the frizz-split-config-out branch June 1, 2017 18:06

ampprojectbot added the PR use: In Beta / Experimental label Jun 12, 2017

Remove A4A Dependency on ads/_config.js #9462

Remove A4A Dependency on ads/_config.js #9462

Conversation

bradfrizzell commented May 22, 2017

bradfrizzell commented May 22, 2017

lannka left a 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

lannka left a 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

bradfrizzell commented Jun 1, 2017 via email