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

[WEB-671]: Setup TriggerCapiEventInput GraphQL Mutation #1792

Merged
merged 4 commits into from Feb 16, 2023
Merged

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Feb 13, 2023

πŸ“² What

Before we can send CAPI events, we need to implement a mutation that will allow us to send the relevant data to our server.
This mutation will be called in the following cases when:

  1. Members have given consent for KS to track their data
  2. The relevant project has it's sendMetaCapiEvents property set to true

Event Cases:

  • Project page visit
  • Select back this project
  • Initiate checkout (selects a reward)
  • Add payment info tapped (could be swapping existing card or adding anew card)
  • Purchase (have reached the thank you page)

πŸ€” Why

If Facebook app users decline permission for FB to track them for advertising purposes, then our KS Facebook Pixel will be rendered useless. Using Facebook's Conversions API (CAPI) is the recommended workaround.

πŸ›  How

The web team will be handling the actual events to Facebook's Conversions API and has added this new mutation so that we can notify them about what data and which events to send.

βœ… Acceptance criteria

  • Network call is readily available and usable.

@scottkicks scottkicks added this to the release 5.6.2 milestone Feb 13, 2023
@scottkicks scottkicks self-assigned this Feb 13, 2023
@scottkicks scottkicks changed the title [WEB-671]: DRAFT | Add CAPI Tracking Support [WEB-671]: DRAFT | Setup TriggerCapiEventInput GraphQL Mutation Feb 14, 2023
@nativeksr
Copy link
Collaborator

nativeksr commented Feb 14, 2023

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

Left some review comments, let's go over in our 1-1, feel free to correct before our meeting too :)

KsApi/Service.swift Outdated Show resolved Hide resolved
KsApi/mutations/inputs/ApplePayParams.swift Outdated Show resolved Hide resolved
KsApi/MockService.swift Outdated Show resolved Hide resolved
KsApi/Service.swift Show resolved Hide resolved
KsApi/ServiceType.swift Outdated Show resolved Hide resolved
Kickstarter.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
@scottkicks scottkicks marked this pull request as ready for review February 15, 2023 19:28
@scottkicks scottkicks changed the title [WEB-671]: DRAFT | Setup TriggerCapiEventInput GraphQL Mutation [WEB-671]: Setup TriggerCapiEventInput GraphQL Mutation Feb 15, 2023
@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #1792 (1af1e3f) into main (e273a8b) will decrease coverage by 0.01%.
The diff coverage is 83.09%.

@@            Coverage Diff             @@
##             main    #1792      +/-   ##
==========================================
- Coverage   85.24%   85.23%   -0.01%     
==========================================
  Files        1277     1281       +4     
  Lines      116682   116753      +71     
  Branches    30801    30823      +22     
==========================================
+ Hits        99462    99519      +57     
- Misses      16146    16160      +14     
  Partials     1074     1074              
Impacted Files Coverage Ξ”
KsApi/Service.swift 9.58% <0.00%> (-0.16%) ⬇️
KsApi/ServiceType.swift 76.06% <ΓΈ> (ΓΈ)
KsApi/MockService.swift 13.21% <14.28%> (+<0.01%) ⬆️
....TriggerCapiEventInput+TriggerCapiEventInput.swift 100.00% <100.00%> (ΓΈ)
...gerCapiEventInput+TriggerCapiEventInputTests.swift 100.00% <100.00%> (ΓΈ)
KsApi/mutations/inputs/TriggerCapiEventInput.swift 100.00% <100.00%> (ΓΈ)
.../mutations/inputs/TriggerCapiEventInputTests.swift 100.00% <100.00%> (ΓΈ)
Library/Navigation.swift 85.20% <0.00%> (-0.65%) ⬇️

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

Almost perfect, just add a test class for TriggerCapiEventInput.swift and use the example of CreatePaymentSourceSetupIntentInputTests to write a quick test.

@scottkicks
Copy link
Contributor Author

scottkicks commented Feb 16, 2023

@msadoon not sure why some files are being added/not added by git. The original test file that I had already done should be in here now. Sorry about that.

Also my charles wan't consistently seeing the call being emitted during testing. if you have time to test that it would be helpful!

Comment on lines +22 to +23
XCTAssertNotNil(input["appData"])
XCTAssertNotNil(input["customData"])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since GraphAPI.AppDataInput & GraphAPI.CustomDataInput don't conform to Equatable and they're auto generated for us, I opted to just assert that the are not nil here.

Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

This looks fine for now. I didn't test it via Charles, but it's probably better if you get Charles successfully logging calls so you can see the results for yourself or in the future with new network calls.

There's a bunch of tutorials online. One I found here seems pretty detailed - https://www.kodeco.com/21931256-charles-proxy-tutorial-for-ios

If you want we can pair to configure this on your mac.

@scottkicks scottkicks merged commit 40d368f into main Feb 16, 2023
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

3 participants