Missing migrations

[JackAtOmenApps]

Bug Report

Expected behavior/code
After installing marion, all migrations should be pre-existing and ready to run via python manage.py migrate.

Actual Behavior
Performing a migration dry-run produces a migration file for marion:

Migrations for 'marion':
  /usr/local/lib/python3.9/site-packages/marion/migrations/0002_alter_documentrequest_issuer.py
    - Alter field issuer on documentrequest
Full migrations file '0002_alter_documentrequest_issuer.py':
# Generated by Django 3.2.4 on 2021-06-13 02:18

from django.db import migrations, models


class Migration(migrations.Migration):

    dependencies = [
        ('marion', '0001_initial'),
    ]

    operations = [
        migrations.AlterField(
            model_name='documentrequest',
            name='issuer',
            field=models.CharField(choices=[('marion.issuers.DummyDocument', 'Dummy')], help_text='The issuer of the document among allowed ones', max_length=200, verbose_name='Issuer'),
        ),
    ]

Steps to Reproduce

1. python manage.py makemigrations --dry-run -v 3
2. The above output is produced

Environment

• Marion version: 0.3.0
• Platform: Ubuntu 20.04 with django 3.2

Possible Solution
Add the new migration in the marion repo

[JackAtOmenApps]

The above example obviously changes (but is still created) after installing my own document issuers.

Maybe I am misunderstanding something. I see in the docs:

Note that modifying this setting requires to create a new database migration as this will change choices of the DocumentRequest.issuer field.

But won't the creation of new migrations in the marion app folder itself result in the inability to apply new migrations from this project if the models.py file is ever updated in future releases? Any future changes to the models.py in this package will not know about the migration changes I've made via adding new issuers, right?

[jmaupetit]

Hi @OmenApps 👋 Thanks for the report! 🙏

You made the point: this is clearly a weakness... We have to come with a relevant solution to avoid messing projects with conflicting migrations. I need to brainstorm with the team to propose a fix. I'll keep you posted.

[JackAtOmenApps]

Since issuer choices are dynamic, and choices should not be dynamic, shouldn't they be stored as a separate model within marion?

A note from (https://docs.djangoproject.com/en/dev/ref/models/fields/#choices) says: Note that choices can be any sequence object – not necessarily a list or tuple. This lets you construct choices dynamically. But if you find yourself hacking choices to be dynamic, you’re probably better off using a proper database table with a ForeignKey. choices is meant for static data that doesn’t change much, if ever.

While less elegant than your current approach, a 'correct' approach might be something like the following.

---

In defaults.py, change from:

DOCUMENT_ISSUER_CHOICES_CLASS = getattr(
    settings,
    "MARION_DOCUMENT_ISSUER_CHOICES_CLASS",
    "marion.defaults.DocumentIssuerChoices",
)

to:

DOCUMENT_ISSUER_CHOICES = getattr(
    settings,
    "MARION_DOCUMENT_ISSUER_CHOICES",
    [
        {"issuer_path": "marion.issuers.DummyDocument", "label": "Dummy"},
    ],
)

MARION_DOCUMENT_ISSUER_CHOICES in project settings should be set up like this:

MARION_DOCUMENT_ISSUER_CHOICES = [
    {"issuer_path": "apps.shop.issuers.invoice.InvoiceDocument", "label": "Invoice"},
    {"issuer_path": "apps.shop.issuers.statement.StatementDocument", "label": "Statement"},
]

Add new model to models.py:

class IssuerChoice(models.Model):
    issuer_path = models.TextField(
        _("Issuer Path"),
        help_text=_("Full dot-notation path to the issuer class. e.g.: 'apps.shop.issuers.invoice.InvoiceDocument'")
    )
    label = models.CharField(_("Label"), max_length=100)

Update the issuer field in DocumentRequest model

    issuer = models.ForeignKey(
        IssuerChoice,
        verbose_name=_("Issuer"),
        on_delete=models.CASCADE,
        help_text=_("The issuer of the document among allowed ones"),
    )

In apps.py, rather than directly using the ready method which is called on every management command (including tests) (https://docs.djangoproject.com/en/3.2/ref/applications/#django.apps.AppConfig.ready), indirectly use the post_migration signal to run only after migration (even if no new migration files were implemented):

"""AppConfig for the marion application"""

from django.apps import AppConfig
from django.db.models.signals import post_migrate

from .defaults import DOCUMENT_ISSUER_CHOICES


def create_issuer_objects(sender, **kwargs):
    from .models import IssuerChoice
    for issuer in DOCUMENT_ISSUER_CHOICES:
        IssuerChoice.objects.get_or_create(**issuer)


class DocumentsConfig(AppConfig):
    """Documents application configuration"""

    default = False
    name = "document"

    def ready(self):
        post_migrate.connect(create_issuer_objects, sender=self)

As for removing Issuer choices that are no longer valid, this could be accomplished in the create_issuer_objects function above (to be implemented), as a management command (to be implemented), or via the admin.

---

The drawbacks to this approach are:

• Using the db, which adds some overhead
• Inability to easily translate the label field
• Several additional changes in the codebase and tests would still be needed to account for the changes here, but it's a start.

Feel free to ignore all of this if you have something else in mind. It's just a suggestion. Hope it helps.

[JackAtOmenApps]

If this looks like the right way to go, I could get it fully functioning and submit a PR based on your developer contribution documents. Let me know if this is something that would be beneficial, or if you would like to go another direction with this issue.

[JackAtOmenApps]

Will have an initial PR for your review this evening. Hopefully this is something your team will find helpful. It's working great on my end, just cleaning things up a bit and modifying the docs.

[JackAtOmenApps]

#54 takes the approach noted above to provide a fix for this issue.

[jmaupetit]

Hi @OmenApps thank you again for your work on this. WDYT about @sampaccoud's comment? #54 (comment)