Commit 7c326f65 by Greg Price

Merge pull request #120 from edx/gprice/help-widget-tweaks

Reviewed by @brianhw https://rbcommons.com/s/edx/r/17/
parents 673c015e 2e38130c
......@@ -15,8 +15,9 @@ import mock
@mock.patch.dict("django.conf.settings.MITX_FEATURES", {"ENABLE_FEEDBACK_SUBMISSION": True})
@override_settings(ZENDESK_URL="dummy", ZENDESK_USER="dummy", ZENDESK_API_KEY="dummy")
@mock.patch("util.views.dog_stats_api")
@mock.patch("util.views._ZendeskApi", autospec=True)
class SubmitFeedbackViaZendeskTest(TestCase):
class SubmitFeedbackTest(TestCase):
def setUp(self):
"""Set up data for the test case"""
self._request_factory = RequestFactory()
......@@ -26,18 +27,19 @@ class SubmitFeedbackViaZendeskTest(TestCase):
username="test",
profile__name="Test User"
)
# This contains a tag to ensure that tags are submitted correctly
# 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",
"tag": "a tag"
"issue_type": "test_issue",
"course_id": "test_course"
}
# This does not contain a tag to ensure that tag is optional
# This does not contain issue_type nor course_id to ensure that they are optional
self._auth_fields = {"subject": "a subject", "details": "some details"}
def _test_request(self, user, fields):
def _build_and_run_request(self, user, fields):
"""
Generate a request and invoke the view, returning the response.
......@@ -48,12 +50,14 @@ class SubmitFeedbackViaZendeskTest(TestCase):
"/submit_feedback",
data=fields,
HTTP_REFERER="test_referer",
HTTP_USER_AGENT="test_user_agent"
HTTP_USER_AGENT="test_user_agent",
REMOTE_ADDR="1.2.3.4",
SERVER_NAME="test_server"
)
req.user = user
return views.submit_feedback_via_zendesk(req)
return views.submit_feedback(req)
def _assert_bad_request(self, response, field, zendesk_mock_class):
def _assert_bad_request(self, response, field, zendesk_mock_class, datadog_mock):
"""
Assert that the given `response` contains correct failure data.
......@@ -67,8 +71,9 @@ class SubmitFeedbackViaZendeskTest(TestCase):
self.assertTrue("error" in resp_json)
# There should be absolutely no interaction with Zendesk
self.assertFalse(zendesk_mock_class.return_value.mock_calls)
self.assertFalse(datadog_mock.mock_calls)
def _test_bad_request_omit_field(self, user, fields, omit_field, zendesk_mock_class):
def _test_bad_request_omit_field(self, user, fields, omit_field, zendesk_mock_class, datadog_mock):
"""
Invoke the view with a request missing a field and assert correctness.
......@@ -79,10 +84,10 @@ class SubmitFeedbackViaZendeskTest(TestCase):
have been invoked.
"""
filtered_fields = {k: v for (k, v) in fields.items() if k != omit_field}
resp = self._test_request(user, filtered_fields)
self._assert_bad_request(resp, omit_field, zendesk_mock_class)
resp = self._build_and_run_request(user, filtered_fields)
self._assert_bad_request(resp, omit_field, zendesk_mock_class, datadog_mock)
def _test_bad_request_empty_field(self, user, fields, empty_field, zendesk_mock_class):
def _test_bad_request_empty_field(self, user, fields, empty_field, zendesk_mock_class, datadog_mock):
"""
Invoke the view with an empty field and assert correctness.
......@@ -94,8 +99,8 @@ class SubmitFeedbackViaZendeskTest(TestCase):
"""
altered_fields = fields.copy()
altered_fields[empty_field] = ""
resp = self._test_request(user, altered_fields)
self._assert_bad_request(resp, empty_field, zendesk_mock_class)
resp = self._build_and_run_request(user, altered_fields)
self._assert_bad_request(resp, empty_field, zendesk_mock_class, datadog_mock)
def _test_success(self, user, fields):
"""
......@@ -105,30 +110,46 @@ class SubmitFeedbackViaZendeskTest(TestCase):
`fields` in the POST body. The response should have a 200 (success)
status code.
"""
resp = self._test_request(user, fields)
resp = self._build_and_run_request(user, fields)
self.assertEqual(resp.status_code, 200)
def test_bad_request_anon_user_no_name(self, zendesk_mock_class):
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 [])
)
]
self.assertEqual(datadog_mock.mock_calls, expected_datadog_calls)
def test_bad_request_anon_user_no_name(self, zendesk_mock_class, datadog_mock):
"""Test a request from an anonymous user not specifying `name`."""
self._test_bad_request_omit_field(self._anon_user, self._anon_fields, "name", zendesk_mock_class)
self._test_bad_request_empty_field(self._anon_user, self._anon_fields, "name", zendesk_mock_class)
self._test_bad_request_omit_field(self._anon_user, self._anon_fields, "name", zendesk_mock_class, datadog_mock)
self._test_bad_request_empty_field(self._anon_user, self._anon_fields, "name", zendesk_mock_class, datadog_mock)
def test_bad_request_anon_user_no_email(self, zendesk_mock_class):
def test_bad_request_anon_user_no_email(self, zendesk_mock_class, datadog_mock):
"""Test a request from an anonymous user not specifying `email`."""
self._test_bad_request_omit_field(self._anon_user, self._anon_fields, "email", zendesk_mock_class)
self._test_bad_request_empty_field(self._anon_user, self._anon_fields, "email", zendesk_mock_class)
self._test_bad_request_omit_field(self._anon_user, self._anon_fields, "email", zendesk_mock_class, datadog_mock)
self._test_bad_request_empty_field(self._anon_user, self._anon_fields, "email", zendesk_mock_class, datadog_mock)
def test_bad_request_anon_user_invalid_email(self, zendesk_mock_class, datadog_mock):
"""Test a request from an anonymous user specifying an invalid `email`."""
fields = self._anon_fields.copy()
fields["email"] = "This is not a valid email address!"
resp = self._build_and_run_request(self._anon_user, fields)
self._assert_bad_request(resp, "email", zendesk_mock_class, datadog_mock)
def test_bad_request_anon_user_no_subject(self, zendesk_mock_class):
def test_bad_request_anon_user_no_subject(self, zendesk_mock_class, datadog_mock):
"""Test a request from an anonymous user not specifying `subject`."""
self._test_bad_request_omit_field(self._anon_user, self._anon_fields, "subject", zendesk_mock_class)
self._test_bad_request_empty_field(self._anon_user, self._anon_fields, "subject", zendesk_mock_class)
self._test_bad_request_omit_field(self._anon_user, self._anon_fields, "subject", zendesk_mock_class, datadog_mock)
self._test_bad_request_empty_field(self._anon_user, self._anon_fields, "subject", zendesk_mock_class, datadog_mock)
def test_bad_request_anon_user_no_details(self, zendesk_mock_class):
def test_bad_request_anon_user_no_details(self, zendesk_mock_class, datadog_mock):
"""Test a request from an anonymous user not specifying `details`."""
self._test_bad_request_omit_field(self._anon_user, self._anon_fields, "details", zendesk_mock_class)
self._test_bad_request_empty_field(self._anon_user, self._anon_fields, "details", zendesk_mock_class)
self._test_bad_request_omit_field(self._anon_user, self._anon_fields, "details", zendesk_mock_class, datadog_mock)
self._test_bad_request_empty_field(self._anon_user, self._anon_fields, "details", zendesk_mock_class, datadog_mock)
def test_valid_request_anon_user(self, zendesk_mock_class):
def test_valid_request_anon_user(self, zendesk_mock_class, datadog_mock):
"""
Test a valid request from an anonymous user.
......@@ -138,14 +159,14 @@ class SubmitFeedbackViaZendeskTest(TestCase):
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_calls = [
expected_zendesk_calls = [
mock.call.create_ticket(
{
"ticket": {
"requester": {"name": "Test User", "email": "test@edx.org"},
"subject": "a subject",
"comment": {"body": "some details"},
"tags": ["a tag"]
"tags": ["test_course", "test_issue", "LMS"]
}
}
),
......@@ -157,26 +178,29 @@ class SubmitFeedbackViaZendeskTest(TestCase):
"public": False,
"body":
"Additional information:\n\n"
"HTTP_USER_AGENT: test_user_agent\n"
"HTTP_REFERER: test_referer"
"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_calls)
self.assertEqual(zendesk_mock_instance.mock_calls, expected_zendesk_calls)
self._assert_datadog_called(datadog_mock, with_tags=True)
def test_bad_request_auth_user_no_subject(self, zendesk_mock_class):
def test_bad_request_auth_user_no_subject(self, zendesk_mock_class, datadog_mock):
"""Test a request from an authenticated user not specifying `subject`."""
self._test_bad_request_omit_field(self._auth_user, self._auth_fields, "subject", zendesk_mock_class)
self._test_bad_request_empty_field(self._auth_user, self._auth_fields, "subject", zendesk_mock_class)
self._test_bad_request_omit_field(self._auth_user, self._auth_fields, "subject", zendesk_mock_class, datadog_mock)
self._test_bad_request_empty_field(self._auth_user, self._auth_fields, "subject", zendesk_mock_class, datadog_mock)
def test_bad_request_auth_user_no_details(self, zendesk_mock_class):
def test_bad_request_auth_user_no_details(self, zendesk_mock_class, datadog_mock):
"""Test a request from an authenticated user not specifying `details`."""
self._test_bad_request_omit_field(self._auth_user, self._auth_fields, "details", zendesk_mock_class)
self._test_bad_request_empty_field(self._auth_user, self._auth_fields, "details", zendesk_mock_class)
self._test_bad_request_omit_field(self._auth_user, self._auth_fields, "details", zendesk_mock_class, datadog_mock)
self._test_bad_request_empty_field(self._auth_user, self._auth_fields, "details", zendesk_mock_class, datadog_mock)
def test_valid_request_auth_user(self, zendesk_mock_class):
def test_valid_request_auth_user(self, zendesk_mock_class, datadog_mock):
"""
Test a valid request from an authenticated user.
......@@ -186,14 +210,14 @@ class SubmitFeedbackViaZendeskTest(TestCase):
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_calls = [
expected_zendesk_calls = [
mock.call.create_ticket(
{
"ticket": {
"requester": {"name": "Test User", "email": "test@edx.org"},
"subject": "a subject",
"comment": {"body": "some details"},
"tags": []
"tags": ["LMS"]
}
}
),
......@@ -206,27 +230,31 @@ class SubmitFeedbackViaZendeskTest(TestCase):
"body":
"Additional information:\n\n"
"username: test\n"
"HTTP_USER_AGENT: test_user_agent\n"
"HTTP_REFERER: test_referer"
"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_calls)
self.assertEqual(zendesk_mock_instance.mock_calls, expected_zendesk_calls)
self._assert_datadog_called(datadog_mock, with_tags=False)
def test_get_request(self, zendesk_mock_class):
def test_get_request(self, zendesk_mock_class, datadog_mock):
"""Test that a GET results in a 405 even with all required fields"""
req = self._request_factory.get("/submit_feedback", data=self._anon_fields)
req.user = self._anon_user
resp = views.submit_feedback_via_zendesk(req)
resp = views.submit_feedback(req)
self.assertEqual(resp.status_code, 405)
self.assertIn("Allow", resp)
self.assertEqual(resp["Allow"], "POST")
# There should be absolutely no interaction with Zendesk
self.assertFalse(zendesk_mock_class.mock_calls)
self.assertFalse(datadog_mock.mock_calls)
def test_zendesk_error_on_create(self, zendesk_mock_class):
def test_zendesk_error_on_create(self, zendesk_mock_class, datadog_mock):
"""
Test Zendesk returning an error on ticket creation.
......@@ -235,11 +263,12 @@ class SubmitFeedbackViaZendeskTest(TestCase):
err = ZendeskError(msg="", error_code=404)
zendesk_mock_instance = zendesk_mock_class.return_value
zendesk_mock_instance.create_ticket.side_effect = err
resp = self._test_request(self._anon_user, self._anon_fields)
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)
def test_zendesk_error_on_update(self, zendesk_mock_class):
def test_zendesk_error_on_update(self, zendesk_mock_class, datadog_mock):
"""
Test for Zendesk returning an error on ticket update.
......@@ -250,20 +279,21 @@ class SubmitFeedbackViaZendeskTest(TestCase):
err = ZendeskError(msg="", error_code=500)
zendesk_mock_instance = zendesk_mock_class.return_value
zendesk_mock_instance.update_ticket.side_effect = err
resp = self._test_request(self._anon_user, self._anon_fields)
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)
@mock.patch.dict("django.conf.settings.MITX_FEATURES", {"ENABLE_FEEDBACK_SUBMISSION": False})
def test_not_enabled(self, zendesk_mock_class):
def test_not_enabled(self, zendesk_mock_class, datadog_mock):
"""
Test for Zendesk submission not enabled in `settings`.
We should raise Http404.
"""
with self.assertRaises(Http404):
self._test_request(self._anon_user, self._anon_fields)
self._build_and_run_request(self._anon_user, self._anon_fields)
def test_zendesk_not_configured(self, zendesk_mock_class):
def test_zendesk_not_configured(self, zendesk_mock_class, datadog_mock):
"""
Test for Zendesk not fully configured in `settings`.
......@@ -273,7 +303,7 @@ class SubmitFeedbackViaZendeskTest(TestCase):
def test_case(missing_config):
with mock.patch(missing_config, None):
with self.assertRaises(Exception):
self._test_request(self._anon_user, self._anon_fields)
self._build_and_run_request(self._anon_user, self._anon_fields)
test_case("django.conf.settings.ZENDESK_URL")
test_case("django.conf.settings.ZENDESK_USER")
......
......@@ -12,6 +12,7 @@ from django.core.validators import ValidationError, validate_email
from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpResponseNotAllowed, HttpResponseServerError
from django.shortcuts import redirect
from django_future.csrf import ensure_csrf_cookie
from dogapi import dog_stats_api
from mitxmako.shortcuts import render_to_response, render_to_string
from urllib import urlencode
import zendesk
......@@ -73,11 +74,64 @@ class _ZendeskApi(object):
self._zendesk_instance.update_ticket(ticket_id=ticket_id, data=update)
def submit_feedback_via_zendesk(request):
def _record_feedback_in_zendesk(realname, email, subject, details, tags, additional_info):
"""
Create a new user-requested Zendesk ticket.
If Zendesk submission is not enabled, any request will raise `Http404`.
Once created, the ticket will be updated with a private comment containing
additional information from the browser and server, such as HTTP headers
and user state. Returns a boolean value indicating whether ticket creation
was successful, regardless of whether the private comment update succeeded.
"""
zendesk_api = _ZendeskApi()
additional_info_string = (
"Additional information:\n\n" +
"\n".join("%s: %s" % (key, value) for (key, value) in additional_info.items() if value is not None)
)
# Tag all issues with LMS to distinguish channel in Zendesk; requested by student support team
zendesk_tags = list(tags.values()) + ["LMS"]
new_ticket = {
"ticket": {
"requester": {"name": realname, "email": email},
"subject": subject,
"comment": {"body": details},
"tags": zendesk_tags
}
}
try:
ticket_id = zendesk_api.create_ticket(new_ticket)
except zendesk.ZendeskError as err:
log.error("Error creating Zendesk ticket: %s", str(err))
return False
# Additional information is provided as a private update so the information
# is not visible to the user.
ticket_update = {"ticket": {"comment": {"public": False, "body": additional_info_string}}}
try:
zendesk_api.update_ticket(ticket_id, ticket_update)
except zendesk.ZendeskError as err:
log.error("Error updating Zendesk ticket: %s", str(err))
# The update is not strictly necessary, so do not indicate failure to the user
pass
return True
DATADOG_FEEDBACK_METRIC = "lms_feedback_submissions"
def _record_feedback_in_datadog(tags):
datadog_tags = ["{k}:{v}".format(k=k, v=v) for k, v in tags.items()]
dog_stats_api.increment(DATADOG_FEEDBACK_METRIC, tags=datadog_tags)
def submit_feedback(request):
"""
Create a new user-requested ticket, currently implemented with Zendesk.
If feedback submission is not enabled, any request will raise `Http404`.
If any configuration parameter (`ZENDESK_URL`, `ZENDESK_USER`, or
`ZENDESK_API_KEY`) is missing, any request will raise an `Exception`.
The request must be a POST request specifying `subject` and `details`.
......@@ -85,12 +139,9 @@ def submit_feedback_via_zendesk(request):
`email`. If the user is authenticated, the `name` and `email` will be
populated from the user's information. If any required parameter is
missing, a 400 error will be returned indicating which field is missing and
providing an error message. If Zendesk returns any error on ticket
creation, a 500 error will be returned with no body. Once created, the
ticket will be updated with a private comment containing additional
information from the browser and server, such as HTTP headers and user
state. Whether or not the update succeeds, if the user's ticket is
successfully created, an empty successful response (200) will be returned.
providing an error message. If Zendesk ticket creation fails, 500 error
will be returned with no body; if ticket creation succeeds, an empty
successful response (200) will be returned.
"""
if not settings.MITX_FEATURES.get('ENABLE_FEEDBACK_SUBMISSION', False):
raise Http404()
......@@ -124,9 +175,9 @@ def submit_feedback_via_zendesk(request):
subject = request.POST["subject"]
details = request.POST["details"]
tags = []
if "tag" in request.POST:
tags = [request.POST["tag"]]
tags = dict(
[(tag, request.POST[tag]) for tag in ["issue_type", "course_id"] if tag in request.POST]
)
if request.user.is_authenticated():
realname = request.user.profile.name
......@@ -140,41 +191,18 @@ def submit_feedback_via_zendesk(request):
except ValidationError:
return build_error_response(400, "email", required_field_errs["email"])
for header in ["HTTP_REFERER", "HTTP_USER_AGENT"]:
additional_info[header] = request.META.get(header)
for header, pretty in [
("HTTP_REFERER", "Page"),
("HTTP_USER_AGENT", "Browser"),
("REMOTE_ADDR", "Client IP"),
("SERVER_NAME", "Host")
]:
additional_info[pretty] = request.META.get(header)
zendesk_api = _ZendeskApi()
additional_info_string = (
"Additional information:\n\n" +
"\n".join("%s: %s" % (key, value) for (key, value) in additional_info.items() if value is not None)
)
new_ticket = {
"ticket": {
"requester": {"name": realname, "email": email},
"subject": subject,
"comment": {"body": details},
"tags": tags
}
}
try:
ticket_id = zendesk_api.create_ticket(new_ticket)
except zendesk.ZendeskError as err:
log.error("Error creating Zendesk ticket: %s", str(err))
return HttpResponse(status=500)
# Additional information is provided as a private update so the information
# is not visible to the user.
ticket_update = {"ticket": {"comment": {"public": False, "body": additional_info_string}}}
try:
zendesk_api.update_ticket(ticket_id, ticket_update)
except zendesk.ZendeskError as err:
log.error("Error updating Zendesk ticket: %s", str(err))
# The update is not strictly necessary, so do not indicate failure to the user
pass
success = _record_feedback_in_zendesk(realname, email, subject, details, tags, additional_info)
_record_feedback_in_datadog(tags)
return HttpResponse()
return HttpResponse(status=(200 if success else 500))
def info(request):
......
......@@ -64,7 +64,10 @@ discussion_link = get_discussion_link(course) if course else None
<label data-field="details">Tell us the details*
<span class="tip">Include error messages, steps which lead to the issue, etc</span></label>
<textarea name="details"></textarea>
<input name="tag" type="hidden">
<input name="issue_type" type="hidden">
% if course:
<input name="course_id" type="hidden" value="${course.id | h}">
% endif
<div class="submit">
<input name="submit" type="submit" value="Submit">
</div>
......@@ -114,21 +117,41 @@ discussion_link = get_discussion_link(course) if course else None
$("#feedback_success_wrapper").css("display", "none");
$("#help_wrapper").css("display", "block");
});
showFeedback = function(e, tag, title) {
showFeedback = function(event, issue_type, title, subject_label, details_label) {
$("#help_wrapper").css("display", "none");
$("#feedback_form input[name='tag']").val(tag);
$("#feedback_form input[name='issue_type']").val(issue_type);
$("#feedback_form_wrapper").css("display", "block");
$("#feedback_form_wrapper header").html("<h2>" + title + "</h2><hr>");
e.preventDefault();
$("#feedback_form_wrapper label[data-field='subject']").html(subject_label);
$("#feedback_form_wrapper label[data-field='details']").html(details_label);
event.preventDefault();
};
$("#feedback_link_problem").click(function(e) {
showFeedback(e, "problem", "Report a Problem");
$("#feedback_link_problem").click(function(event) {
showFeedback(
event,
"problem",
"Report a Problem",
"Brief description of the problem*",
"Details of the problem you are encountering* <span class='tip'>Include error messages, steps which lead to the issue, etc.</span>"
);
});
$("#feedback_link_suggestion").click(function(e) {
showFeedback(e, "suggestion", "Make a Suggestion");
$("#feedback_link_suggestion").click(function(event) {
showFeedback(
event,
"suggestion",
"Make a Suggestion",
"Brief description of your suggestion*",
"Details*"
);
});
$("#feedback_link_question").click(function(e) {
showFeedback(e, "question", "Ask a Question");
$("#feedback_link_question").click(function(event) {
showFeedback(
event,
"question",
"Ask a Question",
"Brief summary of your question*",
"Details*"
);
});
$("#feedback_form").submit(function() {
$("input[type='submit']", this).attr("disabled", "disabled");
......
......@@ -111,7 +111,7 @@ if not settings.MITX_FEATURES["USE_CUSTOM_THEME"]:
# Favicon
(r'^favicon\.ico$', 'django.views.generic.simple.redirect_to', {'url': '/static/images/favicon.ico'}),
url(r'^submit_feedback$', 'util.views.submit_feedback_via_zendesk'),
url(r'^submit_feedback$', 'util.views.submit_feedback'),
# TODO: These urls no longer work. They need to be updated before they are re-enabled
# url(r'^reactivate/(?P<key>[^/]*)$', 'student.views.reactivation_email'),
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment