feat!: Change the way tabs are ordered [BD-38] [TNL-9174] [BB-5076] (#29262)

* feat!: Change the way tabs are ordered
The change imposes a new ordering for tabs based on their new priority. When reordering tabs, this ordering will be maintained.

* fix: Apply suggestions from code review

Co-authored-by: Farhaan Bukhsh <farhaan@opencraft.com>

* fix: review feedback

Co-authored-by: Farhaan Bukhsh <farhaan@opencraft.com>
This commit is contained in:
Kshitij Sobti
2021-11-22 06:17:30 +00:00
committed by GitHub
parent 2e2701d82a
commit e8e8f4acbe
16 changed files with 115 additions and 140 deletions

View File

@@ -6,6 +6,7 @@ import json
from urllib.parse import urlencode
import ddt
import random
from django.urls import reverse
from xmodule.modulestore.tests.factories import ItemFactory
from xmodule.tabs import CourseTabList
@@ -95,36 +96,38 @@ class TabsAPITests(CourseTestCase):
content_type="application/json",
)
def test_reorder_tabs(self):
def test_reorder_static_tabs(self):
"""
Test re-ordering of tabs
Test re-ordering of static tabs in a course.
"""
# get the original tab ids
orig_tab_ids = [tab.tab_id for tab in self.course.tabs]
tab_ids = list(orig_tab_ids)
num_orig_tabs = len(orig_tab_ids)
# get the original tabs
course_tabs = list(self.course.tabs)
num_orig_tabs = len(self.course.tabs)
# make sure we have enough tabs to play around with
assert num_orig_tabs >= 5
# reorder the last two tabs
tab_ids[num_orig_tabs - 1], tab_ids[num_orig_tabs - 2] = tab_ids[num_orig_tabs - 2], tab_ids[num_orig_tabs - 1]
# Randomize the order of static tabs, leaving the rest intact
course_tabs.sort(key=lambda tab: (100 + random.random()) if tab.type == 'static_tab' else tab.priority)
# remove the third to the last tab, the removed tab has to be a static tab
# (the code needs to handle the case where tabs requested for re-ordering is a subset of the tabs in the course)
removed_tab = tab_ids.pop(num_orig_tabs - 3)
self.assertEqual(len(tab_ids), num_orig_tabs - 1)
tabs_data = [
{'tab_locator': str(self.course.id.make_usage_key("static_tab", tab.url_slug))}
for tab in course_tabs
if tab.type == 'static_tab'
]
# Remove one tab randomly. This shouldn't delete the tab.
tabs_data.pop()
# post the request
resp = self.make_reorder_tabs_request([{"tab_id": tab_id} for tab_id in tab_ids if 'static' in tab_id])
# post the request with the reordered static tabs only
resp = self.make_reorder_tabs_request(tabs_data)
assert resp.status_code == 204
# reload the course and verify the new tab order
# Reload the course and verify the new tab order
self.reload_course()
reordered_tab_ids = [tab.tab_id for tab in course_tabs]
new_tab_ids = [tab.tab_id for tab in self.course.tabs]
assert new_tab_ids == tab_ids + [removed_tab]
assert new_tab_ids != orig_tab_ids
assert new_tab_ids == reordered_tab_ids
def test_reorder_tabs_invalid_list(self):
"""

View File

@@ -24,7 +24,7 @@ class AdvancedCourseSettingsView(DeveloperErrorViewMixin, APIView):
class FilterQuery(forms.Form):
"""
Form for validating query marameters passed to advanced course settings view
Form for validating query parameters passed to advanced course settings view
to filter the data it returns.
"""
filter_fields = forms.CharField(strip=True, empty_value=None, required=False)

View File

@@ -177,7 +177,7 @@ class CourseTabReorderView(DeveloperErrorViewMixin, APIView):
return super().handle_exception(exc)
@apidocs.schema(
body=[TabIDLocatorSerializer],
body=TabIDLocatorSerializer(many=True),
parameters=[
apidocs.string_parameter("course_id", apidocs.ParameterLocation.PATH, description="Course ID"),
],
@@ -198,24 +198,12 @@ class CourseTabReorderView(DeveloperErrorViewMixin, APIView):
Move course tabs:
POST /api/contentstore/v0/tabs/{course_id}/reorder [
{
"tab_id": "info"
},
{
"tab_id": "courseware"
},
{
"tab_locator": "block-v1:TstX+DemoX+Demo+type@static_tab+block@d26fcb0e93824fbfa5c9e5f100e2511a"
},
{
"tab_id": "wiki"
"tab_locator": "block-v1:TstX+DemoX+Demo+type@static_tab+block@a011f1bd05af4578ae397ed8cabccf62"
},
{
"tab_id": "discussion"
},
{
"tab_id": "progress"
}
]
@@ -233,7 +221,7 @@ class CourseTabReorderView(DeveloperErrorViewMixin, APIView):
tab_id_locators.is_valid(raise_exception=True)
reorder_tabs_handler(
course_module,
{"tabs": tab_id_locators.validated_data},
tab_id_locators.validated_data,
request.user,
)
return Response(status=status.HTTP_204_NO_CONTENT)

View File

@@ -1,7 +1,7 @@
"""
Views related to course tabs
"""
from typing import Dict, Iterable, List, Optional
from typing import Dict, Iterable, List, Optional, Union
from django.contrib.auth import get_user_model
from django.contrib.auth.decorators import login_required
@@ -110,7 +110,7 @@ def update_tabs_handler(course_item: CourseBlock, tabs_data: Dict, user: User) -
"""
if "tabs" in tabs_data:
reorder_tabs_handler(course_item, tabs_data, user)
reorder_tabs_handler(course_item, tabs_data["tabs"], user)
elif "tab_id_locator" in tabs_data:
edit_tab_handler(course_item, tabs_data, user)
else:
@@ -122,58 +122,45 @@ def reorder_tabs_handler(course_item, tabs_data, user):
Helper function for handling reorder of static tabs request
"""
# Tabs are identified by tab_id or locators.
# The locators are used to identify static tabs since they are xmodules.
# Static tabs are identified by locators (a UsageKey) instead of a tab id like
# other tabs. These can be used to identify static tabs since they are xmodules.
# Although all tabs have tab_ids, newly created static tabs do not know
# their tab_ids since the xmodule editor uses only locators to identify new objects.
requested_tab_id_locators = tabs_data["tabs"]
#get original tab list of only static tabs with their original index(position) in the full course tabs list
old_tab_dict = {}
for idx, tab in enumerate(course_item.tabs):
if isinstance(tab, StaticTab):
old_tab_dict[tab] = idx
old_tab_list = list(old_tab_dict.keys())
new_tab_list = create_new_list(requested_tab_id_locators, old_tab_list)
# Creates a full new course tab list of both default and static course tabs
# by looping through the new tab list of static only tabs and
# putting them in their new position in the list of course item tabs
# original_idx gives the list of positions of all static tabs in course tabs originally
full_new_tab_list = course_item.tabs
original_idx = list(old_tab_dict.values())
for i in range(len(new_tab_list)):
full_new_tab_list[original_idx[i]] = new_tab_list[i]
new_tab_list = create_new_list(tabs_data, course_item.tabs)
# validate the tabs to make sure everything is Ok (e.g., did the client try to reorder unmovable tabs?)
try:
CourseTabList.validate_tabs(full_new_tab_list)
CourseTabList.validate_tabs(new_tab_list)
except InvalidTabsException as exception:
raise ValidationError({"error": f"New list of tabs is not valid: {str(exception)}."}) from exception
# persist the new order of the tabs
course_item.tabs = full_new_tab_list
course_item.tabs = new_tab_list
modulestore().update_item(course_item, user.id)
def create_new_list(requested_tab_id_locators, old_tab_list):
def create_new_list(tab_locators, old_tab_list):
"""
Helper function for creating a new course tab list in the new order
of reordered tabs
Helper function for creating a new course tab list in the new order of
reordered tabs.
It will take tab_locators for static tabs and resolve them to actual tab
instances.
"""
new_tab_list = []
for tab_id_locator in requested_tab_id_locators:
tab = get_tab_by_tab_id_locator(old_tab_list, tab_id_locator)
for tab_locator in tab_locators:
tab = get_tab_by_tab_id_locator(old_tab_list, tab_locator)
if tab is None:
raise ValidationError({"error": f"Tab with id_locator '{tab_id_locator}' does not exist."})
raise ValidationError({"error": f"Tab with id_locator '{tab_locator}' does not exist."})
if not isinstance(tab, StaticTab):
raise ValidationError({"error": f"Cannot reorder tabs of type '{tab.type}'"})
new_tab_list.append(tab)
# the old_tab_list may contain additional tabs that were not rendered in the UI because of
# global or course settings. so add those to the end of the list.
non_displayed_tabs = set(old_tab_list) - set(new_tab_list)
new_tab_list.extend(non_displayed_tabs)
return new_tab_list
return sorted(new_tab_list, key=lambda item: item.priority or float('inf'))
def edit_tab_handler(course_item: CourseBlock, tabs_data: Dict, user: User):
@@ -212,11 +199,12 @@ def get_tab_by_tab_id_locator(tab_list: List[CourseTab], tab_id_locator: Dict[st
return tab
def get_tab_by_locator(tab_list: List[CourseTab], usage_key_string: str) -> Optional[CourseTab]:
def get_tab_by_locator(tab_list: List[CourseTab], tab_location: Union[str, UsageKey]) -> Optional[CourseTab]:
"""
Look for a tab with the specified locator. Returns the first matching tab.
"""
tab_location = UsageKey.from_string(usage_key_string)
if isinstance(tab_location, str):
tab_location = UsageKey.from_string(tab_location)
item = modulestore().get_item(tab_location)
static_tab = StaticTab(
name=item.display_name,

View File

@@ -1,6 +1,8 @@
""" Tests for tab functions (just primitive). """
import json
import random
from cms.djangoapps.contentstore.tests.utils import CourseTestCase
from cms.djangoapps.contentstore.utils import reverse_course_url
@@ -38,9 +40,9 @@ class TabsPageTests(CourseTestCase):
def check_invalid_tab_id_response(self, resp):
"""Verify response is an error listing the invalid_tab_id"""
self.assertEqual(resp.status_code, 400)
assert resp.status_code == 400
resp_content = json.loads(resp.content.decode('utf-8'))
self.assertIn("error", resp_content)
assert "error" in resp_content
def test_not_implemented(self):
"""Verify not implemented errors"""
@@ -75,52 +77,38 @@ class TabsPageTests(CourseTestCase):
def test_reorder_tabs(self):
"""Test re-ordering of tabs"""
# get the original tab ids
orig_tab_ids = [tab.tab_id for tab in self.course.tabs]
tab_ids = list(orig_tab_ids)
num_orig_tabs = len(orig_tab_ids)
# get the original tabs
course_tabs = list(self.course.tabs)
num_orig_tabs = len(self.course.tabs)
# make sure we have enough tabs to play around with
self.assertGreaterEqual(num_orig_tabs, 5)
assert num_orig_tabs >= 5
# reorder the last two tabs
tab_ids[num_orig_tabs - 1], tab_ids[num_orig_tabs - 2] = tab_ids[num_orig_tabs - 2], tab_ids[num_orig_tabs - 1]
# Randomise the order of static tabs, leave the rest intact
course_tabs.sort(key=lambda tab: (100 + random.random()) if tab.type == 'static_tab' else tab.priority)
# remove the third to the last tab, the removed tab has to be a static tab
# (the code needs to handle the case where tabs requested for re-ordering is a subset of the tabs in the course)
removed_tab = tab_ids.pop(num_orig_tabs - 3)
self.assertEqual(len(tab_ids), num_orig_tabs - 1)
tabs_data = [
{'tab_locator': str(self.course.id.make_usage_key("static_tab", tab.url_slug))}
for tab in course_tabs
if tab.type == 'static_tab'
]
# Remove one tab randomly. This shouldn't delete the tab.
tabs_data.pop()
# post the request with the reordered static tabs only
resp = self.client.ajax_post(
self.url,
data={'tabs': [{'tab_id': tab_id} for tab_id in tab_ids if 'static' in tab_id]},
data={
'tabs': tabs_data
},
)
self.assertEqual(resp.status_code, 204)
assert resp.status_code == 204
# reload the course and verify the new tab order
self.reload_course()
reordered_tab_ids = [tab.tab_id for tab in course_tabs]
new_tab_ids = [tab.tab_id for tab in self.course.tabs]
self.assertEqual(new_tab_ids, tab_ids + [removed_tab])
self.assertNotEqual(new_tab_ids, orig_tab_ids)
def test_reorder_tabs_invalid_list(self):
"""Test re-ordering of tabs with invalid tab list"""
orig_tab_ids = [tab.tab_id for tab in self.course.tabs]
tab_ids = list(orig_tab_ids)
# reorder the first two tabs
tab_ids[0], tab_ids[1] = tab_ids[1], tab_ids[0]
# post the request
resp = self.client.ajax_post(
self.url,
data={'tabs': [{'tab_id': tab_id} for tab_id in tab_ids]},
)
self.assertEqual(resp.status_code, 400)
resp_content = json.loads(resp.content.decode('utf-8'))
self.assertIn("error", resp_content)
assert new_tab_ids == reordered_tab_ids
def test_reorder_tabs_invalid_tab(self):
"""Test re-ordering of tabs with invalid tab"""
@@ -141,7 +129,7 @@ class TabsPageTests(CourseTestCase):
old_tab = CourseTabList.get_tab_by_type(self.course.tabs, tab_type)
# visibility should be different from new setting
self.assertNotEqual(old_tab.is_hidden, new_is_hidden_setting)
assert old_tab.is_hidden != new_is_hidden_setting
# post the request
resp = self.client.ajax_post(
@@ -151,12 +139,12 @@ class TabsPageTests(CourseTestCase):
'is_hidden': new_is_hidden_setting,
}),
)
self.assertEqual(resp.status_code, 204)
assert resp.status_code == 204
# reload the course and verify the new visibility setting
self.reload_course()
new_tab = CourseTabList.get_tab_by_type(self.course.tabs, tab_type)
self.assertEqual(new_tab.is_hidden, new_is_hidden_setting)
assert new_tab.is_hidden == new_is_hidden_setting
def test_toggle_tab_visibility(self):
"""Test toggling of tab visibility"""
@@ -182,15 +170,15 @@ class TabsPageTests(CourseTestCase):
preview_url = f'/xblock/{self.test_tabs[0].location}/{STUDENT_VIEW}'
resp = self.client.get(preview_url, HTTP_ACCEPT='application/json')
self.assertEqual(resp.status_code, 200)
assert resp.status_code == 200
resp_content = json.loads(resp.content.decode('utf-8'))
html = resp_content['html']
# Verify that the HTML contains the expected elements
self.assertIn('<span class="action-button-text">Edit</span>', html)
self.assertIn('<span class="sr">Duplicate this component</span>', html)
self.assertIn('<span class="sr">Delete this component</span>', html)
self.assertIn('<span data-tooltip="Drag to reorder" class="drag-handle action"></span>', html)
assert '<span class="action-button-text">Edit</span>' in html
assert '<span class="sr">Duplicate this component</span>' in html
assert '<span class="sr">Delete this component</span>' in html
assert '<span data-tooltip="Drag to reorder" class="drag-handle action"></span>' in html
class PrimitiveTabEdit(ModuleStoreTestCase):
@@ -205,16 +193,17 @@ class PrimitiveTabEdit(ModuleStoreTestCase):
tabs.primitive_delete(course, 1)
with self.assertRaises(IndexError):
tabs.primitive_delete(course, 7)
assert course.tabs[2] != {'type': 'discussion', 'name': 'Discussion'}
tabs.primitive_delete(course, 2)
self.assertNotIn({'type': 'textbooks'}, course.tabs)
assert {'type': 'progress'} not in course.tabs
# Check that discussion has shifted up
self.assertEqual(course.tabs[2], {'type': 'discussion', 'name': 'Discussion'})
assert course.tabs[2] == {'type': 'discussion', 'name': 'Discussion'}
def test_insert(self):
"""Test primitive tab insertion."""
course = CourseFactory.create()
tabs.primitive_insert(course, 2, 'pdf_textbooks', 'aname')
self.assertEqual(course.tabs[2], {'type': 'pdf_textbooks', 'name': 'aname'})
assert course.tabs[2] == {'type': 'pdf_textbooks', 'name': 'aname'}
with self.assertRaises(ValueError):
tabs.primitive_insert(course, 0, 'pdf_textbooks', 'aname')
with self.assertRaises(ValueError):
@@ -225,4 +214,4 @@ class PrimitiveTabEdit(ModuleStoreTestCase):
course = CourseFactory.create()
tabs.primitive_insert(course, 3, 'pdf_textbooks', 'aname')
course2 = modulestore().get_course(course.id)
self.assertEqual(course2.tabs[3], {'type': 'pdf_textbooks', 'name': 'aname'})
assert course2.tabs[3] == {'type': 'pdf_textbooks', 'name': 'aname'}

View File

@@ -9,7 +9,7 @@
from openedx.core.djangolib.js_utils import js_escaped_string
from cms.djangoapps.contentstore.utils import get_pages_and_resources_url
%>
<%block name="title">${_("Pages")}</%block>
<%block name="title">${_("Custom pages")}</%block>
<%block name="bodyclass">is-signedin course view-static-pages</%block>
<%block name="header_extras">
@@ -52,14 +52,14 @@
</nav>
</div>
<h1 class="page-header">
<span class="sr">&gt; </span>${_("Custom Pages")}
<span class="sr">&gt; </span>${_("Custom pages")}
</h1>
% else:
<header class="mast has-actions has-subtitle">
<h1 class="page-header">
<small class="subtitle">${_("Content")}</small>
## Translators: Custom Pages refer to the tabs that appear in the top navigation of each course.
<span class="sr"> > </span>${_("Custom Pages")}
<span class="sr"> > </span>${_("Custom pages")}
</h1>
% endif

View File

@@ -302,6 +302,7 @@ class StaticTab(CourseTab):
type = 'static_tab'
is_default = False # A static tab is never added to a course by default
allow_multiple = True
priority = 100
def __init__(self, tab_dict=None, name=None, url_slug=None):
def link_func(course, reverse_func):
@@ -381,14 +382,14 @@ class CourseTabList(List):
within the course.
"""
course.tabs.extend([
course_tabs = [
CourseTab.load('course_info'),
CourseTab.load('courseware')
])
]
# Presence of syllabus tab is indicated by a course attribute
if hasattr(course, 'syllabus_present') and course.syllabus_present:
course.tabs.append(CourseTab.load('syllabus'))
course_tabs.append(CourseTab.load('syllabus'))
# If the course has a discussion link specified, use that even if we feature
# flag discussions off. Disabling that is mostly a server safety feature
@@ -400,12 +401,16 @@ class CourseTabList(List):
else:
discussion_tab = CourseTab.load('discussion')
course.tabs.extend([
course_tabs.extend([
CourseTab.load('textbooks'),
discussion_tab,
CourseTab.load('wiki'),
CourseTab.load('progress'),
])
# While you should be able to do `tab.priority`, a lot of tests mock tabs to be a dict
# which causes them to throw an error on this line
course_tabs.sort(key=lambda tab: getattr(tab, 'priority', None) or float('inf'))
course.tabs.extend(course_tabs)
@staticmethod
def get_discussion(course):

View File

@@ -18,6 +18,7 @@ class CcxCourseTab(CourseTab):
"""
type = "ccx_coach"
priority = 310
title = gettext_noop("CCX Coach")
view_name = "ccx_coach_dashboard"
is_dynamic = True # The CCX view is dynamically added to the set of tabs when it is enabled

View File

@@ -20,6 +20,7 @@ class WikiTab(EnrolledTab):
view_name = "course_wiki"
is_hideable = True
is_default = False
priority = 70
def __init__(self, tab_dict):
# Default to hidden

View File

@@ -34,7 +34,7 @@ class CoursewareTab(EnrolledTab):
"""
type = 'courseware'
title = gettext_noop('Course')
priority = 10
priority = 11
view_name = 'courseware'
is_movable = False
is_default = False
@@ -69,7 +69,7 @@ class CourseInfoTab(CourseTab):
"""
type = 'course_info'
title = gettext_noop('Home')
priority = 20
priority = 10
view_name = 'info'
tab_id = 'info'
is_movable = False
@@ -86,7 +86,7 @@ class SyllabusTab(EnrolledTab):
"""
type = 'syllabus'
title = gettext_noop('Syllabus')
priority = 30
priority = 80
view_name = 'syllabus'
allow_multiple = True
is_default = False
@@ -104,7 +104,7 @@ class ProgressTab(EnrolledTab):
"""
type = 'progress'
title = gettext_noop('Progress')
priority = 40
priority = 20
view_name = 'progress'
is_hideable = True
is_default = False
@@ -153,7 +153,7 @@ class TextbookTabs(TextbookTabsBase):
A tab representing the collection of all textbook tabs.
"""
type = 'textbooks'
priority = None
priority = 200
view_name = 'book'
@classmethod
@@ -177,7 +177,7 @@ class PDFTextbookTabs(TextbookTabsBase):
A tab representing the collection of all PDF textbook tabs.
"""
type = 'pdf_textbooks'
priority = None
priority = 201
view_name = 'pdf_book'
@classmethod
@@ -196,7 +196,7 @@ class HtmlTextbookTabs(TextbookTabsBase):
A tab representing the collection of all Html textbook tabs.
"""
type = 'html_textbooks'
priority = None
priority = 202
view_name = 'html_book'
@classmethod
@@ -285,7 +285,7 @@ class ExternalLinkCourseTab(LinkTab):
A course tab containing an external link.
"""
type = 'external_link'
priority = None
priority = 110
is_default = False # An external link tab is not added to a course by default
allow_multiple = True
@@ -326,9 +326,9 @@ class DatesTab(EnrolledTab):
A tab representing the relevant dates for a course.
"""
type = "dates"
title = gettext_noop(
"Dates") # We don't have the user in this context, so we don't want to translate it at this level.
priority = 50
# We don't have the user in this context, so we don't want to translate it at this level.
title = gettext_noop("Dates")
priority = 30
view_name = "dates"
is_dynamic = True

View File

@@ -767,12 +767,7 @@ class CourseInfoTabTestCase(TabTestCase):
def test_default_tab(self):
# Verify that the course info tab is the first tab
tabs = get_course_tab_list(self.user, self.course)
# So I know this means course_info is not the first tab, but it is going to be
# retired soon (https://openedx.atlassian.net/browse/TNL-7061) and also it has
# a lower priority than courseware so seems odd that it would ever be first.
# As such, I feel comfortable updating this test so it passes until it is removed
# as part of the linked ticket
assert tabs[1].type == 'course_info'
assert tabs[0].type == 'course_info'
@override_waffle_flag(DISABLE_UNIFIED_COURSE_TAB_FLAG, active=False)
def test_default_tab_for_new_course_experience(self):

View File

@@ -19,7 +19,7 @@ class DiscussionTab(TabFragmentViewMixin, EnrolledTab):
type = 'discussion'
title = gettext_noop('Discussion')
priority = None
priority = 40
view_name = 'forum_form_discussion'
fragment_view_name = 'lms.djangoapps.discussion.views.DiscussionBoardFragmentView'
is_hideable = settings.FEATURES.get('ALLOW_HIDING_DISCUSSION_TAB', False)

View File

@@ -26,6 +26,7 @@ class EdxNotesTab(EnrolledTab):
type = "edxnotes"
title = _("Notes")
view_name = "edxnotes"
priority = 50
@classmethod
def is_enabled(cls, course, user=None):

View File

@@ -81,6 +81,7 @@ class InstructorDashboardTab(CourseTab):
title = gettext_noop('Instructor')
view_name = "instructor_dashboard"
is_dynamic = True # The "Instructor" tab is instead dynamically added when it is enabled
priority = 300
@classmethod
def is_enabled(cls, course, user=None):

View File

@@ -28,6 +28,7 @@ class TeamsTab(EnrolledTab):
type = "teams"
title = _("Teams")
view_name = "teams_dashboard"
priority = 60
@classmethod
def is_enabled(cls, course, user=None):

View File

@@ -195,6 +195,7 @@ class LtiCourseTab(LtiCourseLaunchMixin, EnrolledTab):
A tab to add custom LTI components to a course in a tab.
"""
type = 'lti_tab'
priority = 120
is_default = False
allow_multiple = True
@@ -266,6 +267,7 @@ class DiscussionLtiCourseTab(LtiCourseLaunchMixin, TabFragmentViewMixin, Enrolle
Course tab that loads the associated LTI-based discussion provider in a tab.
"""
type = 'lti_discussion'
priority = 41
allow_multiple = False
is_dynamic = True
title = gettext_lazy("Discussion")