UNIFICATION: get_current_organization don't assume single Organization

[bryanlandia]

Return the first instead to keep from failing outside of Tahoe SaaS
when we may have more than one Organization per Site

I know this needs tests. but putting this out there for discussion. It's the one known blocker I have for using the Tahoe edx-platform branch without changes for DHIS2 standalone. DHIS2 uses multiple course orgs. They and PSU and other enterprise customers use the course org to show who is offering / who created the course; for DHIS2, it's various different NGO partners; for PSU, it's various colleges at the university.

Obviously, it's a function that's very central to how Tahoe handles multitenancy and user management by Org.

[OmarIthawi]

@bryanlandia Thanks for jumping into this. This is a hairy subject 😅 and we honestly still haven't figured out how to do Tahoe orgs/site link properly 🤷🏼‍♂️

The .get() was done so it fails explicitly, which was a way to ensure our sites/orgs are properly linked and avoid None has no properly short_name coming out of nowhere.

I understand this is meant for Tahoe Enterprise setup, so this shouldn't make it into Tahoe at all, otherwise it would cause some errors that are hard to debug especially in the context of Multi-Tenant Emails i.e. make our lives a bit harder.

In such cases my suggestion is to use feature flags as explained in the code. Feature flags are not free and they cost a lot of maintenance cost and some features don't work at unless in certain combinations. Nevertheless, I find it more affordable than having separate branches.

[OmarIthawi]

In general we'd like to keep SaaS/Enterprise veery close together. So if it's a must, we can do something like this:

Suggested change

	current_org = current_site.organizations.first()
	if settings.FEATURES['TAHOE_ENABLE_MULTI_ORGS_PER_SITE']:
	current_org = current_site.organizations.first()
	if not current_org:
	raise Organization.DoesNotExist('TAHOE_ENABLE_MULTI_ORGS_PER_SITE: Could not find current organization for site `{}`'.format(repr(current_site))
	else:
	current_org = current_site.organizations.get()

Also, it needs to be tested so this code needs probably two tests in order to be covered.

[bryanlandia]

Thanks for the review @OmarIthawi . I was thinking more about it and agree we should try with a feature flag. The major goal here is definitely to avoid having separate branches. That's the whole reason we are going down the more difficult road of using the Tahoe branch for DHIS2's Hawthorn upgrade. I'll update the PR with tests and feature flag approach.

[OmarIthawi]

Thanks Bryan! This is exciting. In theory it could be a single feature which says TAHOE_STANDALONE_MODE: True that enables a bunch of features, but I don't prefer huge features, so it could even be something like TAHOE_STANDALONE_MULTI_ORGS_PER_SITE: True to denote the context of this feature should be used only in STANDALONE sites.

I've been a bit lax on having large feature flag names, you're welcome to suggest a better one 😃

[bryanlandia]

I actually like the simpler one you suggested TAHOE_ENABLE_MULTI_ORGS_PER_SITE and agree we shouldn't do a big THIS IS STANDALONE switch to enable a bunch of things. I think I'll have some updated code with tests later today.

[bryanlandia]

Can't import directly from factory. This was breaking tests

[bryanlandia]

@OmarIthawi I'm taking this out of Draft as I've added (passing) tests

[bryanlandia]

Huh... somehow I messed up rebasing and lost a commit that would fix what's breaking in Travis. Moving back to Draft until fixed.

[OmarIthawi]

LGTM! Thanks @bryanlandia!

[johnbaldwin]

Just to note that this will fail with a MultipleObjectsReturned if there happen to be multiple orgs for a site in the deployment that has TAHOE_ENABLE_MULTI_ORGS_PER_SITE=True So we probably want to log this somewhere. Would it get automatically thrown to Sentry?

[bryanlandia]

I think you mean if TAHOE_ENABLE_MULTI_ORGS_PER_SITE=False. Agreed, would probably be good to catch that and generally log the exceptions.