Bug#753910: [PATCH] Add support for showing Debci test failures as an action item. Closes: #753910

[Andrew Starr-Bochicchio <a.starr.b@gmail.com>]

On Mon, Jul 7, 2014 at 9:20 AM, Raphael Hertzog <hertzog@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