Reward container error handling #796

Scollaco · 2019-08-13T14:30:35Z

📲 What

Adds an Error State view to the Rewards Container, allowing the user to tap a button to retry fetching backing infos.

🤔 Why

This is part of the first release of the Native Checkout project.
Trello card: https://trello.com/c/gmkKdvTG/1492-error-handling-for-loading

🛠 How

The fetchProject(projectOrParam:) on ProjectPamphletViewModel now returns SignalProducer<Project, ErrorEnvelope> instead of SignalProducer<Project, Never>. This will tell the callers when an error occurred.
On ProjectPamphletViewModel.swift, the configurePledgeCTAView output now emits Signal<(Either<Project, ErrorEnvelope>, Bool), Never>
The PledgeCTAContainerViewViewModel receives the (Either<Project, ErrorEnvelope>, Bool) as an input and will take care of the UI logic based on what's sent (.left(project) or .right(error)).

👀 See

Before 🐛	After 🦋

♿️ Accessibility

Tap targets use minimum of 44x44 pts dimensions
Works with VoiceOver
Supports Dynamic Type

✅ Acceptance criteria

From Discovery, turn on Flight Mode on your device and tap on any project
The Activity Indicator should be visible
Once the server returns an error, the Activity Indicator should disappear and the Retry button should be visible.
Turn off flight mode and tap the retry button. The Back this Project button should be visible and the retry Button should be hidden.

* 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)

…ope instead of Never

…e> type

ksr-ci-bot · 2019-08-13T14:37:49Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

Scollaco · 2019-08-13T15:51:46Z

Error Handling for loading

Scollaco · 2019-08-13T19:05:17Z

Kickstarter-iOS/Locales/de.lproj/Localizable.strings

@@ -71,11 +71,13 @@
 "Closes_project" = "Schließt das Projekt.";
 "Collapses_subcategories" = "Reduziert die Anzeige der Unterkategorien.";
 "Collections" = "Sammlungen";
-"Comment_reply_digest" = "Comment reply digest";
+"Comment_reply_digest" = "Tägliche Zusammenfassung Kommentare";


The Comment_reply_digest was added while running the git add --patch command for being part of the same 'hunk' of the string we want. Will try to revert it using the git gui

justinswart · 2019-08-14T20:44:16Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

