[NT-364]Unsupported cards carousel #897

cdolm92 · 2019-10-16T21:02:55Z

📲 What

We have disabled the select button, changed the button title to No longer available and provided copy below the unavailable credit card so that users understand that they cannot pledge with an unavailable card type.

🤔 Why

Users want to clearly and easily see which of their saved payment methods may not be used to back a particular project in the Pledge screen so that they know to select or add a different payment method to complete their pledge.

🛠 How

We determine unavailable card types by checking if the user card is included in project.availableCardTypes.

👀 See

Trello, screenshots, external resources?

Before 🐛	After 🦋

✅ Acceptance criteria

Navigate to a non-USD project. Scroll through the available payment options, all unacceptable payment options should have a disabled button with the title text that reads No longer available
Copy below the Cardview should read You can’t use this credit card to back a project from [project location]. Note: this will change to project country in an upcoming PR

✍🏾 TO DO

Snapshot testing for cards

…unsupported-cards-carousel

ifbarrera

There's a lot of logic happening in the PledgePaymentMethodsViewController which should probably be in a view model so it can be tested. I'm wondering if there's a reason we didn't just add the "card unavailable" label to the PledgeCreditCardView and configured the PledgeCreditCardViewModel with a boolean that lets it determine whether or not it should be an unavailable view. This would allow us to better test the logic around disabling unavailable card types, and might mitigate some of the complexity that doing this in the view controller is causing.

I also noticed a couple of bugs that might have to do with the way we're adding the card views:

After adding a new card, it looks like there's a duplicate set of unavailable card views in the list.

Also, the unavailable card label doesn't seem to be responding to dynamic type:

ifbarrera · 2019-10-17T21:31:47Z

Kickstarter-iOS/Views/Cells/PledgeCreditCardView.swift

@@ -169,3 +174,16 @@ private let labelsStackViewStyle: StackViewStyle = { stackView in
    |> \.axis .~ .vertical
    |> \.spacing .~ Styles.gridHalf(1)
 }
+
+private let selectButtonStyle: ButtonStyle = { button in


Why is it necessary to create a custom style for this button? The blackButtonStyle should have a .disabled style, in which case we should be able to just set the button to disabled to get the correct treatment 🤔

ifbarrera · 2019-10-17T21:33:43Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

@@ -21,6 +21,8 @@ final class PledgePaymentMethodsViewController: UIViewController {
  private lazy var cardsStackView: UIStackView = { UIStackView(frame: .zero) }()
  internal weak var delegate: PledgePaymentMethodsViewControllerDelegate?
  internal weak var messageDisplayingDelegate: PledgeViewControllerMessageDisplaying?
+  private lazy var pledgeButton: UIButton = { UIButton.init(type: .custom) }()


Did this get accidentally re-added as part of a merge? I believe @justinswart refactored this button into the main PledgeViewController

ifbarrera · 2019-10-17T21:34:19Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

@@ -98,10 +104,10 @@ final class PledgePaymentMethodsViewController: UIViewController {

    self.viewModel.outputs.reloadPaymentMethods
      .observeForUI()
-      .observeValues { [weak self] cards in
+      .observeValues { [weak self] cardValues in


Any reason for this rename?

Oh I see below the naming has changed.

ifbarrera · 2019-10-17T21:37:18Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

      cardView.configureWith(value: card)

+      guard let cardBrand = card.type?.rawValue else { return self.pledgeCardStackView }
+      let isAvailableCardType = availableCardTypes.contains(cardBrand)


Hmm I feel like this logic should probably live in the view model.

ifbarrera · 2019-10-17T21:46:01Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

+      |> \.axis .~ .vertical
+      |> \.spacing .~ Styles.grid(2)
+
+    let label = UILabel(frame: .zero)


I wonder if we put this label within the PledgeCreditCardView itself, it might be easier to manage it's hide/shown state from PledgeCreditCardViewModel.

ifbarrera · 2019-10-17T21:48:01Z

Library/ViewModels/PledgeCreditCardViewModel.swift

@@ -25,6 +28,9 @@ public protocol PledgeCreditCardViewModelOutputs {
  /// Emits a formatted string containing the card's last four digits with the format: Ending in 8844.
  var cardNumberTextShortStyle: Signal<String, Never> { get }

+  /// Emits whether or not the button is enabled.
+  var disableButton: Signal<Bool, Never> { get }


Usually we try to match the output name with the property being modified: so here it would be var selectButtonEnabled: Signal<Bool, Never> so that in the rac binding it would be:

self.selectButton.rac.enabled = self.viewModel.outputs.selectButtonEnabled

ifbarrera · 2019-10-17T21:52:23Z

Library/ViewModels/PledgeCreditCardViewModel.swift

@@ -95,6 +103,11 @@ public final class PledgeCreditCardViewModel: PledgeCreditCardViewModelInputs,
    self.selectedCardProperty.value = creditCard
  }

+  private let disabledCardProperty = MutableProperty<Bool?>(nil)
+  public func setDisabledCard(_ disabled: Bool) {


I would if we could have just wrapped up the card's disabled state as part of the configure function:

typealias PledgeCreditCardValue = (card: GraphUserCreditCard.CreditCard, isEnabled: Bool) fileprivate let creditCardProperty = MutableProperty<GraphUserCreditCard.CreditCard?>(nil) public func configureWith(value: PledgeCreditCardValue) { self.creditCardProperty.value = value }

Then every card would be configured with whether or not it's enabled and we wouldn't need to do all that logic in the PledgePaymentMethodsViewController.

ifbarrera · 2019-10-21T19:32:16Z

Library/ViewModels/PledgeCreditCardViewModel.swift

@@ -4,9 +4,14 @@ import ReactiveExtensions
 import ReactiveSwift
 import UIKit

+public typealias PledgeCreditCardValue = (
+  card: GraphUserCreditCard.CreditCard,
+  isEnabled: Bool, projectCountry: String?


projectCountry probably doesn't have to be optional.

@cdolm92 does this have to be optional?

ifbarrera · 2019-10-21T19:33:30Z

Library/ViewModels/PledgeCreditCardViewModel.swift

  }

-  fileprivate let creditCardProperty = MutableProperty<GraphUserCreditCard.CreditCard?>(nil)
-  public func configureWith(value: GraphUserCreditCard.CreditCard) {
+  fileprivate let creditCardProperty = MutableProperty<PledgeCreditCardValue?>(nil)


Can we update the name of this property to match the fact that we now configure with PledgeCreditCardValue? Maybe pledgeCreditCardValueProperty?

ifbarrera · 2019-10-21T19:46:09Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

+      project.map { $0.location.displayableName }
+    ).map { $0 as CardViewValues }
+
+    self.reloadPaymentMethods = cardValues


Here is where we could move even more logic out of the view controller – something we could do is instead of returning CardViewValues, return an array of PledgeCreditCardValue:

self.reloadPaymentMethods = Signal.combineLatest( cards, availableCardTypes, project.map { $0.location.displayableName } ).map { cards, availableCardTypes, projectLocation in return cards.map { card in let isEnabled = availableCardTypes.contains(card.type.rawValue) return (card, isEnabled, projectLocation) } }

Then we can test that given a list of stored cards, and a list of availableCardTypes in a project, that the output is exactly what we're feeding into each PledgeCreditCardViewModel to configure the card view.

…unsupported-cards-carousel

ksr-ci-bot · 2019-10-22T14:48:38Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

ksr-ci-bot · 2019-10-22T14:48:37Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

@@ -5,6 +5,10 @@ import ReactiveSwift
 import UIKit

 public typealias PledgePaymentMethodsValue = (user: User, project: Project, applePayCapable: Bool)
+public typealias CardViewValues = (
+  cardAndIsAvailableCardType: [(cards: GraphUserCreditCard.CreditCard, cardTypeIsAvailable: Bool)],


🚫 Line should be 110 characters or less: currently 122 characters
line_length PledgePaymentMethodsViewModel.swift:9

ifbarrera · 2019-10-22T19:59:42Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift


      if let selectedCard = selectedCard {
-        cardView.setSelectedCard(selectedCard)
+        if isAvailableCardType {


I think we might be able to simplify this logic by doing:

let card = cardAndAvailableType.cards let isAvailableCardType = cardAndAvailableType.cardTypeIsAvailable let projectCountry = cardValues.projectCountry cardView.configureWith(value: (card: card, isEnabled: isAvailableCardType, projectCountry: projectCountry)) if let selectedCard = selectedCard { cardView.setSelectedCard(selectedCard) }

ifbarrera · 2019-10-22T20:01:05Z

Kickstarter-iOS/Views/Controllers/PledgePaymentMethodsViewController.swift

@@ -229,6 +236,14 @@ final class PledgePaymentMethodsViewController: UIViewController {
      |> \.font .~ UIFont.ksr_caption1()
      |> \.textAlignment .~ .center
  }
+
+  private let cardRestrictionLabelStyle: LabelStyle = { label in


Is this still being used in this file?

ifbarrera · 2019-10-22T20:04:35Z

Library/ViewModels/PledgeCreditCardViewModel.swift

@@ -4,9 +4,14 @@ import ReactiveExtensions
 import ReactiveSwift
 import UIKit

+public typealias PledgeCreditCardValue = (
+  card: GraphUserCreditCard.CreditCard,
+  isEnabled: Bool, projectCountry: String?


@cdolm92 does this have to be optional?

ifbarrera · 2019-10-22T20:07:42Z

Library/ViewModels/PledgeCreditCardViewModelTests.swift

@@ -13,8 +13,12 @@ internal final class PledgeCreditCardViewModelTests: TestCase {
  private let cardNumberTextShortStyle = TestObserver<String, Never>()
  private let expirationDateText = TestObserver<String, Never>()
  private let notifyDelegateOfCardSelected = TestObserver<String, Never>()
+  private let selectButtonEnable = TestObserver<Bool, Never>()


Small typo here, should be selectButtonEnabled.

ifbarrera · 2019-10-22T20:08:07Z

Library/ViewModels/PledgeCreditCardViewModelTests.swift

@@ -24,12 +28,16 @@ internal final class PledgeCreditCardViewModelTests: TestCase {
    self.vm.outputs.cardNumberTextShortStyle.observe(self.cardNumberTextShortStyle.observer)
    self.vm.outputs.expirationDateText.observe(self.expirationDateText.observer)
    self.vm.outputs.notifyDelegateOfCardSelected.observe(self.notifyDelegateOfCardSelected.observer)
+    self.vm.outputs.selectButtonEnabled.observe(self.selectButtonEnable.observer) //


There's some trailing slashes here 🤔

oh right, I still need to test those

ifbarrera · 2019-10-22T20:14:58Z

Library/ViewModels/PledgePaymentMethodsViewModel.swift

@@ -5,6 +5,10 @@ import ReactiveSwift
 import UIKit

 public typealias PledgePaymentMethodsValue = (user: User, project: Project, applePayCapable: Bool)
+public typealias CardViewValues = (
+  cardAndIsAvailableCardType: [(cards: GraphUserCreditCard.CreditCard, cardTypeIsAvailable: Bool)],


I wonder if this should be cardAndIsAvailableCardType: (card: GraphUserCreditCard.CreditCard, cardTypeIsAvailable: Bool) (singular card instead of cards).

Library/ViewModels/PledgePaymentMethodsViewModel.swift

Library/ViewModels/PledgePaymentMethodsViewModelTests.swift

ifbarrera · 2019-10-22T21:32:51Z

Library/ViewModels/PledgePaymentMethodsViewModelTests.swift


      self.vm.inputs.configureWith((User.template, Project.template, false))
      self.vm.inputs.viewDidLoad()

      self.scheduler.run()

-      self.reloadPaymentMethods.assertValue(response.me.storedCards.nodes)
+      self.reloadPaymentMethodsCards.assertValue(response.me.storedCards.nodes)
+      self.reloadPaymentMethodsAvailableCardTypes.assertValues([


It's a little difficult to follow these assertions when they're split up. Is it possible to do the assertions on the tuples? Something like this:

self.reloadPaymentMethodsCardsAndAvailable.assertValues([(GraphUserCreditCard.visa, true), (GraphUser CreditCard.masterCard, true)...etc])

…unsupported-cards-carousel

cdolm92 added 16 commits October 9, 2019 18:36

wip- very messy, logic works

c8c154f

wip- refactor

8564a7a

wip

5d1d674

wip- project location dispalyed

31b819e

wip

b24782d

wip

ec73f20

new funcs

016683a

wip- removed nested loop from update func

e15325d

rename

6ab3fc2

rename func

c796f11

swiftlint fix

bfec4fe

tests

5410ccc

swiftlint fix

8924ef8

swiftformat

f29ddfb

changed button style

af277a0

Merge branch 'master' of https://github.com/kickstarter/ios-oss into …

7835841

…unsupported-cards-carousel

cdolm92 changed the title ~~Unsupported cards carousel~~ [NT-364]Unsupported cards carousel Oct 16, 2019

cdolm92 added 2 commits October 16, 2019 17:52

fix tests

b6c5359

new snapshots

a175f9f

cdolm92 requested a review from ifbarrera October 17, 2019 15:33

cdolm92 assigned ifbarrera Oct 17, 2019

Merge branch 'master' of https://github.com/kickstarter/ios-oss into …

156d515

…unsupported-cards-carousel

ifbarrera suggested changes Oct 17, 2019

View reviewed changes

cdolm92 added 2 commits October 21, 2019 13:57

pr feedback and rework

bf32ff0

swiftformat and swiftlint fix

d96191d

ifbarrera reviewed Oct 21, 2019

View reviewed changes

cdolm92 added 4 commits October 21, 2019 18:14

re-wip

3e822ee

re-wip

1d0eb0f

Merge branch 'master' of https://github.com/kickstarter/ios-oss into …

689db15

…unsupported-cards-carousel

swiftformat and swiftlint fix

4d1d841

ksr-ci-bot reviewed Oct 22, 2019

View reviewed changes

cdolm92 added 4 commits October 22, 2019 11:40

vm tests

ed69d9a

snapshots

14eff8d

swiftformat

79152c5

tweaked blackbuttonstyle and new snapshots

8c62405

ifbarrera reviewed Oct 22, 2019

View reviewed changes

cdolm92 added 7 commits October 23, 2019 16:24

feedback

c80ed3e

vm tests

b929568

vm tests

e49413f

big fix

d5045e3

Merge branch 'master' of https://github.com/kickstarter/ios-oss into …

a1f7937

…unsupported-cards-carousel

added test

93108e7

swiftformat

9d4496f

ifbarrera approved these changes Oct 24, 2019

View reviewed changes

cdolm92 merged commit 4c65b33 into master Oct 24, 2019

cdolm92 deleted the unsupported-cards-carousel branch October 24, 2019 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NT-364]Unsupported cards carousel #897

[NT-364]Unsupported cards carousel #897

cdolm92 commented Oct 16, 2019 •

edited

Loading

ifbarrera left a comment

ifbarrera Oct 17, 2019

ifbarrera Oct 17, 2019

ifbarrera Oct 17, 2019

ifbarrera Oct 17, 2019

ifbarrera Oct 17, 2019

ifbarrera Oct 17, 2019

ifbarrera Oct 17, 2019

ifbarrera Oct 17, 2019

ifbarrera Oct 21, 2019

ifbarrera Oct 22, 2019

ifbarrera Oct 21, 2019

ifbarrera Oct 21, 2019

ksr-ci-bot commented Oct 22, 2019 •

edited

Loading

ksr-ci-bot Oct 22, 2019

ifbarrera Oct 22, 2019

ifbarrera Oct 22, 2019

ifbarrera Oct 22, 2019

ifbarrera Oct 22, 2019

ifbarrera Oct 22, 2019

cdolm92 Oct 23, 2019

ifbarrera Oct 22, 2019

ifbarrera Oct 22, 2019

[NT-364]Unsupported cards carousel #897

[NT-364]Unsupported cards carousel #897

Conversation

cdolm92 commented Oct 16, 2019 • edited Loading

📲 What

🤔 Why

🛠 How

👀 See

✅ Acceptance criteria

✍🏾 TO DO

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

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

ksr-ci-bot commented Oct 22, 2019 • 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

cdolm92 commented Oct 16, 2019 •

edited

Loading

ksr-ci-bot commented Oct 22, 2019 •

edited

Loading