diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 4d3a84c6ee..35dc76e392 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -6,11 +6,12 @@ from datetime import datetime from collections import defaultdict from fs.errors import ResourceNotFoundError import logging - from path import Path as path import pytz + from django.http import Http404 from django.conf import settings +from django.core.urlresolvers import reverse from edxmako.shortcuts import render_to_string from xmodule.modulestore.django import modulestore @@ -27,8 +28,9 @@ from courseware.date_summary import ( VerifiedUpgradeDeadlineDate, ) from courseware.model_data import FieldDataCache -from courseware.module_render import get_module +from courseware.module_render import get_module, get_module_for_descriptor from lms.djangoapps.courseware.courseware_access_exception import CoursewareAccessException +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from student.models import CourseEnrollment import branding @@ -36,7 +38,6 @@ from opaque_keys.edx.keys import UsageKey from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers - log = logging.getLogger(__name__) @@ -72,12 +73,6 @@ def get_course_by_id(course_key, depth=0): raise Http404("Course not found: {}.".format(unicode(course_key))) -class UserNotEnrolled(Http404): - def __init__(self, course_key): - super(UserNotEnrolled, self).__init__() - self.course_key = course_key - - def get_course_with_access(user, action, course_key, depth=0, check_if_enrolled=False): """ Given a course_key, look up the corresponding course descriptor, @@ -132,10 +127,10 @@ def check_course_access(course, user, action, check_if_enrolled=False): if check_if_enrolled: # Verify that the user is either enrolled in the course or a staff - # member. If user is not enrolled, raise UserNotEnrolled exception - # that will be caught by middleware. + # member. If the user is not enrolled, raise a Redirect exception + # that will be handled by middleware. if not ((user.id and CourseEnrollment.is_enrolled(user, course.id)) or has_access(user, 'staff', course)): - raise UserNotEnrolled(course.id) + raise CourseAccessRedirect(reverse('about_course', args=[unicode(course.id)])) def find_file(filesystem, dirs, filename): @@ -470,3 +465,81 @@ def get_problems_in_section(section): problem_descriptors[unicode(component.location)] = component return problem_descriptors + + +def get_current_child(xmodule, min_depth=None, requested_child=None): + """ + Get the xmodule.position's display item of an xmodule that has a position and + children. If xmodule has no position or is out of bounds, return the first + child with children of min_depth. + + For example, if chapter_one has no position set, with two child sections, + section-A having no children and section-B having a discussion unit, + `get_current_child(chapter, min_depth=1)` will return section-B. + + Returns None only if there are no children at all. + """ + # TODO: convert this method to use the Course Blocks API + def _get_child(children): + """ + Returns either the first or last child based on the value of + the requested_child parameter. If requested_child is None, + returns the first child. + """ + if requested_child == 'first': + return children[0] + elif requested_child == 'last': + return children[-1] + else: + return children[0] + + def _get_default_child_module(child_modules): + """Returns the first child of xmodule, subject to min_depth.""" + if min_depth <= 0: + return _get_child(child_modules) + else: + content_children = [ + child for child in child_modules + if child.has_children_at_depth(min_depth - 1) and child.get_display_items() + ] + return _get_child(content_children) if content_children else None + + child = None + if hasattr(xmodule, 'position'): + children = xmodule.get_display_items() + if len(children) > 0: + if xmodule.position is not None and not requested_child: + pos = xmodule.position - 1 # position is 1-indexed + if 0 <= pos < len(children): + child = children[pos] + if min_depth > 0 and not child.has_children_at_depth(min_depth - 1): + child = None + if child is None: + child = _get_default_child_module(children) + + return child + + +def get_last_accessed_courseware(course, request, user): + """ + Returns a tuple containing the courseware module (URL, id) that the user last accessed, + or (None, None) if it cannot be found. + """ + # TODO: convert this method to use the Course Blocks API + field_data_cache = FieldDataCache.cache_for_descriptor_descendents( + course.id, request.user, course, depth=2 + ) + course_module = get_module_for_descriptor( + user, request, course, field_data_cache, course.id, course=course + ) + chapter_module = get_current_child(course_module) + if chapter_module is not None: + section_module = get_current_child(chapter_module) + if section_module is not None: + url = reverse('courseware_section', kwargs={ + 'course_id': unicode(course.id), + 'chapter': chapter_module.url_name, + 'section': section_module.url_name + }) + return (url, section_module.url_name) + return (None, None) diff --git a/lms/djangoapps/courseware/entrance_exams.py b/lms/djangoapps/courseware/entrance_exams.py index 7716d462df..10b8c1f359 100644 --- a/lms/djangoapps/courseware/entrance_exams.py +++ b/lms/djangoapps/courseware/entrance_exams.py @@ -3,12 +3,8 @@ This file contains all entrance exam related utils/logic. """ from courseware.access import has_access -from courseware.model_data import FieldDataCache, ScoresClient -from opaque_keys.edx.keys import UsageKey -from opaque_keys.edx.locator import BlockUsageLocator from student.models import EntranceExamConfiguration from util.milestones_helpers import get_required_content, is_entrance_exams_enabled -from util.module_utils import yield_dynamic_descriptor_descendants from xmodule.modulestore.django import modulestore diff --git a/lms/djangoapps/courseware/exceptions.py b/lms/djangoapps/courseware/exceptions.py index d7b38f2dec..f87e2e64e2 100644 --- a/lms/djangoapps/courseware/exceptions.py +++ b/lms/djangoapps/courseware/exceptions.py @@ -10,3 +10,10 @@ class Redirect(Exception): def __init__(self, url): super(Redirect, self).__init__() self.url = url + + +class CourseAccessRedirect(Redirect): + """ + Redirect raised when user does not have access to a course. + """ + pass diff --git a/lms/djangoapps/courseware/middleware.py b/lms/djangoapps/courseware/middleware.py index 6f28c7b6cc..0b973732a1 100644 --- a/lms/djangoapps/courseware/middleware.py +++ b/lms/djangoapps/courseware/middleware.py @@ -3,22 +3,17 @@ Middleware for the courseware app """ from django.shortcuts import redirect -from django.core.urlresolvers import reverse -from courseware.courses import UserNotEnrolled +from lms.djangoapps.courseware.exceptions import Redirect -class RedirectUnenrolledMiddleware(object): +class RedirectMiddleware(object): """ - Catch UserNotEnrolled errors thrown by `get_course_with_access` and redirect - users to the course about page + Catch Redirect exceptions and redirect the user to the expected URL. """ def process_exception(self, _request, exception): - if isinstance(exception, UserNotEnrolled): - course_key = exception.course_key - return redirect( - reverse( - 'courseware.views.views.course_about', - args=[course_key.to_deprecated_string()] - ) - ) + """ + Catch Redirect exceptions and redirect the user to the expected URL. + """ + if isinstance(exception, Redirect): + return redirect(exception.url) diff --git a/lms/djangoapps/courseware/tests/test_course_survey.py b/lms/djangoapps/courseware/tests/test_course_survey.py index 87e2357d64..6fa02c7d30 100644 --- a/lms/djangoapps/courseware/tests/test_course_survey.py +++ b/lms/djangoapps/courseware/tests/test_course_survey.py @@ -120,7 +120,7 @@ class SurveyViewsTests(LoginEnrollmentTestCase, SharedModuleStoreTestCase, XssTe def test_anonymous_user_visiting_course_with_survey(self): """ Verifies that anonymous user going to the courseware info with an unanswered survey is not - redirected to survery and info page renders without server error. + redirected to survey and info page renders without server error. """ self.logout() resp = self.client.get( diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index db961ed503..ba6f85a9ef 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -21,6 +21,7 @@ from courseware.courses import ( get_course_info_section, get_course_overview_with_access, get_course_with_access, + get_current_child, ) from courseware.module_render import get_module_for_descriptor from courseware.model_data import FieldDataCache @@ -160,6 +161,23 @@ class CoursesTest(ModuleStoreTestCase): "testing get_courses with filter_={}".format(filter_), ) + def test_get_current_child(self): + mock_xmodule = mock.MagicMock() + self.assertIsNone(get_current_child(mock_xmodule)) + + mock_xmodule.position = -1 + mock_xmodule.get_display_items.return_value = ['one', 'two', 'three'] + self.assertEqual(get_current_child(mock_xmodule), 'one') + + mock_xmodule.position = 2 + self.assertEqual(get_current_child(mock_xmodule), 'two') + self.assertEqual(get_current_child(mock_xmodule, requested_child='first'), 'one') + self.assertEqual(get_current_child(mock_xmodule, requested_child='last'), 'three') + + mock_xmodule.position = 3 + mock_xmodule.get_display_items.return_value = [] + self.assertIsNone(get_current_child(mock_xmodule)) + @attr(shard=1) class ModuleStoreBranchSettingTest(ModuleStoreTestCase): diff --git a/lms/djangoapps/courseware/tests/test_middleware.py b/lms/djangoapps/courseware/tests/test_middleware.py index 66e83822b8..385f45bab2 100644 --- a/lms/djangoapps/courseware/tests/test_middleware.py +++ b/lms/djangoapps/courseware/tests/test_middleware.py @@ -2,17 +2,16 @@ Tests for courseware middleware """ -from django.core.urlresolvers import reverse from django.test.client import RequestFactory from django.http import Http404 -from mock import patch from nose.plugins.attrib import attr -import courseware.courses as courses -from courseware.middleware import RedirectUnenrolledMiddleware from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory +from lms.djangoapps.courseware.exceptions import Redirect +from lms.djangoapps.courseware.middleware import RedirectMiddleware + @attr(shard=1) class CoursewareMiddlewareTestCase(SharedModuleStoreTestCase): @@ -26,32 +25,24 @@ class CoursewareMiddlewareTestCase(SharedModuleStoreTestCase): def setUp(self): super(CoursewareMiddlewareTestCase, self).setUp() - def check_user_not_enrolled_redirect(self): - """A UserNotEnrolled exception should trigger a redirect""" - request = RequestFactory().get("dummy_url") - response = RedirectUnenrolledMiddleware().process_exception( - request, courses.UserNotEnrolled(self.course.id) - ) - self.assertEqual(response.status_code, 302) - # make sure we redirect to the course about page - expected_url = reverse( - "about_course", args=[self.course.id.to_deprecated_string()] - ) - - target_url = response._headers['location'][1] - self.assertTrue(target_url.endswith(expected_url)) - - def test_user_not_enrolled_redirect(self): - self.check_user_not_enrolled_redirect() - - @patch.dict("django.conf.settings.FEATURES", {"ENABLE_MKTG_SITE": True}) - def test_user_not_enrolled_redirect_mktg(self): - self.check_user_not_enrolled_redirect() - def test_process_404(self): """A 404 should not trigger anything""" request = RequestFactory().get("dummy_url") - response = RedirectUnenrolledMiddleware().process_exception( + response = RedirectMiddleware().process_exception( request, Http404() ) self.assertIsNone(response) + + def test_redirect_exceptions(self): + """ + Unit tests for handling of Redirect exceptions. + """ + request = RequestFactory().get("dummy_url") + test_url = '/test_url' + exception = Redirect(test_url) + response = RedirectMiddleware().process_exception( + request, exception + ) + self.assertEqual(response.status_code, 302) + target_url = response._headers['location'][1] + self.assertTrue(target_url.endswith(test_url)) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 0c0ed4eefd..d6b7bc1e51 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -432,46 +432,6 @@ class ViewsTestCase(ModuleStoreTestCase): self.assertEqual(response.status_code, 302) self.assertRedirects(response, '/courses/{}/about'.format(unicode(self.course_key))) - def test_courseware_redirection(self): - """ - Tests that a global staff member is redirected to the staff enrollment page. - - Un-enrolled Staff user should be redirected to the staff enrollment page accessing courseware, - user chooses to enroll in the course. User is enrolled and redirected to the requested url. - - Scenario: - 1. Un-enrolled staff tries to access any course vertical (courseware url). - 2. User is redirected to the staff enrollment page. - 3. User chooses to enroll in the course. - 4. User is enrolled in the course and redirected to the requested courseware url. - """ - self._create_global_staff_user() - courseware_url, enroll_url = self._create_url_for_enroll_staff() - - # Accessing the courseware url in which not enrolled & redirected to staff enrollment page - response = self.client.get(courseware_url, follow=True) - self.assertEqual(response.status_code, 200) - self.assertIn(302, response.redirect_chain[0]) - self.assertEqual(len(response.redirect_chain), 1) - self.assertRedirects(response, enroll_url) - - # Accessing the enroll staff url and verify the correct url - response = self.client.get(enroll_url) - self.assertEqual(response.status_code, 200) - response_content = response.content - self.assertIn('Enroll', response_content) - self.assertIn("dont_enroll", response_content) - - # Post the valid data to enroll the staff in the course - response = self.client.post(enroll_url, data={'enroll': "Enroll"}, follow=True) - self.assertEqual(response.status_code, 200) - self.assertIn(302, response.redirect_chain[0]) - self.assertEqual(len(response.redirect_chain), 1) - self.assertRedirects(response, courseware_url) - - # Verify staff has been enrolled to the given course - self.assertTrue(CourseEnrollment.is_enrolled(self.global_staff, self.course.id)) - @unittest.skipUnless(settings.FEATURES.get('ENABLE_SHOPPING_CART'), "Shopping Cart not enabled in settings") @patch.dict(settings.FEATURES, {'ENABLE_PAID_COURSE_REGISTRATION': True}) def test_course_about_in_cart(self): @@ -557,23 +517,6 @@ class ViewsTestCase(ModuleStoreTestCase): mock_user.is_authenticated.return_value = False self.assertEqual(views.user_groups(mock_user), []) - def test_get_current_child(self): - mock_xmodule = MagicMock() - self.assertIsNone(views.get_current_child(mock_xmodule)) - - mock_xmodule.position = -1 - mock_xmodule.get_display_items.return_value = ['one', 'two', 'three'] - self.assertEqual(views.get_current_child(mock_xmodule), 'one') - - mock_xmodule.position = 2 - self.assertEqual(views.get_current_child(mock_xmodule), 'two') - self.assertEqual(views.get_current_child(mock_xmodule, requested_child='first'), 'one') - self.assertEqual(views.get_current_child(mock_xmodule, requested_child='last'), 'three') - - mock_xmodule.position = 3 - mock_xmodule.get_display_items.return_value = [] - self.assertIsNone(views.get_current_child(mock_xmodule)) - def test_get_redirect_url(self): self.assertIn( 'activate_block_id', diff --git a/lms/djangoapps/courseware/url_helpers.py b/lms/djangoapps/courseware/url_helpers.py index 9acd931a40..b4da0a59de 100644 --- a/lms/djangoapps/courseware/url_helpers.py +++ b/lms/djangoapps/courseware/url_helpers.py @@ -2,9 +2,11 @@ Module to define url helpers functions """ from urllib import urlencode + +from django.core.urlresolvers import reverse + from xmodule.modulestore.search import path_to_location, navigation_index from xmodule.modulestore.django import modulestore -from django.core.urlresolvers import reverse def get_redirect_url(course_key, usage_key): @@ -48,17 +50,3 @@ def get_redirect_url(course_key, usage_key): ) redirect_url += "?{}".format(urlencode({'activate_block_id': unicode(final_target_id)})) return redirect_url - - -def get_redirect_url_for_global_staff(course_key, _next): - """ - Returns the redirect url for staff enrollment - - Args: - course_key(str): Course key string - _next(str): Redirect url of course component - """ - redirect_url = ("{url}?next={redirect}".format( - url=reverse('enroll_staff', args=[unicode(course_key)]), - redirect=_next)) - return redirect_url diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index c3f3784cb0..8bcc292d85 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -16,7 +16,6 @@ from django.views.decorators.csrf import ensure_csrf_cookie from django.views.generic import View from django.shortcuts import redirect -from courseware.url_helpers import get_redirect_url_for_global_staff from edxmako.shortcuts import render_to_response, render_to_string import logging @@ -25,6 +24,7 @@ log = logging.getLogger("edx.courseware.views.index") import urllib import waffle +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from lms.djangoapps.gating.api import get_entrance_exam_score_ratio, get_entrance_exam_usage_key from lms.djangoapps.grades.new.course_grade_factory import CourseGradeFactory from opaque_keys.edx.keys import CourseKey @@ -35,29 +35,25 @@ from openedx.core.djangoapps.monitoring_utils import set_custom_metrics_for_cour from openedx.features.enterprise_support.api import data_sharing_consent_required from request_cache.middleware import RequestCache from shoppingcart.models import CourseRegistrationCode -from student.models import CourseEnrollment from student.views import is_course_blocked -from student.roles import GlobalStaff from util.views import ensure_valid_course_key from xmodule.modulestore.django import modulestore from xmodule.x_module import STUDENT_VIEW -from survey.utils import must_answer_survey from web_fragments.fragment import Fragment from ..access import has_access, _adjust_start_date_for_beta_testers from ..access_utils import in_preview_mode -from ..courses import get_studio_url, get_course_with_access +from ..courses import get_current_child, get_studio_url, get_course_with_access from ..entrance_exams import ( course_has_entrance_exam, get_entrance_exam_content, user_has_passed_entrance_exam, user_can_skip_entrance_exam, ) -from ..exceptions import Redirect from ..masquerade import setup_masquerade from ..model_data import FieldDataCache from ..module_render import toc_for_course, get_module_for_descriptor -from .views import get_current_child, registered_for_course +from .views import CourseTabView, check_access_to_course TEMPLATE_IMPORTS = {'urllib': urllib} @@ -100,25 +96,23 @@ class CoursewareIndex(View): self.section_url_name = section self.position = position self.chapter, self.section = None, None + self.course = None self.url = request.path try: set_custom_metrics_for_course_key(self.course_key) self._clean_position() with modulestore().bulk_operations(self.course_key): - self.course = get_course_with_access(request.user, 'load', self.course_key, depth=CONTENT_DEPTH) + self.course = get_course_with_access( + request.user, 'load', self.course_key, + depth=CONTENT_DEPTH, + check_if_enrolled=True, + ) self.is_staff = has_access(request.user, 'staff', self.course) self._setup_masquerade_for_effective_user() return self._get(request) - except Redirect as redirect_error: - return redirect(redirect_error.url) - except UnicodeEncodeError: - raise Http404("URL contains Unicode characters") - except Http404: - # let it propagate - raise - except Exception: # pylint: disable=broad-except - return self._handle_unexpected_error() + except Exception as exception: # pylint: disable=broad-except + return CourseTabView.handle_exceptions(request, self.course, exception) def _setup_masquerade_for_effective_user(self): """ @@ -139,7 +133,8 @@ class CoursewareIndex(View): """ Render the index page. """ - self._redirect_if_needed_to_access_course() + check_access_to_course(request, self.course) + self._redirect_if_needed_to_pay_for_course() self._prefetch_and_bind_course(request) if self.course.has_children_at_depth(CONTENT_DEPTH): @@ -165,7 +160,7 @@ class CoursewareIndex(View): self.chapter.url_name != self.original_chapter_url_name or (self.original_section_url_name and self.section.url_name != self.original_section_url_name) ): - raise Redirect( + raise CourseAccessRedirect( reverse( 'courseware_section', kwargs={ @@ -186,15 +181,6 @@ class CoursewareIndex(View): except ValueError: raise Http404(u"Position {} is not an integer!".format(self.position)) - def _redirect_if_needed_to_access_course(self): - """ - Verifies that the user can enter the course. - """ - self._redirect_if_needed_to_pay_for_course() - self._redirect_if_needed_to_register() - self._redirect_if_needed_for_prereqs() - self._redirect_if_needed_for_course_survey() - def _redirect_if_needed_to_pay_for_course(self): """ Redirect to dashboard if the course is blocked due to non-payment. @@ -213,47 +199,7 @@ class CoursewareIndex(View): self.real_user, unicode(self.course_key), ) - raise Redirect(reverse('dashboard')) - - def _redirect_if_needed_to_register(self): - """ - Verify that the user is registered in the course. - """ - if not registered_for_course(self.course, self.effective_user): - log.debug( - u'User %s tried to view course %s but is not enrolled', - self.effective_user, - unicode(self.course.id) - ) - user_is_global_staff = GlobalStaff().has_user(self.effective_user) - user_is_enrolled = CourseEnrollment.is_enrolled(self.effective_user, self.course_key) - if user_is_global_staff and not user_is_enrolled: - redirect_url = get_redirect_url_for_global_staff(self.course_key, _next=self.url) - raise Redirect(redirect_url) - raise Redirect(reverse('about_course', args=[unicode(self.course.id)])) - - def _redirect_if_needed_for_prereqs(self): - """ - See if all pre-requisites (as per the milestones app feature) have been - fulfilled. Note that if the pre-requisite feature flag has been turned off - (default) then this check will always pass. - """ - if not has_access(self.effective_user, 'view_courseware_with_prerequisites', self.course): - # Prerequisites have not been fulfilled. - # Therefore redirect to the Dashboard. - log.info( - u'User %d tried to view course %s ' - u'without fulfilling prerequisites', - self.effective_user.id, unicode(self.course.id)) - raise Redirect(reverse('dashboard')) - - def _redirect_if_needed_for_course_survey(self): - """ - Check to see if there is a required survey that must be taken before - the user can access the course. - """ - if must_answer_survey(self.course, self.effective_user): - raise Redirect(reverse('course_survey', args=[unicode(self.course.id)])) + raise CourseAccessRedirect(reverse('dashboard')) def _reset_section_to_exam_if_required(self): """ @@ -487,33 +433,6 @@ class CoursewareIndex(View): section_context['specific_masquerade'] = self._is_masquerading_as_specific_student() return section_context - def _handle_unexpected_error(self): - """ - Handle unexpected exceptions raised by View. - """ - # In production, don't want to let a 500 out for any reason - if settings.DEBUG: - raise - log.exception( - u"Error in index view: user=%s, effective_user=%s, course=%s, chapter=%s section=%s position=%s", - self.real_user, - self.effective_user, - unicode(self.course_key), - self.chapter_url_name, - self.section_url_name, - self.position, - ) - try: - return render_to_response('courseware/courseware-error.html', { - 'staff_access': self.is_staff, - 'course': self.course - }) - except: - # Let the exception propagate, relying on global config to - # at least return a nice error message - log.exception("Error while rendering courseware-error page") - raise - def render_accordion(request, course, table_of_contents): """ diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 25dc1a1d07..009499e661 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -38,12 +38,13 @@ from markupsafe import escape from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey from rest_framework import status -from lms.djangoapps.instructor.views.api import require_global_staff +from lms.djangoapps.ccx.custom_exception import CCXLocatorValidationException from lms.djangoapps.ccx.utils import prep_course_for_grading +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect, Redirect from lms.djangoapps.grades.new.course_grade_factory import CourseGradeFactory from lms.djangoapps.instructor.enrollment import uses_shib +from lms.djangoapps.instructor.views.api import require_global_staff from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification -from lms.djangoapps.ccx.custom_exception import CCXLocatorValidationException import shoppingcart import survey.utils @@ -62,19 +63,19 @@ from courseware.courses import ( get_courses, get_course, get_course_by_id, - get_permission_for_course_about, - get_studio_url, get_course_overview_with_access, get_course_with_access, + get_last_accessed_courseware, + get_permission_for_course_about, + get_studio_url, sort_by_announcement, sort_by_start_date, - UserNotEnrolled ) from courseware.date_summary import VerifiedUpgradeDeadlineDate from courseware.masquerade import setup_masquerade from courseware.model_data import FieldDataCache from courseware.models import StudentModule, BaseStudentModuleHistory -from courseware.url_helpers import get_redirect_url, get_redirect_url_for_global_staff +from courseware.url_helpers import get_redirect_url from courseware.user_state_client import DjangoXBlockUserStateClient from edxmako.shortcuts import render_to_response, render_to_string, marketing_link from openedx.core.djangoapps.catalog.utils import get_programs, get_programs_with_type @@ -90,9 +91,11 @@ from openedx.core.djangoapps.site_configuration import helpers as configuration_ from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration from openedx.core.djangoapps.monitoring_utils import set_custom_metrics_for_course_key from openedx.features.enterprise_support.api import data_sharing_consent_required +from shoppingcart.models import CourseRegistrationCode from shoppingcart.utils import is_shopping_cart_enabled from student.models import UserTestGroup, CourseEnrollment from student.roles import GlobalStaff +from survey.utils import must_answer_survey from util.cache import cache, cache_if_anonymous from util.date_utils import strftime_localized from util.db import outer_atomic @@ -103,11 +106,11 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from xmodule.tabs import CourseTabList from xmodule.x_module import STUDENT_VIEW +from web_fragments.fragment import Fragment + from ..entrance_exams import user_can_skip_entrance_exam from ..module_render import get_module_for_descriptor, get_module, get_module_by_usage_id -from web_fragments.fragment import Fragment - log = logging.getLogger("edx.courseware") @@ -179,58 +182,6 @@ def courses(request): ) -def get_current_child(xmodule, min_depth=None, requested_child=None): - """ - Get the xmodule.position's display item of an xmodule that has a position and - children. If xmodule has no position or is out of bounds, return the first - child with children of min_depth. - - For example, if chapter_one has no position set, with two child sections, - section-A having no children and section-B having a discussion unit, - `get_current_child(chapter, min_depth=1)` will return section-B. - - Returns None only if there are no children at all. - """ - def _get_child(children): - """ - Returns either the first or last child based on the value of - the requested_child parameter. If requested_child is None, - returns the first child. - """ - if requested_child == 'first': - return children[0] - elif requested_child == 'last': - return children[-1] - else: - return children[0] - - def _get_default_child_module(child_modules): - """Returns the first child of xmodule, subject to min_depth.""" - if min_depth <= 0: - return _get_child(child_modules) - else: - content_children = [ - child for child in child_modules - if child.has_children_at_depth(min_depth - 1) and child.get_display_items() - ] - return _get_child(content_children) if content_children else None - - child = None - if hasattr(xmodule, 'position'): - children = xmodule.get_display_items() - if len(children) > 0: - if xmodule.position is not None and not requested_child: - pos = xmodule.position - 1 # position is 1-indexed - if 0 <= pos < len(children): - child = children[pos] - if min_depth > 0 and not child.has_children_at_depth(min_depth - 1): - child = None - if child is None: - child = _get_default_child_module(children) - - return child - - @ensure_csrf_cookie @ensure_valid_course_key def jump_to_id(request, course_id, module_id): @@ -275,11 +226,6 @@ def jump_to(_request, course_id, location): raise Http404(u"Invalid course_key or usage_key") try: redirect_url = get_redirect_url(course_key, usage_key) - user = _request.user - user_is_global_staff = GlobalStaff().has_user(user) - user_is_enrolled = CourseEnrollment.is_enrolled(user, course_key) - if user_is_global_staff and not user_is_enrolled: - redirect_url = get_redirect_url_for_global_staff(course_key, _next=redirect_url) except ItemNotFoundError: raise Http404(u"No data at this location: {0}".format(usage_key)) except NoPathToItem: @@ -330,22 +276,20 @@ def course_info(request, course_id): # to access CCX redirect him to dashboard. return redirect(reverse('dashboard')) + # Redirect the user if they are not yet allowed to view this course + check_access_to_course(request, course) + # If the user needs to take an entrance exam to access this course, then we'll need # to send them to that specific course module before allowing them into other areas if not user_can_skip_entrance_exam(user, course): return redirect(reverse('courseware', args=[unicode(course.id)])) - # check to see if there is a required survey that must be taken before - # the user can access the course. - if request.user.is_authenticated() and survey.utils.must_answer_survey(course, user): - return redirect(reverse('course_survey', args=[unicode(course.id)])) - + # If the user is coming from the dashboard and bypass_home setting is set, + # redirect them straight to the courseware page. is_from_dashboard = reverse('dashboard') in request.META.get('HTTP_REFERER', []) if course.bypass_home and is_from_dashboard: return redirect(reverse('courseware', args=[course_id])) - studio_url = get_studio_url(course, 'course_info') - # link to where the student should go to enroll in the course: # about page if there is not marketing site, SITE_NAME if there is url_to_enroll = reverse(course_about, args=[course_id]) @@ -380,7 +324,7 @@ def course_info(request, course_id): 'staff_access': staff_access, 'masquerade': masquerade, 'supports_preview_menu': True, - 'studio_url': studio_url, + 'studio_url': get_studio_url(course, 'course_info'), 'show_enroll_banner': show_enroll_banner, 'url_to_enroll': url_to_enroll, 'upgrade_link': upgrade_link, @@ -422,30 +366,6 @@ def course_info(request, course_id): return response -def get_last_accessed_courseware(course, request, user): - """ - Returns a tuple containing the courseware module (URL, id) that the user last accessed, - or (None, None) if it cannot be found. - """ - field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - course.id, request.user, course, depth=2 - ) - course_module = get_module_for_descriptor( - user, request, course, field_data_cache, course.id, course=course - ) - chapter_module = get_current_child(course_module) - if chapter_module is not None: - section_module = get_current_child(chapter_module) - if section_module is not None: - url = reverse('courseware_section', kwargs={ - 'course_id': unicode(course.id), - 'chapter': chapter_module.url_name, - 'section': section_module.url_name - }) - return (url, section_module.url_name) - return (None, None) - - class StaticCourseTabView(EdxFragmentView): """ View that displays a static course tab with a given name. @@ -497,10 +417,51 @@ class CourseTabView(EdxFragmentView): course_key = CourseKey.from_string(course_id) with modulestore().bulk_operations(course_key): course = get_course_with_access(request.user, 'load', course_key) - tab = CourseTabList.get_tab_by_type(course.tabs, tab_type) - page_context = self.create_page_context(request, course=course, tab=tab, **kwargs) - set_custom_metrics_for_course_key(course_key) - return super(CourseTabView, self).get(request, course=course, page_context=page_context, **kwargs) + try: + # Verify that the user has access to the course + check_access_to_course(request, course) + + # Render the page + tab = CourseTabList.get_tab_by_type(course.tabs, tab_type) + page_context = self.create_page_context(request, course=course, tab=tab, **kwargs) + set_custom_metrics_for_course_key(course_key) + return super(CourseTabView, self).get(request, course=course, page_context=page_context, **kwargs) + except Exception as exception: # pylint: disable=broad-except + return CourseTabView.handle_exceptions(request, course, exception) + + @staticmethod + def handle_exceptions(request, course, exception): + """ + Handle exceptions raised when rendering a view. + """ + if isinstance(exception, Redirect) or isinstance(exception, Http404): + raise + if isinstance(exception, UnicodeEncodeError): + raise Http404("URL contains Unicode characters") + if settings.DEBUG: + raise + user = request.user + log.exception( + u"Error in %s: user=%s, effective_user=%s, course=%s", + request.path, + getattr(user, 'real_user', user), + user, + unicode(course.id), + ) + try: + return render_to_response( + 'courseware/courseware-error.html', + { + 'staff_access': has_access(user, 'staff', course), + 'course': course, + }, + status=500, + ) + except: + # Let the exception propagate, relying on global config to + # at least return a nice error message + log.exception("Error while rendering courseware-error page") + raise def create_page_context(self, request, course=None, tab=None, **kwargs): """ @@ -834,11 +795,6 @@ def _progress(request, course_key, student_id): course = get_course_with_access(request.user, 'load', course_key) - # check to see if there is a required survey that must be taken before - # the user can access the course. - if survey.utils.must_answer_survey(course, request.user): - return redirect(reverse('course_survey', args=[unicode(course.id)])) - staff_access = bool(has_access(request.user, 'staff', course)) masquerade = None @@ -860,6 +816,12 @@ def _progress(request, course_key, student_id): except User.DoesNotExist: raise Http404 + # NOTE: To make sure impersonation by instructor works, use + # student instead of request.user in the rest of the function. + + # Redirect the user if they are not yet allowed to view this course + check_access_to_course(request, course) + # The pre-fetching of groups is done to make auth checks not require an # additional DB lookup (this kills the Progress page in particular). student = User.objects.prefetch_related("groups").get(id=student.id) @@ -1394,7 +1356,7 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True): # verify the user has access to the course, including enrollment check try: course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=check_if_enrolled) - except UserNotEnrolled: + except CourseAccessRedirect: raise Http404("Course not found.") # get the block, which verifies whether the user has access to the block. @@ -1651,3 +1613,20 @@ def get_financial_aid_courses(user): ) return financial_aid_courses + + +def check_access_to_course(request, course): + """ + Raises Redirect exceptions if the user does not have course access. + """ + # Redirect to the dashboard if not all prerequisites have been met + if not has_access(request.user, 'view_courseware_with_prerequisites', course): + log.info( + u'User %d tried to view course %s ' + u'without fulfilling prerequisites', + request.user.id, unicode(course.id)) + raise CourseAccessRedirect(reverse('dashboard')) + + # Redirect if the user must answer a survey before entering the course. + if must_answer_survey(course, request.user): + raise CourseAccessRedirect(reverse('course_survey', args=[unicode(course.id)])) diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 534a8c3e49..f29cc20170 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -34,12 +34,12 @@ from xmodule.modulestore.tests.django_utils import ( ) from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory, ItemFactory -from courseware.courses import UserNotEnrolled from nose.tools import assert_true from mock import patch, Mock, ANY, call from openedx.core.djangoapps.course_groups.models import CourseUserGroup +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from lms.djangoapps.teams.tests.factories import CourseTeamFactory log = logging.getLogger(__name__) @@ -1574,7 +1574,7 @@ class EnrollmentTestCase(ForumsEnableMixin, ModuleStoreTestCase): mock_request.side_effect = make_mock_request_impl(course=self.course, text='dummy') request = RequestFactory().get('dummy_url') request.user = self.student - with self.assertRaises(UserNotEnrolled): + with self.assertRaises(CourseAccessRedirect): views.forum_form_discussion(request, course_id=self.course.id.to_deprecated_string()) diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 27299d1f14..587eb66a2b 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -14,6 +14,7 @@ from openedx.core.djangoapps.user_api.accounts.views import AccountViewSet from rest_framework.exceptions import PermissionDenied +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseKey from courseware.courses import get_course_with_access @@ -80,6 +81,11 @@ def _get_course(course_key, user): try: course = get_course_with_access(user, 'load', course_key, check_if_enrolled=True) except Http404: + # Convert 404s into CourseNotFoundErrors. + raise CourseNotFoundError("Course not found.") + except CourseAccessRedirect: + # Raise course not found if the user cannot access the course + # since it doesn't make sense to redirect an API. raise CourseNotFoundError("Course not found.") if not any([tab.type == 'discussion' and tab.is_enabled(course, user) for tab in course.tabs]): raise DiscussionDisabledError("Discussion is disabled for the course.") diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index 31cc366c26..1bc658553f 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -17,6 +17,8 @@ from opaque_keys.edx.keys import CourseKey from courseware.access import has_access from util.file import store_uploaded_file from courseware.courses import get_course_with_access, get_course_overview_with_access, get_course_by_id +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect + import django_comment_client.settings as cc_settings from django_comment_common.signals import ( thread_created, @@ -778,6 +780,9 @@ def users(request, course_id): except Http404: # course didn't exist, or requesting user does not have access to it. return JsonError(status=404) + except CourseAccessRedirect: + # user does not have access to the course. + return JsonError(status=404) try: username = request.GET['username'] diff --git a/lms/djangoapps/edxnotes/helpers.py b/lms/djangoapps/edxnotes/helpers.py index 2184844e88..62598d9a30 100644 --- a/lms/djangoapps/edxnotes/helpers.py +++ b/lms/djangoapps/edxnotes/helpers.py @@ -22,8 +22,8 @@ from provider.oauth2.models import Client from edxnotes.exceptions import EdxNotesParseError, EdxNotesServiceUnavailable from edxnotes.plugins import EdxNotesTab -from courseware.views.views import get_current_child from courseware.access import has_access +from courseware.courses import get_current_child from openedx.core.lib.token_utils import JwtBuilder from student.models import anonymous_id_for_user from util.date_utils import get_default_time_display diff --git a/lms/djangoapps/grades/api/views.py b/lms/djangoapps/grades/api/views.py index c211204149..9fcd57b4c3 100644 --- a/lms/djangoapps/grades/api/views.py +++ b/lms/djangoapps/grades/api/views.py @@ -13,9 +13,9 @@ from rest_framework.response import Response from courseware.access import has_access from lms.djangoapps.ccx.utils import prep_course_for_grading from lms.djangoapps.courseware import courses +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from lms.djangoapps.grades.api.serializers import GradingPolicySerializer from lms.djangoapps.grades.new.course_grade_factory import CourseGradeFactory -from openedx.core.lib.api.authentication import OAuth2AuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from student.roles import CourseStaffRole @@ -47,15 +47,17 @@ class GradeViewMixin(DeveloperErrorViewMixin): user, access_action, course_key, - check_if_enrolled=True + check_if_enrolled=True, ) except Http404: log.info('Course with ID "%s" not found', course_key_string) - return self.make_error_response( - status_code=status.HTTP_404_NOT_FOUND, - developer_message='The user, the course or both do not exist.', - error_code='user_or_course_does_not_exist' - ) + except CourseAccessRedirect: + log.info('User %s does not have access to course with ID "%s"', user.username, course_key_string) + return self.make_error_response( + status_code=status.HTTP_404_NOT_FOUND, + developer_message='The user, the course or both do not exist.', + error_code='user_or_course_does_not_exist', + ) def _get_effective_user(self, request, course): """ diff --git a/lms/djangoapps/mobile_api/decorators.py b/lms/djangoapps/mobile_api/decorators.py index 58cb99d73e..abc04c6fcd 100644 --- a/lms/djangoapps/mobile_api/decorators.py +++ b/lms/djangoapps/mobile_api/decorators.py @@ -5,8 +5,11 @@ import functools from rest_framework import status from rest_framework.response import Response +from django.http import Http404 + from lms.djangoapps.courseware.courses import get_course_with_access from lms.djangoapps.courseware.courseware_access_exception import CoursewareAccessException +from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from opaque_keys.edx.keys import CourseKey from xmodule.modulestore.django import modulestore @@ -39,6 +42,9 @@ def mobile_course_access(depth=0): ) except CoursewareAccessException as error: return Response(data=error.to_json(), status=status.HTTP_404_NOT_FOUND) + except CourseAccessRedirect as error: + # Raise a 404 if the user does not have course access + raise Http404 return func(self, request, course=course, *args, **kwargs) return _wrapper diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index b1aee073ba..a447645266 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -13,10 +13,10 @@ from opaque_keys.edx.keys import UsageKey from opaque_keys import InvalidKeyError from courseware.access import is_mobile_available_for_user +from courseware.courses import get_current_child from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor from courseware.views.index import save_positions_recursively_up -from courseware.views.views import get_current_child from student.models import CourseEnrollment, User from xblock.fields import Scope diff --git a/lms/djangoapps/survey/models.py b/lms/djangoapps/survey/models.py index 28b4d91df2..bf4dd451b4 100644 --- a/lms/djangoapps/survey/models.py +++ b/lms/djangoapps/survey/models.py @@ -177,6 +177,8 @@ class SurveyAnswer(TimeStampedModel): Returns whether a user has any answers for a given SurveyForm for a course This can be used to determine if a user has taken a CourseSurvey. """ + if user.is_anonymous(): + return False return SurveyAnswer.objects.filter(form=form, user=user).exists() @classmethod diff --git a/lms/djangoapps/survey/tests/test_views.py b/lms/djangoapps/survey/tests/test_views.py index 3924ae81d5..db52d06a3a 100644 --- a/lms/djangoapps/survey/tests/test_views.py +++ b/lms/djangoapps/survey/tests/test_views.py @@ -83,7 +83,7 @@ class SurveyViewsTests(ModuleStoreTestCase): # is the SurveyForm html present in the HTML response? self.assertIn(self.test_form, resp.content) - def test_unautneticated_survey_postback(self): + def test_unauthenticated_survey_postback(self): """ Asserts that an anonymous user cannot answer a survey """ diff --git a/lms/djangoapps/survey/utils.py b/lms/djangoapps/survey/utils.py index b71f8d7960..39c03e58f9 100644 --- a/lms/djangoapps/survey/utils.py +++ b/lms/djangoapps/survey/utils.py @@ -25,6 +25,10 @@ def must_answer_survey(course_descriptor, user): if not is_survey_required_for_course(course_descriptor): return False + # anonymous users do not need to answer the survey + if user.is_anonymous(): + return False + # this will throw exception if not found, but a non existing survey name will # be trapped in the above is_survey_required_for_course() method survey = SurveyForm.get(course_descriptor.course_survey_name) diff --git a/lms/envs/common.py b/lms/envs/common.py index 8a9c2f4c78..11fc81db2e 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1180,7 +1180,7 @@ MIDDLEWARE_CLASSES = ( 'django.middleware.clickjacking.XFrameOptionsMiddleware', # to redirected unenrolled students to the course info page - 'courseware.middleware.RedirectUnenrolledMiddleware', + 'courseware.middleware.RedirectMiddleware', 'course_wiki.middleware.WikiAccessMiddleware', diff --git a/openedx/features/course_experience/templates/course_experience/course-home-fragment.html b/openedx/features/course_experience/templates/course_experience/course-home-fragment.html index 2d14b7be1a..bf85f20732 100644 --- a/openedx/features/course_experience/templates/course_experience/course-home-fragment.html +++ b/openedx/features/course_experience/templates/course_experience/course-home-fragment.html @@ -20,7 +20,11 @@ from openedx.core.djangolib.markup import HTML