Send form-submit success/error analytics events #5868

Merged
merged 6 commits into from Nov 7, 2016

Projects

None yet

6 participants

@mkhatib
Member
mkhatib commented Oct 28, 2016

Fix #5674

@mkhatib
Member
mkhatib commented Oct 28, 2016

/cc @avimehta @rudygalfi let me know if this looks good - event naming and if example should be updated - I don't know 100% if I am using the analytics config correct.

extensions/amp-form/0.1/amp-form.js
+ * @private
+ */
+ analyticsEvent_(eventType, opt_vars) {
+ triggerAnalyticsEvent(this.win_, eventType, opt_vars);
@jridgewell
jridgewell Oct 28, 2016 Member

Isn't it this.win?

@mkhatib
mkhatib Oct 28, 2016 Member

No, amp-form is not a base-element.

@avimehta

mostly LGTM. Feel free to merge if others approve before me and you have addressed non-nit comments.

examples/forms.amp.html
+ <script type="application/json">
+ {
+ "requests": {
+ "test-ping": "https://my-analytics.com/ping?title=${title}&acct=${account}",
@avimehta
avimehta Oct 28, 2016 Collaborator

nit: This URL will show an errror in network tab. Slight preference for a real URL like: https://www.ampproject.org/static/img/logo-blue.svg.

examples/forms.amp.html
+ "test-ping": "https://my-analytics.com/ping?title=${title}&acct=${account}",
+ "event": "https://my-analytics.com/ping?eid=${eventId}&elab=${eventLabel}"
+ },
+ "vars": {
@avimehta
avimehta Oct 28, 2016 edited Collaborator

nit: Nothing is wrong with this exampe but this is not necessary to showcase analytics abilities in forms. Maybe remove? Also remove ${account} from the request?

examples/forms.amp.html
+ "account": "12345"
+ },
+ "triggers": {
+ "page-view": {
@avimehta
avimehta Oct 28, 2016 edited Collaborator

nit: this can be removed as this isn't specific to forms.

src/analytics.js
+
+
+/**
+ * Helper method to trigger analytics event if analytics is available.
@avimehta
avimehta Oct 28, 2016 Collaborator

if amp-analytics is available?

extensions/amp-form/0.1/amp-form.js
@@ -218,10 +219,12 @@ export class AmpForm {
requireAmpResponseSourceOrigin: true,
}).then(response => {
this.actions_.trigger(this.form_, 'submit-success', null);
+ this.analyticsEvent_('amp-form-submit-success');
@avimehta
avimehta Oct 28, 2016 Collaborator

These need to be documented. Please see how access and slides document this and use the same convention?

@mkhatib
mkhatib Oct 28, 2016 Member

Yeah the plan was to document these once this change is in prod otherwise we'll get lots of issues opened that this isn't working if we update docs right away. I'll send a follow up PR later.

@camelburrito
camelburrito Nov 2, 2016 Collaborator

file a bug and add a todo here.

@@ -88,7 +88,7 @@ class AnalyticsEvent {
/**
* @param {!AnalyticsEventType|string} type The type of event.
- * @param {!Object<string, string>=} opt_vars A map of vars and their values.
@avimehta
avimehta Oct 28, 2016 Collaborator

Confused. How does ! and = work together? How is it different from ? and = being used together?

@mkhatib
mkhatib Oct 28, 2016 Member

Afaik, the = allows undefined and the ? allows null as values.

@jridgewell
jridgewell Oct 28, 2016 Member

= means the parameter can be omitted:

fn({});
fn();
@avimehta
avimehta Oct 28, 2016 Collaborator

so we don't really need ? here then?

@mkhatib
mkhatib Nov 1, 2016 Member

Using ! instead.

src/analytics.js
@@ -41,3 +41,21 @@ export function analyticsForOrNull(window) {
>} */ (getElementServiceIfAvailable(
window, 'amp-analytics-instrumentation', 'amp-analytics')));
};
+
+
@avimehta
avimehta Oct 28, 2016 Collaborator

nit: extra empty line?

@mkhatib mkhatib assigned camelburrito and unassigned cramforce Nov 1, 2016
src/analytics.js
+ * @returns {!Promise<boolean>} Whether the event was sent or not.
+ */
+export function triggerAnalyticsEvent(window, eventType, opt_vars) {
+ return analyticsForOrNull(window).then(analytics => {
@camelburrito
camelburrito Nov 1, 2016 Collaborator

why do we have to return anything here, seems like it is not being used anywhere?

@mkhatib
mkhatib Nov 2, 2016 Member

Dropped.

src/analytics.js
+export function triggerAnalyticsEvent(window, eventType, opt_vars) {
+ return analyticsForOrNull(window).then(analytics => {
+ if (!analytics) {
+ return false;
@camelburrito
camelburrito Nov 1, 2016 Collaborator

i guess we should also be doing some kind of logging here, so that we know if we are triggering before analytics is available.

@mkhatib
mkhatib Nov 2, 2016 Member

Dropped.

src/analytics.js
+ return false;
+ }
+ analytics.triggerEvent(eventType, opt_vars);
+ return true;
@camelburrito
camelburrito Nov 1, 2016 Collaborator

i don't think this is needed either.

@mkhatib
mkhatib Nov 2, 2016 Member

Dropped.

@@ -16,7 +16,7 @@
import {Animation} from '../../../src/animation';
import {BaseSlides} from './base-slides';
-import {analyticsForOrNull} from '../../../src/analytics';
+import {triggerAnalyticsEvent} from '../../../src/analytics';
@camelburrito
camelburrito Nov 1, 2016 Collaborator

you must also make amp-access use this.

@camelburrito

Please fix the comments above!

mkhatib added some commits Oct 28, 2016
@mkhatib mkhatib Send form-submit success/error analytics events.
0819e6d
@mkhatib mkhatib fix types
1b72b26
@mkhatib mkhatib address comments
cf230fd
@mkhatib mkhatib fix comments
cbf7cec
@mkhatib mkhatib update access
acc106f
@mkhatib
Member
mkhatib commented Nov 2, 2016

@camelburrito fixed comments PTAL 👀

extensions/amp-form/0.1/amp-form.js
@@ -218,10 +219,12 @@ export class AmpForm {
requireAmpResponseSourceOrigin: true,
}).then(response => {
this.actions_.trigger(this.form_, 'submit-success', null);
+ this.analyticsEvent_('amp-form-submit-success');
@camelburrito
camelburrito Nov 2, 2016 Collaborator

file a bug and add a todo here.

+ * @param {string} eventType
+ * @param {!Object<string, string>=} opt_vars A map of vars and their values.
+ */
+export function triggerAnalyticsEvent(window, eventType, opt_vars) {
@camelburrito
camelburrito Nov 2, 2016 Collaborator

tests for this?

@mkhatib mkhatib add test and todo
198efc2
@mkhatib
Member
mkhatib commented Nov 5, 2016

@camelburrito 👀 PTAL

@mkhatib mkhatib removed the NEEDS REVIEW label Nov 7, 2016
@mkhatib mkhatib merged commit 926b444 into ampproject:master Nov 7, 2016

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mkhatib mkhatib deleted the mkhatib:submit-analytics branch Nov 7, 2016
@Lith Lith added a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
@mkhatib @Lith mkhatib + Lith Send form-submit success/error analytics events (#5868) 0c32769
@Lith Lith added a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
@mkhatib @Lith mkhatib + Lith Send form-submit success/error analytics events (#5868) 44f7604
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment