Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions common/djangoapps/student/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@ def anonymous_id_for_user(user, course_id, save=True):
save -- Whether the id should be saved in an AnonymousUserId object.
"""
# This part is for ability to get xblock instance in xblock_noauth handlers, where user is unauthenticated.
assert user

if user.is_anonymous():
return None

Expand Down Expand Up @@ -681,6 +683,8 @@ def should_user_reset_password_now(cls, user):
Returns whether a password has 'expired' and should be reset. Note there are two different
expiry policies for staff and students
"""
assert user

if not settings.FEATURES['ADVANCED_SECURITY']:
return False

Expand Down Expand Up @@ -736,6 +740,8 @@ def is_allowable_password_reuse(cls, user, new_password):
"""
Verifies that the password adheres to the reuse policies
"""
assert user

if not settings.FEATURES['ADVANCED_SECURITY']:
return True

Expand Down Expand Up @@ -1082,6 +1088,10 @@ def get_enrollment(cls, user, course_key):
Returns:
Course enrollment object or None
"""
assert user

if user.is_anonymous():
return None
try:
return cls.objects.get(
user=user,
Expand Down Expand Up @@ -1397,11 +1407,8 @@ def is_enrolled(cls, user, course_key):

`course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall)
"""
if not user.is_authenticated():
return False
else:
enrollment_state = cls._get_enrollment_state(user, course_key)
return enrollment_state.is_active or False
enrollment_state = cls._get_enrollment_state(user, course_key)
return enrollment_state.is_active or False

@classmethod
def is_enrolled_by_partial(cls, user, course_id_partial):
Expand Down Expand Up @@ -1497,6 +1504,8 @@ def generate_enrollment_status_hash(cls, user):
Returns:
str: Hash of the user's active enrollments. If the user is anonymous, `None` is returned.
"""
assert user

if user.is_anonymous():
return None

Expand Down Expand Up @@ -1704,6 +1713,10 @@ def _get_enrollment_state(cls, user, course_key):
Returns the CourseEnrollmentState for the given user
and course_key, caching the result for later retrieval.
"""
assert user

if user.is_anonymous():
return CourseEnrollmentState(None, None)
enrollment_state = cls._get_enrollment_in_request_cache(user, course_key)
if not enrollment_state:
try:
Expand Down
53 changes: 50 additions & 3 deletions common/lib/xmodule/xmodule/modulestore/tests/django_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,38 @@
import functools
import os
from contextlib import contextmanager
from enum import Enum

from courseware.field_overrides import OverrideFieldData # pylint: disable=import-error
from courseware.tests.factories import StaffFactory
from django.conf import settings
from django.contrib.auth.models import User
from django.contrib.auth.models import AnonymousUser, User
from django.test import TestCase
from django.test.utils import override_settings
from mock import patch
from openedx.core.djangolib.testing.utils import CacheIsolationMixin, CacheIsolationTestCase, FilteredQueryCountMixin
from openedx.core.lib.tempdir import mkdtemp_clean
from student.models import CourseEnrollment
from student.tests.factories import UserFactory
from xmodule.contentstore.django import _CONTENTSTORE
from xmodule.modulestore import ModuleStoreEnum
from xmodule.modulestore.django import SignalHandler, clear_existing_modulestores, modulestore
from xmodule.modulestore.tests.factories import XMODULE_FACTORY_LOCK
from xmodule.modulestore.tests.mongo_connection import MONGO_HOST, MONGO_PORT_NUM


class CourseUserType(Enum):
"""
Types of users to be used when testing a course.
"""
ANONYMOUS = 'anonymous'
COURSE_STAFF = 'course_staff'
ENROLLED = 'enrolled'
GLOBAL_STAFF = 'global_staff'
UNENROLLED = 'unenrolled'
UNENROLLED_STAFF = 'unenrolled_staff'


class StoreConstructors(object):
"""Enumeration of store constructor types."""
draft, split = range(2)
Expand Down Expand Up @@ -308,7 +324,36 @@ def end_modulestore_isolation(cls):
cls.enable_all_signals()


class SharedModuleStoreTestCase(FilteredQueryCountMixin, ModuleStoreIsolationMixin, CacheIsolationTestCase):
class ModuleStoreTestUsersMixin():
"""
A mixin to help manage test users.
"""
TEST_PASSWORD = 'test'

