Enable combined logistration page by default #8600
Conversation
|
Thanks for the pull request, @bradenmacdonald! I've created OSPR-668 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here. |
|
Jenkins, test this please |
|
👍 |
|
@cpennington This is ready for your review. (It's small) |
|
ugh I have some concerns about this, as shown by the discussion here: https://openedx.atlassian.net/browse/OPEN-507 |
lms/envs/bok_choy.env.json
Outdated
There was a problem hiding this comment.
Is there an issue of just leaving this defined in test files?
There was a problem hiding this comment.
No. Since the default in envs/common.py would now be true, there's no need to override the default and set it true in the test file, so I removed this. But there'd be no harm in leaving it in, either.
There was a problem hiding this comment.
Yeah I guess I'm just wondering - it seems like the settings that tests depend on should all, always, be explicitly laid out in these files rather than implicitly relying on the values in common.py. If I run my own fork and I change values in common.py then try to run tests, I'll get unexpected, hard to trace failures.
There was a problem hiding this comment.
I guess I was naively hoping that we'd be completely removing this feature flag in the near future :-p
I will push a little reversal of the changes to the test envs so the flag is there explicitly. I think you make a good point.
|
From an engineering pov, 👍 modulo my questions. From an Open edX and UX pov, outstanding questions so please don't merge just yet. |
openedx-webhooks
added
open-source-contribution
|
@sarina I understand that; thanks for reviewing and for linking to the discussions. Note that some of the new SSO features only work with the combined logistration page, but from my perspective it is not critical to include this change in the release. That said, I do think the SSO work is a good excuse for switching everyone over, so we can start to remove the duplicate code paths, which would be really nice. |
|
While I 100% agree about the duplicate code paths (note that I filed the On Tue, Jun 23, 2015 at 11:19 AM, Braden MacDonald <notifications@github.com
|
|
Test failure is an unrelated buggy test with a hardcoded date that will always fail after June 22, 2015. |
|
Isn't a fix for that merged to master? On Tue, Jun 23, 2015 at 1:20 PM, Braden MacDonald notifications@github.com
|
|
Probably but this code is based on the feature branch and I haven't rebased it in a couple of days. |
|
@mhoeber this may have docs impact, fyi |
|
@bradenmacdonald and @sarina, thanks for the ping on this PR. The UX team has taken very light steps to make this initially edX-only and marketing-driven UI look more like overall edX UI and use elements that are leveraged in Open edX (e.g. buttons). So the thoughts captured in https://openedx.atlassian.net/browse/OPEN-507 are slightly diffused. If @ebporter has asked for this feature (and SSO functionality is built on top of this), I can understand the drive to get this merged. However, I would like the following items to be addressed by Beth as blockers or not, especially if this is getting into the official Cypress release:
@ebporter, I'll leave it in your hands to note if these concerns are blockers as well as if they should be addressed at all. |
|
Just a note of clarification: This only changes the default setting. Any instances/customizations/themes that depend on the old version will continue to work fine as long as they change the setting locally to use the old pages. So it is fully backwards compatible, and it doesn't remove any code. |
|
@bradenmacdonald Why must this be the default? I am concerned about On Thu, Jun 25, 2015 at 4:03 PM, Braden MacDonald notifications@github.com
|
|
@sarina I am 100% fine either way. If it's looking like too much hassle to do this now and everyone is fine with it, we can just close this PR, and just make sure that DevOps switches Edge to the new combined page. There is a concern that new Open edX users who want to use SSO will find it confusing that they have to change the login page first, so we'll have to make that clear in the docs. The code is very unambiguous with deprecation warnings all over the old login/register views, but I'm not sure that the choice is as clear to people setting up instances. |
|
Thanks for the 411 and discussion, @bradenmacdonald and @sarina. I'd say which version to lead with is @ebporter's call. |
|
@talbs this PR would make it the default for Edge, devstack, and all Open On Thu, Jun 25, 2015 at 5:52 PM, Brian Talbot notifications@github.com
|
Hmm, yeah, I'm not sure I can make that call (FWIW, I don't think we're ready for this to be the default in all of those places). @sarina, this sounds like a Beth (@ebporter) call. Can you get her to weigh in tomorrow while I'm OOO? I'm definitely very comfortable with the option to set up Edge this way while figuring out a plan for the rest of the environments noted above. |
|
Hey there, @wedaly pinged me on this PR. I'd like to try this branch out locally and see if there might be any impact on our existing white label sites in edX production servers which do not use the new logistration page (because our customer wants enhanced registration fields). My sense is that this will break our production environment. Please don't merge this until I can check it out. I suspect there will have to be an additional small code change and a change to edX's production configuration. I believe I understand there's a bit of time urgency, so I'll get back to everyone by EOD tomorrow. |
|
OK, I noticed @sarina's comment that this would be turned on - via production configuration - for Edge (not edx.org deployment). Then we wouldn't impact our White Label customers, but it'd probably still be good to get any necessary code changes (e.g. using microsite.get_value(), etc.) before it gets merged in, otherwise we might forget to do so in the future. |
|
@bradenmacdonald @chrisndodge @talbs I just spoke about this with Beth and we've decided for now, we're going to turn this on just for edge. After Cypress is cut we can discuss turning this on for everyone by default. Before we do that we need to make sure that works with white label settings, per Chris' concerns. Additionally we may choose to do this after the ongoing UX audit is completed and we have a chance to clean up the stylings a bit. We are in agreement we'd like to turn this on by default before the Dogwood release. Does that work for everyone? If so I'm going to close this PR for now and we can pick it back up in a few weeks. |
|
@sarina Works for me; please go ahead and close this. |
Description:
This simple change sets
ENABLE_COMBINED_LOGIN_REGISTRATIONto beTrueby default in all environments. Beth had asked me to do this as part of the SSO work.This is part of the Shibboleth/SSO work and is being merged to the shibboleth-tpa feature branch, which is due to be merged to master ASAP.
Compatibility / Notes: People can always override the feature flag locally, if they don't want the new page yet. The new page is required for many of the new Shibboleth UX enhancements, though the basic SAML/Shibboleth login support should work with the old as well as the new pages.
Sandbox: https://courses.stage.edx.org/login :-p
JIRA tickets / Discussions: OPEN-507