Update developer agreement / submission process for post-review (#5754)

Update developer agreement / submission process for post-review

Every developer need to read the new agreement page again, which
now contains review rules & policies as well.

src/olympia/devhub/forms.py

@@ -836,3 +836,9 @@ class DistributionChoiceForm(happyforms.Form):
             ('listed', mark_safe_lazy(LISTED_LABEL)),
             ('unlisted', mark_safe_lazy(UNLISTED_LABEL))),
         widget=forms.RadioSelect(attrs={'class': 'channel'}))
+
+
+class AgreementForm(happyforms.Form):
+    distribution_agreement = forms.BooleanField()
+    review_policy = forms.BooleanField()
+    review_rules = forms.BooleanField()

src/olympia/devhub/templates/devhub/addons/submit/describe.html

@@ -123,11 +123,19 @@
     <div class="submission-buttons addon-submission-field">
       <button class="delete-button" type="sumbit"
               formaction="{{ url('devhub.addons.cancel', addon.slug) }}">
-          {{ _('Cancel Review and Disable Version') }}
+          {% if waffle.switch('post-review') and version.is_webextension %}
+            {{ _('Cancel and Disable Version') }}
+          {% else %}
+            {{ _('Cancel Review and Disable Version') }}
+          {% endif %}
       </button>
       &nbsp;
       <button type="submit">
-        {{ _('Submit Version for Review') }}
+        {% if waffle.switch('post-review') and version.is_webextension %}
+          {{ _('Submit Version') }}
+        {% else %}
+          {{ _('Submit Version for Review') }}
+        {% endif %}
       </button>
     </div>
   </form>

src/olympia/devhub/templates/devhub/addons/submit/describe_minimal.html

@@ -26,11 +26,19 @@
     <div class="submission-buttons addon-submission-field">
       <button class="delete-button" type="sumbit"
               formaction="{{ url('devhub.addons.cancel', addon.slug) }}">
-          {{ _('Cancel Review and Disable Version') }}
+          {% if waffle.switch('post-review') and version.is_webextension %}
+            {{ _('Cancel and Disable Version') }}
+          {% else %}
+            {{ _('Cancel Review and Disable Version') }}
+          {% endif %}
       </button>
       &nbsp;
       <button type="submit">
-        {{ _('Submit Version for Review') }}
+        {% if waffle.switch('post-review') and version.is_webextension %}
+          {{ _('Submit Version') }}
+        {% else %}
+          {{ _('Submit Version for Review') }}
+        {% endif %}
       </button>
     </div>
   </form>

src/olympia/devhub/templates/devhub/addons/submit/done.html

@@ -12,12 +12,19 @@
       {{ _("You’re done!") }}
     </p>
   {% else %}
-    <h3>{{ _("Version Submitted for Review") }}</h3>
+    {% if waffle.switch('post-review') and uploaded_version.is_webextension %}
-    <p>
+      <h3>{{ _("Version Submitted") }}</h3>
-      {{ _("You’re done! This version has been submitted for review. You will be "
+      <p>
-           "notified when the review has been completed, or if our reviewers have "
+        {{ _("You’re done! This version will be available on our site shortly.") }}
-           "any questions about your submission.") }}
+      </p>
-    </p>
+    {% else %}
+      <h3>{{ _("Version Submitted for Review") }}</h3>
+      <p>
+        {{ _("You’re done! This version has been submitted for review. You will be "
+             "notified when the review has been completed, or if our reviewers have "
+             "any questions about your submission.") }}
+      </p>
+    {% endif %}
     {% if submit_page == 'addon' %}
       <p>
         {{ _("Your listing will be more successful by adding a detailed description "

src/olympia/devhub/templates/devhub/agreement.html

@@ -1,25 +1,47 @@
-<p>
+<form method="post">
-{% trans %}
+  {% if waffle.switch('post-review') %}
-  Before starting, please read and accept our Firefox Add-on
+    <p>
-  Distribution Agreement. It also links to our Privacy Notice
+    {% trans %}
-  which explains how we handle your information.
+      Before starting, please read and accept our Firefox Add-on Distribution
-{% endtrans %}
+      Agreement as well as our Review Policies and Rules. The Firefox
-</p>
+      Add-on Distribution Agreement also links to our Privacy Notice which
-<div id="agreement-container">
+      explains how we handle your information.
-  {% include 'amo/developer_agreement.html' %}
+    {% endtrans %}
-</div>
+    </p>
-<div id="agreement-extra-links">
+    <ul class="agreement-links">
-  <a href="{{ url('devhub.docs', 'policies/agreement') }}"
+      <li>{{ agreement_form.distribution_agreement }}<a href="{{ url('devhub.docs', 'policies/agreement') }}" target="_blank" rel="noopener noreferrer">{{ _('Distribution Agreement') }}</a> {{ agreement_form.distribution_agreement.errors }}</li>
-     target="_blank" rel="noopener noreferrer">
+      <li>{{ agreement_form.review_policy }}<a href="{{ url('devhub.docs', 'policies/reviews') }}" target="_blank" rel="noopener noreferrer">{{ _('Review Policy') }}</a> {{ agreement_form.review_policy.errors }}</li>
-    {{ _('Printable Version') }}</a>
+      <li>{{ agreement_form.review_rules }}<a href="{{ url('devhub.docs', 'policies/rules') }}" target="_blank" rel="noopener noreferrer">{{ _('Review Rules') }}</a> {{ agreement_form.review_rules.errors }}</li>
-</div>
+    </ul>
-<div class="submit-buttons">
+  {% else %}
-  <form method="post">
+    <p>
-    {{ csrf() }}
+    {% trans %}
-    <button id="accept-agreement" type="submit">
+      Before starting, please read and accept our Firefox Add-on
-      {{ _('I Accept this Agreement') }}
+      Distribution Agreement. It also links to our Privacy Notice
-    </button>
+      which explains how we handle your information.
-  {{ _('or <a href="{0}">Cancel</a>')|fe(url('devhub.index')) }}
+    {% endtrans %}
-  </form>
+    </p>
-</div>
+    </p>
+    <div class="agreement-container agreement-links">
+      {% include 'amo/developer_agreement.html' %}
+    </div>
 
+    <div class="agreement-extra-links">
+      <a href="{{ url('devhub.docs', 'policies/agreement') }}"
+         target="_blank" rel="noopener noreferrer">
+        {{ _('Printable Version') }}</a>
+    </div>
+  {% endif %}
+
+  <div class="submit-buttons">
+      {{ csrf() }}
+      <button id="accept-agreement" type="submit">
+        {% if waffle.switch('post-review') %}
+          {{ _('I have read and accept this Agreement and the Rules and Policies') }}
+        {% else %}
+          {{ _('I Accept this Agreement') }}
+        {% endif %}
+      </button>
+    {{ _('or <a href="{0}">Cancel</a>')|fe(url('devhub.index')) }}
+  </div>
+</form>

src/olympia/devhub/tests/test_views_submit.py

@@ -6,6 +6,8 @@ from django.core.files import temp
 
 import mock
 from pyquery import PyQuery as pq
+from waffle.models import Switch
+from waffle.testutils import override_switch
 
 from olympia import amo
 from olympia.activity.models import ActivityLog
@@ -84,27 +86,27 @@ class TestSubmitBase(TestCase):
 
 
 class TestAddonSubmitAgreement(TestSubmitBase):
-    def test_step1_submit(self):
+    def test_submit_agreement_page_links(self):
         self.user.update(read_dev_agreement=None)
         response = self.client.get(reverse('devhub.submit.agreement'))
         assert response.status_code == 200
         doc = pq(response.content)
-        links = doc('#agreement-container a')
+        links = doc('.agreement-links a')
         assert links
         for ln in links:
             href = ln.attrib['href']
-            assert not href.startswith('%'), (
+            assert href.startswith(('https://', '/', 'mailto:')), (
                 "Looks like link %r to %r is still a placeholder" %
                 (href, ln.text))
 
-    def test_read_dev_agreement_set(self):
+    def test_set_read_dev_agreement(self):
         """Store current date when the user agrees with the user agreement."""
         self.user.update(read_dev_agreement=None)
 
-        response = self.client.post(reverse('devhub.submit.agreement'),
+        response = self.client.post(reverse('devhub.submit.agreement'))
-                                    follow=True)
+        assert response.status_code == 302
-        user = response.context['user']
+        self.user.reload()
-        self.assertCloseToNow(user.read_dev_agreement)
+        self.assertCloseToNow(self.user.read_dev_agreement)
 
     def test_read_dev_agreement_skip(self):
         # The current user fixture has already read the agreement so we skip
@@ -112,6 +114,44 @@ class TestAddonSubmitAgreement(TestSubmitBase):
         self.assert3xx(response, reverse('devhub.submit.distribution'))
 
 
+@override_switch('post-review', active=True)
+class TestAddonSubmitAgreementWithPostReviewEnabled(TestAddonSubmitAgreement):
+    def test_set_read_dev_agreement(self):
+        response = self.client.post(reverse('devhub.submit.agreement'), {
+            'distribution_agreement': 'on',
+            'review_policy': 'on',
+            'review_rules': 'on',
+        })
+        assert response.status_code == 302
+        self.user.reload()
+        self.assertCloseToNow(self.user.read_dev_agreement)
+
+    def test_set_read_dev_agreement_error(self):
+        response = self.client.post(reverse('devhub.submit.agreement'))
+        assert response.status_code == 200
+        assert 'agreement_form' in response.context
+        form = response.context['agreement_form']
+        assert form.is_valid() is False
+        assert form.errors == {
+            'distribution_agreement': [u'This field is required.'],
+            'review_policy': [u'This field is required.'],
+            'review_rules': [u'This field is required.']
+        }
+        doc = pq(response.content)
+        for id_ in form.errors.keys():
+            selector = 'li input#id_%s + a + .errorlist' % id_
+            assert doc(selector).text() == 'This field is required.'
+
+    def test_read_dev_agreement_skip(self):
+        # Make the switch modified date older so that the user read dev
+        # agreement date is more recent than the switch.
+        Switch.objects.filter(name='post-review').update(
+            modified=self.user.read_dev_agreement - timedelta(days=1))
+        super(
+            TestAddonSubmitAgreementWithPostReviewEnabled,
+            self).test_read_dev_agreement_skip()
+
+
 class TestAddonSubmitDistribution(TestCase):
     fixtures = ['base/users']
 
@@ -138,13 +178,20 @@ class TestAddonSubmitDistribution(TestCase):
         assert doc('.notification-box.warning').html().strip() == config.value
 
     def test_redirect_back_to_agreement(self):
-        # We require a cookie that gets set in step 1.
         self.user.update(read_dev_agreement=None)
 
         response = self.client.get(
             reverse('devhub.submit.distribution'), follow=True)
         self.assert3xx(response, reverse('devhub.submit.agreement'))
 
+        with override_switch('post-review', active=True):
+            # If the post-review waffle is enabled, read_dev_agreement also
+            # needs to be a more recent date than the waffle modification date.
+            self.user.update(read_dev_agreement=self.days_ago(42))
+            response = self.client.get(
+                reverse('devhub.submit.distribution'), follow=True)
+            self.assert3xx(response, reverse('devhub.submit.agreement'))
+
     def test_listed_redirects_to_next_step(self):
         response = self.client.post(reverse('devhub.submit.distribution'),
                                     {'channel': 'listed'})
@@ -655,6 +702,10 @@ class TestAddonSubmitFinish(TestSubmitBase):
         # Third back to my submissions.
         assert links[2].attrib['href'] == reverse('devhub.addons')
 
+    @override_switch('post-review', active=True)
+    def test_finish_submitting_listed_addon_with_post_review_enabled(self):
+        self.test_finish_submitting_listed_addon()
+
     def test_finish_submitting_unlisted_addon(self):
         self.make_addon_unlisted(self.addon)
 
@@ -675,6 +726,10 @@ class TestAddonSubmitFinish(TestSubmitBase):
         # Second back to my submissions.
         assert links[1].attrib['href'] == reverse('devhub.addons')
 
+    @override_switch('post-review', active=True)
+    def test_finish_submitting_unlisted_addon_with_post_review_enabled(self):
+        self.test_finish_submitting_unlisted_addon()
+
     @mock.patch('olympia.devhub.tasks.send_welcome_email.delay', new=mock.Mock)
     def test_finish_submitting_platform_specific_listed_addon(self):
         latest_version = self.addon.find_latest_version(
@@ -789,6 +844,13 @@ class TestVersionSubmitDistribution(TestSubmitBase):
         response = self.client.get(self.url)
         assert response.status_code == 200
 
+    def test_has_read_agreement(self):
+        self.user.update(read_dev_agreement=None)
+        response = self.client.get(self.url)
+        self.assert3xx(
+            response,
+            reverse('devhub.submit.version.agreement', args=[self.addon.slug]))
+
 
 class TestVersionSubmitAutoChannel(TestSubmitBase):
     """ Just check we chose the right upload channel.  The upload tests
@@ -826,6 +888,13 @@ class TestVersionSubmitAutoChannel(TestSubmitBase):
             reverse('devhub.submit.version.distribution',
                     args=[self.addon.slug]))
 
+    def test_has_read_agreement(self):
+        self.user.update(read_dev_agreement=None)
+        response = self.client.get(self.url)
+        self.assert3xx(
+            response,
+            reverse('devhub.submit.version.agreement', args=[self.addon.slug]))
+
 
 class VersionSubmitUploadMixin(object):
     channel = None
@@ -1125,6 +1194,10 @@ class TestVersionSubmitDetails(TestSubmitBase):
         assert self.version.approvalnotes == 'approove plz'
         assert self.version.releasenotes == 'loadsa stuff'
 
+    @override_switch('post-review', active=True)
+    def test_submit_success_with_post_review_enabled(self):
+        self.test_submit_success()
+
     def test_submit_details_unlisted_should_redirect(self):
         self.version.update(channel=amo.RELEASE_CHANNEL_UNLISTED)
         assert all(self.get_addon().get_required_metadata())

src/olympia/devhub/urls.py

@@ -68,6 +68,9 @@ detail_patterns = [
     url('^versions/submit/$',
         views.submit_version_auto,
         name='devhub.submit.version'),
+    url('^versions/submit/agreement$',
+        views.submit_version_agreement,
+        name='devhub.submit.version.agreement'),
     url('^versions/submit/distribution$',
         views.submit_version_distribution,
         name='devhub.submit.version.distribution'),

src/olympia/devhub/views.py

@@ -11,12 +11,14 @@ from django.core.exceptions import PermissionDenied
 from django.core.files.storage import default_storage as storage
 from django.db import transaction
 from django.db.models import Count
+from django.forms import Form
 from django.shortcuts import get_object_or_404, redirect
 from django.template import Context, loader
 from django.utils.translation import ugettext, ugettext_lazy as _
 from django.views.decorators.cache import never_cache
 from django.views.decorators.csrf import csrf_exempt
 
+import waffle
 from django_statsd.clients import statsd
 from PIL import Image
 
@@ -40,7 +42,7 @@ from olympia.amo.utils import escape_all, MenuItem, send_mail, render
 from olympia.api.models import APIKey
 from olympia.applications.models import AppVersion
 from olympia.devhub.decorators import dev_required, no_admin_disabled
-from olympia.devhub.forms import CheckCompatibilityForm
+from olympia.devhub.forms import AgreementForm, CheckCompatibilityForm
 from olympia.devhub.models import BlogPost, RssKey
 from olympia.devhub.utils import process_validation
 from olympia.editors.helpers import get_position, ReviewHelper
@@ -1282,10 +1284,16 @@ def submit_addon(request):
                             'devhub.submit.distribution')
 
 
+@dev_required
+def submit_version_agreement(request, addon_id, addon):
+    return render_agreement(
+        request, 'devhub/addons/submit/start.html',
+        reverse('devhub.submit.version', args=(addon.slug,)),
+        submit_page='version')
+
+
 @transaction.atomic
 def _submit_distribution(request, addon, next_view):
-    if request.user.read_dev_agreement is None:
-        return redirect('devhub.submit.agreement')
     # Accept GET for the first load so we can preselect the channel.
     form = forms.DistributionChoiceForm(
         request.POST if request.method == 'POST' else
@@ -1305,11 +1313,15 @@ def _submit_distribution(request, addon, next_view):
 
 @login_required
 def submit_addon_distribution(request):
+    if not request.user.has_read_developer_agreement():
+        return redirect('devhub.submit.agreement')
     return _submit_distribution(request, None, 'devhub.submit.upload')
 
 
 @dev_required(submitting=True)
 def submit_version_distribution(request, addon_id, addon):
+    if not request.user.has_read_developer_agreement():
+        return redirect('devhub.submit.version.agreement', addon.slug)
     return _submit_distribution(request, addon, 'devhub.submit.version.upload')
 
 
@@ -1430,6 +1442,8 @@ def submit_version_upload(request, addon_id, addon, channel):
 @dev_required
 @no_admin_disabled
 def submit_version_auto(request, addon_id, addon):
+    if not request.user.has_read_developer_agreement():
+        return redirect('devhub.submit.version.agreement', addon.slug)
     # choose the channel we need from the last upload
     last_version = addon.find_latest_version(None, exclude=())
     if not last_version:
@@ -1450,7 +1464,6 @@ def submit_file(request, addon_id, addon, version_id):
 
 
 def _submit_details(request, addon, version):
-    forms_list, context = [], {}
     if version and version.channel == amo.RELEASE_CHANNEL_UNLISTED:
         # Not a listed version ? Then nothing to do here.
         return redirect('devhub.submit.version.finish', addon.slug, version.pk)
@@ -1461,6 +1474,11 @@ def _submit_details(request, addon, version):
     if not latest_version:
         # No listed version ? Then nothing to do in the listed submission flow.
         return redirect('devhub.submit.finish', addon.slug)
+    forms_list = []
+    context = {
+        'addon': addon,
+        'version': version,
+    }
     post_data = request.POST if request.method == 'POST' else None
     show_all_fields = not version or not addon.has_complete_metadata()
 
@@ -1670,9 +1688,8 @@ def docs(request, doc_name=None):
         'themes': '/Themes/Background',
         'themes/faq': '/Themes/Background/FAQ',
         'policies': '/AMO/Policy',
-        'policies/submission': '/AMO/Policy/Submission',
         'policies/reviews': '/AMO/Policy/Reviews',
-        'policies/maintenance': '/AMO/Policy/Maintenance',
+        'policies/rules': '/AMO/Policy/Rules',
         'policies/contact': '/AMO/Policy/Contact',
         'policies/agreement': '/AMO/Policy/Agreement',
     }
@@ -1690,14 +1707,29 @@ def api_key_agreement(request):
     return render_agreement(request, 'devhub/api/agreement.html', next_step)
 
 
-def render_agreement(request, template, next_step):
+def render_agreement(request, template, next_step, **extra_context):
-    if request.method == 'POST':
+    new_style_agreement = waffle.switch_is_active('post-review')
+    # If using the new style agreement, use AgreementForm, otherwise just an
+    # empty django Form that will always be valid when you POST things to it.
+    form_class = AgreementForm if new_style_agreement else Form
+    form = form_class(request.POST if request.method == 'POST' else None)
+    if request.method == 'POST' and form.is_valid():
+        # Developer has validated the form: let's update its profile and
+        # redirect to next step.
         request.user.update(read_dev_agreement=datetime.datetime.now())
         return redirect(next_step)
-
+    elif not request.user.has_read_developer_agreement():
-    if request.user.read_dev_agreement is None:
+        # Developer has either posted an invalid form or just landed on the
-        return render(request, template)
+        # page but haven't read the agreement yet: show the form (with
+        # potential errors highlighted)
+        context = {
+            'agreement_form': form,
+        }
+        context.update(extra_context)
+        return render(request, template, context)
     else:
+        # The developer has already read the agreement, we should just redirect
+        # to the next step.
         response = redirect(next_step)
         return response
 
@@ -1705,7 +1737,7 @@ def render_agreement(request, template, next_step):
 @login_required
 @transaction.atomic
 def api_key(request):
-    if request.user.read_dev_agreement is None:
+    if not request.user.has_read_developer_agreement():
         return redirect(reverse('devhub.api_key_agreement'))
 
     try:

src/olympia/users/models.py

@@ -16,6 +16,8 @@ from django.utils.encoding import force_text
 from django.utils.functional import cached_property, lazy
 
 import caching.base as caching
+import waffle
+from waffle.models import Switch
 
 import olympia.core.logger
 from olympia import amo, core
@@ -176,6 +178,18 @@ class UserProfile(OnChangeMixin, ModelBase, AbstractBaseUser):
     def has_module_perms(self, app_label):
         return self.is_superuser
 
+    def has_read_developer_agreement(self):
+        if self.read_dev_agreement is None:
+            return False
+        if waffle.switch_is_active('post-review'):
+            # We want to make sure developers read the latest version of the
+            # agreement. The cutover date is the date the switch was last
+            # modified to turn it on. (When removing the waffle, change this
+            # for a static date).
+            switch = Switch.objects.get(name='post-review')
+            return self.read_dev_agreement > switch.modified
+        return True
+
     backend = 'django.contrib.auth.backends.ModelBackend'
 
     def get_session_auth_hash(self):

src/olympia/users/tests/test_models.py

@@ -7,6 +7,8 @@ from django.db import models, migrations
 from django.db.migrations.writer import MigrationWriter
 
 import pytest
+from waffle.models import Switch
+from waffle.testutils import override_switch
 
 import olympia  # noqa
 from olympia import amo
@@ -294,6 +296,43 @@ class TestUserProfile(TestCase):
         hash2 = user.get_session_auth_hash()
         assert hash1 != hash2
 
+    def test_has_read_developer_agreement(self):
+        now = self.days_ago(0)
+        recently = self.days_ago(1)
+        older_date = self.days_ago(42)
+
+        assert not UserProfile().has_read_developer_agreement()
+        assert not UserProfile(
+            read_dev_agreement=None).has_read_developer_agreement()
+        assert UserProfile(
+            read_dev_agreement=older_date).has_read_developer_agreement()
+        with override_switch('post-review', active=True):
+            Switch.objects.filter(name='post-review').update(modified=recently)
+
+            # Still False.
+            assert not UserProfile().has_read_developer_agreement()
+
+            # User has read the agreement, before it was modified for
+            # post-review: it should return False.
+            assert not UserProfile(
+                read_dev_agreement=older_date).has_read_developer_agreement()
+            # User has read the agreement after it was modified for
+            # post-review: it should return True.
+            assert UserProfile(
+                read_dev_agreement=now).has_read_developer_agreement()
+
+        with override_switch('post-review', active=False):
+            Switch.objects.filter(name='post-review').update(modified=recently)
+
+            # Still False.
+            assert not UserProfile().has_read_developer_agreement()
+
+            # Both should be True, the date does not matter any more.
+            assert UserProfile(
+                read_dev_agreement=older_date).has_read_developer_agreement()
+            assert UserProfile(
+                read_dev_agreement=now).has_read_developer_agreement()
+
 
 class TestDeniedName(TestCase):
     fixtures = ['users/test_backends']

static/css/devhub/submission.less

@@ -26,11 +26,11 @@
 }
 
 /* @group Add-on Submission */
-#agreement-container {
+.agreement-container {
     background: #fff;
     border: 1px solid #6a89b0;
     margin: 0 0 10px 0;
-    max-height: 300px;
+    max-height: 175px;
     overflow: auto;
     padding: 15px;
 
@@ -60,7 +60,7 @@ ol.submit-addon-progress.unlisted {
     }
 }
 
-#agreement-extra-links {
+.agreement-extra-links {
     font-size: 90%;
     margin-bottom: 2em;
     text-align: right;