[MBL 873] Implement TriggerThirdPartyEvent Mutation #1835

scottkicks · 2023-07-17T20:50:05Z

📲 What

Replacing TriggerCapiEventInput calls with TriggerThirdPartyEvent

🤔 Why

We're going to handle Facebook CAPI and Google Analytics calls with a single mutation.
For more details see the engineering plan

🛠 How

Remove TriggerCapiEvent related files and references.

Replace current .triggerCapiEventInput calls with .triggerThirdPartyEvent
We’re currently triggering this mutation in the following ViewModels:

PledgePaymentMethodsViewModel
ProjectPageViewModel
RewardsCollectionViewModel
ThanksViewModel

Existing parameter differences below:

externalId → deviceId
eventName → eventName
projectId → projectId
appData → appData (we can keep hardcoding these values to true, just like we do now because externalId is required)
we do not need to pass anything into firebaseScreen, firebasePreviousScreen
userId →AppEnvironment.current.currentUser?.id

New Parameters (These are only passed in on the ThanksViewModel):
We can use this existing code…

      if let checkoutDataValues = configData.checkoutData {
        ...
      }

pledgeAmount → checkoutDataValues.revenueInUsd + checkoutDataValues.bonusAmountInUsd + checkoutDataValues.addOnsMinimumUsd
shipping → checkoutDataValues.shippingAmountUsd
transactionId → checkoutDataValues.checkoutId
clientMutationId → ““

✅ Acceptance criteria

Use Charles Proxy, or something similar, to verify that these calls are being sent at the expected times with success 200 responses

codecov · 2023-07-24T16:14:16Z

Codecov Report

Merging #1835 (49e6889) into main (61ffb8b) will increase coverage by 0.05%.
The diff coverage is 79.34%.

@@            Coverage Diff             @@
##             main    #1835      +/-   ##
==========================================
+ Coverage   84.49%   84.54%   +0.05%     
==========================================
  Files        1277     1273       -4     
  Lines      115232   115190      -42     
  Branches    30674    30656      -18     
==========================================
+ Hits        97365    97389      +24     
+ Misses      16799    16732      -67     
- Partials     1068     1069       +1

Files Changed	Coverage Δ
KsApi/MockService.swift	`13.21% <ø> (-0.01%)`	⬇️
KsApi/Service.swift	`9.39% <ø> (+0.14%)`	⬆️
KsApi/ServiceType.swift	`76.06% <ø> (ø)`
...iewModels/PledgePaymentMethodsViewModelTests.swift	`96.79% <ø> (-0.01%)`	⬇️
...ary/ViewModels/PledgePaymentMethodsViewModel.swift	`88.37% <5.55%> (-1.87%)`	⬇️
Library/NSBundleType.swift	`38.29% <50.00%> (+0.52%)`	⬆️
Library/ViewModels/ThanksViewModel.swift	`97.51% <96.87%> (+10.98%)`	⬆️
Library/NSBundleType+Tests.swift	`100.00% <100.00%> (ø)`
Library/TestHelpers/MockBundle.swift	`100.00% <100.00%> (ø)`
Library/ViewModels/ProjectPageViewModel.swift	`97.52% <100.00%> (+5.95%)`	⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

according to CAPI docs all values are required and must be in the given indexed order.

scottkicks · 2023-07-24T16:18:30Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+                extinfo: [
+                  "i2",
+                  "",
+                  "",
+                  "",
+                  "\(ProcessInfo.processInfo.operatingSystemVersion)",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  ""
+                ]


According to the CAPI documentation all values in extInfo are required and must be in the given indexed order.
I paired with Isa on this and this format is the only way we can get a successful 200 response for this mutation.

IMO we should only have to pass the used values (i2, OS version) to the backend and they should be responsible for formatting the data correctly.

Since the docs require this I chose to implement it this way on our end.

So here's my recommendation for how this function should written. The big take-away is to process the value sent into the function outside the function, not inline. Keeps things cleaner.

guard let externalId = AppEnvironment.current.appTrackingTransparency.advertisingIdentifier else { return } var userId = "" if let userValue = AppEnvironment.current.currentUser { userId = "\(userValue.id)" } let projectId = "\(project.id)" var extInfo = Array(repeating: "", count: 16) extInfo[0] = "i2" extInfo[4] = AppEnvironment.current.mainBundle.platformVersion _ = AppEnvironment .current .apiService .triggerThirdPartyEventInput( input: .init( deviceId: externalId, eventName: ThirdPartyEventInputName.AddNewPaymentMethod.rawValue, projectId: projectId, pledgeAmount: nil, shipping: nil, transactionId: nil, userId: userId, appData: .init( advertiserTrackingEnabled: true, applicationTrackingEnabled: true, extinfo: extInfo ), clientMutationId: "" ) )

