Skip to content

Conversation

@johncox-google
Copy link
Contributor

@mgainer for detailed review
@cpennington, @nedbat for edx-specific review
@marcotuts for UX (see below for 2 caveats where I can use your help)

This PR adds new account creation functionality. @marcotuts has seen a demo and approved the UX with two caveats:

  1. We need to finalize the language in the password field.
  2. We need to finalize the HTML/CSS for the provider list (buttons rather than numbered choices).

Also, integration/e2e tests aren't written yet. I wanted to finalize the implementation before burning in the tests. We don't want to merge until tests are done.

Also not done are signing in to existing accounts via a third party, and managing account associations. Those will come in later PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

should be _("Sign in with {provider_name}").format(provider_name=enabled.NAME), maybe with a translator note, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added .format(); didn't add translator note yet since I'll be replacing this HTML with some buttons once @marcotuts lets me know what CSS to use.

@nedbat
Copy link
Contributor

nedbat commented Mar 20, 2014

Thanks for the continued progress!

  1. You should add yourself to the AUTHORS file, btw.
  2. I don't know @mgainer, what is his role here?

@johncox-google
Copy link
Contributor Author

@mgainer is one of the engineers on my team. I asked him for a review from a general engineering perspective to help me iterate quickly. Obviously we're deferring to you for final review.

I'll address the AUTHORS and translation comments in a follow-up. Any other comments before I move on?

@johncox-google
Copy link
Contributor Author

AUTHORS updated.

Copy link

Choose a reason for hiding this comment

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

and and

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@nedbat
Copy link
Contributor

nedbat commented Mar 21, 2014

I can understand updating AUTHORS in a new pull request (though there's no need), but why wait to fix the translations? UPDATED: I see you've done the AUTHORS and translations. Thanks!

I will be doing a review also in the next day or so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pylint pragmas are best when they don't interrupt the reading flow: put them on the same line as the import, and no need to explain them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment removed; suppression inlined.

I like to document suppressions because I find their intent easy to understand when I'm writing them but basically opaque when I'm reading them a few weeks later.

@johncox-google
Copy link
Contributor Author

@jbau, @alexmdavis, @version101, @AnuragRamdasan: just tagging you on this to keep you in the loop -- nothing you need to do.

@johncox-google
Copy link
Contributor Author

@nedbat, @mgainer: thanks for the review! Looks like we're all in agreement on all major points. I'll resolve comments and push a new version today.

@nedbat, I could use your advice on testing this. Do you have half an hour today or tomorrow to do a VC?

@jzoldak
Copy link
Contributor

jzoldak commented Apr 23, 2014

@johncox-google all set running in devstack. Couple minor points from the instructions above:
1a) after flipping ENABLE_THIRD_PARTY_AUTH bool in lms/envs/common.py, you need to syncdb and migrate
2) The secrets and keys need to go in /edx/app/edxapp/lms.auth.json (rather than lms.envs.json)

@johncox-google
Copy link
Contributor Author

@jzoldak Yeah, I typod my instructions, sorry. The docs in settings.py are correct, though, and the docs in common.py mention the syncdb.

@johncox-google
Copy link
Contributor Author

@jzoldak Re: the comment: two things:

  1. This PR does not contain the full feature set (see https://github.com/edx/edx-platform/pull/3408 for the remainder).
  2. The agreed-upon plan is to leave the bool off during these PRs and let you folks flip it in a follow-up if/when you're comfortable with enabling the feature by default.

@johncox-google
Copy link
Contributor Author

@jzoldak Oh, and:

  1. Depending on your preferences it may make sense to leave the bool off forever since the feature doesn't work without providers, and many providers will require deployment-specific configuration that you can't check into edx-platform.

@jzoldak
Copy link
Contributor

jzoldak commented Apr 23, 2014

👍 for code & tests.
@nedbat do you want to do a final code review?
@marcotuts do you want to follow up on styling in the next PR? Are you OK with the wording. E.g. "or, if you have connected one of these providers, log in below." IIRC, once we merge this in and send it up to transifex, if we decide to change it later it will need to be retranslated in other languages. So it'd be best to settle now.

@johncox-google
Copy link
Contributor Author

@jzoldak Thanks!

@johncox-google
Copy link
Contributor Author

@nedbat, @jzoldak, @marcotuts: had some spare time today so I created https://github.com/edx/edx-platform/pull/3450, which switches the signin control from a text list to buttons. @marcotuts, could you have a look? If you like the UX (and haven't already done this) I'd like to merge that PR this week along with this one and https://github.com/edx/edx-platform/pull/3408.

@jzoldak
Copy link
Contributor

jzoldak commented Apr 24, 2014

@johncox-google it looks like the branch needs a rebase. Also we need to squash the commits before merging.

@johncox-google
Copy link
Contributor Author

@jzoldak Yup, I'll squash and rebase once I've got the final word from @nedbat that there are no other change requests.

@johncox-google
Copy link
Contributor Author

OK, merged as part of https://github.com/edx/edx-platform/pull/3450. Thanks, all (and shout out to @nedbat for all his help with the final merge).

@johncox-google johncox-google deleted the johncox/feature/auth branch April 25, 2014 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants