stephenmcd / django-forms-builder

Let users build forms in Django admin
BSD 2-Clause "Simplified" License
691 stars 281 forks source link

FORMS_BUILDER_EXTRA_FIELDS creates Django Migration #220

Closed tdsymonds closed 6 years ago

tdsymonds commented 6 years ago

The issue I'm going to describe below appears to have been a problem since Django added their migrations in 1.7, however I haven't confirmed this, and I'm using 1.11.

If you create a new project in Django, install django-forms-builder and run the migrations the project all works fine as expected. However, if you then add FORMS_BUILDER_EXTRA_FIELDS to the settings, like the example in the README:

FORMS_BUILDER_EXTRA_FIELDS = (
    (100, "django.forms.BooleanField", "My cool checkbox"),
)

Django will explain that you need to run makemigrations, and if you do, then a new migration is created in the pip package's migrations folder, outside of your project.

This is because Django creates a migration for any change to the choices attribute even though this doesn't change the db schema.

I've found a Django ticket #23586 where somebody explains the problem, but Django don't seem to have acknowledged this. From what I've read else where online, it appears that this is by design.

This creates a problem, because having a migration outside of your project on your local machine seems like a gateway for problems further down the line. You can simply ignore the suggestion by Django, and run ./manage.py makemigrations [app] for any future changes you make to your project, but this isn't ideal and opens up a gateway for mistakes to happen and problems to come later.

This is a really nice feature to have, but unless you're still using South for migrations it seems to be a gateway for potential issues.

tdsymonds commented 6 years ago

The Django ticket I reference above was closed as a duplicate, and in the other ticket's thread it led me to the solution. The migration needs to point to the choices in the settings, not have them hard coded. I've resolved this in #221.