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-225] CTA Clicked (pledge_initiate) #1383

Merged
merged 9 commits into from
Mar 3, 2021

Conversation

tobitech
Copy link
Contributor

@tobitech tobitech commented Mar 1, 2021

馃摬 What

"Project Page Pledge Button Clicked" event is updated to CTA Clicked with pledge_initiate context

馃 Why

Convert DataLake event to Segment event

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #1383 (a5b0f0a) into master (f113547) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1383   +/-   ##
=======================================
  Coverage   85.86%   85.86%           
=======================================
  Files        1104     1104           
  Lines       96140    96147    +7     
=======================================
+ Hits        82550    82558    +8     
+ Misses      13590    13589    -1     
Impacted Files Coverage 螖
Library/Tracking/KSRAnalytics.swift 82.60% <100.00%> (+0.02%) 猬嗭笍
Library/Tracking/KSRAnalyticsTests.swift 100.00% <100.00%> (酶)
...wModels/PledgeCTAContainerViewViewModelTests.swift 100.00% <100.00%> (酶)
Library/Navigation.swift 77.25% <0.00%> (+0.36%) 猬嗭笍

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 f113547...a5b0f0a. Read the comment docs.

@tobitech tobitech requested a review from singhhari March 1, 2021 21:32
Copy link
Contributor

@singhhari singhhari 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, just a couple of small changes.

* Add CTA Context test to PledgeCTAContainerViewModel
@singhhari singhhari self-assigned this Mar 2, 2021
@singhhari singhhari self-requested a review March 2, 2021 15:55
@tobitech tobitech requested a review from singhhari March 2, 2021 19:33
Copy link
Contributor

@singhhari singhhari left a comment

Choose a reason for hiding this comment

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

left a small comment but lgtm

@@ -764,9 +763,10 @@ public final class KSRAnalytics {
case .pledge:
let allProps = props
.withAllValuesFrom(optimizelyProperties)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, I missed this earlier but can you get rid of the optimizelyProperties param. It doesn't look like the new event requires this info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I got rid of it, this test testTrackingEvents_Pledge() in PledgeCTAContainerViewViewModelTests fails, because it checks for those optimizelyProperties

@tobitech tobitech merged commit 8d91740 into master Mar 3, 2021
@tobitech tobitech deleted the EP-225-cta_clicked_pledge_initiate branch March 3, 2021 15:58
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.

None yet

2 participants