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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
