Skip login after registration (mostly for NYIF)

[tkeemon]

I've added two major changes to the account registration workflow here:

1. Try to import a custom model from our fork of edx-organizations, then create an object from that model if the account registration happens within a microsite.
2. Add a new flag APPSEMBLER_FEATURES['SKIP_LOGIN_AFTER_REGISTRATION']. If set to True, users aren't automatically redirected to /dashboard after creating a new account.

The default behavior for each change is:

1. If edx-organizations isn't installed, then the import fails gracefully.
2. If SKIP_LOGIN_AFTER_REGISTRATION doesn't exist, then the normal registration workflow takes place and the user is logged in and redirected to /dashboard after registration.

Related commits:
https://github.com/noderabbit-team/edx-configs/commit/6df426793ef349659e503bf05aea95a9ed6d19d7

[tkeemon]

(6) NYIF: test CCA workflows for Ficus upgrade

[OmarIthawi]

Looks good to me. Only two changes to go!

[OmarIthawi]

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

[OmarIthawi]

@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]

@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.

[OmarIthawi]

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

[tkeemon]

Done

[OmarIthawi]

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

[tkeemon]

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.

[OmarIthawi]

@tkeemon somewhere the PEP8 there's a good practice:

Additionally, for all try/except clauses, limit the try clause to the absolute minimum amount of code necessary. Again, this avoids masking bugs.

Yes:

try:
    value = collection[key]
except KeyError:
    return key_not_found(key)
else:
    return handle_value(value)

No:

try:
    # Too broad!
    return handle_value(collection[key])
except KeyError:
    # Will also catch KeyError raised by handle_value()
    return key_not_found(key)

[OmarIthawi]

@tkeemon One additional read about the except: thingy:

• The Most Diabolical Python Antipattern

[OmarIthawi]

One suggestion and we're good to!

[OmarIthawi]

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]

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.