blocklist dual sign off for multi-guid submission (#13266)

* Add signoff to multi-block submissions

* MultiBlockSubmit to BlockSubmission

* add signoff approval and rejection

* Add logging for BlockSubmission (with admin.LogEntry)

* show full metadata for blocks being proposed

* squash django migrations

* only show LogEntry for that model instance on the page

* Add some comments to the permission checks in BlockSubmission

* .trim() all guid lines everywhere

src/olympia/blocklist/admin.py

@@ -1,22 +1,26 @@
-from django.contrib import admin
+from django.contrib import admin, contenttypes
 from django.core.exceptions import PermissionDenied
 from django.forms import modelform_factory
 from django.forms.fields import CharField, ChoiceField
 from django.forms.widgets import HiddenInput
 from django.shortcuts import redirect
+from django.template.loader import render_to_string
 from django.template.response import TemplateResponse
 from django.urls import path
 from django.utils.html import format_html
 
+import waffle
+
 from olympia.activity.models import ActivityLog
 from olympia.amo.urlresolvers import reverse
 from olympia.amo.utils import HttpResponseTemporaryRedirect
 
 from .forms import MultiAddForm, MultiDeleteForm
-from .models import Block, MultiBlockSubmit
+from .models import Block, BlockSubmission
 from .tasks import create_blocks_from_multi_block
 from .utils import (
-    block_activity_log_delete, block_activity_log_save, format_block_history)
+    block_activity_log_delete, block_activity_log_save, format_block_history,
+    splitlines)
 
 
 # The limit for how many GUIDs should be fully loaded with all metadata
@@ -49,7 +53,7 @@ class BlockAdminAddMixin():
         if request.method == 'POST':
             form = MultiAddForm(request.POST)
             if form.is_valid():
-                guids = form.data.get('guids', '').splitlines()
+                guids = splitlines(form.data.get('guids'))
                 if len(guids) == 1:
                     # If the guid already has a Block go to the change view
                     if form.existing_block:
@@ -64,7 +68,7 @@ class BlockAdminAddMixin():
                 elif len(guids) > 1:
                     # If there's > 1 guid go to multi view.
                     return HttpResponseTemporaryRedirect(
-                        reverse('admin:blocklist_multiblocksubmit_add'))
+                        reverse('admin:blocklist_blocksubmission_add'))
         else:
             form = MultiAddForm()
         return self._render_multi_guid_input(
@@ -107,31 +111,16 @@ class BlockAdminAddMixin():
             request, form_url=form_url, extra_context=extra_context)
 
 
-@admin.register(MultiBlockSubmit)
-class MultiBlockSubmitAdmin(admin.ModelAdmin):
+@admin.register(BlockSubmission)
+class BlockSubmissionAdmin(admin.ModelAdmin):
     list_display = (
-        'created',
-        'min_version',
-        'max_version',
-        'updated_by',
-        'invalid_guid_count',
-        'existing_guid_count',
         'blocks_count',
-        'blocks_submitted_count',
-        'submission_complete',
-    )
-    # Only used for the change view
-    fields = (
-        'input_guids',
-        'blocks_submitted',
-        'min_version',
-        'max_version',
-        'url',
-        'reason',
+        'signoff_state',
         'updated_by',
-        'include_in_legacy',
+        'modified',
     )
-    add_fields = (
+    # only used by add view currently - see get_fieldsets() for change view
+    fields = (
         'input_guids',
         'min_version',
         'max_version',
@@ -141,14 +130,86 @@ class MultiBlockSubmitAdmin(admin.ModelAdmin):
     )
     ordering = ['-created']
     view_on_site = False
-    list_select_related = ('updated_by',)
+    list_select_related = ('updated_by', 'signoff_by')
+    change_form_template = 'blocklist/block_submission_change_form.html'
+
+    class Media:
+        css = {
+            'all': ('css/admin/blocklist_blocksubmission.css',)
+        }
 
     def has_delete_permission(self, request, obj=None):
+        # For now, keep all BlockSubmission records.
+        # TODO: define under what cirumstances records can be safely deleted.
         return False
 
-    # Read-only mode
     def has_change_permission(self, request, obj=None):
-        return False
+        """ While a block submission is pending we want it to be partially
+        editable (the url and reason).  Once it's been rejected or approved it
+        can't be changed though.  Note: as sign-off uses the changeform this
+        can't conflict with `has_signoff_permission`."""
+        has = super().has_change_permission(request, obj=obj)
+        pending = obj and obj.signoff_state == BlockSubmission.SIGNOFF_PENDING
+        return has and (not obj or pending)
+
+    def has_signoff_permission(self, request, obj=None):
+        """ This controls whether the sign-off approve and reject actions are
+        available on the change form.  `BlockSubmission.can_user_signoff`
+        confirms the current user, who will signoff, is different from the user
+        who submitted the guids (unless settings.DEBUG is True or the check is
+        ignored)"""
+        # Because it uses the changeform, `has_change_permission` must also be
+        # true, so check it first.
+        has = self.has_change_permission(request, obj=obj)
+        pending = obj and obj.signoff_state == BlockSubmission.SIGNOFF_PENDING
+        return has and obj and pending and obj.can_user_signoff(request.user)
+
+    def get_fieldsets(self, request, obj):
+        input_guids = (
+            'Input Guids', {
+                'fields': (
+                    'input_guids',
+                ),
+                'classes': ('collapse',)
+            })
+        if not obj:
+            title = 'Add New Blocks'
+        elif obj.signoff_state == BlockSubmission.SIGNOFF_PUBLISHED:
+            title = 'Blocks Published'
+        else:
+            title = 'Proposed New Blocks'
+
+        edit = (
+            title, {
+                'fields': (
+                    'blocks',
+                    'min_version',
+                    'max_version',
+                    'url',
+                    'reason',
+                    'updated_by',
+                    'signoff_by',
+                    'include_in_legacy',
+                    'submission_logs',
+                ),
+            })
+
+        return (input_guids, edit)
+
+    def get_readonly_fields(self, request, obj=None):
+        if obj:
+            ro_fields = (
+                'blocks',
+                'updated_by',
+                'signoff_by',
+                'submission_logs',
+                'input_guids',
+                'min_version',
+                'max_version',
+            )
+        else:
+            ro_fields = ()
+        return ro_fields
 
     def add_view(self, request, **kwargs):
         if not self.has_add_permission(request):
@@ -158,8 +219,9 @@ class MultiBlockSubmitAdmin(admin.ModelAdmin):
             self.form.__name__, (self.form,), {
                 'existing_min_version': CharField(widget=HiddenInput),
                 'existing_max_version': CharField(widget=HiddenInput)})