@@ -68,6 +79,10 @@ final class PledgeCTAContainerView: UIView {

    let isAccessibilityCategory = self.traitCollection.preferredContentSizeCategory.isAccessibilityCategory

+    _ = self.pledgeRetryButton
+      |> pledgeRetryButtonStyle
+      |> \.isHidden .~ true


I think let's only manage the hidden state using the view model output.

justinswart · 2019-08-14T20:47:08Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

@@ -96,7 +111,8 @@ final class PledgeCTAContainerView: UIView {

    _ = self.activityIndicator
      |> \.color .~ UIColor.ksr_dark_grey_500
-      |> \.hidesWhenStopped .~ true
+
+    self.activityIndicator.startAnimating()


Let's move this to the lazy var initializer.

justinswart · 2019-08-14T20:49:07Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift


-    // NB: vm needs to return button style


😄 this hasn't been done yet.

justinswart · 2019-08-14T20:49:29Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

+    self.viewModel.outputs.activityIndicatorIsHidden
+      .observeForUI()
+      .observeValues { [weak self] isHidden in
+        self?.activityIndicatorContainerView.isHidden = isHidden


Should be able to just use the rac binding here.

Oh yeah, missed that during cleanup.

justinswart · 2019-08-14T20:52:16Z

Library/ViewModels/PledgeCTAContainerViewViewModel.swift

      .skipNil()
      .filter(second >>> isFalse)
-      .map(first)
+      .map { $0.0 }


.map(first) not working here? 🤔

XCode was complaining, but now it seems to be fine 🤦‍♂

justinswart · 2019-08-14T20:54:22Z

Library/ViewModels/PledgeCTAContainerViewViewModel.swift

+      .merge(
+        project.ignoreValues().mapConst(false),
+        projectError.ignoreValues().mapConst(true),
+        self.activityIndicatorIsHidden.filter(isFalse).mapConst(true)


Do we need the filter here? or could this be .negate() 🤔

We want to use the value of activityIndicatorIsHidden only when it's false, to make sure that the buttons are hidden while the indicator is visible. If activityIndicatorIsHidden returns true, either the Back this Project button or Tap to retry should be visible. Does it make sense?

I see, how about expressing it something like this 🤔 I find the merging a little hard to read.

let inError = Signal.merge( projectOrError.mapConst(false), projectError.ignoreValues().mapConst(true), project.ignoreValues().mapConst(false) ) let updateButtonStates = Signal.merge( projectOrError.ignoreValues(), self.activityIndicatorIsHidden.ignoreValues() ) self.pledgeRetryButtonIsHidden = inError .map(isFalse) .takeWhen(updateButtonStates) self.pledgeCTAButtonIsHidden = inError .map(isTrue) .takeWhen(updateButtonStates)

justinswart · 2019-08-14T20:55:28Z

Library/ViewModels/PledgeCTAContainerViewViewModel.swift

@@ -53,19 +75,21 @@ public final class PledgeCTAContainerViewViewModel: PledgeCTAContainerViewViewMo
      .map(subtitle(project:pledgeState:))
  }

-  fileprivate let configureWithValueProperty = MutableProperty<(project: Project, isLoading: Bool)?>(nil)
-  public func configureWith(value: (project: Project, isLoading: Bool)) {
+  fileprivate let configureWithValueProperty =


I'd go with projectOrErrorProperty.

justinswart · 2019-08-14T20:58:16Z

Library/ViewModels/ProjectPamphletViewModel.swift

@@ -14,6 +14,10 @@ public protocol ProjectPamphletViewModelInputs {
  /// Call after the view loads and passes the initial TopConstraint constant.
  func initial(topConstraint: CGFloat)

+  /// Call when "Content isn't loading right now." is tapped.


The text on the button could change so let's just say Called when pledgeRetryButtonTapped is tapped.

justinswart · 2019-08-14T21:00:21Z

Library/ViewModels/ProjectPamphletViewModelTests.swift

+      self.scheduler.advance()
+
+      XCTAssertTrue(self.configurePledgeCTAViewProject.lastValue?.left == projectFull2)
+      self.configurePledgeCTAViewIsLoading.assertValues([true, true, false, true, true, false])


What do the multiple trues mean? Should we just add a skipRepeats()?

This happens because configurePledgeCTAView combines the latest values of freshProjectAndRefTagEvent and isLoading. So whenever one signal updates, configurePledgeCTAView emits again. In this case, the true right before a false is simply the latest value of isLoading when freshProjectAndRefTagEvent emits a fresh project (or error).

justinswart · 2019-08-15T21:38:02Z

Library/ViewModels/PledgeCTAContainerViewViewModel.swift

+      .map(Either.right)
+      .skipNil()
+
+    self.activityIndicatorIsHidden = self.projectOrErrorProperty.signal


Let's chain this to projectOrError as well.

projectOrError only emits when the project is no longer loading. For a better readability, I will create a property isLoading

let isLoading = self.projectOrErrorProperty.signal .skipNil() .map(second)

justinswart · 2019-08-15T21:39:37Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

@@ -194,3 +229,17 @@ private func pledgeCTAButtonStyle(
      |> (UIButton.lens.titleLabel .. UILabel.lens.lineBreakMode) .~ lineBreakMode
  }
 }
+
+private let pledgeRetryButtonStyle: ButtonStyle = { button in


Think we need to take another look at this button style, the highlight state causes the text to disappear completely.

Also, the image doesn't have a down state, we could give it a tint for that.

We should use the mixDarker and mixLighter extensions for the highlight and disabled states - see ButtonStyles.swift.

Dynamic type is also a little weird with this button:

For the .highlighted state I created an UIImage extension and now we are able to apply alpha to images, also changed the colour of the text for that state to grey (and used mixedLighter).

For the dynamic type I set the correct insets for the text (instead of image) and it seems to solve this issue.

…-handling

justinswart · 2019-08-19T17:16:12Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift


    // NB: vm needs to return button style
+


Please remove the comment above this line, think it was just there as a reminder before.

justinswart · 2019-08-19T17:19:16Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

+private let pledgeRetryButtonStyle: ButtonStyle = { button in
+  button
+    |> baseButtonStyle
+    |> UIButton.lens.titleColor(for: .normal) .~ .ksr_soft_black


Let's make this .ksr_text_black.

justinswart · 2019-08-19T17:19:46Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

+    |> UIButton.lens.titleLabel.font .~ .ksr_caption1()
+    |> UIButton.lens.backgroundColor(for: .normal) .~ .clear
+    |> UIButton.lens.titleEdgeInsets .~ .init(top: 0, left: Styles.grid(3), bottom: 0, right: 0)
+    |> UIButton.lens.titleColor(for: .highlighted) .~ UIColor.ksr_grey_500.mixLighter(0.36)


ksr_text_black here too 👍

justinswart · 2019-08-19T17:21:12Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

+    |> UIButton.lens.titleColor(for: .highlighted) .~ UIColor.ksr_grey_500.mixLighter(0.36)
+    |> UIButton.lens.contentEdgeInsets .~ UIEdgeInsets(topBottom: Styles.gridHalf(1))
+    |> UIButton.lens.image(for: .normal) %~ { _ in image(named: "icon--refresh-small") }
+    |> UIButton.lens.image(for: .highlighted) %~ { _ in image(named: "icon--refresh-small", alpha: 0.36) }


Let's use alpha 0.66 so that's not too washed out.

justinswart · 2019-08-19T17:23:09Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

-      self.pledgeCTAButton.widthAnchor.constraint(greaterThanOrEqualToConstant: Layout.Button.minWidth)
+      self.pledgeCTAButton.widthAnchor.constraint(greaterThanOrEqualToConstant: Layout.Button.minWidth),
+      self.pledgeRetryButton.heightAnchor.constraint(greaterThanOrEqualToConstant: Layout.Button.minHeight),
+      self.pledgeRetryButton.widthAnchor.constraint(greaterThanOrEqualToConstant: Layout.Button.minWidth)


This constraint is creating a warning, do we need it for this type of button?

justinswart

Cool lgtm!

justinswart and others added 30 commits February 15, 2019 11:17

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

8e02483

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

91269f3

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

2bb3cef

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

48d2742

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

15aa6ac

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

267b0da

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

64897e4

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

822a34b

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

818afed

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

c38221b

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

e906b5d

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

dfbab73

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

da7e0de

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

4f89370

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

877eb2f

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

ce57299

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

8d274ec

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

ba84670

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

08bc27d

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

7399a74

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

905344c

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

70714ff

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

48fffda

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

4ad67f3

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

2972953

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

af095fe

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

79b162f

Added reload icon, changed fetchProject function to return ErrorEnvel…

38a6c08

…ope instead of Never

Changed configureWith input to receive a Either<Project, ErrorEnvelop…

5393491

…e> type

Scollaco requested a review from justinswart August 13, 2019 14:30

Scollaco assigned ifbarrera Aug 13, 2019

Scollaco assigned justinswart and unassigned ifbarrera Aug 13, 2019

Scollaco commented Aug 13, 2019

View reviewed changes

Scollaco added 4 commits August 13, 2019 17:01

strings

33817ce

Swiftformat

737c0fa

Fixed test

da3c258

Swiftformat again

ea74d70

Scollaco force-pushed the reward-container-error-handling branch from d007873 to ea74d70 Compare August 13, 2019 21:30

Scollaco added 2 commits August 13, 2019 17:31

Merge branch 'master' into reward-container-error-handling

f5aca07

Merge branch 'master' into reward-container-error-handling

613acc7

justinswart reviewed Aug 14, 2019

View reviewed changes

Scollaco added 2 commits August 15, 2019 13:45

Merge branch 'master' into reward-container-error-handling

13c7f22

Justins feedback

e45b6ce

justinswart reviewed Aug 15, 2019

View reviewed changes

Scollaco added 6 commits August 16, 2019 16:44

Added extension to apply alpha in UIImage, Justins feedback

5417202

Swiftformat

12bfca8

Merge remote-tracking branch 'oss/master' into reward-container-error…

6870704

…-handling

Merge remote-tracking branch 'oss/master' into reward-container-error…

e192b73

…-handling

Merge remote-tracking branch 'oss/master' into reward-container-error…

931f76a

…-handling

Fixed failing test

fb00396

justinswart reviewed Aug 19, 2019

View reviewed changes

Scollaco added 2 commits August 19, 2019 13:49

Feedback again

d11a6fd

Fixed broken constraints, fixed logic that hides buttons while loading

75693d6

justinswart approved these changes Aug 20, 2019

View reviewed changes

Scollaco merged commit 56e0138 into master Aug 20, 2019

Scollaco deleted the reward-container-error-handling branch August 20, 2019 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reward container error handling #796

Reward container error handling #796

Scollaco commented Aug 13, 2019

ksr-ci-bot commented Aug 13, 2019 •

edited

Loading

Scollaco commented Aug 13, 2019

Scollaco Aug 13, 2019 •

edited

Loading

justinswart Aug 14, 2019

justinswart Aug 14, 2019

justinswart Aug 14, 2019

justinswart Aug 14, 2019

Scollaco Aug 15, 2019

justinswart Aug 14, 2019

Scollaco Aug 15, 2019

justinswart Aug 14, 2019

Scollaco Aug 15, 2019

justinswart Aug 15, 2019

Scollaco Aug 16, 2019

justinswart Aug 14, 2019

justinswart Aug 14, 2019

justinswart Aug 14, 2019

Scollaco Aug 15, 2019 •

edited

Loading

justinswart Aug 15, 2019

Scollaco Aug 16, 2019

justinswart Aug 15, 2019

justinswart Aug 15, 2019

Scollaco Aug 16, 2019 •

edited

Loading

justinswart Aug 19, 2019

justinswart Aug 19, 2019

justinswart Aug 19, 2019

justinswart Aug 19, 2019

justinswart Aug 19, 2019

justinswart left a comment

Reward container error handling #796

Reward container error handling #796

Conversation

Scollaco commented Aug 13, 2019

📲 What

🤔 Why

🛠 How

👀 See

♿️ Accessibility

✅ Acceptance criteria

ksr-ci-bot commented Aug 13, 2019 • edited Loading

Scollaco commented Aug 13, 2019

Scollaco Aug 13, 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

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

Scollaco Aug 15, 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

Scollaco Aug 16, 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

justinswart left a comment

Choose a reason for hiding this comment

ksr-ci-bot commented Aug 13, 2019 •

edited

Loading

Scollaco Aug 13, 2019 •

edited

Loading

Scollaco Aug 15, 2019 •

edited

Loading

Scollaco Aug 16, 2019 •

edited

Loading