Merged
Conversation
gsa-manish
changed the title
gsa-manish
changed the title
gsa-manish
force-pushed
the
LG-5929-document-analytics-11
branch
2 times, most recently
from
83cfee6 to
53f3ee7
Compare
zachmargolis
reviewed
May 3, 2022
app/services/analytics_events.rb
Outdated
Contributor
There was a problem hiding this comment.
ditto for this blank line
Suggested change
Contributor
There was a problem hiding this comment.
Hmmmm this is an interesting case, it makes me rethink our position about constants... we should file a ticket to see if we can clean this up the new world of documented methods
Contributor
Author
There was a problem hiding this comment.
Yes, this seems a little off. I'll add a ticket in the backlog to revisit constants.
zachmargolis
reviewed
May 3, 2022
gsa-manish
force-pushed
the
LG-5929-document-analytics-11
branch
4 times, most recently
from
5849a60 to
cb71678
Compare
changelog: Analytics, Doc auth, doc auth events for vendor, form, pii, final, and warning
gsa-manish
force-pushed
the
LG-5929-document-analytics-11
branch
4 times, most recently
from
d97b55d to
55c01fd
Compare
changelog: Analytics, Doc auth, doc auth events for vendor, form, pii, final, and warning
gsa-manish
force-pushed
the
LG-5929-document-analytics-11
branch
from
55c01fd to
9b5e8f0
Compare
* Remove @identity.idp.event_name - As of #6294, having it will cause build breakage since it's an unknown tag * Remove blank lines * Add clearer comments for each event
changelog: Analytics, Doc auth, doc auth events for vendor, form, pii, final, and warning
gsa-manish
force-pushed
the
LG-5929-document-analytics-11
branch
2 times, most recently
from
1b6006f to
1078c12
Compare
zachmargolis
reviewed
May 4, 2022
Comment on lines
33
to
36
Contributor
There was a problem hiding this comment.
this should probably be @flow.analytics.idv_doc_auth_warning_visisted(
Contributor
Author
There was a problem hiding this comment.
Nice catch, this has been updated.
app/services/analytics_events.rb
Outdated
Contributor
There was a problem hiding this comment.
codeclimate is hinting that this is untested, which was my queue to look for the usage above of the string outside of specs
gsa-manish
force-pushed
the
LG-5929-document-analytics-11
branch
from
1078c12 to
1132c8b
Compare
…ty-idp into LG-5929-document-analytics-11 changelog: Analytics, Doc auth, doc auth events for vendor, form, pii, final, and warning
gsa-manish
force-pushed
the
LG-5929-document-analytics-11
branch
from
1132c8b to
6bf3585
Compare
zachmargolis
approved these changes
May 5, 2022
| **client_response.to_h.merge( | ||
| client_image_metrics: image_metadata, | ||
| async: false, | ||
| flow_path: params[:flow_path], |
Contributor
There was a problem hiding this comment.
why did we remove flow_path here?
Contributor
There was a problem hiding this comment.
I believe we need to keep it/bring it back otherwise we lose data
| # @param [Integer] remaining_attempts | ||
| # @param [String] user_id | ||
| # @param [String] flow_path | ||
| # The document capture image uploaded was locally validated during the IDV process |
Contributor
There was a problem hiding this comment.
supernit... extra space
Suggested change
| # The document capture image uploaded was locally validated during the IDV process | |
| # The document capture image uploaded was locally validated during the IDV process |
| def idv_doc_auth_submitted_image_upload_form( | ||
| success:, | ||
| errors:, | ||
| remaining_attempts:, flow_path:, attempts: nil, |
Contributor
There was a problem hiding this comment.
let's break these on to separate lines for consistency
Suggested change
| remaining_attempts:, flow_path:, attempts: nil, | |
| remaining_attempts:, | |
| flow_path:, | |
| attempts: nil, |
peggles2
pushed a commit
that referenced
this pull request
May 5, 2022
* LG-5929-document-analytics-11 * Patch: analytics events 11 (#6303) * Remove @identity.idp.event_name - As of #6294, having it will cause build breakage since it's an unknown tag * Remove blank lines * Add clearer comments for each event Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.