Merge pull request #1656 from mstriemer/handle-logged-in-fxa-errors-1655

Send logged in users to migrate for FxA errors (fixes #1655)

src/olympia/accounts/tests/test_views.py

@@ -1,9 +1,8 @@
 import base64
 
+from django.contrib.auth.models import AnonymousUser
 from django.contrib.messages import get_messages
-from django.contrib.messages.storage.fallback import FallbackStorage
 from django.core.urlresolvers import resolve, reverse
-from django.test import TestCase
 from django.test.utils import override_settings
 
 import mock
@@ -13,7 +12,7 @@
 from olympia.accounts import verify, views
 from olympia.amo.helpers import absolutify, urlparams
 from olympia.amo.tests import (
-    assert_url_equal, create_switch, InitializeSessionMixin)
+    assert_url_equal, create_switch, InitializeSessionMixin, TestCase)
 from olympia.api.tests.utils import APIAuthTestCase
 from olympia.users.models import UserProfile
 
@@ -62,9 +61,7 @@ class TestLoginUser(TestCase):
 
     def setUp(self):
         self.request = APIRequestFactory().get('/login')
-        setattr(self.request, 'session', 'session')
-        messages = FallbackStorage(self.request)
-        setattr(self.request, '_messages', messages)
+        self.enable_messages(self.request)
         self.user = UserProfile.objects.create(
             email='real@yeahoo.com', fxa_id='9001')
         self.identity = {'email': 'real@yeahoo.com', 'uid': '9001'}
@@ -126,38 +123,67 @@ def test_two_users_exist(self):
 class TestRenderErrorHTML(TestCase):
 
     def make_request(self):
-        return APIRequestFactory().get(reverse('accounts.authenticate'))
+        request = APIRequestFactory().get(reverse('accounts.authenticate'))
+        request.user = AnonymousUser()
+        return self.enable_messages(request)
 
-    def login_error_url(self, **params):
+    def login_url(self, **params):
         return urlparams(reverse('users.login'), **params)
 
-    def render_error(self, error, next_path=None):
+    def migrate_url(self, **params):
+        return absolutify(urlparams(reverse('users.migrate'), **params))
+
+    def render_error(self, request, error, next_path=None):
         return views.render_error(
-            self.make_request(), error, format='html', next_path=next_path)
+            request, error, format='html', next_path=next_path)
 
     def test_error_no_code_with_safe_path(self):
+        request = self.make_request()
+        assert len(get_messages(request)) == 0
         response = self.render_error(
-            views.ERROR_NO_CODE, next_path='/over/here')
+            request, views.ERROR_NO_CODE, next_path='/over/here')
         assert response.status_code == 302
-        assert_url_equal(
-            response['location'],
-            self.login_error_url(to='/over/here', error=views.ERROR_NO_CODE))
+        messages = get_messages(request)
+        assert len(messages) == 1
+        assert 'could not be parsed' in next(iter(messages)).message
+        assert_url_equal(response['location'], self.login_url(to='/over/here'))
 
     def test_error_no_profile_with_no_path(self):
-        response = self.render_error(views.ERROR_NO_PROFILE)
+        request = self.make_request()
+        assert len(get_messages(request)) == 0
+        response = self.render_error(request, views.ERROR_NO_PROFILE)
         assert response.status_code == 302
-        assert_url_equal(
-            response['location'],
-            self.login_error_url(error=views.ERROR_NO_PROFILE))
+        messages = get_messages(request)
+        assert len(messages) == 1
+        assert ('Firefox Account could not be found'
+                in next(iter(messages)).message)
+        assert_url_equal(response['location'], self.login_url())
 
     def test_error_state_mismatch_with_unsafe_path(self):
+        request = self.make_request()
+        assert len(get_messages(request)) == 0
         response = self.render_error(
-            views.ERROR_STATE_MISMATCH,
+            request, views.ERROR_STATE_MISMATCH,
             next_path='https://www.google.com/')
         assert response.status_code == 302
