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
60 changes: 60 additions & 0 deletions cms/djangoapps/contentstore/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -423,3 +423,63 @@ def test_inheritance_in_locked_subsection(self):
def test_no_inheritance_for_orphan(self):
"""Tests that an orphaned xblock does not inherit staff lock"""
self.assertFalse(utils.ancestor_has_staff_lock(self.orphan))


class GroupVisibilityTest(CourseTestCase):
"""
Test content group access rules.
"""
def setUp(self):
super(GroupVisibilityTest, self).setUp()

chapter = ItemFactory.create(category='chapter', parent_location=self.course.location)
sequential = ItemFactory.create(category='sequential', parent_location=chapter.location)
vertical = ItemFactory.create(category='vertical', parent_location=sequential.location)
html = ItemFactory.create(category='html', parent_location=vertical.location)
problem = ItemFactory.create(
category='problem', parent_location=vertical.location, data="<problem></problem>"
)
self.sequential = self.store.get_item(sequential.location)
self.vertical = self.store.get_item(vertical.location)
self.html = self.store.get_item(html.location)
self.problem = self.store.get_item(problem.location)

def set_group_access(self, xblock, value):
""" Sets group_access to specified value and calls update_item to persist the change. """
xblock.group_access = value
self.store.update_item(xblock, self.user.id)

def test_no_visibility_set(self):
""" Tests when group_access has not been set on anything. """

def verify_all_components_visible_to_all(): # pylint: disable=invalid-name
""" Verifies when group_access has not been set on anything. """
for item in (self.sequential, self.vertical, self.html, self.problem):
self.assertFalse(utils.has_children_visible_to_specific_content_groups(item))
self.assertFalse(utils.is_visible_to_specific_content_groups(item))

verify_all_components_visible_to_all()

# Test with group_access set to Falsey values.
self.set_group_access(self.vertical, {1: []})
self.set_group_access(self.html, {2: None})

verify_all_components_visible_to_all()

def test_sequential_and_problem_have_group_access(self):
""" Tests when group_access is set on a few different components. """
self.set_group_access(self.sequential, {1: [0]})
# This is a no-op.
self.set_group_access(self.vertical, {1: []})
self.set_group_access(self.problem, {2: [3, 4]})

# Note that "has_children_visible_to_specific_content_groups" only checks immediate children.
self.assertFalse(utils.has_children_visible_to_specific_content_groups(self.sequential))
self.assertTrue(utils.has_children_visible_to_specific_content_groups(self.vertical))
self.assertFalse(utils.has_children_visible_to_specific_content_groups(self.html))
self.assertFalse(utils.has_children_visible_to_specific_content_groups(self.problem))

self.assertTrue(utils.is_visible_to_specific_content_groups(self.sequential))
self.assertFalse(utils.is_visible_to_specific_content_groups(self.vertical))
self.assertFalse(utils.is_visible_to_specific_content_groups(self.html))
self.assertTrue(utils.is_visible_to_specific_content_groups(self.problem))
30 changes: 30 additions & 0 deletions cms/djangoapps/contentstore/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,36 @@ def is_currently_visible_to_students(xblock):
return True


def has_children_visible_to_specific_content_groups(xblock):
"""
Returns True if this xblock has children that are limited to specific content groups.
Note that this method is not recursive (it does not check grandchildren).
"""
if not xblock.has_children:
return False

for child in xblock.get_children():
if is_visible_to_specific_content_groups(child):
return True

return False


def is_visible_to_specific_content_groups(xblock):
"""
Returns True if this xblock has visibility limited to specific content groups.
"""
if not xblock.group_access:
return False
for __, value in xblock.group_access.iteritems():
# value should be a list of group IDs. If it is an empty list or None, the xblock is visible
# to all groups in that particular partition. So if value is a truthy value, the xblock is
# restricted in some way.
if value:
return True
return False


