Take care to cast dates with empty strings to NULL in model managers

[jeancochrane]

Summary

While working on datamade/django-councilmatic-notifications#33, I noticed that if OCD BillActions or Events were saved with date values corresponding to empty strings, their date attributes would fail to be properly cast to DateTimeFields:

(councilmatic_models.BillAction.objects.annotate(test=Cast('date', models.DateTimeField()))
*** django.db.utils.DataError: invalid input syntax for type timestamp with time zone: ""

This PR adjusts the model managers implicated in django-councilmatic-notifications to appropriately cast empty strings to NULL.

Notes

@hancush pointed out that all of the date-related model attributes probably need to be handled in this way. That's a good point, but I'm not sure whether or not we should do it as part of this work. Since I don't have a good grasp of how pervasive this problem might be, I leave that decision up to others.

Testing instructions

I added tests to datamade/django-councilmatic-notifications#33 using these new null managers, so install this branch of django-councilmatic and run those tests to confirm tha they work.

[jeancochrane]

At first I thought that we might be able to set output_field to models.DateTimeField() and skip the enclosing Cast call altogether. Unfortunately, that doesn't work! Not entirely sure why, since output_field isn't really documented properly.

[hancush]

i've had trouble with that, too. boo!

[hancush]

thanks for taking this on, @jeancochrane. question re: pulling out repeat casting code inline!

[hancush]

we use this same annotation in a handful of places, and it seems like we may want to use it more in the future, if we decide to handle all datetimes. what if we defined a mixin with a helper method and included it in the custom managers that need this functionality?

class CastToDateTimeMixin:
    def cast_to_datetime(self, field):
        return Cast(
            Case(
                When(**{field: '', 'then': None}),
                default=field,
                output_field=models.CharField()
            ),
            models.DateTimeField()
        )

class EventManager(CastToDateTimeMixin, models.Manager):
    def get_queryset(self):
        return super().get_queryset().annotate(
            start_time=self.cast_to_datetime('start_date')
        )

[jeancochrane]

This sounds like a great idea! I'll make the change. What's the advantage of making it a mixin as opposed to a standalone function? Seems like it doesn't actually interface with the Manager methods or properties at all.

[hancush]

i've had trouble with that, too. boo!

[hancush]

i think it’s normal, even conventional, for a mixin to be agnostic of child classes. i like them because they’re a nice way to organize reusable functionality and i think they make the code more expressive, i.e., classes that require the functionality inherit from the mixin, making that “useful for...” more obvious from the code itself. if you’re interested, this from django expounds a lot better than i can on the benefits of mixin use (granted django has a much richer use case, heh): https://docs.djangoproject.com/en/2.2/topics/class-based-views/mixins/ in any case, it’s not a change i require, if it’s not one you want to make! i just like the mixin pattern, myself.

…

On Fri, Jul 12, 2019 at 5:06 PM Jean Cochrane ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In councilmatic_core/models.py <#253 (comment)> : > @@ -309,8 +309,16 @@ class Meta: class EventManager(models.Manager): def get_queryset(self): - return super().get_queryset().annotate(start_time=Cast('start_date', - models.DateTimeField())) + return super().get_queryset().annotate( + start_time=Cast( + Case( This sounds like a great idea! I'll make the change. What's the advantage of making it a mixin as opposed to a standalone function? Seems like it doesn't actually interface with the Manager methods or properties at all. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#253?email_source=notifications&email_token=AC44WLKS2MW3QXG5VBQNB43P7D55PA5CNFSM4ICQKYIKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB6LBFVY#discussion_r303163366>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC44WLI22NVUQKJR2YTYQJLP7D55PANCNFSM4ICQKYIA> .

[hancush]

p.s. could you use the new method in the existing membership manager, as well? 🙃

[hancush]

thank you so much for taking this on, @jeancochrane!

i don't think order makes a difference right now, but it will if the casttodatetime mixin (or another one we add later) overrides or extends the manager base class sometime in the future, since python does class hierarchy right to left. would you mind swapping the order in the managers where we include the mixin?

other than that, this looks great to me.