Email undeliverable/unverified #478

ifbarrera · 2018-11-08T19:39:14Z

What

Let users know when their email is unverified or undeliverable.

Why

So they can correct the issue.

See 👀

Undeliverable:

Unverified:

TODO ⏰

missing one string, still waiting to merge and deploy that

…email-undeliverable # Conflicts: # Kickstarter-iOS/Views/Controllers/ChangeEmailViewController.swift # Kickstarter-iOS/Views/Storyboards/Settings.storyboard # Library/ViewModels/ChangeEmailViewModel.swift # Library/ViewModels/ChangeEmailViewModelTests.swift

…email-undeliverable

justinswart

Cool! Few minor comments 💬

justinswart · 2018-11-08T21:38:20Z

Kickstarter-iOS/Views/Controllers/ChangeEmailViewController.swift

@@ -70,12 +69,21 @@ internal final class ChangeEmailViewController: UIViewController {
    _ = onePasswordButton
      |> onePasswordButtonStyle

-    _ = errorLabel
+    _ = messageLabelView


Couple references to self missing in this function.

justinswart · 2018-11-08T21:40:18Z

Library/ViewModels/ChangeEmailViewModel.swift

          .ksr_delay(AppEnvironment.current.apiDelayInterval, on: AppEnvironment.current.scheduler)
          .materialize()
-    }
+    }.logEvents()


Some debugging code ☝️

Just removed that 😄

justinswart · 2018-11-08T21:51:04Z

Library/ViewModels/ChangeEmailViewModel.swift

+    self.resendVerificationEmailViewIsHidden = Signal.combineLatest(isEmailVerified, isEmailDeliverable)
+      .map { $0 && $1 }
+
+    self.unverifiedEmailLabelHidden = Signal


Finding this a little hard to grok with the negates 😅. Is it the same as:

self.unverifiedEmailLabelHidden = Signal .combineLatest(isEmailVerified, isEmailDeliverable) .map { isEmailVerified, isEmailDeliverable in guard isEmailVerified else { return !isEmailDeliverable } return true }

Not sure how much more readable that is tbh

Feels like this might not be right - lemme test it

Agree the negates make it a bit more complicated. I'll update to this format 👍

justinswart · 2018-11-08T22:10:19Z

Library/ViewModels/ChangeEmailViewModel.swift

+
+    self.messageLabelViewHidden = Signal
+      .merge(self.unverifiedEmailLabelHidden, self.warningMessageLabelHidden)
+      .filter { isFalse($0) }


Can do .filter(isFalse) here.

justinswart · 2018-11-08T22:12:05Z

Library/ViewModels/ChangeEmailViewModelTests.swift

