diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 06abbe7706..85c2b6def0 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -23,8 +23,9 @@ VERIFY_STATUS_MISSED_DEADLINE = "verify_missed_deadline" VERIFY_STATUS_NEED_TO_REVERIFY = "verify_need_to_reverify" -def check_verify_status_by_course(user, course_enrollment_pairs, all_course_modes): - """Determine the per-course verification statuses for a given user. +def check_verify_status_by_course(user, course_enrollments, all_course_modes): + """ + Determine the per-course verification statuses for a given user. The possible statuses are: * VERIFY_STATUS_NEED_TO_VERIFY: The student has not yet submitted photos for verification. @@ -46,8 +47,7 @@ def check_verify_status_by_course(user, course_enrollment_pairs, all_course_mode Arguments: user (User): The currently logged-in user. - course_enrollment_pairs (list): The courses the user is enrolled in. - The list should contain tuples of `(Course, CourseEnrollment)`. + course_enrollments (list[CourseEnrollment]): The courses the user is enrolled in. all_course_modes (list): List of all course modes for the student's enrolled courses, including modes that have expired. @@ -75,15 +75,15 @@ def check_verify_status_by_course(user, course_enrollment_pairs, all_course_mode recent_verification_datetime = None - for course, enrollment in course_enrollment_pairs: + for enrollment in course_enrollments: # Get the verified mode (if any) for this course # We pass in the course modes we have already loaded to avoid # another database hit, as well as to ensure that expired # course modes are included in the search. verified_mode = CourseMode.verified_mode_for_course( - course.id, - modes=all_course_modes[course.id] + enrollment.course_id, + modes=all_course_modes[enrollment.course_id] ) # If no verified mode has ever been offered, or the user hasn't enrolled @@ -156,7 +156,7 @@ def check_verify_status_by_course(user, course_enrollment_pairs, all_course_mode if deadline is not None and deadline > now: days_until_deadline = (deadline - now).days - status_by_course[course.id] = { + status_by_course[enrollment.course_id] = { 'status': status, 'days_until_deadline': days_until_deadline } diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 0e9f5566fb..23abdd071c 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -850,6 +850,13 @@ class CourseEnrollment(models.Model): unique_together = (('user', 'course_id'),) ordering = ('user', 'course_id') + def __init__(self, *args, **kwargs): + super(CourseEnrollment, self).__init__(*args, **kwargs) + + # Private variable for storing course_overview to minimize calls to the database. + # When the property .course_overview is accessed for the first time, this variable will be set. + self._course_overview = None + def __unicode__(self): return ( "[CourseEnrollment] {}: {} ({}); active: ({})" @@ -1318,10 +1325,21 @@ class CourseEnrollment(models.Model): @property def course_overview(self): """ - Return a CourseOverview of this enrollment's course. - """ - from openedx.core.djangoapps.content.course_overviews.models import CourseOverview - return CourseOverview.get_from_id(self.course_id) + Returns a CourseOverview of the course to which this enrollment refers. + Returns None if an error occurred while trying to load the course. + + Note: + If the course is re-published within the lifetime of this + CourseEnrollment object, then the value of this property will + become stale. + """ + if not self._course_overview: + from openedx.core.djangoapps.content.course_overviews.models import CourseOverview + try: + self._course_overview = CourseOverview.get_from_id(self.course_id) + except (CourseOverview.DoesNotExist, IOError): + self._course_overview = None + return self._course_overview def is_verified_enrollment(self): """ diff --git a/common/djangoapps/student/tests/test_course_listing.py b/common/djangoapps/student/tests/test_course_listing.py index 0e6bdd7299..cdf75a342b 100644 --- a/common/djangoapps/student/tests/test_course_listing.py +++ b/common/djangoapps/student/tests/test_course_listing.py @@ -14,7 +14,7 @@ from xmodule.modulestore.django import modulestore from xmodule.error_module import ErrorDescriptor from django.test.client import Client from student.models import CourseEnrollment -from student.views import get_course_enrollment_pairs +from student.views import get_course_enrollments from util.milestones_helpers import ( get_pre_requisite_courses_not_completed, set_prerequisite_courses, @@ -73,13 +73,13 @@ class TestCourseListing(ModuleStoreTestCase): self._create_course_with_access_groups(course_location) # get dashboard - courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + courses_list = list(get_course_enrollments(self.student, None, [])) self.assertEqual(len(courses_list), 1) - self.assertEqual(courses_list[0][0].id, course_location) + self.assertEqual(courses_list[0].course_id, course_location) CourseEnrollment.unenroll(self.student, course_location) # get dashboard - courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + courses_list = list(get_course_enrollments(self.student, None, [])) self.assertEqual(len(courses_list), 0) def test_errored_course_regular_access(self): @@ -95,7 +95,7 @@ class TestCourseListing(ModuleStoreTestCase): self.assertIsInstance(modulestore().get_course(course_key), ErrorDescriptor) # get courses through iterating all courses - courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + courses_list = list(get_course_enrollments(self.student, None, [])) self.assertEqual(courses_list, []) def test_course_listing_errored_deleted_courses(self): @@ -112,9 +112,9 @@ class TestCourseListing(ModuleStoreTestCase): self._create_course_with_access_groups(course_location, default_store=ModuleStoreEnum.Type.mongo) mongo_store.delete_course(course_location, ModuleStoreEnum.UserID.test) - courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + courses_list = list(get_course_enrollments(self.student, None, [])) self.assertEqual(len(courses_list), 1, courses_list) - self.assertEqual(courses_list[0][0].id, good_location) + self.assertEqual(courses_list[0].course_id, good_location) @mock.patch.dict("django.conf.settings.FEATURES", {'ENABLE_PREREQUISITE_COURSES': True, 'MILESTONES_APP': True}) def test_course_listing_has_pre_requisite_courses(self): @@ -142,9 +142,11 @@ class TestCourseListing(ModuleStoreTestCase): set_prerequisite_courses(course_location, pre_requisite_courses) # get dashboard - course_enrollment_pairs = list(get_course_enrollment_pairs(self.student, None, [])) - courses_having_prerequisites = frozenset(course.id for course, _enrollment in course_enrollment_pairs - if course.pre_requisite_courses) + course_enrollments = list(get_course_enrollments(self.student, None, [])) + courses_having_prerequisites = frozenset( + enrollment.course_id for enrollment in course_enrollments + if enrollment.course_overview.pre_requisite_courses + ) courses_requirements_not_met = get_pre_requisite_courses_not_completed( self.student, courses_having_prerequisites diff --git a/common/djangoapps/student/tests/test_recent_enrollments.py b/common/djangoapps/student/tests/test_recent_enrollments.py index 618c378477..07516af01a 100644 --- a/common/djangoapps/student/tests/test_recent_enrollments.py +++ b/common/djangoapps/student/tests/test_recent_enrollments.py @@ -15,7 +15,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from course_modes.tests.factories import CourseModeFactory from student.models import CourseEnrollment, DashboardConfiguration -from student.views import get_course_enrollment_pairs, _get_recently_enrolled_courses +from student.views import get_course_enrollments, _get_recently_enrolled_courses # pylint: disable=protected-access @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') @@ -67,7 +67,7 @@ class TestRecentEnrollments(ModuleStoreTestCase): self._configure_message_timeout(60) # get courses through iterating all courses - courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + courses_list = list(get_course_enrollments(self.student, None, [])) self.assertEqual(len(courses_list), 2) recent_course_list = _get_recently_enrolled_courses(courses_list) @@ -78,7 +78,7 @@ class TestRecentEnrollments(ModuleStoreTestCase): Tests that the recent enrollment list is empty if configured to zero seconds. """ self._configure_message_timeout(0) - courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + courses_list = list(get_course_enrollments(self.student, None, [])) self.assertEqual(len(courses_list), 2) recent_course_list = _get_recently_enrolled_courses(courses_list) @@ -106,16 +106,16 @@ class TestRecentEnrollments(ModuleStoreTestCase): enrollment.save() courses.append(course) - courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + courses_list = list(get_course_enrollments(self.student, None, [])) self.assertEqual(len(courses_list), 6) recent_course_list = _get_recently_enrolled_courses(courses_list) self.assertEqual(len(recent_course_list), 5) - self.assertEqual(recent_course_list[1][0], courses[0]) - self.assertEqual(recent_course_list[2][0], courses[1]) - self.assertEqual(recent_course_list[3][0], courses[2]) - self.assertEqual(recent_course_list[4][0], courses[3]) + self.assertEqual(recent_course_list[1].course, courses[0]) + self.assertEqual(recent_course_list[2].course, courses[1]) + self.assertEqual(recent_course_list[3].course, courses[2]) + self.assertEqual(recent_course_list[4].course, courses[3]) def test_dashboard_rendering(self): """ diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 82f4900907..e1575da5e9 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -1,9 +1,6 @@ # -*- coding: utf-8 -*- """ -This file demonstrates writing tests using the unittest module. These will pass -when you run "manage.py test". - -Replace this with more appropriate tests for your application. +Miscellaneous tests for the student app. """ from datetime import datetime, timedelta import logging @@ -28,8 +25,8 @@ from student.views import (process_survey_link, _cert_info, from student.tests.factories import UserFactory, CourseModeFactory from util.testing import EventTestMixin from util.model_utils import USER_SETTINGS_CHANGED_EVENT_NAME -from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, ModuleStoreEnum # These imports refer to lms djangoapps. # Their testcases are only run under lms. @@ -193,6 +190,7 @@ class CourseEndingTest(TestCase): self.assertIsNone(_cert_info(user, course2, cert_status, course_mode)) +@ddt.ddt class DashboardTest(ModuleStoreTestCase): """ Tests for dashboard utility functions @@ -487,6 +485,48 @@ class DashboardTest(ModuleStoreTestCase): ) self.assertContains(response, expected_url) + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') + @ddt.data((ModuleStoreEnum.Type.mongo, 1), (ModuleStoreEnum.Type.split, 3)) + @ddt.unpack + def test_dashboard_metadata_caching(self, modulestore_type, expected_mongo_calls): + """ + Check that the student dashboard makes use of course metadata caching. + + The first time the student dashboard displays a specific course, it will + make a call to the module store. After that first request, though, the + course's metadata should be cached as a CourseOverview. + + Arguments: + modulestore_type (ModuleStoreEnum.Type): Type of modulestore to create + test course in. + expected_mongo_calls (int >=0): Number of MongoDB queries expected for + a single call to the module store. + + Note to future developers: + If you break this test so that the "check_mongo_calls(0)" fails, + please do NOT change it to "check_mongo_calls(n>1)". Instead, change + your code to not load courses from the module store. This may + involve adding fields to CourseOverview so that loading a full + CourseDescriptor isn't necessary. + """ + # Create a course, log in the user, and enroll them in the course. + test_course = CourseFactory.create(default_store=modulestore_type) + self.client.login(username="jack", password="test") + CourseEnrollment.enroll(self.user, test_course.id) + + # The first request will result in a modulestore query. + with check_mongo_calls(expected_mongo_calls): + response_1 = self.client.get(reverse('dashboard')) + self.assertEquals(response_1.status_code, 200) + + # Subsequent requests will only result in SQL queries to load the + # CourseOverview object that has been created. + with check_mongo_calls(0): + response_2 = self.client.get(reverse('dashboard')) + self.assertEquals(response_2.status_code, 200) + response_3 = self.client.get(reverse('dashboard')) + self.assertEquals(response_3.status_code, 200) + class UserSettingsEventTestMixin(EventTestMixin): """ diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 5a4e3aafb3..6a827fcd99 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -4,7 +4,6 @@ Student Views import datetime import logging import uuid -import time import json import warnings from datetime import timedelta @@ -115,7 +114,6 @@ from student.helpers import ( ) from student.cookies import set_logged_in_cookies, delete_logged_in_cookies from student.models import anonymous_id_for_user -from xmodule.error_module import ErrorDescriptor from shoppingcart.models import DonationConfiguration, CourseRegistrationCode from embargo import api as embargo_api @@ -185,33 +183,42 @@ def process_survey_link(survey_link, user): return survey_link.format(UNIQUE_ID=unique_id_for_user(user)) -def cert_info(user, course, course_mode): +def cert_info(user, course_overview, course_mode): """ Get the certificate info needed to render the dashboard section for the given - student and course. Returns a dictionary with keys: + student and course. - 'status': one of 'generating', 'ready', 'notpassing', 'processing', 'restricted' - 'show_download_url': bool - 'download_url': url, only present if show_download_url is True - 'show_disabled_download_button': bool -- true if state is 'generating' - 'show_survey_button': bool - 'survey_url': url, only if show_survey_button is True - 'grade': if status is not 'processing' + Arguments: + user (User): A user. + course_overview (CourseOverview): A course. + course_mode (str): The enrollment mode (honor, verified, audit, etc.) + + Returns: + dict: A dictionary with keys: + 'status': one of 'generating', 'ready', 'notpassing', 'processing', 'restricted' + 'show_download_url': bool + 'download_url': url, only present if show_download_url is True + 'show_disabled_download_button': bool -- true if state is 'generating' + 'show_survey_button': bool + 'survey_url': url, only if show_survey_button is True + 'grade': if status is not 'processing' """ - if not course.may_certify(): + if not course_overview.may_certify(): return {} - - return _cert_info(user, course, certificate_status_for_student(user, course.id), course_mode) + return _cert_info( + user, + course_overview, + certificate_status_for_student(user, course_overview.id), + course_mode + ) -def reverification_info(course_enrollment_pairs, user, statuses): +def reverification_info(statuses): """ Returns reverification-related information for *all* of user's enrollments whose - reverification status is in status_list + reverification status is in statuses. Args: - course_enrollment_pairs (list): list of (course, enrollment) tuples - user (User): the user whose information we want statuses (list): a list of reverification statuses we want information for example: ["must_reverify", "denied"] @@ -229,39 +236,56 @@ def reverification_info(course_enrollment_pairs, user, statuses): return reverifications -def get_course_enrollment_pairs(user, course_org_filter, org_filter_out_set): +def get_course_enrollments(user, org_to_include, orgs_to_exclude): """ - Get the relevant set of (Course, CourseEnrollment) pairs to be displayed on - a student's dashboard. + Given a user, return a filtered set of his or her course enrollments. + + Arguments: + user (User): the user in question. + org_to_include (str): for use in Microsites. If not None, ONLY courses + of this org will be returned. + orgs_to_exclude (list[str]): If org_to_include is not None, this + argument is ignored. Else, courses of this org will be excluded. + + Returns: + generator[CourseEnrollment]: a sequence of enrollments to be displayed + on the user's dashboard. """ for enrollment in CourseEnrollment.enrollments_for_user(user): - store = modulestore() - with store.bulk_operations(enrollment.course_id): - course = store.get_course(enrollment.course_id) - if course and not isinstance(course, ErrorDescriptor): - # if we are in a Microsite, then filter out anything that is not - # attributed (by ORG) to that Microsite - if course_org_filter and course_org_filter != course.location.org: - continue - # Conversely, if we are not in a Microsite, then let's filter out any enrollments - # with courses attributed (by ORG) to Microsites - elif course.location.org in org_filter_out_set: - continue + # If the course is missing or broken, log an error and skip it. + course_overview = enrollment.course_overview + if not course_overview: + log.error( + "User %s enrolled in broken or non-existent course %s", + user.username, + enrollment.course_id + ) + continue - yield (course, enrollment) - else: - log.error( - u"User %s enrolled in %s course %s", - user.username, - "broken" if course else "non-existent", - enrollment.course_id - ) + # If we are in a Microsite, then filter out anything that is not + # attributed (by ORG) to that Microsite. + if org_to_include and course_overview.location.org != org_to_include: + continue + + # Conversely, if we are not in a Microsite, then filter out any enrollments + # with courses attributed (by ORG) to Microsites. + elif course_overview.location.org in orgs_to_exclude: + continue + + # Else, include the enrollment. + else: + yield enrollment -def _cert_info(user, course, cert_status, course_mode): +def _cert_info(user, course_overview, cert_status, course_mode): # pylint: disable=unused-argument """ Implements the logic for cert_info -- split out for testing. + + Arguments: + user (User): A user. + course_overview (CourseOverview): A course. + course_mode (str): The enrollment mode (honor, verified, audit, etc.) """ # simplify the status for the template using this lookup table template_state = { @@ -285,7 +309,7 @@ def _cert_info(user, course, cert_status, course_mode): is_hidden_status = cert_status['status'] in ('unavailable', 'processing', 'generating', 'notpassing') - if course.certificates_display_behavior == 'early_no_info' and is_hidden_status: + if course_overview.certificates_display_behavior == 'early_no_info' and is_hidden_status: return None status = template_state.get(cert_status['status'], default_status) @@ -299,20 +323,20 @@ def _cert_info(user, course, cert_status, course_mode): } if (status in ('generating', 'ready', 'notpassing', 'restricted') and - course.end_of_course_survey_url is not None): + course_overview.end_of_course_survey_url is not None): status_dict.update({ 'show_survey_button': True, - 'survey_url': process_survey_link(course.end_of_course_survey_url, user)}) + 'survey_url': process_survey_link(course_overview.end_of_course_survey_url, user)}) else: status_dict['show_survey_button'] = False if status == 'ready': # showing the certificate web view button if certificate is ready state and feature flags are enabled. - if has_html_certificates_enabled(course.id, course): - if get_active_web_certificate(course) is not None: + if has_html_certificates_enabled(course_overview.id, course_overview): + if course_overview.has_any_active_web_certificate: certificate_url = get_certificate_url( user_id=user.id, - course_id=unicode(course.id) + course_id=unicode(course_overview.id), ) status_dict.update({ 'show_cert_web_view': True, @@ -325,7 +349,7 @@ def _cert_info(user, course, cert_status, course_mode): log.warning( u"User %s has a downloadable cert for %s, but no download url", user.username, - course.id + course_overview.id ) return default_info else: @@ -337,8 +361,8 @@ def _cert_info(user, course, cert_status, course_mode): linkedin_config = LinkedInAddToProfileConfiguration.current() if linkedin_config.enabled: status_dict['linked_in_url'] = linkedin_config.add_to_profile_url( - course.id, - course.display_name, + course_overview.id, + course_overview.display_name, cert_status.get('mode'), cert_status['download_url'] ) @@ -506,13 +530,13 @@ def dashboard(request): # Build our (course, enrollment) list for the user, but ignore any courses that no # longer exist (because the course IDs have changed). Still, we don't delete those # enrollments, because it could have been a data push snafu. - course_enrollment_pairs = list(get_course_enrollment_pairs(user, course_org_filter, org_filter_out_set)) + course_enrollments = list(get_course_enrollments(user, course_org_filter, org_filter_out_set)) # sort the enrollment pairs by the enrollment date - course_enrollment_pairs.sort(key=lambda x: x[1].created, reverse=True) + course_enrollments.sort(key=lambda x: x.created, reverse=True) # Retrieve the course modes for each course - enrolled_course_ids = [course.id for course, __ in course_enrollment_pairs] + enrolled_course_ids = [enrollment.course_id for enrollment in course_enrollments] all_course_modes, unexpired_course_modes = CourseMode.all_and_unexpired_modes_for_courses(enrolled_course_ids) course_modes_by_course = { course_id: { @@ -525,14 +549,9 @@ def dashboard(request): # Check to see if the student has recently enrolled in a course. # If so, display a notification message confirming the enrollment. enrollment_message = _create_recent_enrollment_message( - course_enrollment_pairs, course_modes_by_course + course_enrollments, course_modes_by_course ) - # Retrieve the course modes for each course - enrolled_courses_dict = {} - for course, __ in course_enrollment_pairs: - enrolled_courses_dict[unicode(course.id)] = course - course_optouts = Optout.objects.filter(user=user).values_list('course_id', flat=True) message = "" @@ -551,20 +570,20 @@ def dashboard(request): errored_courses = modulestore().get_errored_courses() show_courseware_links_for = frozenset( - course.id for course, _enrollment in course_enrollment_pairs - if has_access(request.user, 'load', course) - and has_access(request.user, 'view_courseware_with_prerequisites', course) + enrollment.course_id for enrollment in course_enrollments + if has_access(request.user, 'load', enrollment.course_overview) + and has_access(request.user, 'view_courseware_with_prerequisites', enrollment.course_overview) ) # Construct a dictionary of course mode information # used to render the course list. We re-use the course modes dict # we loaded earlier to avoid hitting the database. course_mode_info = { - course.id: complete_course_mode_info( - course.id, enrollment, - modes=course_modes_by_course[course.id] + enrollment.course_id: complete_course_mode_info( + enrollment.course_id, enrollment, + modes=course_modes_by_course[enrollment.course_id] ) - for course, enrollment in course_enrollment_pairs + for enrollment in course_enrollments } # Determine the per-course verification status @@ -583,20 +602,20 @@ def dashboard(request): # there is no verification messaging to display. verify_status_by_course = check_verify_status_by_course( user, - course_enrollment_pairs, + course_enrollments, all_course_modes ) cert_statuses = { - course.id: cert_info(request.user, course, _enrollment.mode) - for course, _enrollment in course_enrollment_pairs + enrollment.course_id: cert_info(request.user, enrollment.course_overview, enrollment.mode) + for enrollment in course_enrollments } # only show email settings for Mongo course and when bulk email is turned on show_email_settings_for = frozenset( - course.id for course, _enrollment in course_enrollment_pairs if ( + enrollment.course_id for enrollment in course_enrollments if ( settings.FEATURES['ENABLE_INSTRUCTOR_EMAIL'] and - modulestore().get_modulestore_type(course.id) != ModuleStoreEnum.Type.xml and - CourseAuthorization.instructor_email_enabled(course.id) + modulestore().get_modulestore_type(enrollment.course_id) != ModuleStoreEnum.Type.xml and + CourseAuthorization.instructor_email_enabled(enrollment.course_id) ) ) @@ -606,16 +625,29 @@ def dashboard(request): # Gets data for midcourse reverifications, if any are necessary or have failed statuses = ["approved", "denied", "pending", "must_reverify"] - reverifications = reverification_info(course_enrollment_pairs, user, statuses) + reverifications = reverification_info(statuses) - show_refund_option_for = frozenset(course.id for course, _enrollment in course_enrollment_pairs - if _enrollment.refundable()) + show_refund_option_for = frozenset( + enrollment.course_id for enrollment in course_enrollments + if enrollment.refundable() + ) - block_courses = frozenset(course.id for course, enrollment in course_enrollment_pairs - if is_course_blocked(request, CourseRegistrationCode.objects.filter(course_id=course.id, registrationcoderedemption__redeemed_by=request.user), course.id)) + block_courses = frozenset( + enrollment.course_id for enrollment in course_enrollments + if is_course_blocked( + request, + CourseRegistrationCode.objects.filter( + course_id=enrollment.course_id, + registrationcoderedemption__redeemed_by=request.user + ), + enrollment.course_id + ) + ) - enrolled_courses_either_paid = frozenset(course.id for course, _enrollment in course_enrollment_pairs - if _enrollment.is_paid_course()) + enrolled_courses_either_paid = frozenset( + enrollment.course_id for enrollment in course_enrollments + if enrollment.is_paid_course() + ) # If there are *any* denied reverifications that have not been toggled off, # we'll display the banner @@ -625,8 +657,10 @@ def dashboard(request): order_history_list = order_history(user, course_org_filter=course_org_filter, org_filter_out_set=org_filter_out_set) # get list of courses having pre-requisites yet to be completed - courses_having_prerequisites = frozenset(course.id for course, _enrollment in course_enrollment_pairs - if course.pre_requisite_courses) + courses_having_prerequisites = frozenset( + enrollment.course_id for enrollment in course_enrollments + if enrollment.course_overview.pre_requisite_courses + ) courses_requirements_not_met = get_pre_requisite_courses_not_completed(user, courses_having_prerequisites) ccx_membership_triplets = [] @@ -638,7 +672,7 @@ def dashboard(request): context = { 'enrollment_message': enrollment_message, - 'course_enrollment_pairs': course_enrollment_pairs, + 'course_enrollments': course_enrollments, 'course_optouts': course_optouts, 'message': message, 'staff_access': staff_access, @@ -646,7 +680,7 @@ def dashboard(request): 'show_courseware_links_for': show_courseware_links_for, 'all_course_modes': course_mode_info, 'cert_statuses': cert_statuses, - 'credit_statuses': _credit_statuses(user, course_enrollment_pairs), + 'credit_statuses': _credit_statuses(user, course_enrollments), 'show_email_settings_for': show_email_settings_for, 'reverifications': reverifications, 'verification_status': verification_status, @@ -669,13 +703,15 @@ def dashboard(request): return render_to_response('dashboard.html', context) -def _create_recent_enrollment_message(course_enrollment_pairs, course_modes): - """Builds a recent course enrollment message +def _create_recent_enrollment_message(course_enrollments, course_modes): # pylint: disable=invalid-name + """ + Builds a recent course enrollment message. - Constructs a new message template based on any recent course enrollments for the student. + Constructs a new message template based on any recent course enrollments + for the student. Args: - course_enrollment_pairs (list): A list of tuples containing courses, and the associated enrollment information. + course_enrollments (list[CourseEnrollment]): a list of course enrollments. course_modes (dict): Mapping of course ID's to course mode dictionaries. Returns: @@ -683,16 +719,16 @@ def _create_recent_enrollment_message(course_enrollment_pairs, course_modes): None if there are no recently enrolled courses. """ - recently_enrolled_courses = _get_recently_enrolled_courses(course_enrollment_pairs) + recently_enrolled_courses = _get_recently_enrolled_courses(course_enrollments) if recently_enrolled_courses: messages = [ { - "course_id": course.id, - "course_name": course.display_name, - "allow_donation": _allow_donation(course_modes, course.id, enrollment) + "course_id": enrollment.course_overview.id, + "course_name": enrollment.course_overview.display_name, + "allow_donation": _allow_donation(course_modes, enrollment.course_overview.id, enrollment) } - for course, enrollment in recently_enrolled_courses + for enrollment in recently_enrolled_courses ] platform_name = microsite.get_value('platform_name', settings.PLATFORM_NAME) @@ -703,22 +739,20 @@ def _create_recent_enrollment_message(course_enrollment_pairs, course_modes): ) -def _get_recently_enrolled_courses(course_enrollment_pairs): - """Checks to see if the student has recently enrolled in courses. - - Checks to see if any of the enrollments in the course_enrollment_pairs have been recently created and activated. +def _get_recently_enrolled_courses(course_enrollments): + """ + Given a list of enrollments, filter out all but recent enrollments. Args: - course_enrollment_pairs (list): A list of tuples containing courses, and the associated enrollment information. + course_enrollments (list[CourseEnrollment]): A list of course enrollments. Returns: - A list of courses - + list[CourseEnrollment]: A list of recent course enrollments. """ seconds = DashboardConfiguration.current().recent_enrollment_time_delta time_delta = (datetime.datetime.now(UTC) - datetime.timedelta(seconds=seconds)) return [ - (course, enrollment) for course, enrollment in course_enrollment_pairs + enrollment for enrollment in course_enrollments # If the enrollment has no created date, we are explicitly excluding the course # from the list of recent enrollments. if enrollment.is_active and enrollment.created > time_delta @@ -752,7 +786,7 @@ def _update_email_opt_in(request, org): preferences_api.update_email_opt_in(request.user, org, email_opt_in_boolean) -def _credit_statuses(user, course_enrollment_pairs): +def _credit_statuses(user, course_enrollments): """ Retrieve the status for credit courses. @@ -768,7 +802,8 @@ def _credit_statuses(user, course_enrollment_pairs): Arguments: user (User): The currently logged-in user. - course_enrollment_pairs (list): List of (Course, CourseEnrollment) tuples. + course_enrollments (list[CourseEnrollment]): List of enrollments for the + user. Returns: dict @@ -785,7 +820,7 @@ def _credit_statuses(user, course_enrollment_pairs): so the user should contact the support team. Example: - >>> _credit_statuses(user, course_enrollment_pairs) + >>> _credit_statuses(user, course_enrollments) { CourseKey.from_string("edX/DemoX/Demo_Course"): { "course_key": "edX/DemoX/Demo_Course", @@ -812,8 +847,8 @@ def _credit_statuses(user, course_enrollment_pairs): } credit_enrollments = { - course.id: enrollment - for course, enrollment in course_enrollment_pairs + enrollment.course_id: enrollment + for enrollment in course_enrollments if enrollment.mode == "credit" } diff --git a/common/djangoapps/util/milestones_helpers.py b/common/djangoapps/util/milestones_helpers.py index b0fe8b069c..d1514d0dee 100644 --- a/common/djangoapps/util/milestones_helpers.py +++ b/common/djangoapps/util/milestones_helpers.py @@ -9,6 +9,7 @@ from django.utils.translation import ugettext as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from xmodule.modulestore.django import modulestore NAMESPACE_CHOICES = { @@ -86,33 +87,46 @@ def set_prerequisite_courses(course_key, prerequisite_course_keys): add_prerequisite_course(course_key, prerequisite_course_key) -def get_pre_requisite_courses_not_completed(user, enrolled_courses): +def get_pre_requisite_courses_not_completed(user, enrolled_courses): # pylint: disable=invalid-name """ - It would make dict of prerequisite courses not completed by user among courses - user has enrolled in. It calls the fulfilment api of milestones app and - iterates over all fulfilment milestones not achieved to make dict of - prerequisite courses yet to be completed. - """ - pre_requisite_courses = {} - if settings.FEATURES.get('ENABLE_PREREQUISITE_COURSES', False): - from milestones import api as milestones_api - for course_key in enrolled_courses: - required_courses = [] - fulfilment_paths = milestones_api.get_course_milestones_fulfillment_paths(course_key, {'id': user.id}) - for milestone_key, milestone_value in fulfilment_paths.items(): # pylint: disable=unused-variable - for key, value in milestone_value.items(): - if key == 'courses' and value: - for required_course in value: - required_course_key = CourseKey.from_string(required_course) - required_course_descriptor = modulestore().get_course(required_course_key) - required_courses.append({ - 'key': required_course_key, - 'display': get_course_display_name(required_course_descriptor) - }) + Makes a dict mapping courses to their unfulfilled milestones using the + fulfillment API of the milestones app. + + Arguments: + user (User): the user for whom we are checking prerequisites. + enrolled_courses (CourseKey): a list of keys for the courses to be + checked. The given user must be enrolled in all of these courses. + + Returns: + dict[CourseKey: dict[ + 'courses': list[dict['key': CourseKey, 'display': str]] + ]] + If a course has no incomplete prerequisites, it will be excluded from the + dictionary. + """ + if not settings.FEATURES.get('ENABLE_PREREQUISITE_COURSES', False): + return {} + + from milestones import api as milestones_api + pre_requisite_courses = {} + + for course_key in enrolled_courses: + required_courses = [] + fulfillment_paths = milestones_api.get_course_milestones_fulfillment_paths(course_key, {'id': user.id}) + for __, milestone_value in fulfillment_paths.items(): + for key, value in milestone_value.items(): + if key == 'courses' and value: + for required_course in value: + required_course_key = CourseKey.from_string(required_course) + required_course_overview = CourseOverview.get_from_id(required_course_key) + required_courses.append({ + 'key': required_course_key, + 'display': get_course_display_string(required_course_overview) + }) + # If there are required courses, add them to the result dict. + if required_courses: + pre_requisite_courses[course_key] = {'courses': required_courses} - # if there are required courses add to dict - if required_courses: - pre_requisite_courses[course_key] = {'courses': required_courses} return pre_requisite_courses @@ -129,15 +143,18 @@ def get_prerequisite_courses_display(course_descriptor): required_course_descriptor = modulestore().get_course(course_key) prc = { 'key': course_key, - 'display': get_course_display_name(required_course_descriptor) + 'display': get_course_display_string(required_course_descriptor) } pre_requisite_courses.append(prc) return pre_requisite_courses -def get_course_display_name(descriptor): +def get_course_display_string(descriptor): """ - It would return display name from given course descriptor + Returns a string to display for a course or course overview. + + Arguments: + descriptor (CourseDescriptor|CourseOverview): a course or course overview. """ return ' '.join([ descriptor.display_org_with_default, diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index 6487f407fa..3691ac4d45 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -12,6 +12,7 @@ from django.core.urlresolvers import reverse from eventtracking import tracker from opaque_keys.edx.keys import CourseKey +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from xmodule.modulestore.django import modulestore from certificates.models import ( @@ -216,13 +217,18 @@ def generate_example_certificates(course_key): def has_html_certificates_enabled(course_key, course=None): """ - It determines if course has html certificates enabled + Determine if a course has html certificates enabled. + + Arguments: + course_key (CourseKey|str): A course key or a string representation + of one. + course (CourseDescriptor|CourseOverview): A course. """ html_certificates_enabled = False try: if not isinstance(course_key, CourseKey): course_key = CourseKey.from_string(course_key) - course = course if course else modulestore().get_course(course_key, depth=0) + course = course if course else CourseOverview.get_from_id(course_key) if settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False) and course.cert_html_view_enabled: html_certificates_enabled = True except: # pylint: disable=bare-except diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 3d41f5525d..5713e4cbac 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -33,6 +33,7 @@ from xmodule.util.django import get_current_request_hostname from external_auth.models import ExternalAuthMap from courseware.masquerade import get_masquerade_role, is_masquerading_as_student +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from student import auth from student.models import CourseEnrollmentAllowed from student.roles import ( @@ -100,6 +101,9 @@ def has_access(user, action, obj, course_key=None): if isinstance(obj, CourseDescriptor): return _has_access_course_desc(user, action, obj) + if isinstance(obj, CourseOverview): + return _has_access_course_overview(user, action, obj) + if isinstance(obj, ErrorDescriptor): return _has_access_error_desc(user, action, obj, course_key) @@ -129,6 +133,87 @@ def has_access(user, action, obj, course_key=None): # ================ Implementation helpers ================================ +def _can_access_descriptor_with_start_date(user, descriptor, course_key): # pylint: disable=invalid-name + """ + Checks if a user has access to a descriptor based on its start date. + + If there is no start date specified, grant access. + Else, check if we're past the start date. + + Note: + We do NOT check whether the user is staff or if the descriptor + is detached... it is assumed both of these are checked by the caller. + + Arguments: + user (User): the user whose descriptor access we are checking. + descriptor (AType): the descriptor for which we are checking access. + where AType is any descriptor that has the attributes .location and + .days_early_for_beta + """ + start_dates_disabled = settings.FEATURES['DISABLE_START_DATES'] + if start_dates_disabled and not is_masquerading_as_student(user, course_key): + return True + else: + now = datetime.now(UTC()) + effective_start = _adjust_start_date_for_beta_testers( + user, + descriptor, + course_key=course_key + ) + return ( + descriptor.start is None + or now > effective_start + or in_preview_mode() + ) + + +def _can_view_courseware_with_prerequisites(user, course): # pylint: disable=invalid-name + """ + Checks if a user has access to a course based on its prerequisites. + + If the user is staff or anonymous, immediately grant access. + Else, return whether or not the prerequisite courses have been passed. + + Arguments: + user (User): the user whose course access we are checking. + course (AType): the course for which we are checking access. + where AType is CourseDescriptor, CourseOverview, or any other class that + represents a course and has the attributes .location and .id. + """ + return ( + not settings.FEATURES['ENABLE_PREREQUISITE_COURSES'] + or _has_staff_access_to_descriptor(user, course, course.id) + or not course.pre_requisite_courses + or user.is_anonymous() + or not get_pre_requisite_courses_not_completed(user, [course.id]) + ) + + +def _can_load_course_on_mobile(user, course): + """ + Checks if a user can view the given course on a mobile device. + + This function only checks mobile-specific access restrictions. Other access + restrictions such as start date and the .visible_to_staff_only flag must + be checked by callers in *addition* to the return value of this function. + + Arguments: + user (User): the user whose course access we are checking. + course (CourseDescriptor|CourseOverview): the course for which we are + checking access. + + Returns: + bool: whether the course can be accessed on mobile. + """ + return ( + is_mobile_available_for_user(user, course) and + ( + _has_staff_access_to_descriptor(user, course, course.id) or + not any_unfulfilled_milestones(course.id, user.id) + ) + ) + + def _has_access_course_desc(user, action, course): """ Check if user has access to a course descriptor. @@ -154,23 +239,6 @@ def _has_access_course_desc(user, action, course): # delegate to generic descriptor check to check start dates return _has_access_descriptor(user, 'load', course, course.id) - def can_load_mobile(): - """ - Can this user access this course from a mobile device? - """ - return ( - # check start date - can_load() and - # check mobile_available flag - is_mobile_available_for_user(user, course) and - ( - # either is a staff user or - _has_staff_access_to_descriptor(user, course, course.id) or - # check for unfulfilled milestones - not any_unfulfilled_milestones(course.id, user.id) - ) - ) - def can_enroll(): """ First check if restriction of enrollment by login method is enabled, both @@ -274,25 +342,11 @@ def _has_access_course_desc(user, action, course): _has_staff_access_to_descriptor(user, course, course.id) ) - def can_view_courseware_with_prerequisites(): # pylint: disable=invalid-name - """ - Checks if prerequisite courses feature is enabled and course has prerequisites - and user is neither staff nor anonymous then it returns False if user has not - passed prerequisite courses otherwise return True. - """ - if settings.FEATURES['ENABLE_PREREQUISITE_COURSES'] \ - and not _has_staff_access_to_descriptor(user, course, course.id) \ - and course.pre_requisite_courses \ - and not user.is_anonymous() \ - and get_pre_requisite_courses_not_completed(user, [course.id]): - return False - else: - return True - checkers = { 'load': can_load, - 'view_courseware_with_prerequisites': can_view_courseware_with_prerequisites, - 'load_mobile': can_load_mobile, + '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), 'enroll': can_enroll, 'see_exists': see_exists, 'staff': lambda: _has_staff_access_to_descriptor(user, course, course.id), @@ -304,6 +358,51 @@ def _has_access_course_desc(user, action, course): 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. + """ + return ( + not course_overview.visible_to_staff_only + and _can_access_descriptor_with_start_date(user, course_overview, course_overview.id) + ) or _has_staff_access_to_descriptor(user, course_overview, course_overview.id) + + +_COURSE_OVERVIEW_CHECKERS = { + '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() # pylint: disable=invalid-name + + +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)) + + def _has_access_error_desc(user, action, descriptor, course_key): """ Only staff should see error descriptors. @@ -408,38 +507,14 @@ def _has_access_descriptor(user, action, descriptor, course_key=None): students to see modules. If not, views should check the course, so we don't have to hit the enrollments table on every module load. """ - if descriptor.visible_to_staff_only and not _has_staff_access_to_descriptor(user, descriptor, course_key): - return False - - # enforce group access - if not _has_group_access(descriptor, user, course_key): - # if group_access check failed, deny access unless the requestor is staff, - # in which case immediately grant access. - return _has_staff_access_to_descriptor(user, descriptor, course_key) - - # If start dates are off, can always load - if settings.FEATURES['DISABLE_START_DATES'] and not is_masquerading_as_student(user, course_key): - debug("Allow: DISABLE_START_DATES") - return True - - # Check start date - if 'detached' not in descriptor._class_tags and descriptor.start is not None: - now = datetime.now(UTC()) - effective_start = _adjust_start_date_for_beta_testers( - user, - descriptor, - course_key=course_key + return ( + not descriptor.visible_to_staff_only + and _has_group_access(descriptor, user, course_key) + and ( + 'detached' in descriptor._class_tags # pylint: disable=protected-access + or _can_access_descriptor_with_start_date(user, descriptor, course_key) ) - if in_preview_mode() or now > effective_start: - # after start date, everyone can see it - debug("Allow: now > effective start date") - return True - # otherwise, need staff access - return _has_staff_access_to_descriptor(user, descriptor, course_key) - - # No start date, so can always load. - debug("Allow: no start date") - return True + ) or _has_staff_access_to_descriptor(user, descriptor, course_key) checkers = { 'load': can_load, @@ -700,4 +775,4 @@ def in_preview_mode(): Returns whether the user is in preview mode or not. """ hostname = get_current_request_hostname() - return hostname and settings.PREVIEW_DOMAIN in hostname.split('.') + return bool(hostname and settings.PREVIEW_DOMAIN in hostname.split('.')) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 43e9b9b791..20f7db7fcf 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -152,6 +152,16 @@ 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(course, section_key): """ This returns the snippet of html to be rendered on the course about page, @@ -227,7 +237,7 @@ def get_course_about_section(course, section_key): elif section_key == "title": return course.display_name_with_default elif section_key == "university": - return course.display_org_with_default + return get_course_university_about_section(course) elif section_key == "number": return course.display_number_with_default diff --git a/lms/djangoapps/courseware/tests/helpers.py b/lms/djangoapps/courseware/tests/helpers.py index 5cea78277b..a72b5c2cdf 100644 --- a/lms/djangoapps/courseware/tests/helpers.py +++ b/lms/djangoapps/courseware/tests/helpers.py @@ -5,6 +5,8 @@ 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 openedx.core.djangoapps.content.course_overviews.models import CourseOverview from student.models import Registration @@ -137,3 +139,50 @@ class LoginEnrollmentTestCase(TestCase): 'course_id': course.id.to_deprecated_string(), } self.assert_request_status_code(200, url, method="POST", data=request_data) + + +class CourseAccessTestMixin(TestCase): + """ + Utility mixin for asserting access (or lack thereof) to courses. + If relevant, also checks access for courses' corresponding CourseOverviews. + """ + + def assertCanAccessCourse(self, user, action, course): + """ + 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. + + 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))) + + 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. + + Arguments: + user (User): a user. + action (str): type of access to test. + See access.py:COURSE_OVERVIEW_SUPPORTED_ACTIONS. + course (CourseDescriptor): a course. + + Note: + It may seem redundant to have one method for testing access + and another method for testing lack thereof (why not just combine + them into one method with a boolean flag?), but it makes reading + 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))) diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 8b7fdf46c5..8c2715c9e5 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -1,4 +1,6 @@ import datetime +import ddt +import itertools import pytz from django.test import TestCase @@ -9,8 +11,9 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey import courseware.access as access from courseware.masquerade import CourseMasquerade -from courseware.tests.factories import UserFactory, StaffFactory, InstructorFactory +from courseware.tests.factories import UserFactory, StaffFactory, InstructorFactory, BetaTesterFactory from courseware.tests.helpers import LoginEnrollmentTestCase +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from student.tests.factories import AnonymousUserFactory, CourseEnrollmentAllowedFactory, CourseEnrollmentFactory from xmodule.course_module import ( CATALOG_VISIBILITY_CATALOG_AND_ABOUT, CATALOG_VISIBILITY_ABOUT, @@ -18,6 +21,7 @@ from xmodule.course_module import ( ) from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from util.milestones_helpers import fulfill_course_milestone from util.milestones_helpers import ( set_prerequisite_courses, @@ -424,3 +428,91 @@ class UserRoleTestCase(TestCase): 'student', access.get_user_role(self.anonymous_user, self.course_key) ) + + +@ddt.ddt +class CourseOverviewAccessTestCase(ModuleStoreTestCase): + """ + Tests confirming that has_access works equally on CourseDescriptors and + CourseOverviews. + """ + + def setUp(self): + super(CourseOverviewAccessTestCase, self).setUp() + + today = datetime.datetime.now(pytz.UTC) + last_week = today - datetime.timedelta(days=7) + next_week = today + datetime.timedelta(days=7) + + self.course_default = CourseFactory.create() + self.course_started = CourseFactory.create(start=last_week) + self.course_not_started = CourseFactory.create(start=next_week, days_early_for_beta=10) + self.course_staff_only = CourseFactory.create(visible_to_staff_only=True) + self.course_mobile_available = CourseFactory.create(mobile_available=True) + self.course_with_pre_requisite = CourseFactory.create( + pre_requisite_courses=[str(self.course_started.id)] + ) + self.course_with_pre_requisites = CourseFactory.create( + pre_requisite_courses=[str(self.course_started.id), str(self.course_not_started.id)] + ) + + self.user_normal = UserFactory.create() + self.user_beta_tester = BetaTesterFactory.create(course_key=self.course_not_started.id) + self.user_completed_pre_requisite = UserFactory.create() # pylint: disable=invalid-name + fulfill_course_milestone(self.user_completed_pre_requisite, self.course_started.id) + self.user_staff = UserFactory.create(is_staff=True) + self.user_anonymous = AnonymousUserFactory.create() + + LOAD_TEST_DATA = list(itertools.product( + ['user_normal', 'user_beta_tester', 'user_staff'], + ['load'], + ['course_default', 'course_started', 'course_not_started', 'course_staff_only'], + )) + + LOAD_MOBILE_TEST_DATA = list(itertools.product( + ['user_normal', 'user_staff'], + ['load_mobile'], + ['course_default', 'course_mobile_available'], + )) + + PREREQUISITES_TEST_DATA = list(itertools.product( + ['user_normal', 'user_completed_pre_requisite', 'user_staff', 'user_anonymous'], + ['view_courseware_with_prerequisites'], + ['course_default', 'course_with_pre_requisite', 'course_with_pre_requisites'], + )) + + @ddt.data(*(LOAD_TEST_DATA + LOAD_MOBILE_TEST_DATA + PREREQUISITES_TEST_DATA)) + @ddt.unpack + def test_course_overview_access(self, user_attr_name, action, course_attr_name): + """ + Check that a user's access to a course is equal to the user's access to + the corresponding course overview. + + Instead of taking a user and course directly as arguments, we have to + take their attribute names, as ddt doesn't allow us to reference self. + + Arguments: + 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. + """ + user = getattr(self, user_attr_name) + course = getattr(self, course_attr_name) + + course_overview = CourseOverview.get_from_id(course.id) + self.assertEqual( + access.has_access(user, action, course, course_key=course.id), + access.has_access(user, action, course_overview, course_key=course.id) + ) + + def test_course_overivew_unsupported_action(self): + """ + Check that calling has_access with an unsupported action raises a + ValueError. + """ + overview = CourseOverview.get_from_id(self.course_default.id) + with self.assertRaises(ValueError): + access.has_access(self.user, '_non_existent_action', overview) diff --git a/lms/djangoapps/courseware/tests/test_view_authentication.py b/lms/djangoapps/courseware/tests/test_view_authentication.py index 8d85111ed5..52cf037a9e 100644 --- a/lms/djangoapps/courseware/tests/test_view_authentication.py +++ b/lms/djangoapps/courseware/tests/test_view_authentication.py @@ -6,7 +6,7 @@ from mock import patch from nose.plugins.attrib import attr from courseware.access import has_access -from courseware.tests.helpers import LoginEnrollmentTestCase +from courseware.tests.helpers import CourseAccessTestMixin, LoginEnrollmentTestCase from courseware.tests.factories import ( BetaTesterFactory, StaffFactory, @@ -389,7 +389,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): @attr('shard_1') -class TestBetatesterAccess(ModuleStoreTestCase): +class TestBetatesterAccess(ModuleStoreTestCase, CourseAccessTestMixin): """ Tests for the beta tester feature """ @@ -411,12 +411,8 @@ class TestBetatesterAccess(ModuleStoreTestCase): Check that beta-test access works for courses. """ self.assertFalse(self.course.has_started()) - - # student user shouldn't see it - self.assertFalse(has_access(self.normal_student, 'load', self.course)) - - # now the student should see it - self.assertTrue(has_access(self.beta_tester, 'load', self.course)) + self.assertCannotAccessCourse(self.normal_student, 'load', self.course) + self.assertCanAccessCourse(self.beta_tester, 'load', self.course) @patch.dict('courseware.access.settings.FEATURES', {'DISABLE_START_DATES': False}) def test_content_beta_period(self): diff --git a/lms/templates/ccx/_dashboard_ccx_listing.html b/lms/templates/ccx/_dashboard_ccx_listing.html index 5856238498..c9d1c36f4b 100644 --- a/lms/templates/ccx/_dashboard_ccx_listing.html +++ b/lms/templates/ccx/_dashboard_ccx_listing.html @@ -1,4 +1,4 @@ -<%page args="ccx, membership, course, show_courseware_link, is_course_blocked" /> +<%page args="ccx, membership, course_overview, show_courseware_link, is_course_blocked" /> <%! from django.utils.translation import ugettext as _ @@ -7,7 +7,7 @@ from courseware.courses import course_image_url, get_course_about_section from ccx_keys.locator import CCXLocator %> <% - ccx_target = reverse('info', args=[CCXLocator.from_course_locator(course.id, ccx.id)]) + ccx_target = reverse('info', args=[CCXLocator.from_course_locator(course_overview.id, ccx.id)]) %>
  • @@ -16,16 +16,16 @@ from ccx_keys.locator import CCXLocator % if show_courseware_link: % if not is_course_blocked: - ${_('{course_number} {ccx_name} Cover Image').format(course_number=course.number, ccx_name=ccx.display_name) |h} + ${_('{course_number} {ccx_name} Cover Image').format(course_number=course_overview.number, ccx_name=ccx.display_name) |h} % else: - ${_('{course_number} {ccx_name} Cover Image').format(course_number=course.number, ccx_name=ccx.display_name) |h} + ${_('{course_number} {ccx_name} Cover Image').format(course_number=course_overview.number, ccx_name=ccx.display_name) |h} % endif % else: - ${_('{course_number} {ccx_name} Cover Image').format(course_number=course.number, ccx_name=ccx.display_name) |h} + ${_('{course_number} {ccx_name} Cover Image').format(course_number=course_overview.number, ccx_name=ccx.display_name) |h} % endif @@ -43,7 +43,7 @@ from ccx_keys.locator import CCXLocator
    ${get_course_about_section(course, 'university')} - - ${course.display_number_with_default | h} + ${course_overview.display_number_with_default | h} % if ccx.has_ended(): ${_("Ended - {end_date}").format(end_date=ccx.end_datetime_text("SHORT_DATE"))} diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index 231ffc6575..8d3fb0ad43 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -70,28 +70,28 @@ from django.core.urlresolvers import reverse - % if len(course_enrollment_pairs) > 0: + % if len(course_enrollments) > 0:
    - ${_('Verify Now')} + ${_('Verify Now')}
    % elif verification_status['status'] == VERIFY_STATUS_SUBMITTED:

    ${_('You have already verified your ID!')}

    @@ -329,7 +329,7 @@ from student.helpers import (