LG-9848: Add analytics to onbeforeunload by matthinz · Pull Request #8512 · 18F/identity-idp

matthinz · 2023-05-30T23:52:43Z

🎫 Ticket

🛠 Summary of changes

Team Ada is considering removing some onbeforeunload prompts (the ones that say "If you navigate away from this page, your data may not be saved" or something to that effect) from the IdV flow on the grounds that they are annoying. But before we do that, we want actual data about how they are used and if they are doing their job.

How often do users encounter these prompts?
Do the prompts keep users on the page?

To that end, this PR refactors onbeforeunload code out into its own package and adds some analytics calls:

User prompted before navigation fires when the onbeforeunload event handler is called
User prompted before navigation and still on page fires at 5s, 15s, and 30s after to indicate the user stayed on the page

app/javascript/packages/document-capture-polling/index.ts

app/javascript/packages/prompt-on-navigate/index.ts

Set an onbeforeunload handler, do some analytics, manage cleanup.

Avoid strange-looking `NodeJS.Timeout` type

Don't leave onbeforeunload handlers hanging around

Sinon's useFakeTimers covers global.setTimeout (which is callable as plain `setTimeout(...)`, but does not cover `window.setTimeout`.

matthinz · 2023-06-02T16:35:41Z

app/javascript/packages/test-helpers/use-sandbox.ts

    sandbox.clock.restore();
  }

+  // useFakeTimers overrides global.setTimeout, etc. (callable as setTimeout()), but does not


FYI @aduth this is fun fallout from the setTimeout vs window.setTimeout issue. Sinon replaces global.setTimeout but NOT window.setTimeout. There is a config option to set the global it uses, but I think that would break any existing "bare" references to setTimeout.

Gotcha, this makes sense then 👍

Alternatively, I wonder what might happen if we changed the "window" setTimeout to point to the Node global in the test setup?

window.setTimeout = setTimeout;

zachmargolis · 2023-06-02T16:39:58Z

app/javascript/packages/document-capture-polling/index.ts

+      const cleanUp = this.cleanUpPromptOnNavigate ?? (() => {});
+      this.cleanUpPromptOnNavigate = undefined;
+      cleanUp();


personally I love the null-safe navigator for stuff like this but this is just style

Suggested change

const cleanUp = this.cleanUpPromptOnNavigate ?? (() => {});

this.cleanUpPromptOnNavigate = undefined;

cleanUp();

this.cleanUpPromptOnNavigate = undefined;

this.cleanUpPromptOnNavigate?.();

oh wait I just realized that we can't do this because we're clearing 🤦

aduth

LGTM 👍

aduth · 2023-06-02T16:41:11Z

app/javascript/packages/document-capture-polling/index.ts