+        fields = self.fields
         MultiBlockForm = modelform_factory(
-            MultiBlockSubmit, fields=self.add_fields, form=ModelForm,
+            self.model, fields=fields, form=ModelForm,
             widgets={'input_guids': HiddenInput()})
 
         guids_data = request.POST.get('guids', request.GET.get('guids'))
@@ -180,11 +242,10 @@ class MultiBlockSubmitAdmin(admin.ModelAdmin):
                 frm_data['max_version'] == frm_data['existing_max_version'])
             if form.is_valid() and versions_unchanged:
                 # Save the object so we have the guids
-                obj = form.save()
-                obj.update(updated_by=request.user)
+                obj = form.save(commit=False)
+                obj.updated_by = request.user
+                self.save_model(request, obj, form, change=False)
                 self.log_addition(request, obj, [{'added': {}}])
-                # Then launch a task to async save the individual blocks
-                create_blocks_from_multi_block.delay(obj.id)
                 if request.POST.get('_addanother'):
                     return redirect('admin:blocklist_block_add')
                 else:
@@ -210,17 +271,107 @@ class MultiBlockSubmitAdmin(admin.ModelAdmin):
             'title': 'Block Add-ons',
             'save_as': False,
         }
+        context.update(**self._get_enhanced_guid_context(request, guids_data))
+        return TemplateResponse(
+            request, 'blocklist/block_submission_add_form.html', context)
+
+    def _get_enhanced_guid_context(self, request, guids_data):
         load_full_objects = guids_data.count('\n') < GUID_FULL_LOAD_LIMIT
-        objects = MultiBlockSubmit.process_input_guids(
+        objects = self.model.process_input_guids(
             guids_data,
             v_min=request.POST.get('min_version', Block.MIN),
             v_max=request.POST.get('max_version', Block.MAX),
             load_full_objects=load_full_objects)
-        context.update(objects)
         if load_full_objects:
             Block.preload_addon_versions(objects['blocks'])
-        return TemplateResponse(
-            request, 'blocklist/multiple_block.html', context)
+        return objects
+
+    def change_view(self, request, object_id, form_url='', extra_context=None):
+        extra_context = extra_context or {}
+        obj = self.model.objects.filter(id=object_id).latest()
+        extra_context['has_signoff_permission'] = self.has_signoff_permission(
+            request, obj)
+        if obj.signoff_state != BlockSubmission.SIGNOFF_PUBLISHED:
+            extra_context.update(
+                **self._get_enhanced_guid_context(request, obj.input_guids))
+        else:
+            extra_context['blocks'] = obj.blocks_saved
+        return super().change_view(
+            request, object_id, form_url=form_url, extra_context=extra_context)
+
+    def render_change_form(self, request, context, add=False, change=False,
+                           form_url='', obj=None):
+        if change:
+            # add this to the instance so blocks() below can reference it.
+            obj._blocks = context['blocks']
+        return super().render_change_form(
+            request, context, add=add, change=change, form_url=form_url,
+            obj=obj)
+
+    def save_model(self, request, obj, form, change):
+        if change:
+            is_signoff = '_signoff' in request.POST
+            is_reject = '_reject' in request.POST
+            if is_signoff:
+                obj.signoff_state = BlockSubmission.SIGNOFF_APPROVED
+                obj.signoff_by = request.user
+            elif is_reject:
+                obj.signoff_state = BlockSubmission.SIGNOFF_REJECTED
+        else:
+            # TODO: something more fine-grained that looks at user counts
+            if waffle.switch_is_active('blocklist_admin_dualsignoff_disabled'):
+                obj.signoff_state = BlockSubmission.SIGNOFF_NOTNEEDED
+
+        super().save_model(request, obj, form, change)
+        if obj.is_save_to_blocks_permitted:
+            # Then launch a task to async save the individual blocks
+            create_blocks_from_multi_block.delay(obj.id)
+
+    def log_change(self, request, obj, message):
+        log_entry = None
+        is_signoff = '_signoff' in request.POST
+        is_reject = '_reject' in request.POST
+        if is_signoff:
+            signoff_msg = 'Sign-off Approval'
+        elif is_reject:
+            signoff_msg = 'Sign-off Rejection'
+        else:
+            signoff_msg = ''
+
+        if not message and signoff_msg:
+            # if there's no message (i.e. no changed fields) just use ours.
+            log_entry = super().log_change(request, obj, signoff_msg)
+        else:
+            # otherwise let the message be built as normal
+            log_entry = super().log_change(request, obj, message)
+            if signoff_msg:
+                # before flattening it if we need to add on ours
+                log_entry.change_message = (
+                    signoff_msg + ' & ' + log_entry.get_change_message())
+                log_entry.save()
+
+        return log_entry
+
+    def submission_logs(self, obj):
+        content_type = contenttypes.models.ContentType.objects.get_for_model(
+            self.model)
+        logs = admin.models.LogEntry.objects.filter(
+            object_id=obj.id, content_type=content_type)
+        return '\n'.join(
+            f'{log.action_time.date()}: {str(log)}' for log in logs)
+
+    def blocks(self, obj):
+        # Annoyingly, we don't have the full context, but we stashed blocks
+        # earlier in render_change_form().
+        return render_to_string(
+            'blocklist/includes/enhanced_blocks.html',
+            {
+                'blocks': obj._blocks
+            },
+        )
+
+    def blocks_count(self, obj):
+        return f"{len(obj.toblock_guids)} add-ons"
 
 
 @admin.register(Block)

src/olympia/blocklist/forms.py

@@ -3,7 +3,9 @@ from django.core.exceptions import ValidationError
 from django.utils.translation import ugettext_lazy as _
 
 from olympia.addons.models import Addon
-from olympia.blocklist.models import Block
+
+from .models import Block
+from .utils import splitlines
 
 
 class MultiGUIDInputForm(forms.Form):
@@ -16,7 +18,7 @@ class MultiDeleteForm(MultiGUIDInputForm):
     existing_block_qs = None
 
     def clean(self):
-        guids = self.cleaned_data.get('guids', '').splitlines()
+        guids = splitlines(self.cleaned_data.get('guids'))
         if len(guids) >= 1:
             # Note: we retrieve a full queryset here because we later need one
             # to pass to admin.actions.delete_selected in delete_multiple_view.
@@ -36,7 +38,7 @@ class MultiAddForm(MultiGUIDInputForm):
     existing_block = None
 
     def clean(self):
-        guids = self.cleaned_data.get('guids', '').splitlines()
+        guids = splitlines(self.cleaned_data.get('guids'))
         if len(guids) == 1:
             guid = guids[0]
             blk = self.existing_block = Block.objects.filter(guid=guid).first()

src/olympia/blocklist/migrations/0008_blocksubmission.py

@@ -0,0 +1,30 @@
+# Generated by Django 2.2.9 on 2020-01-15 18:03
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('blocklist', '0007_block_kinto_id'),
+    ]
+
+    operations = [
+        migrations.RenameModel(
+            old_name='MultiBlockSubmit',
+            new_name='BlockSubmission',
+        ),
+        migrations.AddField(
+            model_name='blocksubmission',
+            name='signoff_by',
+            field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL),
+        ),
+        migrations.AddField(
+            model_name='blocksubmission',
+            name='signoff_state',
+            field=models.SmallIntegerField(choices=[(0, 'Pending'), (1, 'Approved'), (2, 'Rejected'), (3, 'No Sign-off'), (4, 'Published to Blocks')], default=0),
+        ),
+    ]

