Skip to content
Browse files

[bug 969667] Replace mobile/desktop forms with generic

* nixes desktop and mobile form routing (yay!)
* replaces that with new generic form
* update tests to denote that we now essentially ignore validation
  errors with email addresses and descriptions--Thanks!
* update ancient android form post handling to also ignore errors
* clean up code after all that
  • Loading branch information...
1 parent e708164 commit 600f02c5e94b29dd8d4aeff725969e8d51a175a2 @willkg willkg committed
Showing with 151 additions and 282 deletions.
  1. +0 −27 fjord/feedback/forms.py
  2. +41 −78 fjord/feedback/tests/test_views.py
  3. +110 −177 fjord/feedback/views.py
View
27 fjord/feedback/forms.py
@@ -30,30 +30,3 @@ class ResponseForm(forms.Form):
attrs={'class': 'manufacturer'}))
device = forms.CharField(required=False, widget=forms.HiddenInput(
attrs={'class': 'device'}))
-
- def clean(self):
- cleaned_data = super(ResponseForm, self).clean()
-
- email_ok = cleaned_data.get('email_ok')
- email = cleaned_data.get('email')
-
- if email_ok and not email or 'email' in self._errors:
- self._errors['email'] = self.error_class([
- _(u'Please enter a valid email address, or uncheck the '
- 'box allowing us to contact you.')
- ])
-
- # If email_ok is not checked, ignore errors on email.
- if not email_ok and 'email' in self._errors:
- del self._errors['email']
-
- return cleaned_data
-
- def clean_description(self):
- # If the response description consists solely of whitespace,
- # kick up an error.
- description = self.cleaned_data.get('description')
- if not description or not description.strip():
- raise forms.ValidationError(_(u'This field is required.'))
-
- return description
View
119 fjord/feedback/tests/test_views.py
@@ -38,7 +38,7 @@ def test_valid_happy(self):
"""
amount = models.Response.objects.count()
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
r = self.client.post(url, {
'happy': 1,
'description': u'Firefox rocks!',
@@ -67,7 +67,7 @@ def test_valid_sad(self):
"""
amount = models.Response.objects.count()
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
r = self.client.post(url, {
'happy': 0,
'description': u"Firefox doesn't make me sandwiches. :("
@@ -230,56 +230,56 @@ def test_urls_product_version_channel_android_ua(self):
eq_(u'nightly', feedback.channel)
def test_invalid_form(self):
- """Submitting a really bad form should return an error."""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ """Bad data gets ignored. Thanks!"""
+ url = reverse('feedback')
r = self.client.post(url, {
'url': 'http://mozilla.org/'
# No happy/sad
# No description
})
- self.assertContains(r, 'This field is required')
- self.assertTemplateUsed(r, 'feedback/feedback.html')
+ self.assertRedirects(r, reverse('thanks'))
+ eq_(models.Response.objects.count(), 0)
def test_invalid_form_happy(self):
- """Submitting a bad happy form should return an error."""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ """Bad data gets ignored. Thanks!"""
+ url = reverse('feedback')
r = self.client.post(url, {
'url': 'http://mozilla.org/',
'happy': 0
# No description
})
- self.assertContains(r, 'This field is required')
- self.assertTemplateUsed(r, 'feedback/feedback.html')
+ self.assertRedirects(r, reverse('thanks'))
+ eq_(models.Response.objects.count(), 0)
def test_invalid_form_sad(self):
- """Submitting a bad sad form should return an error."""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ """Bad data gets ignored. Thanks!"""
+ url = reverse('feedback')
r = self.client.post(url, {
'url': 'http://mozilla.org/',
'happy': 1
# No description
})
- self.assertContains(r, 'This field is required')
- self.assertTemplateUsed(r, 'feedback/feedback.html')
+ self.assertRedirects(r, reverse('thanks'))
+ eq_(models.Response.objects.count(), 0)
def test_whitespace_description(self):
- """Descriptions should have more than just whitespace"""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ """Descriptions with just whitespace get thrown out"""
+ url = reverse('feedback')
r = self.client.post(url, {
'url': 'http://mozilla.org/',
'happy': 0,
'description': u' '
})
- self.assertContains(r, 'This field is required')
- self.assertTemplateUsed(r, 'feedback/feedback.html')
+ self.assertRedirects(r, reverse('thanks'))
+ eq_(models.Response.objects.count(), 0)
def test_unicode_in_description(self):
"""Description should accept unicode data"""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
r = self.client.post(url, {
'url': 'http://mozilla.org/',
'happy': 0,
@@ -291,7 +291,7 @@ def test_unicode_in_description(self):
def test_unicode_in_url(self):
"""URL should accept unicode data"""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
r = self.client.post(url, {
'url': u'http://mozilla.org/\u2713',
'happy': 0,
@@ -310,33 +310,11 @@ def test_feedback_router(self):
r = self.client.get(url, HTTP_USER_AGENT=ua)
eq_(200, r.status_code)
- self.assertTemplateUsed(r, 'feedback/feedback.html')
-
- def test_reject_non_firefox(self):
- """Using non-Firefox browser lands you on download-firefox page."""
- # TODO: This test might need to change when the router starts routing.
- url = reverse('feedback')
-
- for ua in (
- ('Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; WOW64; '
- 'Trident/5.0)'),
- ('Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.17 '
- '(KHTML, like Gecko) Chrome/24.0.1312.68 Safari/537.17'),
- ('Mozilla/5.0 (Linux; U; Android 4.0.2; en-us; Galaxy Nexus '
- 'Build/ICL53F) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 '
- 'Mobile Safari/534.30'),
- ('Mozilla/5.0 (Linux; U; Android 4.0.4; en-us; Xoom Build/IMM76) '
- 'AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 '
- 'Safari/534.30'),
- ('Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.22 (KHTML, like '
- 'Gecko) Chrome/25.0.1364.97 Safari/537.22')):
-
- r = self.client.get(url, HTTP_USER_AGENT=ua)
- self.assertRedirects(r, reverse('download-firefox'))
+ self.assertTemplateUsed(r, 'feedback/generic_feedback.html')
def test_email_collection(self):
"""If the user enters an email and checks the box, collect the email."""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
r = self.client.post(url, {
'happy': 0,
@@ -351,7 +329,7 @@ def test_email_privacy(self):
"""If an email is entered, but the box is not checked, don't collect."""
email_count = models.ResponseEmail.objects.count()
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
r = self.client.post(url, {
'happy': 0,
@@ -363,23 +341,25 @@ def test_email_privacy(self):
eq_(r.status_code, 302)
def test_email_missing(self):
- """If an email is not entered and the box is checked, don't error out."""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ """If no email, ignore it."""
+ url = reverse('feedback')
r = self.client.post(url, {
'happy': 0,
'description': u'Can you fix it?',
'email_ok': 'on',
})
+ eq_(models.Response.objects.count(), 1)
+ eq_(models.ResponseEmail.objects.count(), 0)
assert not models.ResponseEmail.objects.exists()
- # No redirect to thank you page, since there is a form error.
- eq_(r.status_code, 200)
- self.assertContains(r, 'Please enter a valid email')
+ eq_(r.status_code, 302)
def test_email_invalid(self):
- """If an email is not entered and the box is checked, don't error out."""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ """If email_ok box is checked, but bad email or no email, ignore it."""
+ url = reverse('feedback')
+ # Invalid email address gets ignored, but response is
+ # otherwise saved.
r = self.client.post(url, {
'happy': 0,
'description': u'There is something wrong here.\0',
@@ -387,10 +367,10 @@ def test_email_invalid(self):
'email': '/dev/sda1\0',
})
assert not models.ResponseEmail.objects.exists()
- # No redirect to thank you page, since there is a form error.
- eq_(r.status_code, 200)
- self.assertContains(r, 'Please enter a valid email')
+ eq_(r.status_code, 302)
+ # Invalid email address is ignored if email_ok box is not
+ # checked.
r = self.client.post(url, {
'happy': 0,
'description': u'There is something wrong here.\0',
@@ -403,7 +383,7 @@ def test_email_invalid(self):
def test_src_to_source(self):
"""We capture the src querystring arg in the source column"""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
r = self.client.post(url + '?src=newsletter', {
'happy': 0,
@@ -417,7 +397,7 @@ def test_src_to_source(self):
def test_utm_source_to_source(self):
"""We capture the utm_source querystring arg in the source column"""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
r = self.client.post(url + '?utm_source=newsletter', {
'happy': 0,
@@ -431,7 +411,7 @@ def test_utm_source_to_source(self):
def test_utm_campaign_to_source(self):
"""We capture the utm_campaign querystring arg in the source column"""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
r = self.client.post(url + '?utm_campaign=20140220_email', {
'happy': 0,
@@ -587,7 +567,7 @@ def setUp(self):
def test_no_csrf_regular_form_fails(self):
"""No csrf token in post data from anonymous user yields 403."""
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
r = self.client.post(url, {
'happy': 1,
'description': u'Firefox rocks!',
@@ -618,7 +598,7 @@ def test_throttled(self):
initial_amount = models.Response.objects.count()
eq_(initial_amount, 0)
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
# Toss 100 responses in.
for i in range(100):
@@ -648,7 +628,7 @@ def test_double_submit_throttling(self):
initial_amount = models.Response.objects.count()
eq_(initial_amount, 0)
- url = reverse('feedback', args=('firefox.desktop.stable',))
+ url = reverse('feedback')
data = {
'happy': 1,
@@ -674,7 +654,6 @@ def test_double_submit_throttling(self):
class TestRouting(TestCase):
-
uas = {
'android': 'Mozilla/5.0 (Android; Mobile; rv:18.0) Gecko/18.0 '
'Firefox/18.0',
@@ -688,22 +667,6 @@ def setUp(self):
super(TestRouting, self).setUp()
self.factory = RequestFactory()
- def sub_routing(self, ua, mobile):
- url = reverse('feedback')
- extra = {
- 'HTTP_USER_AGENT': ua,
- }
- r = self.client.get(url, **extra)
- if mobile:
- self.assertTemplateUsed(r, 'feedback/mobile/feedback.html')
- else:
- self.assertTemplateUsed(r, 'feedback/feedback.html')
-
- def test_routing(self):
- self.sub_routing(self.uas['android'], True)
- self.sub_routing(self.uas['osx'], False)
- self.sub_routing(self.uas['linux'], False)
-
def sub_prodchan(self, ua, prodchan):
# _get_prodchan checks request.BROWSER to decide what to do, so
# give it a mocked object that has that.
View
287 fjord/feedback/views.py
@@ -68,95 +68,102 @@ def _handle_feedback_post(request, locale=None, product=None,
if getattr(request, 'limited', False):
# If we're throttled, then return the thanks page, but don't
# add the response to the db.
- return HttpResponseRedirect(reverse('thanks')), None
+ return HttpResponseRedirect(reverse('thanks'))
+ # Get the form and run is_valid() so it goes through the
+ # validation and cleaning machinery. We don't really care if it's
+ # valid, though, since we will take what we got and do the best we
+ # can with it. Error validation is now in JS.
form = ResponseForm(request.POST)
- if form.is_valid():
- # Do some data validation of product, channel and version
- # coming from the url.
- product = models.Product.get_product_map().get(smart_str(product), u'')
- # FIXME - validate these better
- channel = smart_str(channel).lower()
- version = smart_str(version)
-
- # src, then source, then utm_source
- source = request.GET.get('src', u'')
- if not source:
- source = request.GET.get('utm_source', u'')
-
- campaign = request.GET.get('utm_campaign', u'')
-
- data = form.cleaned_data
-
- # Most platforms aren't different enough between versions to care.
- # Windows is.
- platform = request.BROWSER.platform
- if platform == 'Windows':
- platform += ' ' + request.BROWSER.platform_version
-
- opinion = models.Response(
- # Data coming from the user
- happy=data['happy'],
- url=data['url'],
- description=data['description'],
-
- # Inferred data from user agent
- prodchan=_get_prodchan(request, product, channel),
- user_agent=request.META.get('HTTP_USER_AGENT', ''),
- browser=request.BROWSER.browser,
- browser_version=request.BROWSER.browser_version,
- platform=platform,
-
- # Pulled from the form data or the url
- locale=data.get('locale', locale),
-
- # Data from mobile devices which is probably only
- # applicable to mobile devices
- manufacturer=data.get('manufacturer', ''),
- device=data.get('device', ''),
- )
-
- if source:
- opinion.source = source[:100]
-
- if campaign:
- opinion.campaign = campaign[:100]
+ form.is_valid()
+
+ data = form.cleaned_data
+ description = data.get('description', u'').strip()
+ if not description:
+ # If there's no description, then there's nothing to do here,
+ # so thank the user and move on.
+ return HttpResponseRedirect(reverse('thanks'))
+
+ # Do some data validation of product, channel and version
+ # coming from the url.
+ product = models.Product.get_product_map().get(smart_str(product), u'')
+ # FIXME - validate these better
+ channel = smart_str(channel).lower()
+ version = smart_str(version)
+
+ # src, then source, then utm_source
+ source = request.GET.get('src', u'')
+ if not source:
+ source = request.GET.get('utm_source', u'')
+
+ campaign = request.GET.get('utm_campaign', u'')
+
+ # Most platforms aren't different enough between versions to care.
+ # Windows is.
+ platform = request.BROWSER.platform
+ if platform == 'Windows':
+ platform += ' ' + request.BROWSER.platform_version
+
+ opinion = models.Response(
+ # Data coming from the user
+ happy=data['happy'],
+ url=data['url'],
+ description=data['description'].strip(),
+
+ # Inferred data from user agent
+ prodchan=_get_prodchan(request, product, channel),
+ user_agent=request.META.get('HTTP_USER_AGENT', ''),
+ browser=request.BROWSER.browser,
+ browser_version=request.BROWSER.browser_version,
+ platform=platform,
+
+ # Pulled from the form data or the url
+ locale=data.get('locale', locale),
+
+ # Data from mobile devices which is probably only
+ # applicable to mobile devices
+ manufacturer=data.get('manufacturer', ''),
+ device=data.get('device', ''),
+ )
+
+ if source:
+ opinion.source = source[:100]
+
+ if campaign:
+ opinion.campaign = campaign[:100]
+
+ if product:
+ # If we picked up the product from the url, we use url
+ # bits for everything.
+ product = product or u''
+ version = version or u''
+ channel = channel or u''
+
+ elif opinion.browser != UNKNOWN:
+ # If we didn't pick up a product from the url, then we
+ # infer as much as we can from the user agent.
+ product = data.get(
+ 'product', models.Response.infer_product(platform))
+ version = data.get(
+ 'version', request.BROWSER.browser_version)
+ # Assume everything we don't know about is stable channel.
+ channel = u'stable'
- if product:
- # If we picked up the product from the url, we use url
- # bits for everything.
- product = product or u''
- version = version or u''
- channel = channel or u''
-
- elif opinion.browser != UNKNOWN:
- # If we didn't pick up a product from the url, then we
- # infer as much as we can from the user agent.
- product = data.get(
- 'product', models.Response.infer_product(platform))
- version = data.get(
- 'version', request.BROWSER.browser_version)
- # Assume everything we don't know about is stable channel.
- channel = u'stable'
-
- else:
- product = channel = version = u''
-
- opinion.product = product or u''
- opinion.version = version or u''
- opinion.channel = channel or u''
+ else:
+ product = channel = version = u''
- opinion.save()
+ opinion.product = product or u''
+ opinion.version = version or u''
+ opinion.channel = channel or u''
- # If there was an email address, save that separately.
- if data['email_ok'] and data['email']:
- e = models.ResponseEmail(email=data['email'], opinion=opinion)
- e.save()
+ opinion.save()
- return HttpResponseRedirect(reverse('thanks')), form
+ # If there was an email address, save that separately.
+ if data.get('email_ok') and data.get('email'):
+ e = models.ResponseEmail(email=data['email'], opinion=opinion)
+ e.save()
- # The user did something wrong.
- return None, form
+ return HttpResponseRedirect(reverse('thanks'))
def _get_prodchan(request, product=None, channel=None):
@@ -184,69 +191,18 @@ def _get_prodchan(request, product=None, channel=None):
return '{0}.{1}.{2}'.format(product, platform, channel)
-@requires_firefox
-@csrf_protect
-def desktop_stable_feedback(request, locale=None, product=None,
- version=None, channel=None):
- # Use two instances of the same form because the template changes
- # the text based on the value of ``happy``.
- forms = {
- 'happy': ResponseForm(initial={'happy': 1}),
- 'sad': ResponseForm(initial={'happy': 0}),
- }
-
- if request.method == 'POST':
- response, form = _handle_feedback_post(
- request, locale, product, version, channel)
- if response:
- return response
-
- happy = smart_bool(request.POST.get('happy', None))
- if happy:
- forms['happy'] = form
- else:
- forms['sad'] = form
-
- return render(request, 'feedback/feedback.html', {'forms': forms})
-
-
-@requires_firefox
-@csrf_protect
-def mobile_stable_feedback(request, locale=None, product=None,
- version=None, channel=None):
- form = ResponseForm()
- happy = None
-
- if request.method == 'POST':
- response, form = _handle_feedback_post(
- request, locale, product, version, channel)
- if response:
- return response
- happy = smart_bool(request.POST.get('happy', None), None)
-
- return render(request, 'feedback/mobile/feedback.html', {
- 'form': form,
- 'happy': happy,
- })
-
-
@csrf_protect
def generic_feedback(request, locale=None, product=None, version=None,
channel=None):
"""Generic feedback form for desktop and mobile"""
form = ResponseForm()
- happy = None
if request.method == 'POST':
- response, form = _handle_feedback_post(
- request, locale, product, version, channel)
- if response:
- return response
- happy = smart_bool(request.POST.get('happy', None), None)
+ return _handle_feedback_post(request, locale, product,
+ version, channel)
return render(request, 'feedback/generic_feedback.html', {
'form': form,
- 'happy': happy,
})
@@ -302,24 +258,9 @@ def android_about_feedback(request, locale=None, product=None,
# Note: product, version and channel are always None in this view
# since this is to handle backwards-compatibility. So we don't
# bother passing them along.
- response, form = _handle_feedback_post(request, locale)
-
- if response:
- return response
-
- # This means there was an error. Since FfA doesn't care about the
- # contents anyways, return an error code.
- return HttpResponse('', status=400)
-
-# FIXME - This should go away once we unify the feedback forms.
-# Mapping of product names to views.
-product_routes = {
- 'firefox.desktop.stable': desktop_stable_feedback,
- 'firefox.android.stable': mobile_stable_feedback,
- 'firefox.fxos.stable': firefox_os_stable_feedback,
- 'generic': generic_feedback,
-}
+ # We always return Thanks! now and ignore errors.
+ return _handle_feedback_post(request, locale)
@csrf_exempt
@@ -342,27 +283,20 @@ def feedback_router(request, product=None, version=None, channel=None,
Note: We never want to cache this view.
"""
- # FIXME - Remove this when we nix the form routing. It converts
- # the product to a formname.
- view = product_routes.get(product)
-
- # Checks to see if `_type` is in the POST data and if so this is
- # coming from Firefox for Android which doesn't know anything
- # about csrf tokens. If that's the case, we send it to a view
- # specifically for FfA Otherwise we pass it to one of the normal
- # views, which enforces CSRF.
- #
- # FIXME: Remove this hairbrained monstrosity when we don't need to
- # support the method that Firefox for Android currently uses to
- # post feedback which worked with the old input.mozilla.org.
if '_type' in request.POST:
+ # Checks to see if `_type` is in the POST data and if so this
+ # is coming from Firefox for Android which doesn't know
+ # anything about csrf tokens. If that's the case, we send it
+ # to a view specifically for FfA Otherwise we pass it to one
+ # of the normal views, which enforces CSRF. Also, nix the
+ # product just in case we're crossing the streams and
+ # confusing new-style product urls with old-style backwards
+ # compatability for the Android form.
+ #
+ # FIXME: Remove this hairbrained monstrosity when we don't need to
+ # support the method that Firefox for Android currently uses to
+ # post feedback which worked with the old input.mozilla.org.
view = android_about_feedback
-
- if view:
- # If we have a view, then the "product" was really a formname
- # or we're handling the old way Firefox for Android posted
- # feedback. So we clear out the product so it doesn't cause
- # issues later.
product = None
else:
@@ -374,17 +308,16 @@ def feedback_router(request, product=None, version=None, channel=None,
'product': product
})
- # FIXME - Remove product hard-coding from here
if product == 'fxos' or request.BROWSER.browser == 'Firefox OS':
+ # Firefox OS gets shunted to a different form which has
+ # different Firefox OS specific questions.
view = firefox_os_stable_feedback
- elif product == 'android' or request.BROWSER.mobile:
- view = mobile_stable_feedback
-
else:
- view = desktop_stable_feedback
+ view = generic_feedback
- return view(request, request.locale, product, version, channel, *args, **kwargs)
+ return view(request, request.locale, product, version, channel,
+ *args, **kwargs)
class PostFeedbackAPI(generics.CreateAPIView):

0 comments on commit 600f02c

Please sign in to comment.
Something went wrong with that request. Please try again.