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
22 changes: 22 additions & 0 deletions cms/djangoapps/contentstore/tests/test_access.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from django.test import TestCase
from django.contrib.auth.models import User
from xmodule.modulestore import Location
from xmodule.modulestore.locator import CourseLocator

from student.roles import CourseInstructorRole, CourseStaffRole
from student.tests.factories import AdminFactory
Expand All @@ -20,6 +21,7 @@ def setUp(self):
self.instructor = User.objects.create_user('testinstructor', 'testinstructor+courses@edx.org', 'foo')
self.staff = User.objects.create_user('teststaff', 'teststaff+courses@edx.org', 'foo')
self.location = Location('i4x', 'mitX', '101', 'course', 'test')
self.locator = CourseLocator(url='edx://mitX.101.test')

def test_get_user_role_instructor(self):
"""
Expand All @@ -31,6 +33,16 @@ def test_get_user_role_instructor(self):
get_user_role(self.instructor, self.location, self.location.course_id)
)

def test_get_user_role_instructor_locator(self):
"""
Verifies if user is instructor, using a CourseLocator.
"""
add_users(self.global_admin, CourseInstructorRole(self.locator), self.instructor)
self.assertEqual(
'instructor',
get_user_role(self.instructor, self.locator)
)

def test_get_user_role_staff(self):
"""
Verifies if user is staff.
Expand All @@ -40,3 +52,13 @@ def test_get_user_role_staff(self):
'staff',
get_user_role(self.staff, self.location, self.location.course_id)
)

def test_get_user_role_staff_locator(self):
"""
Verifies if user is staff, using a CourseLocator.
"""
add_users(self.global_admin, CourseStaffRole(self.locator), self.staff)
self.assertEqual(
'staff',
get_user_role(self.staff, self.locator)
)
6 changes: 3 additions & 3 deletions cms/djangoapps/contentstore/views/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ def has_course_access(user, location, role=CourseStaffRole):
return auth.has_access(user, role(location))


def get_user_role(user, location, context):
def get_user_role(user, location, context=None):
"""
Return corresponding string if user has staff or instructor role in Studio.
This will not return student role because its purpose for using in Studio.

:param location: a descriptor.location
:param context: a course_id
:param location: a descriptor.location (which may be a Location or a CourseLocator)
:param context: a course_id. This is not used if location is a CourseLocator.
"""
if auth.has_access(user, CourseInstructorRole(location, context)):
return 'instructor'
Expand Down
22 changes: 13 additions & 9 deletions cms/djangoapps/contentstore/views/preview.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
from xmodule_modifiers import replace_static_urls, wrap_xblock
from xmodule.error_module import ErrorDescriptor
from xmodule.exceptions import NotFoundError, ProcessingError
from xmodule.modulestore.django import modulestore
from xmodule.modulestore.django import modulestore, loc_mapper
from xmodule.modulestore.locator import Locator
from xmodule.x_module import ModuleSystem
from xblock.runtime import KvsFieldData
from xblock.django.request import webob_to_django_response, django_to_webob_request
Expand Down Expand Up @@ -44,7 +45,8 @@ def preview_handler(request, usage_id, handler, suffix=''):
handler: The handler to execute
suffix: The remainder of the url to be passed to the handler
"""

# Note: usage_id is currently the string form of a Location, but in the
# future it will be the string representation of a Locator.
location = unquote_slashes(usage_id)

descriptor = modulestore().get_item(location)
Expand Down Expand Up @@ -95,7 +97,11 @@ def _preview_module_system(request, descriptor):
descriptor: An XModuleDescriptor
"""

course_id = get_course_for_item(descriptor.location).location.course_id
if isinstance(descriptor.location, Locator):
course_location = loc_mapper().translate_locator_to_location(descriptor.location, get_course=True)
course_id = course_location.course_id
else:
course_id = get_course_for_item(descriptor.location).location.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.

This is going to go away when the modulestore handles locator/location translation, right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, though we will have to get the course_id.