def find_release_date_source(xblock):
"""
Finds the ancestor of xblock that set its release date.
Expand Down
7 changes: 4 additions & 3 deletions cms/djangoapps/contentstore/views/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

from contentstore.utils import get_lms_link_for_item
from contentstore.views.helpers import get_parent_xblock, is_unit, xblock_type_display_name
from contentstore.views.item import create_xblock_info
from contentstore.views.item import create_xblock_info, add_container_page_publishing_info

from opaque_keys.edx.keys import UsageKey

Expand Down Expand Up @@ -177,8 +177,9 @@ def container_handler(request, usage_key_string):
# about the block's ancestors and siblings for use by the Unit Outline.
xblock_info = create_xblock_info(xblock, include_ancestor_info=is_unit_page)

# Create the link for preview.
preview_lms_base = settings.FEATURES.get('PREVIEW_LMS_BASE')
if is_unit_page:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note: this variable (preview_lms_base) was unused.

add_container_page_publishing_info(xblock, xblock_info)

# need to figure out where this item is in the list of children as the
# preview will need this
index = 1
Expand Down
12 changes: 1 addition & 11 deletions cms/djangoapps/contentstore/views/course.py
Original file line number Diff line number Diff line change
Expand Up @@ -1343,7 +1343,7 @@ def group_configurations_list_handler(request, course_key_string):
'context_course': course,
'group_configuration_url': group_configuration_url,
'course_outline_url': course_outline_url,
'configurations': configurations if should_show_group_configurations_page(course) else None,
'configurations': configurations,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note that we may still need to specify some conditional logic about when to show the group configurations (split_test) part of the configurations page.

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.

Agreed. Let's tackle that in the next story.

})
elif "application/json" in request.META.get('HTTP_ACCEPT'):
if request.method == 'POST':
Expand Down Expand Up @@ -1422,16 +1422,6 @@ def group_configurations_detail_handler(request, course_key_string, group_config
return JsonResponse(status=204)


def should_show_group_configurations_page(course):
"""
Returns true if Studio should show the "Group Configurations" page for the specified course.
"""
return (
SPLIT_TEST_COMPONENT_TYPE in ADVANCED_COMPONENT_TYPES and
SPLIT_TEST_COMPONENT_TYPE in course.advanced_modules
)


def _get_course_creator_status(user):
"""
Helper method for returning the course creator status for a particular user,
Expand Down
130 changes: 86 additions & 44 deletions cms/djangoapps/contentstore/views/item.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,19 @@

from student.auth import has_course_author_access
from contentstore.utils import find_release_date_source, find_staff_lock_source, is_currently_visible_to_students, \
ancestor_has_staff_lock
ancestor_has_staff_lock, has_children_visible_to_specific_content_groups
from contentstore.views.helpers import is_unit, xblock_studio_url, xblock_primary_child_category, \
xblock_type_display_name, get_parent_xblock
from contentstore.views.preview import get_preview_fragment
from edxmako.shortcuts import render_to_string
from models.settings.course_grading import CourseGradingModel
from cms.lib.xblock.runtime import handler_url, local_resource_url
from opaque_keys.edx.keys import UsageKey, CourseKey
from cms.lib.xblock.authoring_mixin import VISIBILITY_VIEW

__all__ = ['orphan_handler', 'xblock_handler', 'xblock_view_handler', 'xblock_outline_handler']
__all__ = [
'orphan_handler', 'xblock_handler', 'xblock_view_handler', 'xblock_outline_handler', 'xblock_container_handler'
]

log = logging.getLogger(__name__)

Expand All @@ -58,7 +61,6 @@
NEVER = lambda x: False
ALWAYS = lambda x: True


# In order to allow descriptors to use a handler url, we need to
# monkey-patch the x_module library.
# TODO: Remove this code when Runtimes are no longer created by modulestores
Expand Down Expand Up @@ -142,8 +144,8 @@ def xblock_handler(request, usage_key_string):
return JsonResponse(CourseGradingModel.get_section_grader_type(usage_key))
# TODO: pass fields to _get_module_info and only return those
with modulestore().bulk_operations(usage_key.course_key):
rsp = _get_module_info(_get_xblock(usage_key, request.user))
return JsonResponse(rsp)
response = _get_module_info(_get_xblock(usage_key, request.user))
return JsonResponse(response)
else:
return HttpResponse(status=406)

Expand Down Expand Up @@ -216,14 +218,14 @@ def xblock_view_handler(request, usage_key_string, view_name):
request_token=request_token(request),
))

if view_name == STUDIO_VIEW:
if view_name in (STUDIO_VIEW, VISIBILITY_VIEW):
try:
fragment = xblock.render(STUDIO_VIEW)
fragment = xblock.render(view_name)
# catch exceptions indiscriminately, since after this point they escape the
# dungeon and surface as uneditable, unsaveable, and undeletable
# component-goblins.
except Exception as exc: # pylint: disable=broad-except
log.debug("unable to render studio_view for %r", xblock, exc_info=True)
log.debug("Unable to render %s for %r", view_name, xblock, exc_info=True)
fragment = Fragment(render_to_string('html_error.html', {'message': str(exc)}))

