Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[NT-877] Read more about this campaign tracking bugfix #1120

Merged
merged 4 commits into from
Mar 25, 2020

Conversation

justinswart
Copy link
Contributor

@justinswart justinswart commented Mar 24, 2020

📲 What

Fixes a tracking event that wasn't being sent to Optimizely when Read more about this campaign was tapped.

🤔 Why

Tracking this event is necessary for our metrics.

🛠 How

The bug was that we were checking if project.personalization.isBacking was false. In this case isBacking can also be nil so comparing a nil value to false fails. We could coalesce this nil to false but I figured we can also just check to see whether the actual backing property is nil for the same result.

✅ Acceptance criteria

Place yourself in variant 1 or 2 for native_project_page_campaign_details. Open Charles Proxy.

  • Navigate to a project page, tap on the Read more about this campaign button. Observe that an event was tracked for Campaign Details Button Clicked.

.filter { project, _ in project.state == .live && project.personalization.isBacking == false }
.logEvents(identifier: "***")
.filter { project, _ in
project.state == .live && project.personalization.backing == nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh this is such a common pitfall for us. Do you think it could be worthwhile to add a helper function to SharedFunctions.swift to handle all the logical cases for this? Something like

func currentUserHasBacked(project: Project) {
    return project.personalization.backing != nil || project.personalization.isBacking == .some(true)
}

To be honest I'm not entirely sure that personalization.isBacking means what we think it does even though we use it all over the place 🤔.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I could do that. It looks like I also missed a logEvents still in the code there 😂

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

lgtm

@justinswart justinswart merged commit 9ce4d82 into master Mar 25, 2020
@justinswart justinswart deleted the NT-877-tracking-bugfix branch March 25, 2020 19:31
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants