[NT-357][NT-358] Change Payment Method UI #887

justinswart · 2019-10-10T02:23:53Z

📲 What

An unfortunately large PR that adds Change payment method UI to PledgeViewController.

Note: This is UI-only, we'll add the mutation request in a subsequent PR, as well as the ability to update Apple Pay.

🤔 Why

Allows us to update the payment method for a pledge.

🛠 How

This repurposes the PledgeViewController to configure its views based on a third PledgeViewContext, .changedPaymentMethod. It also removes the pledge button from PledgePaymentMethodsViewController so that there is a single submitButton which triggers different events based on the context. We could consider doing this for the continue button too.

Added changePaymentMethod case to PledgeViewContext enum.
Added convenience vars to PledgeViewContext which can be more easily map'd over in PledgeViewModel.
Tests for PledgeViewContext.
Tests for PledgeViewModel for the new context.

👀 See

✅ Acceptance criteria

Navigate to Manage Pledge on a project that you've backed, select Change payment method from the context menu.

You should see the amount that you backed.
If the reward had shipping enabled, this should be shown below the amount.
The total should be shown and the T&Cs.
The payment methods scrollview should be visible and you should be able to select a new payment method.
The confirm button should enable/disable based on whether you've selected the existing payment method for the reward. Note: It appears this isn't currently working because the comparison needs to be made using a GraphID (will fix).

⏰ TODO

Wire up submitButton to perform the updateBacking mutation in a subsequent PR.
Wire up ability to update Apple Pay payment.

# Conflicts: # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_de_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_de_device_phone4_7inch@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_es_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_es_device_phone4_7inch@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_fr_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_fr_device_phone4_7inch@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_ja_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_ja_device_phone4_7inch@2x.png

justinswart · 2019-10-10T02:25:25Z

Kickstarter-iOS/Views/Controllers/PledgeAmountSummaryViewController.swift

+import Prelude_UIKit
+import UIKit
+
+final class PledgeAmountSummaryViewController: UIViewController {


This is just copy-pasting the required views from ManagePledgeSummaryView.

As discussed on Zoom, I'll create a follow-up PR to incorporate this as as subview of ManagePledgeSummaryViewController to promote reuse.

justinswart · 2019-10-10T02:25:45Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

@@ -68,19 +59,12 @@ final class PledgePaymentMethodsViewController: UIViewController {
      action: #selector(PledgePaymentMethodsViewController.applePayButtonTapped),
      for: .touchUpInside
    )
-
-    self.pledgeButton.addTarget(


This is replaced by submitButton in PledgeViewController.

justinswart · 2019-10-10T02:26:09Z

Kickstarter-iOS/Views/Controllers/PledgeViewController.swift

@@ -60,6 +59,10 @@ final class PledgeViewController: UIViewController, MessageBannerViewControllerP

  internal var messageBannerViewController: MessageBannerViewController?

+  private lazy var pledgeAmountSummaryViewController: PledgeAmountSummaryViewController = {
+    PledgeAmountSummaryViewController.instantiate()


Not the most amazing name for this VC, open to suggestions.

justinswart · 2019-10-10T02:26:48Z

Library/PledgeViewContext.swift

+  case update
+  case changePaymentMethod
+
+  var confirmationLabelHidden: Bool {


These booleans basically map directly in PledgeViewModel, it just helped for readability to do this here. They're tested in both places.

justinswart · 2019-10-10T02:28:02Z

Library/ViewModels/PledgeViewModel.swift

+  paymentSourceValid: Bool,
+  context: PledgeViewContext
+) -> Bool {
+  if context.isUpdating {


In an updating context we enable the submit button if anything has changed and his valid. In a creating context we require all values to be valid.

Library/ViewModels/PledgeViewModelTests.swift

ksr-ci-bot · 2019-10-10T17:13:20Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

Kickstarter-iOS/Views/Controllers/PledgeViewControllerTests.swift

ifbarrera · 2019-10-11T15:53:18Z

Library/ViewModels/AddNewCardViewModelTests.swift

@@ -392,8 +392,6 @@ internal final class AddNewCardViewModelTests: TestCase {
  }

  func testUnsupportedCardMessage_hidesWithValidCardBrand_AndExistingCardNumber_withSettingsIntent() {
-    let project = Project.template


ifbarrera · 2019-10-11T15:54:18Z

Library/ViewModels/ManagePledgeViewModelTests.swift

    self.vm.inputs.viewDidLoad()

-    self.goToChangePaymentMethod.assertDidNotEmitValue()
+    self.goToChangePaymentMethodProject.assertDidNotEmitValue()


Should we add aself.goToChangePaymentMethodReward.assertDidNotEmitValue() here too?

Ah yes, thanks!

ifbarrera · 2019-10-11T15:56:14Z

Library/ViewModels/PledgeAmountSummaryViewModel.swift

+    self.shippingAmountText = project
+      .combineLatest(with: shippingAmount)
+      .map { project, shippingAmount in
+        shippingValue(with: project, with: shippingAmount)


Hmm this function name looks a bit strange. Maybe shippingValue(with project: Project, shippingAmount: Double)?

Oh, sure, this was a copy-paste 😄

ifbarrera · 2019-10-11T15:58:09Z

Library/ViewModels/PledgeAmountSummaryViewModel.swift

+    )
+    .map(first)
+
+    let backing = project


Something you might consider doing is configuring this view with a project and a backing:

func configureWith(project: Project, backing: Backing)

It would simplify a few things in this view model.

I think seeing as the Backing is from the Project I'm going to leave it as is for now.

ifbarrera · 2019-10-11T16:02:39Z

Library/ViewModels/PledgeAmountSummaryViewModel.swift

+    let _ = backing.locationName else {
+    return true
+  }
+  if let reward = backing.reward {


This is interesting. Do you think we need this extra layer of logic? I'd imagine if the backing.locationName and backing.locationId are not nil that you could assume that the backing was created with a shipping address. 🤔

I wonder if this could cause a bug in a situation where the reward's shipping eligibility is modified by the creator after the user has created a pledge (not sure if that's even possible).

Another copy-paste, I'll check it out, thanks!

Confirmed with team R&F that this cannot occur, so checking that locationId is not nil on the Backing is sufficient. Good catch!

dusi

Had a first run through this and mostly no red flags..

A11y

Do we want the space here? I believe our formatter returns empty nbsp space at the beginning of the string which is fairly visible when the stack view is aligned vertically.

Changing font size crashes the screen (even after adding the following workaround)...wonder what's the real problem here.

DispatchQueue.main.async {
  self.viewModel.inputs.traitCollectionDidChange()
}

dusi · 2019-10-11T21:49:02Z

Kickstarter-iOS/Views/Controllers/PledgeAmountSummaryViewController.swift

+
+    _ = self.pledgeAmountStackView
+      |> checkoutAdaptableStackViewStyle(
+        self.traitCollection.preferredContentSizeCategory.isAccessibilityCategory


Can we declare local variable and reuse it (since we're using it twice here)?

let isAccessibilityCategory = self.traitCollection.preferredContentSizeCategory.isAccessibilityCategory ... checkoutAdaptableStackViewStyle(isAccessibilityCategory)

dusi · 2019-10-11T21:56:36Z

Kickstarter-iOS/Views/Controllers/PledgeAmountSummaryViewController.swift

+
+private let amountLabelStyle: LabelStyle = { label in
+  label
+    |> \.adjustsFontForContentSizeCategory .~ true


Do we also need to set font? 🤔

pledgeAmountLabel uses an attributed string so the font setting isn't necessary here.

dusi · 2019-10-11T21:58:30Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

@@ -182,20 +153,12 @@ final class PledgePaymentMethodsViewController: UIViewController {
    self.viewModel.inputs.configureWith(pledgePaymentMethodsValue)
  }

-  // MARK: - Accessors
+  // MARK: - Actions


dusi · 2019-10-11T22:00:01Z

Kickstarter-iOS/Views/Controllers/PledgeViewController.swift

@@ -11,10 +11,9 @@ protocol PledgeViewControllerDelegate: AnyObject {
 final class PledgeViewController: UIViewController, MessageBannerViewControllerPresenting {
  // MARK: - Properties

-  private lazy var confirmButton: UIButton = { UIButton(type: .custom) }()


Yeah this will really mess up my loading button but it should be fairly simple to fix

dusi · 2019-10-11T22:00:33Z

Kickstarter-iOS/Views/Controllers/PledgeViewController.swift

@@ -75,8 +78,14 @@ final class PledgeViewController: UIViewController, MessageBannerViewControllerP
      |> \.delegate .~ self
  }()

+  private lazy var submitButton: UIButton = { UIButton(type: .custom) }()


Is init with type the designated initializer?

Yeah we want this to be a custom button to not have the highlight animation that system buttons get.

dusi · 2019-10-11T22:16:59Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

@@ -40,7 +36,8 @@ public final class PledgePaymentMethodsViewModel: PledgePaymentMethodsViewModelT
    let configureWithValue = Signal.combineLatest(
      self.viewDidLoadProperty.signal,
      self.configureWithValueProperty.signal.skipNil()
-    ).map(second)
+    )
+    .map(second)


https://www.youtube.com/watch?v=SsoOG6ZeyUI

😂

dusi

Noticed some minor thing

and also this issue - is the total value calculated properly for the first couple Snapshot tests? testView_ChangePaymentMethodContext?

dusi · 2019-10-15T18:50:55Z

Kickstarter-iOS/Views/Controllers/PledgeAmountSummaryViewController.swift

+    super.viewDidLoad()
+
+    self.configureSubviews()
+    self.bindViewModel()


Isn't this happening through our swizzle thingys? 🤔

Indeed, thanks.

justinswart · 2019-10-16T02:20:56Z

@dusi Good catch on the snapshots - This is because the template Reward's ShippingRule had an amount of 5 but the Backing on the Project had a shipping amount of 10. I think this is ok because in reality when we're changing the payment method these things should line up, but let's test this logic a bit more to be sure.

dusi

I can approve this if @ifbarrera is fine with this!

🚢 🚢 🚢
🚢 🚢 🚢
🚢 🚢 🚢

* Wire up change payment method to mutation for payment source and Apple Pay * Remove payment method when backing with Apple Pay and vice versa * Update tests for apple pay sheet race condition fixes * Replace zip with simpler signals * Add Stripe token error test * Move signals around, improve tests

justinswart added 6 commits October 8, 2019 17:45

Updated validation

3a5e5bd

Fix snapshot tests, add submitButtonHidden

fffa982

Add PledgeAmountSummaryViewController

97f4cd8

Merge branch 'master' into change-payment-method-ui

02f290e

Add updateButtonTapped

02bebe8

justinswart requested review from dusi and ifbarrera October 10, 2019 02:23

justinswart assigned dusi Oct 10, 2019

justinswart commented Oct 10, 2019

View reviewed changes

Add snapshot test and vm test

82b6642

ksr-ci-bot reviewed Oct 10, 2019

View reviewed changes

Kickstarter-iOS/Views/Controllers/PledgeViewControllerTests.swift Outdated Show resolved Hide resolved

justinswart added 5 commits October 10, 2019 10:15

lol

a4b8ffd

Merge branch 'master' into change-payment-method-ui

e70e2e6

Remove unused var

5cbf40b

Rename signal, fix payment source ID comparison

afa9c9a

Fix tests

e2b3afd

justinswart mentioned this pull request Oct 10, 2019

[NT-358] Refactor ManagePledgeSummaryView #890

Merged

1 task

Merge branch 'master' into change-payment-method-ui

1d69e99

ifbarrera reviewed Oct 11, 2019

View reviewed changes

Fix tests, simplify hiding of shipping location stackview

c189ebe

dusi reviewed Oct 11, 2019

View reviewed changes

This was referenced Oct 11, 2019

[NT-359] Integrate Updating Backing mutation #893

Merged

[NT-363] Select a new reward #894

Merged

Merge branch 'master' into change-payment-method-ui

5a03263

dusi reviewed Oct 15, 2019

View reviewed changes

justinswart added 2 commits October 15, 2019 12:52

Merge branch 'master' into change-payment-method-ui

3a6ae51

Fix snapshots

4c49d3a

justinswart added 2 commits October 15, 2019 19:22

Commit the actual snapshots

0ed69e0

Use local var

b46ce84

dusi approved these changes Oct 16, 2019

View reviewed changes

justinswart merged commit 7de27fa into master Oct 16, 2019

justinswart deleted the change-payment-method-ui branch July 14, 2021 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NT-357][NT-358] Change Payment Method UI #887

[NT-357][NT-358] Change Payment Method UI #887

justinswart commented Oct 10, 2019

justinswart Oct 10, 2019

justinswart Oct 10, 2019

justinswart Oct 10, 2019

justinswart Oct 10, 2019

justinswart Oct 10, 2019

justinswart Oct 10, 2019

ksr-ci-bot commented Oct 10, 2019

ifbarrera Oct 11, 2019

ifbarrera Oct 11, 2019

justinswart Oct 11, 2019

ifbarrera Oct 11, 2019

justinswart Oct 11, 2019

ifbarrera Oct 11, 2019

justinswart Oct 11, 2019

ifbarrera Oct 11, 2019

justinswart Oct 11, 2019

justinswart Oct 11, 2019

dusi left a comment

dusi Oct 11, 2019

dusi Oct 11, 2019

justinswart Oct 16, 2019

dusi Oct 11, 2019

dusi Oct 11, 2019

dusi Oct 11, 2019

justinswart Oct 16, 2019

dusi Oct 11, 2019

dusi left a comment

dusi Oct 15, 2019

justinswart Oct 16, 2019

justinswart commented Oct 16, 2019

dusi left a comment

[NT-357][NT-358] Change Payment Method UI #887

[NT-357][NT-358] Change Payment Method UI #887

Conversation

justinswart commented Oct 10, 2019

📲 What

🤔 Why

🛠 How

👀 See

✅ Acceptance criteria

⏰ TODO

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

ksr-ci-bot commented Oct 10, 2019

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

dusi left a comment

Choose a reason for hiding this comment

A11y

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

dusi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinswart commented Oct 16, 2019

dusi left a comment

Choose a reason for hiding this comment