Skip to content

Skip login after registration (mostly for NYIF) #174

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.

Sign up for GitHub

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

Merged
tkeemon merged 5 commits into from
Oct 19, 2017
Merged

Skip login after registration (mostly for NYIF) #174

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a05f9fa
try to import user-org backend and create obj at registration time
tkeemon Oct 18, 2017
6b25443
bypass auto login after registration with APPSEMBLER_FEATURE flag
tkeemon Oct 18, 2017
425efc5
conditionally import our custom org middleware
tkeemon Oct 18, 2017
0dbd8c5
fix formatting; include exception type in try/except
tkeemon Oct 19, 2017
e8fd8a0
remove unnecessary try/except
tkeemon Oct 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions common/djangoapps/student/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@
from openedx.core.djangoapps.user_api.preferences import api as preferences_api
from openedx.core.djangoapps.catalog.utils import get_programs_data

# try to import appsembler fork of edx-organizations (if it's installed)
try:
from organizations.models import UserOrganizationMapping
except ImportError:
pass

log = logging.getLogger("edx.student")
AUDIT_LOG = logging.getLogger("audit")
Expand Down Expand Up @@ -1857,13 +1862,24 @@ def create_account_with_params(request, params):
else:
registration.activate()
_enroll_user_in_pending_courses(user) # Enroll student in any pending courses

#if using custom Appsembler backend from edx-organizations
if u'organizations.backends.OrganizationMemberBackend' in settings.AUTHENTICATION_BACKENDS:
try:
organization = request.site.organizations.first()
if organization:
UserOrganizationMapping.objects.get_or_create(user=user, organization=organization, is_active=False)

@OmarIthawi OmarIthawi Oct 19, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks much better. I'd like to take it further:

Why not completely avoid the try/except? I think having if u'organizations.backends.OrganizationMemberBackend' in settings.AUTHENTICATION_BACKENDS: makes it safe to assume that all the rest will go well:

if u'organizations.backends.OrganizationMemberBackend' in settings.AUTHENTICATION_BACKENDS:
  organization = request.site.organizations.first()
  UserOrganizationMapping.objects.get_or_create(user=user, organization=organization, is_active=False)

Do we expect that organization = request.site.organizations.first() to be None? If so, let's keep the if organization: part.

@tkeemon tkeemon Oct 19, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's a good point. I'll remove the try/except. I do expect organization = request.site.organizations.first() to return None when the site in question is the default site, so I'm going to leave that last if statement in there.

Making that change and merging.

except:

@OmarIthawi OmarIthawi Oct 18, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

except: -> except NameError: to ensure that anything other than missing UserOrganizationMapping being catched (well, at least most of the errors).

@OmarIthawi OmarIthawi Oct 18, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@tkeemon don't take my word for it. Please double check whether this works or not.

Generally it's advisable to avoid except: and except Exception: whenever possible.

@tkeemon tkeemon Oct 19, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@OmarIthawi thanks for catching this. This is definitely a bad habit of mine.

Take a look and let me know if this addresses your concern.

pass

# Immediately after a user creates an account, we log them in. They are only
# logged in until they close the browser. They can't log in again until they click
# the activation link from the email.
new_user = authenticate(username=user.username, password=params['password'])
login(request, new_user)
request.session.set_expiry(0)

if not settings.APPSEMBLER_FEATURES.get('SKIP_LOGIN_AFTER_REGISTRATION',False):

@OmarIthawi OmarIthawi Oct 18, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

...ION',False) -> ...ION', False) space after comma.

@tkeemon tkeemon Oct 19, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

login(request, new_user)
request.session.set_expiry(0)

try:
record_registration_attributions(request, new_user)
Expand Down
11 changes: 11 additions & 0 deletions lms/envs/aws_appsembler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,3 +121,14 @@
if 'LMS_AUTHENTICATION_BACKENDS' in APPSEMBLER_FEATURES.keys():
#default behavior is to replace the existing backends with those in APPSEMBLER_FEATURES
AUTHENTICATION_BACKENDS = tuple(APPSEMBLER_FEATURES['LMS_AUTHENTICATION_BACKENDS'])

#attempt to import model from our custom fork of edx-organizations
# if it works, then also add the middleware
try:
from organizations.models import UserOrganizationMapping
MIDDLEWARE_CLASSES += (
'organizations.middleware.OrganizationMiddleware',
)
except ImportError:
pass

12 changes: 12 additions & 0 deletions lms/envs/devstack_appsembler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,15 @@
if 'LMS_AUTHENTICATION_BACKENDS' in APPSEMBLER_FEATURES.keys():
#default behavior is to replace the existing backends with those in APPSEMBLER_FEATURES
AUTHENTICATION_BACKENDS = tuple(APPSEMBLER_FEATURES['LMS_AUTHENTICATION_BACKENDS'])

#attempt to import model from our custom fork of edx-organizations

@OmarIthawi OmarIthawi Oct 18, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not specific to this PR at all. But we do need to DRY in the envs/*_appsembler.py files.

@tkeemon tkeemon Oct 19, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed. And @johnbaldwin has raised this same point as well:

https://trello.com/c/wfj1jPTd/98-dry-up-awsappsembler-and-devstackappsembler-py-files

That's what I was attempting to do with the appsembler.py file. Initially, this file was supposed to be imported by both aws_appsembler.py and devstack_appsembler.py, so settings could be shared across both of production and devstack deployments.

But with this setup we were running into some problems. I can't remember specifically, but I think it was related to the fact that this file was called without importing common.py, so variables like INSTALLED_APPS didn't yet exist. And if we defined INSTALLED_APPS in appsembler.py, then it would nuke all of the values from common.py and aws.py when imported into aws_appsembler.py.

We can definitely do this, we just need to be careful about how we structure the imports.

# if it works, then also add the middleware
try:
from organizations.models import UserOrganizationMapping
MIDDLEWARE_CLASSES += (
'organizations.middleware.OrganizationMiddleware',
)
except ImportError:
pass