I'm getting obsessed! Where should I put this code?

[Glyn Jackson]

I'm getting obsessed!

I have an API (TatstyPie) which saves a product. Every time this resource is saved I assign stock or rise an error if their is no stock.

get and allocate some stock:

stock = Stock.objects.filter(availability="Available", product=product)[:1].get()

check we have stock:

 except Stock.DoesNotExist:

        raise CustomBadRequest(

            code="stock",

            message="No stock left"

        )

if ok, assign stock to product:

instance.stock = stock

stock.allocate()

So far I have moved this logic around so many times, but where should it really be happening in my Django project? Opinions very much welcome. Thanks

1) product  pre_save - this works really well but validation looks odd here!

2) product  model save() - again validation looks odd here!

3) product model manager - this seems bad as I'm referencing the stock model in product manager.

4) views - I end up repeating the save validation and code looks really bad, also I have to do it again for API and normal views

5) tastypie recourse - just as bad as views I think.

[Arnold Krille]

On Sun, 9 Feb 2014 02:15:46 -0800 (PST) Glyn Jackson
<m...@glynjackson.org> wrote:
> So far I have moved this logic around so many times, but where should
> it really be happening in my Django project? Opinions very much
> welcome. Thanks
>

> 1) product *pre_save* - this works really well but validation looks
> odd here!
> 2) product *model save()* - again validation looks odd here!
> 3) product *model manager *- this seems bad as I'm referencing the

> stock model in product manager.

> 4) *views* - I end up repeating the save validation and code looks

> really bad, also I have to do it again for API and normal views

> 5) *tastypie recourse* - just as bad as views I think.

Model! If its validation and 'save' should stop if there is none in
stock, its either a pre-save-trigger or on your own save-method before
calling super(...).save(). Be aware that if you subclass your product,
a pre-save trigger has to get connected to all derived classes too, so
it might be easier to just add it to the save-method.

The advantage is clearly that the same code is used for all views. And
api-resources are a view too (or a controller but lets not discuss
thie here).

- Arnold

[Nikolas Stevenson-Molnar]

I'm not sure it makes sense to put something that raises a "BadRequest" type error in the model. That's more of an API exception. I'd say put it in the resource. If you find yourself repeating it a lot then put the validation in it's own function and/or use subclasses if appropriate.

_Nik

[Derek]

A common function that can get called from multiple places may be the way to go.  For example (please adapt!):

#view file

# try allocate some stock to product

try:

    instance.stock = get_stock(availability="Available", product=product):

    instance.stock.allocate()

except:

    pass  # do something else?

#common file

from foobar.models import Stock

def get_stock(availability, product, caller='view'):

    """Return stock, if available, else raise an error."""

    try:

        stock = Stock.objects.filter(availability=availability, product=product)[:1].get()

        return stock

    except Stock.DoesNotExist:

        if not caller or caller == 'view':

            raise CustomBadRequest(

                code="stock",

                message="No stock left"

            )

            return None

        elif caller == 'other':

            raise CustomBadError(

                code="stock",

                message="Stock is all gone"

            )

            return None

        else:

            pass

    finally:

        return None

On Monday, 10 February 2014 04:48:05 UTC+2, Nikolas Stevenson-Molnar wrote:

I'm not sure it makes sense to put something that raises a "BadRequest" type error in the model. That's more of an API exception. I'd say put it in the resource. If you find yourself repeating it a lot then put the validation in it's own function and/or use subclasses if appropriate.

_Nik

On 2/9/2014 10:18 AM, Arnold Krille wrote:

> On Sun, 9 Feb 2014 02:15:46 -0800 (PST) Glyn Jackson

[Glyn Jackson]

Thank you so much for all your comments, they were very helpful.

After reading 'a lot' I think I've found some calm.

I decided to move all validation into validation.py. I then used these functions in both forms and the models i.e.

mobile = PhoneNumberField(validators=[validate_mobile])

I found that Django however, does not call the clean method by default when saving a model.

I got rid of the signal pre_save I read it was a bad idea! I found that the save() method is executed at the same time the `pre_save` -signal was.

    def save(self, force_insert=False, force_update=False, *args, **kwargs):

        """

        Override of save() method which is executed the same

        time the ``pre_save``-signal would be

        """

        if not (force_insert or force_update):

            self.full_clean()

I feel a lot better now :)