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

LG-13170: add 5 digit zipcode to analytics #10696

Merged
merged 7 commits into from
May 29, 2024

Conversation

amirbey
Copy link
Contributor

@amirbey amirbey commented May 24, 2024

🎫 Ticket

LG-13170

🛠 Summary of changes

  • add zip_code as a property to the IdV: doc auth image upload vendor submitted analytics event
  • update fake analytics to allow zipcode to be logged

📜 Testing Plan

  • Complet doc auth
  • In the events.log, verify the zip_code is present in the IdV: doc auth image upload vendor submitted event

@amirbey amirbey changed the title LG-13170 add 5 digit zipcode to analytics LG-13170: add 5 digit zipcode to analytics May 24, 2024
@@ -1127,6 +1127,7 @@ def idv_doc_auth_submitted_image_upload_form(
# @param [Hash] portrait_match_results
# @param [Hash] image_metrics
# @param [Boolean] address_line2_present
# @param [String] zip_code
Copy link
Contributor

@zachmargolis zachmargolis May 24, 2024

Choose a reason for hiding this comment

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

looks like we have both zip_code and zipcode in our codebase 😭 but zipcode is more common:

identity-idp:main> git grep zip_code -- 'app' | wc -l
      28
identity-idp:main> git grep zipcode -- 'app' | wc -l 
     119
Suggested change
# @param [String] zip_code
# @param [String] zipcode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

snakecasing words seems to be consistent in our analytics .... are you suggesting making the argument zipcode and then in the body:

zip_code: zipcode,

Copy link
Contributor

Choose a reason for hiding this comment

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

no I was suggesting zipcode (no underscore) everywhere in the PR

@amirbey amirbey requested a review from zachmargolis May 24, 2024 19:30
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

My vote is for zipcode instead of zip_code but LGTM either way

@amirbey amirbey force-pushed the timnit/LG-13170-log-5digit-zipcode branch from 3793183 to 9c52553 Compare May 29, 2024 13:35
@amirbey amirbey merged commit 84fdb3d into main May 29, 2024
2 checks passed
@amirbey amirbey deleted the timnit/LG-13170-log-5digit-zipcode branch May 29, 2024 14:34
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