[NT-995] Settings account header for Apple users #1160

ifbarrera · 2020-04-14T15:03:34Z

📲 What

Adds a header to the Settings account screen for Apple connected users, and hides the Change Password and Change Email functionality.

🤔 Why

Apple connected users will not be able to change their Kickstarter email, and they will not have a password. We want to prevent these users from accessing these features.

🛠 How

added SettingsAccountHeader that displays the user's email and some messaging about how to update their Apple account
updated the logic that hides the change email/password section to hide it for apple connected users
renamed UserCurrency.swift to GraphUser.swift and added tests
added isAppleConnected to the User schema, and updated the account query to include the isAppleConnected field
fixed a .combineLatest bug where the reloadData output was firing multiple times after the first load.
added a bunch of missing VM tests

👀 See

Trello, screenshots, external resources?

iPhone 8	iPad

♿️ Accessibility

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

✅ Acceptance criteria

Using Charles Proxy, enable breakpoints to intercept the response from the GraphQL endpoint. Modify your payload by setting isAppleConnected to true, to see the Apple-connected user treatment.

Navigate to the account section, and modify your response payload by setting isAppleConnected to true. You should see the new Apple header displaying your email.

⏰ TODO

update strings

…NT-995-apple-account-header

ifbarrera · 2020-04-14T15:14:51Z

Library/ViewModels/SettingsAccountViewModelTests.swift

      self.vm.inputs.viewWillAppear()
-      self.reloadDataShouldHideWarningIcon.assertValueCount(1)


This wasn't even testing the thing that the test name says it's testing 😢

ifbarrera · 2020-04-14T15:15:53Z

Library/ViewModels/SettingsAccountViewModelTests.swift

      self.vm.inputs.viewWillAppear()
-      self.reloadDataShouldHideWarningIcon.assertValueCount(1)


This was also not even testing what the value of reloadDataShouldHideWarningIcon was, which meant it could have been the opposite of what was expected and this test would have passed :\

ifbarrera · 2020-04-14T15:18:18Z

Library/UIStackView.swift

@@ -15,3 +15,14 @@ public func ksr_setCustomSpacing(_ spacing: CGFloat) -> ((UIView, UIStackView) -
    return stackView
  }
 }
