Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 3 additions & 23 deletions apps/devhub/templates/devhub/addons/submit/start.html
Original file line number Diff line number Diff line change
@@ -1,31 +1,11 @@
{% extends "devhub/addons/submit/base.html" %}


{% block title %}{{ dev_page_title(_('Step 1'), addon) }}{% endblock %}

{% block primary %}
<h3>{{ _('Step 1. Getting Started') }}</h3>

<p>
{% 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 %}
</p>
<div id="agreement-container">
{% include 'amo/developer_agreement.html' %}
</div>
<div id="agreement-extra-links">
<a href="{{ url('devhub.docs', 'policies/agreement') }}">
{{ _('Printable Version') }}</a>
</div>
<div class="submit-buttons">
<form method="post">
{{ csrf() }}
<button id="accept-agreement" type="submit">
{{ _('I Accept this Agreement') }}
</button>
{{ _('or <a href="{0}">Cancel</a>')|f(url('devhub.index')) }}
</form>
</div>
{% include "devhub/agreement.html" %}

{% endblock %}
24 changes: 24 additions & 0 deletions apps/devhub/templates/devhub/agreement.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<p>
{% 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 %}
</p>
<div id="agreement-container">
{% include 'amo/developer_agreement.html' %}
</div>
<div id="agreement-extra-links">
<a href="{{ url('devhub.docs', 'policies/agreement') }}">
{{ _('Printable Version') }}</a>
</div>
<div class="submit-buttons">
<form method="post">
{{ csrf() }}
<button id="accept-agreement" type="submit">
{{ _('I Accept this Agreement') }}
</button>
{{ _('or <a href="{0}">Cancel</a>')|f(url('devhub.index')) }}
</form>
</div>

21 changes: 21 additions & 0 deletions apps/devhub/templates/devhub/api/agreement.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{% extends "devhub/base.html" %}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this template isn't used anymore so it can be deleted

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? All sorts of templates extend from this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant apps/devhub/templates/devhub/api/agreement.html - where is this used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, so here is where I don't understand the UX of the current flow. Here is what happens in our hypothetical flow.

  1. User logs into AMO
  2. User clicks nav link to generate API key
  3. User is redirected to this page
    screen shot 2015-10-05 at 4 02 56 pm
  4. User accepts and is sent to actual API page

Now the issue here is that the page shown has nothing to do with API keys. it has a side bar that has a whole bunch of steps, none of which mean anything to someone trying to reach the API page. I must have missed this back in my last commit but this is why I suggested having a separate page for accepting the license.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, this is fixed now


{% set title = _('Firefox Add-on Distribution Agreement') %}

{% block title %}
{{ dev_page_title(title) }}
{% endblock %}

{% block content %}

{% set page_title = _('Firefox Add-on Distribution Agreement') %}
<header>
{{ dev_breadcrumbs(items=[(None, page_title)]) }}
<h2 class="is_addon">{{ title }}</h2>
</header>

<section class="addon-submission-process" role="main">
{% include "devhub/agreement.html" %}
</section>
{% endblock content %}

40 changes: 34 additions & 6 deletions apps/devhub/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from datetime import datetime, timedelta
from decimal import Decimal

from django import http
from django.conf import settings
from django.core.files.storage import default_storage as storage
from django.core.files import temp
Expand Down Expand Up @@ -1112,6 +1113,7 @@ class TestSubmitBase(amo.tests.TestCase):
def setUp(self):
super(TestSubmitBase, self).setUp()
assert self.client.login(username='del@icio.us', password='password')
self.user = UserProfile.objects.get(email='del@icio.us')
self.addon = self.get_addon()

def get_addon(self):
Expand All @@ -1124,9 +1126,25 @@ def get_step(self):
return SubmitStep.objects.get(addon=self.get_addon())


