diff --git a/common/djangoapps/util/tests/test_submit_feedback.py b/common/djangoapps/util/tests/test_submit_feedback.py index 077765e1d7..35e80c83d5 100644 --- a/common/djangoapps/util/tests/test_submit_feedback.py +++ b/common/djangoapps/util/tests/test_submit_feedback.py @@ -11,8 +11,19 @@ from util import views from zendesk import ZendeskError import json import mock +from ddt import ddt, data, unpack from student.tests.test_configuration_overrides import fake_get_value +from student.tests.factories import CourseEnrollmentFactory + +TEST_SUPPORT_EMAIL = "support@example.com" +TEST_ZENDESK_CUSTOM_FIELD_CONFIG = {"course_id": 1234, "enrollment_mode": 5678} +TEST_REQUEST_HEADERS = { + "HTTP_REFERER": "test_referer", + "HTTP_USER_AGENT": "test_user_agent", + "REMOTE_ADDR": "1.2.3.4", + "SERVER_NAME": "test_server", +} def fake_support_backend_values(name, default=None): # pylint: disable=unused-argument @@ -21,13 +32,20 @@ def fake_support_backend_values(name, default=None): # pylint: disable=unused-a """ config_dict = { "CONTACT_FORM_SUBMISSION_BACKEND": "email", - "email_from_address": "support_from@example.com", + "email_from_address": TEST_SUPPORT_EMAIL, } return config_dict[name] +@ddt @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_FEEDBACK_SUBMISSION": True}) -@override_settings(ZENDESK_URL="dummy", ZENDESK_USER="dummy", ZENDESK_API_KEY="dummy") +@override_settings( + DEFAULT_FROM_EMAIL=TEST_SUPPORT_EMAIL, + ZENDESK_URL="dummy", + ZENDESK_USER="dummy", + ZENDESK_API_KEY="dummy", + ZENDESK_CUSTOM_FIELDS={} +) @mock.patch("util.views.dog_stats_api") @mock.patch("util.views._ZendeskApi", autospec=True) class SubmitFeedbackTest(TestCase): @@ -44,14 +62,12 @@ class SubmitFeedbackTest(TestCase): username="test", profile__name="Test User" ) - # This contains issue_type and course_id to ensure that tags are submitted correctly self._anon_fields = { "email": "test@edx.org", "name": "Test User", "subject": "a subject", "details": "some details", - "issue_type": "test_issue", - "course_id": "test_course" + "issue_type": "test_issue" } # This does not contain issue_type nor course_id to ensure that they are optional self._auth_fields = {"subject": "a subject", "details": "some details"} @@ -66,10 +82,10 @@ class SubmitFeedbackTest(TestCase): req = self._request_factory.post( "/submit_feedback", data=fields, - HTTP_REFERER="test_referer", - HTTP_USER_AGENT="test_user_agent", - REMOTE_ADDR="1.2.3.4", - SERVER_NAME="test_server", + HTTP_REFERER=TEST_REQUEST_HEADERS["HTTP_REFERER"], + HTTP_USER_AGENT=TEST_REQUEST_HEADERS["HTTP_USER_AGENT"], + REMOTE_ADDR=TEST_REQUEST_HEADERS["REMOTE_ADDR"], + SERVER_NAME=TEST_REQUEST_HEADERS["SERVER_NAME"], ) req.user = user return views.submit_feedback(req) @@ -130,13 +146,58 @@ class SubmitFeedbackTest(TestCase): resp = self._build_and_run_request(user, fields) self.assertEqual(resp.status_code, 200) - def _assert_datadog_called(self, datadog_mock, with_tags): - expected_datadog_calls = [ - mock.call.increment( - views.DATADOG_FEEDBACK_METRIC, - tags=(["course_id:test_course", "issue_type:test_issue"] if with_tags else []) - ) + def _build_zendesk_ticket(self, recipient, name, email, subject, details, tags, custom_fields=None): + """ + Build a Zendesk ticket that can be used in assertions to verify that the correct + data was submitted to create a Zendesk ticket. + """ + ticket = { + "ticket": { + "recipient": recipient, + "requester": {"name": name, "email": email}, + "subject": subject, + "comment": {"body": details}, + "tags": tags + } + } + + if custom_fields is not None: + ticket["ticket"]["custom_fields"] = custom_fields + + return ticket + + def _build_zendesk_ticket_update(self, request_headers, username=None): + """ + Build a Zendesk ticket update that can be used in assertions to verify that the correct + data was submitted to update a Zendesk ticket. + """ + body = [] + if username: + body.append("username: {}".format(username)) + + # FIXME the tests rely on the body string being built in this specific order, which doesn't seem + # reliable given that the view builds the string by iterating over a dictionary. + header_text_mapping = [ + ("Client IP", "REMOTE_ADDR"), + ("Host", "SERVER_NAME"), + ("Page", "HTTP_REFERER"), + ("Browser", "HTTP_USER_AGENT") ] + + for text, header in header_text_mapping: + body.append("{}: {}".format(text, request_headers[header])) + + body = "Additional information:\n\n" + "\n".join(body) + return {"ticket": {"comment": {"public": False, "body": body}}} + + def _assert_zendesk_called(self, zendesk_mock, ticket_id, ticket, ticket_update): + """Assert that Zendesk was called with the correct ticket and ticket_update.""" + expected_zendesk_calls = [mock.call.create_ticket(ticket), mock.call.update_ticket(ticket_id, ticket_update)] + self.assertEqual(zendesk_mock.mock_calls, expected_zendesk_calls) + + def _assert_datadog_called(self, datadog_mock, tags): + """Assert that datadog was called with the correct tags.""" + expected_datadog_calls = [mock.call.increment(views.DATADOG_FEEDBACK_METRIC, tags=tags)] self.assertEqual(datadog_mock.mock_calls, expected_datadog_calls) def test_bad_request_anon_user_no_name(self, zendesk_mock_class, datadog_mock): @@ -174,39 +235,26 @@ class SubmitFeedbackTest(TestCase): the given information should have been submitted via the Zendesk API. """ zendesk_mock_instance = zendesk_mock_class.return_value - zendesk_mock_instance.create_ticket.return_value = 42 - self._test_success(self._anon_user, self._anon_fields) - expected_zendesk_calls = [ - mock.call.create_ticket( - { - "ticket": { - "recipient": "registration@example.com", - "requester": {"name": "Test User", "email": "test@edx.org"}, - "subject": "a subject", - "comment": {"body": "some details"}, - "tags": ["test_course", "test_issue", "LMS"] - } - } - ), - mock.call.update_ticket( - 42, - { - "ticket": { - "comment": { - "public": False, - "body": - "Additional information:\n\n" - "Client IP: 1.2.3.4\n" - "Host: test_server\n" - "Page: test_referer\n" - "Browser: test_user_agent" - } - } - } - ) - ] - self.assertEqual(zendesk_mock_instance.mock_calls, expected_zendesk_calls) - self._assert_datadog_called(datadog_mock, with_tags=True) + user = self._anon_user + fields = self._anon_fields + + ticket_id = 42 + zendesk_mock_instance.create_ticket.return_value = ticket_id + + ticket = self._build_zendesk_ticket( + recipient=TEST_SUPPORT_EMAIL, + name=fields["name"], + email=fields["email"], + subject=fields["subject"], + details=fields["details"], + tags=[fields["issue_type"], "LMS"] + ) + + ticket_update = self._build_zendesk_ticket_update(TEST_REQUEST_HEADERS) + + self._test_success(user, fields) + self._assert_zendesk_called(zendesk_mock_instance, ticket_id, ticket, ticket_update) + self._assert_datadog_called(datadog_mock, ["issue_type:{}".format(fields["issue_type"])]) @mock.patch("openedx.core.djangoapps.site_configuration.helpers.get_value", fake_get_value) def test_valid_request_anon_user_configuration_override(self, zendesk_mock_class, datadog_mock): @@ -218,39 +266,75 @@ class SubmitFeedbackTest(TestCase): tag that will come from site configuration override. """ zendesk_mock_instance = zendesk_mock_class.return_value - zendesk_mock_instance.create_ticket.return_value = 42 - self._test_success(self._anon_user, self._anon_fields) - expected_zendesk_calls = [ - mock.call.create_ticket( - { - "ticket": { - "recipient": "no-reply@fakeuniversity.com", - "requester": {"name": "Test User", "email": "test@edx.org"}, - "subject": "a subject", - "comment": {"body": "some details"}, - "tags": ["test_course", "test_issue", "LMS", "whitelabel_fakeorg"] - } - } - ), - mock.call.update_ticket( - 42, - { - "ticket": { - "comment": { - "public": False, - "body": - "Additional information:\n\n" - "Client IP: 1.2.3.4\n" - "Host: test_server\n" - "Page: test_referer\n" - "Browser: test_user_agent" - } - } - } - ) - ] - self.assertEqual(zendesk_mock_instance.mock_calls, expected_zendesk_calls) - self._assert_datadog_called(datadog_mock, with_tags=True) + user = self._anon_user + fields = self._anon_fields + + ticket_id = 42 + zendesk_mock_instance.create_ticket.return_value = ticket_id + + ticket = self._build_zendesk_ticket( + recipient=fake_get_value("email_from_address"), + name=fields["name"], + email=fields["email"], + subject=fields["subject"], + details=fields["details"], + tags=[fields["issue_type"], "LMS", "whitelabel_{}".format(fake_get_value("course_org_filter"))] + ) + + ticket_update = self._build_zendesk_ticket_update(TEST_REQUEST_HEADERS) + + self._test_success(user, fields) + self._assert_zendesk_called(zendesk_mock_instance, ticket_id, ticket, ticket_update) + self._assert_datadog_called(datadog_mock, ["issue_type:{}".format(fields["issue_type"])]) + + @data("course-v1:testOrg+testCourseNumber+testCourseRun", "", None) + @override_settings(ZENDESK_CUSTOM_FIELDS=TEST_ZENDESK_CUSTOM_FIELD_CONFIG) + def test_valid_request_anon_user_with_custom_fields(self, course_id, zendesk_mock_class, datadog_mock): + """ + Test a valid request from an anonymous user when configured to use Zendesk Custom Fields. + + The response should have a 200 (success) status code, and a ticket with + the given information should have been submitted via the Zendesk API. When course_id is + present, it should be sent to Zendesk via a custom field. When course_id is blank or missing, + the request should still be processed successfully. + """ + zendesk_mock_instance = zendesk_mock_class.return_value + user = self._anon_user + + fields = self._anon_fields.copy() + if course_id is not None: + fields["course_id"] = course_id + + ticket_id = 42 + zendesk_mock_instance.create_ticket.return_value = ticket_id + + zendesk_tags = [fields["issue_type"], "LMS"] + datadog_tags = ["issue_type:{}".format(fields["issue_type"])] + zendesk_custom_fields = None + if course_id: + # FIXME the tests rely on the tags being in this specific order, which doesn't seem + # reliable given that the view builds the list by iterating over a dictionary. + zendesk_tags.insert(0, course_id) + datadog_tags.insert(0, "course_id:{}".format(course_id)) + zendesk_custom_fields = [ + {"id": TEST_ZENDESK_CUSTOM_FIELD_CONFIG["course_id"], "value": course_id} + ] + + ticket = self._build_zendesk_ticket( + recipient=TEST_SUPPORT_EMAIL, + name=fields["name"], + email=fields["email"], + subject=fields["subject"], + details=fields["details"], + tags=zendesk_tags, + custom_fields=zendesk_custom_fields + ) + + ticket_update = self._build_zendesk_ticket_update(TEST_REQUEST_HEADERS) + + self._test_success(user, fields) + self._assert_zendesk_called(zendesk_mock_instance, ticket_id, ticket, ticket_update) + self._assert_datadog_called(datadog_mock, datadog_tags) def test_bad_request_auth_user_no_subject(self, zendesk_mock_class, datadog_mock): """Test a request from an authenticated user not specifying `subject`.""" @@ -270,40 +354,92 @@ class SubmitFeedbackTest(TestCase): the given information should have been submitted via the Zendesk API. """ zendesk_mock_instance = zendesk_mock_class.return_value - zendesk_mock_instance.create_ticket.return_value = 42 - self._test_success(self._auth_user, self._auth_fields) - expected_zendesk_calls = [ - mock.call.create_ticket( - { - "ticket": { - "recipient": "registration@example.com", - "requester": {"name": "Test User", "email": "test@edx.org"}, - "subject": "a subject", - "comment": {"body": "some details"}, - "tags": ["LMS"] - } - } - ), - mock.call.update_ticket( - 42, - { - "ticket": { - "comment": { - "public": False, - "body": - "Additional information:\n\n" - "username: test\n" - "Client IP: 1.2.3.4\n" - "Host: test_server\n" - "Page: test_referer\n" - "Browser: test_user_agent" - } - } - } - ) - ] - self.assertEqual(zendesk_mock_instance.mock_calls, expected_zendesk_calls) - self._assert_datadog_called(datadog_mock, with_tags=False) + user = self._auth_user + fields = self._auth_fields + + ticket_id = 42 + zendesk_mock_instance.create_ticket.return_value = ticket_id + + ticket = self._build_zendesk_ticket( + recipient=TEST_SUPPORT_EMAIL, + name=user.profile.name, + email=user.email, + subject=fields["subject"], + details=fields["details"], + tags=["LMS"] + ) + + ticket_update = self._build_zendesk_ticket_update(TEST_REQUEST_HEADERS, user.username) + + self._test_success(user, fields) + self._assert_zendesk_called(zendesk_mock_instance, ticket_id, ticket, ticket_update) + self._assert_datadog_called(datadog_mock, []) + + @data( + ("course-v1:testOrg+testCourseNumber+testCourseRun", True), + ("course-v1:testOrg+testCourseNumber+testCourseRun", False), + ("", None), + (None, None) + ) + @unpack + @override_settings(ZENDESK_CUSTOM_FIELDS=TEST_ZENDESK_CUSTOM_FIELD_CONFIG) + def test_valid_request_auth_user_with_custom_fields(self, course_id, enrolled, zendesk_mock_class, datadog_mock): + """ + Test a valid request from an authenticated user when configured to use Zendesk Custom Fields. + + The response should have a 200 (success) status code, and a ticket with + the given information should have been submitted via the Zendesk API. When course_id is + present, it should be sent to Zendesk via a custom field, along with the enrollment mode + if the user has an active enrollment for that course. When course_id is blank or missing, + the request should still be processed successfully. + """ + zendesk_mock_instance = zendesk_mock_class.return_value + user = self._auth_user + + fields = self._auth_fields.copy() + if course_id is not None: + fields["course_id"] = course_id + + ticket_id = 42 + zendesk_mock_instance.create_ticket.return_value = ticket_id + + zendesk_tags = ["LMS"] + datadog_tags = [] + zendesk_custom_fields = None + if course_id: + # FIXME the tests rely on the tags being in this specific order, which doesn't seem + # reliable given that the view builds the list by iterating over a dictionary. + zendesk_tags.insert(0, course_id) + datadog_tags.insert(0, "course_id:{}".format(course_id)) + zendesk_custom_fields = [ + {"id": TEST_ZENDESK_CUSTOM_FIELD_CONFIG["course_id"], "value": course_id} + ] + if enrolled is not None: + enrollment = CourseEnrollmentFactory.create( + user=user, + course_id=course_id, + is_active=enrolled + ) + if enrollment.is_active: + zendesk_custom_fields.append( + {"id": TEST_ZENDESK_CUSTOM_FIELD_CONFIG["enrollment_mode"], "value": enrollment.mode} + ) + + ticket = self._build_zendesk_ticket( + recipient=TEST_SUPPORT_EMAIL, + name=user.profile.name, + email=user.email, + subject=fields["subject"], + details=fields["details"], + tags=zendesk_tags, + custom_fields=zendesk_custom_fields + ) + + ticket_update = self._build_zendesk_ticket_update(TEST_REQUEST_HEADERS, user.username) + + self._test_success(user, fields) + self._assert_zendesk_called(zendesk_mock_instance, ticket_id, ticket, ticket_update) + self._assert_datadog_called(datadog_mock, datadog_tags) def test_get_request(self, zendesk_mock_class, datadog_mock): """Test that a GET results in a 405 even with all required fields""" @@ -329,7 +465,7 @@ class SubmitFeedbackTest(TestCase): resp = self._build_and_run_request(self._anon_user, self._anon_fields) self.assertEqual(resp.status_code, 500) self.assertFalse(resp.content) - self._assert_datadog_called(datadog_mock, with_tags=True) + self._assert_datadog_called(datadog_mock, ["issue_type:{}".format(self._anon_fields["issue_type"])]) def test_zendesk_error_on_update(self, zendesk_mock_class, datadog_mock): """ @@ -344,7 +480,7 @@ class SubmitFeedbackTest(TestCase): zendesk_mock_instance.update_ticket.side_effect = err resp = self._build_and_run_request(self._anon_user, self._anon_fields) self.assertEqual(resp.status_code, 200) - self._assert_datadog_called(datadog_mock, with_tags=True) + self._assert_datadog_called(datadog_mock, ["issue_type:{}".format(self._anon_fields["issue_type"])]) @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_FEEDBACK_SUBMISSION": False}) def test_not_enabled(self, zendesk_mock_class, datadog_mock): diff --git a/common/djangoapps/util/views.py b/common/djangoapps/util/views.py index ea886e7777..2c7045b540 100644 --- a/common/djangoapps/util/views.py +++ b/common/djangoapps/util/views.py @@ -25,6 +25,7 @@ from edxmako.shortcuts import render_to_response, render_to_string from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers import track.views from student.roles import GlobalStaff +from student.models import CourseEnrollment log = logging.getLogger(__name__) @@ -222,6 +223,40 @@ class _ZendeskApi(object): return None +def _get_zendesk_custom_field_context(request): + """ + Construct a dictionary of data that can be stored in Zendesk custom fields. + """ + context = {} + + course_id = request.POST.get("course_id") + if not course_id: + return context + + context["course_id"] = course_id + if not request.user.is_authenticated(): + return context + + enrollment = CourseEnrollment.get_enrollment(request.user, CourseKey.from_string(course_id)) + if enrollment and enrollment.is_active: + context["enrollment_mode"] = enrollment.mode + + return context + + +def _format_zendesk_custom_fields(context): + """ + Format the data in `context` for compatibility with the Zendesk API. + Ignore any keys that have not been configured in `ZENDESK_CUSTOM_FIELDS`. + """ + custom_fields = [] + for key, val, in settings.ZENDESK_CUSTOM_FIELDS.items(): + if key in context: + custom_fields.append({"id": val, "value": context[key]}) + + return custom_fields + + def _record_feedback_in_zendesk( realname, email, @@ -231,7 +266,8 @@ def _record_feedback_in_zendesk( additional_info, group_name=None, require_update=False, - support_email=None + support_email=None, + custom_fields=None ): """ Create a new user-requested Zendesk ticket. @@ -246,6 +282,8 @@ def _record_feedback_in_zendesk( If `require_update` is provided, returns False when the update does not succeed. This allows using the private comment to add necessary information which the user will not see in followup emails from support. + + If `custom_fields` is provided, submits data to those fields in Zendesk. """ zendesk_api = _ZendeskApi() @@ -271,6 +309,10 @@ def _record_feedback_in_zendesk( "tags": zendesk_tags } } + + if custom_fields: + new_ticket["ticket"]["custom_fields"] = custom_fields + group = None if group_name is not None: group = zendesk_api.get_group(group_name) @@ -322,7 +364,7 @@ def get_feedback_form_context(request): context["subject"] = request.POST["subject"] context["details"] = request.POST["details"] context["tags"] = dict( - [(tag, request.POST[tag]) for tag in ["issue_type", "course_id"] if tag in request.POST] + [(tag, request.POST[tag]) for tag in ["issue_type", "course_id"] if request.POST.get(tag)] ) context["additional_info"] = {} @@ -412,6 +454,11 @@ def submit_feedback(request): if not settings.ZENDESK_URL or not settings.ZENDESK_USER or not settings.ZENDESK_API_KEY: raise Exception("Zendesk enabled but not configured") + custom_fields = None + if settings.ZENDESK_CUSTOM_FIELDS: + custom_field_context = _get_zendesk_custom_field_context(request) + custom_fields = _format_zendesk_custom_fields(custom_field_context) + success = _record_feedback_in_zendesk( context["realname"], context["email"], @@ -419,7 +466,8 @@ def submit_feedback(request): context["details"], context["tags"], context["additional_info"], - support_email=context["support_email"] + support_email=context["support_email"], + custom_fields=custom_fields ) _record_feedback_in_datadog(context["tags"]) diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 340c2079fa..3e85617b0f 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -333,6 +333,8 @@ COMMENTS_SERVICE_URL = ENV_TOKENS.get("COMMENTS_SERVICE_URL", '') COMMENTS_SERVICE_KEY = ENV_TOKENS.get("COMMENTS_SERVICE_KEY", '') CERT_QUEUE = ENV_TOKENS.get("CERT_QUEUE", 'test-pull') ZENDESK_URL = ENV_TOKENS.get('ZENDESK_URL', ZENDESK_URL) +ZENDESK_CUSTOM_FIELDS = ENV_TOKENS.get('ZENDESK_CUSTOM_FIELDS', ZENDESK_CUSTOM_FIELDS) + FEEDBACK_SUBMISSION_EMAIL = ENV_TOKENS.get("FEEDBACK_SUBMISSION_EMAIL") MKTG_URLS = ENV_TOKENS.get('MKTG_URLS', MKTG_URLS) diff --git a/lms/envs/common.py b/lms/envs/common.py index 6c58a774cf..c826fd1b9f 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1003,6 +1003,7 @@ FEEDBACK_SUBMISSION_EMAIL = None ZENDESK_URL = None ZENDESK_USER = None ZENDESK_API_KEY = None +ZENDESK_CUSTOM_FIELDS = {} ##### EMBARGO ##### EMBARGO_SITE_REDIRECT_URL = None diff --git a/lms/static/sass/_build-lms-v1.scss b/lms/static/sass/_build-lms-v1.scss index bb376fec9f..460e2dbc2d 100644 --- a/lms/static/sass/_build-lms-v1.scss +++ b/lms/static/sass/_build-lms-v1.scss @@ -35,6 +35,7 @@ @import 'shared/modal'; @import 'shared/activation_messages'; @import 'shared/unsubscribe'; +@import 'shared/help-tab'; // shared - platform @import 'multicourse/home'; diff --git a/lms/static/sass/shared-v2/_help-tab.scss b/lms/static/sass/shared-v2/_help-tab.scss index 080c5f4e6c..99cf9b3836 100644 --- a/lms/static/sass/shared-v2/_help-tab.scss +++ b/lms/static/sass/shared-v2/_help-tab.scss @@ -78,3 +78,9 @@ padding: 0 $baseline $baseline; } } + +.feedback-form-select { + background: $white; + margin-bottom: $baseline; + width: 100%; +} diff --git a/lms/static/sass/shared/_help-tab.scss b/lms/static/sass/shared/_help-tab.scss new file mode 100644 index 0000000000..7af1407ac0 --- /dev/null +++ b/lms/static/sass/shared/_help-tab.scss @@ -0,0 +1,5 @@ +.feedback-form-select { + background: $white; + margin-bottom: $baseline; + width: 100%; +} diff --git a/lms/templates/help_modal.html b/lms/templates/help_modal.html index abe1779d99..5198e893c1 100644 --- a/lms/templates/help_modal.html +++ b/lms/templates/help_modal.html @@ -99,15 +99,21 @@ from xmodule.tabs import CourseTabList % endif + +
+ % if course: + + % endif +
+ + ${_('Describe what you were doing when you encountered the issue. Include any details that will help us to troubleshoot, including error messages that you saw.')} + - % if course: - - % endif
@@ -138,7 +144,12 @@ from xmodule.tabs import CourseTabList