[PATCH 1/5] models: Make use of aggregates

[Daniel Axtens]

Hi Stephen,
  
> -        if len(orders) > 0:
> -            max_order = orders[0]['order']
> +        if orders and orders['order__max']:
> +            max_order = orders['order__max'] + 1
>          else:
> -            max_order = 0
> +            max_order = 1
>
I'm not super happy with the change from max_order = 0 to max_order = 1
etc here. That makes the variable represent the new order, not the
current max order.

Could you either:
 - change the name, or
 - revert back to using 0 and adding one at the end.


> -        # see if the patch is already in this bundle
> -        if BundlePatch.objects.filter(bundle=self,
> -                                      patch=patch).count():
> +        if BundlePatch.objects.filter(bundle=self, patch=patch).exists():
>              return
I know you didn't change this line, but should this be an explicit
return None, given that below an object is returned?


> -        bp = BundlePatch.objects.create(bundle=self, patch=patch,
> -                                        order=max_order + 1)
> -        bp.save()
> -
> -        return bp
> +        return BundlePatch.objects.create(bundle=self, patch=patch,
> +                                          order=max_order)
Is this guaranteed to save? My memory of the specific semantics of
create is a bit fuzzy.

Apart from that, looks good to me.

Regards,
Daniel

>  
>      def public_url(self):
>          if not self.public:
> -- 
> 2.7.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork