Signoffs changes and improvements

* Better signoff report with more detail
* Show signoff specification in signoffs view
* Honor disabled/bad flags and display in approval column
* Various other small bugfixes and tweaks

Signed-off-by: Dan McGee <dan@archlinux.org>

4 files changed, 52 insertions, 20 deletions

diff --git a/packages/management/commands/signoff_report.py b/packages/management/commands/signoff_report.py
index 02f3d985..3431dada 100644
--- a/packages/management/commands/signoff_report.py
+++ b/packages/management/commands/signoff_report.py
@@ -56,6 +56,8 @@ def generate_report(email, repo_name):
     # Collect all existing signoffs for these packages
     signoff_groups = sorted(get_signoff_groups([repo]),
             key=attrgetter('target_repo', 'arch', 'pkgbase'))
+    disabled = []
+    bad = []
     complete = []
     incomplete = []
     new = []
@@ -68,10 +70,16 @@ def generate_report(email, repo_name):
     old_cutoff = now - timedelta(days=old_days)
 
     for group in signoff_groups:
-        if group.approved():
+        spec = group.specification
+        if spec.known_bad:
+            bad.append(group)
+        elif not spec.enabled:
+            disabled.append(group)
+        elif group.approved():
             complete.append(group)
         else:
             incomplete.append(group)
+
         if group.package.last_update > new_cutoff:
             new.append(group)
         if group.package.last_update < old_cutoff:
