michaeljohnbarr / django-timezone-utils

Time zone utilities for Django models.
MIT License
29 stars 9 forks source link

PRETTY_*_TIMEZONES_CHOICES is problematic with migrations #12

Open PiDelport opened 6 years ago

PiDelport commented 6 years ago

Currently, setting TimeZoneField's choices parameter to PRETTY_ALL_TIMEZONES_CHOICES or PRETTY_COMMON_TIMEZONES_CHOICES interacts pretty badly with Django migrations: every time any time zone's GMT offset changes, Django want to generate a new schema migration because of the differing choices list.

This is not very workable: we're facing a constant schema churn because of this, without any easy way around it (other than not using the PRETTY_*, but we want to display the offsets for user convenience).

Is there any reasonable way to avoid this problem somehow?

michaeljohnbarr commented 6 years ago

Just so that make sure that I understand, any time an offset changes or any time a new timezone is added/removed, a new migration would be added to update the choices on the field.

The ask is that the change of an offset does not trigger a migration/schema change, but that the addition/removal of a timezone would trigger a schema migration (since the display of the offset is literally just a display value and not a change of the stored value in the database)?

michaeljohnbarr commented 6 years ago

The only thing that I can think to do is to use the deconstruct() method on the custom fields to remove the choices from kwargs. Would this be acceptable?

def deconstruct(self):
    name, path, args, kwargs = super(TimeZoneField, self).deconstruct()
    kwargs.pop('choices', None)
    return name, path, args, kwargs

I don't have time right now to test this. However, if it works I definitely welcome a pull request.

There is already a deconstruct() method on the LinkedTZDateTimeField.

PiDelport commented 6 years ago

Hmm, this seems to be a slightly nuanced issue. There seems to be 3 possible directions:

  1. I've read upstream Django issues and discussions, and it seems choices gets tracked so data migrations have the right metadata to work with, and so on. However, having choices be omitted in deconstruct is an explicitly supported use case for fields that know the choices shouldn't affect the schema (which is arguably the case here?).

  2. There's also an open Django ticket to allow model field choices to be a callable (which would sidestep this problem, I presume?) but this is not implemented yet, so it's not a candidate solution yet.

  3. One possible alternative to futzing with the model field choices is to rework things to support having the choices just declared on the form fields? This would help indicate that they're more for display purposes only (especially with the pretty labels), rather than for actual data / schema purposes.

Do you have thoughts on the above?

michaeljohnbarr commented 6 years ago

@pjdelport

However, having choices be omitted in deconstruct is an explicitly supported use case for fields that know the choices shouldn't affect the schema (which is arguably the case here?).

I believe that your point above is the overall idea. Since choices is not currently callable, we cannot yet entertain this option (as you stated).

One possible alternative to futzing with the model field choices is to rework things to support having the choices just declared on the form fields? This would help indicate that they're more for display purposes only (especially with the pretty labels), rather than for actual data / schema purposes.

We definitely could add additional forms, but that's just more code to maintain down the line. My personal preference is the deconstruct() method, but I also am not necessarily biased between the two options. My thought is that the deconstruct() method exists which allows us to do what we are looking to do.

You are correct in saying that the forms would indicate they're more for display purposes. However, choices in general has two values; the left being the value to be stored, and the right being the display value, which is used in both the models and the forms. It's a tough call.

PiDelport commented 6 years ago

It's a tough call.

Would it perhaps make sense to parametrise this with a field keyword argument that toggles the behaviour, so that users can choose whether they want choices to be included in migrations or not?

This way, the current behaviour can remain the default, and users like me who would rather have the other behaviour can set the flag in the next release that includes it.

Surgo commented 6 years ago

@pjdelport You can edit migration file directory.

import timezone_utils.fields
from timezone_utils.choices import PRETTY_COMMON_TIMEZONES_CHOICES  # Replace all choices when only initial create

class Migration(migrations.Migration):

    initial = True

    dependencies = [
        ('auth', '0008_alter_user_username_max_length'),
    ]

    operations = [
        migrations.CreateModel(
            name='User',
            fields=[
                ('timezone', timezone_utils.fields.TimeZoneField(choices=PRETTY_COMMON_TIMEZONES_CHOICES, default='UTC', max_length=32, verbose_name='timezone')),
                ...
PiDelport commented 6 years ago

@Surgo: Oh, that could work in the mean time, thanks!

michaeljohnbarr commented 6 years ago

https://code.djangoproject.com/ticket/24561 is the ticket we need to track for implementation. Not much movement yet, I'm afraid.

lucasvazq commented 4 years ago

🕸