Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,13 @@ if Backbone?
@responses.add(data['content']['children'])
@renderResponseCountAndPagination(data['content']['resp_total'])
@trigger "thread:responses:rendered"
error: =>
if firstLoad
error: (xhr) =>
if xhr.status == 404
DiscussionUtil.discussionAlert(
gettext("Sorry"),
gettext("The thread you selected has been deleted. Please select another thread.")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about "does not exist or has been deleted" ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no way to reach this without selecting a thread, so it must have existed at some point.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently when i put a bad url directly into the browser such as https://courses.edx.org/courses/BerkeleyX/CS-191x/2013_August/discussion/forum/i4x-BerkeleyX-CS191x-course-2013_Spring/threads/thisdoesnotexist then I get a 500. Does this PR change that?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you access a thread's URL directly, then we will return a 404 with the standard 404 page; this particular bit of code is handling an AJAX request for a thread.

)
else if firstLoad
DiscussionUtil.discussionAlert(
gettext("Sorry"),
gettext("We had some trouble loading responses. Please reload the page.")
Expand Down
22 changes: 20 additions & 2 deletions lms/djangoapps/django_comment_client/forum/tests.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import json
from django.http import Http404
from django.test.utils import override_settings
from django.test.client import Client, RequestFactory
from xmodule.modulestore.tests.factories import CourseFactory
Expand Down Expand Up @@ -108,19 +109,22 @@ def make_mock_thread_data(text, thread_id, include_children):
def make_mock_request_impl(text, thread_id=None):
def mock_request_impl(*args, **kwargs):
url = args[1]
data = None
if url.endswith("threads"):
data = {
"collection": [make_mock_thread_data(text, "dummy_thread_id", False)]
}
elif thread_id and url.endswith(thread_id):
data = make_mock_thread_data(text, thread_id, True)
else: # user query
elif "/users/" in url:
data = {
"upvoted_ids": [],
"downvoted_ids": [],
"subscribed_thread_ids": [],
}
return Mock(status_code=200, text=json.dumps(data), json=Mock(return_value=data))
if data:
return Mock(status_code=200, text=json.dumps(data), json=Mock(return_value=data))
return Mock(status_code=404)
return mock_request_impl


Expand Down Expand Up @@ -233,6 +237,20 @@ def test_post(self, mock_request):
)
self.assertEquals(response.status_code, 405)

def test_not_found(self, mock_request):
request = RequestFactory().get("dummy_url")
request.user = self.student
# Mock request to return 404 for thread request
mock_request.side_effect = make_mock_request_impl("dummy", thread_id=None)
self.assertRaises(
Http404,
views.single_thread,
request,
self.course.id,
"test_discussion_id",
"test_thread_id"
)


@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
@patch('requests.request')
Expand Down
17 changes: 11 additions & 6 deletions lms/djangoapps/django_comment_client/forum/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -237,12 +237,17 @@ def single_thread(request, course_id, discussion_id, thread_id):
# Currently, the front end always loads responses via AJAX, even for this
# page; it would be a nice optimization to avoid that extra round trip to
# the comments service.
thread = cc.Thread.find(thread_id).retrieve(
recursive=request.is_ajax(),
user_id=request.user.id,
response_skip=request.GET.get("resp_skip"),
response_limit=request.GET.get("resp_limit")
)
try:
thread = cc.Thread.find(thread_id).retrieve(
recursive=request.is_ajax(),
user_id=request.user.id,
response_skip=request.GET.get("resp_skip"),
response_limit=request.GET.get("resp_limit")
)
except cc.utils.CommentClientRequestError as e:
if e.status_code == 404:
raise Http404
raise

if request.is_ajax():
with newrelic.agent.FunctionTrace(nr_transaction, "get_annotated_content_infos"):
Expand Down