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

[Payment methods] Optimistically disable edit button on card deletion #589

Merged

Conversation

justinswart
Copy link
Contributor

@justinswart justinswart commented Feb 13, 2019

📲 What

Disables the edit button optimistically once we've deleted our last payment method, before the request has completed.

🤔 Why

We've used an optimistic UI approach for the rest of the behaviour on deleting payment methods and we'd like to use it for the disabling of the edit button too.

🛠 How

  • Keeping track of cards as we delete them using scan.
  • Filtering the users stored cards at the time of reload against the deleted cards in memory.
  • Once the count of the filtered cards reaches zero, we disabled editing.
  • If any error or success occurs we re-enable/keep the button disabled based on the new array of stored cards.

✅ Acceptance criteria

  • Use link conditioner to slow down your connection, delete all your cards, the button should be disabled immediately as the last card is deleted.
  • If the call errors, the cards should reappear and the button should be re-enabled.
  • Navigating to/from this view should cause the cards to refresh.

@@ -67,9 +67,30 @@ PaymentMethodsViewModelInputs, PaymentMethodsViewModelOutputs {
paymentMethodsValues.takeWhen(deletePaymentMethodEventsErrors)
)

let cardsDeletedSinceReload = Signal.merge(
self.didDeleteCreditCardSignal.map { card -> GraphUserCreditCard.CreditCard in card },
Copy link
Contributor Author

@justinswart justinswart Feb 13, 2019

Choose a reason for hiding this comment

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

I had to help the compiler along with the types here, it wasn't able to infer that this can sometimes be a GraphUserCreditCard.CreditCard and sometimes nil. We use nil just to reset the accumulator in the scan.

@justinswart
Copy link
Contributor Author

@Scollaco
Copy link
Contributor

Scollaco commented Feb 13, 2019

I tested using my device on flight mode, but the button doesn't re-enable if there's an error deleting the last card:

@justinswart
Copy link
Contributor Author

@Scollaco oh thanks i'll check that out 👍

self.scheduler.advance()

self.editButtonIsEnabled.assertValues(
[true, false, true], "Editing mode enabled, delete response has count of 2"
Copy link
Contributor

Choose a reason for hiding this comment

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

So in this scenario although all cards were initially deleted, a few calls "failed" and thats why the edit button is re-enabled, correct? How is that different from the test below?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test where the button stays disabled when all cards are successfully deleted?

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.

One comment on the tests, otherwise looks good!

Tested the following scenarios:
✅ delete all cards in quick succession -> Edit button disables as soon as last card is deleted
✅ at 100% network loss, delete the last card -> Edit button immediately disables but is re-enabled once the call times out and the table view refreshes

self.scheduler.advance()

self.editButtonIsEnabled.assertValues(
[true, false, true], "Editing mode enabled, delete response has count of 2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test where the button stays disabled when all cards are successfully deleted?

@justinswart justinswart added the blocked a PR that is blocked for external reasons label Feb 14, 2019
@justinswart justinswart force-pushed the feature-payment-methods-optimistic-delete branch from 4891197 to 4763d93 Compare February 14, 2019 19:13
@justinswart justinswart removed blocked a PR that is blocked for external reasons labels Feb 14, 2019
@justinswart
Copy link
Contributor Author

I've pushed a new solution where:

  • We pass in the visible cell count for the optimistic button enabling/disabling.
  • We use the stored cards in the result from deletion so that if a deletion succeeds and a subsequent one fails, we don't show the cards from the initial load on view.

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.

There's one small edge case that causes the Edit button to have to be tapped twice before putting the table view in edit mode. We can address this in a separate PR.

:shipit:

@justinswart justinswart merged commit e659e15 into feature-payment-methods Feb 15, 2019
@justinswart justinswart deleted the feature-payment-methods-optimistic-delete branch February 15, 2019 18:46
justinswart added a commit that referenced this pull request Feb 15, 2019
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants