Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix self paced course content visiblity #11452

Merged
merged 5 commits into from
Feb 17, 2016
Merged
Show file tree
Hide file tree
Changes from 4 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
9 changes: 9 additions & 0 deletions cms/djangoapps/contentstore/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
from django_comment_common.models import assign_default_role
from django_comment_common.utils import seed_permissions_roles

from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration

from xmodule.modulestore import ModuleStoreEnum
from xmodule.modulestore.django import modulestore
from xmodule.modulestore.exceptions import ItemNotFoundError
Expand Down Expand Up @@ -455,3 +457,10 @@ def get_visibility_partition_info(xblock):
"has_selected_groups": has_selected_groups,
"selected_verified_partition_id": selected_verified_partition_id,
}


def is_self_paced(course):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is worthwhile to abstract out into its own utility. Also, self_paced should always return a boolean: https://github.com/edx/edx-platform/blob/master/common/lib/xmodule/xmodule/course_module.py#L753-L763.

If it's not set, it'll return False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs utility because, we can not always use course. self_paced because in create_xblock_info method it does not have to be a course always ( sometimes library may be). Thus, course object may be None. Also, we still need to check on SelfPacedConfiguration is enabled or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

ugh

"""
Returns True if course is self-paced, False otherwise.
"""
return course and course.self_paced and SelfPacedConfiguration.current().enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check if there is any table in mysql where this mapping is added? I mean where we add information about course if it is self paced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have checked but there is no such SelfPaced to Course mapping table in MySQL.

14 changes: 9 additions & 5 deletions cms/djangoapps/contentstore/views/item.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
from contentstore.views.helpers import is_unit, xblock_studio_url, xblock_primary_child_category, \
xblock_type_display_name, get_parent_xblock, create_xblock, usage_key_with_run
from contentstore.views.preview import get_preview_fragment
from contentstore.utils import is_self_paced

from openedx.core.lib.gating import api as gating_api
from edxmako.shortcuts import render_to_string
from models.settings.course_grading import CourseGradingModel
Expand Down Expand Up @@ -855,7 +857,9 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
release_date = _get_release_date(xblock, user)

if xblock.category != 'course':
visibility_state = _compute_visibility_state(xblock, child_info, is_xblock_unit and has_changes)
visibility_state = _compute_visibility_state(
xblock, child_info, is_xblock_unit and has_changes, is_self_paced(course)
)
else:
visibility_state = None
published = modulestore().has_published_version(xblock) if not is_library_block else None
Expand Down Expand Up @@ -1017,7 +1021,7 @@ class VisibilityState(object):
gated = 'gated'


def _compute_visibility_state(xblock, child_info, is_unit_with_changes):
def _compute_visibility_state(xblock, child_info, is_unit_with_changes, is_course_self_paced):
"""
Returns the current publish state for the specified xblock and its children
"""
Expand All @@ -1027,10 +1031,10 @@ def _compute_visibility_state(xblock, child_info, is_unit_with_changes):
# Note that a unit that has never been published will fall into this category,
# as well as previously published units with draft content.
return VisibilityState.needs_attention

is_unscheduled = xblock.start == DEFAULT_START_DATE
is_live = datetime.now(UTC) > xblock.start
children = child_info and child_info.get('children', [])
if children and len(children) > 0:
is_live = is_course_self_paced or datetime.now(UTC) > xblock.start
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a suggestion, Instead of passing is_course_self_paced in the method argument would require the param to pass everywhere the method will be used.

How about something like this?
is_live = is_self_paced(course) or datetime.now(UTC) > xblock.start

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, but I didn't wanted to pass whole course object which was unnecessary, so I passed only what is required in _compute_visibility_state method.

However, I should make is_course_self_paced param to be false by default.

if child_info and child_info.get('children', []):
all_staff_only = True
all_unscheduled = True
all_live = True
Expand Down
30 changes: 30 additions & 0 deletions cms/djangoapps/contentstore/views/tests/test_item.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from django.core.urlresolvers import reverse
from contentstore.utils import reverse_usage_url, reverse_course_url

from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration
from contentstore.views.component import (
component_handler, get_component_templates
)
Expand Down Expand Up @@ -1847,6 +1848,7 @@ def test_no_add_advanced(self):
self.assertFalse(lib.children)


@ddt.ddt
class TestXBlockPublishingInfo(ItemTest):
"""
Unit tests for XBlock's outline handling.
Expand Down Expand Up @@ -2169,3 +2171,31 @@ def test_locked_unit_staff_only_message(self):
self._verify_has_staff_only_message(xblock_info, True)
self._verify_has_staff_only_message(xblock_info, True, path=self.FIRST_SUBSECTION_PATH)
self._verify_has_staff_only_message(xblock_info, True, path=self.FIRST_UNIT_PATH)

@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_self_paced_item_visibility_state(self, store_type):
"""
Test that in self-paced course, item has `live` visibility state.
Test that when item was initially in `scheduled` state in instructor mode, change course pacing to self-paced,
now in self-paced course, item should have `live` visibility state.
"""
SelfPacedConfiguration(enabled=True).save()

# Create course, chapter and setup future release date to make chapter in scheduled state
course = CourseFactory.create(default_store=store_type)
chapter = self._create_child(course, 'chapter', "Test Chapter")
self._set_release_date(chapter.location, datetime.now(UTC) + timedelta(days=1))

# Check that chapter has scheduled state
xblock_info = self._get_xblock_info(chapter.location)
self._verify_visibility_state(xblock_info, VisibilityState.ready)
self.assertFalse(course.self_paced)

# Change course pacing to self paced
course.self_paced = True
self.store.update_item(course, self.user.id)
self.assertTrue(course.self_paced)

# Check that in self paced course content has live state now
xblock_info = self._get_xblock_info(chapter.location)
self._verify_visibility_state(xblock_info, VisibilityState.live)