src/olympia/blocklist/models.py

@@ -1,6 +1,7 @@
 import datetime
 from collections import defaultdict, namedtuple, OrderedDict
 
+from django.conf import settings
 from django.core.exceptions import ValidationError
 from django.db import models
 from django.utils.html import format_html, format_html_join
@@ -19,7 +20,7 @@ from django.utils.translation import gettext_lazy as _
 from olympia.versions.compare import addon_version_int
 from olympia.versions.models import Version
 
-from .utils import block_activity_log_save
+from .utils import block_activity_log_save, splitlines
 
 
 class Block(ModelBase):
@@ -132,7 +133,20 @@ class Block(ModelBase):
         return ''
 
 
-class MultiBlockSubmit(ModelBase):
+class BlockSubmission(ModelBase):
+    SIGNOFF_PENDING = 0
+    SIGNOFF_APPROVED = 1
+    SIGNOFF_REJECTED = 2
+    SIGNOFF_NOTNEEDED = 3
+    SIGNOFF_PUBLISHED = 4
+    SIGNOFF_STATES = {
+        SIGNOFF_PENDING: 'Pending',
+        SIGNOFF_APPROVED: 'Approved',
+        SIGNOFF_REJECTED: 'Rejected',
+        SIGNOFF_NOTNEEDED: 'No Sign-off',
+        SIGNOFF_PUBLISHED: 'Published to Blocks',
+    }
+
     input_guids = models.TextField()
     processed_guids = JSONField(default={})
     min_version = models.CharField(
@@ -146,6 +160,21 @@ class MultiBlockSubmit(ModelBase):
     include_in_legacy = models.BooleanField(
         default=False,
         help_text='Include in legacy xml blocklist too, as well as new v3')
+    signoff_by = models.ForeignKey(
+        UserProfile, null=True, on_delete=models.SET_NULL, related_name='+')
+    signoff_state = models.SmallIntegerField(
+        choices=SIGNOFF_STATES.items(), default=SIGNOFF_PENDING)
+
+    def __str__(self):
+        guids = splitlines(self.input_guids)
+        repr = []
+        if len(guids) == 1:
+            repr.append(guids[0])
+        elif len(guids) > 1:
+            repr.append(guids[0] + ', ...')
+        repr.append(str(self.url))
+        repr.append(str(self.reason))
+        return f'{self.get_signoff_state_display()}: {"; ".join(repr)}'
 
     def clean(self):
         min_vint = addon_version_int(self.min_version)
@@ -155,24 +184,15 @@ class MultiBlockSubmit(ModelBase):
                 _('Min version can not be greater than Max version'))
 
     @property
-    def invalid_guid_count(self):
-        return len(self.processed_guids.get('invalid_guids', []))
+    def toblock_guids(self):
+        return self.processed_guids.get('toblock_guids', [])
 
     @property
-    def existing_guid_count(self):
-        return len(self.processed_guids.get('existing_guids', []))
-
-    @property
-    def blocks_count(self):
-        return len(self.processed_guids.get('blocks', []))
-
-    @property
-    def blocks_submitted_count(self):
-        return len(self.processed_guids.get('blocks_saved', []))
-
-    def submission_complete(self):
-        return self.blocks_count == self.blocks_submitted_count
-    submission_complete.boolean = True
+    def blocks_saved(self):
+        MinimalFakeBlock = namedtuple(
+            'MinimalFakeBlock', ('guid', 'id'))
+        blocks = self.processed_guids.get('blocks_saved', [])
+        return [MinimalFakeBlock(guid, id_) for (id_, guid) in blocks]
 
     def blocks_submitted(self):
         blocks = self.processed_guids.get('blocks_saved', [])
@@ -182,15 +202,32 @@ class MultiBlockSubmit(ModelBase):
             ((reverse('admin:blocklist_block_change', args=(id_,)), guid)
                 for (id_, guid) in blocks))
 
