[NT-727] Send checkout_id with checkout properties for tracking #1053

Scollaco · 2020-01-28T16:04:14Z

📲 What

Adds checkout_id property to the checkout properties group.

🤔 Why

Because we want to track the checkout id in the DataLake.
For details: JIRA ticket

Note: Even though the ticket determines that all tracking events that require the checkout properties should contain this property, this is not applicable, since some of them will fire before the backing is created.

🛠 How

Changed the createBacking mutation to return checkout_id
Added checkoutId property on Checkout model
The checkoutPropertiesData function now receives an optional checkoutId as argument
Updated tests

✅ Acceptance criteria

After backing a project, the Thanks Page Viewed event should contain the checkout_id property.

ifbarrera

We need to add the id field to the UpdateBackingMutation response query - otherwise this change will break all update backing requests because we won't be able to deserialize the response.

ifbarrera · 2020-01-28T18:04:34Z

Library/StripeTypes.swift

@@ -2,11 +2,16 @@ import Foundation
 import KsApi

 public protocol StripeSCARequiring {
+  var checkoutId: String? { get }


What was the motivation for adding checkoutId in the StripeSCARequiring protocol? From my understanding, checkoutId is not actually required to determine SCA eligibility, so it's a bit misleading to add it in here.

I'd added there to reuse the existing login on the viewModel, but I guess you're right. I will change that.

ifbarrera · 2020-01-28T18:06:57Z

Library/ViewModels/PledgeViewModel.swift


        return (data.project, data.reward, checkoutPropsData)
      }

-    self.goToThanks = thanksPageData
-      .takeWhen(createBackingCompletionEvents)
+    self.goToThanks = createBackingCompletionEvents


Why was this change needed? From what I can see the way it was before should still work, and is one line of code shorter 😄

I forgot to mention here... using takeWhen broke some other test and this solution was the one that made all of them pass 😅

I think we might want to be pretty specific that this should only fire when the createBackingCompletionEvents emit 🤔 Do you know what broke/why?

I noticed that in some tests, thanksPageData emits before createBackingCompletionEvents and in others, like the one I've mentioned, the order changes and the test fails.
Using combineLatest we don't depend on the order that the signals emit and it avoids a race condition

I think it's just that combineLatest will also emit regardless of the order, in this case we just want to be sure that we only goToThanks when createBackingCompletionEvents emits. Given that this is tricky to do regression testing on I would try to avoid this change. Let's rather dig into why the tests break with your change? I can take a look with you too.

Ok, I see what's happening - previously the thanksPageData had all of the necessary data before the completion events emit. This changes that because now we want the checkoutId in the ThanksPageData which causes a bit of a race condition. I would move the creation of the thanksPageData to be a a signal that is takePairWhen createBackingCompletionEvents and retrieve the checkoutId right at the end.

Another option would be using zip instead of combineLatest. This guarantees that goToThanks only emits after both signals emit. But just for curiosity, what are the disadvantages of using .combineLatest in this case?

I think the concern is that goToThanks could fire before createBackingCompletionEvents during a retry flow, causing us to send the user to thanks page before we've confirmed that we're clear to do so, particularly during the SCA flow. We might be prematurely sending users to the thanks page before they've completed the SCA auth flow.

what are the disadvantages of using .combineLatest in this case?

I think the main concern is just that it doesn't accurately represent what we want to happen. Previously this was a takeWhen because that accurately describes the event, "take this data when this other thing happens". This has changed because we now require some data from the thing that has happened, but takePairWhen accurately describes that - "take some data from this other event, combine it with what we have, but only when that other thing happens". combineLatest on the other hand blurs this by saying "combine all this data when either of these things happen".

ifbarrera · 2020-01-28T18:10:33Z

KsApi/models/UpdateBackingEnvelopeTests.swift

