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

[NT-262] Remember this card added to mutation #856

Merged
merged 10 commits into from
Sep 30, 2019

Conversation

justinswart
Copy link
Contributor

@justinswart justinswart commented Sep 26, 2019

📲 What

  • Fixes a bug from [ 💳 Native Checkout ] Refresh Card List #835 in that the delegate was not notified of the initially selected card.
  • Adds the reusable boolean to CreatePaymentSourceInput so that the payment source can be stored for reuse.

🤔 Why

  • An oversight in the [ 💳 Native Checkout ] Refresh Card List #835 meant that the delegate would not be informed of the initially selected card when the payment methods were reloaded.
  • We provide the option during checkout for cards to be single-use or stored for reuse.

🛠 How

  • Modified the signals so that the delegate would be notified.
  • Added the reusable boolean to the CreatePaymentSourceInput mutation.

✅ Acceptance criteria

With native pledge view enabled, Tap on add new card from PledgeViewController.

  • Remember this card toggle is shown and disabled by default.
  • Add a card and observe in the console that the mutation's input contains reusable: false.
  • Rinse and repeat with the toggle turned on, observe in the console that the mutation's input contains reusable: true.

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

I noticed during testing that by making the reusable field default to false, it seems to have broken adding a new card from settings, because although adding the card succeeds, the card isn't shown in the list of stored cards.

I also noticed while looking at the checked out code that the AddNewCardViewControllerDelegate seems to have two functions that are basically the same:

  func addNewCardViewController(
    _ viewController: AddNewCardViewController,
    didSucceedWithMessage message: String
  )
  func addNewCardViewController(
    _ viewController: AddNewCardViewController,
    _ newCard: GraphUserCreditCard.CreditCard
  )

Both of these seem to trigger when a new card is added, would it make sense to just combine them into one delegate function?


// MARK: - Functions

private func createPaymentSourceInput(
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been putting these helper functions into their own files as CreatePaymentSourceInput+Constructor - that way they can be tested separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah cool, thanks!

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

Lgtm! Just one small suggestion.

}

self.newCardAddedWithMessage = addNewCardEvent
.map { $0.value?.paymentSource }
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also use values() which does the same thing as you're doing here (grabbing the value and doing skipNil()):

addNewCardEvent
.values()
.map { $0.paymentSource }
.map { card in (card, Strings.Got_it) }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yeah seems it was that way, I just moved it around a little.

Thanks have updated!

@justinswart justinswart merged commit 39dc6dd into master Sep 30, 2019
@justinswart justinswart deleted the remember-card-mutation branch September 30, 2019 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants