bennylope / django-organizations

:couple: Multi-user accounts for Django projects
http://django-organizations.readthedocs.org/
BSD 2-Clause "Simplified" License
1.31k stars 212 forks source link

Update supported Django 3.2 and 4.x testing with supported Python versions #236

Closed mgrdcm closed 2 years ago

mgrdcm commented 2 years ago
bennylope commented 2 years ago

🥹🙏

Though at this point I'd say it's safe to remove support for Django 2.2 going forward.

And maybe even Python 3.6 regardless of Django 3.2 support.

mgrdcm commented 2 years ago

You're welcome @bennylope and thanks again for this package! I'd think it makes sense to go ahead and get https://github.com/bennylope/django-organizations/pull/237 merged into this main version, and then maybe a Django-organizations version 3 that officially drops support for older versions (Django 2.2, Python 3.6) and truly adds support for Django 4? Or am I being too pedantic, it's your repo :)

bennylope commented 2 years ago

and then maybe a Django-organizations version 3 that officially drops support for older versions (Django 2.2, Python 3.6) and truly adds support for Django 4? Or am I being too pedantic, it's your repo :)

Not over stepping at all, that's an excellent idea, or at least another minor version for the breaking change removing support for older versions.

mgrdcm commented 2 years ago

So with the new useless migrations caused by Django 4's new migration detection, all tests pass. Surprisingly because they'll get applied in Django <4 as well I think because the dependency migrations aren't new they were all there back in 3.2 even. I'll do some testing with my application in 3.2 tonight and then move this out of Draft if it seems that applying those back in 3.2 don't cause any issues. I think it'll be fine but not sure if the automated tests here would catch an issue.

mgrdcm commented 2 years ago

BTW I noticed this and need it fixed here in upstream because when I have some custom Django-organizations models in my app and when I made migrations it was generating migrations IN THE INSTALLED django-organizations directories with a dependency on my application. Hopefully this will keep that from happening.

bennylope commented 2 years ago

🌮🌮🌮

I noticed those issues too when I ran tests locally and will be happy to get this PR landed.

mgrdcm commented 2 years ago

OK I'm a little stumped. Does look like things will be OK in Django 3.2, but in Django 4 when I do makemigrations it's still plopping one new migration into the installed django-organizations directory called 0006_alter_organization_slug.py:

# Generated by Django 4.0.7 on 2022-09-30 22:17

from django.db import migrations
import organizations.fields

class Migration(migrations.Migration):

    dependencies = [
        ('organizations', '0005_alter_organization_users_and_more'),
    ]

    operations = [
        migrations.AlterField(
            model_name='organization',
            name='slug',
            field=organizations.fields.SlugField(blank=True, editable=False, help_text='The name in all lowercase, suitable for URL identification', max_length=200, populate_from='name', unique=True),
        ),
    ]

It's almost like it's trying to take it back to where it was as of 0002_model_update, but it does look like 0003... was applied in my application so not sure what's going on here.

UPDATE: @bennylope looks like this might be related to the issues solved in https://github.com/bennylope/django-organizations/releases/tag/1.1.0 maybe you have an idea?

mgrdcm commented 2 years ago

@bennylope You happen to have any thoughts about ^ issue?

bennylope commented 2 years ago

The one thing I'd add right now is that I saw the same thing and was likewise stumped. I haven't yet had a chance since then to dig in.

mgrdcm commented 2 years ago

Oh boy. This is a doozy. I hope you're sitting down (it's not bad just there are two options and I'm too tired to form an opinion on which is right).

So the reason is that when the autoslugfield got switched from the django-autoslug one to the django-extensions one in most of the project (the abstract model), the manage.py which is used to generate migrations didn't get updated and the config test version didn't get updated either.

Neither of which mattered pre Django4. Because the config test didn't get updated to use the new one, the clever "fail if makemigrations would make a migration" test you had unfortunately didn't catch that migrations hadn't been run :). So in the PRs (yes, multiple here comes the fun) I did update the config test and sure enough it catches that the migrations didn't run. Yay! And I can run make migrations and it makes the pointless migrations that Django4 needs. Yay! And then after doing that the migrations test passes. Yay yay!

BUT there's a catch. The Django-extensions auto slug newer than 2.0.7 will overwrite the explicit slug with the implicit one from the populate_from parameter. Which is different from how older versions of it and all versions of the other auto slug field work. So tests break. So we either have to change the tests themselves, or change how the slug field works so the existing tests work. Hence the two PRs (against this PR) that solve it in each way:

https://github.com/mgrdcm/django-organizations/pull/1 https://github.com/mgrdcm/django-organizations/pull/2

Do you have an opinion on which one to go with? If a consumer of this library has been manually overriding the old django-autoslug version, both are the same to them. But if they're using the default extensions version (probably everyone) the second version could change how their application works. But it also then works more like it's supposed to I think. So it's a bit of a tossup. I suppose the first version is probably safer, but perpetuates the slightly broken functionality.

Sorry this is rambling. Hopefully it explains things, but if not I let me know and I can clean it up in the morning :) Once we pick one I'll merge it into this PR and then the whole thing can be reviewed/merged in full.

mgrdcm commented 2 years ago

After sleeping on it, I think https://github.com/mgrdcm/django-organizations/pull/1 is probably the best way to solve this for right now as it'll have the least impact on existing users. I think if https://github.com/mgrdcm/django-organizations/pull/2 was used it would require a breaking change / major version bump since it would change how consuming applications behave slightly. Sound good @bennylope?

mgrdcm commented 2 years ago

Went with #1 and calling this review ready. I think at some point should consider the changes in https://github.com/mgrdcm/django-organizations/pull/2 but this seemed least impactful way to handle.

bennylope commented 2 years ago

I'm going to try to take a look (and merge, ideally) tomorrow - thank you for putting this together.

mgrdcm commented 2 years ago

You're welcome! Thanks so much for django-organizations! Let me know if you have any questions or if I should change anything.

mgrdcm commented 2 years ago

Added testing for Python 3.11-dev (3.11 isn't installable yet via the GH action but this is a start at least)

mgrdcm commented 2 years ago

OK now it supports installing 3.11 so switched to that. I know Django doesn't officially support 3.11 yet, but looks like it will in 4.1 soon: https://code.djangoproject.com/ticket/33173#comment:13

mgrdcm commented 2 years ago

Woo thank you @bennylope! What needs to happen for a release with all this to go out? Anything I can help with?

bennylope commented 2 years ago

Thanks for putting in the time to pull this altogether. I just bumped the version, the only thing this needs prior to release is an updated entry in the changelog in HISTORY.rst to add what changed for the soon-to-be 2.1.0 version.

mgrdcm commented 2 years ago

I think this covers it all @bennylope: https://github.com/bennylope/django-organizations/pull/242