From ac2b7974fa09822fb3989ac906cea338bfc79ebf Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Tue, 27 Jun 2017 11:12:42 -0700 Subject: [PATCH] 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 | 6 ++ .../devhub/addons/submit/describe.html | 12 ++- .../addons/submit/describe_minimal.html | 12 ++- .../templates/devhub/addons/submit/done.html | 19 ++-- .../devhub/templates/devhub/agreement.html | 70 +++++++++----- src/olympia/devhub/tests/test_views_submit.py | 91 +++++++++++++++++-- src/olympia/devhub/urls.py | 3 + src/olympia/devhub/views.py | 56 +++++++++--- src/olympia/users/models.py | 14 +++ src/olympia/users/tests/test_models.py | 39 ++++++++ static/css/devhub/submission.less | 6 +- 11 files changed, 270 insertions(+), 58 deletions(-) diff --git a/src/olympia/devhub/forms.py b/src/olympia/devhub/forms.py index c89baa58fe..c8e87950fd 100644 --- a/src/olympia/devhub/forms.py +++ b/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() diff --git a/src/olympia/devhub/templates/devhub/addons/submit/describe.html b/src/olympia/devhub/templates/devhub/addons/submit/describe.html index 7130cb9c3b..1fd454c77c 100644 --- a/src/olympia/devhub/templates/devhub/addons/submit/describe.html +++ b/src/olympia/devhub/templates/devhub/addons/submit/describe.html @@ -123,11 +123,19 @@
 
diff --git a/src/olympia/devhub/templates/devhub/addons/submit/describe_minimal.html b/src/olympia/devhub/templates/devhub/addons/submit/describe_minimal.html index be2cd4dbdd..3ccf1aba6e 100644 --- a/src/olympia/devhub/templates/devhub/addons/submit/describe_minimal.html +++ b/src/olympia/devhub/templates/devhub/addons/submit/describe_minimal.html @@ -26,11 +26,19 @@
 
diff --git a/src/olympia/devhub/templates/devhub/addons/submit/done.html b/src/olympia/devhub/templates/devhub/addons/submit/done.html index 093e83a8a2..d60270ed23 100644 --- a/src/olympia/devhub/templates/devhub/addons/submit/done.html +++ b/src/olympia/devhub/templates/devhub/addons/submit/done.html @@ -12,12 +12,19 @@ {{ _("You’re done!") }}

{% else %} -

{{ _("Version Submitted for Review") }}

-

- {{ _("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.") }} -

+ {% if waffle.switch('post-review') and uploaded_version.is_webextension %} +

{{ _("Version Submitted") }}

+

+ {{ _("You’re done! This version will be available on our site shortly.") }} +

+ {% else %} +

{{ _("Version Submitted for Review") }}

+

+ {{ _("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.") }} +

+ {% endif %} {% if submit_page == 'addon' %}

{{ _("Your listing will be more successful by adding a detailed description " diff --git a/src/olympia/devhub/templates/devhub/agreement.html b/src/olympia/devhub/templates/devhub/agreement.html index f45d898523..c3e50d823e 100644 --- a/src/olympia/devhub/templates/devhub/agreement.html +++ b/src/olympia/devhub/templates/devhub/agreement.html @@ -1,25 +1,47 @@ -

-{% trans %} - Before starting, please read and accept our Firefox Add-on - Distribution Agreement. It also links to our Privacy Notice - which explains how we handle your information. -{% endtrans %} -

-
- {% include 'amo/developer_agreement.html' %} -
- -
-
- {{ csrf() }} - - {{ _('or Cancel')|fe(url('devhub.index')) }} -
-
+
+ {% if waffle.switch('post-review') %} +

+ {% trans %} + Before starting, please read and accept our Firefox Add-on Distribution + Agreement as well as our Review Policies and Rules. The Firefox + Add-on Distribution Agreement also links to our Privacy Notice which + explains how we handle your information. + {% endtrans %} +

+ + {% else %} +

+ {% trans %} + Before starting, please read and accept our Firefox Add-on + Distribution Agreement. It also links to our Privacy Notice + which explains how we handle your information. + {% endtrans %} +

+

+
+ {% include 'amo/developer_agreement.html' %} +
+
+ + {{ _('Printable Version') }} +
+ {% endif %} + +
+ {{ csrf() }} + + {{ _('or Cancel')|fe(url('devhub.index')) }} +
+
diff --git a/src/olympia/devhub/tests/test_views_submit.py b/src/olympia/devhub/tests/test_views_submit.py index f1a104f184..889b9d0493 100644 --- a/src/olympia/devhub/tests/test_views_submit.py +++ b/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'), - follow=True) - user = response.context['user'] - self.assertCloseToNow(user.read_dev_agreement) + response = self.client.post(reverse('devhub.submit.agreement')) + assert response.status_code == 302 + self.user.reload() + 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()) diff --git a/src/olympia/devhub/urls.py b/src/olympia/devhub/urls.py index 71411cd2a7..da8a541a03 100644 --- a/src/olympia/devhub/urls.py +++ b/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'), diff --git a/src/olympia/devhub/views.py b/src/olympia/devhub/views.py index 58f28fd871..503237798d 100644 --- a/src/olympia/devhub/views.py +++ b/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): - if request.method == 'POST': +def render_agreement(request, template, next_step, **extra_context): + 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) - - if request.user.read_dev_agreement is None: - return render(request, template) + elif not request.user.has_read_developer_agreement(): + # Developer has either posted an invalid form or just landed on the + # 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: diff --git a/src/olympia/users/models.py b/src/olympia/users/models.py index a038c43812..114a634b23 100644 --- a/src/olympia/users/models.py +++ b/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): diff --git a/src/olympia/users/tests/test_models.py b/src/olympia/users/tests/test_models.py index 631b575aae..6bc57e2020 100644 --- a/src/olympia/users/tests/test_models.py +++ b/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'] diff --git a/static/css/devhub/submission.less b/static/css/devhub/submission.less index 07b194da7b..ba450cb43f 100644 --- a/static/css/devhub/submission.less +++ b/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;