Also add this to NSBundleType

public var platformVersion: String { return self.infoDictionary?["DTPlatformVersion"] as? String ?? "Unknown" }

And write a test for it in NSBundleType + Tests

How did I get the above?

Start with an example from in app ie. line 740 AppDelegateViewModel that leads toNSBundleType for things like appVersionString (similar property).

Build out that “OS version” here. The benefit of keeping it here as opposed to using ProcessInfo in multiple places, is its’ easy to change later and also test.

Why not use process info?

A lot properties in NSBundle+Type use “self.infoDictionary”. It turns out it is a provided property that contains the OS version in the key - "DTPlatformVersion" - at least on simulator this worked.

I suggest following this pattern below in ProjectPageViewModel, RewardsColletionViewModel and ThanksViewModel.

Library/ViewModels/PledgePaymentMethodsViewModel.swift

msadoon

Ok, this is mostly fine.

I refactored a bit of the parameter passing to keep the code clean.

Normally it would be good to test these network requests wherever they are used in the view model. Oddly we didn't test the CAPI ones. Will enforce this in the eng. plan in the future, so no need to do it here.

The changes are all laid out so make those quickly and I'll approve tomorrow morning. Good work!

Library/Tracking/FacebookCAPIEventName.swift

msadoon · 2023-07-24T20:49:40Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+                extinfo: [
+                  "i2",
+                  "",
+                  "",
+                  "",
+                  "\(ProcessInfo.processInfo.operatingSystemVersion)",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  ""
+                ]


So here's my recommendation for how this function should written. The big take-away is to process the value sent into the function outside the function, not inline. Keeps things cleaner.

guard let externalId = AppEnvironment.current.appTrackingTransparency.advertisingIdentifier else { return } var userId = "" if let userValue = AppEnvironment.current.currentUser { userId = "\(userValue.id)" } let projectId = "\(project.id)" var extInfo = Array(repeating: "", count: 16) extInfo[0] = "i2" extInfo[4] = AppEnvironment.current.mainBundle.platformVersion _ = AppEnvironment .current .apiService .triggerThirdPartyEventInput( input: .init( deviceId: externalId, eventName: ThirdPartyEventInputName.AddNewPaymentMethod.rawValue, projectId: projectId, pledgeAmount: nil, shipping: nil, transactionId: nil, userId: userId, appData: .init( advertiserTrackingEnabled: true, applicationTrackingEnabled: true, extinfo: extInfo ), clientMutationId: "" ) )

msadoon · 2023-07-24T20:50:28Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+                extinfo: [
+                  "i2",
+                  "",
+                  "",
+                  "",
+                  "\(ProcessInfo.processInfo.operatingSystemVersion)",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  ""
+                ]


Also add this to NSBundleType

public var platformVersion: String { return self.infoDictionary?["DTPlatformVersion"] as? String ?? "Unknown" }

And write a test for it in NSBundleType + Tests

msadoon · 2023-07-24T20:53:27Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+                extinfo: [
+                  "i2",
+                  "",
+                  "",
+                  "",
+                  "\(ProcessInfo.processInfo.operatingSystemVersion)",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  ""
+                ]


How did I get the above?

Start with an example from in app ie. line 740 AppDelegateViewModel that leads toNSBundleType for things like appVersionString (similar property).

Build out that “OS version” here. The benefit of keeping it here as opposed to using ProcessInfo in multiple places, is its’ easy to change later and also test.

Why not use process info?

A lot properties in NSBundle+Type use “self.infoDictionary”. It turns out it is a provided property that contains the OS version in the key - "DTPlatformVersion" - at least on simulator this worked.

msadoon · 2023-07-24T20:56:01Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+                extinfo: [
+                  "i2",
+                  "",
+                  "",
+                  "",
+                  "\(ProcessInfo.processInfo.operatingSystemVersion)",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  "",
+                  ""
+                ]


I suggest following this pattern below in ProjectPageViewModel, RewardsColletionViewModel and ThanksViewModel.

Library/ViewModels/ThanksViewModel.swift