def create_user_for_course(self, course, user_type=CourseUserType.ENROLLED):
"""
Create a test user for a course.
"""
if user_type is CourseUserType.ANONYMOUS:
return AnonymousUser()

is_enrolled = user_type is CourseUserType.ENROLLED
is_unenrolled_staff = user_type is CourseUserType.UNENROLLED_STAFF

# Set up the test user
if is_unenrolled_staff:
user = StaffFactory(course_key=course.id, password=self.TEST_PASSWORD)
else:
user = UserFactory(password=self.TEST_PASSWORD)
self.client.login(username=user.username, password=self.TEST_PASSWORD)
if is_enrolled:
CourseEnrollment.enroll(user, course.id)
return user


class SharedModuleStoreTestCase(
ModuleStoreTestUsersMixin, FilteredQueryCountMixin, ModuleStoreIsolationMixin, CacheIsolationTestCase
):
"""
Subclass for any test case that uses a ModuleStore that can be shared
between individual tests. This class ensures that the ModuleStore is cleaned
Expand Down Expand Up @@ -391,7 +436,9 @@ def setUp(self):
super(SharedModuleStoreTestCase, self).setUp()


class ModuleStoreTestCase(FilteredQueryCountMixin, ModuleStoreIsolationMixin, TestCase):
class ModuleStoreTestCase(
ModuleStoreTestUsersMixin, FilteredQueryCountMixin, ModuleStoreIsolationMixin, TestCase
):
"""
Subclass for any test case that uses a ModuleStore.
Ensures that the ModuleStore is cleaned before/after each test.
Expand Down
14 changes: 7 additions & 7 deletions lms/djangoapps/courseware/courses.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,21 +113,21 @@ 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.
check_if_enrolled: If true, additionally verifies that the user is enrolled.
"""
access_response = has_access(user, action, course, course.id)
# Allow staff full access to the course even if not enrolled
if has_access(user, 'staff', course.id):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This is changing the logic. You confirmed that the earlier access_response = has_access(user, action, course, course.id) would never deny access for staff?
  2. I wonder why it was written below as it was? You think there was a performance issue calling this for every user? I'm not asking for it to go back...just curious.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good questions:

  1. My understanding is that this isn't changing the logic, because has_access would return true for staff. It just moves the staff check earlier to make it clearer (to my mind) that staff gets to skip all the other logic.

  2. I'm not sure what you're saying. Are you thinking there's a performance impact to moving the staff check earlier? That probably is why some of the counts increased, so we could optimize it. However, I think that the clarity of understanding that staff always get access is a worthwhile trade off.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andy-armstrong.

  1. I didn't check the code, so was just double-checking with you.
  2. I agree with going for clarity. And I am guessing that is why the count increased.

return

access_response = has_access(user, action, course, course.id)
if not access_response:
# Deliberately return a non-specific error message to avoid
# leaking info about access control settings
raise CoursewareAccessException(access_response)

if check_if_enrolled:
# Verify that the user is either enrolled in the course or a staff
# member. If the user is not enrolled, raise a Redirect exception
# that will be handled by middleware.
if not ((user.id and CourseEnrollment.is_enrolled(user, course.id)) or has_access(user, 'staff', course)):
# If the user is not enrolled, redirect them to the about page
if not CourseEnrollment.is_enrolled(user, course.id):
raise CourseAccessRedirect(reverse('about_course', args=[unicode(course.id)]))


Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/courseware/date_summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def is_enabled(self):
future.
"""
if self.date is not None:
return datetime.now(utc) <= self.date
return datetime.now(utc).date() <= self.date.date()
return False

def deadline_has_passed(self):
Expand Down
10 changes: 10 additions & 0 deletions lms/djangoapps/courseware/tabs.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,16 @@ class CoursewareTab(EnrolledTab):
is_default = False
supports_preview_menu = True

@classmethod
def is_enabled(cls, course, user=None):
"""
Returns true if this tab is enabled.
"""
# If this is the unified course tab then it is always enabled
if UNIFIED_COURSE_TAB_FLAG.is_enabled(course.id):
return True
return super(CoursewareTab, cls).is_enabled(course, user)

