Skip to content

Add Studio/Insights links to Staff Instructor Toolbar #24020

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.

Sign up for GitHub

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

Merged
stvstnfrd merged 2 commits into from
Jun 3, 2020
Merged

Add Studio/Insights links to Staff Instructor Toolbar #24020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43ac90e
Add Studio and Insights buttons to Instructor Toolbar
stvstnfrd May 15, 2020
4490e4c
Remove 'View In Studio' link from vertical
stvstnfrd May 18, 2020
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
98 changes: 0 additions & 98 deletions lms/djangoapps/courseware/tests/test_module_render.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1712,104 +1712,6 @@ def test_json_init_data(self, json_data, json_output):
self.assertEqual(html.count("</script>"), 1)


class ViewInStudioTest(ModuleStoreTestCase):
Copy link
Contributor Author

@stvstnfrd stvstnfrd May 19, 2020

Choose a reason for hiding this comment

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

These tests seem fairly module-level specific, so probably not worth porting over as-is.

How much, if any, do we care to add testing here for the legacy view/component?
Spoiler: I've not added any at this point :)

"""Tests for the 'View in Studio' link visiblity."""

def setUp(self):
""" Set up the user and request that will be used. """
super(ViewInStudioTest, self).setUp()
self.staff_user = GlobalStaffFactory.create()
self.request = RequestFactoryNoCsrf().get('/')
self.request.user = self.staff_user
self.request.session = {}
self.module = None
self.default_context = {'bookmarked': False, 'username': self.user.username}

def _get_module(self, course_id, descriptor, location):
"""
Get the module from the course from which to pattern match (or not) the 'View in Studio' buttons
"""
field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
course_id,
self.staff_user,
descriptor
)

return render.get_module(
self.staff_user,
self.request,
location,
field_data_cache,
)

def setup_mongo_course(self, course_edit_method='Studio'):
""" Create a mongo backed course. """
course = CourseFactory.create(
course_edit_method=course_edit_method
)

descriptor = ItemFactory.create(
category='vertical',
parent_location=course.location,
)

child_descriptor = ItemFactory.create(
category='vertical',
parent_location=descriptor.location
)

self.module = self._get_module(course.id, descriptor, descriptor.location)

# pylint: disable=attribute-defined-outside-init
self.child_module = self._get_module(course.id, child_descriptor, child_descriptor.location)


class MongoViewInStudioTest(ViewInStudioTest):
"""Test the 'View in Studio' link visibility in a mongo backed course."""

def test_view_in_studio_link_studio_course(self):
"""Regular Studio courses should see 'View in Studio' links."""
self.setup_mongo_course()
result_fragment = self.module.render(STUDENT_VIEW, context=self.default_context)
self.assertIn('View Unit in Studio', result_fragment.content)

def test_view_in_studio_link_only_in_top_level_vertical(self):
"""Regular Studio courses should not see 'View in Studio' for child verticals of verticals."""
self.setup_mongo_course()
# Render the parent vertical, then check that there is only a single "View Unit in Studio" link.
result_fragment = self.module.render(STUDENT_VIEW, context=self.default_context)
# The single "View Unit in Studio" link should appear before the first xmodule vertical definition.
parts = result_fragment.content.split('data-block-type="vertical"')
self.assertEqual(3, len(parts), "Did not find two vertical blocks")
self.assertIn('View Unit in Studio', parts[0])
self.assertNotIn('View Unit in Studio', parts[1])
self.assertNotIn('View Unit in Studio', parts[2])

def test_view_in_studio_link_xml_authored(self):
"""Courses that change 'course_edit_method' setting can hide 'View in Studio' links."""
self.setup_mongo_course(course_edit_method='XML')
result_fragment = self.module.render(STUDENT_VIEW, context=self.default_context)
self.assertNotIn('View Unit in Studio', result_fragment.content)


class MixedViewInStudioTest(ViewInStudioTest):
"""Test the 'View in Studio' link visibility in a mixed mongo backed course."""

MODULESTORE = TEST_DATA_MIXED_MODULESTORE

def test_view_in_studio_link_mongo_backed(self):
"""Mixed mongo courses that are mongo backed should see 'View in Studio' links."""
self.setup_mongo_course()
result_fragment = self.module.render(STUDENT_VIEW, context=self.default_context)
self.assertIn('View Unit in Studio', result_fragment.content)

def test_view_in_studio_link_xml_authored(self):
"""Courses that change 'course_edit_method' setting can hide 'View in Studio' links."""
self.setup_mongo_course(course_edit_method='XML')
result_fragment = self.module.render(STUDENT_VIEW, context=self.default_context)
self.assertNotIn('View Unit in Studio', result_fragment.content)


@XBlock.tag("detached")
class DetachedXBlock(XBlock):
"""
Expand Down
4 changes: 4 additions & 0 deletions lms/static/sass/course/layout/_courseware_preview.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ $proctoring-banner-text-size: 14px;
@extend %inner-wrapper;

width: auto;

a.btn {
margin: 5px 0 5px 5px;
Copy link
Contributor Author

@stvstnfrd stvstnfrd Jun 2, 2020

Choose a reason for hiding this comment

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

This handles the stacking display, as well as removing the need for the &nbsp;.

}
}

.preview-actions {
Expand Down
7 changes: 0 additions & 7 deletions lms/templates/edit_unit_link.html
View file
Open in desktop

This file was deleted.

76 changes: 72 additions & 4 deletions lms/templates/preview_menu.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,32 @@
staff_selected = selected(not masquerade or masquerade.role != "student")
specific_student_selected = selected(not staff_selected and masquerade.user_name)
student_selected = selected(not staff_selected and not specific_student_selected and not masquerade_group_id)

if settings.HTTPS == 'on':
protocol = 'https'
else:
protocol = 'http'
insights_base_url = settings.ANALYTICS_DASHBOARD_URL
studio_base_url = ''
if settings.CMS_BASE:
studio_base_url = protocol + '://' + settings.CMS_BASE
insights_url = insights_base_url
studio_url = studio_base_url
if course and course.id:
if insights_base_url:
insights_url = "{base_url}/{course_id}".format(
base_url=insights_base_url,
course_id=course.id,
)
if studio_base_url:
studio_url = "{base_url}/course/{course_id}".format(
base_url=studio_base_url,
course_id=course.id,
)
%>
<nav class="wrapper-preview-menu" aria-label="${_('Course View')}">
<div class="preview-menu" style="display: flex; align-items: center;">
Copy link
Contributor

@davidjoy davidjoy Jun 2, 2020

Choose a reason for hiding this comment

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

Could I suggest changing the align-items here to start instead of center? I think having the dropdown aligned to the top when the window width is shrunken down will look a bit cleaner.

Copy link
Contributor

@davidjoy davidjoy Jun 2, 2020

Choose a reason for hiding this comment

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

Otherwise, I screwed around with this for a while just now and came up with much the same layout.

Copy link
Contributor Author

@stvstnfrd stvstnfrd Jun 2, 2020

Choose a reason for hiding this comment

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

Hrmm, it might look a little better when shrunken down, but I think it may look worse when wide, since it's all on a single line and no longer centered vertically on the one side.

center, shrunk

Screenshot_2020-06-02_08-10-24

start, shrunk

Screenshot_2020-06-02_12-09-20

start, wide

Screenshot_2020-06-02_12-07-14

center, wide

Screenshot_2020-06-02_08-09-27

Copy link
Contributor

@davidjoy davidjoy Jun 2, 2020

Choose a reason for hiding this comment

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

Oh, darn. Yup, looks like junk with start. Ah well.

<ol class="preview-actions">
<ol class="preview-actions" style="flex-grow: 1;">
<li class="action-preview">
<form action="#" class="action-preview-form" method="post">
<label for="action-preview-select" class="action-preview-label">${_("View this course as:")}</label>
Expand Down Expand Up @@ -66,13 +88,59 @@
</p>
</div>
% endif
<div style="flex-shrink: 1; text-align: center;">
% if microfrontend_link:
<div style="flex-grow: 1; text-align: right;">
<a class="btn btn-primary" style="border: solid 1px white;" href="${microfrontend_link}">
${_("View this unit in the new experience")}
${_("View in the new experience")}
</a>
</div>
% endif
% if studio_url:
<a
class="btn btn-primary view-in-studio"
style="border: solid 1px white;"
href="${studio_url}"
data-studio-base-url="${studio_base_url}"
>
${_("View in Studio")}
</a>
<script type="text/javascript">
$(function () {
$('.wrapper-preview-menu a.view-in-studio').click(function (event) {
/*
When we start rendering this template, we
don't yet have a vertical in the context
(that happens in a separate `render` call,
if this is even on a unit-level page),
so we dynamically lookup the vertical element in
the DOM and use its data-* attributes to
construct a Studio link directly to this unit.
If no vertical is present, the link behaves as
normal.
*/
var verticals = $('.xblock-student_view-vertical');
if (verticals.length > 0) {
var blockId = verticals[0].dataset.usageId;
var url = [
this.dataset.studioBaseUrl,
'container',
blockId,
].join('/');
window.location.href = url;
return false;
}
// There's no vertical, so let the event
// bubble up, ie, work like a normal link.
return true;
});
});
</script>
% endif
% if insights_url:
<a class="btn btn-primary" style="border: solid 1px white;" href="${insights_url}">
${_("View in Insights")}
</a>
% endif
</div>
</div>
</nav>

Expand Down
18 changes: 1 addition & 17 deletions openedx/core/lib/xblock_utils/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,23 +323,7 @@ def add_staff_markup(user, disable_staff_debug_info, block, view, frag, context)
return frag
# TODO: make this more general, eg use an XModule attribute instead
if isinstance(block, VerticalBlock) and (not context or not context.get('child_of_vertical', False)):
# check that the course is a mongo backed Studio course before doing work
is_studio_course = block.course_edit_method == "Studio"
Copy link
Contributor

@davidjoy davidjoy May 29, 2020

Choose a reason for hiding this comment

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

I'm not 100% on the details here, but where is the equivalent of this check in the new code? Seems like in the old code we don't show the link unless the course_edit_method is "Studio", but I don't see a similar condition in the new stuff.

Copy link
Contributor Author

@stvstnfrd stvstnfrd Jun 2, 2020

Choose a reason for hiding this comment

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

Hrmmm, good point...

We're also not performing this check in the MFE; we always link to Studio.

@davidjoy With this implemented as-is, I don't think we can perform this check; we don't have access to the block, so I think we'd need another approach.

@marcotuts (cc: @ormsbee) Do you have background on this feature, its use/exposure/etc?

Depending on that, we should determine:

  1. If we care about this check, do we need to port this check to the MFE too? (now or backlog)
  2. If we don't, do we care enough to preserve backward compatibility here? (now or backlog)
  3. Or just ignore it and always link to the block in Studio?


if is_studio_course:
# build edit link to unit in CMS. Can't use reverse here as lms doesn't load cms's urls.py
edit_link = "//" + settings.CMS_BASE + '/container/' + text_type(block.location)
Copy link
Contributor Author

@stvstnfrd stvstnfrd May 19, 2020

Choose a reason for hiding this comment

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

Note for later: the // prefix..

Copy link
Contributor Author

@stvstnfrd stvstnfrd May 19, 2020

Choose a reason for hiding this comment

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

Note: No algorithm, no helper method, so we weren't off by creating the URL the same way in the MFE :)

Copy link
Contributor

@davidjoy davidjoy May 29, 2020

Choose a reason for hiding this comment

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

We found some similar limitations in studio for building LMS URLs... there wasn't really a good way to 'reverse' those routes, so to speak. Ended up looking much like this.

We found, also, that studio didn't have a fully reliable LMS base URL, in that if there were multi-tenancy or enterprise domains, it wasn't necessarily right. More details here: https://openedx.atlassian.net/browse/TNL-7198


# return edit link in rendered HTML for display
return wrap_fragment(
frag,
render_to_string(
"edit_unit_link.html",
{'frag_content': frag.content, 'edit_link': edit_link}
)
)
else:
return frag
return frag

if isinstance(block, SequenceModule) or getattr(block, 'HIDDEN', False):
return frag
Expand Down