On Mon, Jul 7, 2014 at 9:20 AM, Raphael Hertzog <hert...@debian.org> wrote: > Hi Andrew, thanks for the patch! > > Here are my comments:
Thanks for the feedback. Just trying to get a feel for the code base. It might be nice if the docs included a quick tutorial on the common task of adding a new panel or action item. It would give people a quick way in to contributing. > On Sun, 06 Jul 2014, Andrew Starr-Bochicchio wrote: >> +@python_2_unicode_compatible >> +class DebciStatus(models.Model): >> + """ >> + Model for debci status of packages. >> + """ >> + package = models.OneToOneField(PackageName, related_name='debci_status') >> + status = JSONField() > > Please don't add a new model for this. Rather use a new "key" in > PackageExtractedInfo. > > PackageExtractedInfo model should probably be renamed into something else > but it's the better place to store this as we can then query most of the > relevant information for a single package in a single query... Right. After I sent this off, I wondered if it needs to store this information at all. Is enough to just store the information in the action item? > Most of the rest looks fine, except for a detail: > >> + def execute(self): >> + all_debci_status = self.get_debci_status() >> + if not all_debci_status: >> + return >> + >> + # Discard all old >> + DebciStatus.objects.all().delete() >> + >> + failures = [] >> + packages = [] >> + for result in all_debci_status: >> + if result['status'] == 'fail': >> + package = PackageName.objects.get(name=result['package']) >> + debci_status = DebciStatus(package=package, >> + status=result) >> + failures.append(debci_status) >> + packages.append(package) >> + self.update_action_item(package, result) >> + >> + # Remove action items for packages without failing tests. >> + ActionItem.objects.delete_obsolete_items( >> + [self.debci_action_item_type], packages) >> + >> + DebciStatus.objects.bulk_create(failures) > > It would be better to do this in a transaction so that we have no time > where the information is lost. Either we have the new content or the old > content, but never an empty list of issues. > > It's easy to do: > > with transaction.atomic(): > # here goes the protected content Will do. Thanks! -- Andrew Starr-Bochicchio Ubuntu Developer <https://launchpad.net/~andrewsomething> Debian Developer <http://qa.debian.org/developer.php?login=asb> PGP/GPG Key ID: D53FDCB1 -- To UNSUBSCRIBE, email to debian-qa-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/cal6k_az_0ujw_q_dbmoz3ihvrocomuuf6xqk0n8xcyqvr7_...@mail.gmail.com