@property
def link_func(self):
"""
Expand Down
141 changes: 0 additions & 141 deletions lms/djangoapps/courseware/tests/__init__.py
Original file line number Diff line number Diff line change
@@ -1,141 +0,0 @@
"""

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewers: I moved this code to helpers.py as otherwise there are circular imports when accessing this package.

integration tests for xmodule

Contains:

1. BaseTestXmodule class provides course and users
for testing Xmodules with mongo store.
"""

from django.core.urlresolvers import reverse
from django.test.client import Client

from edxmako.shortcuts import render_to_string
from lms.djangoapps.lms_xblock.field_data import LmsFieldData
from openedx.core.lib.url_utils import quote_slashes
from student.tests.factories import UserFactory, CourseEnrollmentFactory
from xblock.field_data import DictFieldData
from xmodule.modulestore.tests.django_utils import TEST_DATA_MONGO_MODULESTORE
from xmodule.tests import get_test_system, get_test_descriptor_system
from xmodule.modulestore.django import modulestore
from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase


class BaseTestXmodule(ModuleStoreTestCase):
"""Base class for testing Xmodules with mongo store.

This class prepares course and users for tests:
1. create test course;
2. create, enroll and login users for this course;

Any xmodule should overwrite only next parameters for test:
1. CATEGORY
2. DATA or METADATA
3. MODEL_DATA
4. COURSE_DATA and USER_COUNT if needed

This class should not contain any tests, because CATEGORY
should be defined in child class.
"""
MODULESTORE = TEST_DATA_MONGO_MODULESTORE

USER_COUNT = 2
COURSE_DATA = {}

# Data from YAML common/lib/xmodule/xmodule/templates/NAME/default.yaml
CATEGORY = "vertical"
DATA = ''
# METADATA must be overwritten for every instance that uses it. Otherwise,
# if we'll change it in the tests, it will be changed for all other instances
# of parent class.
METADATA = {}
MODEL_DATA = {'data': '<some_module></some_module>'}

def new_module_runtime(self):
"""
Generate a new ModuleSystem that is minimally set up for testing
"""
return get_test_system(course_id=self.course.id)

def new_descriptor_runtime(self):
runtime = get_test_descriptor_system()
runtime.get_block = modulestore().get_item
return runtime

def initialize_module(self, **kwargs):
kwargs.update({
'parent_location': self.section.location,
'category': self.CATEGORY
})

self.item_descriptor = ItemFactory.create(**kwargs)

self.runtime = self.new_descriptor_runtime()

field_data = {}
field_data.update(self.MODEL_DATA)
student_data = DictFieldData(field_data)
self.item_descriptor._field_data = LmsFieldData(self.item_descriptor._field_data, student_data)

self.item_descriptor.xmodule_runtime = self.new_module_runtime()

self.item_url = unicode(self.item_descriptor.location)

def setup_course(self):
self.course = CourseFactory.create(data=self.COURSE_DATA)

# Turn off cache.
modulestore().request_cache = None
modulestore().metadata_inheritance_cache_subsystem = None

chapter = ItemFactory.create(
parent_location=self.course.location,
category="sequential",
)
self.section = ItemFactory.create(
parent_location=chapter.location,
category="sequential"
)

# username = robot{0}, password = 'test'
self.users = [
UserFactory.create()
for dummy0 in range(self.USER_COUNT)
]

for user in self.users:
CourseEnrollmentFactory.create(user=user, course_id=self.course.id)

# login all users for acces to Xmodule
self.clients = {user.username: Client() for user in self.users}
self.login_statuses = [
self.clients[user.username].login(
username=user.username, password='test')
for user in self.users
]

self.assertTrue(all(self.login_statuses))

def setUp(self):
super(BaseTestXmodule, self).setUp()
self.setup_course()
self.initialize_module(metadata=self.METADATA, data=self.DATA)

def get_url(self, dispatch):
"""Return item url with dispatch."""
return reverse(
'xblock_handler',
args=(unicode(self.course.id), quote_slashes(self.item_url), 'xmodule_handler', dispatch)
)


class XModuleRenderingTestBase(BaseTestXmodule):

def new_module_runtime(self):
"""
Create a runtime that actually does html rendering
"""
runtime = super(XModuleRenderingTestBase, self).new_module_runtime()
runtime.render_template = render_to_string
return runtime
Loading