return PreviewModuleSystem(
static_url=settings.STATIC_URL,
Expand All @@ -115,17 +121,15 @@ def _preview_module_system(request, descriptor):
# Set up functions to modify the fragment produced by student_view
wrappers=(
# This wrapper wraps the module in the template specified above
partial(wrap_xblock, 'PreviewRuntime', display_name_only=descriptor.location.category == 'static_tab'),
partial(wrap_xblock, 'PreviewRuntime', display_name_only=descriptor.category == 'static_tab'),

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.

Are you sure getting rid of data_dir is the right thing to do?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

None is passed for data_directory in the call above. I looked at the code that is being called, and it seems that data_directory is only used for XML modulestore (which Studio doesn't use). The code is in /edx-platform/common/djangoapps/static_replace/init.py

I couldn't think of any reason why the arguments should be different in the 2 places. Do you know of a reason why they should differ?

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.

Fair enough. I think that should be fine in this code. Another good reason that we should clean up the api for the ContentStore and static replacement, so that we don't have to have all of these special cases depending on whether we're in an XML world or a mongo one.


# This wrapper replaces urls in the output that start with /static
# with the correct course-specific url for the static content
partial(
replace_static_urls,
getattr(descriptor, 'data_dir', descriptor.location.course),
course_id=descriptor.location.org + '/' + descriptor.location.course + '/BOGUS_RUN_REPLACE_WHEN_AVAILABLE',
),
partial(replace_static_urls, None, course_id=course_id),
),
error_descriptor_class=ErrorDescriptor,
# get_user_role accepts a location or a CourseLocator.
# If descriptor.location is a CourseLocator, course_id is unused.
get_user_role=lambda: get_user_role(request.user, descriptor.location, course_id),
)

Expand Down
Empty file.
55 changes: 55 additions & 0 deletions cms/djangoapps/contentstore/views/tests/test_preview.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
"""
Tests for contentstore.views.preview.py
"""
from django.test import TestCase
from django.test.client import RequestFactory

from student.tests.factories import UserFactory

from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
from xmodule.modulestore.django import loc_mapper

from contentstore.views.preview import get_preview_fragment


class GetPreviewHtmlTestCase(TestCase):
"""
Tests for get_preview_html.

Note that there are other existing test cases in test_contentstore that indirectly execute
get_preview_html via the xblock RESTful API.
"""

def test_preview_handler_locator(self):
"""
Test for calling get_preview_html when descriptor.location is a Locator.
"""
course = CourseFactory.create()
html = ItemFactory.create(
parent_location=course.location,
category="html",
data={'data': "<html>foobar</html>"}
)

locator = loc_mapper().translate_location(
course.location.course_id, html.location, True, True
)

# Change the stored location to a locator.
html.location = locator
html.save()

request = RequestFactory().get('/dummy-url')
request.user = UserFactory()
request.session = {}

# Must call get_preview_fragment directly, as going through xblock RESTful API will attempt
# to use item.location as a Location.
html = get_preview_fragment(request, html).content
# Verify student view html is returned, and there are no old locations in it.
self.assertRegexpMatches(
html,
'data-usage-id="MITx.999.Robot_Super_Course;_branch;_published;_block;_html_[0-9]*"'
)
self.assertRegexpMatches(html, '<html>foobar</html>')
self.assertNotRegexpMatches(html, 'i4x')
9 changes: 0 additions & 9 deletions common/lib/xmodule/xmodule/contentstore/content.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,6 @@ def get_location_from_path(path):

return Location(path.split('/'))

@staticmethod
def get_id_from_path(path):
return get_id_from_location(get_location_from_path(path))

@staticmethod
def convert_legacy_static_url(path, course_namespace):
loc = StaticContent.compute_location(course_namespace.org, course_namespace.course, path)
return StaticContent.get_url_path_from_location(loc)

@staticmethod
def convert_legacy_static_url_with_course_id(path, course_id):
"""
Expand Down