+        messages = get_messages(request)
+        assert len(messages) == 1
+        assert 'could not be logged in' in next(iter(messages)).message
+        assert_url_equal(response['location'], self.login_url())
+
+    def test_error_no_code_with_safe_path_logged_in(self):
+        request = self.make_request()
+        request.user = UserProfile()
+        assert len(get_messages(request)) == 0
+        response = self.render_error(
+            request, views.ERROR_NO_CODE, next_path='/over/here')
+        assert response.status_code == 302
+        messages = get_messages(request)
+        assert len(messages) == 1
+        assert 'could not be parsed' in next(iter(messages)).message
         assert_url_equal(
             response['location'],
-            self.login_error_url(error=views.ERROR_STATE_MISMATCH))
+            self.migrate_url(to='/over/here'))
 
 
 class TestRenderErrorJSON(TestCase):
@@ -591,25 +617,24 @@ def setUp(self):
         self.login_user = self.patch('olympia.accounts.views.login_user')
         self.register_user = self.patch('olympia.accounts.views.register_user')
 
-    def login_error_url(self, **params):
+    def login_url(self, **params):
         return absolutify(urlparams(reverse('users.login'), **params))
 
     def test_no_code_provided(self):
         response = self.client.get(self.url)
         assert response.status_code == 302
-        assert_url_equal(
-            response['location'],
-            self.login_error_url(error=views.ERROR_NO_CODE))
+        # Messages seem to appear in the context for some reason.
+        assert 'could not be parsed' in response.context['title']
+        assert_url_equal(response['location'], self.login_url())
         assert not self.login_user.called
         assert not self.register_user.called
 
     def test_wrong_state(self):
         response = self.client.get(
             self.url, {'code': 'foo', 'state': '9f865be0'})
         assert response.status_code == 302
-        assert_url_equal(
-            response['location'],
-            self.login_error_url(error=views.ERROR_STATE_MISMATCH))
+        assert 'could not be logged in' in response.context['title']
+        assert_url_equal(response['location'], self.login_url())
         assert not self.login_user.called
         assert not self.register_user.called
 
@@ -618,9 +643,9 @@ def test_no_fxa_profile(self):
         response = self.client.get(
             self.url, {'code': 'codes!!', 'state': self.fxa_state})
         assert response.status_code == 302
-        assert_url_equal(
-            response['location'],
-            self.login_error_url(error=views.ERROR_NO_PROFILE))
+        assert ('Your Firefox Account could not be found'
+                in response.context['title'])
+        assert_url_equal(response['location'], self.login_url())
         self.fxa_identify.assert_called_with('codes!!', config=FXA_CONFIG)
         assert not self.login_user.called
         assert not self.register_user.called

src/olympia/accounts/views.py

@@ -9,6 +9,7 @@
 from django.db.models import Q
 from django.http import HttpResponseRedirect
 from django.utils.http import is_safe_url
+from django.utils.html import format_html
 
 from rest_framework import generics
 from rest_framework.response import Response
@@ -108,15 +109,31 @@ def login_user(request, user, identity):
     login(request, user)
 
 
+def fxa_error_message(message):
+    login_help_url = (
+        'https://support.mozilla.org/kb/access-your-add-ons-firefox-accounts')
+    return format_html(
+        u'{error} <a href="{url}" target="_blank">{help_text}</a>',
+        url=login_help_url, help_text=_(u'Need help?'),
+        error=message)
+
+
 def render_error(request, error, next_path=None, format=None):
     if format == 'json':
         status = ERROR_STATUSES.get(error, 422)
         return Response({'error': error}, status=status)
     else:
         if not is_safe_url(next_path):
             next_path = None
+        messages.error(
+            request, fxa_error_message(LOGIN_ERROR_MESSAGES[error]),
+            extra_tags='fxa')
+        if request.user.is_authenticated():
+            redirect_view = 'users.migrate'
+        else:
+            redirect_view = 'users.login'
         return HttpResponseRedirect(
-            urlparams(reverse('users.login'), error=error, to=next_path))
+            urlparams(reverse(redirect_view), to=next_path))
 
 
 def parse_next_path(state_parts):

src/olympia/amo/tests/__init__.py

@@ -13,6 +13,7 @@
 
 from django import forms, test
 from django.conf import settings
