diff --git a/common/djangoapps/enrollment/tests/test_views.py b/common/djangoapps/enrollment/tests/test_views.py index 914075ee1f..bc2d29b2bd 100644 --- a/common/djangoapps/enrollment/tests/test_views.py +++ b/common/djangoapps/enrollment/tests/test_views.py @@ -150,10 +150,9 @@ class EnrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase, APITestCase): throttle = EnrollmentUserThrottle() self.rate_limit, rate_duration = throttle.parse_rate(throttle.rate) - self.course = CourseFactory.create() - # Load a CourseOverview. This initial load should result in a cache - # miss; the modulestore is queried and course metadata is cached. - __ = CourseOverview.get_from_id(self.course.id) + # Pass emit_signals when creating the course so it would be cached + # as a CourseOverview. + self.course = CourseFactory.create(emit_signals=True) self.user = UserFactory.create( username=self.USERNAME, @@ -336,7 +335,7 @@ class EnrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase, APITestCase): requesting user. """ # Create another course, and enroll self.user in both courses. - other_course = CourseFactory.create() + other_course = CourseFactory.create(emit_signals=True) for course in self.course, other_course: CourseModeFactory.create( course_id=unicode(course.id), @@ -345,7 +344,7 @@ class EnrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase, APITestCase): ) self.assert_enrollment_status( course_id=unicode(course.id), - max_mongo_calls=1, + max_mongo_calls=0, ) # Verify the user himself can see both of his enrollments. self._assert_enrollments_visible_in_list([self.course, other_course]) diff --git a/lms/djangoapps/branding/__init__.py b/lms/djangoapps/branding/__init__.py index a7f8c512c1..3f2e64a09d 100644 --- a/lms/djangoapps/branding/__init__.py +++ b/lms/djangoapps/branding/__init__.py @@ -14,27 +14,22 @@ from django.conf import settings from opaque_keys.edx.locations import SlashSeparatedCourseKey from microsite_configuration import microsite from django.contrib.staticfiles.storage import staticfiles_storage +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview def get_visible_courses(): """ Return the set of CourseDescriptors that should be visible in this branded instance """ - filtered_by_org = microsite.get_value('course_org_filter') - - _courses = modulestore().get_courses(org=filtered_by_org) - - courses = [c for c in _courses - if isinstance(c, CourseDescriptor)] + courses = CourseOverview.get_all_courses(org=filtered_by_org) courses = sorted(courses, key=lambda course: course.number) - subdomain = microsite.get_value('subdomain', 'default') - # See if we have filtered course listings in this domain filtered_visible_ids = None # this is legacy format which is outside of the microsite feature -- also handle dev case, which should not filter + subdomain = microsite.get_value('subdomain', 'default') if hasattr(settings, 'COURSE_LISTINGS') and subdomain in settings.COURSE_LISTINGS and not settings.DEBUG: filtered_visible_ids = frozenset( [SlashSeparatedCourseKey.from_deprecated_string(c) for c in settings.COURSE_LISTINGS[subdomain]] diff --git a/lms/djangoapps/class_dashboard/views.py b/lms/djangoapps/class_dashboard/views.py index c5d6db4be5..d5d67c793e 100644 --- a/lms/djangoapps/class_dashboard/views.py +++ b/lms/djangoapps/class_dashboard/views.py @@ -8,7 +8,7 @@ import json from django.http import HttpResponse from opaque_keys.edx.locations import SlashSeparatedCourseKey -from courseware.courses import get_course_with_access +from courseware.courses import get_course_overview_with_access from courseware.access import has_access from class_dashboard import dashboard_data @@ -21,7 +21,7 @@ def has_instructor_access_for_class(user, course_id): Returns true if the `user` is an instructor for the course. """ - course = get_course_with_access(user, 'staff', course_id, depth=None) + course = get_course_overview_with_access(user, 'staff', course_id) return bool(has_access(user, 'staff', course)) diff --git a/lms/djangoapps/course_wiki/middleware.py b/lms/djangoapps/course_wiki/middleware.py index 89fff00220..76de84234c 100644 --- a/lms/djangoapps/course_wiki/middleware.py +++ b/lms/djangoapps/course_wiki/middleware.py @@ -6,7 +6,7 @@ from django.shortcuts import redirect from django.core.exceptions import PermissionDenied from wiki.models import reverse -from courseware.courses import get_course_with_access +from courseware.courses import get_course_with_access, get_course_overview_with_access from courseware.access import has_access from student.models import CourseEnrollment from util.request import course_id_from_url @@ -29,7 +29,7 @@ class WikiAccessMiddleware(object): if course_id: # See if we are able to view the course. If we are, redirect to it try: - _course = get_course_with_access(request.user, 'load', course_id) + get_course_overview_with_access(request.user, 'load', course_id) return redirect("/courses/{course_id}/wiki/{path}".format(course_id=course_id.to_deprecated_string(), path=wiki_path)) except Http404: # Even though we came from the course, we can't see it. So don't worry about it. diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index d2f24298bf..66cf45e928 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -105,10 +105,10 @@ def has_access(user, action, obj, course_key=None): # delegate the work to type-specific functions. # (start with more specific types, then get more general) if isinstance(obj, CourseDescriptor): - return _has_access_course_desc(user, action, obj) + return _has_access_course(user, action, obj) if isinstance(obj, CourseOverview): - return _has_access_course_overview(user, action, obj) + return _has_access_course(user, action, obj) if isinstance(obj, ErrorDescriptor): return _has_access_error_desc(user, action, obj, course_key) @@ -202,7 +202,7 @@ def _can_load_course_on_mobile(user, course): be checked by callers in *addition* to the return value of this function. Arguments: - user (User): the user whose course access we are checking. + user (User): the user whose course access we are checking. course (CourseDescriptor|CourseOverview): the course for which we are checking access. @@ -270,7 +270,7 @@ def _can_enroll_courselike(user, courselike): return ACCESS_DENIED -def _has_access_courselike(user, action, courselike): +def _has_access_course(user, action, courselike): """ Check if user has access to a course. @@ -297,11 +297,21 @@ def _has_access_courselike(user, action, courselike): NOTE: this is not checking whether user is actually enrolled in the course. """ - # delegate to generic descriptor check to check start dates - return _has_access_descriptor(user, 'load', course, course.id) + response = ( + _visible_to_nonstaff_users(courselike) and + _can_access_descriptor_with_start_date(user, courselike, courselike.id) + ) + + return ( + ACCESS_GRANTED if (response or _has_staff_access_to_descriptor(user, courselike, courselike.id)) + else response + ) def can_enroll(): - return _can_enroll_courselike(user, course) + """ + Returns whether the user can enroll in the course. + """ + return _can_enroll_courselike(user, courselike) def see_exists(): """ @@ -317,8 +327,8 @@ def _has_access_courselike(user, action, courselike): but also allow course staff to see this. """ return ( - _has_catalog_visibility(course, CATALOG_VISIBILITY_CATALOG_AND_ABOUT) - or _has_staff_access_to_descriptor(user, course, course.id) + _has_catalog_visibility(courselike, CATALOG_VISIBILITY_CATALOG_AND_ABOUT) + or _has_staff_access_to_descriptor(user, courselike, courselike.id) ) def can_see_about_page(): @@ -328,75 +338,25 @@ def _has_access_courselike(user, action, courselike): but also allow course staff to see this. """ return ( - _has_catalog_visibility(course, CATALOG_VISIBILITY_CATALOG_AND_ABOUT) - or _has_catalog_visibility(course, CATALOG_VISIBILITY_ABOUT) - or _has_staff_access_to_descriptor(user, course, course.id) + _has_catalog_visibility(courselike, CATALOG_VISIBILITY_CATALOG_AND_ABOUT) + or _has_catalog_visibility(courselike, CATALOG_VISIBILITY_ABOUT) + or _has_staff_access_to_descriptor(user, courselike, courselike.id) ) checkers = { 'load': can_load, 'view_courseware_with_prerequisites': - lambda: _can_view_courseware_with_prerequisites(user, course), - 'load_mobile': lambda: can_load() and _can_load_course_on_mobile(user, course), + lambda: _can_view_courseware_with_prerequisites(user, courselike), + 'load_mobile': lambda: can_load() and _can_load_course_on_mobile(user, courselike), 'enroll': can_enroll, 'see_exists': see_exists, - 'staff': lambda: _has_staff_access_to_descriptor(user, course, course.id), - 'instructor': lambda: _has_instructor_access_to_descriptor(user, course, course.id), + 'staff': lambda: _has_staff_access_to_descriptor(user, courselike, courselike.id), + 'instructor': lambda: _has_instructor_access_to_descriptor(user, courselike, courselike.id), 'see_in_catalog': can_see_in_catalog, 'see_about_page': can_see_about_page, } - return _dispatch(checkers, action, user, course) - - -def _can_load_course_overview(user, course_overview): - """ - Check if a user can load a course overview. - - Arguments: - user (User): the user whose course access we are checking. - course_overview (CourseOverview): a course overview. - - Note: - The user doesn't have to be enrolled in the course in order to have load - load access. - """ - response = ( - _visible_to_nonstaff_users(course_overview) - and _can_access_descriptor_with_start_date(user, course_overview, course_overview.id) - ) - - return ( - ACCESS_GRANTED if (response or _has_staff_access_to_descriptor(user, course_overview, course_overview.id)) - else response - ) - -_COURSE_OVERVIEW_CHECKERS = { - 'enroll': _can_enroll_courselike, - 'load': _can_load_course_overview, - 'load_mobile': lambda user, course_overview: ( - _can_load_course_overview(user, course_overview) - and _can_load_course_on_mobile(user, course_overview) - ), - 'view_courseware_with_prerequisites': _can_view_courseware_with_prerequisites -} -COURSE_OVERVIEW_SUPPORTED_ACTIONS = _COURSE_OVERVIEW_CHECKERS.keys() - - -def _has_access_course_overview(user, action, course_overview): - """ - Check if user has access to a course overview. - - Arguments: - user (User): the user whose course access we are checking. - action (str): the action the user is trying to perform. - See COURSE_OVERVIEW_SUPPORTED_ACTIONS for valid values. - course_overview (CourseOverview): overview of the course in question. - """ - if action in _COURSE_OVERVIEW_CHECKERS: - return _COURSE_OVERVIEW_CHECKERS[action](user, course_overview) - else: - raise ValueError(u"Unknown action for object type 'CourseOverview': '{}'".format(action)) + return _dispatch(checkers, action, user, courselike) def _has_access_error_desc(user, action, descriptor, course_key): diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 328396d21b..a31ed93288 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -14,7 +14,6 @@ from django.conf import settings from edxmako.shortcuts import render_to_string from xmodule.modulestore import ModuleStoreEnum -from opaque_keys.edx.keys import CourseKey from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from static_replace import replace_static_urls @@ -37,6 +36,7 @@ from student.models import CourseEnrollment import branding from opaque_keys.edx.keys import UsageKey +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview log = logging.getLogger(__name__) @@ -58,7 +58,6 @@ def get_course(course_id, depth=0): return course -# TODO please rename this function to get_course_by_key at next opportunity! def get_course_by_id(course_key, depth=0): """ Given a course id, return the corresponding course descriptor. @@ -94,9 +93,39 @@ def get_course_with_access(user, action, course_key, depth=0, check_if_enrolled= check_if_enrolled: If true, additionally verifies that the user is either enrolled in the course or has staff access. """ - assert isinstance(course_key, CourseKey) - course = get_course_by_id(course_key, depth=depth) - access_response = has_access(user, action, course, course_key) + course = get_course_by_id(course_key, depth) + check_course_access(course, user, action, check_if_enrolled) + return course + + +def get_course_overview_with_access(user, action, course_key, check_if_enrolled=False): + """ + Given a course_key, look up the corresponding course overview, + check that the user has the access to perform the specified action + on the course, and return the course overview. + + Raises a 404 if the course_key is invalid, or the user doesn't have access. + + check_if_enrolled: If true, additionally verifies that the user is either enrolled in the course + or has staff access. + """ + try: + course_overview = CourseOverview.get_from_id(course_key) + except CourseOverview.DoesNotExist: + raise Http404("Course not found.") + check_course_access(course_overview, user, action, check_if_enrolled) + return course_overview + + +def check_course_access(course, user, action, check_if_enrolled=False): + """ + Check that the user has the access to perform the specified action + on the course (CourseDescriptor|CourseOverview). + + check_if_enrolled: If true, additionally verifies that the user is either + enrolled in the course or has staff access. + """ + access_response = has_access(user, action, course, course.id) if not access_response: # Deliberately return a non-specific error message to avoid @@ -104,12 +133,11 @@ def get_course_with_access(user, action, course_key, depth=0, check_if_enrolled= raise CoursewareAccessException(access_response) 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. - if not ((user.id and CourseEnrollment.is_enrolled(user, course_key)) or has_access(user, 'staff', course)): - raise UserNotEnrolled(course_key) - - return course + # 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. + if not ((user.id and CourseEnrollment.is_enrolled(user, course.id)) or has_access(user, 'staff', course)): + raise UserNotEnrolled(course.id) def find_file(filesystem, dirs, filename): @@ -129,16 +157,6 @@ def find_file(filesystem, dirs, filename): raise ResourceNotFoundError(u"Could not find {0}".format(filename)) -def get_course_university_about_section(course): # pylint: disable=invalid-name - """ - Returns a snippet of HTML displaying the course's university. - - Arguments: - course (CourseDescriptor|CourseOverview): A course. - """ - return course.display_org_with_default - - def get_course_about_section(request, course, section_key): """ This returns the snippet of html to be rendered on the course about page, @@ -146,9 +164,6 @@ def get_course_about_section(request, course, section_key): Valid keys: - overview - - title - - university - - number - short_description - description - key_dates (includes start, end, exams, etc) @@ -159,6 +174,7 @@ def get_course_about_section(request, course, section_key): - syllabus - textbook - faq + - effort - more_info - ocw_links """ @@ -167,7 +183,6 @@ def get_course_about_section(request, course, section_key): # markup. This can change without effecting this interface when we find a # good format for defining so many snippets of text/html. - # TODO: Remove number, instructors from this set html_sections = { 'short_description', 'description', @@ -180,8 +195,6 @@ def get_course_about_section(request, course, section_key): 'textbook', 'faq', 'more_info', - 'number', - 'instructors', 'overview', 'effort', 'end_date', @@ -225,12 +238,6 @@ def get_course_about_section(request, course, section_key): section_key, course.location.to_deprecated_string() ) return None - elif section_key == "title": - return course.display_name_with_default - elif section_key == "university": - return get_course_university_about_section(course) - elif section_key == "number": - return course.display_number_with_default raise KeyError("Invalid about key " + str(section_key)) @@ -366,22 +373,6 @@ def get_course_syllabus_section(course, section_key): raise KeyError("Invalid about key " + str(section_key)) -def get_courses_by_university(user, domain=None): - ''' - Returns dict of lists of courses available, keyed by course.org (ie university). - Courses are sorted by course.number. - ''' - # TODO: Clean up how 'error' is done. - # filter out any courses that errored. - visible_courses = get_courses(user, domain) - - universities = defaultdict(list) - for course in visible_courses: - universities[course.org].append(course) - - return universities - - def get_courses(user, domain=None): ''' Returns a list of courses available, sorted by course.number @@ -400,6 +391,16 @@ def get_courses(user, domain=None): return courses +def get_permission_for_course_about(): + """ + Returns the CourseOverview object for the course after checking for access. + """ + return microsite.get_value( + 'COURSE_ABOUT_VISIBILITY_PERMISSION', + settings.COURSE_ABOUT_VISIBILITY_PERMISSION + ) + + def sort_by_announcement(courses): """ Sorts a list of courses by their announcement date. If the date is diff --git a/lms/djangoapps/courseware/tests/helpers.py b/lms/djangoapps/courseware/tests/helpers.py index a72b5c2cdf..193de70570 100644 --- a/lms/djangoapps/courseware/tests/helpers.py +++ b/lms/djangoapps/courseware/tests/helpers.py @@ -5,7 +5,7 @@ from django.core.urlresolvers import reverse from django.test import TestCase from django.test.client import RequestFactory -from courseware.access import has_access, COURSE_OVERVIEW_SUPPORTED_ACTIONS +from courseware.access import has_access from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from student.models import Registration @@ -151,30 +151,27 @@ class CourseAccessTestMixin(TestCase): """ Assert that a user has access to the given action for a given course. - Test with both the given course and, if the action is supported, with - a CourseOverview of the given course. + Test with both the given course and with a CourseOverview of the given + course. Arguments: user (User): a user. action (str): type of access to test. - See access.py:COURSE_OVERVIEW_SUPPORTED_ACTIONS. course (CourseDescriptor): a course. """ self.assertTrue(has_access(user, action, course)) - if action in COURSE_OVERVIEW_SUPPORTED_ACTIONS: - self.assertTrue(has_access(user, action, CourseOverview.get_from_id(course.id))) + self.assertTrue(has_access(user, action, CourseOverview.get_from_id(course.id))) def assertCannotAccessCourse(self, user, action, course): """ Assert that a user lacks access to the given action the given course. - Test with both the given course and, if the action is supported, with - a CourseOverview of the given course. + Test with both the given course and with a CourseOverview of the given + course. Arguments: user (User): a user. action (str): type of access to test. - See access.py:COURSE_OVERVIEW_SUPPORTED_ACTIONS. course (CourseDescriptor): a course. Note: @@ -184,5 +181,4 @@ class CourseAccessTestMixin(TestCase): stack traces of failed tests easier to understand at a glance. """ self.assertFalse(has_access(user, action, course)) - if action in COURSE_OVERVIEW_SUPPORTED_ACTIONS: - self.assertFalse(has_access(user, action, CourseOverview.get_from_id(course.id))) + self.assertFalse(has_access(user, action, CourseOverview.get_from_id(course.id))) diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index b60baaca3c..d9b222d69b 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -236,7 +236,7 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): mock_unit.start = start self.verify_access(mock_unit, expected_access, expected_error_type) - def test__has_access_course_desc_can_enroll(self): + def test__has_access_course_can_enroll(self): yesterday = datetime.datetime.now(pytz.utc) - datetime.timedelta(days=1) tomorrow = datetime.datetime.now(pytz.utc) + datetime.timedelta(days=1) @@ -248,11 +248,11 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): id=SlashSeparatedCourseKey('edX', 'test', '2012_Fall'), enrollment_domain='' ) CourseEnrollmentAllowedFactory(email=user.email, course_id=course.id) - self.assertTrue(access._has_access_course_desc(user, 'enroll', course)) + self.assertTrue(access._has_access_course(user, 'enroll', course)) # Staff can always enroll even outside the open enrollment period user = StaffFactory.create(course_key=course.id) - self.assertTrue(access._has_access_course_desc(user, 'enroll', course)) + self.assertTrue(access._has_access_course(user, 'enroll', course)) # Non-staff cannot enroll if it is between the start and end dates and invitation only # and not specifically allowed @@ -262,7 +262,7 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): invitation_only=True ) user = UserFactory.create() - self.assertFalse(access._has_access_course_desc(user, 'enroll', course)) + self.assertFalse(access._has_access_course(user, 'enroll', course)) # Non-staff can enroll if it is between the start and end dates and not invitation only course = Mock( @@ -270,7 +270,7 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): id=SlashSeparatedCourseKey('edX', 'test', '2012_Fall'), enrollment_domain='', invitation_only=False ) - self.assertTrue(access._has_access_course_desc(user, 'enroll', course)) + self.assertTrue(access._has_access_course(user, 'enroll', course)) # Non-staff cannot enroll outside the open enrollment period if not specifically allowed course = Mock( @@ -278,7 +278,7 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): id=SlashSeparatedCourseKey('edX', 'test', '2012_Fall'), enrollment_domain='', invitation_only=False ) - self.assertFalse(access._has_access_course_desc(user, 'enroll', course)) + self.assertFalse(access._has_access_course(user, 'enroll', course)) def test__user_passed_as_none(self): """Ensure has_access handles a user being passed as null""" @@ -296,40 +296,30 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): id=course_id, catalog_visibility=CATALOG_VISIBILITY_CATALOG_AND_ABOUT ) - self.assertTrue(access._has_access_course_desc(user, 'see_in_catalog', course)) - self.assertTrue(access._has_access_course_desc(user, 'see_about_page', course)) - self.assertTrue(access._has_access_course_desc(staff, 'see_in_catalog', course)) - self.assertTrue(access._has_access_course_desc(staff, 'see_about_page', course)) + self.assertTrue(access._has_access_course(user, 'see_in_catalog', course)) + self.assertTrue(access._has_access_course(user, 'see_about_page', course)) + self.assertTrue(access._has_access_course(staff, 'see_in_catalog', course)) + self.assertTrue(access._has_access_course(staff, 'see_about_page', course)) # Now set visibility to just about page course = Mock( id=SlashSeparatedCourseKey('edX', 'test', '2012_Fall'), catalog_visibility=CATALOG_VISIBILITY_ABOUT ) - self.assertFalse(access._has_access_course_desc(user, 'see_in_catalog', course)) - self.assertTrue(access._has_access_course_desc(user, 'see_about_page', course)) - self.assertTrue(access._has_access_course_desc(staff, 'see_in_catalog', course)) - self.assertTrue(access._has_access_course_desc(staff, 'see_about_page', course)) + self.assertFalse(access._has_access_course(user, 'see_in_catalog', course)) + self.assertTrue(access._has_access_course(user, 'see_about_page', course)) + self.assertTrue(access._has_access_course(staff, 'see_in_catalog', course)) + self.assertTrue(access._has_access_course(staff, 'see_about_page', course)) # Now set visibility to none, which means neither in catalog nor about pages course = Mock( id=SlashSeparatedCourseKey('edX', 'test', '2012_Fall'), catalog_visibility=CATALOG_VISIBILITY_NONE ) - self.assertFalse(access._has_access_course_desc(user, 'see_in_catalog', course)) - self.assertFalse(access._has_access_course_desc(user, 'see_about_page', course)) - self.assertTrue(access._has_access_course_desc(staff, 'see_in_catalog', course)) - self.assertTrue(access._has_access_course_desc(staff, 'see_about_page', course)) - - @ddt.data(True, False) - @patch.dict("django.conf.settings.FEATURES", {'ACCESS_REQUIRE_STAFF_FOR_COURSE': True}) - def test_see_exists(self, ispublic): - """ - Test if user can see course - """ - user = UserFactory.create(is_staff=False) - course = Mock(ispublic=ispublic) - self.assertEquals(bool(access._has_access_course_desc(user, 'see_exists', course)), ispublic) + self.assertFalse(access._has_access_course(user, 'see_in_catalog', course)) + self.assertFalse(access._has_access_course(user, 'see_about_page', course)) + self.assertTrue(access._has_access_course(staff, 'see_in_catalog', course)) + self.assertTrue(access._has_access_course(staff, 'see_about_page', course)) @patch.dict("django.conf.settings.FEATURES", {'ENABLE_PREREQUISITE_COURSES': True, 'MILESTONES_APP': True}) def test_access_on_course_with_pre_requisites(self): @@ -351,16 +341,16 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): # user should not be able to load course even if enrolled CourseEnrollmentFactory(user=user, course_id=course.id) - response = access._has_access_course_desc(user, 'view_courseware_with_prerequisites', course) + response = access._has_access_course(user, 'view_courseware_with_prerequisites', course) self.assertFalse(response) self.assertIsInstance(response, access_response.MilestoneError) # Staff can always access course staff = StaffFactory.create(course_key=course.id) - self.assertTrue(access._has_access_course_desc(staff, 'view_courseware_with_prerequisites', course)) + self.assertTrue(access._has_access_course(staff, 'view_courseware_with_prerequisites', course)) # User should be able access after completing required course fulfill_course_milestone(pre_requisite_course.id, user) - self.assertTrue(access._has_access_course_desc(user, 'view_courseware_with_prerequisites', course)) + self.assertTrue(access._has_access_course(user, 'view_courseware_with_prerequisites', course)) @ddt.data( (True, True, True), @@ -377,10 +367,10 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): descriptor.mobile_available = mobile_available self.assertEqual( - bool(access._has_access_course_desc(self.student, 'load_mobile', descriptor)), + bool(access._has_access_course(self.student, 'load_mobile', descriptor)), student_expected ) - self.assertEqual(bool(access._has_access_course_desc(self.staff, 'load_mobile', descriptor)), staff_expected) + self.assertEqual(bool(access._has_access_course(self.staff, 'load_mobile', descriptor)), staff_expected) @patch.dict("django.conf.settings.FEATURES", {'ENABLE_PREREQUISITE_COURSES': True, 'MILESTONES_APP': True}) def test_courseware_page_unfulfilled_prereqs(self): @@ -552,7 +542,6 @@ class CourseOverviewAccessTestCase(ModuleStoreTestCase): user_attr_name (str): the name of the attribute on self that is the User to test with. action (str): action to test with. - See COURSE_OVERVIEW_SUPPORTED_ACTIONS for valid values. course_attr_name (str): the name of the attribute on self that is the CourseDescriptor to test with. """ diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index bf528801a0..c87bf922ee 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -18,7 +18,7 @@ from courseware.courses import ( get_course_info_section, get_course_about_section, get_cms_block_link ) -from courseware.courses import get_course_with_access +from courseware.courses import get_course_with_access, get_course_overview_with_access from courseware.module_render import get_module_for_descriptor from courseware.tests.helpers import get_request_for_user from courseware.model_data import FieldDataCache @@ -30,7 +30,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.xml_importer import import_course_from_xml from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.django_utils import TEST_DATA_MIXED_TOY_MODULESTORE -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls from xmodule.tests.xml import factories as xml from xmodule.tests.xml import XModuleXmlImportTest @@ -40,6 +40,7 @@ TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT @attr('shard_1') +@ddt.ddt class CoursesTest(ModuleStoreTestCase): """Test methods related to fetching courses.""" @@ -57,16 +58,28 @@ class CoursesTest(ModuleStoreTestCase): cms_url = u"//{}/course/{}".format(CMS_BASE_TEST, unicode(self.course.location)) self.assertEqual(cms_url, get_cms_block_link(self.course, 'course')) - def test_get_course_with_access(self): + @ddt.data(get_course_with_access, get_course_overview_with_access) + def test_get_course_func_with_access_error(self, course_access_func): user = UserFactory.create() course = CourseFactory.create(visible_to_staff_only=True) with self.assertRaises(CoursewareAccessException) as error: - get_course_with_access(user, 'load', course.id) + course_access_func(user, 'load', course.id) self.assertEqual(error.exception.message, "Course not found.") self.assertEqual(error.exception.access_response.error_code, "not_visible_to_user") self.assertFalse(error.exception.access_response.has_access) + @ddt.data( + (get_course_with_access, 1), + (get_course_overview_with_access, 0), + ) + @ddt.unpack + def test_get_course_func_with_access(self, course_access_func, num_mongo_calls): + user = UserFactory.create() + course = CourseFactory.create(emit_signals=True) + with check_mongo_calls(num_mongo_calls): + course_access_func(user, 'load', course.id) + @attr('shard_1') class ModuleStoreBranchSettingTest(ModuleStoreTestCase): diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index fdcb37d44f..9f29068485 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -37,11 +37,17 @@ from courseware.access import has_access, _adjust_start_date_for_beta_testers from courseware.access_response import StartDateError from courseware.access_utils import in_preview_mode from courseware.courses import ( - get_courses, get_course, get_course_by_id, - get_studio_url, get_course_with_access, + 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, sort_by_announcement, sort_by_start_date, - UserNotEnrolled) + UserNotEnrolled +) from courseware.masquerade import setup_masquerade from openedx.core.djangoapps.credit.api import ( get_credit_requirement_status, @@ -802,11 +808,8 @@ def course_about(request, course_id): course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) with modulestore().bulk_operations(course_key): - permission_name = microsite.get_value( - 'COURSE_ABOUT_VISIBILITY_PERMISSION', - settings.COURSE_ABOUT_VISIBILITY_PERMISSION - ) - course = get_course_with_access(request.user, permission_name, course_key) + permission = get_permission_for_course_about() + course = get_course_with_access(request.user, permission, course_key) if microsite.get_value('ENABLE_MKTG_SITE', settings.FEATURES.get('ENABLE_MKTG_SITE', False)): return redirect(reverse('info', args=[course.id.to_deprecated_string()])) @@ -1066,7 +1069,7 @@ def submission_history(request, course_id, student_username, location): except (InvalidKeyError, AssertionError): return HttpResponse(escape(_(u'Invalid location.'))) - course = get_course_with_access(request.user, 'load', course_key) + course = get_course_overview_with_access(request.user, 'load', course_key) staff_access = bool(has_access(request.user, 'staff', course)) # Permission Denied if they don't have staff access and are trying to see diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index 4aff5dfc94..79ae6921bc 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -15,7 +15,7 @@ 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_by_id +from courseware.courses import get_course_with_access, get_course_overview_with_access, get_course_by_id import django_comment_client.settings as cc_settings from django_comment_common.signals import ( thread_created, @@ -770,7 +770,7 @@ def users(request, course_id): course_key = CourseKey.from_string(course_id) try: - get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True) + get_course_overview_with_access(request.user, 'load', course_key, check_if_enrolled=True) except Http404: # course didn't exist, or requesting user does not have access to it. return JsonError(status=404) diff --git a/lms/templates/course.html b/lms/templates/course.html index a25dcee6bb..8fffd9b262 100644 --- a/lms/templates/course.html +++ b/lms/templates/course.html @@ -2,30 +2,28 @@ <%! from django.utils.translation import ugettext as _ from django.core.urlresolvers import reverse -from courseware.courses import get_course_about_section -from openedx.core.lib.courses import course_image_url %> <%page args="course" /> -
+
- ${get_course_about_section(request, course, 'title')} ${course.display_number_with_default} + ${course.display_name_with_default} ${course.display_number_with_default | h}
    -
  • ${get_course_about_section(request, course, 'university')}
  • -
  • ${course.display_number_with_default}
  • +
  • ${course.display_org_with_default | h}
  • +
  • ${course.display_number_with_default | h}
  • ${_("Starts")}:
diff --git a/lms/templates/courseware/course_about.html b/lms/templates/courseware/course_about.html index ae0c41deaf..1d009726d4 100644 --- a/lms/templates/courseware/course_about.html +++ b/lms/templates/courseware/course_about.html @@ -13,7 +13,7 @@ from openedx.core.lib.courses import course_image_url <%block name="headextra"> ## OG (Open Graph) title and description added below to give social media info to display ## (https://developers.facebook.com/docs/opengraph/howtos/maximizing-distribution-media-content#tags) - + @@ -102,7 +102,7 @@ from openedx.core.lib.courses import course_image_url -<%block name="pagetitle">${get_course_about_section(request, course, "title")} +<%block name="pagetitle">${course.display_name_with_default}
@@ -111,9 +111,9 @@ from openedx.core.lib.courses import course_image_url

- ${get_course_about_section(request, course, "title")} + ${course.display_name_with_default} % if not self.theme_enabled(): - ${get_course_about_section(request, course, "university")} + ${course.display_org_with_default | h} % endif

@@ -220,10 +220,10 @@ from openedx.core.lib.courses import course_image_url ## or something allowing themes to do whatever they ## want here (and on this whole page, really). % if self.stanford_theme_enabled(): - - % else: @@ -235,7 +235,7 @@ from openedx.core.lib.courses import course_image_url ## Twitter account. {url} should appear at the end of the text. tweet_text = _("I just enrolled in {number} {title} through {account}: {url}").format( number=course.number, - title=get_course_about_section(request, course, 'title'), + title=course.display_name_with_default, account=microsite.get_value('course_about_twitter_account', settings.PLATFORM_TWITTER_ACCOUNT), url=u"http://{domain}{path}".format( domain=site_domain, @@ -250,7 +250,7 @@ from openedx.core.lib.courses import course_image_url subject=_("Take a course with {platform} online").format(platform=platform_name), body=_("I just enrolled in {number} {title} through {platform} {url}").format( number=course.number, - title=get_course_about_section(request, course, 'title'), + title=course.display_name_with_default, platform=platform_name, url=u"http://{domain}{path}".format( domain=site_domain, diff --git a/lms/templates/dashboard/_dashboard_course_listing.html b/lms/templates/dashboard/_dashboard_course_listing.html index 50e6b1ac1a..0f1337700a 100644 --- a/lms/templates/dashboard/_dashboard_course_listing.html +++ b/lms/templates/dashboard/_dashboard_course_listing.html @@ -7,7 +7,6 @@ from django.utils.translation import ugettext as _ from django.utils.translation import ungettext from django.core.urlresolvers import reverse from markupsafe import escape -from courseware.courses import get_course_university_about_section from course_modes.models import CourseMode from course_modes.helpers import enrollment_mode_display from student.helpers import ( @@ -99,7 +98,7 @@ from student.helpers import ( % endif
- ${get_course_university_about_section(course_overview)} - + ${course_overview.display_org_with_default | h} - ${course_overview.display_number_with_default | h} % if course_overview.has_ended(): diff --git a/lms/templates/shoppingcart/receipt.html b/lms/templates/shoppingcart/receipt.html index 66209219b6..6bccd94cc7 100644 --- a/lms/templates/shoppingcart/receipt.html +++ b/lms/templates/shoppingcart/receipt.html @@ -3,7 +3,6 @@ from django.utils.translation import ugettext as _ from django.utils.translation import ungettext from django.core.urlresolvers import reverse -from courseware.courses import get_course_about_section, get_course_by_id from markupsafe import escape from microsite_configuration import microsite from openedx.core.lib.courses import course_image_url @@ -293,7 +292,7 @@ from openedx.core.lib.courses import course_image_url
${course.display_number_with_default | h} ${get_course_about_section(request, course, 'title')} Image + alt="${course.display_number_with_default | h} ${course.display_name_with_default} Image"/>
diff --git a/lms/templates/shoppingcart/registration_code_receipt.html b/lms/templates/shoppingcart/registration_code_receipt.html index 94e21a8de7..b0eba27f9c 100644 --- a/lms/templates/shoppingcart/registration_code_receipt.html +++ b/lms/templates/shoppingcart/registration_code_receipt.html @@ -1,7 +1,6 @@ <%! from django.utils.translation import ugettext as _ from django.core.urlresolvers import reverse -from courseware.courses import get_course_about_section from openedx.core.lib.courses import course_image_url %> <%inherit file="../main.html" /> @@ -21,7 +20,7 @@ from openedx.core.lib.courses import course_image_url ${_(
diff --git a/lms/templates/shoppingcart/registration_code_redemption.html b/lms/templates/shoppingcart/registration_code_redemption.html index 00ab425932..6c8601e651 100644 --- a/lms/templates/shoppingcart/registration_code_redemption.html +++ b/lms/templates/shoppingcart/registration_code_redemption.html @@ -1,7 +1,6 @@ <%! from django.utils.translation import ugettext as _ from django.core.urlresolvers import reverse -from courseware.courses import get_course_about_section from openedx.core.lib.courses import course_image_url %> <%inherit file="../main.html" /> @@ -21,7 +20,7 @@ from openedx.core.lib.courses import course_image_url ${_(
diff --git a/lms/templates/shoppingcart/shopping_cart.html b/lms/templates/shoppingcart/shopping_cart.html index b81c431b9b..eb6f5e8e82 100644 --- a/lms/templates/shoppingcart/shopping_cart.html +++ b/lms/templates/shoppingcart/shopping_cart.html @@ -2,7 +2,6 @@ <%block name="review_highlight">class="active" <%! -from courseware.courses import get_course_about_section from django.core.urlresolvers import reverse from edxmako.shortcuts import marketing_link from django.utils.translation import ugettext as _ @@ -67,7 +66,7 @@ from openedx.core.lib.courses import course_image_url
${course.display_number_with_default | h} ${get_course_about_section(request, course, 'title')} ${_('Cover Image')} + alt="${course.display_number_with_default | h} ${course.display_name_with_default} ${_('Cover Image')}" />
## Translators: "Registration for:" is followed by a course name