Skip to content

Document analytics events part 5#6084

Merged
gsa-manish merged 3 commits intomainfrom
LG-5923-document-analytics-events-5
Mar 18, 2022
Merged

Document analytics events part 5#6084
gsa-manish merged 3 commits intomainfrom
LG-5923-document-analytics-events-5

Conversation

@gsa-manish
Copy link
Contributor

This is an extension of: #6014

The story is: https://cm-jira.usa.gov/browse/LG-5923

Please note that the events ADD_EMAIL, ADD_EMAIL_CONFIRMATION_RESEND , ADD_EMAIL_VISIT were never utilized so they have been removed from the events list.

The only events that are still there are ADD_EMAIL_CONFIRMATION and BANNED_USER_VISITED

@gsa-manish gsa-manish force-pushed the LG-5923-document-analytics-events-5 branch 2 times, most recently from 759c8c1 to 1c417eb Compare March 17, 2022 18:16
Copy link
Contributor

Choose a reason for hiding this comment

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

the goal is to try to document what hash fields. Looks like this comes from EmailConfirmationTokenValidator

Looks like these are some of the attributes: https://github.com/18F/identity-idp/blob/main/app/services/email_confirmation_token_validator.rb#L48-L52

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it - check now?

@gsa-manish gsa-manish force-pushed the LG-5923-document-analytics-events-5 branch 3 times, most recently from 96df557 to c6e1e4c Compare March 17, 2022 19:27
Copy link
Contributor

Choose a reason for hiding this comment

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

now there is no param named result ... can we document any keys we know of for this hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the callsite was missing **, so I added that in f141033

Also when inspecting locally, found a success: key, so I added that in there too

@gsa-manish gsa-manish force-pushed the LG-5923-document-analytics-events-5 branch 2 times, most recently from dff6066 to 9baee93 Compare March 18, 2022 15:06
changelog: Analytics, Yard, Updated banned_user_visited and email_confirmation to use yard
@gsa-manish gsa-manish force-pushed the LG-5923-document-analytics-events-5 branch from 9baee93 to c9eeb09 Compare March 18, 2022 15:23
gsa-manish and others added 2 commits March 18, 2022 14:24
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@gsa-manish gsa-manish merged commit fe1a86e into main Mar 18, 2022
@gsa-manish gsa-manish deleted the LG-5923-document-analytics-events-5 branch March 18, 2022 19:11
end

# @identity.idp.event_name Add Email: Email Confirmation
# @param [Boolean] success
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 have been excluded?

[warn]: @param tag has unknown parameter name: success 
    in file `app/services/analytics_events.rb' near line 91
Suggested change
# @param [Boolean] success

Copy link
Contributor

Choose a reason for hiding this comment

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

#6084 (comment)

I should have added it as a kwarg as well as documenting it

Copy link
Contributor

Choose a reason for hiding this comment

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

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