From 58fadab9399cd32a33f4e3127c1aee181a999803 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Tue, 19 Nov 2019 16:49:46 -0500 Subject: [PATCH] clean-up login part 1 `shim_student_view` is used for login, and is being simplified so it can ulimately be completely deleted. In this commit, the shim preprocessing was removed by deleting unused code, and moving code that is still being used to login_user. Note: `shim_student_view` was originally added in https://github.com/edx/edx-platform/pull/5768/files ARCH-1253 --- .../core/djangoapps/user_authn/views/login.py | 70 ++++++++----------- .../user_authn/views/tests/test_login.py | 55 ++++++++------- 2 files changed, 58 insertions(+), 67 deletions(-) diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index 4ebaed3d08..ac818a23bc 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -340,6 +340,8 @@ def login_user(request): """ AJAX request to log in the user. """ + _parse_analytics_param_for_course_id(request) + third_party_auth_requested = third_party_auth.is_enabled() and pipeline.running(request) first_party_auth_requested = bool(request.POST.get('email')) or bool(request.POST.get('password')) is_user_third_party_authenticated = False @@ -478,6 +480,32 @@ class LoginSessionView(APIView): return super(LoginSessionView, self).dispatch(request, *args, **kwargs) +def _parse_analytics_param_for_course_id(request): + """ If analytics request param is found, parse and add course id as a new request param. """ + # Make a copy of the current POST request to modify. + modified_request = request.POST.copy() + if isinstance(request, HttpRequest): + # Works for an HttpRequest but not a rest_framework.request.Request. + request.POST = modified_request + else: + # The request must be a rest_framework.request.Request. + request._data = modified_request # pylint: disable=protected-access + # Include the course ID if it's specified in the analytics info + # so it can be included in analytics events. + if "analytics" in modified_request: + try: + analytics = json.loads(modified_request["analytics"]) + if "enroll_course_id" in analytics: + modified_request["course_id"] = analytics.get("enroll_course_id") + except (ValueError, TypeError): + set_custom_metric('shim_analytics_course_id', 'parse-error') + log.error( + u"Could not parse analytics object sent to user API: {analytics}".format( + analytics=analytics + ) + ) + + def shim_student_view(view_func, check_logged_in=False): """Create a "shim" view for a view function from the student Django app. @@ -501,7 +529,8 @@ def shim_student_view(view_func, check_logged_in=False): Keyword Args: check_logged_in (boolean): If true, check whether the user successfully - authenticated and if not set the status to 403. + authenticated and if not set the status to 403. This argument is + used to test the shim by skipping this check. Returns: function @@ -509,45 +538,6 @@ def shim_student_view(view_func, check_logged_in=False): """ @wraps(view_func) def _inner(request): # pylint: disable=missing-docstring - # Make a copy of the current POST request to modify. - modified_request = request.POST.copy() - if isinstance(request, HttpRequest): - # Works for an HttpRequest but not a rest_framework.request.Request. - set_custom_metric('shim_request_type', 'traditional') - request.POST = modified_request - else: - set_custom_metric('shim_request_type', 'drf') - # The request must be a rest_framework.request.Request. - request._data = modified_request # pylint: disable=protected-access - - # The login and registration handlers in student view try to change - # the user's enrollment status if these parameters are present. - # Since we want the JavaScript client to communicate directly with - # the enrollment API, we want to prevent the student views from - # updating enrollments. - if "enrollment_action" in modified_request: - set_custom_metric('shim_del_enrollment_action', modified_request["enrollment_action"]) - del modified_request["enrollment_action"] - if "course_id" in modified_request: - set_custom_metric('shim_del_course_id', modified_request["course_id"]) - del modified_request["course_id"] - - # Include the course ID if it's specified in the analytics info - # so it can be included in analytics events. - if "analytics" in modified_request: - try: - analytics = json.loads(modified_request["analytics"]) - if "enroll_course_id" in analytics: - set_custom_metric('shim_analytics_course_id', analytics.get("enroll_course_id")) - modified_request["course_id"] = analytics.get("enroll_course_id") - except (ValueError, TypeError): - set_custom_metric('shim_analytics_course_id', 'parse-error') - log.error( - u"Could not parse analytics object sent to user API: {analytics}".format( - analytics=analytics - ) - ) - # Call the original view to generate a response. # We can safely modify the status code or content # of the response, but to be safe we won't mess diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_login.py b/openedx/core/djangoapps/user_authn/views/tests/test_login.py index 9245c0a3c9..3a6c1b3bf0 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_login.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_login.py @@ -714,15 +714,26 @@ class LoginSessionViewTest(ApiTestCase): }, ]) - def test_login(self): + @ddt.data(True, False) + @patch('openedx.core.djangoapps.user_authn.views.login.segment') + def test_login(self, include_analytics, mock_segment): # Create a test user UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) - # Login - response = self.client.post(self.url, { + data = { "email": self.EMAIL, "password": self.PASSWORD, - }) + } + if include_analytics: + track_label = "edX/DemoX/Fall" + data.update({ + "analytics": json.dumps({"enroll_course_id": track_label}) + }) + else: + track_label = None + + # Login + response = self.client.post(self.url, data) self.assertHttpOK(response) # Verify that we logged in successfully by accessing @@ -730,6 +741,19 @@ class LoginSessionViewTest(ApiTestCase): response = self.client.get(reverse("dashboard")) self.assertHttpOK(response) + # Verify events are called + expected_user_id = 1 + mock_segment.identify.assert_called_once_with( + expected_user_id, + {'username': self.USERNAME, 'email': self.EMAIL}, + {'MailChimp': False} + ) + mock_segment.track.assert_called_once_with( + expected_user_id, + 'edx.bi.user.account.authenticated', + {'category': 'conversion', 'provider': None, 'label': track_label} + ) + def test_session_cookie_expiry(self): # Create a test user UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) @@ -793,29 +817,6 @@ class StudentViewShimTest(TestCase): super(StudentViewShimTest, self).setUp() self.captured_request = None - def test_strip_enrollment_action(self): - view = self._shimmed_view(HttpResponse()) - request = HttpRequest() - request.POST["enrollment_action"] = "enroll" - request.POST["course_id"] = "edx/101/demo" - view(request) - - # Expect that the enrollment action and course ID - # were stripped out before reaching the wrapped view. - self.assertNotIn("enrollment_action", self.captured_request.POST) - self.assertNotIn("course_id", self.captured_request.POST) - - def test_include_analytics_info(self): - view = self._shimmed_view(HttpResponse()) - request = HttpRequest() - request.POST["analytics"] = json.dumps({ - "enroll_course_id": "edX/DemoX/Fall" - }) - view(request) - - # Expect that the analytics course ID was passed to the view - self.assertEqual(self.captured_request.POST.get("course_id"), "edX/DemoX/Fall") - def test_third_party_auth_login_failure(self): view = self._shimmed_view( HttpResponse(status=403),