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

Added support for rootAttributes #557

Merged
merged 5 commits into from
Jul 23, 2019

Conversation

cocojoe
Copy link
Member

@cocojoe cocojoe commented Jul 15, 2019

Changes

Storage for Custom Fields can now be changed to use rootAttributes, default is false (non breaking) which means use the existing behaviour of being stored in user metaData

public init(name: String, placeholder: String, rootAttribute: Bool = false, icon: LazyImage? = nil, keyboardType: UIKeyboardType = .default, autocorrectionType: UITextAutocorrectionType = .default, secure: Bool = false, contentType: UITextContentType? = nil, validation: @escaping (String?) -> Error? = nonEmpty)

References

auth0/Auth0.swift#287

Testing

Manually tested as well to check API call output

  • This change adds unit test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@cocojoe cocojoe marked this pull request as ready for review July 18, 2019 09:46
@cocojoe cocojoe requested a review from a team July 18, 2019 09:46
it("should clear root attributes") {
user.rootAttributes["family_name"] = "Doe"
user.reset()
expect(user.rootAttributes["first_name"]).to(beNil())

Choose a reason for hiding this comment

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

Is first_name a typo here, or should it be testing that family_name is Nil? If not, the intent of the test isn't too clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the intent is that he sets a value first, then clears the map, and then checks that the map no longer has that value he first added. So yes, Steve is right and it should be checking against the key added before.

Suggested change
expect(user.rootAttributes["first_name"]).to(beNil())
expect(user.rootAttributes["family_name"]).to(beNil())

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, was a typo

Lock/CustomTextField.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

given that userMetadata is a root profile attribute, would you consider changing the implementation so that all the methods which signatures can be changed now receive a single param named "rootAttributes" instead of "userMetadata" and "rootAttributes"?

When constructing the map with values, the metadata would go into a "user_metadata" key if the map is not empty.

@@ -203,13 +203,15 @@ func applyDefaultOptions(_ options: inout OptionBuildable) {

if #available(iOS 10, *) {
options.customSignupFields = [
CustomTextField(name: "first_name", placeholder: "First Name", icon: LazyImage(name: "ic_person", bundle: Lock.bundle), contentType: .givenName),
CustomTextField(name: "last_name", placeholder: "Last Name", icon: LazyImage(name: "ic_person", bundle: Lock.bundle), contentType: .familyName)
CustomTextField(name: "given_name", placeholder: "First Name", rootAttribute: true, icon: LazyImage(name: "ic_person", bundle: Lock.bundle), contentType: .givenName),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the sample app, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I use it for testing, will remove these actually in this case.

@@ -350,28 +350,38 @@ class DatabaseInteractorSpec: QuickSpec {
}

it("should always store value") {
let _ = try? database.update(.custom(name: "first_name"), value: "Auth0")
Copy link
Contributor

Choose a reason for hiding this comment

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

keep one as it was OR add a new test case to test the "default" behavior (still storing into user_metadata, or additional attributes in terms of iOS)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree with you. However, these are using mocks so it's not quite the same so I do need to specify.

@@ -303,6 +303,11 @@ class MockAuthentication: Authentication {
}

class MockWebAuth: WebAuth {

func useLegacyAuthentication(withStyle style: UIModalPresentationStyle) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Lock not been updated in a bit, bunch of changes in Auth0.swift as this PR requires latest.

@cocojoe cocojoe added this to the vNext milestone Jul 22, 2019
@cocojoe cocojoe force-pushed the added-support-root-attributes-signup branch from 49f727f to dda4429 Compare July 22, 2019 11:45
@cocojoe cocojoe merged commit 642d33e into master Jul 23, 2019
@cocojoe cocojoe deleted the added-support-root-attributes-signup branch July 23, 2019 09:15
@cocojoe cocojoe mentioned this pull request Jul 26, 2019
@joshcanhelp joshcanhelp added review:medium Medium review and removed medium labels Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants