Make the models swappable

[lgarvey]

This PR follows the pattern used in django-oauth2-toolkit - and is suggested in #90

I have a need to extend the ServiceProvider model to add some additional features so I thought I'd raise a PR to add swappable functionality. I can also add some documentation if required.

I assume unit tests aren't required as it's leveraging the swappable api in django?

[lgarvey]

Hi @mhindery I've squashed the previous migrations and used the squashed version to apply the swappable logic. So that existing installations should still function without any issues.

Doing some manual testing the PR works with:

an existing installation of djangosaml2idp with previous migrations applied
a new installation of djangosaml2idp without overridden models
a new installation of djangosaml2idp with overridden models (will do some more testing as currently I have only overridden the ServiceProvider model)

I haven't looked at the admin system - but typically this involves unregistering and re-registering the admin models in the overriding app.

It'd be good to gauge whether you'd be interested in merging the PR as I'm currently building some functionality that depends on it. Essentially I need to add some extra fields to the ServiceProvider model

[Amertz08]

Shouldn't this be from django.conf import settings? that way you're actually importing the settings proxy object.

[Amertz08]

Nevermind. I looked back over the Oauth toolkit implementation and it looks like you're doing it the same way.

[Amertz08]

I don't see any issues with this.

[mhindery]

Hi @lgarvey , this is a great addition, I'm definitely happy to merge this in.

[mhindery]

Ideally this gets some documentation added as well, but that is something I can add later. Let me know if you are done and not plan any more changes in the PR, then I'll merge it.

[lgarvey]

It all seems good from this end - one very minor point:

If overriding a model, the migration for the overridden model has to run before the migrations for djangosaml2idp or else there'll be an error. The django-oauth-toolkit advises to put a run_before statement into your overriding migration to ensure they are executed in the correct order, e.g

class Migration(migrations.Migration):

    run_before = [
        ('djangosaml2idp', '0001_initial_squashed_0002_persistent_id_swappable_models'),
    ]

I think the name of the squashed (and swappable) migration should be something less verbose, e.g: 0001_initial_swappable.py

But aside from that everything seems good.

[lgarvey]

This seems all good to merge now.

[farzeni]

Hello,

Is there any reason why this PR has not been merged in upstream?