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

feat(analytics): add tag name(s) as tag event value #671

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

dskuza
Copy link
Contributor

@dskuza dskuza commented Apr 26, 2023

Summary

Adds tag name(s) as tag event values to tag events sent from both the app and the Save extension.

References

IN-1355

Implementation Details

A tag(Name) parameter was added to the appropriate static functions to add a value to the Engagement event. The correct functions were then updated to supply the correct name(s), either by utilizing a string in a calling function, or a TagType as appropriate.

Test Steps

  • When performing tag engagements, the Engagement events sent should include a value field.

PR Checklist:

  • Added Unit / UI tests
  • Self Review (review, clean up, documentation, run tests)
  • Basic Self QA

@dskuza dskuza requested a review from a team as a code owner April 26, 2023 20:15
@dskuza dskuza requested review from cyndichin and timc-mozilla and removed request for a team April 26, 2023 20:15
@Gio2018 Gio2018 self-requested a review April 26, 2023 20:16
@pocket-ci
Copy link
Contributor

Messages
📖 No SwiftLint violations! 🎉
📖 Project coverage: 32.26%
📖 Checking XCode Environment Variables
📖 Edited 6 files
📖 Created 0 files

Analytics: Coverage: 29.76

File Coverage
SaveTo.swift 51.61%
Tags.swift 75.61%

PocketKit: Coverage: 20.15

File Coverage
TagsFilterViewModel.swift 92.0%
PocketAddTagsViewModel.swift 79.69%

SaveToPocketKit: Coverage: 29.16

File Coverage
SaveToAddTagsViewModel.swift 80.47%

SharedPocketKit: Coverage: 51.65

File Coverage
AddTagsViewModel.swift 70.0%

Generated by 🚫 Danger Swift against 5f09963

Copy link
Collaborator

@Gio2018 Gio2018 left a comment

Choose a reason for hiding this comment

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

Looks good to me @dskuza ! side note: it's be nice if we could see those values represented in the console somehow, but I feel this is a broader scope than just tag-related engagement events, so it does not concern this particular PR.

Copy link
Contributor

@cyndichin cyndichin left a comment

Choose a reason for hiding this comment

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

analytics call look good! thanks for picking this up! one teeny nit I noticed.

static func remoteInputTag(itemUrl: URL) -> Event {
/// - Parameters:
/// - tag: The tag that was removed from the list of input tags.
/// /// - itemUrl: The url of the item to which a tag was removed.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra /// also in Tags.swift

@dskuza dskuza force-pushed the tag-analytics-adding-value branch from 5f09963 to 12fb233 Compare April 26, 2023 20:44
@dskuza dskuza enabled auto-merge (rebase) April 26, 2023 20:44
@dskuza dskuza merged commit 601c641 into develop Apr 26, 2023
@dskuza dskuza deleted the tag-analytics-adding-value branch April 26, 2023 20:57
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.

5 participants