scottkicks · 2023-07-25T02:08:44Z

@msadoon I've addressed your comments. thanks for the feedback. I tried your your suggestion for calculating the pledgeAmount and didn't find that it added more accuracy. I think your solution makes more sense anyway though so I've made that change as well.

msadoon

There were mistakes here I found, please correct before we can merge.

Library/ViewModels/ProjectPageViewModel.swift

scottkicks added enhancement WIP labels Jul 17, 2023

scottkicks self-assigned this Jul 17, 2023

scottkicks added this to the release-5.9.0 milestone Jul 17, 2023

scottkicks added 4 commits July 24, 2023 08:33

rename FacebookCAPIEventName to ThirdPartyEventInputName

790433c

replace .triggerCapiEventInput calls with .triggerThirdPartyEventInput

5f15543

cleanup TriggerCapiEventInput related files and references

b3677fa

cleanup mutation calls

d0f3453

scottkicks force-pushed the scott/mbl-873 branch from fae847e to d0f3453 Compare July 24, 2023 15:17

include required OS version in extInfo at correct index

2f7b85a

according to CAPI docs all values are required and must be in the given indexed order.

scottkicks commented Jul 24, 2023

View reviewed changes

Library/ViewModels/PledgePaymentMethodsViewModel.swift Outdated Show resolved Hide resolved

scottkicks removed the WIP label Jul 24, 2023

scottkicks requested a review from msadoon July 24, 2023 17:32

scottkicks marked this pull request as ready for review July 24, 2023 17:32

scottkicks requested a review from ifosli July 24, 2023 17:41

msadoon suggested changes Jul 24, 2023

View reviewed changes

scottkicks added 2 commits July 24, 2023 15:21

rename input name

8b39777

add platformVersion computed property to NSBundleType

1c0597f

scottkicks requested a review from msadoon July 25, 2023 02:08

scottkicks force-pushed the scott/mbl-873 branch from e6bcf1d to cfd99ce Compare July 25, 2023 02:11

msadoon suggested changes Jul 25, 2023

View reviewed changes

Library/ViewModels/ProjectPageViewModel.swift Show resolved Hide resolved

Library/ViewModels/ProjectPageViewModel.swift Outdated Show resolved Hide resolved

Library/ViewModels/ProjectPageViewModel.swift Outdated Show resolved Hide resolved

scottkicks force-pushed the scott/mbl-873 branch from e42b6a7 to e5c4989 Compare July 25, 2023 15:01

refactor mutation calls

106e19a

scottkicks force-pushed the scott/mbl-873 branch from e5c4989 to 106e19a Compare July 25, 2023 15:15

kickstarter deleted a comment from nativeksr Jul 25, 2023

Merge branch 'main' into scott/mbl-873

49e6889

scottkicks requested a review from msadoon July 25, 2023 16:26

msadoon approved these changes Jul 25, 2023

View reviewed changes

scottkicks merged commit 91d3abe into main Jul 25, 2023

scottkicks deleted the scott/mbl-873 branch July 25, 2023 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MBL 873] Implement TriggerThirdPartyEvent Mutation #1835

[MBL 873] Implement TriggerThirdPartyEvent Mutation #1835

scottkicks commented Jul 17, 2023 •

edited

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading

scottkicks Jul 24, 2023 •

edited

Loading

msadoon Jul 24, 2023

msadoon Jul 24, 2023

msadoon Jul 24, 2023

msadoon Jul 24, 2023

msadoon left a comment

msadoon Jul 24, 2023

msadoon Jul 24, 2023

msadoon Jul 24, 2023

msadoon Jul 24, 2023

scottkicks commented Jul 25, 2023 •

edited

Loading

msadoon left a comment

[MBL 873] Implement TriggerThirdPartyEvent Mutation #1835

[MBL 873] Implement TriggerThirdPartyEvent Mutation #1835

Conversation

scottkicks commented Jul 17, 2023 • edited Loading

📲 What

🤔 Why

🛠 How

✅ Acceptance criteria

codecov bot commented Jul 24, 2023 • edited Loading

Codecov Report

scottkicks Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msadoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottkicks commented Jul 25, 2023 • edited Loading

msadoon left a comment

Choose a reason for hiding this comment

scottkicks commented Jul 17, 2023 •

edited

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading

scottkicks Jul 24, 2023 •

edited

Loading

scottkicks commented Jul 25, 2023 •

edited

Loading