Add test for, and logging of, unpatched amp-geo, (ampproject#20055)

* Add test for, and logging of, unpatched amp-geo,
PHCCorso · Jan 10, 2019 · abc4c14 · abc4c14
1 parent b03c6fd
commit abc4c14
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 21 deletions.
diff --git a/extensions/amp-geo/0.1/amp-geo.js b/extensions/amp-geo/0.1/amp-geo.js
@@ -48,12 +48,12 @@ import {Services} from '../../../src/services';
  */
 import {ampGeoPresets} from './amp-geo-presets';
 
+import {dev, userAssert} from '../../../src/log';
 import {getMode} from '../../../src/mode';
 import {isArray, isObject} from '../../../src/types';
 import {isCanary} from '../../../src/experiments';
 import {isJsonScriptTag, waitForBodyPromise} from '../../../src/dom';
 import {tryParseJson} from '../../../src/json';
-import {userAssert} from '../../../src/log';
 
 /**
  * @enum {number}
@@ -110,6 +110,8 @@ export class AmpGeo extends AMP.BaseElement {
 
     /** @private {number} */
     this.mode_ = mode.GEO_HOT_PATCH;
+    /** @private {boolean} */
+    this.error_ = false;
     /** @private {string} */
     this.country_ = 'unknown';
     /** @private {Array<string>} */
@@ -169,32 +171,38 @@ export class AmpGeo extends AMP.BaseElement {
    * @param {Document} doc
    */
   findCountry_(doc) {
-    // First see if we've been pre-rendered with a country, if so set it
+    // Flag to see if we've been pre-rendered with a country
     const preRenderMatch = doc.body.className.match(PRE_RENDER_REGEX);
+    // Trim the spaces off the patched country
+    const trimmedCountry = COUNTRY.trim();
 
-    if (preRenderMatch &&
-        !Services.urlForDoc(this.element).isProxyOrigin(doc.location)) {
-      this.mode_ = mode.GEO_PRERENDER;
-      this.country_ = preRenderMatch[1];
-    } else {
-      this.mode_ = mode.GEO_HOT_PATCH;
-      this.country_ = COUNTRY.trim();
-      // If we got a country code it will be 2 characters
-      // If the lengths is 0 the country is unknown
-      // If the length is > 2 we didn't get patched
-      // (probably local dev) so we treat it as unknown.
-      if (this.country_.length !== 2) {
-        this.country_ = 'unknown';
-      }
-    }
+    // default country is 'unknown' which is also the zero length case
 
-    // Are we in debug override?
-    // match to \w characters only to prevent xss vector
     if (getMode(this.win).geoOverride &&
       (isCanary(this.win) || getMode(this.win).localDev) &&
       /^\w+$/.test(getMode(this.win).geoOverride)) {
+      // debug override case, only works in canary or localdev
+      // match to \w characters only to prevent xss vector
       this.mode_ = mode.GEO_OVERRIDE;
       this.country_ = getMode(this.win).geoOverride.toLowerCase();
+    } else if (preRenderMatch &&
+        !Services.urlForDoc(this.element).isProxyOrigin(doc.location)) {
+      // pre-rendered by a publisher case, if we're a cache we ignore that
+      // since there is no way the publisher could know the geo of the client.
+      // When caches start pre-rendering geo we'll need to add specifc code
+      // to handle that.
+      this.mode_ = mode.GEO_PRERENDER;
+      this.country_ = preRenderMatch[1];
+    } else if (trimmedCountry.length == 2) {
+      // We have a valid 2 letter ISO country
+      this.mode_ = mode.GEO_HOT_PATCH;
+      this.country_ = trimmedCountry;
+    } else if (trimmedCountry.length > 2 && !getMode(this.win).localDev) {
+      // We were not patched, if we're not in dev this is an error
+      // and we leave the country at the default 'unknown'
+      this.error_ = true;
+      dev().error(TAG,
+          'GEONOTPATCHED: amp-geo served unpatched, ISO country not set');
     }
   }
 
@@ -307,6 +315,10 @@ export class AmpGeo extends AMP.BaseElement {
             classesToAdd.push('amp-geo-no-group');
           }
 
+          if (self.error_) {
+            classesToAdd.push('amp-geo-error');
+          }
+
           states.ISOCountryGroups = self.matchedGroups_;
           classesToAdd.push(COUNTRY_PREFIX + this.country_);
 

diff --git a/extensions/amp-geo/0.1/test/test-amp-geo.js b/extensions/amp-geo/0.1/test/test-amp-geo.js
@@ -62,7 +62,6 @@ describes.realWin('amp-geo', {
   let el;
   let userErrorStub;
 
-
   beforeEach(() => {
     userErrorStub = sandbox.stub(user(), 'error');
     win = env.win;
@@ -320,6 +319,10 @@ describes.realWin('amp-geo', {
     });
   });
 
+  /**
+   * pre-rendered geo is the the case where a publisher uses their own
+   * infrastructure to add a country tag to the body.
+   */
   it('should respect pre-rendered geo tags', () => {
     addConfigElement('script');
     doc.body.classList.add('amp-iso-country-nz', 'amp-geo-group-anz');
@@ -381,6 +384,20 @@ describes.realWin('amp-geo', {
     return expect(Services.geoForDocOrNull(el)).to.eventually.equal(null);
   });
 
+  it('geo should log an error if unpatched in production. ', () => {
+    expectAsyncConsoleError(/GEONOTPATCHED/);
+    sandbox.stub(win.AMP_MODE, 'localDev').value(false);
+    addConfigElement('script');
+
+    geo.buildCallback();
+    return Services.geoForDocOrNull(el).then(geo => {
+      expect(geo.ISOCountry).to.equal('unknown');
+      expectBodyHasClass([
+        'amp-geo-error',
+      ], true);
+    });
+  });
+
   it('should throw if it has multiple script child elements', () => {
     expect(() => {
       addConfigElement('script');
@@ -412,7 +429,9 @@ describes.realWin('amp-geo', {
   it('should error if the child script element has non-JSON content', () => {
     expect(() => {
       addConfigElement('script', 'application/json', '{not json}');
-      geo.buildCallback();
+      allowConsoleError(() => {
+        geo.buildCallback();
+      });
     }).to.throw(/JSON/);
   });