+
+public func ksr_setBackgroundColor(_ color: UIColor) -> ((UIStackView) -> (UIStackView)) {


✨ new ✨ so that we can set a background color on UIStackViews

Handy as this may be 😄 If we're calling this in bindStyles() we may end up with many background views? 🤔 I would suggest this as an extension on UIStackView with an associated object to track any existing background views perhaps and, if one exists, just update its backgroundColor. Another fairly rudimentary way could be to use UIView's tag property and just set some sentinel value that you can check for.

Calling it again in the test shows that it's not idempotent:

Ahh good catch!

nativeksr · 2020-04-14T15:22:42Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

justinswart

Looks good! had some thoughts/questions.

justinswart · 2020-04-14T23:28:48Z

Kickstarter-iOS/DataSources/SettingsAccountDataSource.swift

@@ -9,11 +9,17 @@ final class SettingsAccountDataSource: ValueCellDataSource {
  func configureRows(


Seems like this can rather be a single type SettingsAccountRowData which should make it easier to modify when needed. Perhaps not as part of this PR 🤷‍♂️

justinswart · 2020-04-14T23:31:53Z

Kickstarter-iOS/Views/HeaderFooterViews/SettingsAccountHeaderView.swift

+  private lazy var emailLabel = { UILabel(frame: .zero) }()
+  private lazy var manageThisAccountLabel = {
+    UILabel(frame: .zero)
+      |> UILabel.lens.translatesAutoresizingMaskIntoConstraints .~ false


Worth noting that our constraint helper functions automatically set this and it's also redundant when a view is added to a stackview.

So actually this label is not constrained using our helper functions, nor added to a stack view, which is why it needs this to be set.

Ah apologies!

justinswart · 2020-04-14T23:32:36Z

Kickstarter-iOS/Views/HeaderFooterViews/SettingsAccountHeaderView.swift

+      |> UILabel.lens.translatesAutoresizingMaskIntoConstraints .~ false
+  }()
+
+  private lazy var stackView = {


Think we've mostly standardized on rootStackView for the top-most stack.

justinswart · 2020-04-14T23:37:38Z

KsApi/models/GraphUserTests.swift

+    }
+    """
+
+    guard let data = jsonString.data(using: .utf8) else {


Saves a guard statement if we use the non-failable Data initializer:

let data = Data(jsonString.utf8)

justinswart · 2020-04-14T23:40:48Z

Library/UIStackView.swift

@@ -15,3 +15,14 @@ public func ksr_setCustomSpacing(_ spacing: CGFloat) -> ((UIView, UIStackView) -
    return stackView
  }
 }
+
+public func ksr_setBackgroundColor(_ color: UIColor) -> ((UIStackView) -> (UIStackView)) {


Handy as this may be 😄 If we're calling this in bindStyles() we may end up with many background views? 🤔 I would suggest this as an extension on UIStackView with an associated object to track any existing background views perhaps and, if one exists, just update its backgroundColor. Another fairly rudimentary way could be to use UIView's tag property and just set some sentinel value that you can check for.

Calling it again in the test shows that it's not idempotent:

justinswart · 2020-04-14T23:46:22Z

Kickstarter-iOS/Views/HeaderFooterViews/SettingsAccountHeaderView.swift

+  label
+    |> settingsDescriptionLabelStyle
+    |> \.text %~ { _ in localizedString(
+      key: "manage_this_account",


Here too. Not sure if these are legacy.

justinswart · 2020-04-14T23:50:32Z

Library/ViewModels/SettingsAccountViewModel.swift

@@ -2,6 +2,11 @@ import KsApi
 import Prelude
 import ReactiveSwift

+public typealias SettingsAccountData = (
+  currency: Currency, email: String,


Could we put each label on new line?

justinswart · 2020-04-15T00:01:18Z

Library/ViewModels/SettingsAccountViewModel.swift

+          return true
+        }
+
+        guard let isEmailVerified = user.isEmailVerified,


For your consideration:

if [user.isEmailVerified, user.isDeliverable] .map({ $0 ?? false }) .allSatisfy(isTrue) { return true }

Not sure how much it really adds for readability, just less guard lets.

Hmm yeah I'm not sure the extra mental overhead is worth it here 😅

Library/ViewModels/SettingsAccountViewModel.swift

justinswart · 2020-04-15T00:06:15Z

Library/ViewModels/SettingsAccountViewModelTests.swift

      self.vm.inputs.viewWillAppear()
-      self.reloadDataShouldHideWarningIcon.assertValueCount(1)


justinswart

🚢

Library/ViewModels/SettingsAccountViewModel.swift

ifbarrera added 9 commits April 13, 2020 12:01

Setting up SettingsAcccountHeaderView

37cdaa8

Tweaking view

cf8eb01

Rename UserAccountFields to GraphUser

a77828a

Update logic to account for apple connected accounts

a230279

VM tests and GraphUser tests

3798175

Formatting

1babb72

Cleanup

e7d56e5

Swiftlint

3ba2789

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

9a56687

…NT-995-apple-account-header

ifbarrera commented Apr 14, 2020

View reviewed changes

Add test for ksr_setBackgroundColor

7340fb6

ifbarrera commented Apr 14, 2020

View reviewed changes

ifbarrera assigned justinswart Apr 14, 2020

ifbarrera added the needs review label Apr 14, 2020

justinswart reviewed Apr 15, 2020

View reviewed changes

Fixing bug with StackView helper and cleanup

bb05f5a

justinswart approved these changes Apr 16, 2020

View reviewed changes

Library/ViewModels/SettingsAccountViewModel.swift Show resolved Hide resolved

ifbarrera added 2 commits April 17, 2020 14:02

Apple ID Strings

b3ab1d5

Use translations

7eb6299

ifbarrera merged commit a95db3d into master Apr 17, 2020

ifbarrera deleted the NT-995-apple-account-header branch April 17, 2020 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NT-995] Settings account header for Apple users #1160

[NT-995] Settings account header for Apple users #1160

ifbarrera commented Apr 14, 2020 •

edited by justinswart

Loading

ifbarrera Apr 14, 2020

ifbarrera Apr 14, 2020

justinswart Apr 15, 2020

ifbarrera Apr 14, 2020

justinswart Apr 14, 2020

ifbarrera Apr 15, 2020

nativeksr commented Apr 14, 2020

justinswart left a comment

justinswart Apr 14, 2020 •

edited

Loading

justinswart Apr 14, 2020

ifbarrera Apr 15, 2020

justinswart Apr 15, 2020

justinswart Apr 14, 2020

justinswart Apr 14, 2020

justinswart Apr 14, 2020

justinswart Apr 14, 2020

justinswart Apr 14, 2020

justinswart Apr 15, 2020 •

edited

Loading

ifbarrera Apr 15, 2020

justinswart Apr 15, 2020

justinswart left a comment

		self.vm.inputs.viewWillAppear()
		self.reloadDataShouldHideWarningIcon.assertValueCount(1)

		@@ -9,11 +9,17 @@ final class SettingsAccountDataSource: ValueCellDataSource {
		func configureRows(

[NT-995] Settings account header for Apple users #1160

[NT-995] Settings account header for Apple users #1160

Conversation

ifbarrera commented Apr 14, 2020 • edited by justinswart Loading

📲 What

🤔 Why

🛠 How

👀 See

♿️ Accessibility

✅ Acceptance criteria

⏰ TODO

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

nativeksr commented Apr 14, 2020

justinswart left a comment

Choose a reason for hiding this comment

justinswart Apr 14, 2020 • 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

justinswart Apr 15, 2020 • edited Loading

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

ifbarrera commented Apr 14, 2020 •

edited by justinswart

Loading

justinswart Apr 14, 2020 •

edited

Loading

justinswart Apr 15, 2020 •

edited

Loading