Skip to content

Fix hinted login view to be compatible with secondary providers. - #8815

Merged
bradenmacdonald merged 1 commit into
openedx:masterfrom
open-craft:hinted-login-fix
Jul 8, 2015
Merged

Fix hinted login view to be compatible with secondary providers.#8815
bradenmacdonald merged 1 commit into
openedx:masterfrom
open-craft:hinted-login-fix

Conversation

@bradenmacdonald

Copy link
Copy Markdown
Contributor

Bug
Two features were developed in parallel and merged at about the same time during the SSO work, and though they work fine in isolation, they don't work together:

I was recently taking some screenshots on a sandbox and noticed that the "hinting" feature doesn't work for providers marked as "secondary".

Fix
The fix is quite simple: In the JS, the code currently looks for information about the hinted provider in the list of providers it is given. With this fix, it will also search the list of secondaryProviders.

Includes an updated JS test to cover this case, and I confirmed that the test fails without this fix.

Affects
This two features are key to the user experience of the new SSO functionality, and it is critical that they are able to work together.

Sandbox
http://sandbox5.opencraft.com/

Testing Instructions:
Try the following links in an incognito window. Note: the actual login process may not complete successfully on this sandbox as the providers are not configured, but the login form should at least display properly and the buttons should all work.

This link should display the Google login hint (this was working before):
http://sandbox5.opencraft.com/courses/course-v1:BradenX+DW+15/courseware/3d6ec3a921a645fea6e63e3e240f8946/0ae68e6072cd47beb332a87693339d8a/1?tpa_hint=oa2-google-oauth2

This link should also work, and display a UBC prompt (this was broken):
http://sandbox5.opencraft.com/courses/course-v1:BradenX+DW+15/courseware/3d6ec3a921a645fea6e63e3e240f8946/0ae68e6072cd47beb332a87693339d8a/1?tpa_hint=saml-ubc-staging

Before this fix, the above page was blank except for a header, due to a JS error.

Reviewers
@Kelketek and ??

Merge deadline
ASAP.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=8815

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I noticed that these parameters weren't being used at all by the template, so I removed them while I was at it.

@Kelketek

Kelketek commented Jul 8, 2015

Copy link
Copy Markdown
Contributor

👍

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

@cpennington Would you mind reviewing this really simple (but important) JS bugfix for SSO work?

@sarina

sarina commented Jul 8, 2015

Copy link
Copy Markdown
Contributor

@singingwolfboy may be able to find a few minutes to review as well.

@singingwolfboy

Copy link
Copy Markdown
Contributor

I haven't run the code, but the changes look fine to me. 👍

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

Thanks @singingwolfboy :)
@cpennington You're off the hook.

bradenmacdonald added a commit that referenced this pull request Jul 8, 2015
Fix hinted login view to be compatible with secondary providers.
@bradenmacdonald
bradenmacdonald merged commit d07a279 into openedx:master Jul 8, 2015
@bradenmacdonald
bradenmacdonald deleted the hinted-login-fix branch July 8, 2015 17:57
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.

5 participants