Skip to content

Commit

Permalink
Adding warning
Browse files Browse the repository at this point in the history
  • Loading branch information
Micajuine Ho committed Apr 23, 2020
1 parent 2dc62e8 commit 9d15c94
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 85 deletions.
52 changes: 29 additions & 23 deletions extensions/amp-analytics/0.1/analytics-root.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,11 +301,8 @@ export class AnalyticsRoot {
userAssert(false, `Invalid query selector ${selector}`);
}
for (let j = 0; j < nodeList.length; j++) {
if (
this.contains(nodeList[j]) &&
this.elementContainsDataVarsAttribute_(nodeList[j])
) {
elementArray.push(nodeList[j]);
if (this.contains(nodeList[j])) {
this.checkElementDataVars(nodeList[j], elementArray, selector);
}
}
userAssert(elementArray.length, `Element "${selector}" not found`);
Expand All @@ -319,6 +316,33 @@ export class AnalyticsRoot {
});
}

/**
* Check if the given element has a data-vars attribute.
* Warn if not the case.
* @param {!Element} element
* @param {!Array<!Element>} elementArray
* @param {string} selector
*/
checkElementDataVars(element, elementArray, selector) {
const dataVarKeys = Object.keys(
getDataParamsFromAttributes(
element,
/* computeParamNameFunc */ undefined,
VARIABLE_DATA_ATTRIBUTE_KEY
)
);
if (dataVarKeys.length) {
elementArray.push(element);
} else {
user().warn(
TAG,
'An element was ommited from selector "%s"' +
' because no data-vars-* attribute was found.',
selector
);
}
}

/**
* Searches the AMP element that matches the selector within the scope of the
* analytics root in relationship to the specified context node.
Expand Down Expand Up @@ -384,24 +408,6 @@ export class AnalyticsRoot {
}
}

/**
* Ensure that the element has a data-vars attribute for
* unique requests.
* @param {!Element} element
* @return {boolean}
*/
elementContainsDataVarsAttribute_(element) {
return (
Object.keys(
getDataParamsFromAttributes(
element,
/* computeParamNameFunc */ undefined,
VARIABLE_DATA_ATTRIBUTE_KEY
)
).length !== 0
);
}

