From bf85380dc3f7deb3571529a0de49da4f5ecf84c2 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 20 Nov 2019 16:36:04 -0500 Subject: [PATCH] add temporary login custom metrics. (#22365) - add temporary custom metrics for shim_student_view. - remove some pointless tests. This is in service of ARCH-1253 clean-up of login. ARCH-1253 --- .../core/djangoapps/user_authn/views/login.py | 20 ++++++++++++ .../user_authn/views/tests/test_login.py | 32 ------------------- 2 files changed, 20 insertions(+), 32 deletions(-) diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index cc528ee464..7f0f1779c9 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -22,6 +22,7 @@ from django.utils.translation import ugettext as _ from django.views.decorators.csrf import csrf_exempt, csrf_protect, ensure_csrf_cookie from django.views.decorators.debug import sensitive_post_parameters from django.views.decorators.http import require_http_methods +from edx_django_utils.monitoring import set_custom_metric from ratelimitbackend.exceptions import RateLimitException from rest_framework.views import APIView @@ -341,6 +342,9 @@ def login_user(request): first_party_auth_requested = bool(request.POST.get('email')) or bool(request.POST.get('password')) is_user_third_party_authenticated = False + set_custom_metric('login_user_enrollment_action', request.POST.get('enrollment_action')) + set_custom_metric('login_user_course_id', request.POST.get('course_id')) + try: if third_party_auth_requested and not first_party_auth_requested: # The user has already authenticated via third-party auth and has not @@ -462,6 +466,9 @@ class LoginSessionView(APIView): def shim_student_view(view_func, check_logged_in=False): """Create a "shim" view for a view function from the student Django app. + UPDATE: This shim is only used to wrap `login_user`, which now lives in + the user_authn Django app (not the student app). + Specifically, we need to: * Strip out enrollment params, since the client for the new registration/login page will communicate with the enrollment API to update enrollments. @@ -491,8 +498,10 @@ def shim_student_view(view_func, check_logged_in=False): 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 @@ -502,8 +511,10 @@ def shim_student_view(view_func, check_logged_in=False): # 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 @@ -512,8 +523,10 @@ def shim_student_view(view_func, check_logged_in=False): 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 @@ -550,9 +563,14 @@ def shim_student_view(view_func, check_logged_in=False): response_dict = json.loads(response.content.decode('utf-8')) msg = response_dict.get("value", u"") success = response_dict.get("success") + set_custom_metric('shim_original_response_is_json', True) except (ValueError, TypeError): msg = response.content success = True + set_custom_metric('shim_original_response_is_json', False) + set_custom_metric('shim_original_response_msg', msg) + set_custom_metric('shim_original_response_success', success) + set_custom_metric('shim_original_response_status', response.status_code) # If the user is not authenticated when we expect them to be # send the appropriate status code. @@ -594,6 +612,8 @@ def shim_student_view(view_func, check_logged_in=False): else: response.content = msg + set_custom_metric('shim_final_response_msg', response.content) + set_custom_metric('shim_final_response_status', response.status_code) # Return the response, preserving the original headers. # This is really important, since the student views set cookies # that are used elsewhere in the system (such as the marketing site). 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 b61034eacd..13e9ab8554 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_login.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_login.py @@ -426,38 +426,6 @@ class LoginTest(SiteMixin, CacheIsolationTestCase): response = client1.get(url) self.assertEqual(response.status_code, 200) - def test_change_enrollment_400(self): - """ - Tests that a 400 in change_enrollment doesn't lead to a 404 - and in fact just logs in the user without incident - """ - # add this post param to trigger a call to change_enrollment - extra_post_params = {"enrollment_action": "enroll"} - with patch('student.views.change_enrollment') as mock_change_enrollment: - mock_change_enrollment.return_value = HttpResponseBadRequest("I am a 400") - response, _ = self._login_response( - self.user_email, self.password, extra_post_params=extra_post_params, - ) - response_content = json.loads(response.content.decode('utf-8')) - self.assertIsNone(response_content["redirect_url"]) - self._assert_response(response, success=True) - - def test_change_enrollment_200_no_redirect(self): - """ - Tests "redirect_url" is None if change_enrollment returns a HttpResponse - with no content - """ - # add this post param to trigger a call to change_enrollment - extra_post_params = {"enrollment_action": "enroll"} - with patch('student.views.change_enrollment') as mock_change_enrollment: - mock_change_enrollment.return_value = HttpResponse() - response, _ = self._login_response( - self.user_email, self.password, extra_post_params=extra_post_params, - ) - response_content = json.loads(response.content.decode('utf-8')) - self.assertIsNone(response_content["redirect_url"]) - self._assert_response(response, success=True) - @override_settings(PASSWORD_POLICY_COMPLIANCE_ROLLOUT_CONFIG={'ENFORCE_COMPLIANCE_ON_LOGIN': True}) def test_check_password_policy_compliance(self): """