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.

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

Jump to bottom

MA-1919 making mobile handout links accommodate jump to id's and cour… #11602

Merged
merged 1 commit into from
Feb 25, 2016
Merged

MA-1919 making mobile handout links accommodate jump to id's and cour… #11602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
132 changes: 85 additions & 47 deletions lms/djangoapps/mobile_api/course_info/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,60 +84,98 @@ def test_updates(self, new_format):
self.assertIn("Update" + str(num), update_data['content'])


@ddt.ddt
class TestHandouts(MobileAPITestCase, MobileAuthTestMixin, MobileCourseAccessTestMixin, MilestonesTestCaseMixin):
"""
Tests for /api/mobile/v0.5/course_info/{course_id}/handouts
"""
REVERSE_INFO = {'name': 'course-handouts-list', 'params': ['course_id']}

def setUp(self):
super(TestHandouts, self).setUp()

# Deleting handouts fails with split modulestore because the handout has no parent.
# This needs further investigation to determine if it is a bug in the split modulestore.
# pylint: disable=protected-access
self.store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo)

# use toy course with handouts, and make it mobile_available
course_items = import_course_from_xml(self.store, self.user.id, settings.COMMON_TEST_DATA_ROOT, ['toy'])
@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_handouts(self, default_ms):
with self.store.default_store(default_ms):
self.add_mobile_available_toy_course()
response = self.api_response(expected_response_code=200)
self.assertIn("Sample", response.data['handouts_html'])
Copy link
Contributor

Choose a reason for hiding this comment

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

See earlier comment. This should be implemented inside verify_success, according to this module's test configuration using the MobileAPITestCase base class.

Copy link
Contributor

Choose a reason for hiding this comment

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

nevermind. I see you probably can't use verify_success since you need to override the modulestore.


@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_no_handouts(self, default_ms):
with self.store.default_store(default_ms):
self.add_mobile_available_toy_course()

# delete handouts in course
handouts_usage_key = self.course.id.make_usage_key('course_info', 'handouts')
with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, self.course.id):
self.store.delete_item(handouts_usage_key, self.user.id)

response = self.api_response(expected_response_code=200)
self.assertIsNone(response.data['handouts_html'])

@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_empty_handouts(self, default_ms):
with self.store.default_store(default_ms):
self.add_mobile_available_toy_course()

# set handouts to empty tags
handouts_usage_key = self.course.id.make_usage_key('course_info', 'handouts')
underlying_handouts = self.store.get_item(handouts_usage_key)
underlying_handouts.data = "<ol></ol>"
self.store.update_item(underlying_handouts, self.user.id)
response = self.api_response(expected_response_code=200)
self.assertIsNone(response.data['handouts_html'])

@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_handouts_static_rewrites(self, default_ms):
with self.store.default_store(default_ms):
self.add_mobile_available_toy_course()

# check that we start with relative static assets
handouts_usage_key = self.course.id.make_usage_key('course_info', 'handouts')
underlying_handouts = self.store.get_item(handouts_usage_key)
self.assertIn('\'/static/', underlying_handouts.data)

# but shouldn't finish with any
response = self.api_response()
self.assertNotIn('\'/static/', response.data['handouts_html'])

@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_jump_to_id_handout_href(self, default_ms):
with self.store.default_store(default_ms):
self.add_mobile_available_toy_course()

# check that we start with relative static assets
handouts_usage_key = self.course.id.make_usage_key('course_info', 'handouts')
underlying_handouts = self.store.get_item(handouts_usage_key)
underlying_handouts.data = "<a href=\"/jump_to_id/identifier\">Intracourse Link</a>"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a relative URL, why does it start with a leading slash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the style of URL we allow instructors to use when they are creating courses. For example, MIT's "Minds and Machines" course uses:


<ol>
  <a href="/jump_to_id/2b02305f3c494aa0b7f0d43aa0778fa9">Discussion Forum Guidelines</a>
</ol>
<ol>
  <a href="/jump_to_id/095f84e31ecf485bb14b51e7b7991db8">Class Schedule</a>
</ol>

https://studio.edx.org/course_info/course-v1:MITx+24.09x+3T2015#

self.store.update_item(underlying_handouts, self.user.id)

# but shouldn't finish with any
response = self.api_response()
self.assertIn("/courses/{}/jump_to_id/".format(self.course.id), response.data['handouts_html'])

@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_course_url_handout_href(self, default_ms):
with self.store.default_store(default_ms):
self.add_mobile_available_toy_course()

# check that we start with relative static assets
handouts_usage_key = self.course.id.make_usage_key('course_info', 'handouts')
underlying_handouts = self.store.get_item(handouts_usage_key)
underlying_handouts.data = "<a href=\"/course/identifier\">Linked Content</a>"
self.store.update_item(underlying_handouts, self.user.id)

# but shouldn't finish with any
response = self.api_response()
self.assertIn("/courses/{}/".format(self.course.id), response.data['handouts_html'])

def add_mobile_available_toy_course(self):
""" use toy course with handouts, and make it mobile_available """
course_items = import_course_from_xml(
self.store, self.user.id,
settings.COMMON_TEST_DATA_ROOT, ['toy'],
create_if_not_present=True
)
self.course = course_items[0]
self.course.mobile_available = True
self.store.update_item(self.course, self.user.id)

def verify_success(self, response):
super(TestHandouts, self).verify_success(response)
self.assertIn('Sample', response.data['handouts_html'])

def test_no_handouts(self):
self.login_and_enroll()

# delete handouts in course
handouts_usage_key = self.course.id.make_usage_key('course_info', 'handouts')
with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, self.course.id):
self.store.delete_item(handouts_usage_key, self.user.id)

response = self.api_response(expected_response_code=200)
self.assertIsNone(response.data['handouts_html'])

def test_empty_handouts(self):
self.login_and_enroll()

# set handouts to empty tags
handouts_usage_key = self.course.id.make_usage_key('course_info', 'handouts')
underlying_handouts = self.store.get_item(handouts_usage_key)
underlying_handouts.data = "<ol></ol>"
self.store.update_item(underlying_handouts, self.user.id)
response = self.api_response(expected_response_code=200)
self.assertIsNone(response.data['handouts_html'])

def test_handouts_static_rewrites(self):
self.login_and_enroll()

# check that we start with relative static assets
handouts_usage_key = self.course.id.make_usage_key('course_info', 'handouts')
underlying_handouts = self.store.get_item(handouts_usage_key)
self.assertIn('\'/static/', underlying_handouts.data)

# but shouldn't finish with any
response = self.api_response()
self.assertNotIn('\'/static/', response.data['handouts_html'])
41 changes: 27 additions & 14 deletions lms/djangoapps/mobile_api/course_info/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from rest_framework.response import Response

from courseware.courses import get_course_info_section_module
from static_replace import make_static_urls_absolute, replace_static_urls
from static_replace import make_static_urls_absolute
from openedx.core.lib.xblock_utils import get_course_update_items

from ..utils import mobile_view, mobile_course_access
Expand Down Expand Up @@ -46,12 +46,7 @@ def list(self, request, course, *args, **kwargs):
]

for item in updates_to_show:
content = item['content']
content = replace_static_urls(
content,
course_id=course.id,
static_asset_path=course.static_asset_path)
item['content'] = make_static_urls_absolute(request, content)
item['content'] = apply_wrappers_to_content(item['content'], course_updates_module, request)

return Response(updates_to_show)

Expand Down Expand Up @@ -82,14 +77,32 @@ def list(self, request, course, *args, **kwargs):
if course_handouts_module.data == "<ol></ol>":
handouts_html = None
else:
handouts_html = course_handouts_module.data
handouts_html = replace_static_urls(
handouts_html,
course_id=course.id,
static_asset_path=course.static_asset_path
)
handouts_html = make_static_urls_absolute(self.request, handouts_html)
handouts_html = apply_wrappers_to_content(course_handouts_module.data, course_handouts_module, request)
return Response({'handouts_html': handouts_html})
else:
# course_handouts_module could be None if there are no handouts
return Response({'handouts_html': None})


def apply_wrappers_to_content(content, module, request):
"""
Updates a piece of html content with the filter functions stored in its module system, then replaces any
static urls with absolute urls.

Args:
content: The html content to which to apply the content wrappers generated for this module system.
module: The module containing a reference to the module system which contains functions to apply to the
content. These functions include:
* Replacing static url's
* Replacing course url's
* Replacing jump to id url's
request: The request, used to replace static URLs with absolute URLs.

Returns: A piece of html content containing the original content updated by each wrapper.

"""
content = module.system.replace_urls(content)
content = module.system.replace_course_urls(content)
content = module.system.replace_jump_to_id_urls(content)
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 changed from iterating over wrappers to this approach because I think that it is FAR more readable and does not run the risk of unwanted filters being applied to our html content.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good.


return make_static_urls_absolute(request, content)