Change email implementation

[Scollaco]

What

Change Email feature! \o/

Why

As part of settings V2.

How

Added a new query to fetch email user and created a graphUser object in order to decode the email field from graphQL.

See 👀

[ifbarrera]

Nice work @Scollaco! Left some comments.

I think also if you pull the latest from master, you can implement the "loading" state using the new LoadingBarButtonItemView. Lmk if you have questions about how to use it!

[ifbarrera]

Is there any way we could put this logic ("if email and password fields have text, then do ...") in the view model? Having this logic in the saveButtonTapped function makes more difficult to test.

[ifbarrera]

I think this functionality could also live in the view model self.viewModel.inputs.resetFields() because then we can write a test that verifies that the two outputs passwordTextFieldText and newEmailTextFieldText both emit signals with the value "".

[ifbarrera]

Same here, can this live in the view model? self.viewModel.inputs.textFieldShouldReturn(with returnKeyType), because then we could test that when the return key type is .next, then some output (lets say self.viewModel.outputs.passwordTextFieldBecomeFirstResponder emits a signal. We could then also test that when the returnKeyType is .go, that the password form is submitted.

[ifbarrera]

Take a look at the generic UserEnvelope I created here. Instead of having a GraphUser, I think we should try to create small structs that contain the data we need, for example UserEmail, so we avoid having an entire other user object in to reckon with in the app. What do you think?

I also think you can just use Swift.Decodable's automatic conformance, since your UserEmail struct only contains one Codable property.

[Scollaco]

Now that the PR with the UserEnvelope is merged I can use it. Regarding the usage of multiple micro structs, I just wonder what would be the complexities involved in case we needed more than one field (like email and currency, for example. Is this case, how would we decode multiple object types?)

[ifbarrera]

I think this is kind of a bigger discussion around how we want to think of our "models" moving forward as we start to use GraphQL more and more. My gut says that for now lets just take it case-by-case and see what makes the most sense: if a new feature were to come up that required both email, currency, and some other user values, then it might make more sense to use the converted Swift.Decodable User object to fetch all those fields.

But here's another example: in change email, eventually we have to add the "isEmailVerified" property. I imagine that in that case, since it's still only two properties, we would add that to the struct where email lives. But then what do we call this struct? UserEmail no longer makes sense, but what could make sense is UserChangeEmailModel, ie. the model containing the properties the feature needs in order to function. This starts to move us in a direction where our models are no longer necessarily closely associated with the underlying data model of the overall Kickstarter application, but rather the model required to populate our client's views. From my understanding this is part of the GraphQL design, and something they see as a strength. From the iOS side, we need to decide whether we want to move in that direction, or maintain more "pure" models objects (models like User, Project, Reward, etc) with all optional properties (similar to what @justinswart described as their approach on Drip)

[justinswart]

Yeah 🤔it's kind've tricky with Swift types. I'm guessing this stuff is easier in JS/TypeScript, but I don't really know.

Another option I was considering could be something like a UserProtocol which the various GraphQL types could conform to through extensions.

Also, we could consider really tiny protocols like UserEmailProtocol and UserCurrencyProtocol and then in a situation where both are required perhaps they can be composed UserEmailProtocol & UserCurrencyProtocol.

Although I'm not sure if this would help at all, it might just make more complicated.

[ifbarrera]

I wonder if we can find a way to reuse the same mutation for ChangePassword since the Graph mutation itself is exactly the same? We could possibly do something like this:

public struct UpdateUserAccountMutation<T: GraphMutationInput>: GraphMutation {
  var input: T

  public init(input: T) {
    self.input = input
  }

  public var description: String {
    return """
    mutation updateUserAccount($input: UpdateUserAccountInput!) {\
      updateUserAccount(input: $input) {\
        clientMutationId\
      }\
    }
    """
  }
}

Both change password and change email could use this mutation.

[Scollaco]

Good call. This is totally possible because of the protocols. Love it!

[ifbarrera]

Nice! ⭐️

[ifbarrera]

Hm maybe we should avoid updating the value of two properties within a single input function, you can always use Signal.merge to listen for changePasswordProperty wherever you need to grab a "save" event.

[Scollaco]

Actually the saveButtonTappedProperty is no longer needed 😅

[ifbarrera]

Maybe this email should be different so we can see whether two values were emitted: one for the current email, and one for the updated email?

[Scollaco]

The testEmailText_AfterFetchingUsersEmail() is testing the response, so the email should always be equal "ksr@kickstarter.com". (On the comment below I explained what may be causing confusion).

[ifbarrera]

The name of this test seems to imply that its just testing that the current email is fetched and correctly triggers a signal, but the implementation of the test seems to test the passwordFieldDidTapGo input, which I think is associated with submitting a new email. Does this test need to be renamed?

[Scollaco]

The name of the test is correct. I had used the passwordFieldDidGo input because it's one of the inputs that triggers the signal. But it's a good idea to replace it with self.vm.inputs.viewDidLoad() (since I'm merging both), so it will avoid any confusion.

[ifbarrera]

LGTM! 👏

[Scollaco]

Change Email Implementation

[Scollaco]

One password implementation in Change email