@@ -7,6 +7,7 @@ final class UpdateBackingEnvelopeTests: XCTestCase {
    {
      "updateBacking": {
        "checkout": {
+          "id": "2020",


Since the UpdateBackingEnvelope now requires a checkoutId to be present for deserialization, you'll need to also add it to the UpdateBackingMutation as well so that deserialization doesn't break for update backing.

…StripeSCARequiring protocol

…ithub.com/kickstarter/ios-oss into NT-727_createBacking_mutation_checkout_id

…l is modified

ifbarrera · 2020-01-29T20:54:10Z

Library/ViewModels/PledgeViewModel.swift

@@ -352,7 +352,7 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge
      .switchMap { input in
        AppEnvironment.current.apiService.updateBacking(input: input)
          .ksr_delay(AppEnvironment.current.apiDelayInterval, on: AppEnvironment.current.scheduler)
-          .map { $0 as StripeSCARequiring }
+          .map { ($0.updateBacking.checkout.id, $0 as StripeSCARequiring) }


Why do we need to map out the checkout id in the updateBacking case? 🤔

Because createOrUpdateEvent requires that createBackingEvents and updateBackingEvents have the same type (<(String, StripeSCARequiring), GraphError>.Event>) . So I saw some options here:
1- map out checkout id even if we won't use it
2- make the string of the tuple optional in both createBackingEvents and updateBackingEvents signals

Tried other solutions but wasn't able to make the compiler happy.

Hmm I think just mapping the value in createOrUpdateEvent seems to work and doesn't require mapping out the checkoutId in the update events:

let createOrUpdateEvent = Signal.merge( createBackingEvents.map { $0.map(second) }, updateBackingEvents )

I don't know why I didn't think of that 🤦🏻‍♂️ Thanks, @ifbarrera !

ifbarrera · 2020-01-29T20:58:09Z

Library/ViewModels/PledgeViewModel.swift


        return (data.project, data.reward, checkoutPropsData)
      }

-    self.goToThanks = thanksPageData
-      .takeWhen(createBackingCompletionEvents)
+    self.goToThanks = createBackingCompletionEvents


ifbarrera · 2020-01-31T15:48:00Z

Library/ViewModels/PledgeViewModel.swift

@@ -352,7 +352,7 @@ public class PledgeViewModel: PledgeViewModelType, PledgeViewModelInputs, Pledge
      .switchMap { input in
        AppEnvironment.current.apiService.updateBacking(input: input)
          .ksr_delay(AppEnvironment.current.apiDelayInterval, on: AppEnvironment.current.scheduler)
-          .map { $0 as StripeSCARequiring }
+          .map { ($0.updateBacking.checkout.id, $0 as StripeSCARequiring) }


Hmm I think just mapping the value in createOrUpdateEvent seems to work and doesn't require mapping out the checkoutId in the update events:

let createOrUpdateEvent = Signal.merge( createBackingEvents.map { $0.map(second) }, updateBackingEvents )

ifbarrera · 2020-01-31T15:51:13Z

Library/ViewModels/PledgeViewModel.swift

@@ -867,6 +882,7 @@ private func checkoutPropertiesData(from createBackingData: CreateBackingData, i

  return Koala.CheckoutPropertiesData(
    amount: amount,
+    checkoutId: checkoutId,


While testing this I noticed that the checkoutId that's returned from Graph is the relayId. I think for tracking we actually want to send the Int value of the checkoutId 🤔

…ithub.com/kickstarter/ios-oss into NT-727_createBacking_mutation_checkout_id

justinswart · 2020-01-31T21:55:39Z

Library/ViewModels/PledgeViewModel.swift

-      .map { data, isApplePay -> ThanksPageData in
-        let checkoutPropsData = checkoutPropertiesData(from: data, isApplePay: isApplePay)
+      .takeWhen(createBackingCompletionEvents)
+      .combineLatest(with: checkoutId)


So this change is actually the same thing as before, although we've moved things around this still combines the latest emissions from the two signals. Basically it reads like this "emit createBackingDataAndIsApplePay when createBackingCompletionEvents emits and combine its emission with checkoutId" which means that whenever checkoutId or createBackingCompletionEvents emit then this will emit. We just want to be explicit that this should only emit at all when createBackingCompletionEvents emits.

This is actually achievable just by moving things around slightly and carrying the checkoutId all the way through to createBackingCompletionEvents which will require a few changes higher up in the chain:

let thanksPageData = createBackingDataAndIsApplePay .takePairWhen(createBackingCompletionEvents) .map(unpack) .map { data, isApplePay, checkoutId -> ThanksPageData in let checkoutPropsData = checkoutPropertiesData( from: data, checkoutId: checkoutId, isApplePay: isApplePay ) return (data.project, data.reward, checkoutPropsData) }

Scollaco added 7 commits January 24, 2020 16:10

Request checkout id in the createBacking mutation response

6f871af

Made tests buildable again

3b71256

Checkpoint - Sending checkout_id with checkout properties

72e5cd7

Reverted code that added checkout_id to button event

0f2557a

Updated tests

c156c2c

SwiftFormat

9d1cc8a

Reverted koala tracking environment property

f166605

Scollaco requested a review from ifbarrera January 28, 2020 16:04

Scollaco assigned ifbarrera Jan 28, 2020

ifbarrera changed the title ~~[NT-727] Create backing mutation checkout~~ [NT-727] Send checkout_id with checkout properties for tracking Jan 28, 2020

ifbarrera suggested changes Jan 28, 2020

View reviewed changes

Scollaco added 5 commits January 28, 2020 13:33

Merge branch 'master' into NT-727_createBacking_mutation_checkout_id

94a77eb

added id to updateBacking mutation, removed checkoutId property from …

ac847e1

…StripeSCARequiring protocol

Merge branch 'NT-727_createBacking_mutation_checkout_id' of https://g…

35e12b7

…ithub.com/kickstarter/ios-oss into NT-727_createBacking_mutation_checkout_id

Merge branch 'master' into NT-727_createBacking_mutation_checkout_id

9f6b906

Added comment to make sure mutations will be updated if Checkout mode…

4776755

…l is modified

ifbarrera reviewed Jan 29, 2020

View reviewed changes

Scollaco added 2 commits January 30, 2020 13:51

Changed way goToThanks signal is created

14b2abc

Merge branch 'master' into NT-727_createBacking_mutation_checkout_id

b9193f1

ifbarrera reviewed Jan 31, 2020

View reviewed changes

Scollaco added 4 commits January 31, 2020 11:59

Reverted change made on updateBackingEvents

1519b3e

Merge branch 'NT-727_createBacking_mutation_checkout_id' of https://g…

54ea848

…ithub.com/kickstarter/ios-oss into NT-727_createBacking_mutation_checkout_id

Changed checkoutId to send Int instead

101e15a

Reverted code that broke tests

06249fd

justinswart suggested changes Jan 31, 2020

View reviewed changes

ifbarrera and others added 4 commits January 31, 2020 17:53

Guarantee completion event fires last

fae2c49

Minimal changes to current behaviour while adding checkoutId

d5fe252

Merge branch 'master' into NT-727_createBacking_mutation_checkout_id

53f2ed4

Fix alignment

c649eb7

justinswart approved these changes Feb 4, 2020

View reviewed changes

ifbarrera approved these changes Feb 4, 2020

View reviewed changes

Merge branch 'master' into NT-727_createBacking_mutation_checkout_id

a7549a6

Scollaco merged commit 09457af into master Feb 4, 2020

Scollaco deleted the NT-727_createBacking_mutation_checkout_id branch February 4, 2020 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NT-727] Send checkout_id with checkout properties for tracking #1053

[NT-727] Send checkout_id with checkout properties for tracking #1053

Scollaco commented Jan 28, 2020 •

edited by justinswart

Loading

ifbarrera left a comment

ifbarrera Jan 28, 2020

Scollaco Jan 28, 2020

ifbarrera Jan 28, 2020

Scollaco Jan 28, 2020

justinswart Jan 28, 2020

Scollaco Jan 29, 2020 •

edited

Loading

justinswart Jan 29, 2020

justinswart Jan 29, 2020

ifbarrera Jan 29, 2020

Scollaco Jan 29, 2020

ifbarrera Jan 30, 2020

justinswart Jan 31, 2020

ifbarrera Jan 28, 2020

ifbarrera Jan 29, 2020

Scollaco Jan 30, 2020

ifbarrera Jan 31, 2020

Scollaco Jan 31, 2020

ifbarrera Jan 29, 2020

ifbarrera Jan 31, 2020

ifbarrera Jan 31, 2020

justinswart Jan 31, 2020

[NT-727] Send checkout_id with checkout properties for tracking #1053

[NT-727] Send checkout_id with checkout properties for tracking #1053

Conversation

Scollaco commented Jan 28, 2020 • edited by justinswart Loading

📲 What

🤔 Why

🛠 How

✅ Acceptance criteria

ifbarrera 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

Choose a reason for hiding this comment

Scollaco Jan 29, 2020 • 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

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

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

Choose a reason for hiding this comment

Scollaco commented Jan 28, 2020 •

edited by justinswart

Loading

Scollaco Jan 29, 2020 •

edited

Loading