-
Notifications
You must be signed in to change notification settings - Fork 112
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
Conversation
@@ -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 |
There was a problem hiding this comment.
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
# @param [String] zip_code | |
# @param [String] zipcode |
There was a problem hiding this comment.
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,
There was a problem hiding this comment.
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
There was a problem hiding this 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
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
3793183
to
9c52553
Compare
🎫 Ticket
LG-13170
🛠 Summary of changes
📜 Testing Plan