/**
* Creates listener-filter for DOM events to check against the specified
* selector. If the node (or its ancestors) match the selector the listener
Expand Down
28 changes: 20 additions & 8 deletions extensions/amp-analytics/0.1/test/test-analytics-root.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
} from '../visibility-manager';
import {VisibilityManagerForMApp} from '../visibility-manager-for-mapp';
import {toggleExperiment} from '../../../../src/experiments';
import {user} from '../../../../src/log';

describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => {
let win;
Expand Down Expand Up @@ -391,16 +392,21 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => {
});

it('should only find elements with data-vars-*', async () => {
const spy = env.sandbox.spy(user(), 'warn');

child.classList.add('myClass');
child2.classList.add('myClass');
child3.classList.add('myClass');

child3.removeAttribute('data-vars-id');

expect(await root.getAmpElements(body, ['.myClass'])).to.deep.equal([
child,
child2,
]);
const children = await root.getAmpElements(body, ['.myClass']);
expect(spy).callCount(1);
expect(spy).to.have.been.calledWith(
'amp-analytics/analytics-root',
'An element was ommited from selector "%s" because no data-vars-* attribute was found.',
'.myClass'
);
expect(children).to.deep.equal([child, child2]);
});

it('should remove duplicate elements found', async () => {
Expand Down Expand Up @@ -756,6 +762,7 @@ describes.realWin(
});

it('should find all elements by selector', async () => {
const spy = env.sandbox.spy(user(), 'warn');
const child2 = win.document.createElement('child');
const child3 = win.document.createElement('child');
const child4 = win.document.createElement('child');
Expand All @@ -778,9 +785,14 @@ describes.realWin(
child2.setAttribute('data-vars-id', '456');
child3.setAttribute('data-vars-id', '789');
parentChild.setAttribute('data-vars-id', 'abc');
expect(
await root.getAmpElements(body, ['.myClass'], null)
).to.deep.equals([child, child2]);
const elements = await root.getAmpElements(body, ['.myClass'], null);
expect(spy).callCount(1);
expect(spy).to.have.been.calledWith(
'amp-analytics/analytics-root',
'An element was ommited from selector "%s" because no data-vars-* attribute was found.',
'.myClass'
);
expect(elements).to.deep.equals([child, child2]);
// Check that non-experiment version works
toggleExperiment(win, 'visibility-trigger-improvements', false);
expect(
Expand Down
56 changes: 2 additions & 54 deletions extensions/amp-analytics/0.1/test/test-events.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1944,14 +1944,10 @@ describes.realWin('Events', {amp: 1}, (env) => {
describe('multi selector visibility trigger', () => {
let unlisten;
let unlisten2;
let unlisten3;
let target3;
let config;
let readyPromise;
let targetSignals2;
let targetSignals3;
let saveCallback2;
let saveCallback3;
let eventsSpy;
let res;
let error;
Expand All @@ -1961,53 +1957,33 @@ describes.realWin('Events', {amp: 1}, (env) => {
readyPromise = Promise.resolve();
unlisten = env.sandbox.spy();
unlisten2 = env.sandbox.spy();
unlisten3 = env.sandbox.spy();
config = {};

eventsSpy = env.sandbox.spy(tracker, 'onEvent_');

target3 = win.document.createElement('div');
target3.classList.add('target2');
win.document.body.appendChild(target3);

target2.classList.add('i-amphtml-element');
targetSignals2 = new Signals();
target2.signals = () => targetSignals2;

target3.classList.add('i-amphtml-element');
targetSignals3 = new Signals();
target3.signals = () => targetSignals3;

target.setAttribute('data-vars-id', '123');
target2.setAttribute('data-vars-id', '456');
target3.setAttribute('data-vars-id', '789');

saveCallback2 = env.sandbox.match((arg) => {
if (typeof arg == 'function') {
saveCallback2.callback = arg;
return true;
}
return false;
});
saveCallback3 = env.sandbox.match((arg) => {
if (typeof arg == 'function') {
saveCallback3.callback = arg;
return true;
}
return false;
});
});

afterEach(async () => {
if (!error) {
[unlisten, unlisten2, unlisten3].forEach((value) => {
[unlisten, unlisten2].forEach((value) => {
if (value) {
expect(value).to.not.be.called;
}
});
expect(res).to.be.a('function');
await res();
[unlisten, unlisten2, unlisten3].forEach((value) => {
[unlisten, unlisten2].forEach((value) => {
if (value) {
expect(value).to.be.calledOnce;
}
Expand All @@ -2032,11 +2008,6 @@ describes.realWin('Events', {amp: 1}, (env) => {
.withExactArgs('ini-load', target2)
.returns(readyPromise)
.once();
iniLoadTrackerMock
.expects('getElementSignal')
.withExactArgs('ini-load', target3)
.returns(readyPromise)
.once();
visibilityManagerMock
.expects('listenElement')
.withExactArgs(
Expand All @@ -2059,54 +2030,31 @@ describes.realWin('Events', {amp: 1}, (env) => {
)
.returns(unlisten2)
.once();
visibilityManagerMock
.expects('listenElement')
.withExactArgs(
target3,
config.visibilitySpec,
readyPromise,
/* createReportReadyPromiseFunc */ null,
saveCallback3
)
.returns(unlisten3)
.once();
// Dispose function
res = tracker.add(analyticsElement, 'visible', config, eventResolver);
const unlistenReady = getAmpElementSpy.returnValues[0];
const unlistenReady2 = getAmpElementSpy.returnValues[1];
const unlistenReady3 = getAmpElementSpy.returnValues[2];
// #getAmpElement Promise
await unlistenReady;
await unlistenReady2;
await unlistenReady3;
// #assertMeasurable_ Promise
await macroTask();
await macroTask();
saveCallback.callback({totalVisibleTime: 10});
saveCallback2.callback({totalVisibleTime: 15});
saveCallback3.callback({totalVisibleTime: 20});

// Testing that visibilty manager mock sends state to onEvent_
expect(eventsSpy.getCall(0).args[0]).to.equal('visible');
expect(eventsSpy.getCall(0).args[1]).to.equal(eventResolver);
expect(eventsSpy.getCall(0).args[2]).to.equal(target);
expect(eventsSpy.getCall(0).args[3]).to.deep.equal({
totalVisibleTime: 10,
id: '123',
});
expect(eventsSpy.getCall(1).args[0]).to.equal('visible');
expect(eventsSpy.getCall(1).args[1]).to.equal(eventResolver);
expect(eventsSpy.getCall(1).args[2]).to.equal(target2);
expect(eventsSpy.getCall(1).args[3]).to.deep.equal({
totalVisibleTime: 15,
id: '456',
});
expect(eventsSpy.getCall(2).args[0]).to.equal('visible');
expect(eventsSpy.getCall(2).args[1]).to.equal(eventResolver);
expect(eventsSpy.getCall(2).args[2]).to.equal(target3);
expect(eventsSpy.getCall(2).args[3]).to.deep.equal({
totalVisibleTime: 20,
id: '789',
});
});

Expand Down

0 comments on commit 9d15c94

Please sign in to comment.