elif view_name in (PREVIEW_VIEWS + container_views):
Expand Down Expand Up @@ -301,6 +303,32 @@ def xblock_outline_handler(request, usage_key_string):
return Http404


# pylint: disable=unused-argument
@require_http_methods(("GET"))
@login_required
@expect_json
def xblock_container_handler(request, usage_key_string):
"""
The restful handler for requests for XBlock information about the block and its children.
This is used by the container page in particular to get additional information about publish state
and ancestor state.
"""
usage_key = usage_key_with_run(usage_key_string)

if not has_course_author_access(request.user, usage_key.course_key):
raise PermissionDenied()

response_format = request.REQUEST.get('format', 'html')
if response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'):
with modulestore().bulk_operations(usage_key.course_key):
response = _get_module_info(
_get_xblock(usage_key, request.user), include_ancestor_info=True, include_publishing_info=True
)
return JsonResponse(response)
else:
return Http404


def _update_with_callback(xblock, user, old_metadata=None, old_content=None):
"""
Updates the xblock in the modulestore.
Expand Down Expand Up @@ -635,7 +663,7 @@ def _get_xblock(usage_key, user):
return JsonResponse({"error": "Can't find item by location: " + unicode(usage_key)}, 404)


def _get_module_info(xblock, rewrite_static_links=True):
def _get_module_info(xblock, rewrite_static_links=True, include_ancestor_info=False, include_publishing_info=False):
"""
metadata, data, id representation of a leaf module fetcher.
:param usage_key: A UsageKey
Expand All @@ -653,7 +681,12 @@ def _get_module_info(xblock, rewrite_static_links=True):
modulestore().has_changes(modulestore().get_course(xblock.location.course_key, depth=None))

# Note that children aren't being returned until we have a use case.
return create_xblock_info(xblock, data=data, metadata=own_metadata(xblock), include_ancestor_info=True)
xblock_info = create_xblock_info(
xblock, data=data, metadata=own_metadata(xblock), include_ancestor_info=include_ancestor_info
)
if include_publishing_info:
add_container_page_publishing_info(xblock, xblock_info)
return xblock_info


def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=False, include_child_info=False,
Expand All @@ -673,24 +706,6 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
In addition, an optional include_children_predicate argument can be provided to define whether or
not a particular xblock should have its children included.
"""

def safe_get_username(user_id):
"""
Guard against bad user_ids, like the infamous "**replace_user**".
Note that this will ignore our special known IDs (ModuleStoreEnum.UserID).
We should consider adding special handling for those values.

