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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EP-149] Rename Properties (Phase One) #1370

Merged
merged 4 commits into from
Feb 9, 2021

Conversation

singhhari
Copy link
Contributor

馃摬 What

As part of our ongoing initiative to implement Segment and replace our legacy tracking clients, we begin here with some renaming of old events. The Insights team has provided us with a set of properties and the contexts they belong to; this PR is one of many upcoming extensions to the addition of Segment.

馃 Why

Our events and their respective tracking clients are present everywhere in this project. It's important that we break down all these tasks into manageable bits. Each phase of work will be represented by anywhere from one to three tickets.

馃洜 How

A lot of the work is in our KSRAnalytics file and the tests that make use of it.

@codecov
Copy link

codecov bot commented Feb 9, 2021

Codecov Report

Merging #1370 (e6cccc3) into master (c3a1833) will increase coverage by 0.01%.
The diff coverage is 98.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1370      +/-   ##
==========================================
+ Coverage   85.83%   85.85%   +0.01%     
==========================================
  Files        1100     1100              
  Lines       95932    95971      +39     
==========================================
+ Hits        82347    82394      +47     
+ Misses      13585    13577       -8     
Impacted Files Coverage 螖
...els/graphql/adapters/Reward+GraphRewardTests.swift 94.87% <95.00%> (+0.13%) 猬嗭笍
KsApi/models/Reward.swift 95.91% <100.00%> (+0.36%) 猬嗭笍
Library/OptimizelyClientType.swift 96.22% <100.00%> (酶)
Library/OptimizelyClientTypeTests.swift 100.00% <100.00%> (酶)
Library/SharedFunctions.swift 84.23% <100.00%> (酶)
Library/TestHelpers/MockOptimizelyClient.swift 100.00% <100.00%> (酶)
Library/Tracking/KSRAnalytics.swift 81.64% <100.00%> (+1.35%) 猬嗭笍
Library/Tracking/KSRAnalyticsTests.swift 100.00% <100.00%> (酶)
Library/UIDeviceType.swift 62.50% <100.00%> (酶)
...brary/ViewModels/DiscoveryPageViewModelTests.swift 100.00% <100.00%> (酶)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update c3a1833...e6cccc3. Read the comment docs.

result["reward_id"] = data.rewardId
result["reward_title"] = data.rewardTitle
result["reward_is_limited_quantity"] = reward.limit != nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using this logic anywhere else in the app? What do you think about making a computed property on the Reward model?

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 good catch. It looks like we are using this same logic for this property and the one below it in another file as well. Changed to a computed property and replaced in a few places.

@singhhari singhhari merged commit 1eed8aa into master Feb 9, 2021
@singhhari singhhari deleted the EP-149-rename-properties-phase-1 branch February 9, 2021 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants