From a59363432cbbc387d1e865642219ba46787d933b Mon Sep 17 00:00:00 2001
From: saadyousafarbi <saad.yousaf@arbisoft.com>
Date: Mon, 18 Nov 2019 18:16:57 +0500
Subject: [PATCH] handle section link bug for staff-only units for learners.
---
.../lib/xmodule/xmodule/modulestore/search.py | 48 +++++++++++-
.../tests/test_mixed_modulestore.py | 2 +-
.../tests/lms/test_lms_dashboard.py | 76 +++++++++++++++++--
lms/djangoapps/courseware/tests/test_views.py | 39 +++++++++-
lms/djangoapps/courseware/url_helpers.py | 4 +-
lms/djangoapps/courseware/views/views.py | 2 +-
6 files changed, 153 insertions(+), 18 deletions(-)
diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py
index e89520ab6e5..5c5bd97bea9 100644
--- a/common/lib/xmodule/xmodule/modulestore/search.py
+++ b/common/lib/xmodule/xmodule/modulestore/search.py
@@ -5,12 +5,14 @@ from logging import getLogger
from six.moves import range
+from lms.djangoapps.courseware.masquerade import MASQUERADE_SETTINGS_KEY
+from student.roles import GlobalStaff
from .exceptions import ItemNotFoundError, NoPathToItem
LOGGER = getLogger(__name__)
-def path_to_location(modulestore, usage_key, full_path=False):
+def path_to_location(modulestore, usage_key, request=None, full_path=False):
'''
Try to find a course_id/chapter/section[/position] path to location in
modulestore. The courseware insists that the first level in the course is
@@ -19,6 +21,7 @@ def path_to_location(modulestore, usage_key, full_path=False):
Args:
modulestore: which store holds the relevant objects
usage_key: :class:`UsageKey` the id of the location to which to generate the path
+ request: Request object containing information about user and masquerade settings, Default is None
full_path: :class:`Bool` if True, return the full path to location. Default is False.
Raises
@@ -104,6 +107,7 @@ def path_to_location(modulestore, usage_key, full_path=False):
# (e.g. sequential and videosequence) currently deal with this form of
# representing nested positions. This needs to happen before jumping to a
# module nested in more than one positional module will work.
+
if n > 3:
position_list = []
for path_index in range(2, n - 1):
@@ -112,18 +116,54 @@ def path_to_location(modulestore, usage_key, full_path=False):
section_desc = modulestore.get_item(path[path_index])
# this calls get_children rather than just children b/c old mongo includes private children
# in children but not in get_children
- child_locs = [c.location for c in section_desc.get_children()]
+ child_locs = get_child_locations(section_desc, request, course_id)
# positions are 1-indexed, and should be strings to be consistent with
# url parsing.
- position_list.append(str(child_locs.index(path[path_index + 1]) + 1))
+ if path[path_index + 1] in child_locs:
+ position_list.append(str(child_locs.index(path[path_index + 1]) + 1))
position = "_".join(position_list)
return (course_id, chapter, section, vertical, position, path[-1])
+def get_child_locations(section_desc, request, course_id):
+ """
+ Returns all child locations for a section. If user is learner or masquerading as learner,
+ staff only blocks are excluded.
+ """
+ is_staff_user = GlobalStaff().has_user(request.user) if request else False
+
+ def is_masquerading_as_student():
+ """
+ Return True if user is masquerading as learner.
+ """
+ masquerade_settings = request.session.get(MASQUERADE_SETTINGS_KEY, {})
+ course_info = masquerade_settings.get(course_id)
+ return masquerade_settings and course_info and getattr(course_info, 'role', '') == 'student'
+
+ def is_user_staff_and_not_masquerading_learner():
+ """
+ Return True if user is staff and not masquerading as learner.
+ """
+ return is_staff_user and not is_masquerading_as_student()
+
+ def is_child_appendable(child_instance):
+ """
+ Return True if child is appendable based on request and request's user type.
+ """
+ return (request and is_user_staff_and_not_masquerading_learner()) or not child_instance.visible_to_staff_only
+
+ child_locs = []
+ for child in section_desc.get_children():
+ if not is_child_appendable(child):
+ continue
+ child_locs.append(child.location)
+ return child_locs
+
+
def navigation_index(position):
"""
- Get the navigation index from the position argument (where the position argument was recieved from a call to
+ Get the navigation index from the position argument (where the position argument was received from a call to
path_to_location)
Argument:
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
index 3dbc6a9addf..8eeb6782625 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
@@ -1651,7 +1651,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 8-9. get vertical, compute inheritance
# 10-11. get other vertical_x1b (why?) and compute inheritance
# Split: active_versions & structure
- @ddt.data((ModuleStoreEnum.Type.mongo, [12, 3], 0), (ModuleStoreEnum.Type.split, [2, 2], 0))
+ @ddt.data((ModuleStoreEnum.Type.mongo, [12, 3], 0), (ModuleStoreEnum.Type.split, [3, 2], 0))
@ddt.unpack
def test_path_to_location(self, default_ms, num_finds, num_sends):
"""
diff --git a/common/test/acceptance/tests/lms/test_lms_dashboard.py b/common/test/acceptance/tests/lms/test_lms_dashboard.py
index 96209c432c7..fec1e849133 100644
--- a/common/test/acceptance/tests/lms/test_lms_dashboard.py
+++ b/common/test/acceptance/tests/lms/test_lms_dashboard.py
@@ -7,11 +7,13 @@ from __future__ import absolute_import
import datetime
import re
import six
-
from six.moves.urllib.parse import unquote # pylint: disable=import-error
-from common.test.acceptance.fixtures.course import CourseFixture
+from common.test.acceptance.fixtures.course import CourseFixture, XBlockFixtureDesc
from common.test.acceptance.pages.common.auto_auth import AutoAuthPage
+from common.test.acceptance.pages.lms.course_home import CourseHomePage
from common.test.acceptance.pages.lms.dashboard import DashboardPage
+from common.test.acceptance.pages.lms.problem import ProblemPage
+from common.test.acceptance.pages.lms.staff_view import StaffPreviewPage
from common.test.acceptance.tests.helpers import UniqueCourseTest, generate_course_key
DEFAULT_SHORT_DATE_FORMAT = u'{dt:%b} {dt.day}, {dt.year}'
@@ -125,8 +127,24 @@ class BaseLmsDashboardTestMultiple(UniqueCourseTest):
u"social_sharing_url": {u"value": "http://custom/course/url"},
u"cert_name_long": {u"value": value['cert_name_long']}
})
-
- course_fixture.install()
+ course_fixture.add_children(
+ XBlockFixtureDesc('chapter', 'Test Section 1').add_children(
+ XBlockFixtureDesc('sequential', 'Test Subsection 1,1').add_children(
+ XBlockFixtureDesc('problem', 'Test Problem 1', data='<problem>problem 1 dummy body</problem>'),
+ XBlockFixtureDesc('html', 'html 1', data="<html>html 1 dummy body</html>"),
+ XBlockFixtureDesc('problem', 'Test Problem 2', data="<problem>problem 2 dummy body</problem>"),
+ XBlockFixtureDesc('html', 'html 2', data="<html>html 2 dummy body</html>"),
+ ),
+ XBlockFixtureDesc('sequential', 'Test Subsection 1,2').add_children(
+ XBlockFixtureDesc('problem', 'Test Problem 3', data='<problem>problem 3 dummy body</problem>'),
+ ),
+ XBlockFixtureDesc(
+ 'sequential', 'Test HIDDEN Subsection', metadata={'visible_to_staff_only': True}
+ ).add_children(
+ XBlockFixtureDesc('problem', 'Test HIDDEN Problem', data='<problem>hidden problem</problem>'),
+ ),
+ )
+ ).install()
self.course_keys[key] = course_key
self.course_fixtures[key] = course_fixture
@@ -201,7 +219,6 @@ class LmsDashboardPageTest(BaseLmsDashboardTest):
Scenario:
Course Date should have the format 'Ended - Sep 23, 2015'
if the course on student dashboard has ended.
-
As a Student,
Given that I have enrolled to a course
And the course has ended in the past
@@ -233,7 +250,6 @@ class LmsDashboardPageTest(BaseLmsDashboardTest):
Scenario:
Course Date should have the format 'Started - Sep 23, 2015'
if the course on student dashboard is running.
-
As a Student,
Given that I have enrolled to a course
And the course has started
@@ -266,7 +282,6 @@ class LmsDashboardPageTest(BaseLmsDashboardTest):
Scenario:
Course Date should have the format 'Starts - Sep 23, 2015'
if the course on student dashboard starts in future.
-
As a Student,
Given that I have enrolled to a course
And the course starts in future
@@ -300,7 +315,6 @@ class LmsDashboardPageTest(BaseLmsDashboardTest):
Scenario:
Course Date should have the format 'Starts - Wednesday at 5am UTC'
if the course on student dashboard starts within 5 days.
-
As a Student,
Given that I have enrolled to a course
And the course starts within 5 days
@@ -436,3 +450,49 @@ class LmsDashboardA11yTest(BaseLmsDashboardTestMultiple):
course_listings = self.dashboard_page.get_courses()
self.assertEqual(len(course_listings), 3)
self.dashboard_page.a11y_audit.check_for_accessibility_errors()
+
+
+class TestMasqueradeAndSwitchCourse(BaseLmsDashboardTestMultiple):
+ """
+ Class to test lms dashboard accessibility of courses when masquerading as learner.
+ """
+
+ def test_masquerade_and_switch_course(self):
+ """
+ Scenario:
+ Staff user should be able to access other courses after
+ masquerading as student in one course
+
+ As Staff user, Select a course
+ When I click to change view from Staff to Learner
+ Then the first subsection from course outline should be visible as Learner
+ When I click to select a different course
+ Then the first subsection from new course outline should be visible as Staff
+ """
+ AutoAuthPage(
+ self.browser,
+ username=self.username,
+ email=self.email,
+ staff=True
+ ).visit()
+ self.dashboard_page.visit()
+
+ section_title = 'Test Section 1'
+ subsection_title = 'Test Subsection 1,1'
+ course_page = CourseHomePage(self.browser, str(self.course_keys['A']))
+ course_page.visit()
+
+ problem_name = u'Test Problem 1'
+
+ staff_page = StaffPreviewPage(self.browser)
+ staff_page.set_staff_view_mode('Learner')
+
+ course_page.outline.go_to_section(section_title, subsection_title)
+ self.assertEqual(staff_page.staff_view_mode, 'Learner')
+ self.assertEqual(ProblemPage(self.browser).problem_name, problem_name)
+
+ course_page.course_id = str(self.course_keys['B'])
+ course_page.visit()
+ course_page.outline.go_to_section(section_title, subsection_title)
+ self.assertNotEqual(staff_page.staff_view_mode, 'Learner')
+ self.assertEqual(ProblemPage(self.browser).problem_name, problem_name)
diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py
index aa0021bfbbf..d79ed2f7e70 100644
--- a/lms/djangoapps/courseware/tests/test_views.py
+++ b/lms/djangoapps/courseware/tests/test_views.py
@@ -25,7 +25,6 @@ from freezegun import freeze_time
from markupsafe import escape
from milestones.tests.utils import MilestonesTestCaseMixin
from mock import MagicMock, PropertyMock, create_autospec, patch
-from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator
from pytz import UTC
from six import text_type
@@ -62,6 +61,7 @@ from lms.djangoapps.commerce.utils import EcommerceService
from lms.djangoapps.grades.config.waffle import ASSUME_ZERO_GRADE_IF_ABSENT
from lms.djangoapps.grades.config.waffle import waffle as grades_waffle
from lms.djangoapps.verify_student.services import IDVerificationService
+from opaque_keys.edx.keys import CourseKey, UsageKey
from openedx.core.djangoapps.catalog.tests.factories import CourseFactory as CatalogCourseFactory
from openedx.core.djangoapps.catalog.tests.factories import CourseRunFactory, ProgramFactory
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
@@ -104,6 +104,7 @@ FEATURES_WITH_DISABLE_HONOR_CERTIFICATE = settings.FEATURES.copy()
FEATURES_WITH_DISABLE_HONOR_CERTIFICATE['DISABLE_HONOR_CERTIFICATES'] = True
+@ddt.ddt
class TestJumpTo(ModuleStoreTestCase):
"""
Check the jumpto link for a course.
@@ -191,7 +192,6 @@ class TestJumpTo(ModuleStoreTestCase):
module2 = ItemFactory.create(category='html', parent_location=nested_vertical2.location)
# internal position of module2 will be 1_2 (2nd item withing 1st item)
-
expected = '/courses/{course_id}/courseware/{chapter_id}/{section_id}/1?{activate_block_id}'.format(
course_id=six.text_type(course.id),
chapter_id=chapter.url_name,
@@ -215,6 +215,41 @@ class TestJumpTo(ModuleStoreTestCase):
response = self.client.get(jumpto_url)
self.assertEqual(response.status_code, 404)
+ @ddt.data(
+ (False, '1'),
+ (True, '2')
+ )
+ @ddt.unpack
+ def test_jump_to_for_learner_with_staff_only_content(self, is_staff_user, position):
+ """
+ Test for checking correct position in redirect_url for learner when a course has staff-only units.
+ """
+ course = CourseFactory.create()
+ request = RequestFactory().get('/')
+ request.user = UserFactory(is_staff=is_staff_user, username="staff")
+ request.session = {}
+ course_key = CourseKey.from_string(six.text_type(course.id))
+ chapter = ItemFactory.create(category='chapter', parent_location=course.location)
+ section = ItemFactory.create(category='sequential', parent_location=chapter.location)
+ __ = ItemFactory.create(category='vertical', parent_location=section.location)
+ staff_only_vertical = ItemFactory.create(category='vertical', parent_location=section.location,
+ metadata=dict(visible_to_staff_only=True))
+ __ = ItemFactory.create(category='vertical', parent_location=section.location)
+
+ usage_key = UsageKey.from_string(six.text_type(staff_only_vertical.location)).replace(course_key=course_key)
+ expected_url = reverse(
+ 'courseware_position',
+ kwargs={
+ 'course_id': six.text_type(course.id),
+ 'chapter': chapter.url_name,
+ 'section': section.url_name,
+ 'position': position,
+ }
+ )
+ expected_url += "?{}".format(urlencode({'activate_block_id': six.text_type(staff_only_vertical.location)}))
+
+ self.assertEqual(expected_url, get_redirect_url(course_key, usage_key, request))
+
@ddt.ddt
class IndexQueryTestCase(ModuleStoreTestCase):
diff --git a/lms/djangoapps/courseware/url_helpers.py b/lms/djangoapps/courseware/url_helpers.py
index 0cf00a194bd..2f753407b6e 100644
--- a/lms/djangoapps/courseware/url_helpers.py
+++ b/lms/djangoapps/courseware/url_helpers.py
@@ -11,7 +11,7 @@ from xmodule.modulestore.django import modulestore
from xmodule.modulestore.search import navigation_index, path_to_location
-def get_redirect_url(course_key, usage_key):
+def get_redirect_url(course_key, usage_key, request=None):
""" Returns the redirect url back to courseware
Args:
@@ -28,7 +28,7 @@ def get_redirect_url(course_key, usage_key):
(
course_key, chapter, section, vertical_unused,
position, final_target_id
- ) = path_to_location(modulestore(), usage_key)
+ ) = path_to_location(modulestore(), usage_key, request)
# choose the appropriate view (and provide the necessary args) based on the
# args provided by the redirect.
diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py
index 33dff1d60cb..da08896d562 100644
--- a/lms/djangoapps/courseware/views/views.py
+++ b/lms/djangoapps/courseware/views/views.py
@@ -355,7 +355,7 @@ def jump_to(_request, course_id, location):
except InvalidKeyError:
raise Http404(u"Invalid course_key or usage_key")
try:
- redirect_url = get_redirect_url(course_key, usage_key)
+ redirect_url = get_redirect_url(course_key, usage_key, _request)
except ItemNotFoundError:
raise Http404(u"No data at this location: {0}".format(usage_key))
except NoPathToItem:
--
GitLab