Review of DRF rewrite

[tedmiston]

DRF rewrite looks good to me.

I don't have any major model changes I would make.

If these points below seem worthwhile, let's break them off into separate issues or just do them tomorrow.

Apps

I know it's a first pass but only comment I have now is it could break up nicely into a few Django apps instead of one:

1. core (meal, ingredient, allergen, dietary filter, user?)
2. orders (order, address)
3. reviews

A few more (minor) things...

Models

I think we could replace the Image model's URLField with ImageField and using django-storages backed by S3.

Let's make a UUIDModelMixin since we're using UUID PKs everywhere.

I think Ingredient.name should be unique.

For Meal.portions* there is a PositiveIntegerField, or even better PositiveSmallIntegerField (good from 0 to 2^15).

For Order, I feel like we're going to have to store a Stripe transaction ID or something from the front end for reference purposes. Also transaction price for analytics.

Also let's consider using TimeStampedModel from django-model-utils for created at type fields, like Review.posted_at.

https://django-model-utils.readthedocs.org/en/latest/models.html#timestampedmodel

On SellerReview fields, etc., I don't really care about explicitly providing the kwargs for on_delete, related_name, etc. when we don't diverge from the default. It'd be an easy thing to add before an upgrade to 2.0 if it's just for forward compatibility.

Serializers

I think it's easier to see all of the serializers for an app in one file. Open to ideas if you're following a new pattern or something here.

[DavidJFelix]

Apps

Tentative on splitting out apps this early because of how sloppy it can make imports. I was going to wait until we had a more solid model. I'm also not utilizing migrations at this time (I delete 00001 and recreate it) since we haven't run it in a non-local env.

Models

I'm sold on the split out for UUID. I'll look into model utils for timestamps. For related_name and on_delete, these cause huge issues and pycharm pretty much complained about everything I didn't set, so I started setting them. on_delete is apparently required in a later version and recommended in 1.9 so I don't see a harm in putting it in now. Since APIUser and reviews has multiple relations into them, I set explicit related names just to prevent issues on model creation, since those are annoying.

Serializers

Initially I had these in one file like views, but here's the real issue: when you start utilizing depth and sub-serializers (see meal) you need the sub serializer to be defined before the serializer. By splitting the serializers into their own files, they always get defined at import, which is naturally above the serializer you define. The serializers also started getting larger and rather than split one or two out and have a big file, I figured it was easier to make them all their own so we can find them.

Example serializer that causes issues:

class MealSerializer(...):
    user = UserSerializer()
    depth = 1

class UserSerializer(...):
    meals = MealSerializer(many=True)
    depth = 1

the above is a valid serializer map that we might want (especially since mobile is favoring "fat" objects). Since python evaluates class declarations when parsing, it will attempt to evaluate UserSerializer before it's declared.

[DavidJFelix]

I just did a quick trial to see if I could get UUIDModelMixin working and having fields in sub models is making migrations freak out or revert to pk integers. Could you show me how you had this in mind?

class UUIDModelMixin:
    id = id = models.UUIDField(default=uuid.uuid4, primary_key=True)

class Meal(UUIDModelMixin, models.Model):
    ...

Didn't work, nor did:

class UUIDModelMixin(models.Model):
    id = id = models.UUIDField(default=uuid.uuid4, primary_key=True)

class Meal(UUIDModelMixin, models.Model):
    ...

I didn't have time to check subclassing, but that's not really a mixin.

...

Nevermind, I got it.

class Meta:
    abstract = True

Belongs in the mixin.