@@ -179,19 +184,111 @@ final class ChangeEmailViewModelTests: TestCase {
  }

  func testResendVerificationStackViewIsHidden_IfEmailIsVerified() {
-    self.resendVerificationStackViewIsHidden.assertDidNotEmitValue()
+    withEnvironment {


No need for withEnvironment unless we're modifying the environment in some way.

justinswart · 2018-11-08T22:13:05Z

Library/ViewModels/ChangeEmailViewModelTests.swift

+      self.scheduler.advance()
+
+      self.resendVerificationEmailViewIsHiddenObserver
+        .assertValues([false], "Email is underliverable")


Small typo in the comment

ifbarrera · 2018-11-09T20:54:39Z

Email update: unverified email iOS

Scollaco · 2018-11-09T21:15:30Z

Kickstarter-iOS/Views/Controllers/ChangeEmailViewController.swift

      |> onePasswordButtonStyle

-    _ = errorLabel
+    _ = self.messageLabelView


I think here would be a good opportunity to remove the lenses and adopt the same pattern of self.passwordTextField styling.

Scollaco · 2018-11-12T21:48:40Z

Library/ViewModels/ChangeEmailViewModelTests.swift

@@ -179,19 +184,102 @@ final class ChangeEmailViewModelTests: TestCase {
  }

  func testResendVerificationStackViewIsHidden_IfEmailIsVerified() {


I would just rename the tests to testResendVerificationEmailViewIsHidden..., since we are not using stackview anymore.

Scollaco · 2018-11-12T21:48:56Z

Library/ViewModels/ChangeEmailViewModelTests.swift

+    self.scheduler.advance()
+
+    self.resendVerificationEmailViewIsHiddenObserver
+      .assertValues([true], "Email is deliverable and verified")
  }

  func testResendVerificationStackViewIsNotHidden_IfEmailIsNotVerified() {


Scollaco · 2018-11-12T21:49:07Z

Library/ViewModels/ChangeEmailViewModelTests.swift

  }

-  func testResendVerificationStackViewAppears_AfterChangingEmail() {
+  func testResendVerificationStackViewIsNotHidden_IfEmailIsUndeliverable() {


Scollaco · 2018-11-12T21:52:04Z

Library/ViewModels/ChangeEmailViewModelTests.swift

+    }
+  }
+
+  func testWarningMessageLabel_isHidden() {


Even though the assertion message gives a little more detail of what's being tested, I would make the test name a little more descriptive, like: testWarningMessageLabel_isHidden_If.... So we can understand what's being tested just by reading the function name.

Scollaco · 2018-11-12T21:52:19Z

Library/ViewModels/ChangeEmailViewModelTests.swift

+    self.warningMessageLabelHiddenObserver.assertValues([true], "Email is deliverable")
+  }
+
+  func testWarningMessageLabel_isNotHidden() {


Scollaco · 2018-11-12T21:52:27Z

Library/ViewModels/ChangeEmailViewModelTests.swift

+    }
+  }
+
+  func testUnverifiedEmailLabel_isHidden() {


Scollaco · 2018-11-12T21:53:31Z

Library/ViewModels/ChangeEmailViewModelTests.swift

+    self.unverifiedEmailLabelHiddenObserver.assertValues([true], "Email is verified & deliverable")
+  }
+
+  func testUnverifiedEmailLabel_isNotHidden() {


Scollaco

Thanks for addressing the feedbacks, @ifbarrera . I just let a few suggestions of how to make some tests more descriptive.

* 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

* 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

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

ifbarrera added 6 commits November 5, 2018 15:43

Adding emailIsVerified

54a7b0d

Renaming GraphUserEmail

de4c08e

ViewModel tests

a0e6b06

ChangeEmailViewController tests

cea76bc

Strings, swiftlint

943ac3f

ifbarrera changed the title ~~Email undeliverable~~ Email undeliverable/unverified Nov 8, 2018

ifbarrera added 3 commits November 8, 2018 14:39

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

5df399a

…email-undeliverable

Strings & new screenshots

8fa5757

Cleanup

46d2366

ifbarrera requested review from Scollaco and justinswart November 8, 2018 22:10

ifbarrera added the needs review label Nov 8, 2018

justinswart reviewed Nov 8, 2018

View reviewed changes

PR comments

d6b22cf

Scollaco reviewed Nov 9, 2018

View reviewed changes

PR updates

48fbcdf

Scollaco reviewed Nov 12, 2018

View reviewed changes

Library/ViewModels/ChangeEmailViewModelTests.swift Outdated

}

}

func testUnverifiedEmailLabel_isHidden() {

Copy link

Contributor

Scollaco Nov 12, 2018

Choose a reason for hiding this comment

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

And here

Scollaco reviewed Nov 12, 2018

View reviewed changes

Scollaco approved these changes Nov 12, 2018

View reviewed changes

Test naming

6c329ea

ifbarrera merged commit e3cd226 into master Nov 12, 2018

ifbarrera deleted the email-undeliverable branch November 12, 2018 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Email undeliverable/unverified #478

Email undeliverable/unverified #478

ifbarrera commented Nov 8, 2018

justinswart left a comment

justinswart Nov 8, 2018

justinswart Nov 8, 2018

ifbarrera Nov 8, 2018

justinswart Nov 8, 2018

justinswart Nov 8, 2018

ifbarrera Nov 8, 2018

justinswart Nov 8, 2018

justinswart Nov 8, 2018

justinswart Nov 8, 2018

ifbarrera commented Nov 9, 2018

Scollaco Nov 9, 2018

Scollaco Nov 12, 2018 •

edited

Loading

Scollaco Nov 12, 2018

Scollaco Nov 12, 2018

Scollaco Nov 12, 2018 •

edited

Loading

Scollaco Nov 12, 2018

Scollaco Nov 12, 2018

Scollaco Nov 12, 2018

Scollaco left a comment

		@@ -179,19 +184,102 @@ final class ChangeEmailViewModelTests: TestCase {
		}

		func testResendVerificationStackViewIsHidden_IfEmailIsVerified() {

Email undeliverable/unverified #478

Email undeliverable/unverified #478

Conversation

ifbarrera commented Nov 8, 2018

What

Why

See 👀

TODO ⏰

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

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

ifbarrera commented Nov 9, 2018

Choose a reason for hiding this comment

Scollaco Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scollaco Nov 12, 2018 • 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

Scollaco left a comment

Choose a reason for hiding this comment

Scollaco Nov 12, 2018 •

edited

Loading

Scollaco Nov 12, 2018 •

edited

Loading