AndyRussG has uploaded a new change for review.
https://gerrit.wikimedia.org/r/260311
Change subject: Fix fallback to geoiplookup.wikimedia.org for empty IPv6 geo
cookies
......................................................................
Fix fallback to geoiplookup.wikimedia.org for empty IPv6 geo cookies
The code was treating country.length = 0 as valid which doesn't
make any sense. This was explicitly causing the code to NOT fallback
to geoiplookup for IPv6 Geo cookies.
From what I can tell the previous code *never* triggered the fallback
for IPv6 cookies.
Follows-up 9af91ae72adca97 which seems to have caused this regression.
Before that commit, the code used 'mw.centralNotice.data.country'
instead of 'Geo.country' to decide whether to load the fallback lookup
and that property fell back to 'XX' if empty or falsey (includes
empty string). The new code has no default of 'XX', but still treated
empty string as valid value.
Bug: T121938
Bug: T121926
Change-Id: Ie5865fbcb981f98f800af2ecf2f883debb8dedaf
---
M resources/subscribing/ext.centralNotice.geoIP.js
M tests/qunit/subscribing/ext.centralNotice.geoIP.tests.js
2 files changed, 35 insertions(+), 7 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CentralNotice
refs/changes/11/260311/1
diff --git a/resources/subscribing/ext.centralNotice.geoIP.js
b/resources/subscribing/ext.centralNotice.geoIP.js
index 93e746d..864ea97 100644
--- a/resources/subscribing/ext.centralNotice.geoIP.js
+++ b/resources/subscribing/ext.centralNotice.geoIP.js
@@ -12,6 +12,11 @@
* Parse geo data in cookieValue and return an object with properties
from
* the fields therein. Returns null if the value couldn't be parsed.
*
+ * The cookie will look like one of the following:
+ * - "US:CO:Denver:39.6762:-104.887:v4"
+ * - ":::::v6"
+ * - ""
+ *
* @param {string} cookieValue
* @returns {?Object}
*/
@@ -77,13 +82,17 @@
* @returns boolean
*/
function isGeoDataValid( geoObj ) {
- // TODO: Verify that the following is correct, especially the
check for
- // an empty country string.
- // Note: In refactoring, we added the check for geoObj.af !==
'vx',
- // which wasn't in the previous version of the code. Following
the
- // refactor, it (or something similar) is necesssary, though.
+ // - Ensure 'country' is set to detect whether the cookie was
succesfully
+ // parsed by parseCookieValue().
+ // - Ensure 'country' is non-empty to detect empty values for
when
+ // geo lookup failed (typically on IPv6 connections). This
check
+ // is mandatory as otherwise the below code does not fallback
to
+ // geoiplookup.wikimedia.org (IPv4-powered).
+ // - The check for geoObj.af !== 'vx' became mandatory in recent
+ // refactoring to account for the temporary Geo value for
during the
+ // lookup request. It (or something similar) is necesssary.
return ( typeof geoObj.country === 'string' &&
- ( geoObj.country.length === 2 || geoObj.country.length
=== 0 ) &&
+ geoObj.country.length > 0 &&
geoObj.af !== 'vx' );
}
@@ -194,4 +203,4 @@
mw.geoIP.setWindowGeo();
-} )( jQuery, mediaWiki );
\ No newline at end of file
+} )( jQuery, mediaWiki );
diff --git a/tests/qunit/subscribing/ext.centralNotice.geoIP.tests.js
b/tests/qunit/subscribing/ext.centralNotice.geoIP.tests.js
index 2c5db0e..3e7ae46 100644
--- a/tests/qunit/subscribing/ext.centralNotice.geoIP.tests.js
+++ b/tests/qunit/subscribing/ext.centralNotice.geoIP.tests.js
@@ -4,6 +4,7 @@
var
COOKIE_NAME = 'GeoIP',
BAD_COOKIE = 'asdfasdf',
+ UNKNOWN_IPV6_COOKIE = ':::::v6',
GOOD_COOKIE = 'US:CO:Denver:39.6762:-104.887:v4',
GOOD_GEO = {
af: 'v4',
@@ -86,4 +87,22 @@
} );
} );
+ QUnit.test( 'unknown ipv6 cookie', 3, function ( assert ) {
+ $.cookie( COOKIE_NAME, UNKNOWN_IPV6_COOKIE, { path: '/' } );
+ mw.geoIP.deferred = $.Deferred();
+ window.Geo = null;
+
+ $.ajax = function () {
+ assert.equal( window.Geo.af, 'vx', 'geo filled with vx'
);
+ window.Geo = GOOD_GEO;
+ return $.Deferred().resolve().promise();
+ };
+
+ mw.geoIP.setWindowGeo();
+ mw.geoIP.getPromise().done( function () {
+ assert.equal( $.cookie( COOKIE_NAME ), GOOD_COOKIE,
'cookie updated' );
+ assert.deepEqual( window.Geo, GOOD_GEO, 'good geo was
loaded' );
+ } );
+ } );
+
}( mediaWiki, jQuery ) );
--
To view, visit https://gerrit.wikimedia.org/r/260311
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie5865fbcb981f98f800af2ecf2f883debb8dedaf
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/CentralNotice
Gerrit-Branch: wmf_deploy
Gerrit-Owner: AndyRussG <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
