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

Add profile api #68

Merged
merged 25 commits into from
Mar 9, 2017
Merged

Add profile api #68

merged 25 commits into from
Mar 9, 2017

Conversation

motyd
Copy link
Contributor

@motyd motyd commented Feb 28, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.9%) to 74.714% when pulling f930f26 on add-profile-api into c3821b5 on development.

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.9%) to 74.714% when pulling f930f26 on add-profile-api into c3821b5 on development.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 81.408% when pulling 06d4377 on add-profile-api into c3821b5 on development.

@motyd motyd requested a review from aal80 March 7, 2017 09:05
@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 81.159% when pulling 0092666 on add-profile-api into b9291a8 on development.

@@ -178,6 +178,7 @@ internal class AppIDConstants {
internal static let USER_IDENTITY_LABEL = "userIdentity"
// labels

internal static let AnonymousIdpName = "appid_anon"
internal static let BMSSecurityErrorDomain = "com.ibm.mobilefirstplatform.clientsdk.swift.bmssecurity"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the BMSSecurityErrorDomain property used for? It is part of MCA SDK, there shouldn't be any mentioning of com.ibm.mobilefirstplatform at all in AppID SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know about this. It is not related to the profiles API, and I am sure there are more of those hiding. Let's finish this, and then we'll do sweep around the code and remove any such "old" constants

self.loginAnonymously(accessTokenString: accessTokenString, allowCreateNewAnonymousUsers: true, authorizationDelegate: authorizationDelegate)
}

public func loginAnonymously(accessTokenString:String?, allowCreateNewAnonymousUsers: Bool, authorizationDelegate:AuthorizationDelegate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to have three signatures. You can use Swift optional arguments with default values. Talked to @motyd about this.

@@ -11,7 +11,6 @@ import Foundation

public protocol UserAttributeDelegate {
Copy link
Contributor

Choose a reason for hiding this comment

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

UserAttributes APIs are simple APIs that return true/false. We should use completionHandlers instead of delegates.

}

public func launch(accessTokenString: String?, delegate: AuthorizationDelegate) {
self.oauthManager.authorizationManager?.launchAuthorizationUI(accessTokenString: accessTokenString, authorizationDelegate: delegate)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

having one method signature with optional arguments with default value should be enough

Copy link
Contributor

@aal80 aal80 left a comment

Choose a reason for hiding this comment

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

See comments

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 81.125% when pulling 6058e6e on add-profile-api into b9291a8 on development.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 81.125% when pulling 6058e6e on add-profile-api into b9291a8 on development.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 81.125% when pulling 3024f45 on add-profile-api into b9291a8 on development.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 81.245% when pulling c45d34b on add-profile-api into b9291a8 on development.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 81.245% when pulling 547c130 on add-profile-api into 351ed3c on development.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 81.245% when pulling 0c85dd3 on add-profile-api into 1bd791e on development.

@@ -14,5 +14,5 @@
import Foundation

public protocol LoginWidget {
func launch(delegate:AuthorizationDelegate)
func launch(accessTokenString: String?, delegate: AuthorizationDelegate)
Copy link
Contributor

Choose a reason for hiding this comment

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

default accessTokenString to nil

@motyd motyd merged commit 794c20c into development Mar 9, 2017
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

3 participants