@@ -96,6 +104,9 @@ def generate_report(email, repo_name):
     c = Context({
         'repo': repo,
         'signoffs_url': signoffs_url,
+        'disabled': disabled,
+        'bad': bad,
+        'all': signoff_groups,
         'incomplete': incomplete,
         'complete': complete,
         'new': new,
diff --git a/packages/models.py b/packages/models.py
index a2b53a06..b70c21bf 100644
--- a/packages/models.py
+++ b/packages/models.py
@@ -1,3 +1,5 @@
+from collections import namedtuple
+
 from django.db import models
 from django.db.models.signals import pre_save, post_save
 from django.contrib.auth.models import User
@@ -42,22 +44,26 @@ class PackageRelation(models.Model):
 class SignoffSpecificationManager(models.Manager):
     def get_from_package(self, pkg):
         '''Utility method to pull all relevant name-version fields from a
-        package and get a matching specification.'''
+        package and get a matching signoff specification.'''
         return self.get(
                 pkgbase=pkg.pkgbase, pkgver=pkg.pkgver, pkgrel=pkg.pkgrel,
                 epoch=pkg.epoch, arch=pkg.arch, repo=pkg.repo)
 
-    def get_or_create_from_package(self, pkg):
-        '''Utility method to pull all relevant name-version fields from a
-        package and get or create a matching specification.'''
-        return self.get_or_create(
-                pkgbase=pkg.pkgbase, pkgver=pkg.pkgver, pkgrel=pkg.pkgrel,
-                epoch=pkg.epoch, arch=pkg.arch, repo=pkg.repo)
+    def get_or_default_from_package(self, pkg):
+        '''utility method to pull all relevant name-version fields from a
+        package and get a matching signoff specification, or return the default
+        base case.'''
+        try:
+            return self.get(
+                    pkgbase=pkg.pkgbase, pkgver=pkg.pkgver, pkgrel=pkg.pkgrel,
+                    epoch=pkg.epoch, arch=pkg.arch, repo=pkg.repo)
+        except SignoffSpecification.DoesNotExist:
+            return DEFAULT_SIGNOFF_SPEC
 
 class SignoffSpecification(models.Model):
     '''
     A specification for the signoff policy for this particular revision of a
-    pakcage. The default is requiring two signoffs for a given package. These
+    package. The default is requiring two signoffs for a given package. These
     are created only if necessary; e.g., if one wanted to override the
     required=2 attribute, otherwise a sane default object is used.
     '''
@@ -89,6 +95,12 @@ class SignoffSpecification(models.Model):
         return u'%s-%s' % (self.pkgbase, self.full_version)
 
 
+# fake default signoff spec when we don't have a persisted one in the database
+FakeSignoffSpecification = namedtuple('FakeSignoffSpecification',
+        ('required', 'enabled', 'known_bad', 'comments'))
+DEFAULT_SIGNOFF_SPEC = FakeSignoffSpecification(2, True, False, u'')
+
+
 class SignoffManager(models.Manager):
     def get_from_package(self, pkg, user, revoked=False):
         '''Utility method to pull all relevant name-version fields from a
diff --git a/packages/utils.py b/packages/utils.py
index 42cfbe0f..60b95e21 100644
--- a/packages/utils.py
+++ b/packages/utils.py
@@ -5,7 +5,8 @@ from django.db.models import Count, Max
 
 from main.models import Package, Repo
 from main.utils import cache_function, groupby_preserve_order, PackageStandin
-from .models import PackageGroup, PackageRelation, SignoffSpecification, Signoff
+from .models import (PackageGroup, PackageRelation,
+        SignoffSpecification, Signoff, DEFAULT_SIGNOFF_SPEC)
 
 @cache_function(300)
 def get_group_info(include_arches=None):
@@ -148,9 +149,7 @@ SELECT DISTINCT id
     return relations
 
 
-DEFAULT_SIGNOFF_SPEC = SignoffSpecification()
-
-def approved_by_signoffs(signoffs, spec=DEFAULT_SIGNOFF_SPEC):
+def approved_by_signoffs(signoffs, spec):
     if signoffs:
         good_signoffs = sum(1 for s in signoffs if not s.revoked)
         return good_signoffs >= spec.required
@@ -158,14 +157,13 @@ def approved_by_signoffs(signoffs, spec=DEFAULT_SIGNOFF_SPEC):
 
 class PackageSignoffGroup(object):
     '''Encompasses all packages in testing with the same pkgbase.'''
-    def __init__(self, packages, user=None):
+    def __init__(self, packages):
         if len(packages) == 0:
             raise Exception
         self.packages = packages
-        self.user = user
+        self.user = None
         self.target_repo = None
         self.signoffs = set()
-        self.specification = DEFAULT_SIGNOFF_SPEC
 
         first = packages[0]
         self.pkgbase = first.pkgbase
@@ -175,6 +173,10 @@ class PackageSignoffGroup(object):
         self.last_update = first.last_update
         self.packager = first.packager
 
+        self.specification = \
+                SignoffSpecification.objects.get_or_default_from_package(first)
+        self.default_spec = self.specification is DEFAULT_SIGNOFF_SPEC
+
         version = first.full_version
         if all(version == pkg.full_version for pkg in packages):
             self.version = version
diff --git a/packages/views.py b/packages/views.py
index 66bcd3fc..307691e2 100644
--- a/packages/views.py
+++ b/packages/views.py
@@ -7,8 +7,9 @@ from django.conf import settings
 from django.core.mail import send_mail
 from django.core.serializers.json import DjangoJSONEncoder
 from django.db.models import Q
-from django.http import HttpResponse, Http404
-from django.shortcuts import get_object_or_404, get_list_or_404, redirect
+from django.http import HttpResponse, Http404, HttpResponseForbidden
+from django.shortcuts import (get_object_or_404, get_list_or_404,
+        redirect, render)
 from django.template import loader, Context
 from django.utils import simplejson
 from django.views.decorators.cache import never_cache
@@ -404,12 +405,16 @@ def signoff_package(request, name, repo, arch, revoke=False):
                 package, request.user)
 
     all_signoffs = Signoff.objects.for_package(package)
+    spec = SignoffSpecification.objects.get_or_default_from_package(package)
 
     if request.is_ajax():
         data = {
             'created': created,
             'revoked': bool(signoff.revoked),
-            'approved': approved_by_signoffs(all_signoffs),
+            'approved': approved_by_signoffs(all_signoffs, spec),
+            'required': spec.required,
+            'enabled': spec.enabled,
+            'known_bad': spec.known_bad,
             'user': str(request.user),
         }
         return HttpResponse(simplejson.dumps(data, ensure_ascii=False),
@@ -429,7 +434,9 @@ def signoff_options(request, name, repo, arch):
             arch__name=arch, repo__name__iexact=repo, repo__testing=True)
     package = packages[0]
 
-    # TODO ensure submitter is maintainer and/or packager
+    if request.user != package.packager and \
+            request.user not in package.maintainers:
+        return render(request, '403.html', status=403)
 
     try:
         spec = SignoffSpecification.objects.get_from_package(package)