Skip to content

login with external ID#403

Merged
liujoshua merged 2 commits intoResearchStack:developfrom
DwayneJengSage:develop
Nov 11, 2017
Merged

login with external ID#403
liujoshua merged 2 commits intoResearchStack:developfrom
DwayneJengSage:develop

Conversation

@DwayneJengSage
Copy link
Collaborator

Adding page for login with external ID.

Copy link
Collaborator

@liujoshua liujoshua left a comment

Choose a reason for hiding this comment

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

Thanks, looks great. One change request for consistency with ResearchKit constants

@@ -57,6 +57,8 @@ public enum SurveyItemType {
ACCOUNT_EMAIL_VERIFICATION ("emailVerification" ), // EmailVerificationStep
@SerializedName("externalID")
ACCOUNT_EXTERNAL_ID ("externalID" ), // ExternalIDStep
Copy link
Collaborator

Choose a reason for hiding this comment

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

ACCOUNT_EXTERNAL_ID, which currently produces a non-implemented step, is intended to support this feature. We should use this.

@dephillipsmichael, does that sound right?

See:

https://github.com/Sage-Bionetworks/BridgeAppSDK/blob/6a135f98a2e7c4020f278a76fcdf97ff2f5fc777/BridgeAppSDKSample/StudyOverviewViewController.swift#L63

@@ -690,8 +724,9 @@ public ProfileStep createProfileStep(Context context, ProfileSurveyItem item) {
* @param item InstructionSurveyItem from JSON
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add @param loginOptions

static {
List<ProfileInfoOption> tempList = new ArrayList<>();
tempList.add(ProfileInfoOption.EXTERNAL_ID);
EXTERNAL_ID_LOGIN_OPTIONS = Collections.unmodifiableList(tempList);
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for immutable

@liujoshua liujoshua merged commit 5d863e7 into ResearchStack:develop Nov 11, 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.

2 participants