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

[jeancochrane]

Summary

While working on https://github.com/datamade/django-councilmatic-notifications/pull/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 https://github.com/datamade/django-councilmatic-notifications/pull/33 using these new null managers, so install this branch of django-councilmatic and run those tests to confirm tha they work.

[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 notifications@github.com wrote:

@jeancochrane commented on this pull request.

In councilmatic_core/models.py https://github.com/datamade/django-councilmatic/pull/253#discussion_r303163366 :

@@ -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 https://github.com/datamade/django-councilmatic/pull/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? 🙃