:param user_id: the user id to get the username of
:return: username, or None if the user does not exist or user_id is None
"""
if user_id:
try:
return User.objects.get(id=user_id).username
except: # pylint: disable=bare-except
pass

return None

is_xblock_unit = is_unit(xblock, parent_xblock)
# this should not be calculated for Sections and Subsections on Unit page
has_changes = modulestore().has_changes(xblock) if (is_xblock_unit or course_outline) else None

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.

I was hoping that your change would stop calling has_changes for every sibling of the unit page. Any chance we could get that improvement here too? All I can think of is adding include_has_changes parameter...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think one could change this code so that has_changes is only added in this method if course_outline is True, and then also add the computation to add_publishing_info (which covers the is_xblock_unit case).

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.

Actually, it looks like in the case of the container page, has_changes has already been computed and cached by _get_module_info.

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.

The problem is that it hasn't been cached for the siblings, so as the algorithm goes up the tree to compute the ancestor info it ends up going back to the database.

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.

(For documenting purposes, I know we discussed this in person) _get_module_info calls has_changes on the whole course by way of xblock_container_handler.

Expand All @@ -710,8 +725,6 @@ def safe_get_username(user_id):
else:
child_info = None

# Treat DEFAULT_START_DATE as a magic number that means the release date has not been set
release_date = get_default_time_display(xblock.start) if xblock.start != DEFAULT_START_DATE else None
if xblock.category != 'course':
visibility_state = _compute_visibility_state(xblock, child_info, is_xblock_unit and has_changes)
else:
Expand All @@ -727,7 +740,7 @@ def safe_get_username(user_id):
"published_on": get_default_time_display(xblock.published_on) if xblock.published_on else None,

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.

It's kind of strange that this isn't provided by add_publishing_info. I see that it is used by the course outline too. Maybe there should be two levels of add_publishing_info: the default just adds published_on and has_changes, while the full one includes everything else

"studio_url": xblock_studio_url(xblock, parent_xblock),
"released_to_students": datetime.now(UTC) > xblock.start,
"release_date": release_date,
"release_date": _get_release_date(xblock),
"visibility_state": visibility_state,
"has_explicit_staff_lock": xblock.fields['visible_to_staff_only'].is_set_on(xblock),
"start": xblock.fields['start'].to_json(xblock.start),
Expand All @@ -751,19 +764,6 @@ def safe_get_username(user_id):
else:
xblock_info["ancestor_has_staff_lock"] = False

# Currently, 'edited_by', 'published_by', and 'release_date_from' are only used by the
# container page when rendering a unit. Since they are expensive to compute, only include them for units
# that are not being rendered on the course outline.
if is_xblock_unit and not course_outline:
xblock_info["edited_by"] = safe_get_username(xblock.subtree_edited_by)
xblock_info["published_by"] = safe_get_username(xblock.published_by)
xblock_info["currently_visible_to_students"] = is_currently_visible_to_students(xblock)
if release_date:
xblock_info["release_date_from"] = _get_release_date_from(xblock)
if visibility_state == VisibilityState.staff_only:
xblock_info["staff_lock_from"] = _get_staff_lock_from(xblock)
else:
xblock_info["staff_lock_from"] = None
if course_outline:
if xblock_info["has_explicit_staff_lock"]:
xblock_info["staff_only_message"] = True
Expand All @@ -775,6 +775,40 @@ def safe_get_username(user_id):
return xblock_info


def add_container_page_publishing_info(xblock, xblock_info): # pylint: disable=invalid-name
"""
Adds information about the xblock's publish state to the supplied
xblock_info for the container page.
"""
def safe_get_username(user_id):
"""
Guard against bad user_ids, like the infamous "**replace_user**".
Note that this will ignore our special known IDs (ModuleStoreEnum.UserID).
We should consider adding special handling for those values.

:param user_id: the user id to get the username of
:return: username, or None if the user does not exist or user_id is None
"""
if user_id:
try:
return User.objects.get(id=user_id).username
except: # pylint: disable=bare-except
pass

return None

xblock_info["edited_by"] = safe_get_username(xblock.subtree_edited_by)
xblock_info["published_by"] = safe_get_username(xblock.published_by)
xblock_info["currently_visible_to_students"] = is_currently_visible_to_students(xblock)
xblock_info["has_content_group_components"] = has_children_visible_to_specific_content_groups(xblock)
if xblock_info["release_date"]:
xblock_info["release_date_from"] = _get_release_date_from(xblock)
if xblock_info["visibility_state"] == VisibilityState.staff_only:
xblock_info["staff_lock_from"] = _get_staff_lock_from(xblock)
else:
xblock_info["staff_lock_from"] = None


class VisibilityState(object):
"""
Represents the possible visibility states for an xblock:
Expand Down Expand Up @@ -894,6 +928,14 @@ def _create_xblock_child_info(xblock, course_outline, graders, include_children_
return child_info


def _get_release_date(xblock):
"""
Returns the release date for the xblock, or None if the release date has never been set.
"""
# Treat DEFAULT_START_DATE as a magic number that means the release date has not been set
return get_default_time_display(xblock.start) if xblock.start != DEFAULT_START_DATE else None


def _get_release_date_from(xblock):
"""
Returns a string representation of the section or subsection that sets the xblock's release date
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,17 +208,6 @@ def test_view_index_ok(self):
self.assertContains(response, 'First name')
self.assertContains(response, 'Group C')

def test_view_index_disabled(self):
"""
Check that group configuration page is not displayed when turned off.
"""
if SPLIT_TEST_COMPONENT_TYPE in self.course.advanced_modules:
self.course.advanced_modules.remove(SPLIT_TEST_COMPONENT_TYPE)
self.store.update_item(self.course, self.user.id)

resp = self.client.get(self._url())
self.assertContains(resp, "module is disabled")

def test_unsupported_http_accept_header(self):
"""
Test if not allowed header present in request.
Expand Down
Loading