-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[EP-439] QA Phase 2 - Properties to remove #1416
[EP-439] QA Phase 2 - Properties to remove #1416
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1416 +/- ##
==========================================
- Coverage 86.11% 86.10% -0.01%
==========================================
Files 1106 1106
Lines 97950 97922 -28
==========================================
- Hits 84347 84319 -28
Misses 13603 13603
Continue to review full report at Codecov.
|
props["device_orientation"] = self.deviceOrientation | ||
props["device_distinct_id"] = self.distinctId |
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.
Doesn't look like we are using distinctId for anything else in the file. I would delete this and double check everything is compiling fine.
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.
Thanks, I missed that.
@@ -1503,10 +1503,7 @@ public final class KSRAnalytics { | |||
props["country"] = self.config?.countryCode | |||
props["display_language"] = AppEnvironment.current.language.rawValue | |||
props["device_type"] = self.device.deviceType | |||
props["device_manufacturer"] = "Apple" | |||
props["device_model"] = KSRAnalytics.deviceModel |
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.
Doesn't look like we are using deviceModel for anything else in the file. I would delete this and double check everything is compiling fine.
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.
lgtm
📲 What
Remove the following properties from segment.
🤔 Why
Based on feedback from QA on Phase 2 tickets, some Segment event properties aren't needed.