+    def can_user_signoff(self, signoff_user):
+        require_different_users = not settings.DEBUG
+        different_users = (
+            self.updated_by and signoff_user and
+            self.updated_by != signoff_user)
+        return not require_different_users or different_users
+
+    @property
+    def is_save_to_blocks_permitted(self):
+        """Has this submission been signed off, or sign-off isn't required."""
+        return (
+            self.signoff_state == self.SIGNOFF_NOTNEEDED or (
+                self.signoff_state == self.SIGNOFF_APPROVED and
+                self.can_user_signoff(self.signoff_by)
+            )
+        )
+
     def save(self, *args, **kwargs):
         if self.input_guids and not self.processed_guids:
-            processed_guids = self.process_input_guids(
+            processed = self.process_input_guids(
                 self.input_guids, self.min_version, self.max_version,
                 load_full_objects=False)
-            # flatten blocks back to just the guids
-            processed_guids['blocks'] = [
-                block.guid for block in processed_guids['blocks']]
-            self.processed_guids = processed_guids
+            # flatten blocks to just the guids and replace
+            blocks = processed.pop('blocks', [])
+            processed['toblock_guids'] = [block.guid for block in blocks]
+            self.processed_guids = processed
         super().save(*args, **kwargs)
 
     @classmethod
@@ -211,7 +248,7 @@ class MultiBlockSubmit(ModelBase):
         FakeBlock = namedtuple(
             'FakeBlock', ('guid', 'addon', 'min_version', 'max_version'))
         FakeAddon = namedtuple('FakeAddon', ('guid', 'average_daily_users'))
-        all_guids = set(guids.splitlines())
+        all_guids = set(splitlines(guids))
 
         # load all the Addon instances together
         addon_qs = Addon.unfiltered.filter(guid__in=all_guids).order_by(
@@ -296,6 +333,7 @@ class MultiBlockSubmit(ModelBase):
         return blocks
 
     def save_to_blocks(self):
+        assert self.is_save_to_blocks_permitted
         common_args = {
             'min_version': self.min_version,
             'max_version': self.max_version,
@@ -306,9 +344,8 @@ class MultiBlockSubmit(ModelBase):
         }
 
         modified_datetime = datetime.datetime.now()
-        all_guids_to_block = self.processed_guids.get('blocks', [])
+        all_guids_to_block = self.processed_guids.get('toblock_guids', [])
         self.processed_guids['blocks_saved'] = []
-        blocks_saved = []
         for guids_chunk in chunked(all_guids_to_block, 100):
             blocks = self._get_blocks_from_list(guids_chunk)
             Block.preload_addon_versions(blocks)
@@ -320,8 +357,8 @@ class MultiBlockSubmit(ModelBase):
                     setattr(block, 'modified', modified_datetime)
                 block.save()
                 block_activity_log_save(block, change=change)
-                blocks_saved.append((block.id, block.guid))
-            self.processed_guids['blocks_saved'] = blocks_saved
+                self.processed_guids['blocks_saved'].append(
+                    (block.id, block.guid))
             self.save()
 
-        return blocks
+        self.update(signoff_state=self.SIGNOFF_PUBLISHED)

src/olympia/blocklist/tasks.py

@@ -9,7 +9,7 @@ from olympia.amo.celery import task
 from olympia.amo.decorators import use_primary_db
 from olympia.users.utils import get_task_user
 
-from .models import Block, MultiBlockSubmit
+from .models import Block, BlockSubmission
 from .utils import block_activity_log_save
 
 
@@ -22,7 +22,7 @@ bracket_close_regex = re.compile(r'(?<!\\)}')
 @task
 @use_primary_db
 def create_blocks_from_multi_block(multi_block_submit_id, **kw):
-    obj = MultiBlockSubmit.objects.get(pk=multi_block_submit_id)
+    obj = BlockSubmission.objects.get(pk=multi_block_submit_id)
     # create the blocks from the guids in the multi_block
     obj.save_to_blocks()
 

src/olympia/blocklist/templates/blocklist/block_submission_add_form.html

@@ -52,21 +52,7 @@
     <h2>{% trans 'Add New Blocks' %}</h2>
     <div class="form-row field-blocks-to-add">
       <div>
-        <h3>{% blocktrans with count=blocks|length %}{{ count }} Add-on GUIDs to Block{% endblocktrans %}:</h3>
-         <ul class="guid_list">
-          {% for block_obj in blocks %}
-            <li>
-              {{ block_obj.guid }}.
-              <span class="addon-name">{{ block_obj.addon.name }}</span>
-              ({{ block_obj.addon.average_daily_users }} {% trans 'users' %}).
-              {{ block_obj.review_listed_link }}
-              {{ block_obj.review_unlisted_link }}
-              {% if block_obj.id %}
-                <span class="existing_block">[<a href="{% url 'admin:blocklist_block_change' block_obj.id %}">{% trans 'Edit Block' %}</a>: {{ block_obj.min_version }} - {{ block_obj.max_version }}]</span>
-              {% endif %}
-            </li>
-          {% endfor %}
-        </ul>
+        {% include 'blocklist/includes/enhanced_blocks.html' %}
       </div>
     </div>
     {% if form.non_field_errors %}

src/olympia/blocklist/templates/blocklist/block_submission_change_form.html

@@ -0,0 +1,18 @@
+{% extends "admin/change_form.html" %}
+{% load i18n admin_urls %}
+
+{% block submit_buttons_bottom %}
+  <div class="submit-row">
+    {% if has_delete_permission and change and show_delete %}
+        {% url opts|admin_urlname:'delete' original.pk|admin_urlquote as delete_url %}
+        <p class="deletelink-box"><a href="{% add_preserved_filters delete_url %}" class="deletelink">{% trans "Delete" %}</a></p>
+    {% endif %}
+    {% if has_change_permission and change %}
+        <input type="submit" value="{% trans 'Update' %}" class="default" name="_save">
+    {% endif %}
+    {% if has_signoff_permission and change %}
+      <input type="submit" value="{% trans 'Reject Submission' %}" name="_reject">
+      <input type="submit" value="{% trans 'Approve Submission' %}" name="_signoff">
+    {% endif %}
+  </div>
+{% endblock %}

src/olympia/blocklist/templates/blocklist/includes/enhanced_blocks.html

@@ -0,0 +1,19 @@
+{% load i18n %}
+
+      <h3>{% blocktrans with count=blocks|length %}{{ count }} Add-on GUIDs{% endblocktrans %}:</h3>
+       <ul class="guid_list">
+        {% for block_obj in blocks %}
+          <li>
+            {{ block_obj.guid }}.
+            {% if block_obj.addon %}
+              <span class="addon-name">{{ block_obj.addon.name }}</span>
+              ({{ block_obj.addon.average_daily_users }} {% trans 'users' %}).
+            {% endif %}
+            {{ block_obj.review_listed_link }}
+            {{ block_obj.review_unlisted_link }}
+            {% if block_obj.id %}
+              <span class="existing_block">[<a href="{% url 'admin:blocklist_block_change' block_obj.id %}">{% trans 'Edit Block' %}</a>: {{ block_obj.min_version }} - {{ block_obj.max_version }}]</span>
+            {% endif %}
+          </li>
+        {% endfor %}
+      </ul>

src/olympia/blocklist/tests/test_admin.py

@@ -1,8 +1,13 @@
 import datetime
+import json
 
 from unittest import mock
 
+from django.contrib.admin.models import LogEntry, ADDITION
+from django.contrib.contenttypes.models import ContentType
+
 from pyquery import PyQuery as pq
+from waffle.testutils import override_switch
 
 from olympia import amo
 from olympia.activity.models import ActivityLog
@@ -11,7 +16,7 @@ from olympia.amo.tests import (
     TestCase, addon_factory, user_factory, version_factory)
 from olympia.amo.urlresolvers import reverse
 
-from ..models import Block, MultiBlockSubmit
+from ..models import Block, BlockSubmission
 
 
 class TestBlockAdminList(TestCase):
@@ -66,7 +71,7 @@ class TestBlockAdminAdd(TestCase):
     def setUp(self):
         self.add_url = reverse('admin:blocklist_block_add')
         self.single_url = reverse('admin:blocklist_block_add_single')
-        self.multi_url = reverse('admin:blocklist_multiblocksubmit_add')
+        self.multi_url = reverse('admin:blocklist_blocksubmission_add')
 
     def test_add(self):
         user = user_factory()
@@ -264,13 +269,13 @@ class TestBlockAdminAdd(TestCase):
         assert Block.objects.count() == 0
 
 
-class TestMultiBlockSubmitAdmin(TestCase):
+class TestBlockSubmissionAdmin(TestCase):
     def setUp(self):
-        self.multi_url = reverse('admin:blocklist_multiblocksubmit_add')
+        self.multi_url = reverse('admin:blocklist_blocksubmission_add')
         self.multi_list_url = reverse(
-            'admin:blocklist_multiblocksubmit_changelist')
+            'admin:blocklist_blocksubmission_changelist')
 
-    def test_add_multiple(self):
+    def _test_add_multiple_submit(self):
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
         self.grant_permission(user, 'Reviews:Admin')
@@ -312,9 +317,9 @@ class TestMultiBlockSubmitAdmin(TestCase):
         assert 'invalid@' in content
         # Check we didn't create the block already
         assert Block.objects.count() == 2
-        assert MultiBlockSubmit.objects.count() == 0
+        assert BlockSubmission.objects.count() == 0
 
-        # Create the block
+        # Create the block submission
         response = self.client.post(
             self.multi_url, {
                 'input_guids': (
@@ -329,8 +334,13 @@ class TestMultiBlockSubmitAdmin(TestCase):
             },
             follow=True)
         assert response.status_code == 200
+        return (
+            new_addon, existing_and_full, partial_addon, existing_and_partial)
+
+    def _test_add_multiple_verify_blocks(self, new_addon, existing_and_full,
+                                         partial_addon, existing_and_partial):
         assert Block.objects.count() == 3
-        assert MultiBlockSubmit.objects.count() == 1
+        assert BlockSubmission.objects.count() == 1
         all_blocks = Block.objects.all()
 
         new_block = all_blocks[2]
@@ -381,7 +391,7 @@ class TestMultiBlockSubmitAdmin(TestCase):
         assert not ActivityLog.objects.for_version(
             existing_and_full.addon.current_version).exists()
 
-        multi = MultiBlockSubmit.objects.get()
+        multi = BlockSubmission.objects.get()
         assert multi.input_guids == (
             'any@new\npartial@existing\nfull@existing\ninvalid@')
         assert multi.min_version == new_block.min_version
@@ -392,12 +402,37 @@ class TestMultiBlockSubmitAdmin(TestCase):
         assert multi.processed_guids == {
             'invalid_guids': ['invalid@'],
             'existing_guids': ['full@existing'],
-            'blocks': ['any@new', 'partial@existing'],
+            'toblock_guids': ['any@new', 'partial@existing'],
             'blocks_saved': [
                 [new_block.id, 'any@new'],
                 [existing_and_partial.id, 'partial@existing']],
         }
 
+    @override_switch('blocklist_admin_dualsignoff_disabled', active=True)
+    def test_submit_no_dual_signoff(self):
+        new_addon, existing_and_full, partial_addon, existing_and_partial = (
+            self._test_add_multiple_submit())
+        self._test_add_multiple_verify_blocks(
+            new_addon, existing_and_full, partial_addon, existing_and_partial)
+
+    @override_switch('blocklist_admin_dualsignoff_disabled', active=False)
+    def test_submit_dual_signoff(self):
+        new_addon, existing_and_full, partial_addon, existing_and_partial = (
+            self._test_add_multiple_submit())
+        # no new Block objects yet
+        assert Block.objects.count() == 2
+        # and existing block wasn't updated
+
+        multi = BlockSubmission.objects.get()
+        multi.update(
+            signoff_state=BlockSubmission.SIGNOFF_APPROVED,
+            signoff_by=user_factory())
+        assert multi.is_save_to_blocks_permitted
+        multi.save_to_blocks()
+        self._test_add_multiple_verify_blocks(
+            new_addon, existing_and_full, partial_addon, existing_and_partial)
+
+    @override_switch('blocklist_admin_dualsignoff_disabled', active=True)
     def test_add_and_edit_with_different_min_max_versions(self):
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
@@ -432,7 +467,7 @@ class TestMultiBlockSubmitAdmin(TestCase):
 
         # Check we didn't create the block already
         assert Block.objects.count() == 2
-        assert MultiBlockSubmit.objects.count() == 0
+        assert BlockSubmission.objects.count() == 0
 
         # Change the min/max versions
         response = self.client.post(
@@ -451,7 +486,7 @@ class TestMultiBlockSubmitAdmin(TestCase):
         assert response.status_code == 200
         # No Block should have been changed or added
         assert Block.objects.count() == 2
-        assert MultiBlockSubmit.objects.count() == 0
+        assert BlockSubmission.objects.count() == 0
 
         # The guids should have been processed differently now
         doc = pq(response.content)
@@ -476,7 +511,7 @@ class TestMultiBlockSubmitAdmin(TestCase):
 
         # This time the blocks are updated
         assert Block.objects.count() == 3
-        assert MultiBlockSubmit.objects.count() == 1
+        assert BlockSubmission.objects.count() == 1
         all_blocks = Block.objects.all()
 
         new_block = all_blocks[2]
@@ -528,7 +563,7 @@ class TestMultiBlockSubmitAdmin(TestCase):
         assert not ActivityLog.objects.for_version(
             existing_one_to_ten.addon.current_version).exists()
 
-        multi = MultiBlockSubmit.objects.get()
+        multi = BlockSubmission.objects.get()
         assert multi.input_guids == (
             'any@new\npartial@existing\nfull@existing')
         assert multi.min_version == new_block.min_version
@@ -539,7 +574,7 @@ class TestMultiBlockSubmitAdmin(TestCase):
         assert multi.processed_guids == {
             'invalid_guids': [],
             'existing_guids': ['partial@existing'],
-            'blocks': ['any@new', 'full@existing'],
+            'toblock_guids': ['any@new', 'full@existing'],
             'blocks_saved': [
                 [new_block.id, 'any@new'],
                 [existing_zero_to_max.id, 'full@existing']],
@@ -711,54 +746,237 @@ class TestMultiBlockSubmitAdmin(TestCase):
         assert existing.min_version == '1'  # check the values didn't update.
 
     def test_can_list(self):
-        mbs = MultiBlockSubmit.objects.create(
+        mbs = BlockSubmission.objects.create(
             updated_by=user_factory(display_name='Bób'))
-        user = user_factory()
-        self.grant_permission(user, 'Admin:Tools')
-        self.grant_permission(user, 'Reviews:Admin')
-        self.client.login(email=user.email)
-        response = self.client.get(self.multi_list_url, follow=True)
-        assert response.status_code == 200
-        assert 'Bób' in response.content.decode('utf-8')
-
         # add some guids to the multi block to test out the counts in the list
         addon_factory(guid='guid@', name='Danger Danger')
         mbs.update(input_guids='guid@\ninvalid@\nsecond@invalid')
         mbs.save()
-        assert mbs.processed_guids['existing_guids'] == []
-        # the order of invalid_guids is indeterminate.
-        assert set(mbs.processed_guids['invalid_guids']) == {
-            'invalid@', 'second@invalid'}
-        assert len(mbs.processed_guids['invalid_guids']) == 2
-        assert mbs.processed_guids['blocks'] == ['guid@']
+        assert mbs.processed_guids['toblock_guids'] == ['guid@']
+
+        user = user_factory()
+        self.grant_permission(user, 'Admin:Tools')
+        self.grant_permission(user, 'Reviews:Admin')
+        self.client.login(email=user.email)
+
         response = self.client.get(self.multi_list_url, follow=True)
+        assert response.status_code == 200
+        assert 'Bób' in response.content.decode('utf-8')
         doc = pq(response.content)
-        assert doc('td.field-invalid_guid_count').text() == '2'
-        assert doc('td.field-existing_guid_count').text() == '0'
-        assert doc('td.field-blocks_count').text() == '1'
-        assert doc('td.field-blocks_submitted_count').text() == '0'
+        assert doc('th.field-blocks_count').text() == '1 add-ons'
+        assert doc('.field-signoff_state').text() == 'Pending'
 
     def test_can_not_list_without_permission(self):
-        MultiBlockSubmit.objects.create(
+        BlockSubmission.objects.create(
             updated_by=user_factory(display_name='Bób'))
         user = user_factory()
         self.grant_permission(user, 'Admin:Tools')
         self.client.login(email=user.email)
+
         response = self.client.get(self.multi_list_url, follow=True)
         assert response.status_code == 403
         assert 'Bób' not in response.content.decode('utf-8')
 
-    def test_view(self):
+    def test_signoff_page(self):
         addon_factory(guid='guid@', name='Danger Danger')
-        mbs = MultiBlockSubmit.objects.create(
+        mbs = BlockSubmission.objects.create(
             input_guids='guid@\ninvalid@\nsecond@invalid',
-            updated_by=user_factory())
-        assert mbs.processed_guids['existing_guids'] == []
-        # the order of invalid_guids is indeterminate.
-        assert set(mbs.processed_guids['invalid_guids']) == {
-            'invalid@', 'second@invalid'}
-        assert len(mbs.processed_guids['invalid_guids']) == 2
-        assert mbs.processed_guids['blocks'] == ['guid@']
+            updated_by=user_factory(),
+            signoff_by=user_factory())
+        assert mbs.processed_guids['toblock_guids'] == ['guid@']
+
+        user = user_factory()
+        self.grant_permission(user, 'Admin:Tools')
+        self.grant_permission(user, 'Reviews:Admin')
+        self.client.login(email=user.email)
+        multi_url = reverse(
+            'admin:blocklist_blocksubmission_change', args=(mbs.id,))
+
+        response = self.client.get(multi_url, follow=True)
+        assert response.status_code == 200
+        assert b'guid@<br>invalid@<br>second@invalid' in response.content
+        doc = pq(response.content)
+        buttons = doc('.submit-row input')
+        assert buttons[0].attrib['value'] == 'Update'
+        assert buttons[1].attrib['value'] == 'Reject Submission'
+        assert buttons[2].attrib['value'] == 'Approve Submission'
+
+        response = self.client.post(
+            multi_url, {
+                'input_guids': 'guid2@\nfoo@baa',  # should be ignored
+                'min_version': '1',  # should be ignored
+                'max_version': '99',  # should be ignored
+                'url': 'new.url',
+                'reason': 'a reason',
+                '_save': 'Update',
+            },
+            follow=True)
+        assert response.status_code == 200
+        mbs = mbs.reload()
+
+        # the read-only values above weren't changed.
+        assert mbs.input_guids == 'guid@\ninvalid@\nsecond@invalid'
+        assert mbs.min_version == '0'
+        assert mbs.max_version == '*'
+        # but the other details were
+        assert mbs.url == 'new.url'
+        assert mbs.reason == 'a reason'
+
+        # The blocksubmission wasn't approved or rejected though
+        assert mbs.signoff_state == BlockSubmission.SIGNOFF_PENDING
+        assert Block.objects.count() == 0
+
+        log_entry = LogEntry.objects.get()
+        assert log_entry.user == user
+        assert log_entry.object_id == str(mbs.id)
+        assert log_entry.change_message == json.dumps(
+            [{'changed': {'fields': ['url', 'reason']}}])
+
+        response = self.client.get(multi_url, follow=True)
+        assert (
+            b'Changed &quot;Pending: guid@, ...; new.url; a reason' in
+            response.content)
+
+    def test_signoff_approve(self):
+        addon = addon_factory(guid='guid@', name='Danger Danger')
+        mbs = BlockSubmission.objects.create(
+            input_guids='guid@\ninvalid@',
+            updated_by=user_factory(),
+            signoff_by=user_factory())
+        assert mbs.processed_guids['toblock_guids'] == ['guid@']
+
+        user = user_factory()
+        self.grant_permission(user, 'Admin:Tools')
+        self.grant_permission(user, 'Reviews:Admin')
+        self.client.login(email=user.email)
+        multi_url = reverse(
+            'admin:blocklist_blocksubmission_change', args=(mbs.id,))
+        response = self.client.post(
+            multi_url, {
+                'input_guids': 'guid2@\nfoo@baa',  # should be ignored
+                'min_version': '1',  # should be ignored
+                'max_version': '99',  # should be ignored
+                'url': 'new.url',
+                'reason': 'a reason',
+                '_signoff': 'Approve Submission',
+            },
+            follow=True)
+        assert response.status_code == 200
+        mbs = mbs.reload()
+        assert mbs.signoff_by == user
+
+        # the read-only values above weren't changed.
+        assert mbs.input_guids == 'guid@\ninvalid@'
+        assert mbs.min_version == '0'
+        assert mbs.max_version == '*'
+        # but the other details were
+        assert mbs.url == 'new.url'
+        assert mbs.reason == 'a reason'
+
+        # As it was signed off, the block should have been created
+        assert Block.objects.count() == 1
+        new_block = Block.objects.get()
+
+        assert new_block.addon == addon
+        log = ActivityLog.objects.for_addons(addon).get()
+        assert log.action == amo.LOG.BLOCKLIST_BLOCK_ADDED.id
+        assert log.arguments == [addon, addon.guid, new_block]
+        assert log.details['min_version'] == '0'
+        assert log.details['max_version'] == '*'
+        assert log.details['reason'] == 'a reason'
+        block_log = ActivityLog.objects.for_block(new_block).filter(
+            action=log.action).last()
+        assert block_log == log
+        vlog = ActivityLog.objects.for_version(addon.current_version).last()
+        assert vlog == log
+
+        assert mbs.processed_guids == {
+            'invalid_guids': ['invalid@'],
+            'existing_guids': [],
+            'toblock_guids': ['guid@'],
+            'blocks_saved': [
+                [new_block.id, 'guid@'],
+            ],
+        }
+
+        log_entry = LogEntry.objects.last()
+        assert log_entry.user == user
+        assert log_entry.object_id == str(mbs.id)
+        other_obj = addon_factory(id=mbs.id)
+        LogEntry.objects.log_action(
+            user_factory().id, ContentType.objects.get_for_model(other_obj).pk,
+            other_obj.id, repr(other_obj), ADDITION, 'not a Block!')
+
+        response = self.client.get(multi_url, follow=True)
+        assert (
+            b'Changed &quot;Approved: guid@, ...; new.url; '
+            b'a reason&quot; - Sign-off Approval' in
+            response.content)
+        assert b'not a Block!' not in response.content
+
+    def test_signoff_reject(self):
+        addon_factory(guid='guid@', name='Danger Danger')
+        mbs = BlockSubmission.objects.create(
+            input_guids='guid@\ninvalid@',
+            updated_by=user_factory(),
+            signoff_by=user_factory())
+        assert mbs.processed_guids['toblock_guids'] == ['guid@']
+
+        user = user_factory()
+        self.grant_permission(user, 'Admin:Tools')
+        self.grant_permission(user, 'Reviews:Admin')
+        self.client.login(email=user.email)
+        multi_url = reverse(
+            'admin:blocklist_blocksubmission_change', args=(mbs.id,))
+        response = self.client.post(
+            multi_url, {
+                'input_guids': 'guid2@\nfoo@baa',  # should be ignored
+                'min_version': '1',  # should be ignored
+                'max_version': '99',  # should be ignored
+                'url': 'new.url',
+                'reason': 'a reason',
+                '_reject': 'Reject Submission',
+            },
+            follow=True)
+        assert response.status_code == 200
+        mbs = mbs.reload()
+
+        # the read-only values above weren't changed.
+        assert mbs.input_guids == 'guid@\ninvalid@'
+        assert mbs.min_version == '0'
+        assert mbs.max_version == '*'
+        # but the other details were
+        assert mbs.url == 'new.url'
+        assert mbs.reason == 'a reason'
+
+        # And the blocksubmission was rejected, so no Blocks created
+        assert mbs.signoff_state == BlockSubmission.SIGNOFF_REJECTED
+        assert Block.objects.count() == 0
+        assert not mbs.is_save_to_blocks_permitted
+
+        log_entry = LogEntry.objects.last()
+        assert log_entry.user == user
+        assert log_entry.object_id == str(mbs.id)
+        other_obj = addon_factory(id=mbs.id)
+        LogEntry.objects.log_action(
+            user_factory().id, ContentType.objects.get_for_model(other_obj).pk,
+            other_obj.id, repr(other_obj), ADDITION, 'not a Block!')
+
+        response = self.client.get(multi_url, follow=True)
+        assert (
+            b'Changed &quot;Rejected: guid@, ...; new.url; '
+            b'a reason&quot; - Sign-off Rejection' in
+            response.content)
+        assert b'not a Block!' not in response.content
+
+    def test_signed_off_view(self):
+        addon_factory(guid='guid@', name='Danger Danger')
+        mbs = BlockSubmission.objects.create(
+            input_guids='guid@\ninvalid@\nsecond@invalid',
+            updated_by=user_factory(),
+            signoff_by=user_factory(),
+            signoff_state=BlockSubmission.SIGNOFF_APPROVED)
+        assert mbs.processed_guids['toblock_guids'] == ['guid@']
         mbs.save_to_blocks()
         block = Block.objects.get()
 
@@ -767,18 +985,16 @@ class TestMultiBlockSubmitAdmin(TestCase):
         self.grant_permission(user, 'Reviews:Admin')
         self.client.login(email=user.email)
         multi_view_url = reverse(
-            'admin:blocklist_multiblocksubmit_change', args=(mbs.id,))
+            'admin:blocklist_blocksubmission_change', args=(mbs.id,))
 
         response = self.client.get(multi_view_url, follow=True)
         assert response.status_code == 200
-
         assert b'guid@<br>invalid@<br>second@invalid' in response.content
-
         doc = pq(response.content)
-        guid_link = doc('div.field-blocks_submitted div div a')
+        guid_link = doc('div.field-blocks div div a')
         assert guid_link.attr('href') == reverse(
             'admin:blocklist_block_change', args=(block.pk,))
-        assert guid_link.text() == 'guid@'
+        assert not doc('submit-row input')
 
 
 class TestBlockAdminEdit(TestCase):

src/olympia/blocklist/tests/test_models.py

@@ -1,7 +1,9 @@
-from olympia.amo.tests import TestCase
+from django.test.utils import override_settings
+
+from olympia.amo.tests import TestCase, user_factory
 from olympia.versions.compare import MAX_VERSION_PART
 
-from ..models import Block
+from ..models import Block, BlockSubmission
 
 
 class TestBlock(TestCase):
@@ -27,3 +29,37 @@ class TestBlock(TestCase):
         assert block.is_version_blocked('9')
         assert block.is_version_blocked('10.1')
         assert block.is_version_blocked('10.%s' % (MAX_VERSION_PART + 1))
+
+
+class TestMultiBlockSubmission(TestCase):
+    def test_is_save_to_blocks_permitted(self):
+        submitter = user_factory()
+        signoffer = user_factory()
+        block = BlockSubmission.objects.create()
+
+        # No signoff_by so not permitted
+        assert not block.signoff_state
+        assert not block.signoff_by
+        assert not block.is_save_to_blocks_permitted
+
+        # Except when the state is NOTNEEDED.
+        block.update(signoff_state=BlockSubmission.SIGNOFF_NOTNEEDED)
+        assert block.is_save_to_blocks_permitted
+
+        # But if the state is APPROVED we need to know the signoff user
+        block.update(signoff_state=BlockSubmission.SIGNOFF_APPROVED)
+        assert not block.is_save_to_blocks_permitted
+
+        # If different users update and signoff, it's permitted.
+        block.update(
+            updated_by=submitter,
+            signoff_by=signoffer)
+        assert block.is_save_to_blocks_permitted
+
+        # But not if submitter is also the sign off user.
+        block.update(signoff_by=submitter)
+        assert not block.is_save_to_blocks_permitted
+
+        # Except when that's not enforced locally
+        with override_settings(DEBUG=True):
+            assert block.is_save_to_blocks_permitted

src/olympia/blocklist/utils.py

@@ -84,3 +84,7 @@ def format_block_history(logs):
     return format_html(
         '<ul>\n{}\n</ul>',
         format_html_join_kw('\n', history_format_string, log_entries_gen))
+
+
+def splitlines(text):
+    return [line.strip() for line in str(text or '').splitlines()]

src/olympia/constants/permissions.py

@@ -122,8 +122,10 @@ DJANGO_PERMISSIONS_MAPPING.update({
     'blocklist.add_block': REVIEWS_ADMIN,
     'blocklist.change_block': REVIEWS_ADMIN,
     'blocklist.delete_block': REVIEWS_ADMIN,
-    'blocklist.add_multiblocksubmit': REVIEWS_ADMIN,
-    'blocklist.view_multiblocksubmit': REVIEWS_ADMIN,
+    'blocklist.add_blocksubmission': REVIEWS_ADMIN,
+    'blocklist.change_blocksubmission': REVIEWS_ADMIN,
+    'blocklist.delete_blocksubmission': REVIEWS_ADMIN,
+    'blocklist.view_blocksubmission': REVIEWS_ADMIN,
 
     'discovery.add_discoveryitem': DISCOVERY_EDIT,
     'discovery.change_discoveryitem': DISCOVERY_EDIT,

static/css/admin/blocklist_blocksubmission.css

@@ -0,0 +1,24 @@
+form ul.guid_list {
+    margin: 0;
+    border: solid 1px black;
+    max-height: 8em;
+    min-height: 8em;
+    overflow:hidden; overflow-y:scroll;
+    padding: 5px 20px;
+}
+
+form ul.guid_list li {
+    list-style-type: disc;
+}
+
+form ul.guid_list .addon-name {
+    font-weight: 800;
+}
+
+.field-blocks label {
+    display: none;
+}
+.field-blocks div.readonly {
+    margin-left: 0 !important;
+    margin-right: 0;
+}