MA-779 Make has_access work on CourseOverview objects

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.
+    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 = {}
-    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)
-                            })
-
-            # if there are required courses add to dict
-            if required_courses:
-                pre_requisite_courses[course_key] = {'courses': required_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}
+
     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,

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)))

lms/djangoapps/courseware/tests/test_access.py

 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)

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):

openedx/core/djangoapps/content/course_overviews/migrations/0002_add_days_early_for_beta.py

+# -*- coding: utf-8 -*-
+from south.utils import datetime_utils as datetime
+from south.db import db
+from south.v2 import SchemaMigration
+from django.db import models
+
+
+class Migration(SchemaMigration):
+
+    def forwards(self, orm):
+        # Adding field 'CourseOverview.days_early_for_beta'
+
+        # The default value for the days_early_for_beta column is null. However,
+        # for courses already in the table that have a non-null value for
+        # days_early_for_beta, this would be invalid. So, we must clear the
+        # table before adding the new column.
+
+        db.clear_table('course_overviews_courseoverview')
+        db.add_column('course_overviews_courseoverview', 'days_early_for_beta',
+                      self.gf('django.db.models.fields.FloatField')(null=True),
+                      keep_default=False)
+
+
+    def backwards(self, orm):
+        # Deleting field 'CourseOverview.days_early_for_beta'
+        db.delete_column('course_overviews_courseoverview', 'days_early_for_beta')
+
+
+    models = {
+        'course_overviews.courseoverview': {
+            'Meta': {'object_name': 'CourseOverview'},
+            '_location': ('xmodule_django.models.UsageKeyField', [], {'max_length': '255'}),
+            '_pre_requisite_courses_json': ('django.db.models.fields.TextField', [], {}),
+            'advertised_start': ('django.db.models.fields.TextField', [], {'null': 'True'}),
+            'cert_name_long': ('django.db.models.fields.TextField', [], {}),
+            'cert_name_short': ('django.db.models.fields.TextField', [], {}),
+            'certificates_display_behavior': ('django.db.models.fields.TextField', [], {'null': 'True'}),
+            'certificates_show_before_end': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'course_image_url': ('django.db.models.fields.TextField', [], {}),
+            'days_early_for_beta': ('django.db.models.fields.FloatField', [], {'null': 'True'}),
+            'display_name': ('django.db.models.fields.TextField', [], {'null': 'True'}),
+            'display_number_with_default': ('django.db.models.fields.TextField', [], {}),
+            'display_org_with_default': ('django.db.models.fields.TextField', [], {}),
+            'end': ('django.db.models.fields.DateTimeField', [], {'null': 'True'}),
+            'end_of_course_survey_url': ('django.db.models.fields.TextField', [], {'null': 'True'}),
+            'facebook_url': ('django.db.models.fields.TextField', [], {'null': 'True'}),
+            'has_any_active_web_certificate': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'id': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'primary_key': 'True', 'db_index': 'True'}),
+            'lowest_passing_grade': ('django.db.models.fields.DecimalField', [], {'max_digits': '5', 'decimal_places': '2'}),
+            'mobile_available': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'social_sharing_url': ('django.db.models.fields.TextField', [], {'null': 'True'}),
+            'start': ('django.db.models.fields.DateTimeField', [], {'null': 'True'}),
+            'visible_to_staff_only': ('django.db.models.fields.BooleanField', [], {'default': 'False'})
+        }
+    }
+
+    complete_apps = ['course_overviews']
\ No newline at end of file

openedx/core/djangoapps/content/course_overviews/models.py

@@ -5,11 +5,9 @@ Declaration of CourseOverview model
 import json
 
 import django.db.models
-from django.db.models.fields import BooleanField, DateTimeField, DecimalField, TextField
+from django.db.models.fields import BooleanField, DateTimeField, DecimalField, TextField, FloatField
 from django.utils.translation import ugettext
 
-from lms.djangoapps.certificates.api import get_active_web_certificate
-from lms.djangoapps.courseware.courses import course_image_url
 from util.date_utils import strftime_localized
 from xmodule import course_metadata_utils
 from xmodule.modulestore.django import modulestore
@@ -54,6 +52,7 @@ class CourseOverview(django.db.models.Model):
     lowest_passing_grade = DecimalField(max_digits=5, decimal_places=2)
 
     # Access parameters
+    days_early_for_beta = FloatField(null=True)
     mobile_available = BooleanField()
     visible_to_staff_only = BooleanField()
     _pre_requisite_courses_json = TextField()  # JSON representation of list of CourseKey strings
@@ -72,6 +71,9 @@ class CourseOverview(django.db.models.Model):
         Returns:
             CourseOverview: overview extracted from the given course
         """
+        from lms.djangoapps.certificates.api import get_active_web_certificate
+        from lms.djangoapps.courseware.courses import course_image_url
+
         return CourseOverview(
             id=course.id,
             _location=course.location,
@@ -95,6 +97,7 @@ class CourseOverview(django.db.models.Model):
             lowest_passing_grade=course.lowest_passing_grade,
             end_of_course_survey_url=course.end_of_course_survey_url,
 
+            days_early_for_beta=course.days_early_for_beta,
             mobile_available=course.mobile_available,
             visible_to_staff_only=course.visible_to_staff_only,
             _pre_requisite_courses_json=json.dumps(course.pre_requisite_courses)