diff --git a/common/djangoapps/enrollment/data.py b/common/djangoapps/enrollment/data.py index 68880e1a3e..ab82543be0 100644 --- a/common/djangoapps/enrollment/data.py +++ b/common/djangoapps/enrollment/data.py @@ -8,11 +8,12 @@ from django.contrib.auth.models import User from opaque_keys.edx.keys import CourseKey from enrollment.errors import ( - CourseNotFoundError, CourseEnrollmentClosedError, CourseEnrollmentFullError, + CourseEnrollmentClosedError, CourseEnrollmentFullError, CourseEnrollmentExistsError, UserNotFoundError, InvalidEnrollmentAttribute ) from enrollment.serializers import CourseEnrollmentSerializer, CourseSerializer from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.lib.exceptions import CourseNotFoundError from student.models import ( CourseEnrollment, NonExistentCourseError, EnrollmentClosedError, CourseFullError, AlreadyEnrolledError, CourseEnrollmentAttribute diff --git a/common/djangoapps/enrollment/errors.py b/common/djangoapps/enrollment/errors.py index cc66e86ed3..36d6f827b9 100644 --- a/common/djangoapps/enrollment/errors.py +++ b/common/djangoapps/enrollment/errors.py @@ -13,10 +13,6 @@ class CourseEnrollmentError(Exception): self.data = data -class CourseNotFoundError(CourseEnrollmentError): - pass - - class UserNotFoundError(CourseEnrollmentError): pass diff --git a/common/djangoapps/enrollment/tests/test_data.py b/common/djangoapps/enrollment/tests/test_data.py index 6702ac7e2d..f4ce7b001a 100644 --- a/common/djangoapps/enrollment/tests/test_data.py +++ b/common/djangoapps/enrollment/tests/test_data.py @@ -11,9 +11,10 @@ from django.conf import settings from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from enrollment.errors import ( - CourseNotFoundError, UserNotFoundError, CourseEnrollmentClosedError, + UserNotFoundError, CourseEnrollmentClosedError, CourseEnrollmentFullError, CourseEnrollmentExistsError, ) +from openedx.core.lib.exceptions import CourseNotFoundError from student.tests.factories import UserFactory, CourseModeFactory from student.models import CourseEnrollment, EnrollmentClosedError, CourseFullError, AlreadyEnrolledError from enrollment import data diff --git a/common/djangoapps/enrollment/views.py b/common/djangoapps/enrollment/views.py index 69aea5996c..796e183b94 100644 --- a/common/djangoapps/enrollment/views.py +++ b/common/djangoapps/enrollment/views.py @@ -24,11 +24,13 @@ from openedx.core.lib.api.authentication import ( SessionAuthenticationAllowInactiveUser, OAuth2AuthenticationAllowInactiveUser, ) +from openedx.core.lib.exceptions import CourseNotFoundError from util.disable_rate_limit import can_disable_rate_limit from enrollment import api from enrollment.errors import ( - CourseNotFoundError, CourseEnrollmentError, - CourseModeNotFoundError, CourseEnrollmentExistsError + CourseEnrollmentError, + CourseModeNotFoundError, + CourseEnrollmentExistsError ) from student.auth import user_has_role from student.models import User diff --git a/lms/djangoapps/course_structure_api/v0/views.py b/lms/djangoapps/course_structure_api/v0/views.py index d75f018315..50359ecb9c 100644 --- a/lms/djangoapps/course_structure_api/v0/views.py +++ b/lms/djangoapps/course_structure_api/v0/views.py @@ -23,6 +23,7 @@ from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor from openedx.core.lib.api.view_utils import view_course_access, view_auth_classes from openedx.core.djangoapps.content.course_structures.api.v0 import api, errors +from openedx.core.lib.exceptions import CourseNotFoundError from student.roles import CourseInstructorRole, CourseStaffRole from util.module_utils import get_dynamic_descriptor_children @@ -73,7 +74,7 @@ class CourseViewMixin(object): self.course_key = CourseKey.from_string(course_id) self.check_course_permissions(self.request.user, self.course_key) return func(self, *args, **kwargs) - except errors.CourseNotFoundError: + except CourseNotFoundError: raise Http404 return func_wrapper diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 639f066a0d..4b36b7f6aa 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -16,6 +16,7 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseKey from courseware.courses import get_course_with_access +from discussion_api.exceptions import ThreadNotFoundError, CommentNotFoundError, DiscussionDisabledError from discussion_api.forms import CommentActionsForm, ThreadActionsForm from discussion_api.pagination import get_paginated_data from discussion_api.permissions import ( @@ -41,17 +42,21 @@ from lms.lib.comment_client.comment import Comment from lms.lib.comment_client.thread import Thread from lms.lib.comment_client.utils import CommentClientRequestError from openedx.core.djangoapps.course_groups.cohorts import get_cohort_id +from openedx.core.lib.exceptions import CourseNotFoundError, PageNotFoundError -def _get_course_or_404(course_key, user): +def _get_course(course_key, user): """ - Get the course descriptor, raising Http404 if the course is not found, - the user cannot access forums for the course, or the discussion tab is - disabled for the course. + Get the course descriptor, raising CourseNotFoundError if the course is not found or + the user cannot access forums for the course, and DiscussionDisabledError if the + discussion tab is disabled for the course. """ - course = get_course_with_access(user, 'load', course_key, check_if_enrolled=True) + try: + course = get_course_with_access(user, 'load', course_key, check_if_enrolled=True) + except Http404: + raise CourseNotFoundError("Course not found.") if not any([tab.type == 'discussion' and tab.is_enabled(course, user) for tab in course.tabs]): - raise Http404 + raise DiscussionDisabledError("Discussion is disabled for the course.") return course @@ -60,8 +65,8 @@ def _get_thread_and_context(request, thread_id, retrieve_kwargs=None): Retrieve the given thread and build a serializer context for it, returning both. This function also enforces access control for the thread (checking both the user's access to the course and to the thread's cohort if - applicable). Raises Http404 if the thread does not exist or the user cannot - access it. + applicable). Raises ThreadNotFoundError if the thread does not exist or the + user cannot access it. """ retrieve_kwargs = retrieve_kwargs or {} try: @@ -69,7 +74,7 @@ def _get_thread_and_context(request, thread_id, retrieve_kwargs=None): retrieve_kwargs["mark_as_read"] = False cc_thread = Thread(id=thread_id).retrieve(**retrieve_kwargs) course_key = CourseKey.from_string(cc_thread["course_id"]) - course = _get_course_or_404(course_key, request.user) + course = _get_course(course_key, request.user) context = get_context(course, request, cc_thread) if ( not context["is_requester_privileged"] and @@ -78,12 +83,12 @@ def _get_thread_and_context(request, thread_id, retrieve_kwargs=None): ): requester_cohort = get_cohort_id(request.user, course.id) if requester_cohort is not None and cc_thread["group_id"] != requester_cohort: - raise Http404 + raise ThreadNotFoundError("Thread not found.") return cc_thread, context except CommentClientRequestError: # params are validated at a higher level, so the only possible request # error is if the thread doesn't exist - raise Http404 + raise ThreadNotFoundError("Thread not found.") def _get_comment_and_context(request, comment_id): @@ -91,15 +96,15 @@ def _get_comment_and_context(request, comment_id): Retrieve the given comment and build a serializer context for it, returning both. This function also enforces access control for the comment (checking both the user's access to the course and to the comment's thread's cohort if - applicable). Raises Http404 if the comment does not exist or the user cannot - access it. + applicable). Raises CommentNotFoundError if the comment does not exist or the + user cannot access it. """ try: cc_comment = Comment(id=comment_id).retrieve() _, context = _get_thread_and_context(request, cc_comment["thread_id"]) return cc_comment, context except CommentClientRequestError: - raise Http404 + raise CommentNotFoundError("Comment not found.") def _is_user_author_or_privileged(cc_content, context): @@ -146,10 +151,10 @@ def get_course(request, course_key): Raises: - Http404: if the course does not exist or is not accessible to the - requesting user + CourseNotFoundError: if the course does not exist or is not accessible + to the requesting user """ - course = _get_course_or_404(course_key, request.user) + course = _get_course(course_key, request.user) return { "id": unicode(course_key), "blackouts": [ @@ -184,8 +189,7 @@ def get_course_topics(request, course_key): setting if absent) """ return module.sort_key or module.discussion_target - - course = _get_course_or_404(course_key, request.user) + course = _get_course(course_key, request.user) discussion_modules = get_accessible_discussion_modules(course, request.user) modules_by_category = defaultdict(list) for module in discussion_modules: @@ -279,8 +283,8 @@ def get_thread_list( ValidationError: if an invalid value is passed for a field. ValueError: if more than one of the mutually exclusive parameters is provided - Http404: if the requesting user does not have access to the requested course - or a page beyond the last is requested + CourseNotFoundError: if the requesting user does not have access to the requested course + PageNotFoundError: if page requested is beyond the last """ exclusive_param_count = sum(1 for param in [topic_id_list, text_search, following] if param) if exclusive_param_count > 1: # pragma: no cover @@ -297,7 +301,7 @@ def get_thread_list( "order_direction": ["Invalid value. '{}' must be 'asc' or 'desc'".format(order_direction)] }) - course = _get_course_or_404(course_key, request.user) + course = _get_course(course_key, request.user) context = get_context(course, request) query_params = { @@ -332,9 +336,9 @@ def get_thread_list( threads, result_page, num_pages, text_search_rewrite = Thread.search(query_params) # The comments service returns the last page of results if the requested # page is beyond the last page, but we want be consistent with DRF's general - # behavior and return a 404 in that case + # behavior and return a PageNotFoundError in that case if result_page != page: - raise Http404 + raise PageNotFoundError("Page not found (No results on this page).") results = [ThreadSerializer(thread, context=context).data for thread in threads] ret = get_paginated_data(request, results, page, num_pages) @@ -402,9 +406,9 @@ def get_comment_list(request, thread_id, endorsed, page, page_size): # The comments service returns the last page of results if the requested # page is beyond the last page, but we want be consistent with DRF's general - # behavior and return a 404 in that case + # behavior and return a PageNotFoundError in that case if not responses and page != 1: - raise Http404 + raise PageNotFoundError("Page not found (No results on this page).") num_pages = (resp_total + page_size - 1) / page_size if resp_total else 1 results = [CommentSerializer(response, context=context).data for response in responses] @@ -535,8 +539,8 @@ def create_thread(request, thread_data): raise ValidationError({"course_id": ["This field is required."]}) try: course_key = CourseKey.from_string(course_id) - course = _get_course_or_404(course_key, user) - except (Http404, InvalidKeyError): + course = _get_course(course_key, user) + except InvalidKeyError: raise ValidationError({"course_id": ["Invalid value."]}) context = get_context(course, request) @@ -581,10 +585,7 @@ def create_comment(request, comment_data): thread_id = comment_data.get("thread_id") if not thread_id: raise ValidationError({"thread_id": ["This field is required."]}) - try: - cc_thread, context = _get_thread_and_context(request, thread_id) - except Http404: - raise ValidationError({"thread_id": ["Invalid value."]}) + cc_thread, context = _get_thread_and_context(request, thread_id) # if a thread is closed; no new comments could be made to it if cc_thread['closed']: @@ -660,8 +661,8 @@ def update_comment(request, comment_id, update_data): Raises: - Http404: if the comment does not exist or is not accessible to the - requesting user + CommentNotFoundError: if the comment does not exist or is not accessible + to the requesting user PermissionDenied: if the comment is accessible to but not editable by the requesting user @@ -752,7 +753,7 @@ def get_response_comments(request, comment_id, page, page_size): num_pages = (comments_count + page_size - 1) / page_size if comments_count else 1 return get_paginated_data(request, results, page, num_pages) except CommentClientRequestError: - raise Http404 + raise CommentNotFoundError("Comment not found") def delete_thread(request, thread_id): diff --git a/lms/djangoapps/discussion_api/exceptions.py b/lms/djangoapps/discussion_api/exceptions.py new file mode 100644 index 0000000000..c251a88fa7 --- /dev/null +++ b/lms/djangoapps/discussion_api/exceptions.py @@ -0,0 +1,17 @@ +""" Errors used by the Discussion API. """ +from django.core.exceptions import ObjectDoesNotExist + + +class DiscussionDisabledError(ObjectDoesNotExist): + """ Discussion is disabled. """ + pass + + +class ThreadNotFoundError(ObjectDoesNotExist): + """ Thread was not found. """ + pass + + +class CommentNotFoundError(ObjectDoesNotExist): + """ Comment was not found. """ + pass diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 3294bd8d7f..f333861f78 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -12,7 +12,6 @@ import mock from pytz import UTC from django.core.exceptions import ValidationError -from django.http import Http404 from django.test.client import RequestFactory from rest_framework.exceptions import PermissionDenied @@ -35,6 +34,7 @@ from discussion_api.api import ( update_thread, get_thread, ) +from discussion_api.exceptions import DiscussionDisabledError, ThreadNotFoundError, CommentNotFoundError from discussion_api.tests.utils import ( CommentsServiceMockMixin, make_minimal_cs_comment, @@ -49,6 +49,7 @@ from django_comment_common.models import ( ) from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory +from openedx.core.lib.exceptions import CourseNotFoundError, PageNotFoundError from student.tests.factories import CourseEnrollmentFactory, UserFactory from util.testing import UrlResetMixin from xmodule.modulestore.django import modulestore @@ -99,17 +100,17 @@ class GetCourseTest(UrlResetMixin, SharedModuleStoreTestCase): self.request.user = self.user def test_nonexistent_course(self): - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): get_course(self.request, CourseLocator.from_string("non/existent/course")) def test_not_enrolled(self): unenrolled_user = UserFactory.create() self.request.user = unenrolled_user - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): get_course(self.request, self.course.id) def test_discussions_disabled(self): - with self.assertRaises(Http404): + with self.assertRaises(DiscussionDisabledError): get_course(self.request, _discussion_disabled_course_for(self.user).id) def test_basic(self): @@ -226,18 +227,18 @@ class GetCourseTopicsTest(UrlResetMixin, ModuleStoreTestCase): return node def test_nonexistent_course(self): - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): get_course_topics(self.request, CourseLocator.from_string("non/existent/course")) def test_not_enrolled(self): unenrolled_user = UserFactory.create() self.request.user = unenrolled_user - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): self.get_course_topics() def test_discussions_disabled(self): _remove_discussion_tab(self.course, self.user.id) - with self.assertRaises(Http404): + with self.assertRaises(DiscussionDisabledError): self.get_course_topics() def test_without_courseware(self): @@ -523,16 +524,16 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto return ret def test_nonexistent_course(self): - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): get_thread_list(self.request, CourseLocator.from_string("non/existent/course"), 1, 1) def test_not_enrolled(self): self.request.user = UserFactory.create() - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): self.get_thread_list([]) def test_discussions_disabled(self): - with self.assertRaises(Http404): + with self.assertRaises(DiscussionDisabledError): self.get_thread_list([], course=_discussion_disabled_course_for(self.user)) def test_empty(self): @@ -752,7 +753,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto # Test page past the last one self.register_get_threads_response([], page=3, num_pages=3) - with self.assertRaises(Http404): + with self.assertRaises(PageNotFoundError): get_thread_list(self.request, self.course.id, page=4, page_size=10) @ddt.data(None, "rewritten search string") @@ -952,21 +953,21 @@ class GetCommentListTest(CommentsServiceMockMixin, SharedModuleStoreTestCase): def test_nonexistent_thread(self): thread_id = "nonexistent_thread" self.register_get_thread_error_response(thread_id, 404) - with self.assertRaises(Http404): + with self.assertRaises(ThreadNotFoundError): get_comment_list(self.request, thread_id, endorsed=False, page=1, page_size=1) def test_nonexistent_course(self): - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): self.get_comment_list(self.make_minimal_cs_thread({"course_id": "non/existent/course"})) def test_not_enrolled(self): self.request.user = UserFactory.create() - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): self.get_comment_list(self.make_minimal_cs_thread()) def test_discussions_disabled(self): disabled_course = _discussion_disabled_course_for(self.user) - with self.assertRaises(Http404): + with self.assertRaises(DiscussionDisabledError): self.get_comment_list( self.make_minimal_cs_thread( overrides={"course_id": unicode(disabled_course.id)} @@ -1023,7 +1024,7 @@ class GetCommentListTest(CommentsServiceMockMixin, SharedModuleStoreTestCase): try: self.get_comment_list(thread) self.assertFalse(expected_error) - except Http404: + except ThreadNotFoundError: self.assertTrue(expected_error) @ddt.data(True, False) @@ -1255,7 +1256,7 @@ class GetCommentListTest(CommentsServiceMockMixin, SharedModuleStoreTestCase): response_field: [], response_total_field: 5 }) - with self.assertRaises(Http404): + with self.assertRaises(PageNotFoundError): self.get_comment_list(thread, endorsed=endorsed_arg, page=2, page_size=5) def test_question_endorsed_pagination(self): @@ -1327,7 +1328,7 @@ class GetCommentListTest(CommentsServiceMockMixin, SharedModuleStoreTestCase): ) # Page past the end - with self.assertRaises(Http404): + with self.assertRaises(PageNotFoundError): self.get_comment_list(thread, endorsed=True, page=2, page_size=10) @@ -1561,22 +1562,19 @@ class CreateThreadTest( self.assertEqual(assertion.exception.message_dict, {"course_id": ["Invalid value."]}) def test_nonexistent_course(self): - with self.assertRaises(ValidationError) as assertion: + with self.assertRaises(CourseNotFoundError): create_thread(self.request, {"course_id": "non/existent/course"}) - self.assertEqual(assertion.exception.message_dict, {"course_id": ["Invalid value."]}) def test_not_enrolled(self): self.request.user = UserFactory.create() - with self.assertRaises(ValidationError) as assertion: + with self.assertRaises(CourseNotFoundError): create_thread(self.request, self.minimal_data) - self.assertEqual(assertion.exception.message_dict, {"course_id": ["Invalid value."]}) def test_discussions_disabled(self): disabled_course = _discussion_disabled_course_for(self.user) self.minimal_data["course_id"] = unicode(disabled_course.id) - with self.assertRaises(ValidationError) as assertion: + with self.assertRaises(DiscussionDisabledError): create_thread(self.request, self.minimal_data) - self.assertEqual(assertion.exception.message_dict, {"course_id": ["Invalid value."]}) def test_invalid_field(self): data = self.minimal_data.copy() @@ -1775,23 +1773,20 @@ class CreateCommentTest( def test_thread_id_not_found(self): self.register_get_thread_error_response("test_thread", 404) - with self.assertRaises(ValidationError) as assertion: + with self.assertRaises(ThreadNotFoundError): create_comment(self.request, self.minimal_data) - self.assertEqual(assertion.exception.message_dict, {"thread_id": ["Invalid value."]}) def test_nonexistent_course(self): self.register_get_thread_response( make_minimal_cs_thread({"id": "test_thread", "course_id": "non/existent/course"}) ) - with self.assertRaises(ValidationError) as assertion: + with self.assertRaises(CourseNotFoundError): create_comment(self.request, self.minimal_data) - self.assertEqual(assertion.exception.message_dict, {"thread_id": ["Invalid value."]}) def test_not_enrolled(self): self.request.user = UserFactory.create() - with self.assertRaises(ValidationError) as assertion: + with self.assertRaises(CourseNotFoundError): create_comment(self.request, self.minimal_data) - self.assertEqual(assertion.exception.message_dict, {"thread_id": ["Invalid value."]}) def test_discussions_disabled(self): disabled_course = _discussion_disabled_course_for(self.user) @@ -1802,9 +1797,8 @@ class CreateCommentTest( "commentable_id": "test_topic", }) ) - with self.assertRaises(ValidationError) as assertion: + with self.assertRaises(DiscussionDisabledError): create_comment(self.request, self.minimal_data) - self.assertEqual(assertion.exception.message_dict, {"thread_id": ["Invalid value."]}) @ddt.data( *itertools.product( @@ -1845,12 +1839,8 @@ class CreateCommentTest( try: create_comment(self.request, data) self.assertFalse(expected_error) - except ValidationError as err: + except ThreadNotFoundError: self.assertTrue(expected_error) - self.assertEqual( - err.message_dict, - {"thread_id": ["Invalid value."]} - ) def test_invalid_field(self): data = self.minimal_data.copy() @@ -1974,24 +1964,24 @@ class UpdateThreadTest( def test_nonexistent_thread(self): self.register_get_thread_error_response("test_thread", 404) - with self.assertRaises(Http404): + with self.assertRaises(ThreadNotFoundError): update_thread(self.request, "test_thread", {}) def test_nonexistent_course(self): self.register_thread({"course_id": "non/existent/course"}) - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): update_thread(self.request, "test_thread", {}) def test_not_enrolled(self): self.register_thread() self.request.user = UserFactory.create() - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): update_thread(self.request, "test_thread", {}) def test_discussions_disabled(self): disabled_course = _discussion_disabled_course_for(self.user) self.register_thread(overrides={"course_id": unicode(disabled_course.id)}) - with self.assertRaises(Http404): + with self.assertRaises(DiscussionDisabledError): update_thread(self.request, "test_thread", {}) @ddt.data( @@ -2029,7 +2019,7 @@ class UpdateThreadTest( try: update_thread(self.request, "test_thread", {}) self.assertFalse(expected_error) - except Http404: + except ThreadNotFoundError: self.assertTrue(expected_error) @ddt.data( @@ -2357,23 +2347,23 @@ class UpdateCommentTest( def test_nonexistent_comment(self): self.register_get_comment_error_response("test_comment", 404) - with self.assertRaises(Http404): + with self.assertRaises(CommentNotFoundError): update_comment(self.request, "test_comment", {}) def test_nonexistent_course(self): self.register_comment(thread_overrides={"course_id": "non/existent/course"}) - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): update_comment(self.request, "test_comment", {}) def test_unenrolled(self): self.register_comment() self.request.user = UserFactory.create() - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): update_comment(self.request, "test_comment", {}) def test_discussions_disabled(self): self.register_comment(course=_discussion_disabled_course_for(self.user)) - with self.assertRaises(Http404): + with self.assertRaises(DiscussionDisabledError): update_comment(self.request, "test_comment", {}) @ddt.data( @@ -2416,7 +2406,7 @@ class UpdateCommentTest( try: update_comment(self.request, "test_comment", {}) self.assertFalse(expected_error) - except Http404: + except ThreadNotFoundError: self.assertTrue(expected_error) @ddt.data(*itertools.product( @@ -2690,24 +2680,24 @@ class DeleteThreadTest( def test_thread_id_not_found(self): self.register_get_thread_error_response("missing_thread", 404) - with self.assertRaises(Http404): + with self.assertRaises(ThreadNotFoundError): delete_thread(self.request, "missing_thread") def test_nonexistent_course(self): self.register_thread({"course_id": "non/existent/course"}) - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): delete_thread(self.request, self.thread_id) def test_not_enrolled(self): self.register_thread() self.request.user = UserFactory.create() - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): delete_thread(self.request, self.thread_id) def test_discussions_disabled(self): disabled_course = _discussion_disabled_course_for(self.user) self.register_thread(overrides={"course_id": unicode(disabled_course.id)}) - with self.assertRaises(Http404): + with self.assertRaises(DiscussionDisabledError): delete_thread(self.request, self.thread_id) @ddt.data( @@ -2770,7 +2760,7 @@ class DeleteThreadTest( try: delete_thread(self.request, self.thread_id) self.assertFalse(expected_error) - except Http404: + except ThreadNotFoundError: self.assertTrue(expected_error) @@ -2838,20 +2828,20 @@ class DeleteCommentTest( def test_comment_id_not_found(self): self.register_get_comment_error_response("missing_comment", 404) - with self.assertRaises(Http404): + with self.assertRaises(CommentNotFoundError): delete_comment(self.request, "missing_comment") def test_nonexistent_course(self): self.register_comment_and_thread( thread_overrides={"course_id": "non/existent/course"} ) - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): delete_comment(self.request, self.comment_id) def test_not_enrolled(self): self.register_comment_and_thread() self.request.user = UserFactory.create() - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): delete_comment(self.request, self.comment_id) def test_discussions_disabled(self): @@ -2860,7 +2850,7 @@ class DeleteCommentTest( thread_overrides={"course_id": unicode(disabled_course.id)}, overrides={"course_id": unicode(disabled_course.id)} ) - with self.assertRaises(Http404): + with self.assertRaises(DiscussionDisabledError): delete_comment(self.request, self.comment_id) @ddt.data( @@ -2928,7 +2918,7 @@ class DeleteCommentTest( try: delete_comment(self.request, self.comment_id) self.assertFalse(expected_error) - except Http404: + except ThreadNotFoundError: self.assertTrue(expected_error) @@ -3017,7 +3007,7 @@ class RetrieveThreadTest( def test_thread_id_not_found(self): self.register_get_thread_error_response("missing_thread", 404) - with self.assertRaises(Http404): + with self.assertRaises(ThreadNotFoundError): get_thread(self.request, "missing_thread") def test_nonauthor_enrolled_in_course(self): @@ -3062,7 +3052,7 @@ class RetrieveThreadTest( def test_not_enrolled_in_course(self): self.register_thread() self.request.user = UserFactory.create() - with self.assertRaises(Http404): + with self.assertRaises(CourseNotFoundError): get_thread(self.request, self.thread_id) @ddt.data( @@ -3108,5 +3098,5 @@ class RetrieveThreadTest( try: get_thread(self.request, self.thread_id) self.assertFalse(expected_error) - except Http404: + except ThreadNotFoundError: self.assertTrue(expected_error) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 2727566877..d05bcf8196 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -360,7 +360,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.assert_response_correct( response, 404, - {"developer_message": "Not found."} + {"developer_message": "Page not found (No results on this page)."} ) self.assert_last_query_params({ "user_id": [unicode(self.user.id)], @@ -884,7 +884,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.assert_response_correct( response, 404, - {"developer_message": "Not found."} + {"developer_message": "Thread not found."} ) def test_basic(self): @@ -976,7 +976,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.assert_response_correct( response, 404, - {"developer_message": "Not found."} + {"developer_message": "Page not found (No results on this page)."} ) self.assert_query_params_equal( httpretty.httpretty.latest_requests[-2], diff --git a/openedx/core/djangoapps/content/course_structures/api/v0/api.py b/openedx/core/djangoapps/content/course_structures/api/v0/api.py index 016b613782..1fa2e0b45f 100644 --- a/openedx/core/djangoapps/content/course_structures/api/v0/api.py +++ b/openedx/core/djangoapps/content/course_structures/api/v0/api.py @@ -6,8 +6,9 @@ of the tricky interactions between DRF and the code. Most of that information is available by accessing the course objects directly. """ from collections import OrderedDict +from openedx.core.lib.exceptions import CourseNotFoundError from .serializers import GradingPolicySerializer, CourseStructureSerializer -from .errors import CourseNotFoundError, CourseStructureNotAvailableError +from .errors import CourseStructureNotAvailableError from openedx.core.djangoapps.content.course_structures import models, tasks from util.cache import cache from xmodule.modulestore.django import modulestore diff --git a/openedx/core/djangoapps/content/course_structures/api/v0/errors.py b/openedx/core/djangoapps/content/course_structures/api/v0/errors.py index e0413a3abb..378a8ca975 100644 --- a/openedx/core/djangoapps/content/course_structures/api/v0/errors.py +++ b/openedx/core/djangoapps/content/course_structures/api/v0/errors.py @@ -1,11 +1,6 @@ """ Errors used by the Course Structure API. """ -class CourseNotFoundError(Exception): - """ The course was not found. """ - pass - - class CourseStructureNotAvailableError(Exception): """ The course structure still needs to be generated. """ pass diff --git a/openedx/core/lib/api/view_utils.py b/openedx/core/lib/api/view_utils.py index 14bb3d462a..8183e25a38 100644 --- a/openedx/core/lib/api/view_utils.py +++ b/openedx/core/lib/api/view_utils.py @@ -2,7 +2,7 @@ Utilities related to API views """ import functools -from django.core.exceptions import NON_FIELD_ERRORS, ValidationError +from django.core.exceptions import NON_FIELD_ERRORS, ValidationError, ObjectDoesNotExist from django.http import Http404 from django.utils.translation import ugettext as _ @@ -68,7 +68,7 @@ class DeveloperErrorViewMixin(object): if isinstance(exc, APIException): return self.make_error_response(exc.status_code, exc.detail) - elif isinstance(exc, Http404): + elif isinstance(exc, Http404) or isinstance(exc, ObjectDoesNotExist): return self.make_error_response(404, exc.message or "Not found.") elif isinstance(exc, ValidationError): return self.make_validation_error_response(exc) diff --git a/openedx/core/lib/exceptions.py b/openedx/core/lib/exceptions.py new file mode 100644 index 0000000000..d6e5714ac7 --- /dev/null +++ b/openedx/core/lib/exceptions.py @@ -0,0 +1,12 @@ +""" Common Purpose Errors""" +from django.core.exceptions import ObjectDoesNotExist + + +class CourseNotFoundError(ObjectDoesNotExist): + """ Course was not found. """ + pass + + +class PageNotFoundError(ObjectDoesNotExist): + """ Page was not found. Used for paginated endpoint. """ + pass