Reduce tight coupling in GLAMkit model relationships to permit easier customisation

[jmurty]

Look into reducing the tight coupling between models in GLAMkit, especially between more fundamental models (like EventBase) and plugins which should not necessarily be required.

At least, it should be possible to swap out model implementations for portable apps with alternative or derived custom versions. For example, to use a custom icekit_plugins_location.Location model in a project while retaining the FK relationship from EventBase.location.

Ideally even more fundamental models should be swappable without too much work. For example, having a custom derived Occurrence model in a project that adds fields or behaviour to the default occurrence, while keeping intact all the complex interactions in the EventBase-to-Occurrence relationship.

@markfinger suggests:

I wonder if we should replace all the FK invocations in icekit with something similar to Django's auth user, eg: a setting that produces an app/model label. This might help to remove the coupling between the various bits and pieces.

The trickiest part might be handling DB migrations in derived or custom models in a project, without the fairly awful fallback option of copying all existing migrations from GLAMkit into the project.

[jmurty]

This is related to #124

[Aramgutang]

@aweakley's commit afa9c02 is actually closely related to this.

He was using his own Location model, instead of plugins.location.Location, but he was not able to delete event objects (and hence unable to publish changes to events, because publishing involves deleting the published object and replacing it with the draft object).

The reason that was happening was because when the Django collector went looking through related models to delete, it bumped into plugins.location.LocationItem, which didn't actually exist within the project (since the app was removed from INSTALLED_APPS), and thus had no tables, causing an error.

The reason the related models included LocationItem was that plugins.location.models was getting imported, so the BaseModel metaclass code was executed for LocationItem, which registered it as a related object for events. And the thing importing the models was the URL include directive within icekit itself, which imported locations.urls, which in turn imported locations.views, which imported locations.models.

Alastair's commit makes the inclusions in the urlconf conditional on the relevant apps being present in INSTALLED_APPS. I think it might be relevant to this ticket.

[jmurty]

That's interesting @Aramgutang, thanks for the details.

So it looks like we need a way to make the URL includes more portable as well, or at least add guards to every call to include an app's URLs to first check whether that app is installed.

Perhaps a GLAMkit implementation of include like include_if_installed that does this extra check would be a cleaner way of doing it, and less likely to be forgotten?

(Though implementing such a thing without triggering any imports of the app that isn't installed might be difficult)

[Aramgutang]

Add guards to every include call is what we did, but I agree that an include_if_installed thing would be better. I think it should be implementable without imports. If not, it can just take the full app name as a second argument to check against INSTALLE_APPS.

[markfinger]

Regarding migrations of swappable models, it looks like the new migrations framework has some support

Given

from django.conf import settings

class SomeModel(models.Model):
    user = models.ForeignKey(settings.AUTH_USER_MODEL)

The following migration is generated

class Migration(migrations.Migration):

    dependencies = [
        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
    ]

    operations = [
        migrations.CreateModel(
            name='SomeModel',
            fields=[
                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
                ('user', models.ForeignKey(to=settings.AUTH_USER_MODEL),
            ],
        ),
    ]

This is generated from https://github.com/django/django/blob/0ec0e5029ca5c499eac93787c3073c3e716b5951/django/db/migrations/writer.py#L156-L164 which appears to look for any string references derived from a settings lookup.