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

Codable for EmailVerification and PasswordReset metadata objects #319

Merged
merged 8 commits into from
Sep 13, 2018

Conversation

ifabijanovic
Copy link
Contributor

Description

EmailVerification and PasswordReset metadata objects which are a part of UserMetadata do not have the Codable protocol implemented. This caused the User object to fail to initialize using init(from:) with the following error:

screen shot 2018-09-10 at 10 50 05

Changes

Implemented Codable for both EmailVerification and PasswordReset objects. Also updated the mapping(map:) method of Metadata as it seems it wasn't passing the correct keys to map.

Tests

Tested in an app making use of Kinvey user accounts, the above mentioned error is gone, checked that all of the data was correctly parsed and available inside the User instance.

@heyzooi
Copy link
Contributor

heyzooi commented Sep 10, 2018

Hi @ifabijanovic, thanks for your contribution.
We need to add unit tests as well. Maybe we can do that after we accept this PR. Let me check the best way to do this.

@heyzooi
Copy link
Contributor

heyzooi commented Sep 10, 2018

@ifabijanovic I'm wondering if would be possible for you to add a unit test, even a simple one and we could add more later if needed.

@ifabijanovic
Copy link
Contributor Author

I will have a look at your unit tests and try to produce one.

- this causes unit tests using socialIdentity to start failing as it seems that object does not implement Codable
- test testMICCancelUserAction also seems to crash with SIGABRT on line 4835
@ifabijanovic
Copy link
Contributor Author

I've implemented Codable for MyUser in UserTests.swift but this has an effect of tests checking socialIdentity to start failing. I am also getting a SIGABRT when running just UserTests in test testMICCancelUserAction on line 4835.

@heyzooi
Copy link
Contributor

heyzooi commented Sep 11, 2018

I suggest you create your own user class (a new one, like MyUserCodable) so it will not affect the existing unit tests.

@ifabijanovic
Copy link
Contributor Author

That does not make sense - you want to make use of existing unit tests when introducing changes and this change found that you have a bug in UserSocialIdentity not implementing Codable. I am also not sure why testMICCancelUserAction is timing out when just running UserTests because it passes when running ALL tests in KinveyTests target - unit tests are not atomic?

@heyzooi
Copy link
Contributor

heyzooi commented Sep 12, 2018

@ifabijanovic I can't reproduce the issue that you are reporting with the testMICCancelUserAction test. Running it alone or all tests of UserTests, I'm always getting a success result.

I suggest you create your own user class and duplicate the unit tests to cover the Codable use case.

@ifabijanovic
Copy link
Contributor Author

That's strange that everything passes for you... Anyway I've reverted my original unit test changes and added a new custom user class as you advised. I implemented two unit tests (based off other unit tests) - save one works, but refresh one fails for the custom property. It seems that updating a custom user property on the client side results in correct behaviour, but updating a custom value on the backend is not propagated to the client after calling refresh.

@heyzooi heyzooi merged commit f6fd366 into Kinvey:develop Sep 13, 2018
@ifabijanovic
Copy link
Contributor Author

Nice changes! I think User should be good now - I will give this a try in my project. We don't use any 3rd party auth (Facebook, Google) so that part might still need some work in regards to Codable - the socialIdentities bit does not implement it yet.

@ifabijanovic ifabijanovic deleted the metadata branch September 17, 2018 05:46
@heyzooi
Copy link
Contributor

heyzooi commented Sep 17, 2018

awesome @ifabijanovic! please let us know, looking forward for your feedback.
we are going to add socialIdentities to our roadmap to be done ASAP.
thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants