Skip to content

LG-5935 - Document analytics 16#6349

Merged
gsa-manish merged 1 commit intomainfrom
LG-5935-document-analytics-16
May 16, 2022
Merged

LG-5935 - Document analytics 16#6349
gsa-manish merged 1 commit intomainfrom
LG-5935-document-analytics-16

Conversation

@gsa-manish
Copy link
Contributor

changelog: Analytics, Events, update events #16

Comment on lines 17 to 19
Copy link
Contributor

Choose a reason for hiding this comment

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

this should use the new method, not the string, right?
analytics.idv_gpo_address_visited(letter_already_sent: @presenter.letter_already_sent?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes - updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

We try to call this flow "GPO" now, the string was left to have consistency in event names

Suggested change
# USPS address letter requested
# GPO address letter requested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok. They've been updated.

Copy link

Choose a reason for hiding this comment

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

Was wondering about GPO myself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# USPS address submitted
# GPO address submitted

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# USPS address visited
# GPO address visited

@gsa-manish gsa-manish force-pushed the LG-5935-document-analytics-16 branch from 4f48240 to 0a9dd34 Compare May 13, 2022 18:05
Comment on lines +631 to +636
Copy link
Contributor

Choose a reason for hiding this comment

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

This event had a # Previously: note...

  IDV_GPO_VERIFICATION_SUBMITTED = 'IdV: GPO verification submitted' # Previously: "Account verification submitted"

can we annotate that as well?

Suggested change
# @param [Boolean] success
# @param [Hash] errors
# @param [Hash] pii_like_keypaths
# GPO verification submitted
def idv_gpo_verification_submitted(
# @identity.idp.previous_event_name Account verification submitted
# @param [Boolean] success
# @param [Hash] errors
# @param [Hash] pii_like_keypaths
# GPO verification submitted
def idv_gpo_verification_submitted(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to the annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please take a look at my suggested comment, we have a specific YARD tag for this, @identity.previous_event_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see it now. Suggested comment added.

@gsa-manish gsa-manish force-pushed the LG-5935-document-analytics-16 branch 2 times, most recently from 48e123a to a0f65d2 Compare May 13, 2022 19:26
changelog: Analytics, Events, update events #16
@gsa-manish gsa-manish force-pushed the LG-5935-document-analytics-16 branch from a0f65d2 to 2a99ccd Compare May 13, 2022 19:26
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.

LGTM

@gsa-manish gsa-manish merged commit 7bc12ff into main May 16, 2022
@gsa-manish gsa-manish deleted the LG-5935-document-analytics-16 branch May 16, 2022 17:16
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