+from django.contrib.messages.storage.fallback import FallbackStorage
 from django.core.management import call_command
 from django.db.models.signals import post_save
 from django.forms.fields import Field
@@ -559,6 +560,12 @@ def assertUrlEqual(self, url, other, compare_host=False):
         """Compare url paths and query strings."""
         assert_url_equal(url, other, compare_host)
 
+    def enable_messages(self, request):
+        setattr(request, 'session', 'session')
+        messages = FallbackStorage(request)
+        setattr(request, '_messages', messages)
+        return request
+
 
 class AMOPaths(object):
     """Mixin for getting common AMO Paths."""

src/olympia/users/templates/users/login_form.html

@@ -1,15 +1,12 @@
 <form method="post" action="{{ action }}"
       class="{% if is_ajax %}ajax-submit {% endif %}listing-footer{% if login_source_form %} login-source-form{% else %} login-form{% endif %}">
   {{ csrf() }}
-  {% if form.non_field_errors() or login_error %}
+  {% if form.non_field_errors() %}
       <div class="notification-box error">
         <ul>
         {% for error in form.non_field_errors() %}
         <li>{{ error }}</li>
         {% endfor %}
-        {% if login_error %}
-        <li>{{ login_error }} <a href="{{ fxa_login_help_url }}" target="_blank">{{ _('Need help?') }}</a></li>
-        {% endif %}
         </ul>
       </div>
   {% endif %}

src/olympia/users/tests/test_views.py

@@ -18,7 +18,6 @@
 from olympia.amo.tests import TestCase
 from olympia.abuse.models import AbuseReport
 from olympia.access.models import Group, GroupUser
-from olympia.accounts.views import ERROR_NO_CODE
 from olympia.addons.models import Addon, AddonUser, Category
 from olympia.amo.helpers import urlparams
 from olympia.amo.pyquery_wrapper import PyQuery as pq
@@ -621,11 +620,6 @@ def test_login_ajax_wrong(self):
         text = 'Please enter a correct username and password.'
         assert res.context['form'].errors['__all__'][0].startswith(text)
 
-    def test_login_fxa_error(self):
-        response = self.client.get(urlparams(self.url, error=ERROR_NO_CODE))
-        assert response.status_code == 200
-        assert 'could not be parsed' in response.content
-
     def test_login_no_recaptcha(self):
         res = self.client.post(self.url, data=self.data)
         eq_(res.status_code, 302)

src/olympia/users/views.py

@@ -28,7 +28,6 @@
 from olympia.abuse.models import send_abuse_report
 from olympia.access import acl
 from olympia.access.middleware import ACLMiddleware
-from olympia.accounts.views import LOGIN_ERROR_MESSAGES
 from olympia.addons.decorators import addon_view_factory
 from olympia.addons.models import Addon, AddonUser, Category
 from olympia.amo import messages
@@ -376,11 +375,6 @@ def _login(request, template=None, data=None, dont_redirect=False):
 
     data['login_source_form'] = (waffle.switch_is_active('fxa-auth') and
                                  not request.POST)
-    login_error = request.GET.get('error')
-    if login_error in LOGIN_ERROR_MESSAGES:
-        data['login_error'] = LOGIN_ERROR_MESSAGES[login_error]
-        data['fxa_login_help_url'] = ('https://support.mozilla.org/kb/'
-                                      'access-your-add-ons-firefox-accounts')
 
     limited = getattr(request, 'limited', 'recaptcha_shown' in request.POST)
     user = None

static/css/impala/fxa-migration.less

@@ -15,18 +15,27 @@
 }
 
 .notification-box.fxa {
+    z-index: 75;
+    position: relative;
+
+    h2 {
+        font-family: @head-sans;
+    }
+    p {
+        font-size: 14px;
+    }
+}
+
+.notification-box.success.fxa {
     background: lighten(@button-green-light, 7.5%);
     border-color: @button-green-light;
     color: white;
 
     h2 {
         color: white;
-        font-family: @head-sans;
         text-shadow: 0px 0px 2px rgba(0, 0, 0, 0.5);
     }
-
     p {
-        font-size: 14px;
         text-shadow: 0px 0px 2px rgba(0, 0, 0, 0.5);
     }
 }