Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: DAH-3043 Analytics for Application Flow #2448

Merged
merged 4 commits into from
Dec 19, 2024

Conversation

chadbrokaw
Copy link
Collaborator

@chadbrokaw chadbrokaw commented Dec 9, 2024

Description

This PR adds in logging for the application flow including application start, application abandon, and application complete.

Jira ticket

DAH-3043

Checklist before requesting review

Version Control

  • branch name begins with angular if it contains updates to Angular code
  • branch name contains the Jira ticket number
  • PR name follows type: TICKET-NUMBER Description format, e.g. feat: DAH-123 New Feature

Code quality

  • the set of changes is small
  • all automated code checks pass (linting, tests, coverage, etc.)
  • code irrelevant to the ticket is not modified e.g. changing indentation due to automated formatting
  • if the code changes the UI, it matches the UI design exactly
  • if the changes include human translations, follow the human translations process

Review instructions

  • instructions specify which environment(s) it applies to
  • instructions work for PA testers
  • instructions have already been performed at least once

Request review

  • PR has needs review label
  • Use Housing Eng group to automatically assign reviewers, and/or assign specific engineers
  • If time sensitive, notify engineers in Slack

@chadbrokaw chadbrokaw force-pushed the DAH-3043_measure_app_submission_time branch from 975d2ab to e868bee Compare December 9, 2024 18:17
@chadbrokaw chadbrokaw temporarily deployed to dahlia-webapp-pr-2448 December 9, 2024 18:17 Inactive
@chadbrokaw chadbrokaw temporarily deployed to dahlia-webapp-pr-2448 December 9, 2024 18:19 Inactive
@chadbrokaw chadbrokaw force-pushed the DAH-3043_measure_app_submission_time branch from e868bee to 6ec2a35 Compare December 12, 2024 19:29
@chadbrokaw chadbrokaw temporarily deployed to dahlia-webapp-pr-2448 December 12, 2024 19:29 Inactive
@chadbrokaw chadbrokaw changed the title First pass feat: DAH-3043 Analytics for Application Flow Dec 12, 2024
@chadbrokaw chadbrokaw requested review from a team, tallulahkay and cade-exygy and removed request for a team December 12, 2024 21:21
@chadbrokaw chadbrokaw marked this pull request as ready for review December 12, 2024 21:21
@jimlin-sfgov jimlin-sfgov self-requested a review December 13, 2024 19:12
@chadbrokaw chadbrokaw temporarily deployed to dahlia-webapp-pr-2448 December 17, 2024 22:25 Inactive
@@ -1,5 +1,5 @@
.back-link-container.small-only-text-center ng-if="rememberedShortFormState"
a.back-link ui-sref="{{rememberedShortFormState}}"
a.back-link(ui-sref="{{rememberedShortFormState}}" ng-click="logBackLinkClick()")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: I wonder about how useful this will be.

  • create-account: the button is clicked when the applicant starts creating an account, but changes their mind and goes back to the application
  • sign-in: this now defaults to the React version of the page
  • forgot-password: the button is clicked when the button to reset their password, then click back to the application

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimlin-sfgov Tenley can distinguish on her end where the user is coming from because the data sent to google analytics includes the origin url cc: @chadbrokaw

@@ -136,6 +136,9 @@
onConfirm: ->
# fires only if user clicks 'ok' to leave page
# reloads this stateChangeStart method with skipConfirm true
AnalyticsService.trackApplicationAbandon(ShortFormApplicationService.listing.listingID, null, "Leaving for " + toState.name)
$window.removeEventListener('unload', $rootScope.onUnload)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: When leaving an application, we show either the Angular modal or the browser dialog. We seem to be showing the browser dialog in most cases now, because users are navigating to a non-Angular page. Does this cover the browser dialog scenario?
Screenshot 2024-12-17 at 3 36 19 PM
Screenshot 2024-12-17 at 3 38 09 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! The browser dialog situation is covered by the onUnload handler that I added

Copy link
Contributor

@tallulahkay tallulahkay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree w/ Jim's comments - but also not seeing tags being fired on the pr app. Should I test locally only?

@@ -620,6 +620,7 @@

if ShortFormApplicationService.application.status == 'Submitted'
# send them to their review page if the application is already submitted
AnalyticsService.trackApplicationAbandon(listing.Id, AccountService.loggedInUser?.id || null, "Application already submitted")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Why would there ever not be an id here?

@@ -6,27 +6,38 @@ AnalyticsService = ($state) ->
Service = {}
Service.timer = {}

Service.resetProperties = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this object have any values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not currently, there may be some values we want to nullify in the future though so I left the empty object in there

doubleSubmit = !! $scope.appIsSubmitted($scope.application)
if $window.ACCOUNT_INFORMATION_PAGES_REACT is "true"
currentUrl = window.location.origin
newUrl = "#{currentUrl}/my-applications?"
if previousApp.id
AnalyticsService.trackApplicationAbandon($scope.listing.Id, AccountService.loggedInUser?.id || null, 'Already Submitted')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why would the id ever be null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't ever be null, I added the check just to be 100% sure

newUrl += "alreadySubmittedId=#{previousApp.id}"
if doubleSubmit
newUrl += "&"
if doubleSubmit
# As we rebuilt the My Applications page in React we were not able to figure out a way to trigger the Double Submit Modal.
# We are leaving the code here both to document past behavior and to protect the application in case somehow the modal is triggered
AnalyticsService.trackApplicationAbandon($scope.listing.Id, AccountService.loggedInUser?.id || null, 'Double Submitted')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has not been triggered right? Only there to cover our bases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I honestly added this for me to see if it is ever being triggered, based on my exploration of the code this situation should never occur

Copy link
Contributor

@tallulahkay tallulahkay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I'm comfortable moving forward with it and updating as needed. It seems to me that it covers all the situations you laid out.

My main question is about allowing a null id - in the cases I pointed out, it seems like not having a user id would indicate a bug.

@chadbrokaw chadbrokaw force-pushed the DAH-3043_measure_app_submission_time branch from ed1d817 to 39af3e7 Compare December 19, 2024 21:49
@chadbrokaw chadbrokaw temporarily deployed to dahlia-webapp-pr-2448 December 19, 2024 21:49 Inactive
@chadbrokaw chadbrokaw merged commit ba927d9 into main Dec 19, 2024
18 checks passed
@chadbrokaw chadbrokaw deleted the DAH-3043_measure_app_submission_time branch December 19, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants