[Payment methods] Refresh payment methods table view properly #588

dusi · 2019-02-13T00:30:06Z

📲 What

Refreshes payment methods properly

🤔 Why

Previously if you have added a new card on iPad, the payment methods table would not refresh due to the way we present the modal. It relies on UIViewController lifecycle methods that are called differently when we present things using .formSheet modal presentation style.

🛠 How

Causes the view model to re-fetch on "Add New Card" modal success or dismissal.

✅ Acceptance criteria

on iPad..

Payment methods screen properly refreshes when first navigating to the screen
Payment methods screen properly refreshes when dismissing the Add New Card modal (add or remove payment methods on the web after the modal has been presented and before dismissing)
Payment methods screen properly refreshes when submitting the Add New Card modal (no need to do anything as this will by itself add a new payment method)

dusi · 2019-02-13T00:30:30Z

Kickstarter-iOS/Views/Controllers/AddNewCardViewController.swift

@@ -6,7 +6,8 @@ import Stripe
 import UIKit

 internal protocol AddNewCardViewControllerDelegate: class {
-  func presentAddCardSuccessfulBanner(_ message: String)
+  func addNewCardViewControllerSucceeded(with message: String)


I'm happy to discuss the naming on these

A bit more verbose but I think a good convention is to always pass in the thing that is calling the delegate, even if in this case we don't care about it - this is just a practice we started using at some point, also described here: https://google.github.io/swift/#delegate-methods

func addNewCardViewController(_ viewController: AddNewCardViewController, didSucceedWithMessage message: String) func addNewCardViewControllerDismissed(_ viewController: AddNewCardViewController)

This would be more about establishing a convention in this case as we're not using the source of the delegate.

Sounds good. At some point I was passing vc back but then decided not to for obvious reasons...I'll change this back.

dusi · 2019-02-13T00:31:12Z

Kickstarter-iOS/Views/Controllers/AddNewCardViewController.swift

-    self.navigationController?.dismiss(animated: true, completion: { [weak self] in
-      self?.delegate?.presentAddCardSuccessfulBanner(message)
-    })
+    self.delegate?.addNewCardViewControllerSucceeded(with: message)


Delegate should be responsible for the rest (no need to dismiss anything from here as it adds more responsibility to the controller which it shouldn't be aware of)

dusi · 2019-02-13T00:31:33Z

Kickstarter-iOS/Views/Controllers/PaymentMethodsViewController.swift

-    self.viewModel.inputs.cardAddedSuccessfully(message)
+  func addNewCardViewControllerSucceeded(with message: String) {
+    self.dismiss(animated: true) { [weak self] in
+      guard let self = self else { return }


guard might not be needed, we can just optionally chain here...either should be fine

Hmm I would even consider testing this without the weak reference as I'm not sure this kind of closure would cause a retain cycle 🤔 but I guess it's good practice to just always do it.

You're right! We're not holding a reference to this block so there should not be a retain cycle.

dusi · 2019-02-13T00:31:57Z

Library/ViewModels/PaymentMethodsViewModelTests.swift

-    let response = UserEnvelope<GraphUserCreditCard>(
-      me: GraphUserCreditCard.template
-    )
+    let response = UserEnvelope<GraphUserCreditCard>(me: GraphUserCreditCard.template)


1 liner for consistency

dusi · 2019-02-13T00:32:29Z

Payment Methods 🐛 - Refresh payment methods properly on iPad when Add payment method is dismissed (cancel or success)

justinswart

lgtm!

ifbarrera · 2019-02-13T15:33:57Z

Kickstarter-iOS/Views/Controllers/PaymentMethodsViewController.swift

+    }
+  }
+
+  func addNewCardViewControllerDismissed(_ viewController: AddNewCardViewController) {


Just wondering, why do we need to refresh the table view when the "Add Card" screen is dismissed but no new card has been added? Would it not be better to only refresh the table view on on 1) viewDidLoad 2) when a new card has been added (via the cardAddedSuccessfully input).

My reasoning was that since we don't have pull to refresh or any other technique to refresh to payment methods screen (other than presenting it)...it's more useful for now to refresh both when dismissing and submitting the modal. This will for now help in situations where you'd modified your cards (on the web / other mobile client) while the modal was on and then dismissed the screen.

* New card screen ui (#471) * add card view controller * transition to add new card and uibarbuttons * settings style applied to add new card screen * changed keyboard style for textfields * view controller tests * placeholder for cardholder name textfield * Record new screenshots * new strings, added strings as place holder and text label * pr feedback - inferring type for cancel button tintcolor * pr feedback - keypath, no more lenses * swiftlint fix * new snapshots * Payment methods (#457) * Stripe element add new card (#473) * New card screen ui (#471) * add card view controller * transition to add new card and uibarbuttons * settings style applied to add new card screen * changed keyboard style for textfields * view controller tests * placeholder for cardholder name textfield * Record new screenshots * new strings, added strings as place holder and text label * pr feedback - inferring type for cancel button tintcolor * pr feedback - keypath, no more lenses * swiftlint fix * new snapshots * new stppaymentcardtextfield showing * wip - placeholder colors * snapshot tests * Email undeliverable/unverified (#478) * Adding emailIsVerified * Renaming GraphUserEmail * ViewModel tests * ChangeEmailViewController tests * Strings, swiftlint * Strings & new screenshots * Cleanup * PR comments * PR updates * Test naming * pr feedback * new snapshots * new placeholder colors * new snapshots * Delete payment methods (#479) * Add payment method deletion * Test datasource * Test view model * Clean-up and address PR feedback * Rename credit card -> payment method * Fix test * Add credit card implementation (#503) * Email undeliverable/unverified (#478) * Adding emailIsVerified * Renaming GraphUserEmail * ViewModel tests * ChangeEmailViewController tests * Strings, swiftlint * Strings & new screenshots * Cleanup * PR comments * PR updates * Test naming * Use correct function to log events in crash logs (#481) * Use correct function to log events in crash logs * Fix indentation * changed ui colors for textfield text and font size of text label * vm and work on enabling save * wip- get stripe token * wip- payment source mutation * wip- keyboard response * wip- getting a stripe error here * wip- error fix w/ publishable key, error banner showing * wip- saving with error and keyboard functionality * wip- updating card immediately * wip * wip - ACs met * wip - refactor in view model, made IDs testable, begane VM tests * wip -refactor on vm/vmtest * wip -refactor deleted comments on vmt * wip -snapshot tests * swiftlint fixes * renaming/refactor, corrected paymentmethodstests * pr feedback * swiftlint fixes * changed function name * indentation * Payment methods event tracking (#496) * Reverted code that deleted SettingsNewsletters from Storyboard.swift * swiftlint * Reverted code to instantiate settings newsletters vc on tests * Alphabetized storyboard enum * Settings payments colors (#530) * Unsupported Credit Cards (#561) * Viewmodel logic for unsupported cards * Tests and accessibility fixes * Simplify add card button * Screenshots and string updates * Cleanup design and remove logs * Improving comment message * Removing filter debug builds * Addressing PR comments * Updating payment methods screenshots * Screenshots * Possible fix for alignment issues * Moving iOS 10 handling to output signal closure * Remove line * Renaming * Last cleanup * Strings & Asset update * Switching from unsupported to supported * Zipcode field in Add New Card screen (#566) * Viewmodel logic for unsupported cards * Tests and accessibility fixes * Simplify add card button * Screenshots and string updates * Cleanup design and remove logs * Improving comment message * Removing filter debug builds * Addressing PR comments * Updating payment methods screenshots * Screenshots * Possible fix for alignment issues * Moving iOS 10 handling to output signal closure * Remove line * Renaming * Last cleanup * Shared styled form field * Adding functionality for zipcode form field * More functionality * View model & screenshot tests * Swiftlint * Cleanup * Improving test and cleanup * PR feedback and autocapitalizing zipcode * Regenerating ChangePassword screenshots * [Payment methods] CVC bug fix (#574) * wip * wip * wip * wip-swiftlint * moved CreatePaymentSourceEnvelope to its own file, Mockservice * swiftlint fix * deleted unnecesary debugging code * new paymentsource temploate and vm tests * swiftlint fix * fixed vm test * PR feedback * update schema * [Payment methods] Minor bug fixes (#579) * tableViewIsEditing false and show banner after dismiss * Swiftlint * Adding StripePublishableKey to example file * Update publishableKey test * Spacing * Point-free helper * [Payment Methods] Update padding and image view size (#581) * Update padding and image view size * Update stack view's spacing * Update snapshots * Use layoutMargins to set the padding * [Payment methods] Disable edit button if no payment methods (#586) * [Payment methods] Bugs & visual fixes (#578) * Adding custom UITableViewHeader and always calling reload data on viewDidLoad * Updating screenshots * Cleanup autolayout warnings * Screenshots * PR comments * No autolayout for footer view * Cleanup * Remove intrinsicContentSize * Tableview appears without delay fix (#587) * Fix header/footer auto sizing * Update header background color * Update snapshots * Screenshots again * Updating constraint priority to resolve ambiguity * [Payment methods] Refresh payment methods table view properly (#588) * Refresh payment methods table view properly * Remove reference to weak self * Rename delegate methods * [Payment methods] Refetch payment methods on viewDidLoad or explicitly (by delegate) (#591) * Do not refresh on viewWillAppear * Use optional string to avoid unnecessary initializer * Bring back viewWillAppear signal to better reflect view controller's lifecycle state * [Payment methods] A11y - Credit card name (#593) * Add card name to the a11y label * Add comment * [Payment Methods] Design Fixes (#592) * Design fixes * Fresh screenshots ✨ * Remove recordMode * Deleting stale screenshots * Re-add accidentally deleted line * update strings * [Payment Methods] Support optional card type and error handling for payment methods (#596) * Optional card type, payment methods load error * Optional support for imageName * Fix accessibility label * Fix merge conflict spacing * Swiftlint * Implementing PR suggestions * Fixed bug that shows wrong expiration date (#595) * [Payment Methods] Add New Card Design Fixes (#594) * error message present and return key to done * removed error message banner for incomplete payment details * new snapshots for correct insets * [Payment methods] Optimistically disable edit button on card deletion (#589) * Disable edit button optimistically, use stored cards in result from card deletion * Fix issue where edit button is not re-enabled upon the failed deletion of the last card. * Remove unnecessary .init * Add skipRepeats() to editButtonIsEnabled signal. * Fixing potential reference cycles (#598)

dusi assigned justinswart Feb 13, 2019

dusi requested a review from ifbarrera February 13, 2019 00:30

dusi commented Feb 13, 2019

View reviewed changes

dusi changed the title ~~[Payment Methods] Refresh payment methods table view properly~~ [Payment methods] Refresh payment methods table view properly Feb 13, 2019

dusi added the needs review label Feb 13, 2019

Refresh payment methods table view properly

162d916

dusi force-pushed the feature-payment-methods-fix-refresh-on-ipad branch from 873ba0b to 162d916 Compare February 13, 2019 00:38

dusi added 2 commits February 12, 2019 17:23

Remove reference to weak self

27392c3

Rename delegate methods

fc03051

justinswart approved these changes Feb 13, 2019

View reviewed changes

ifbarrera reviewed Feb 13, 2019

View reviewed changes

dusi merged commit e380302 into feature-payment-methods Feb 13, 2019

dusi deleted the feature-payment-methods-fix-refresh-on-ipad branch February 13, 2019 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Payment methods] Refresh payment methods table view properly #588

[Payment methods] Refresh payment methods table view properly #588

dusi commented Feb 13, 2019 •

edited by justinswart

Loading

dusi Feb 13, 2019

justinswart Feb 13, 2019

dusi Feb 13, 2019

dusi Feb 13, 2019

dusi Feb 13, 2019

justinswart Feb 13, 2019

dusi Feb 13, 2019

dusi Feb 13, 2019 •

edited

Loading

dusi commented Feb 13, 2019

justinswart left a comment

ifbarrera Feb 13, 2019

dusi Feb 13, 2019

[Payment methods] Refresh payment methods table view properly #588

[Payment methods] Refresh payment methods table view properly #588

Conversation

dusi commented Feb 13, 2019 • edited by justinswart Loading

📲 What

🤔 Why

🛠 How

✅ Acceptance criteria

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 Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

dusi commented Feb 13, 2019

justinswart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusi commented Feb 13, 2019 •

edited by justinswart

Loading

dusi Feb 13, 2019 •

edited

Loading