Skip to content

Commit

Permalink
amp-form: enable detached analytics & fix error flow (#32578)
Browse files Browse the repository at this point in the history 
* amp-form: use cached doc for analytics events instead of inferring from <form>

* fix form handling order

* add unit test

* remove humor

* remove all the noisy console.error calls from test output

* ampdoc_ --> form_

* remove outdated comment
  • Loading branch information
@samouri
samouri committed Feb 12, 2021
1 parent a365eea commit deba5a4
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 12 deletions.
8 changes: 6 additions & 2 deletions extensions/amp-form/0.1/amp-form.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,11 @@ export class AmpForm {
}
formDataForAnalytics['formId'] = this.form_.id;

this.analyticsEvent_(eventType, formDataForAnalytics);
try {
this.analyticsEvent_(eventType, formDataForAnalytics);
} catch (err) {
dev().error(TAG, 'Sending analytics failed:', err);
}
}

/**
Expand Down Expand Up @@ -1042,8 +1046,8 @@ export class AmpForm {
promise = Promise.resolve(null);
}
return promise.then((responseJson) => {
this.triggerFormSubmitInAnalytics_('amp-form-submit-error');
this.handleSubmitFailure_(e, responseJson, incomingTrust);
this.triggerFormSubmitInAnalytics_('amp-form-submit-error');
this.maybeHandleRedirect_(e.response);
});
}
Expand Down
74 changes: 64 additions & 10 deletions extensions/amp-form/0.1/test/test-amp-form.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {DIRTINESS_INDICATOR_CLASS} from '../form-dirtiness';
import {Services} from '../../../../src/services';
import {cidServiceForDocForTesting} from '../../../../src/service/cid-impl';
import {createCustomEvent} from '../../../../src/event-helper';
import {createElementWithAttributes} from '../../../../src/dom';
import {
createFormDataWrapper,
isFormDataWrapper,
Expand Down Expand Up @@ -73,7 +74,8 @@ describes.repeated(
document = env.ampdoc.getRootNode();
timer = Services.timerFor(env.win);
const ownerDoc = document.ownerDocument || document;
createElement = ownerDoc.createElement.bind(ownerDoc);
createElement = (tagName, attrs) =>
createElementWithAttributes(ownerDoc, tagName, attrs);
createTextNode = ownerDoc.createTextNode.bind(ownerDoc);

// Force sync mutateElement to make testing easier.
Expand Down Expand Up @@ -101,6 +103,7 @@ describes.repeated(
env.sandbox
.stub(ampForm.ssrTemplateHelper_, 'isEnabled')
.returns(false);
env.sandbox.stub(ampForm, 'analyticsEvent_');
return Promise.resolve(ampForm);
}

Expand Down Expand Up @@ -245,7 +248,6 @@ describes.repeated(
ampForm.method_ = 'GET';
env.sandbox.stub(form, 'submit');
env.sandbox.stub(form, 'checkValidity').returns(true);
env.sandbox.stub(ampForm, 'analyticsEvent_');
ampForm.ssrTemplateHelper_.isEnabled.restore();
env.sandbox
.stub(ampForm.ssrTemplateHelper_, 'isEnabled')
Expand Down Expand Up @@ -1118,6 +1120,66 @@ describes.repeated(
});
});

it('should render responses even if analytics throws', () => {
expectAsyncConsoleError(/Form submission failed/);
expectAsyncConsoleError(/Sending analytics failed/, 2);

return getAmpForm(getForm()).then((ampForm) => {
const form = ampForm.form_;

const errorTemplate = createElement('template', {
id: 'errorTemplate',
type: 'amp-mustache',
});
errorTemplate.content.appendChild(
createTextNode('Sorry, {{name}}')
);
form.appendChild(errorTemplate);

const messageContainer = createElement('div', {
id: 'message',
'submit-error': '',
template: 'errorTemplate',
});
form.appendChild(messageContainer);
env.sandbox.stub(ampForm.xhr_, 'fetch').rejects({
response: {
json: () => {
return Promise.resolve({'name': 'John Doe'});
},
},
});
ampForm.analyticsEvent_.throws('Test.');
const renderedTemplate = createElement('div');
renderedTemplate.innerText = 'Sorry, John Doe';
env.sandbox
.stub(ampForm.templates_, 'findAndRenderTemplate')
.resolves(renderedTemplate);
const event = {
stopImmediatePropagation: env.sandbox.spy(),
target: form,
preventDefault: env.sandbox.spy(),
};

const submitEventPromise = ampForm.handleSubmitEvent_(event);

return whenCalled(ampForm.templates_.findAndRenderTemplate)
.then(() => {
return ampForm.renderTemplatePromiseForTesting();
})
.then(() => {
expect(ampForm.templates_.findAndRenderTemplate).to.be.called;
expect(
ampForm.templates_.findAndRenderTemplate
).calledWith(messageContainer, {'name': 'John Doe'});
expect(mutateElementStub).to.have.been.calledOnce;
expect(messageContainer.firstChild).to.equal(renderedTemplate);

return submitEventPromise;
});
});
});

it('should replace previously rendered responses', () => {
return getAmpForm(getForm(/*button1*/ true)).then((ampForm) => {
const form = ampForm.form_;
Expand Down Expand Up @@ -1378,7 +1440,6 @@ describes.repeated(
env.sandbox.stub(ampForm.xhr_, 'fetch').resolves({
json: () => Promise.resolve({}),
});
env.sandbox.stub(ampForm, 'analyticsEvent_');

const event = {
stopImmediatePropagation: env.sandbox.spy(),
Expand Down Expand Up @@ -1437,7 +1498,6 @@ describes.repeated(
});
env.sandbox.spy(ampForm.urlReplacement_, 'expandInputValueAsync');
env.sandbox.stub(ampForm.urlReplacement_, 'expandInputValueSync');
env.sandbox.stub(ampForm, 'analyticsEvent_');

// Setup some Promises
const submitPromise = ampForm.submit_(ActionTrust.HIGH);
Expand Down Expand Up @@ -1578,7 +1638,6 @@ describes.repeated(
fetchResolver = resolve;
})
);
env.sandbox.stub(ampForm, 'analyticsEvent_');
env.sandbox.stub(ampForm.actions_, 'trigger');
const form = ampForm.form_;
const event = {
Expand Down Expand Up @@ -1631,7 +1690,6 @@ describes.repeated(
return getAmpForm(getForm(/*button1*/ true, /*button2*/ true)).then(
(ampForm) => {
let fetchRejecter;
env.sandbox.stub(ampForm, 'analyticsEvent_');
env.sandbox.stub(ampForm.xhr_, 'fetch').returns(
new Promise((unusedResolve, reject) => {
fetchRejecter = reject;
Expand Down Expand Up @@ -2758,7 +2816,6 @@ describes.repeated(
env.sandbox.stub(form, 'submit');
env.sandbox.stub(form, 'checkValidity').returns(true);
env.sandbox.stub(ampForm.xhr_, 'fetch').resolves();
env.sandbox.stub(ampForm, 'analyticsEvent_');

env.sandbox.stub(ampForm, 'handleXhrSubmitSuccess_').resolves();
const submitActionPromise = ampForm.handleSubmitAction_(
Expand Down Expand Up @@ -2802,7 +2859,6 @@ describes.repeated(
fetchResolver = resolve;
})
);
env.sandbox.stub(ampForm, 'analyticsEvent_');
const event = {
stopImmediatePropagation: env.sandbox.spy(),
target: form,
Expand Down Expand Up @@ -2856,7 +2912,6 @@ describes.repeated(
fetchRejecter = reject;
})
);
env.sandbox.stub(ampForm, 'analyticsEvent_');
const event = {
stopImmediatePropagation: env.sandbox.spy(),
target: form,
Expand Down Expand Up @@ -2914,7 +2969,6 @@ describes.repeated(
env.sandbox.stub(ampForm.xhr_, 'fetch').resolves();
env.sandbox.stub(ampForm.urlReplacement_, 'expandInputValueAsync');
env.sandbox.spy(ampForm.urlReplacement_, 'expandInputValueSync');
env.sandbox.stub(ampForm, 'analyticsEvent_');
ampForm.handleSubmitAction_(/* invocation */ {});

const expectedFormData = {
Expand Down

0 comments on commit deba5a4

Please sign in to comment.