+    const isAlreadyBound = !!this.cleanUpPromptOnNavigate;
+
+    if (shouldPrompt && !isAlreadyBound) {
+      this.cleanUpPromptOnNavigate = promptOnNavigate();
+    } else if (!shouldPrompt && isAlreadyBound) {
+      const cleanUp = this.cleanUpPromptOnNavigate ?? (() => {});
+      this.cleanUpPromptOnNavigate = undefined;
+      cleanUp();


Kinda unfortunate that TypeScript isn't able to follow the type narrowing through the variable assignment. It could if we avoided the variable, but maybe that sacrifices readability too much?

Suggested change

const isAlreadyBound = !!this.cleanUpPromptOnNavigate;

if (shouldPrompt && !isAlreadyBound) {

this.cleanUpPromptOnNavigate = promptOnNavigate();

} else if (!shouldPrompt && isAlreadyBound) {

const cleanUp = this.cleanUpPromptOnNavigate ?? (() => {});

this.cleanUpPromptOnNavigate = undefined;

cleanUp();

if (shouldPrompt && !this.cleanUpPromptOnNavigate) {

this.cleanUpPromptOnNavigate = promptOnNavigate();

} else if (!shouldPrompt && this.cleanUpPromptOnNavigate) {

this.cleanUpPromptOnNavigate();

this.cleanUpPromptOnNavigate = undefined;

I prefer the readability of isAlreadyBound but yeah I wish TS could deduce that cleanUpPromptOnNavigate is there when isAlreadyBound is true.

aduth · 2023-06-02T16:46:16Z

app/javascript/packages/prompt-on-navigate/README.md

+
+## Analytics
+
+By default, `promptOnNavigate` will call `trackEvent` to log a `User prompted before navigation` event when the onbeforeunload handler is called. It will then log `User prompted before navigation and still on page` events at 5, 15, and 30 seconds after the onbeforeunload handler is called (the `seconds` property on the event will contain the number of seconds since the initial prompt).


Just to confirm my understanding of the behavior: When I tested this, as long as the confirmation prompt was visible, there was no logging happening. It was only after I dismissed the confirmation prompt that it started logging the next events. Is that expected?

Yeah, I don't think there's a good way for us to know when the prompt is dismissed. That was part of the motivation for doing the follow-up logging at intervals.

Well it was more that nothing was logged while the prompt was visible. So as far as how it's described here:

It will then log User prompted before navigation and still on page events at 5, 15, and 30 seconds after the onbeforeunload handler is called

In reality it was more like:

It will then log User prompted before navigation and still on page events at 5, 15, and 30 seconds after the onbeforeunload handler dialog is dismissed

matthinz added the status - work in progress label May 30, 2023

zachmargolis approved these changes May 31, 2023

View reviewed changes

app/javascript/packages/document-capture-polling/index.ts Outdated Show resolved Hide resolved

app/javascript/packages/prompt-on-navigate/index.ts Outdated Show resolved Hide resolved

aduth reviewed May 31, 2023

View reviewed changes

app/javascript/packages/prompt-on-navigate/index.ts Outdated Show resolved Hide resolved

matthinz force-pushed the matthinz/9848-onbeforeunload-logging branch from 7eef1d2 to 9aad622 Compare May 31, 2023 19:14

matthinz added 14 commits June 1, 2023 13:28

Analytics for doc capture polling onbeforeunload

d287366

Refactor promptOnNavigate logic out into a package

3df9ca0

Set an onbeforeunload handler, do some analytics, manage cleanup.

Ping 5, 15, and 30s after onbeforeunload

dfd4707

remove stray extra newline

cf31d97

Remove peerDependency from prompt-on-navigate package

14be5bf

Remove stray file

faf19f8

Don't use a separate var to track whether promptOnNavigate was bound

676befe

Use window.setTimeout / window.clearTimeout

a5c7a61

Avoid strange-looking `NodeJS.Timeout` type

Lint

da8465c

Update doc capture polling tests to clean up

a42a74c

Don't leave onbeforeunload handlers hanging around

changelog: Internal, Analytics, Add analytics to onbeforeunload handlers

967341a

Put event names in variables

c0593ed

Add README for prompt-on-navigate package

78b373d

Update events + add to FrontendLogController

0b84e23

matthinz force-pushed the matthinz/9848-onbeforeunload-logging branch from e47f560 to 0b84e23 Compare June 1, 2023 22:31

matthinz added 2 commits June 2, 2023 09:03

Update tests to reference event name variables

8b59656

Fake window.setTimeout etc. with useFakeTimers

c07c772

Sinon's useFakeTimers covers global.setTimeout (which is callable as plain `setTimeout(...)`, but does not cover `window.setTimeout`.

matthinz marked this pull request as ready for review June 2, 2023 16:29

matthinz commented Jun 2, 2023

View reviewed changes

matthinz removed the status - work in progress label Jun 2, 2023

zachmargolis reviewed Jun 2, 2023

View reviewed changes

aduth approved these changes Jun 2, 2023

View reviewed changes

matthinz merged commit bbb17ac into main Jun 2, 2023

matthinz deleted the matthinz/9848-onbeforeunload-logging branch June 2, 2023 18:54

solipet mentioned this pull request Jun 6, 2023

Deploy RC 286 to Prod #8539

Merged

matthinz mentioned this pull request Jun 7, 2023

Add location to events logged by promptOnNavigate #8550

Merged

aduth mentioned this pull request Jan 24, 2024

LG-12047 Remove onbeforeunload logging code #9955

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

LG-9848: Add analytics to onbeforeunload#8512

LG-9848: Add analytics to onbeforeunload#8512
matthinz merged 16 commits intomainfrom
matthinz/9848-onbeforeunload-logging

matthinz commented May 30, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matthinz Jun 2, 2023

Uh oh!

aduth Jun 2, 2023

Uh oh!

zachmargolis Jun 2, 2023

Uh oh!

zachmargolis Jun 2, 2023

Uh oh!

aduth left a comment

Uh oh!

aduth Jun 2, 2023

Uh oh!

matthinz Jun 2, 2023

Uh oh!

aduth Jun 2, 2023

Uh oh!

matthinz Jun 2, 2023

Uh oh!

aduth Jun 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		## Analytics

		By default, `promptOnNavigate` will call `trackEvent` to log a `User prompted before navigation` event when the onbeforeunload handler is called. It will then log `User prompted before navigation and still on page` events at 5, 15, and 30 seconds after the onbeforeunload handler is called (the `seconds` property on the event will contain the number of seconds since the initial prompt).

Comments

Conversation

matthinz commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matthinz commented May 30, 2023 •

edited

Loading