class TestSubmitStep1(TestSubmitBase):
class TestAgreement(TestSubmitBase):
def test_agreement_first(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is a duplicate of TestSubmitStep1 which will just slow down the suite and create extra test code to maintain.

Instead, I'd suggest asserting contractually that the new view you created is calling the submit() handler. You can do that with mock objects like this:

import mock
from django import http
...
with mock.patch('devhub.views.submit') as mock_submit:
    mock_submit.return_value = http.HttpResponse('')
    self.client.get(reverse('devhub.api_key_agreement'))
assert mock_submit.called

This will guarantee that the view still works by asserting it inherits all the properties of a known-to-work view handler, the submit() function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since that the agreement page had to be abstracted into a separate page we can't mock the submit function. We pass the parameter of the page to actually render into a separate function and then mock it in both this function and the one that tests submit but at that point I think it ends up making the tests confusing to the point of where it isn't worth it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, this is fixed now

with mock.patch('devhub.views.render_agreement') as mock_submit:
mock_submit.return_value = http.HttpResponse("Okay")
self.client.get(reverse('devhub.api_key_agreement'))
assert mock_submit.called

def test_agreement_second(self):
self.user.update(read_dev_agreement=None)

response = self.client.post(reverse('devhub.api_key_agreement'),
follow=True)

self.assertRedirects(response, reverse('devhub.api_key'))


class TestSubmitStep1(TestSubmitBase):
def test_step1_submit(self):
self.user.update(read_dev_agreement=None)
response = self.client.get(reverse('devhub.submit.1'))
eq_(response.status_code, 200)
doc = pq(response.content)
Expand All @@ -1141,12 +1159,16 @@ def test_step1_submit(self):

def test_read_dev_agreement_set(self):
"""Store current date when the user agrees with the user agreement."""
response = self.client.get(reverse('devhub.submit.1'))
previous = response.context['user'].read_dev_agreement
self.user.update(read_dev_agreement=None)

response = self.client.post(reverse('devhub.submit.1'), follow=True)
user = response.context['user']
assert user.read_dev_agreement != previous
self.assertCloseToNow(user.read_dev_agreement)

def test_read_dev_agreement_skip(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for readability, I think it would be good to add a comment saying "the logged in user already has a read_dev_agreement timestamp." This is part of why I dislike fixtures -- they hide all the data requirements for each test

# The current user fixture has already read the agreement so we skip
response = self.client.get(reverse('devhub.submit.1'))
self.assertRedirects(response, reverse('devhub.submit.2'))


class TestSubmitStep2(amo.tests.TestCase):
Expand All @@ -1156,15 +1178,18 @@ class TestSubmitStep2(amo.tests.TestCase):
def setUp(self):
super(TestSubmitStep2, self).setUp()
self.client.login(username='regular@mozilla.com', password='password')
self.user = UserProfile.objects.get(email='regular@mozilla.com')

def test_step_2_with_cookie(self):
def test_step_2_seen(self):
r = self.client.post(reverse('devhub.submit.1'))
self.assertRedirects(r, reverse('devhub.submit.2'))
r = self.client.get(reverse('devhub.submit.2'))
eq_(r.status_code, 200)

def test_step_2_no_cookie(self):
def test_step_2_not_seen(self):
# We require a cookie that gets set in step 1.
self.user.update(read_dev_agreement=None)

r = self.client.get(reverse('devhub.submit.2'), follow=True)
self.assertRedirects(r, reverse('devhub.submit.1'))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because the submit view has a new feature where it skips the agreement if already read, you need to add a test for that. It would look something like this:

self.user.update(read_dev_agreement=now())
res = self.client.get(reverse('devhub.submit.1'), follow=True)
self.assertRedirects(res, reverse('devhub.submit.2'))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't that test already exist here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly. Those tests are checking that a developer can post (to say they've read the agreement) and then continue. This does not cover the case where a developer has logged out, gone off, then returned to the devhub site later. What you need is to initialize their user with read_dev_agreement=now(), load the first view with a GET (not a post), and make sure it is redirected.


Expand Down Expand Up @@ -1856,6 +1881,7 @@ class TestSubmitSteps(amo.tests.TestCase):
def setUp(self):
super(TestSubmitSteps, self).setUp()
assert self.client.login(username='del@icio.us', password='password')
self.user = UserProfile.objects.get(email='del@icio.us')

def assert_linked(self, doc, numbers):
"""Check that the nth <li> in the steps list is a link."""
Expand All @@ -1875,6 +1901,7 @@ def assert_highlight(self, doc, num):
eq_(len(pq('.current', lis)), 1)

def test_step_1(self):
self.user.update(read_dev_agreement=None)
r = self.client.get(reverse('devhub.submit.1'))
eq_(r.status_code, 200)

Expand All @@ -1899,6 +1926,7 @@ def test_all_done(self):
self.assertRedirects(r, reverse('devhub.submit.7', args=['a3615']))

def test_menu_step_1(self):
self.user.update(read_dev_agreement=None)
doc = pq(self.client.get(reverse('devhub.submit.1')).content)
self.assert_linked(doc, [1])
self.assert_highlight(doc, 1)
Expand Down
6 changes: 6 additions & 0 deletions apps/devhub/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,12 @@
url('^addon/submit/1$', views.submit, name='devhub.submit.1'),
url('^addon/submit/2$', views.submit_addon, name='devhub.submit.2'),

# Submission API
url('^addon/submit/agreement/$', views.api_key_agreement,
name='devhub.api_key_agreement'),

url('^addon/api/key/$', views.api_key, name='devhub.api_key'),

# Standalone validator:
url('^addon/validate/?$', views.validate_addon,
name='devhub.validate_addon'),
Expand Down
35 changes: 26 additions & 9 deletions apps/devhub/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@


# We use a session cookie to make sure people see the dev agreement.
DEV_AGREEMENT_COOKIE = 'yes-I-read-the-dev-agreement'

MDN_BASE = 'https://developer.mozilla.org/Add-ons'

Expand Down Expand Up @@ -1387,19 +1386,14 @@ def _step_url(step):
@login_required
@submit_step(1)
def submit(request, step):
if request.method == 'POST':
request.user.update(read_dev_agreement=now())
response = redirect(_step_url(2))
response.set_cookie(DEV_AGREEMENT_COOKIE)
return response

return render(request, 'devhub/addons/submit/start.html', {'step': step})
return render_agreement(request, 'devhub/addons/submit/start.html',
_step_url(2), step)


@login_required
@submit_step(2)
def submit_addon(request, step):
if DEV_AGREEMENT_COOKIE not in request.COOKIES:
if request.user.read_dev_agreement is None:
return redirect(_step_url(1))
form = forms.NewAddonForm(
request.POST or None,
Expand Down Expand Up @@ -1735,3 +1729,26 @@ def docs(request, doc_name=None):
def search(request):
query = request.GET.get('q', '')
return render(request, 'devhub/devhub_search.html', {'query': query})


def api_key_agreement(request):
next_step = reverse('devhub.api_key')
return render_agreement(request, 'devhub/api/agreement.html', next_step)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even though this is a positional argument, python will let you pass it as a keyword arg for readability like render_agreement(..., next_step=reverse('devhub.api_key'))

This isn't something you need to change, just letting you know.



def render_agreement(request, template, next_step, step=None):
if request.method == 'POST':
request.user.update(read_dev_agreement=now())
return redirect(next_step)

if request.user.read_dev_agreement is None:
return render(request, template,
{'step': step})
else:
response = redirect(next_step)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need some improvement in the future: a dev uploading a new add-on will get redirected straight to step2 if he already accepted the agreement. Clicking on the big fat unread link "1. Getting started" will have no effect (other than redirecting straight to the same step 2).

I believe it would make more sense to have this "step 1" taken out of the flow, and just be an extra initial requirement (deliberately not talking about a "step" here) to the step 1 (which is uploading the add-on).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the developer submission flow working as expected after this change? If it's not, we should fix it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return response


def api_key(request):
"""TODO: this is simply a stub to be filled in at a later date"""
return http.HttpResponse('Ok')