jazzband / django-fsm-log

Automatic logging for Django FSM
https://django-fsm-log.readthedocs.io/en/latest/
MIT License
242 stars 79 forks source link

Fix for django >= 4.0 makemigrations #184

Closed alcad closed 5 months ago

alcad commented 1 year ago

It's necessary to specify the default_auto_field param in apps.py to inhibit generation of migrations in package

alcad commented 1 year ago

This PR refers to yet well documented issue #182

ticosax commented 1 year ago

Will be solved only through https://github.com/jazzband/django-fsm-log/discussions/133 , I'm afraid.

alcad commented 1 year ago

Hi @ticosax You don't need an abstract model and cannot force user to extend your.

Imagine this use case: i create a new migration for fsm_log (even extending and mantaining in my project); all my new models and migrations depending on it will evolve during time. If you tomorrow you release a new migration in django_fsm_log altering AutoField bringing it to BigAutoField i will have a conflict in my db because cannot apply your migration; probably i can revert my migration, but what if my id cannot be reverted because i have a lot o records? I can revert fake my migration and apply fake your but you understand that is very boring.

The alternative is to migrate your model to BigAutoField and persist in apps.py so the migration is your and mantained in the package meanwile #133 will be well defined.

Let me know what you think please

Regards

ticosax commented 1 year ago

The changes brought by this PR, were already attempted and discussed.

https://github.com/jazzband/django-fsm-log/pull/132 https://github.com/jazzband/django-fsm-log/pull/138

I don't know if you already acknowledged them ?

alcad commented 1 year ago

Hi @ticosax i read all before pull request.

The reason for my PR is that the new django >= 4.0 default for new projects is BigAutoField so every new project started with this package create a migrations outside te boundaries of the project and request manual interventions to recover problematic situations like deploy on a server or sharing with colleagues (you cannot migrate nothing or generate migrations because of missing migrations outside project).

Can we al least imagine a fix for this specific use case?

Best regards

ticosax commented 1 year ago

Can we al least imagine a fix for this specific use case?

To my knowledge, the better, known today, approach to solve this issue, is captured in https://github.com/jazzband/django-fsm-log/discussions/133. Unfortunately it's not a easy & fast solution. Given we don't dedicate much time to it. I started a PoC in https://github.com/jazzband/django-fsm-log/pull/176. It's far to be finished, but it might be good enough, to get the ball rolling and push it to the finish line.

logginst commented 1 year ago

I'd also advocate for this change, over moving to AbstractModel. We ended up going with a monkey patch of the config class as a solution in the meantime:

class CustomDjangoFSMLogConfig(DjangoFSMLogAppConfig):
    """
    This Config class is meant to replace the AppConfig of the `django_fsm_log`
    third-party package. Since this app has not yet been updated to be compatible
    with the `default_auto_field` config setting introduced in Django 3.2, we
    get a warning related to uncreated migrations. This patch fixes this issue.
    """

    default_auto_field = "django.db.models.AutoField"

For what it's worth, this was also the proposed and accepted fix in some other third-party libraries that encountered the same issue. For example, https://github.com/jazzband/django-invitations/pull/211/files

lorenzomorandini commented 12 months ago

Given the current state of the project I agree with merging the PR. Moving to an abstract model doesn't seem worth it.

lorenzomorandini commented 8 months ago

@ticosax could we merge?

Vyko commented 8 months ago

Hi @ticosax, Generally speaking, thanks a lot for your work on this project ! Could we see this merged as it's an issue occurring since 2021 ? :pray: