diff --git a/.gitignore b/.gitignore index b3f7473dc077..1a23b36b2ad2 100644 --- a/.gitignore +++ b/.gitignore @@ -17,6 +17,7 @@ cms/envs/private.py /nbproject .idea/ .redcar/ +codekit-config.json ### OS X artifacts *.DS_Store @@ -48,14 +49,18 @@ reports/ .prereqs_cache .vagrant/ node_modules +.bundle/ +bin/ ### Static assets pipeline artifacts *.scssc +lms/static/css/ lms/static/sass/*.css lms/static/sass/application.scss lms/static/sass/application-extend1.scss lms/static/sass/application-extend2.scss lms/static/sass/course.scss +cms/static/css/ cms/static/sass/*.css ### Logging artifacts diff --git a/AUTHORS b/AUTHORS index 9326b6781a27..700bc6e63879 100644 --- a/AUTHORS +++ b/AUTHORS @@ -97,3 +97,5 @@ Iain Dunning Olivier Marquez Florian Dufour Manuel Freire +Daniel Cebrián Robles +Carson Gee diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b0239dc86b61..4507bcccf915 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,11 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Blades: Fix Numerical input to support mathematical operations. BLD-525. + +Blades: Improve calculator's tooltip accessibility. Add possibility to navigate + through the hints via arrow keys. BLD-533. + LMS: Add feature for providing background grade report generation via Celery instructor task, with reports uploaded to S3. Feature is visible on the beta instructor dashboard. LMS-58 @@ -13,9 +18,38 @@ Blades: Added grading support for LTI module. LTI providers can now grade student's work and send edX scores. OAuth1 based authentication implemented. BLD-384. -LMS: Beta-tester status is now set on a per-course-run basis, rather than being valid - across all runs with the same course name. Old group membership will still work - across runs, but new beta-testers will only be added to a single course run. +LMS: Beta-tester status is now set on a per-course-run basis, rather than being + valid across all runs with the same course name. Old group membership will + still work across runs, but new beta-testers will only be added to a single + course run. + +Blades: Enabled several Video Jasmine tests. BLD-463. + +Studio: Continued modification of Studio pages to follow a RESTful framework. +includes Settings pages, edit page for Subsection and Unit, and interfaces +for updating xblocks (xmodules) and getting their editing HTML. + +LMS: Improve accessibility of inline discussions in courseware. + +Blades: Put 2nd "Hide output" button at top of test box & increase text size for +code response questions. BLD-126. + +Blades: Update the calculator hints tooltip with full information. BLD-400. + +Blades: Fix transcripts 500 error in studio (BLD-530) + +LMS: Add error recovery when a user loads or switches pages in an +inline discussion. + +Blades: Allow multiple strings as the correct answer to a string response +question. BLD-474. + +Blades: a11y - Videos will alert screenreaders when the video is over. + +LMS: Trap focus on the loading element when a user loads more threads +in the forum sidebar to improve accessibility. + +LMS: Add error recovery when a user loads more threads in the forum sidebar. LMS: Add a user-visible alert modal when a forums AJAX request fails. @@ -36,7 +70,8 @@ text like with bold or italics. (BLD-449) LMS: Beta instructor dashboard will only count actively enrolled students for course enrollment numbers. -Blades: Fix speed menu that is not rendered correctly when YouTube is unavailable. (BLD-457). +Blades: Fix speed menu that is not rendered correctly when YouTube is +unavailable. (BLD-457). LMS: Users with is_staff=True no longer have the STAFF label appear on their forum posts. @@ -54,6 +89,9 @@ key in course settings. (BLD-426) Blades: Fix bug when the speed can only be changed when the video is playing. +LMS: The dialogs on the wiki "changes" page are now accessible to screen +readers. Now all wiki pages have been made accessible. (LMS-1337) + LMS: Change bulk email implementation to use less memory, and to better handle duplicate tasks in celery. @@ -70,8 +108,8 @@ client error are correctly passed through to the client. LMS: Improve performance of page load and thread list load for discussion tab -LMS: The wiki markup cheatsheet dialog is now accessible to people with -disabilites. (LMS-1303) +LMS: The wiki markup cheatsheet dialog is now accessible to screen readers. +(LMS-1303) Common: Add skip links for accessibility to CMS and LMS. (LMS-1311) diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index c4d1a9ddffb3..1a1f138cb588 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -1,3 +1,6 @@ +""" +Studio authorization functions primarily for course creators, instructors, and staff +""" #======================================================================================================================= # # This code is somewhat duplicative of access.py in the LMS. We will unify the code as a separate story @@ -11,7 +14,8 @@ from xmodule.modulestore import Location from xmodule.modulestore.locator import CourseLocator, Locator from xmodule.modulestore.django import loc_mapper -from xmodule.modulestore.exceptions import InvalidLocationError +from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError +import itertools # define a couple of simple roles, we just need ADMIN and EDITOR now for our purposes @@ -26,7 +30,11 @@ # of those two variables -def get_course_groupname_for_role(location, role): +def get_all_course_role_groupnames(location, role, use_filter=True): + ''' + Get all of the possible groupnames for this role location pair. If use_filter==True, + only return the ones defined in the groups collection. + ''' location = Locator.to_locator_or_location(location) # hack: check for existence of a group name in the legacy LMS format _ @@ -38,22 +46,46 @@ def get_course_groupname_for_role(location, role): except InvalidLocationError: # will occur on old locations where location is not of category course pass if isinstance(location, Location): + # least preferred role_course format groupnames.append('{0}_{1}'.format(role, location.course)) + try: + locator = loc_mapper().translate_location(location.course_id, location, False, False) + groupnames.append('{0}_{1}'.format(role, locator.course_id)) + except (InvalidLocationError, ItemNotFoundError): + pass elif isinstance(location, CourseLocator): old_location = loc_mapper().translate_locator_to_location(location, get_course=True) if old_location: + # the slashified version of the course_id (myu/mycourse/myrun) groupnames.append('{0}_{1}'.format(role, old_location.course_id)) - - for groupname in groupnames: - if Group.objects.filter(name=groupname).exists(): - return groupname - return groupnames[0] + # add the least desirable but sometimes occurring format. + groupnames.append('{0}_{1}'.format(role, old_location.course)) + # filter to the ones which exist + default = groupnames[0] + if use_filter: + groupnames = [group for group in groupnames if Group.objects.filter(name=group).exists()] + return groupnames, default -def get_users_in_course_group_by_role(location, role): - groupname = get_course_groupname_for_role(location, role) - (group, _created) = Group.objects.get_or_create(name=groupname) - return group.user_set.all() +def get_course_groupname_for_role(location, role): + ''' + Get the preferred used groupname for this role, location combo. + Preference order: + * role_course_id (e.g., staff_myu.mycourse.myrun) + * role_old_course_id (e.g., staff_myu/mycourse/myrun) + * role_old_course (e.g., staff_mycourse) + ''' + groupnames, default = get_all_course_role_groupnames(location, role) + return groupnames[0] if groupnames else default + + +def get_course_role_users(course_locator, role): + ''' + Get all of the users with the given role in the given course. + ''' + groupnames, _ = get_all_course_role_groupnames(course_locator, role) + groups = [Group.objects.get(name=groupname) for groupname in groupnames] + return list(itertools.chain.from_iterable(group.user_set.all() for group in groups)) def create_all_course_groups(creator, location): @@ -65,11 +97,11 @@ def create_all_course_groups(creator, location): def create_new_course_group(creator, location, role): - groupname = get_course_groupname_for_role(location, role) - (group, created) = Group.objects.get_or_create(name=groupname) - if created: - group.save() - + ''' + Create the new course group always using the preferred name even if another form already exists. + ''' + groupnames, __ = get_all_course_role_groupnames(location, role, use_filter=False) + group, __ = Group.objects.get_or_create(name=groupnames[0]) creator.groups.add(group) creator.save() @@ -82,15 +114,13 @@ def _delete_course_group(location): asserted permissions """ # remove all memberships - instructors = Group.objects.get(name=get_course_groupname_for_role(location, INSTRUCTOR_ROLE_NAME)) - for user in instructors.user_set.all(): - user.groups.remove(instructors) - user.save() - - staff = Group.objects.get(name=get_course_groupname_for_role(location, STAFF_ROLE_NAME)) - for user in staff.user_set.all(): - user.groups.remove(staff) - user.save() + for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]: + groupnames, _ = get_all_course_role_groupnames(location, role) + for groupname in groupnames: + group = Group.objects.get(name=groupname) + for user in group.user_set.all(): + user.groups.remove(group) + user.save() def _copy_course_group(source, dest): @@ -98,25 +128,25 @@ def _copy_course_group(source, dest): This is to be called only by either a command line code path or through an app which has already asserted permissions to do this action """ - instructors = Group.objects.get(name=get_course_groupname_for_role(source, INSTRUCTOR_ROLE_NAME)) - new_instructors_group = Group.objects.get(name=get_course_groupname_for_role(dest, INSTRUCTOR_ROLE_NAME)) - for user in instructors.user_set.all(): - user.groups.add(new_instructors_group) - user.save() - - staff = Group.objects.get(name=get_course_groupname_for_role(source, STAFF_ROLE_NAME)) - new_staff_group = Group.objects.get(name=get_course_groupname_for_role(dest, STAFF_ROLE_NAME)) - for user in staff.user_set.all(): - user.groups.add(new_staff_group) - user.save() + for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]: + groupnames, _ = get_all_course_role_groupnames(source, role) + for groupname in groupnames: + group = Group.objects.get(name=groupname) + new_group, _ = Group.objects.get_or_create(name=get_course_groupname_for_role(dest, INSTRUCTOR_ROLE_NAME)) + for user in group.user_set.all(): + user.groups.add(new_group) + user.save() def add_user_to_course_group(caller, user, location, role): + """ + If caller is authorized, add the given user to the given course's role + """ # only admins can add/remove other users if not is_user_in_course_group_role(caller, location, INSTRUCTOR_ROLE_NAME): raise PermissionDenied - group = Group.objects.get(name=get_course_groupname_for_role(location, role)) + group, _ = Group.objects.get_or_create(name=get_course_groupname_for_role(location, role)) return _add_user_to_group(user, group) @@ -132,9 +162,7 @@ def add_user_to_creator_group(caller, user): if not caller.is_active or not caller.is_authenticated or not caller.is_staff: raise PermissionDenied - (group, created) = Group.objects.get_or_create(name=COURSE_CREATOR_GROUP_NAME) - if created: - group.save() + (group, _) = Group.objects.get_or_create(name=COURSE_CREATOR_GROUP_NAME) return _add_user_to_group(user, group) @@ -152,6 +180,9 @@ def _add_user_to_group(user, group): def get_user_by_email(email): + """ + Get the user whose email is the arg. Return None if no such user exists. + """ user = None # try to look up user, return None if not found try: @@ -163,13 +194,21 @@ def get_user_by_email(email): def remove_user_from_course_group(caller, user, location, role): + """ + If caller is authorized, remove the given course x role authorization for user + """ # only admins can add/remove other users if not is_user_in_course_group_role(caller, location, INSTRUCTOR_ROLE_NAME): raise PermissionDenied # see if the user is actually in that role, if not then we don't have to do anything - if is_user_in_course_group_role(user, location, role): - _remove_user_from_group(user, get_course_groupname_for_role(location, role)) + groupnames, _ = get_all_course_role_groupnames(location, role) + for groupname in groupnames: + groups = user.groups.filter(name=groupname) + if groups: + # will only be one with that name + user.groups.remove(groups[0]) + user.save() def remove_user_from_creator_group(caller, user): @@ -195,11 +234,16 @@ def _remove_user_from_group(user, group_name): def is_user_in_course_group_role(user, location, role, check_staff=True): + """ + Check whether the given user has the given role in this course. If check_staff + then give permission if the user is staff without doing a course-role query. + """ if user.is_active and user.is_authenticated: # all "is_staff" flagged accounts belong to all groups if check_staff and user.is_staff: return True - return user.groups.filter(name=get_course_groupname_for_role(location, role)).exists() + groupnames, _ = get_all_course_role_groupnames(location, role) + return any(user.groups.filter(name=groupname).exists() for groupname in groupnames) return False diff --git a/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py b/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py index 5473438571e1..d3293c474e72 100644 --- a/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py +++ b/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py @@ -6,14 +6,6 @@ from terrain.steps import reload_the_page -def _is_expected_element_count(css, expected_number): - """ - Returns whether the number of elements found on the page by css locator - the same number that you expected. - """ - return len(world.css_find(css)) == expected_number - - @world.absorb def create_component_instance(step, category, component_type=None, is_advanced=False): """ @@ -47,8 +39,11 @@ def create_component_instance(step, category, component_type=None, is_advanced=F world.wait_for_invisible(component_button_css) click_component_from_menu(category, component_type, is_advanced) - world.wait_for(lambda _: _is_expected_element_count(module_css, - module_count_before + 1)) + expected_count = module_count_before + 1 + world.wait_for( + lambda _: len(world.css_find(module_css)) == expected_count, + timeout=20 + ) @world.absorb diff --git a/cms/djangoapps/contentstore/features/course-updates.feature b/cms/djangoapps/contentstore/features/course-updates.feature index 6f24fba68c6d..152da9c3499f 100644 --- a/cms/djangoapps/contentstore/features/course-updates.feature +++ b/cms/djangoapps/contentstore/features/course-updates.feature @@ -76,3 +76,17 @@ Feature: CMS.Course updates Then I see the handout "/c4x/MITx/999/asset/modified.jpg" And when I reload the page Then I see the handout "/c4x/MITx/999/asset/modified.jpg" + + Scenario: Users cannot save handouts with bad html until edit or update it properly + Given I have opened a new course in Studio + And I go to the course updates page + When I modify the handout to "

[LINK TEXT]

" + Then I see the handout error text + And I see handout save button disabled + When I edit the handout to "

home

" + Then I see handout save button re-enabled + When I save handout edit + # Can only do partial text matches because of the quotes with in quotes (and regexp step matching). + Then I see the handout "https://www.google.com.pk/" + And when I reload the page + Then I see the handout "https://www.google.com.pk/" diff --git a/cms/djangoapps/contentstore/features/course-updates.py b/cms/djangoapps/contentstore/features/course-updates.py index da74f5aa4bb3..b41578c907c8 100644 --- a/cms/djangoapps/contentstore/features/course-updates.py +++ b/cms/djangoapps/contentstore/features/course-updates.py @@ -90,6 +90,35 @@ def check_handout(_step, handout): assert handout in world.css_html(handout_css) +@step(u'I see the handout error text') +def check_handout_error(_step): + handout_error_css = 'div#handout_error' + assert world.css_has_class(handout_error_css, 'is-shown') + + +@step(u'I see handout save button disabled') +def check_handout_error(_step): + handout_save_button = 'form.edit-handouts-form a.save-button' + assert world.css_has_class(handout_save_button, 'is-disabled') + + +@step(u'I edit the handout to "([^"]*)"$') +def edit_handouts(_step, text): + type_in_codemirror(0, text) + + +@step(u'I see handout save button re-enabled') +def check_handout_error(_step): + handout_save_button = 'form.edit-handouts-form a.save-button' + assert not world.css_has_class(handout_save_button, 'is-disabled') + + +@step(u'I save handout edit') +def check_handout_error(_step): + save_css = 'a.save-button' + world.css_click(save_css) + + def change_text(text): type_in_codemirror(0, text) save_css = 'a.save-button' diff --git a/cms/djangoapps/contentstore/features/static-pages.feature b/cms/djangoapps/contentstore/features/static-pages.feature index 54d23d985de2..39399fb20725 100644 --- a/cms/djangoapps/contentstore/features/static-pages.feature +++ b/cms/djangoapps/contentstore/features/static-pages.feature @@ -9,10 +9,8 @@ Feature: CMS.Static Pages Then I should see a static page named "Empty" Scenario: Users can delete static pages - Given I have opened a new course in Studio - And I go to the static pages page - And I add a new page - And I "delete" the static page + Given I have created a static page + When I "delete" the static page Then I am shown a prompt When I confirm the prompt Then I should not see any static pages @@ -20,9 +18,16 @@ Feature: CMS.Static Pages # Safari won't update the name properly @skip_safari Scenario: Users can edit static pages - Given I have opened a new course in Studio - And I go to the static pages page - And I add a new page + Given I have created a static page When I "edit" the static page And I change the name to "New" Then I should see a static page named "New" + + # Safari won't update the name properly + @skip_safari + Scenario: Users can reorder static pages + Given I have created two different static pages + When I reorder the tabs + Then the tabs are in the reverse order + And I reload the page + Then the tabs are in the reverse order diff --git a/cms/djangoapps/contentstore/features/static-pages.py b/cms/djangoapps/contentstore/features/static-pages.py index 58932ad8e2bc..0adb4b1e54fa 100644 --- a/cms/djangoapps/contentstore/features/static-pages.py +++ b/cms/djangoapps/contentstore/features/static-pages.py @@ -48,3 +48,47 @@ def change_name(step, new_name): world.trigger_event(input_css) save_button = 'a.save-button' world.css_click(save_button) + + +@step(u'I reorder the tabs') +def reorder_tabs(_step): + # For some reason, the drag_and_drop method did not work in this case. + draggables = world.css_find('.drag-handle') + source = draggables.first + target = draggables.last + source.action_chains.click_and_hold(source._element).perform() + source.action_chains.move_to_element_with_offset(target._element, 0, 50).perform() + source.action_chains.release().perform() + + +@step(u'I have created a static page') +def create_static_page(step): + step.given('I have opened a new course in Studio') + step.given('I go to the static pages page') + step.given('I add a new page') + + +@step(u'I have created two different static pages') +def create_two_pages(step): + step.given('I have created a static page') + step.given('I "edit" the static page') + step.given('I change the name to "First"') + step.given('I add a new page') + # Verify order of tabs + _verify_tab_names('First', 'Empty') + + +@step(u'the tabs are in the reverse order') +def tabs_in_reverse_order(step): + _verify_tab_names('Empty', 'First') + + +def _verify_tab_names(first, second): + world.wait_for( + func=lambda _: len(world.css_find('.xmodule_StaticTabModule')) == 2, + timeout=200, + timeout_msg="Timed out waiting for two tabs to be present" + ) + tabs = world.css_find('.xmodule_StaticTabModule') + assert tabs[0].text == first + assert tabs[1].text == second diff --git a/cms/djangoapps/contentstore/features/transcripts.feature b/cms/djangoapps/contentstore/features/transcripts.feature index dad9cbb49e01..159c8a3c5abc 100644 --- a/cms/djangoapps/contentstore/features/transcripts.feature +++ b/cms/djangoapps/contentstore/features/transcripts.feature @@ -641,6 +641,7 @@ Feature: Video Component Editor And I save changes Then when I view the video it does show the captions + And I see "好 各位同学" text in the captions And I edit the component And I open tab "Advanced" diff --git a/cms/djangoapps/contentstore/features/transcripts.py b/cms/djangoapps/contentstore/features/transcripts.py index 5cbb65dc9d16..4fac5e5b9340 100644 --- a/cms/djangoapps/contentstore/features/transcripts.py +++ b/cms/djangoapps/contentstore/features/transcripts.py @@ -116,6 +116,7 @@ def i_see_status_message(_step, status): world.wait(DELAY) world.wait_for_ajax_complete() + assert not world.css_visible(SELECTORS['error_bar']) assert world.css_has_text(SELECTORS['status_bar'], STATUSES[status.strip()]) diff --git a/cms/djangoapps/contentstore/features/video.feature b/cms/djangoapps/contentstore/features/video.feature index 06bf36747e31..b126652ecc7c 100644 --- a/cms/djangoapps/contentstore/features/video.feature +++ b/cms/djangoapps/contentstore/features/video.feature @@ -53,6 +53,8 @@ Feature: CMS.Video Component Then Captions become "invisible" # 8 + # Disabled 11/26 due to flakiness in master. + # Enabled back on 11/29. Scenario: Open captions never become invisible Given I have created a Video component with subtitles And Make sure captions are open @@ -63,6 +65,8 @@ Feature: CMS.Video Component Then Captions are "visible" # 9 + # Disabled 11/26 due to flakiness in master. + # Enabled back on 11/29. Scenario: Closed captions are invisible when mouse doesn't hover on CC button Given I have created a Video component with subtitles And Make sure captions are closed @@ -71,6 +75,8 @@ Feature: CMS.Video Component Then Captions are "invisible" # 10 + # Disabled 11/26 due to flakiness in master. + # Enabled back on 11/29. Scenario: When enter key is pressed on a caption shows an outline around it Given I have created a Video component with subtitles And Make sure captions are opened diff --git a/cms/djangoapps/contentstore/features/video.py b/cms/djangoapps/contentstore/features/video.py index 5408c482908c..c97dba10b929 100644 --- a/cms/djangoapps/contentstore/features/video.py +++ b/cms/djangoapps/contentstore/features/video.py @@ -181,7 +181,7 @@ def click_on_the_caption(_step, index): @step('I see caption line with data-index "([^"]*)" has class "([^"]*)"$') def caption_line_has_class(_step, index, className): SELECTOR = ".subtitles > li[data-index='{index}']".format(index=int(index.strip())) - world.css_has_class(SELECTOR, className.strip()) + assert world.css_has_class(SELECTOR, className.strip()) @step('I see a range on slider$') diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 58cf3be70b0f..91dce0f6ee7f 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1,6 +1,5 @@ #pylint: disable=E1101 -import json import shutil import mock @@ -15,6 +14,7 @@ import copy from json import loads from datetime import timedelta +from django.test import TestCase from django.contrib.auth.models import User from django.dispatch import Signal @@ -42,6 +42,7 @@ from xmodule.course_module import CourseDescriptor from xmodule.seq_module import SequenceDescriptor from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.modulestore.locator import BlockUsageLocator from contentstore.views.component import ADVANCED_COMPONENT_TYPES from xmodule.exceptions import NotFoundError @@ -53,6 +54,7 @@ from uuid import uuid4 from pymongo import MongoClient from student.models import CourseEnrollment +import re from contentstore.utils import delete_course_and_groups from xmodule.modulestore.django import loc_mapper @@ -132,9 +134,10 @@ def check_components_on_page(self, component_types, expected_types): # just pick one vertical descriptor = store.get_items(Location('i4x', 'edX', 'simple', 'vertical', None, None))[0] - - resp = self.client.get_html(reverse('edit_unit', kwargs={'location': descriptor.location.url()})) + locator = loc_mapper().translate_location(course.location.course_id, descriptor.location, False, True) + resp = self.client.get_html(locator.url_reverse('unit')) self.assertEqual(resp.status_code, 200) + _test_no_locations(self, resp) for expected in expected_types: self.assertIn(expected, resp.content) @@ -152,25 +155,24 @@ def test_advanced_components_require_two_clicks(self): def test_malformed_edit_unit_request(self): store = modulestore('direct') - import_from_xml(store, 'common/test/data/', ['simple']) + _, course_items = import_from_xml(store, 'common/test/data/', ['simple']) # just pick one vertical descriptor = store.get_items(Location('i4x', 'edX', 'simple', 'vertical', None, None))[0] location = descriptor.location.replace(name='.' + descriptor.location.name) + locator = loc_mapper().translate_location(course_items[0].location.course_id, location, False, True) - resp = self.client.get_html(reverse('edit_unit', kwargs={'location': location.url()})) + resp = self.client.get_html(locator.url_reverse('unit')) self.assertEqual(resp.status_code, 400) + _test_no_locations(self, resp, status_code=400) def check_edit_unit(self, test_course_name): - import_from_xml(modulestore('direct'), 'common/test/data/', [test_course_name]) + _, course_items = import_from_xml(modulestore('direct'), 'common/test/data/', [test_course_name]) - for descriptor in modulestore().get_items(Location(None, None, 'vertical', None, None)): - print "Checking ", descriptor.location.url() - print descriptor.__class__, descriptor.location - resp = self.client.get_html(reverse('edit_unit', kwargs={'location': descriptor.location.url()})) - self.assertEqual(resp.status_code, 200) + items = modulestore().get_items(Location('i4x', 'edX', test_course_name, 'vertical', None, None)) + self._check_verticals(items, course_items[0].location.course_id) - def lockAnAsset(self, content_store, course_location): + def _lock_an_asset(self, content_store, course_location): """ Lock an arbitrary asset in the course :param course_location: @@ -398,24 +400,7 @@ def test_create_static_tab_and_rename(self): self.assertEqual(course.tabs, expected_tabs) def test_static_tab_reordering(self): - def get_tab_locator(tab): - tab_location = 'i4x://MITx/999/static_tab/{0}'.format(tab['url_slug']) - return unicode(loc_mapper().translate_location( - course.location.course_id, Location(tab_location), False, True - )) - - module_store = modulestore('direct') - locator = _course_factory_create_course() - course_location = loc_mapper().translate_locator_to_location(locator) - - ItemFactory.create( - parent_location=course_location, - category="static_tab", - display_name="Static_1") - ItemFactory.create( - parent_location=course_location, - category="static_tab", - display_name="Static_2") + module_store, course_location, new_location = self._create_static_tabs() course = module_store.get_item(course_location) @@ -423,9 +408,9 @@ def get_tab_locator(tab): reverse_tabs = [] for tab in course.tabs: if tab['type'] == 'static_tab': - reverse_tabs.insert(0, get_tab_locator(tab)) + reverse_tabs.insert(0, unicode(self._get_tab_locator(course, tab))) - self.client.ajax_post(reverse('reorder_static_tabs'), {'tabs': reverse_tabs}) + self.client.ajax_post(new_location.url_reverse('tabs'), {'tabs': reverse_tabs}) course = module_store.get_item(course_location) @@ -433,10 +418,57 @@ def get_tab_locator(tab): course_tabs = [] for tab in course.tabs: if tab['type'] == 'static_tab': - course_tabs.append(get_tab_locator(tab)) + course_tabs.append(unicode(self._get_tab_locator(course, tab))) self.assertEqual(reverse_tabs, course_tabs) + def test_static_tab_deletion(self): + module_store, course_location, _ = self._create_static_tabs() + + course = module_store.get_item(course_location) + num_tabs = len(course.tabs) + last_tab = course.tabs[num_tabs - 1] + url_slug = last_tab['url_slug'] + delete_url = self._get_tab_locator(course, last_tab).url_reverse('xblock') + + self.client.delete(delete_url) + + course = module_store.get_item(course_location) + self.assertEqual(num_tabs - 1, len(course.tabs)) + + def tab_matches(tab): + """ Checks if the tab matches the one we deleted """ + return tab['type'] == 'static_tab' and tab['url_slug'] == url_slug + + tab_found = any(tab_matches(tab) for tab in course.tabs) + + self.assertFalse(tab_found, "tab should have been deleted") + + def _get_tab_locator(self, course, tab): + """ Returns the locator for a given tab. """ + tab_location = 'i4x://MITx/999/static_tab/{0}'.format(tab['url_slug']) + return loc_mapper().translate_location( + course.location.course_id, Location(tab_location), False, True + ) + + def _create_static_tabs(self): + """ Creates two static tabs in a dummy course. """ + module_store = modulestore('direct') + CourseFactory.create(org='edX', course='999', display_name='Robot Super Course') + course_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None]) + new_location = loc_mapper().translate_location(course_location.course_id, course_location, False, True) + + ItemFactory.create( + parent_location=course_location, + category="static_tab", + display_name="Static_1") + ItemFactory.create( + parent_location=course_location, + category="static_tab", + display_name="Static_2") + + return module_store, course_location, new_location + def test_import_polls(self): module_store = modulestore('direct') import_from_xml(module_store, 'common/test/data/', ['toy']) @@ -454,31 +486,38 @@ def test_xlint_fails(self): @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/toy/.*']) def test_module_preview_in_whitelist(self): - ''' + """ Tests the ajax callback to render an XModule - ''' - direct_store = modulestore('direct') - import_from_xml(direct_store, 'common/test/data/', ['toy']) - - # also try a custom response which will trigger the 'is this course in whitelist' logic - problem_module_location = Location(['i4x', 'edX', 'toy', 'vertical', 'vertical_test', None]) - url = reverse('preview_component', kwargs={'location': problem_module_location.url()}) - resp = self.client.get_html(url) - self.assertEqual(resp.status_code, 200) + """ + resp = self._test_preview(Location(['i4x', 'edX', 'toy', 'vertical', 'vertical_test', None])) + # These are the data-ids of the xblocks contained in the vertical. + # Ultimately, these must be converted to new locators. + self.assertContains(resp, 'i4x://edX/toy/video/sample_video') + self.assertContains(resp, 'i4x://edX/toy/video/separate_file_video') + self.assertContains(resp, 'i4x://edX/toy/video/video_with_end_time') + self.assertContains(resp, 'i4x://edX/toy/poll_question/T1_changemind_poll_foo_2') def test_video_module_caption_asset_path(self): - ''' + """ This verifies that a video caption url is as we expect it to be - ''' + """ + resp = self._test_preview(Location(['i4x', 'edX', 'toy', 'video', 'sample_video', None])) + self.assertContains(resp, 'data-caption-asset-path="/c4x/edX/toy/asset/subs_"') + + def _test_preview(self, location): + """ Preview test case. """ direct_store = modulestore('direct') - import_from_xml(direct_store, 'common/test/data/', ['toy']) + _, course_items = import_from_xml(direct_store, 'common/test/data/', ['toy']) # also try a custom response which will trigger the 'is this course in whitelist' logic - video_module_location = Location(['i4x', 'edX', 'toy', 'video', 'sample_video', None]) - url = reverse('preview_component', kwargs={'location': video_module_location.url()}) - resp = self.client.get_html(url) + locator = loc_mapper().translate_location( + course_items[0].location.course_id, location, False, True + ) + resp = self.client.get_html(locator.url_reverse('xblock')) self.assertEqual(resp.status_code, 200) - self.assertContains(resp, 'data-caption-asset-path="/c4x/edX/toy/asset/subs_"') + # TODO: uncomment when preview no longer has locations being returned. + # _test_no_locations(self, resp) + return resp def test_delete(self): direct_store = modulestore('direct') @@ -617,7 +656,7 @@ def test_asset_delete_and_restore(self): thumbnail = content_store.find(thumbnail_location, throw_on_not_found=False) self.assertIsNotNone(thumbnail) - def _delete_asset_in_course (self): + def _delete_asset_in_course(self): """ Helper method for: 1) importing course from xml @@ -836,6 +875,7 @@ def test_illegal_draft_crud_ops(self): def test_bad_contentstore_request(self): resp = self.client.get_html('http://localhost:8001/c4x/CDX/123123/asset/&images_circuits_Lab7Solution2.png') self.assertEqual(resp.status_code, 400) + _test_no_locations(self, resp, 400) def test_rewrite_nonportable_links_on_import(self): module_store = modulestore('direct') @@ -955,7 +995,7 @@ def test_export_course(self, mock_get): self.assertIn(private_location_no_draft.url(), sequential.children) - locked_asset = self.lockAnAsset(content_store, location) + locked_asset = self._lock_an_asset(content_store, location) locked_asset_attrs = content_store.get_attrs(locked_asset) # the later import will reupload del locked_asset_attrs['uploadDate'] @@ -1010,7 +1050,7 @@ def test_export_course(self, mock_get): shutil.rmtree(root_dir) def check_import(self, module_store, root_dir, draft_store, content_store, stub_location, course_location, - locked_asset, locked_asset_attrs): + locked_asset, locked_asset_attrs): # reimport import_from_xml( module_store, root_dir, ['test_export'], draft_store=draft_store, @@ -1018,15 +1058,11 @@ def check_import(self, module_store, root_dir, draft_store, content_store, stub_ target_location_namespace=course_location ) + # Unit test fails in Jenkins without this. + loc_mapper().translate_location(course_location.course_id, course_location, False, True) + items = module_store.get_items(stub_location.replace(category='vertical', name=None)) - self.assertGreater(len(items), 0) - for descriptor in items: - # don't try to look at private verticals. Right now we're running - # the service in non-draft aware - if getattr(descriptor, 'is_draft', False): - print "Checking {0}....".format(descriptor.location.url()) - resp = self.client.get_html(reverse('edit_unit', kwargs={'location': descriptor.location.url()})) - self.assertEqual(resp.status_code, 200) + self._check_verticals(items, course_location.course_id) # verify that we have the content in the draft store as well vertical = draft_store.get_item( @@ -1210,7 +1246,7 @@ def test_course_handouts_rewrites(self): handouts_locator = loc_mapper().translate_location('edX/toy/2012_Fall', handout_location) # get module info (json) - resp = self.client.get(handouts_locator.url_reverse('/xblock', '')) + resp = self.client.get(handouts_locator.url_reverse('/xblock')) # make sure we got a successful response self.assertEqual(resp.status_code, 200) @@ -1309,6 +1345,16 @@ def test_export_course_without_content_store(self): items = module_store.get_items(stub_location) self.assertEqual(len(items), 1) + def _check_verticals(self, items, course_id): + """ Test getting the editing HTML for each vertical. """ + # Assert is here to make sure that the course being tested actually has verticals (units) to check. + self.assertGreater(len(items), 0) + for descriptor in items: + unit_locator = loc_mapper().translate_location(course_id, descriptor.location, False, True) + resp = self.client.get_html(unit_locator.url_reverse('unit')) + self.assertEqual(resp.status_code, 200) + _test_no_locations(self, resp) + @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, MODULESTORE=TEST_MODULESTORE) class ContentStoreTest(ModuleStoreTestCase): @@ -1387,7 +1433,7 @@ def test_forum_unseeding_with_multiple_courses(self): second_course_data = self.assert_created_course(number_suffix=uuid4().hex) # unseed the forums for the first course - course_id =_get_course_id(test_course_data) + course_id = _get_course_id(test_course_data) delete_course_and_groups(course_id, commit=True) self.assertFalse(are_permissions_roles_seeded(course_id)) @@ -1503,6 +1549,7 @@ def test_course_index_view_with_no_courses(self): status_code=200, html=True ) + _test_no_locations(self, resp) def test_course_factory(self): """Test that the course factory works correctly.""" @@ -1525,6 +1572,7 @@ def test_course_index_view_with_course(self): status_code=200, html=True ) + _test_no_locations(self, resp) def test_course_overview_view_with_course(self): """Test viewing the course overview page with an existing course""" @@ -1550,12 +1598,13 @@ def test_create_item(self): } resp = self.client.ajax_post('/xblock', section_data) + _test_no_locations(self, resp, html=False) self.assertEqual(resp.status_code, 200) data = parse_json(resp) self.assertRegexpMatches( - data['id'], - r"^i4x://MITx/999/chapter/([0-9]|[a-f]){32}$" + data['locator'], + r"^MITx.999.Robot_Super_Course/branch/draft/block/chapter([0-9]|[a-f]){3}$" ) def test_capa_module(self): @@ -1571,7 +1620,7 @@ def test_capa_module(self): self.assertEqual(resp.status_code, 200) payload = parse_json(resp) - problem_loc = Location(payload['id']) + problem_loc = loc_mapper().translate_locator_to_location(BlockUsageLocator(payload['locator'])) problem = get_modulestore(problem_loc).get_item(problem_loc) # should be a CapaDescriptor self.assertIsInstance(problem, CapaDescriptor, "New problem is not a CapaDescriptor") @@ -1584,6 +1633,13 @@ def test_cms_imported_course_walkthrough(self): Import and walk through some common URL endpoints. This just verifies non-500 and no other correct behavior, so it is not a deep test """ + def test_get_html(page): + # Helper function for getting HTML for a page in Studio and + # checking that it does not error. + resp = self.client.get_html(new_location.url_reverse(page)) + self.assertEqual(resp.status_code, 200) + _test_no_locations(self, resp) + import_from_xml(modulestore('direct'), 'common/test/data/', ['simple']) loc = Location(['i4x', 'edX', 'simple', 'course', '2012_Fall', None]) new_location = loc_mapper().translate_location(loc.course_id, loc, False, True) @@ -1593,55 +1649,38 @@ def test_cms_imported_course_walkthrough(self): self.assertContains(resp, 'Chapter 2') # go to various pages - - # import page - resp = self.client.get_html(new_location.url_reverse('import/', '')) - self.assertEqual(resp.status_code, 200) - - # export page - resp = self.client.get_html(new_location.url_reverse('export/', '')) - self.assertEqual(resp.status_code, 200) - - # course team - url = new_location.url_reverse('course_team/', '') - resp = self.client.get_html(url) - self.assertEqual(resp.status_code, 200) - - # course info - resp = self.client.get(new_location.url_reverse('course_info')) - self.assertEqual(resp.status_code, 200) - - # settings_details - resp = self.client.get(reverse('settings_details', - kwargs={'org': loc.org, - 'course': loc.course, - 'name': loc.name})) - self.assertEqual(resp.status_code, 200) - - # settings_details - resp = self.client.get(reverse('settings_grading', - kwargs={'org': loc.org, - 'course': loc.course, - 'name': loc.name})) - self.assertEqual(resp.status_code, 200) - - # assets_handler (HTML for full page content) - url = new_location.url_reverse('assets/', '') - resp = self.client.get_html(url) + test_get_html('import') + test_get_html('export') + test_get_html('course_team') + test_get_html('course_info') + test_get_html('checklists') + test_get_html('assets') + test_get_html('tabs') + test_get_html('settings/details') + test_get_html('settings/grading') + test_get_html('settings/advanced') + + # textbook index + resp = self.client.get_html(reverse('textbook_index', + kwargs={'org': loc.org, + 'course': loc.course, + 'name': loc.name})) self.assertEqual(resp.status_code, 200) + _test_no_locations(self, resp) # go look at a subsection page subsection_location = loc.replace(category='sequential', name='test_sequence') - resp = self.client.get_html( - reverse('edit_subsection', kwargs={'location': subsection_location.url()}) - ) + subsection_locator = loc_mapper().translate_location(loc.course_id, subsection_location, False, True) + resp = self.client.get_html(subsection_locator.url_reverse('subsection')) self.assertEqual(resp.status_code, 200) + _test_no_locations(self, resp) # go look at the Edit page unit_location = loc.replace(category='vertical', name='test_vertical') - resp = self.client.get_html( - reverse('edit_unit', kwargs={'location': unit_location.url()})) + unit_locator = loc_mapper().translate_location(loc.course_id, unit_location, False, True) + resp = self.client.get_html(unit_locator.url_reverse('unit')) self.assertEqual(resp.status_code, 200) + _test_no_locations(self, resp) def delete_item(category, name): """ Helper method for testing the deletion of an xblock item. """ @@ -1649,6 +1688,7 @@ def delete_item(category, name): del_location = loc_mapper().translate_location(loc.course_id, del_loc, False, True) resp = self.client.delete(del_location.url_reverse('xblock')) self.assertEqual(resp.status_code, 204) + _test_no_locations(self, resp, status_code=204, html=False) # delete a component delete_item(category='html', name='test_html') @@ -1848,7 +1888,9 @@ def _show_course_overview(self, location): Show the course overview page. """ new_location = loc_mapper().translate_location(location.course_id, location, False, True) - return self.client.get_html(new_location.url_reverse('course/', '')) + resp = self.client.get_html(new_location.url_reverse('course/', '')) + _test_no_locations(self, resp) + return resp @override_settings(MODULESTORE=TEST_MODULESTORE) @@ -1915,6 +1957,32 @@ def test_metadata_persistence(self): pass +class EntryPageTestCase(TestCase): + """ + Tests entry pages that aren't specific to a course. + """ + def setUp(self): + self.client = AjaxEnabledTestClient() + + def _test_page(self, page, status_code=200): + resp = self.client.get_html(page) + self.assertEqual(resp.status_code, status_code) + _test_no_locations(self, resp, status_code) + + def test_how_it_works(self): + self._test_page("/howitworks") + + def test_signup(self): + self._test_page("/signup") + + def test_login(self): + self._test_page("/signin") + + def test_logout(self): + # Logout redirects. + self._test_page("/logout", 302) + + def _create_course(test, course_data): """ Creates a course via an AJAX request and verifies the URL returned in the response. @@ -1926,7 +1994,7 @@ def _create_course(test, course_data): test.assertEqual(response.status_code, 200) data = parse_json(response) test.assertNotIn('ErrMsg', data) - test.assertEqual(data['url'], new_location.url_reverse("course/", "")) + test.assertEqual(data['url'], new_location.url_reverse("course")) def _course_factory_create_course(): @@ -1940,3 +2008,19 @@ def _course_factory_create_course(): def _get_course_id(test_course_data): """Returns the course ID (org/number/run).""" return "{org}/{number}/{run}".format(**test_course_data) + + +def _test_no_locations(test, resp, status_code=200, html=True): + """ + Verifies that "i4x", which appears in old locations, but not + new locators, does not appear in the HTML response output. + Used to verify that database refactoring is complete. + """ + test.assertNotContains(resp, 'i4x', status_code=status_code, html=html) + if html: + # For HTML pages, it is nice to call the method with html=True because + # it checks that the HTML properly parses. However, it won't find i4x usages + # in JavaScript blocks. + content = resp.content + hits = len(re.findall(r"(?bar" # encode - decode to convert date fields and other data which changes form self.assertEqual( - CourseDetails.update_from_json(jsondetails.__dict__).syllabus, + CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).syllabus, jsondetails.syllabus, "After set syllabus" ) jsondetails.overview = "Overview" self.assertEqual( - CourseDetails.update_from_json(jsondetails.__dict__).overview, + CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).overview, jsondetails.overview, "After set overview" ) jsondetails.intro_video = "intro_video" self.assertEqual( - CourseDetails.update_from_json(jsondetails.__dict__).intro_video, + CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).intro_video, jsondetails.intro_video, "After set intro_video" ) jsondetails.effort = "effort" self.assertEqual( - CourseDetails.update_from_json(jsondetails.__dict__).effort, + CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).effort, jsondetails.effort, "After set effort" ) jsondetails.start_date = datetime.datetime(2010, 10, 1, 0, tzinfo=UTC()) self.assertEqual( - CourseDetails.update_from_json(jsondetails.__dict__).start_date, + CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).start_date, jsondetails.start_date ) jsondetails.course_image_name = "an_image.jpg" self.assertEqual( - CourseDetails.update_from_json(jsondetails.__dict__).course_image_name, + CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).course_image_name, jsondetails.course_image_name ) @override_settings(MKTG_URLS={'ROOT': 'dummy-root'}) def test_marketing_site_fetch(self): - settings_details_url = reverse( - 'settings_details', - kwargs={ - 'org': self.course.location.org, - 'name': self.course.location.name, - 'course': self.course.location.course - } - ) + settings_details_url = self.course_locator.url_reverse('settings/details/') with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'ENABLE_MKTG_SITE': True}): - response = self.client.get(settings_details_url) + response = self.client.get_html(settings_details_url) self.assertNotContains(response, "Course Summary Page") self.assertNotContains(response, "Send a note to students via email") self.assertContains(response, "course summary page will not be viewable") @@ -135,17 +125,10 @@ def test_marketing_site_fetch(self): self.assertNotContains(response, "Requirements") def test_regular_site_fetch(self): - settings_details_url = reverse( - 'settings_details', - kwargs={ - 'org': self.course.location.org, - 'name': self.course.location.name, - 'course': self.course.location.course - } - ) + settings_details_url = self.course_locator.url_reverse('settings/details/') with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'ENABLE_MKTG_SITE': False}): - response = self.client.get(settings_details_url) + response = self.client.get_html(settings_details_url) self.assertContains(response, "Course Summary Page") self.assertContains(response, "Send a note to students via email") self.assertNotContains(response, "course summary page will not be viewable") @@ -168,10 +151,12 @@ class CourseDetailsViewTest(CourseTestCase): Tests for modifying content on the first course settings page (course dates, overview, etc.). """ def alter_field(self, url, details, field, val): + """ + Change the one field to the given value and then invoke the update post to see if it worked. + """ setattr(details, field, val) # Need to partially serialize payload b/c the mock doesn't handle it correctly payload = copy.copy(details.__dict__) - payload['course_location'] = details.course_location.url() payload['start_date'] = CourseDetailsViewTest.convert_datetime_to_iso(details.start_date) payload['end_date'] = CourseDetailsViewTest.convert_datetime_to_iso(details.end_date) payload['enrollment_start'] = CourseDetailsViewTest.convert_datetime_to_iso(details.enrollment_start) @@ -181,16 +166,17 @@ def alter_field(self, url, details, field, val): @staticmethod def convert_datetime_to_iso(datetime_obj): + """ + Use the xblock serializer to convert the datetime + """ return Date().to_json(datetime_obj) def test_update_and_fetch(self): - loc = self.course.location - details = CourseDetails.fetch(loc) + details = CourseDetails.fetch(self.course_locator) # resp s/b json from here on - url = reverse('course_settings', kwargs={'org': loc.org, 'course': loc.course, - 'name': loc.name, 'section': 'details'}) - resp = self.client.get(url) + url = self.course_locator.url_reverse('settings/details/') + resp = self.client.get_json(url) self.compare_details_with_encoding(json.loads(resp.content), details.__dict__, "virgin get") utc = UTC() @@ -206,6 +192,9 @@ def test_update_and_fetch(self): self.alter_field(url, details, 'course_image_name', "course_image_name") def compare_details_with_encoding(self, encoded, details, context): + """ + compare all of the fields of the before and after dicts + """ self.compare_date_fields(details, encoded, context, 'start_date') self.compare_date_fields(details, encoded, context, 'end_date') self.compare_date_fields(details, encoded, context, 'enrollment_start') @@ -216,6 +205,9 @@ def compare_details_with_encoding(self, encoded, details, context): self.assertEqual(details['course_image_name'], encoded['course_image_name'], context + " images not ==") def compare_date_fields(self, details, encoded, context, field): + """ + Compare the given date fields between the before and after doing json deserialization + """ if details[field] is not None: date = Date() if field in encoded and encoded[field] is not None: @@ -234,142 +226,191 @@ class CourseGradingTest(CourseTestCase): Tests for the course settings grading page. """ def test_initial_grader(self): - descriptor = get_modulestore(self.course.location).get_item(self.course.location) - test_grader = CourseGradingModel(descriptor) - # ??? How much should this test bake in expectations about defaults and thus fail if defaults change? - self.assertEqual(self.course.location, test_grader.course_location, "Course locations") - self.assertIsNotNone(test_grader.graders, "No graders") - self.assertIsNotNone(test_grader.grade_cutoffs, "No cutoffs") + test_grader = CourseGradingModel(self.course) + self.assertIsNotNone(test_grader.graders) + self.assertIsNotNone(test_grader.grade_cutoffs) def test_fetch_grader(self): - test_grader = CourseGradingModel.fetch(self.course.location.url()) - self.assertEqual(self.course.location, test_grader.course_location, "Course locations") - self.assertIsNotNone(test_grader.graders, "No graders") - self.assertIsNotNone(test_grader.grade_cutoffs, "No cutoffs") - - test_grader = CourseGradingModel.fetch(self.course.location) - self.assertEqual(self.course.location, test_grader.course_location, "Course locations") + test_grader = CourseGradingModel.fetch(self.course_locator) self.assertIsNotNone(test_grader.graders, "No graders") self.assertIsNotNone(test_grader.grade_cutoffs, "No cutoffs") for i, grader in enumerate(test_grader.graders): - subgrader = CourseGradingModel.fetch_grader(self.course.location, i) + subgrader = CourseGradingModel.fetch_grader(self.course_locator, i) self.assertDictEqual(grader, subgrader, str(i) + "th graders not equal") - subgrader = CourseGradingModel.fetch_grader(self.course.location.list(), 0) - self.assertDictEqual(test_grader.graders[0], subgrader, "failed with location as list") - - def test_fetch_cutoffs(self): - test_grader = CourseGradingModel.fetch_cutoffs(self.course.location) - # ??? should this check that it's at least a dict? (expected is { "pass" : 0.5 } I think) - self.assertIsNotNone(test_grader, "No cutoffs via fetch") - - test_grader = CourseGradingModel.fetch_cutoffs(self.course.location.url()) - self.assertIsNotNone(test_grader, "No cutoffs via fetch with url") - - def test_fetch_grace(self): - test_grader = CourseGradingModel.fetch_grace_period(self.course.location) - # almost a worthless test - self.assertIn('grace_period', test_grader, "No grace via fetch") - - test_grader = CourseGradingModel.fetch_grace_period(self.course.location.url()) - self.assertIn('grace_period', test_grader, "No cutoffs via fetch with url") - def test_update_from_json(self): - test_grader = CourseGradingModel.fetch(self.course.location) - altered_grader = CourseGradingModel.update_from_json(test_grader.__dict__) + test_grader = CourseGradingModel.fetch(self.course_locator) + altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__) self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "Noop update") test_grader.graders[0]['weight'] = test_grader.graders[0].get('weight') * 2 - altered_grader = CourseGradingModel.update_from_json(test_grader.__dict__) + altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__) self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "Weight[0] * 2") test_grader.grade_cutoffs['D'] = 0.3 - altered_grader = CourseGradingModel.update_from_json(test_grader.__dict__) + altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__) self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "cutoff add D") test_grader.grace_period = {'hours': 4, 'minutes': 5, 'seconds': 0} - altered_grader = CourseGradingModel.update_from_json(test_grader.__dict__) + altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__) self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "4 hour grace period") def test_update_grader_from_json(self): - test_grader = CourseGradingModel.fetch(self.course.location) - altered_grader = CourseGradingModel.update_grader_from_json(test_grader.course_location, test_grader.graders[1]) + test_grader = CourseGradingModel.fetch(self.course_locator) + altered_grader = CourseGradingModel.update_grader_from_json(self.course_locator, test_grader.graders[1]) self.assertDictEqual(test_grader.graders[1], altered_grader, "Noop update") test_grader.graders[1]['min_count'] = test_grader.graders[1].get('min_count') + 2 - altered_grader = CourseGradingModel.update_grader_from_json(test_grader.course_location, test_grader.graders[1]) + altered_grader = CourseGradingModel.update_grader_from_json(self.course_locator, test_grader.graders[1]) self.assertDictEqual(test_grader.graders[1], altered_grader, "min_count[1] + 2") test_grader.graders[1]['drop_count'] = test_grader.graders[1].get('drop_count') + 1 - altered_grader = CourseGradingModel.update_grader_from_json(test_grader.course_location, test_grader.graders[1]) + altered_grader = CourseGradingModel.update_grader_from_json(self.course_locator, test_grader.graders[1]) self.assertDictEqual(test_grader.graders[1], altered_grader, "drop_count[1] + 2") def test_update_cutoffs_from_json(self): - test_grader = CourseGradingModel.fetch(self.course.location) - CourseGradingModel.update_cutoffs_from_json(test_grader.course_location, test_grader.grade_cutoffs) + test_grader = CourseGradingModel.fetch(self.course_locator) + CourseGradingModel.update_cutoffs_from_json(self.course_locator, test_grader.grade_cutoffs) # Unlike other tests, need to actually perform a db fetch for this test since update_cutoffs_from_json # simply returns the cutoffs you send into it, rather than returning the db contents. - altered_grader = CourseGradingModel.fetch(self.course.location) + altered_grader = CourseGradingModel.fetch(self.course_locator) self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "Noop update") test_grader.grade_cutoffs['D'] = 0.3 - CourseGradingModel.update_cutoffs_from_json(test_grader.course_location, test_grader.grade_cutoffs) - altered_grader = CourseGradingModel.fetch(self.course.location) + CourseGradingModel.update_cutoffs_from_json(self.course_locator, test_grader.grade_cutoffs) + altered_grader = CourseGradingModel.fetch(self.course_locator) self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "cutoff add D") test_grader.grade_cutoffs['Pass'] = 0.75 - CourseGradingModel.update_cutoffs_from_json(test_grader.course_location, test_grader.grade_cutoffs) - altered_grader = CourseGradingModel.fetch(self.course.location) + CourseGradingModel.update_cutoffs_from_json(self.course_locator, test_grader.grade_cutoffs) + altered_grader = CourseGradingModel.fetch(self.course_locator) self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "cutoff change 'Pass'") def test_delete_grace_period(self): - test_grader = CourseGradingModel.fetch(self.course.location) - CourseGradingModel.update_grace_period_from_json(test_grader.course_location, test_grader.grace_period) + test_grader = CourseGradingModel.fetch(self.course_locator) + CourseGradingModel.update_grace_period_from_json(self.course_locator, test_grader.grace_period) # update_grace_period_from_json doesn't return anything, so query the db for its contents. - altered_grader = CourseGradingModel.fetch(self.course.location) + altered_grader = CourseGradingModel.fetch(self.course_locator) self.assertEqual(test_grader.grace_period, altered_grader.grace_period, "Noop update") test_grader.grace_period = {'hours': 15, 'minutes': 5, 'seconds': 30} - CourseGradingModel.update_grace_period_from_json(test_grader.course_location, test_grader.grace_period) - altered_grader = CourseGradingModel.fetch(self.course.location) + CourseGradingModel.update_grace_period_from_json(self.course_locator, test_grader.grace_period) + altered_grader = CourseGradingModel.fetch(self.course_locator) self.assertDictEqual(test_grader.grace_period, altered_grader.grace_period, "Adding in a grace period") test_grader.grace_period = {'hours': 1, 'minutes': 10, 'seconds': 0} # Now delete the grace period - CourseGradingModel.delete_grace_period(test_grader.course_location) + CourseGradingModel.delete_grace_period(self.course_locator) # update_grace_period_from_json doesn't return anything, so query the db for its contents. - altered_grader = CourseGradingModel.fetch(self.course.location) + altered_grader = CourseGradingModel.fetch(self.course_locator) # Once deleted, the grace period should simply be None self.assertEqual(None, altered_grader.grace_period, "Delete grace period") def test_update_section_grader_type(self): # Get the descriptor and the section_grader_type and assert they are the default values descriptor = get_modulestore(self.course.location).get_item(self.course.location) - section_grader_type = CourseGradingModel.get_section_grader_type(self.course.location) + section_grader_type = CourseGradingModel.get_section_grader_type(self.course_locator) self.assertEqual('Not Graded', section_grader_type['graderType']) self.assertEqual(None, descriptor.format) self.assertEqual(False, descriptor.graded) # Change the default grader type to Homework, which should also mark the section as graded - CourseGradingModel.update_section_grader_type(self.course.location, {'graderType': 'Homework'}) + CourseGradingModel.update_section_grader_type(self.course, 'Homework') descriptor = get_modulestore(self.course.location).get_item(self.course.location) - section_grader_type = CourseGradingModel.get_section_grader_type(self.course.location) + section_grader_type = CourseGradingModel.get_section_grader_type(self.course_locator) self.assertEqual('Homework', section_grader_type['graderType']) self.assertEqual('Homework', descriptor.format) self.assertEqual(True, descriptor.graded) # Change the grader type back to Not Graded, which should also unmark the section as graded - CourseGradingModel.update_section_grader_type(self.course.location, {'graderType': 'Not Graded'}) + CourseGradingModel.update_section_grader_type(self.course, 'Not Graded') descriptor = get_modulestore(self.course.location).get_item(self.course.location) - section_grader_type = CourseGradingModel.get_section_grader_type(self.course.location) + section_grader_type = CourseGradingModel.get_section_grader_type(self.course_locator) self.assertEqual('Not Graded', section_grader_type['graderType']) self.assertEqual(None, descriptor.format) self.assertEqual(False, descriptor.graded) + def test_get_set_grader_types_ajax(self): + """ + Test configuring the graders via ajax calls + """ + grader_type_url_base = self.course_locator.url_reverse('settings/grading') + # test get whole + response = self.client.get_json(grader_type_url_base) + whole_model = json.loads(response.content) + self.assertIn('graders', whole_model) + self.assertIn('grade_cutoffs', whole_model) + self.assertIn('grace_period', whole_model) + # test post/update whole + whole_model['grace_period'] = {'hours': 1, 'minutes': 30, 'seconds': 0} + response = self.client.ajax_post(grader_type_url_base, whole_model) + self.assertEqual(200, response.status_code) + response = self.client.get_json(grader_type_url_base) + whole_model = json.loads(response.content) + self.assertEqual(whole_model['grace_period'], {'hours': 1, 'minutes': 30, 'seconds': 0}) + # test get one grader + self.assertGreater(len(whole_model['graders']), 1) # ensure test will make sense + response = self.client.get_json(grader_type_url_base + '/1') + grader_sample = json.loads(response.content) + self.assertEqual(grader_sample, whole_model['graders'][1]) + # test add grader + new_grader = { + "type": "Extra Credit", + "min_count": 1, + "drop_count": 2, + "short_label": None, + "weight": 15, + } + response = self.client.ajax_post( + '{}/{}'.format(grader_type_url_base, len(whole_model['graders'])), + new_grader + ) + self.assertEqual(200, response.status_code) + grader_sample = json.loads(response.content) + new_grader['id'] = len(whole_model['graders']) + self.assertEqual(new_grader, grader_sample) + # test delete grader + response = self.client.delete(grader_type_url_base + '/1', HTTP_ACCEPT="application/json") + self.assertEqual(204, response.status_code) + response = self.client.get_json(grader_type_url_base) + updated_model = json.loads(response.content) + new_grader['id'] -= 1 # one fewer and the id mutates + self.assertIn(new_grader, updated_model['graders']) + self.assertNotIn(whole_model['graders'][1], updated_model['graders']) + + def setup_test_set_get_section_grader_ajax(self): + """ + Populate the course, grab a section, get the url for the assignment type access + """ + self.populateCourse() + sections = get_modulestore(self.course_location).get_items( + self.course_location.replace(category="sequential", name=None) + ) + # see if test makes sense + self.assertGreater(len(sections), 0, "No sections found") + section = sections[0] # just take the first one + section_locator = loc_mapper().translate_location(self.course_location.course_id, section.location, False, True) + return section_locator.url_reverse('xblock') + + def test_set_get_section_grader_ajax(self): + """ + Test setting and getting section grades via the grade as url + """ + grade_type_url = self.setup_test_set_get_section_grader_ajax() + response = self.client.ajax_post(grade_type_url, {'graderType': u'Homework'}) + self.assertEqual(200, response.status_code) + response = self.client.get_json(grade_type_url + '?fields=graderType') + self.assertEqual(json.loads(response.content).get('graderType'), u'Homework') + # and unset + response = self.client.ajax_post(grade_type_url, {'graderType': u'Not Graded'}) + self.assertEqual(200, response.status_code) + response = self.client.get_json(grade_type_url + '?fields=graderType') + self.assertEqual(json.loads(response.content).get('graderType'), u'Not Graded') + class CourseMetadataEditingTest(CourseTestCase): """ @@ -377,15 +418,19 @@ class CourseMetadataEditingTest(CourseTestCase): """ def setUp(self): CourseTestCase.setUp(self) - CourseFactory.create(org='edX', course='999', display_name='Robot Super Course') - self.fullcourse_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None]) + self.fullcourse = CourseFactory.create(org='edX', course='999', display_name='Robot Super Course') + self.course_setting_url = self.course_locator.url_reverse('settings/advanced') + self.fullcourse_setting_url = loc_mapper().translate_location( + self.fullcourse.location.course_id, + self.fullcourse.location, False, True + ).url_reverse('settings/advanced') def test_fetch_initial_fields(self): - test_model = CourseMetadata.fetch(self.course.location) + test_model = CourseMetadata.fetch(self.course) self.assertIn('display_name', test_model, 'Missing editable metadata field') self.assertEqual(test_model['display_name'], 'Robot Super Course', "not expected value") - test_model = CourseMetadata.fetch(self.fullcourse_location) + test_model = CourseMetadata.fetch(self.fullcourse) self.assertNotIn('graceperiod', test_model, 'blacklisted field leaked in') self.assertIn('display_name', test_model, 'full missing editable metadata field') self.assertEqual(test_model['display_name'], 'Robot Super Course', "not expected value") @@ -394,17 +439,17 @@ def test_fetch_initial_fields(self): self.assertIn('xqa_key', test_model, 'xqa_key field ') def test_update_from_json(self): - test_model = CourseMetadata.update_from_json(self.course.location, { + test_model = CourseMetadata.update_from_json(self.course, { "advertised_start": "start A", - "testcenter_info": {"c": "test"}, "days_early_for_beta": 2 }) self.update_check(test_model) # try fresh fetch to ensure persistence - test_model = CourseMetadata.fetch(self.course.location) + fresh = modulestore().get_item(self.course_location) + test_model = CourseMetadata.fetch(fresh) self.update_check(test_model) # now change some of the existing metadata - test_model = CourseMetadata.update_from_json(self.course.location, { + test_model = CourseMetadata.update_from_json(fresh, { "advertised_start": "start B", "display_name": "jolly roger"} ) @@ -418,13 +463,15 @@ def update_check(self, test_model): self.assertEqual(test_model['display_name'], 'Robot Super Course', "not expected value") self.assertIn('advertised_start', test_model, 'Missing new advertised_start metadata field') self.assertEqual(test_model['advertised_start'], 'start A', "advertised_start not expected value") - self.assertIn('testcenter_info', test_model, 'Missing testcenter_info metadata field') - self.assertDictEqual(test_model['testcenter_info'], {"c": "test"}, "testcenter_info not expected value") self.assertIn('days_early_for_beta', test_model, 'Missing days_early_for_beta metadata field') self.assertEqual(test_model['days_early_for_beta'], 2, "days_early_for_beta not expected value") def test_delete_key(self): - test_model = CourseMetadata.delete_key(self.fullcourse_location, {'deleteKeys': ['doesnt_exist', 'showanswer', 'xqa_key']}) + test_model = CourseMetadata.update_from_json( + self.fullcourse, { + "unsetKeys": ['showanswer', 'xqa_key'] + } + ) # ensure no harm self.assertNotIn('graceperiod', test_model, 'blacklisted field leaked in') self.assertIn('display_name', test_model, 'full missing editable metadata field') @@ -434,27 +481,113 @@ def test_delete_key(self): self.assertEqual('finished', test_model['showanswer'], 'showanswer field still in') self.assertEqual(None, test_model['xqa_key'], 'xqa_key field still in') + def test_http_fetch_initial_fields(self): + response = self.client.get_json(self.course_setting_url) + test_model = json.loads(response.content) + self.assertIn('display_name', test_model, 'Missing editable metadata field') + self.assertEqual(test_model['display_name'], 'Robot Super Course', "not expected value") + + response = self.client.get_json(self.fullcourse_setting_url) + test_model = json.loads(response.content) + self.assertNotIn('graceperiod', test_model, 'blacklisted field leaked in') + self.assertIn('display_name', test_model, 'full missing editable metadata field') + self.assertEqual(test_model['display_name'], 'Robot Super Course', "not expected value") + self.assertIn('rerandomize', test_model, 'Missing rerandomize metadata field') + self.assertIn('showanswer', test_model, 'showanswer field ') + self.assertIn('xqa_key', test_model, 'xqa_key field ') + + def test_http_update_from_json(self): + response = self.client.ajax_post(self.course_setting_url, { + "advertised_start": "start A", + "testcenter_info": {"c": "test"}, + "days_early_for_beta": 2, + "unsetKeys": ['showanswer', 'xqa_key'], + }) + test_model = json.loads(response.content) + self.update_check(test_model) + self.assertEqual('finished', test_model['showanswer'], 'showanswer field still in') + self.assertEqual(None, test_model['xqa_key'], 'xqa_key field still in') + + response = self.client.get_json(self.course_setting_url) + test_model = json.loads(response.content) + self.update_check(test_model) + # now change some of the existing metadata + response = self.client.ajax_post(self.course_setting_url, { + "advertised_start": "start B", + "display_name": "jolly roger" + }) + test_model = json.loads(response.content) + self.assertIn('display_name', test_model, 'Missing editable metadata field') + self.assertEqual(test_model['display_name'], 'jolly roger', "not expected value") + self.assertIn('advertised_start', test_model, 'Missing revised advertised_start metadata field') + self.assertEqual(test_model['advertised_start'], 'start B', "advertised_start not expected value") + + def test_advanced_components_munge_tabs(self): + """ + Test that adding and removing specific advanced components adds and removes tabs. + """ + self.assertNotIn(EXTRA_TAB_PANELS.get("open_ended"), self.course.tabs) + self.assertNotIn(EXTRA_TAB_PANELS.get("notes"), self.course.tabs) + self.client.ajax_post(self.course_setting_url, { + ADVANCED_COMPONENT_POLICY_KEY: ["combinedopenended"] + }) + course = modulestore().get_item(self.course_location) + self.assertIn(EXTRA_TAB_PANELS.get("open_ended"), course.tabs) + self.assertNotIn(EXTRA_TAB_PANELS.get("notes"), course.tabs) + self.client.ajax_post(self.course_setting_url, { + ADVANCED_COMPONENT_POLICY_KEY: [] + }) + course = modulestore().get_item(self.course_location) + self.assertNotIn(EXTRA_TAB_PANELS.get("open_ended"), course.tabs) + class CourseGraderUpdatesTest(CourseTestCase): + """ + Test getting, deleting, adding, & updating graders + """ def setUp(self): + """Compute the url to use in tests""" super(CourseGraderUpdatesTest, self).setUp() - self.url = reverse("course_settings", kwargs={ - 'org': self.course.location.org, - 'course': self.course.location.course, - 'name': self.course.location.name, - 'grader_index': 0, - }) + self.url = self.course_locator.url_reverse('settings/grading') + self.starting_graders = CourseGradingModel(self.course).graders def test_get(self): - resp = self.client.get(self.url) + """Test getting a specific grading type record.""" + resp = self.client.get_json(self.url + '/0') self.assertEqual(resp.status_code, 200) obj = json.loads(resp.content) + self.assertEqual(self.starting_graders[0], obj) def test_delete(self): - resp = self.client.delete(self.url) + """Test deleting a specific grading type record.""" + resp = self.client.delete(self.url + '/0', HTTP_ACCEPT="application/json") self.assertEqual(resp.status_code, 204) + current_graders = CourseGradingModel.fetch(self.course_locator).graders + self.assertNotIn(self.starting_graders[0], current_graders) + self.assertEqual(len(self.starting_graders) - 1, len(current_graders)) - def test_post(self): + def test_update(self): + """Test updating a specific grading type record.""" + grader = { + "id": 0, + "type": "manual", + "min_count": 5, + "drop_count": 10, + "short_label": "yo momma", + "weight": 17.3, + } + resp = self.client.ajax_post(self.url + '/0', grader) + self.assertEqual(resp.status_code, 200) + obj = json.loads(resp.content) + self.assertEqual(obj, grader) + current_graders = CourseGradingModel.fetch(self.course_locator).graders + self.assertEqual(len(self.starting_graders), len(current_graders)) + + def test_add(self): + """Test adding a grading type record.""" + # the same url works for changing the whole grading model (graceperiod, cutoffs, and grading types) when + # the grading_index is None; thus, using None to imply adding a grading_type doesn't work; so, it uses an + # index out of bounds to imply create item. grader = { "type": "manual", "min_count": 5, @@ -462,6 +595,11 @@ def test_post(self): "short_label": "yo momma", "weight": 17.3, } - resp = self.client.ajax_post(self.url, grader) + resp = self.client.ajax_post('{}/{}'.format(self.url, len(self.starting_graders) + 1), grader) self.assertEqual(resp.status_code, 200) obj = json.loads(resp.content) + self.assertEqual(obj['id'], len(self.starting_graders)) + del obj['id'] + self.assertEqual(obj, grader) + current_graders = CourseGradingModel.fetch(self.course_locator).graders + self.assertEqual(len(self.starting_graders) + 1, len(current_graders)) diff --git a/cms/djangoapps/contentstore/tests/test_import_export.py b/cms/djangoapps/contentstore/tests/test_import_export.py index 20957a350809..85df894cd414 100644 --- a/cms/djangoapps/contentstore/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/tests/test_import_export.py @@ -263,7 +263,7 @@ def test_export_failure_subsection_level(self): parent_location=vertical.location, category='aawefawef' ) - self._verify_export_failure('/edit/i4x://MITx/999/vertical/foo') + self._verify_export_failure(u'/unit/MITx.999.Robot_Super_Course/branch/draft/block/foo') def _verify_export_failure(self, expectedText): """ Export failure helper method. """ diff --git a/cms/djangoapps/contentstore/tests/test_item.py b/cms/djangoapps/contentstore/tests/test_item.py index b34dab0a6d29..4922b4888a32 100644 --- a/cms/djangoapps/contentstore/tests/test_item.py +++ b/cms/djangoapps/contentstore/tests/test_item.py @@ -9,6 +9,7 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.django import loc_mapper from xmodule.modulestore.locator import BlockUsageLocator +from xmodule.modulestore.exceptions import ItemNotFoundError class ItemTest(CourseTestCase): @@ -30,7 +31,7 @@ def get_item_from_modulestore(self, locator, draft=False): """ Get the item referenced by the locator from the modulestore """ - store = modulestore('draft') if draft else modulestore() + store = modulestore('draft') if draft else modulestore('direct') return store.get_item(self.get_old_id(locator)) def response_locator(self, response): @@ -251,3 +252,105 @@ def test_reorder_children(self): self.assertEqual(self.get_old_id(self.problem_locator).url(), children[0]) self.assertEqual(self.get_old_id(unit1_locator).url(), children[2]) self.assertEqual(self.get_old_id(unit2_locator).url(), children[1]) + + def test_make_public(self): + """ Test making a private problem public (publishing it). """ + # When the problem is first created, it is only in draft (because of its category). + with self.assertRaises(ItemNotFoundError): + self.get_item_from_modulestore(self.problem_locator, False) + self.client.ajax_post( + self.problem_update_url, + data={'publish': 'make_public'} + ) + self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False)) + + def test_make_private(self): + """ Test making a public problem private (un-publishing it). """ + # Make problem public. + self.client.ajax_post( + self.problem_update_url, + data={'publish': 'make_public'} + ) + self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False)) + # Now make it private + self.client.ajax_post( + self.problem_update_url, + data={'publish': 'make_private'} + ) + with self.assertRaises(ItemNotFoundError): + self.get_item_from_modulestore(self.problem_locator, False) + + def test_make_draft(self): + """ Test creating a draft version of a public problem. """ + # Make problem public. + self.client.ajax_post( + self.problem_update_url, + data={'publish': 'make_public'} + ) + self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False)) + # Now make it draft, which means both versions will exist. + self.client.ajax_post( + self.problem_update_url, + data={'publish': 'create_draft'} + ) + # Update the draft version and check that published is different. + self.client.ajax_post( + self.problem_update_url, + data={'metadata': {'due': '2077-10-10T04:00Z'}} + ) + published = self.get_item_from_modulestore(self.problem_locator, False) + self.assertIsNone(published.due) + draft = self.get_item_from_modulestore(self.problem_locator, True) + self.assertEqual(draft.due, datetime.datetime(2077, 10, 10, 4, 0, tzinfo=UTC)) + + def test_make_public_with_update(self): + """ Update a problem and make it public at the same time. """ + self.client.ajax_post( + self.problem_update_url, + data={ + 'metadata': {'due': '2077-10-10T04:00Z'}, + 'publish': 'make_public' + } + ) + published = self.get_item_from_modulestore(self.problem_locator, False) + self.assertEqual(published.due, datetime.datetime(2077, 10, 10, 4, 0, tzinfo=UTC)) + + def test_make_private_with_update(self): + """ Make a problem private and update it at the same time. """ + # Make problem public. + self.client.ajax_post( + self.problem_update_url, + data={'publish': 'make_public'} + ) + self.client.ajax_post( + self.problem_update_url, + data={ + 'metadata': {'due': '2077-10-10T04:00Z'}, + 'publish': 'make_private' + } + ) + with self.assertRaises(ItemNotFoundError): + self.get_item_from_modulestore(self.problem_locator, False) + draft = self.get_item_from_modulestore(self.problem_locator, True) + self.assertEqual(draft.due, datetime.datetime(2077, 10, 10, 4, 0, tzinfo=UTC)) + + def test_create_draft_with_update(self): + """ Create a draft and update it at the same time. """ + # Make problem public. + self.client.ajax_post( + self.problem_update_url, + data={'publish': 'make_public'} + ) + self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False)) + # Now make it draft, which means both versions will exist. + self.client.ajax_post( + self.problem_update_url, + data={ + 'metadata': {'due': '2077-10-10T04:00Z'}, + 'publish': 'create_draft' + } + ) + published = self.get_item_from_modulestore(self.problem_locator, False) + self.assertIsNone(published.due) + draft = self.get_item_from_modulestore(self.problem_locator, True) + self.assertEqual(draft.due, datetime.datetime(2077, 10, 10, 4, 0, tzinfo=UTC)) diff --git a/cms/djangoapps/contentstore/tests/test_permissions.py b/cms/djangoapps/contentstore/tests/test_permissions.py new file mode 100644 index 000000000000..6dae9d883171 --- /dev/null +++ b/cms/djangoapps/contentstore/tests/test_permissions.py @@ -0,0 +1,127 @@ +""" +Test CRUD for authorization. +""" +from django.test.utils import override_settings +from django.contrib.auth.models import User, Group + +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from contentstore.tests.modulestore_config import TEST_MODULESTORE +from contentstore.tests.utils import AjaxEnabledTestClient +from xmodule.modulestore.django import loc_mapper +from xmodule.modulestore import Location +from auth.authz import INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME +from auth import authz +import copy +from contentstore.views.access import has_access + + +@override_settings(MODULESTORE=TEST_MODULESTORE) +class TestCourseAccess(ModuleStoreTestCase): + """ + Course-based access (as opposed to access of a non-course xblock) + """ + def setUp(self): + """ + Create a staff user and log them in (creating the client). + + Create a pool of users w/o granting them any permissions + """ + super(TestCourseAccess, self).setUp() + uname = 'testuser' + email = 'test+courses@edx.org' + password = 'foo' + + # Create the use so we can log them in. + self.user = User.objects.create_user(uname, email, password) + + # Note that we do not actually need to do anything + # for registration if we directly mark them active. + self.user.is_active = True + # Staff has access to view all courses + self.user.is_staff = True + self.user.save() + + self.client = AjaxEnabledTestClient() + self.client.login(username=uname, password=password) + + # create a course via the view handler which has a different strategy for permissions than the factory + self.course_location = Location(['i4x', 'myu', 'mydept.mycourse', 'course', 'myrun']) + self.course_locator = loc_mapper().translate_location( + self.course_location.course_id, self.course_location, False, True + ) + self.client.ajax_post( + self.course_locator.url_reverse('course'), + { + 'org': self.course_location.org, + 'number': self.course_location.course, + 'display_name': 'My favorite course', + 'run': self.course_location.name, + } + ) + + self.users = self._create_users() + + def _create_users(self): + """ + Create 8 users and return them + """ + users = [] + for i in range(8): + username = "user{}".format(i) + email = "test+user{}@edx.org".format(i) + user = User.objects.create_user(username, email, 'foo') + user.is_active = True + user.save() + users.append(user) + return users + + def tearDown(self): + """ + Reverse the setup + """ + self.client.logout() + ModuleStoreTestCase.tearDown(self) + + def test_get_all_users(self): + """ + Test getting all authors for a course where their permissions run the gamut of allowed group + types. + """ + # first check the groupname for the course creator. + self.assertTrue( + self.user.groups.filter( + name="{}_{}".format(INSTRUCTOR_ROLE_NAME, self.course_locator.course_id) + ).exists(), + "Didn't add creator as instructor." + ) + users = copy.copy(self.users) + user_by_role = {} + # add the misc users to the course in different groups + for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]: + user_by_role[role] = [] + groupnames, _ = authz.get_all_course_role_groupnames(self.course_locator, role) + for groupname in groupnames: + group, _ = Group.objects.get_or_create(name=groupname) + user = users.pop() + user_by_role[role].append(user) + user.groups.add(group) + user.save() + self.assertTrue(has_access(user, self.course_locator), "{} does not have access".format(user)) + self.assertTrue(has_access(user, self.course_location), "{} does not have access".format(user)) + + response = self.client.get_html(self.course_locator.url_reverse('course_team')) + for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]: + for user in user_by_role[role]: + self.assertContains(response, user.email) + + # test copying course permissions + copy_course_location = Location(['i4x', 'copyu', 'copydept.mycourse', 'course', 'myrun']) + copy_course_locator = loc_mapper().translate_location( + copy_course_location.course_id, copy_course_location, False, True + ) + # pylint: disable=protected-access + authz._copy_course_group(self.course_locator, copy_course_locator) + for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]: + for user in user_by_role[role]: + self.assertTrue(has_access(user, copy_course_locator), "{} no copy access".format(user)) + self.assertTrue(has_access(user, copy_course_location), "{} no copy access".format(user)) \ No newline at end of file diff --git a/cms/djangoapps/contentstore/tests/test_transcripts.py b/cms/djangoapps/contentstore/tests/test_transcripts.py index 695fdcd09f99..f4c7f773adf0 100644 --- a/cms/djangoapps/contentstore/tests/test_transcripts.py +++ b/cms/djangoapps/contentstore/tests/test_transcripts.py @@ -20,6 +20,7 @@ from xmodule.contentstore.content import StaticContent from xmodule.exceptions import NotFoundError from xmodule.modulestore.django import loc_mapper +from xmodule.modulestore.locator import BlockUsageLocator from contentstore.tests.modulestore_config import TEST_MODULESTORE TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) @@ -59,7 +60,7 @@ def setUp(self): 'type': 'video' } resp = self.client.ajax_post('/xblock', data) - self.item_location = json.loads(resp.content).get('id') + self.item_locator, self.item_location = self._get_locator(resp) self.assertEqual(resp.status_code, 200) # hI10vDNYz4M - valid Youtube ID with transcripts. @@ -72,6 +73,11 @@ def setUp(self): # Remove all transcripts for current module. self.clear_subs_content() + def _get_locator(self, resp): + """ Returns the locator and old-style location (as a string) from the response returned by a create operation. """ + locator = json.loads(resp.content).get('locator') + return locator, loc_mapper().translate_locator_to_location(BlockUsageLocator(locator)).url() + def get_youtube_ids(self): """Return youtube speeds and ids.""" item = modulestore().get_item(self.item_location) @@ -136,7 +142,7 @@ def test_success_video_module_source_subs_uploading(self): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0] resp = self.client.post(link, { - 'id': self.item_location, + 'locator': self.item_locator, 'file': self.good_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -158,20 +164,20 @@ def test_fail_data_without_id(self): link = reverse('upload_transcripts') resp = self.client.post(link, {'file': self.good_srt_file}) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), 'POST data without "id" form data.') + self.assertEqual(json.loads(resp.content).get('status'), 'POST data without "locator" form data.') def test_fail_data_without_file(self): link = reverse('upload_transcripts') - resp = self.client.post(link, {'id': self.item_location}) + resp = self.client.post(link, {'locator': self.item_locator}) self.assertEqual(resp.status_code, 400) self.assertEqual(json.loads(resp.content).get('status'), 'POST data without "file" form data.') - def test_fail_data_with_bad_location(self): + def test_fail_data_with_bad_locator(self): # Test for raising `InvalidLocationError` exception. link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0] resp = self.client.post(link, { - 'id': 'BAD_LOCATION', + 'locator': 'BAD_LOCATOR', 'file': self.good_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -180,13 +186,13 @@ def test_fail_data_with_bad_location(self): }]) }) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by location.") + self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by locator.") # Test for raising `ItemNotFoundError` exception. link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0] resp = self.client.post(link, { - 'id': '{0}_{1}'.format(self.item_location, 'BAD_LOCATION'), + 'locator': '{0}_{1}'.format(self.item_locator, 'BAD_LOCATOR'), 'file': self.good_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -195,7 +201,7 @@ def test_fail_data_with_bad_location(self): }]) }) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by location.") + self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by locator.") def test_fail_for_non_video_module(self): # non_video module: setup @@ -205,7 +211,7 @@ def test_fail_for_non_video_module(self): 'type': 'non_video' } resp = self.client.ajax_post('/xblock', data) - item_location = json.loads(resp.content).get('id') + item_locator, item_location = self._get_locator(resp) data = '' modulestore().update_item(item_location, data) @@ -214,7 +220,7 @@ def test_fail_for_non_video_module(self): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0] resp = self.client.post(link, { - 'id': item_location, + 'locator': item_locator, 'file': self.good_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -232,7 +238,7 @@ def test_fail_bad_xml(self): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0] resp = self.client.post(link, { - 'id': self.item_location, + 'locator': self.item_locator, 'file': self.good_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -249,7 +255,7 @@ def test_fail_bad_data_srt_file(self): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.bad_data_srt_file.name))[0] resp = self.client.post(link, { - 'id': self.item_location, + 'locator': self.item_locator, 'file': self.bad_data_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -264,7 +270,7 @@ def test_fail_bad_name_srt_file(self): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(self.bad_name_srt_file.name))[0] resp = self.client.post(link, { - 'id': self.item_location, + 'locator': self.item_locator, 'file': self.bad_name_srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -291,7 +297,7 @@ def test_undefined_file_extension(self): link = reverse('upload_transcripts') filename = os.path.splitext(os.path.basename(srt_file.name))[0] resp = self.client.post(link, { - 'id': self.item_location, + 'locator': self.item_locator, 'file': srt_file, 'video_list': json.dumps([{ 'type': 'html5', @@ -353,7 +359,7 @@ def test_success_download_youtube(self): self.save_subs_to_store(subs, 'JMD_ifUUfsU') link = reverse('download_transcripts') - resp = self.client.get(link, {'id': self.item_location, 'subs_id': "JMD_ifUUfsU"}) + resp = self.client.get(link, {'locator': self.item_locator, 'subs_id': "JMD_ifUUfsU"}) self.assertEqual(resp.status_code, 200) self.assertEqual(resp.content, """0\n00:00:00,100 --> 00:00:00,200\nsubs #1\n\n1\n00:00:00,200 --> 00:00:00,240\nsubs #2\n\n2\n00:00:00,240 --> 00:00:00,380\nsubs #3\n\n""") @@ -380,7 +386,7 @@ def test_success_download_nonyoutube(self): self.save_subs_to_store(subs, subs_id) link = reverse('download_transcripts') - resp = self.client.get(link, {'id': self.item_location, 'subs_id': subs_id}) + resp = self.client.get(link, {'locator': self.item_locator, 'subs_id': subs_id}) self.assertEqual(resp.status_code, 200) self.assertEqual( resp.content, @@ -391,21 +397,21 @@ def test_success_download_nonyoutube(self): def test_fail_data_without_file(self): link = reverse('download_transcripts') - resp = self.client.get(link, {'id': ''}) + resp = self.client.get(link, {'locator': ''}) self.assertEqual(resp.status_code, 404) resp = self.client.get(link, {}) self.assertEqual(resp.status_code, 404) - def test_fail_data_with_bad_location(self): + def test_fail_data_with_bad_locator(self): # Test for raising `InvalidLocationError` exception. link = reverse('download_transcripts') - resp = self.client.get(link, {'id': 'BAD_LOCATION'}) + resp = self.client.get(link, {'locator': 'BAD_LOCATOR'}) self.assertEqual(resp.status_code, 404) # Test for raising `ItemNotFoundError` exception. link = reverse('download_transcripts') - resp = self.client.get(link, {'id': '{0}_{1}'.format(self.item_location, 'BAD_LOCATION')}) + resp = self.client.get(link, {'locator': '{0}_{1}'.format(self.item_locator, 'BAD_LOCATOR')}) self.assertEqual(resp.status_code, 404) def test_fail_for_non_video_module(self): @@ -416,7 +422,7 @@ def test_fail_for_non_video_module(self): 'type': 'videoalpha' } resp = self.client.ajax_post('/xblock', data) - item_location = json.loads(resp.content).get('id') + item_locator, item_location = self._get_locator(resp) subs_id = str(uuid4()) data = textwrap.dedent(""" @@ -439,7 +445,7 @@ def test_fail_for_non_video_module(self): self.save_subs_to_store(subs, subs_id) link = reverse('download_transcripts') - resp = self.client.get(link, {'id': item_location}) + resp = self.client.get(link, {'locator': item_locator}) self.assertEqual(resp.status_code, 404) def test_fail_nonyoutube_subs_dont_exist(self): @@ -453,7 +459,7 @@ def test_fail_nonyoutube_subs_dont_exist(self): modulestore().update_item(self.item_location, data) link = reverse('download_transcripts') - resp = self.client.get(link, {'id': self.item_location}) + resp = self.client.get(link, {'locator': self.item_locator}) self.assertEqual(resp.status_code, 404) def test_empty_youtube_attr_and_sub_attr(self): @@ -467,7 +473,7 @@ def test_empty_youtube_attr_and_sub_attr(self): modulestore().update_item(self.item_location, data) link = reverse('download_transcripts') - resp = self.client.get(link, {'id': self.item_location}) + resp = self.client.get(link, {'locator': self.item_locator}) self.assertEqual(resp.status_code, 404) @@ -492,7 +498,7 @@ def test_fail_bad_sjson_subs(self): self.save_subs_to_store(subs, 'JMD_ifUUfsU') link = reverse('download_transcripts') - resp = self.client.get(link, {'id': self.item_location}) + resp = self.client.get(link, {'locator': self.item_locator}) self.assertEqual(resp.status_code, 404) @@ -547,7 +553,7 @@ def test_success_download_nonyoutube(self): self.save_subs_to_store(subs, subs_id) data = { - 'id': self.item_location, + 'locator': self.item_locator, 'videos': [{ 'type': 'html5', 'video': subs_id, @@ -591,7 +597,7 @@ def test_check_youtube(self): self.save_subs_to_store(subs, 'JMD_ifUUfsU') link = reverse('check_transcripts') data = { - 'id': self.item_location, + 'locator': self.item_locator, 'videos': [{ 'type': 'youtube', 'video': 'JMD_ifUUfsU', @@ -619,7 +625,7 @@ def test_check_youtube(self): def test_fail_data_without_id(self): link = reverse('check_transcripts') data = { - 'id': '', + 'locator': '', 'videos': [{ 'type': '', 'video': '', @@ -628,13 +634,13 @@ def test_fail_data_without_id(self): } resp = self.client.get(link, {'data': json.dumps(data)}) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by location.") + self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by locator.") - def test_fail_data_with_bad_location(self): + def test_fail_data_with_bad_locator(self): # Test for raising `InvalidLocationError` exception. link = reverse('check_transcripts') data = { - 'id': '', + 'locator': '', 'videos': [{ 'type': '', 'video': '', @@ -643,11 +649,11 @@ def test_fail_data_with_bad_location(self): } resp = self.client.get(link, {'data': json.dumps(data)}) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by location.") + self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by locator.") # Test for raising `ItemNotFoundError` exception. data = { - 'id': '{0}_{1}'.format(self.item_location, 'BAD_LOCATION'), + 'locator': '{0}_{1}'.format(self.item_locator, 'BAD_LOCATOR'), 'videos': [{ 'type': '', 'video': '', @@ -656,7 +662,7 @@ def test_fail_data_with_bad_location(self): } resp = self.client.get(link, {'data': json.dumps(data)}) self.assertEqual(resp.status_code, 400) - self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by location.") + self.assertEqual(json.loads(resp.content).get('status'), "Can't find item by locator.") def test_fail_for_non_video_module(self): # Not video module: setup @@ -666,7 +672,7 @@ def test_fail_for_non_video_module(self): 'type': 'not_video' } resp = self.client.ajax_post('/xblock', data) - item_location = json.loads(resp.content).get('id') + item_locator, item_location = self._get_locator(resp) subs_id = str(uuid4()) data = textwrap.dedent(""" @@ -689,7 +695,7 @@ def test_fail_for_non_video_module(self): self.save_subs_to_store(subs, subs_id) data = { - 'id': item_location, + 'locator': item_locator, 'videos': [{ 'type': '', 'video': '', diff --git a/cms/djangoapps/contentstore/tests/test_users.py b/cms/djangoapps/contentstore/tests/test_users.py index c10dec61bd61..ec8b5570a534 100644 --- a/cms/djangoapps/contentstore/tests/test_users.py +++ b/cms/djangoapps/contentstore/tests/test_users.py @@ -29,8 +29,8 @@ def setUp(self): self.detail_url = self.location.url_reverse('course_team', self.ext_user.email) self.inactive_detail_url = self.location.url_reverse('course_team', self.inactive_user.email) self.invalid_detail_url = self.location.url_reverse('course_team', "nonexistent@user.com") - self.staff_groupname = get_course_groupname_for_role(self.course.location, "staff") - self.inst_groupname = get_course_groupname_for_role(self.course.location, "instructor") + self.staff_groupname = get_course_groupname_for_role(self.course_locator, "staff") + self.inst_groupname = get_course_groupname_for_role(self.course_locator, "instructor") def test_index(self): resp = self.client.get(self.index_url, HTTP_ACCEPT='text/html') @@ -145,18 +145,6 @@ def test_detail_post_missing_role(self): self.assertIn("error", result) self.assert_not_enrolled() - def test_detail_post_bad_json(self): - resp = self.client.post( - self.detail_url, - data="{foo}", - content_type="application/json", - HTTP_ACCEPT="application/json", - ) - self.assertEqual(resp.status_code, 400) - result = json.loads(resp.content) - self.assertIn("error", result) - self.assert_not_enrolled() - def test_detail_post_no_json(self): resp = self.client.post( self.detail_url, diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index b59f21405476..0e716cc878bb 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -10,8 +10,9 @@ from django.test.utils import override_settings from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from contentstore.tests.modulestore_config import TEST_MODULESTORE +from xmodule.modulestore.django import loc_mapper def parse_json(response): @@ -41,6 +42,7 @@ def ajax_post(self, path, data=None, content_type="application/json", **kwargs): if not isinstance(data, basestring): data = json.dumps(data or {}) kwargs.setdefault("HTTP_X_REQUESTED_WITH", "XMLHttpRequest") + kwargs.setdefault("HTTP_ACCEPT", "application/json") return self.post(path=path, data=data, content_type=content_type, **kwargs) def get_html(self, path, data=None, follow=False, **extra): @@ -88,6 +90,9 @@ def setUp(self): display_name='Robot Super Course', ) self.course_location = self.course.location + self.course_locator = loc_mapper().translate_location( + self.course.location.course_id, self.course.location, False, True + ) def createNonStaffAuthedUserClient(self): """ @@ -106,3 +111,16 @@ def createNonStaffAuthedUserClient(self): client = Client() client.login(username=uname, password=password) return client, nonstaff + + def populateCourse(self): + """ + Add 2 chapters, 4 sections, 8 verticals, 16 problems to self.course (branching 2) + """ + def descend(parent, stack): + xblock_type = stack.pop(0) + for _ in range(2): + child = ItemFactory.create(category=xblock_type, parent_location=parent.location) + if stack: + descend(child, stack) + + descend(self.course, ['chapter', 'sequential', 'vertical', 'problem']) diff --git a/cms/djangoapps/contentstore/views/checklist.py b/cms/djangoapps/contentstore/views/checklist.py index 5643e5c04421..61c6c672a73f 100644 --- a/cms/djangoapps/contentstore/views/checklist.py +++ b/cms/djangoapps/contentstore/views/checklist.py @@ -5,7 +5,6 @@ from django.http import HttpResponseBadRequest from django.contrib.auth.decorators import login_required from django.views.decorators.http import require_http_methods -from django.core.urlresolvers import reverse from django_future.csrf import ensure_csrf_cookie from mitxmako.shortcuts import render_to_response from django.http import HttpResponseNotFound @@ -22,6 +21,8 @@ __all__ = ['checklists_handler'] + +# pylint: disable=unused-argument @require_http_methods(("GET", "POST", "PUT")) @login_required @ensure_csrf_cookie @@ -85,8 +86,8 @@ def checklists_handler(request, tag=None, course_id=None, branch=None, version_g return JsonResponse(expanded_checklist) else: return HttpResponseBadRequest( - ( "Could not save checklist state because the checklist index " - "was out of range or unspecified."), + ("Could not save checklist state because the checklist index " + "was out of range or unspecified."), content_type="text/plain" ) else: @@ -113,14 +114,12 @@ def expand_checklist_action_url(course_module, checklist): The method does a copy of the input checklist and does not modify the input argument. """ expanded_checklist = copy.deepcopy(checklist) - oldurlconf_map = { - "SettingsDetails": "settings_details", - "SettingsGrading": "settings_grading" - } urlconf_map = { "ManageUsers": "course_team", - "CourseOutline": "course" + "CourseOutline": "course", + "SettingsDetails": "settings/details", + "SettingsGrading": "settings/grading", } for item in expanded_checklist.get('items'): @@ -130,12 +129,5 @@ def expand_checklist_action_url(course_module, checklist): ctx_loc = course_module.location location = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True) item['action_url'] = location.url_reverse(url_prefix, '') - elif action_url in oldurlconf_map: - urlconf_name = oldurlconf_map[action_url] - item['action_url'] = reverse(urlconf_name, kwargs={ - 'org': course_module.location.org, - 'course': course_module.location.course, - 'name': course_module.location.name, - }) return expanded_checklist diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 327e75c7f47e..ce20e004ee8a 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -2,21 +2,19 @@ import logging from collections import defaultdict -from django.http import (HttpResponse, HttpResponseBadRequest, - HttpResponseForbidden) +from django.http import HttpResponseBadRequest from django.contrib.auth.decorators import login_required from django.views.decorators.http import require_http_methods from django.core.exceptions import PermissionDenied from django_future.csrf import ensure_csrf_cookie from django.conf import settings -from xmodule.modulestore.exceptions import (ItemNotFoundError, - InvalidLocationError) +from xmodule.modulestore.exceptions import ItemNotFoundError from mitxmako.shortcuts import render_to_response -from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore from xmodule.util.date_utils import get_default_time_display from xmodule.modulestore.django import loc_mapper +from xmodule.modulestore.locator import BlockUsageLocator from xblock.fields import Scope from util.json_request import expect_json, JsonResponse @@ -25,7 +23,6 @@ from models.settings.course_grading import CourseGradingModel -from .helpers import _xmodule_recurse from .access import has_access from xmodule.x_module import XModuleDescriptor from xblock.plugin import PluginMissingError @@ -33,17 +30,13 @@ __all__ = ['OPEN_ENDED_COMPONENT_TYPES', 'ADVANCED_COMPONENT_POLICY_KEY', - 'edit_subsection', - 'edit_unit', - 'assignment_type_update', - 'create_draft', - 'publish_draft', - 'unpublish_unit', + 'subsection_handler', + 'unit_handler' ] log = logging.getLogger(__name__) -# NOTE: edit_unit assumes this list is disjoint from ADVANCED_COMPONENT_TYPES +# NOTE: unit_handler assumes this list is disjoint from ADVANCED_COMPONENT_TYPES COMPONENT_TYPES = ['discussion', 'html', 'problem', 'video'] OPEN_ENDED_COMPONENT_TYPES = ["combinedopenended", "peergrading"] @@ -58,93 +51,87 @@ ADVANCED_COMPONENT_POLICY_KEY = 'advanced_modules' +@require_http_methods(["GET"]) @login_required -def edit_subsection(request, location): - "Edit the subsection of a course" - # check that we have permissions to edit this item - try: - course = get_course_for_item(location) - except InvalidLocationError: - return HttpResponseBadRequest() - - if not has_access(request.user, course.location): - raise PermissionDenied() +def subsection_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): + """ + The restful handler for subsection-specific requests. - try: - item = modulestore().get_item(location, depth=1) - except ItemNotFoundError: - return HttpResponseBadRequest() + GET + html: return html page for editing a subsection + json: not currently supported + """ + if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'): + locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + try: + old_location, course, item, lms_link = _get_item_in_course(request, locator) + except ItemNotFoundError: + return HttpResponseBadRequest() - lms_link = get_lms_link_for_item( - location, course_id=course.location.course_id - ) - preview_link = get_lms_link_for_item( - location, course_id=course.location.course_id, preview=True - ) + preview_link = get_lms_link_for_item(old_location, course_id=course.location.course_id, preview=True) - # make sure that location references a 'sequential', otherwise return - # BadRequest - if item.location.category != 'sequential': - return HttpResponseBadRequest() + # make sure that location references a 'sequential', otherwise return + # BadRequest + if item.location.category != 'sequential': + return HttpResponseBadRequest() - parent_locs = modulestore().get_parent_locations(location, None) + parent_locs = modulestore().get_parent_locations(old_location, None) - # we're for now assuming a single parent - if len(parent_locs) != 1: - logging.error( + # we're for now assuming a single parent + if len(parent_locs) != 1: + logging.error( 'Multiple (or none) parents have been found for %s', - location + unicode(locator) + ) + + # this should blow up if we don't find any parents, which would be erroneous + parent = modulestore().get_item(parent_locs[0]) + + # remove all metadata from the generic dictionary that is presented in a + # more normalized UI. We only want to display the XBlocks fields, not + # the fields from any mixins that have been added + fields = getattr(item, 'unmixed_class', item.__class__).fields + + policy_metadata = dict( + (field.name, field.read_from(item)) + for field + in fields.values() + if field.name not in ['display_name', 'start', 'due', 'format'] and field.scope == Scope.settings + ) + + can_view_live = False + subsection_units = item.get_children() + for unit in subsection_units: + state = compute_unit_state(unit) + if state == UnitState.public or state == UnitState.draft: + can_view_live = True + break + + course_locator = loc_mapper().translate_location( + course.location.course_id, course.location, False, True ) - # this should blow up if we don't find any parents, which would be erroneous - parent = modulestore().get_item(parent_locs[0]) - - # remove all metadata from the generic dictionary that is presented in a - # more normalized UI. We only want to display the XBlocks fields, not - # the fields from any mixins that have been added - fields = getattr(item, 'unmixed_class', item.__class__).fields - - policy_metadata = dict( - (field.name, field.read_from(item)) - for field - in fields.values() - if field.name not in ['display_name', 'start', 'due', 'format'] - and field.scope == Scope.settings - ) - - can_view_live = False - subsection_units = item.get_children() - for unit in subsection_units: - state = compute_unit_state(unit) - if state == UnitState.public or state == UnitState.draft: - can_view_live = True - break - - locator = loc_mapper().translate_location( - course.location.course_id, item.location, False, True - ) - - return render_to_response( - 'edit_subsection.html', - { - 'subsection': item, - 'context_course': course, - 'new_unit_category': 'vertical', - 'lms_link': lms_link, - 'preview_link': preview_link, - 'course_graders': json.dumps(CourseGradingModel.fetch(course.location).graders), - # For grader, which is not yet converted - 'parent_location': course.location, - 'parent_item': parent, - 'locator': locator, - 'policy_metadata': policy_metadata, - 'subsection_units': subsection_units, - 'can_view_live': can_view_live - } - ) - - -def load_mixed_class(category): + return render_to_response( + 'edit_subsection.html', + { + 'subsection': item, + 'context_course': course, + 'new_unit_category': 'vertical', + 'lms_link': lms_link, + 'preview_link': preview_link, + 'course_graders': json.dumps(CourseGradingModel.fetch(course_locator).graders), + 'parent_item': parent, + 'locator': locator, + 'policy_metadata': policy_metadata, + 'subsection_units': subsection_units, + 'can_view_live': can_view_live + } + ) + else: + return HttpResponseBadRequest("Only supports html requests") + + +def _load_mixed_class(category): """ Load an XBlock by category name, and apply all defined mixins """ @@ -153,139 +140,121 @@ def load_mixed_class(category): return mixologist.mix(component_class) +@require_http_methods(["GET"]) @login_required -def edit_unit(request, location): +def unit_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): """ - Display an editing page for the specified module. - - Expects a GET request with the parameter `id`. + The restful handler for unit-specific requests. - id: A Location URL + GET + html: return html page for editing a unit + json: not currently supported """ - try: - course = get_course_for_item(location) - except InvalidLocationError: - return HttpResponseBadRequest() - - if not has_access(request.user, course.location): - raise PermissionDenied() + if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'): + locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + try: + old_location, course, item, lms_link = _get_item_in_course(request, locator) + except ItemNotFoundError: + return HttpResponseBadRequest() + + component_templates = defaultdict(list) + for category in COMPONENT_TYPES: + component_class = _load_mixed_class(category) + # add the default template + # TODO: Once mixins are defined per-application, rather than per-runtime, + # this should use a cms mixed-in class. (cpennington) + if hasattr(component_class, 'display_name'): + display_name = component_class.display_name.default or 'Blank' + else: + display_name = 'Blank' + component_templates[category].append(( + display_name, + category, + False, # No defaults have markdown (hardcoded current default) + None # no boilerplate for overrides + )) + # add boilerplates + if hasattr(component_class, 'templates'): + for template in component_class.templates(): + filter_templates = getattr(component_class, 'filter_templates', None) + if not filter_templates or filter_templates(template, course): + component_templates[category].append(( + template['metadata'].get('display_name'), + category, + template['metadata'].get('markdown') is not None, + template.get('template_id') + )) - try: - item = modulestore().get_item(location, depth=1) - except ItemNotFoundError: - return HttpResponseBadRequest() - lms_link = get_lms_link_for_item( - item.location, - course_id=course.location.course_id - ) - - # Note that the unit_state (draft, public, private) does not match up with the published value - # passed to translate_location. The two concepts are different at this point. - unit_locator = loc_mapper().translate_location( - course.location.course_id, Location(location), False, True - ) - - component_templates = defaultdict(list) - for category in COMPONENT_TYPES: - component_class = load_mixed_class(category) - # add the default template - # TODO: Once mixins are defined per-application, rather than per-runtime, - # this should use a cms mixed-in class. (cpennington) - if hasattr(component_class, 'display_name'): - display_name = component_class.display_name.default or 'Blank' + # Check if there are any advanced modules specified in the course policy. + # These modules should be specified as a list of strings, where the strings + # are the names of the modules in ADVANCED_COMPONENT_TYPES that should be + # enabled for the course. + course_advanced_keys = course.advanced_modules + + # Set component types according to course policy file + if isinstance(course_advanced_keys, list): + for category in course_advanced_keys: + if category in ADVANCED_COMPONENT_TYPES: + # Do I need to allow for boilerplates or just defaults on the + # class? i.e., can an advanced have more than one entry in the + # menu? one for default and others for prefilled boilerplates? + try: + component_class = _load_mixed_class(category) + + component_templates['advanced'].append( + ( + component_class.display_name.default or category, + category, + False, + None # don't override default data + ) + ) + except PluginMissingError: + # dhm: I got this once but it can happen any time the + # course author configures an advanced component which does + # not exist on the server. This code here merely + # prevents any authors from trying to instantiate the + # non-existent component type by not showing it in the menu + pass else: - display_name = 'Blank' - component_templates[category].append(( - display_name, - category, - False, # No defaults have markdown (hardcoded current default) - None # no boilerplate for overrides - )) - # add boilerplates - if hasattr(component_class, 'templates'): - for template in component_class.templates(): - filter_templates = getattr(component_class, 'filter_templates', None) - if not filter_templates or filter_templates(template, course): - component_templates[category].append(( - template['metadata'].get('display_name'), - category, - template['metadata'].get('markdown') is not None, - template.get('template_id') - )) - - # Check if there are any advanced modules specified in the course policy. - # These modules should be specified as a list of strings, where the strings - # are the names of the modules in ADVANCED_COMPONENT_TYPES that should be - # enabled for the course. - course_advanced_keys = course.advanced_modules - - # Set component types according to course policy file - if isinstance(course_advanced_keys, list): - for category in course_advanced_keys: - if category in ADVANCED_COMPONENT_TYPES: - # Do I need to allow for boilerplates or just defaults on the - # class? i.e., can an advanced have more than one entry in the - # menu? one for default and others for prefilled boilerplates? - try: - component_class = load_mixed_class(category) - - component_templates['advanced'].append(( - component_class.display_name.default or category, - category, - False, - None # don't override default data - )) - except PluginMissingError: - # dhm: I got this once but it can happen any time the - # course author configures an advanced component which does - # not exist on the server. This code here merely - # prevents any authors from trying to instantiate the - # non-existent component type by not showing it in the menu - pass - else: - log.error( - "Improper format for course advanced keys! %s", - course_advanced_keys - ) + log.error( + "Improper format for course advanced keys! %s", + course_advanced_keys + ) - components = [ - [ - component.location.url(), + components = [ loc_mapper().translate_location( course.location.course_id, component.location, False, True ) + for component + in item.get_children() ] - for component - in item.get_children() - ] - - # TODO (cpennington): If we share units between courses, - # this will need to change to check permissions correctly so as - # to pick the correct parent subsection - - containing_subsection_locs = modulestore().get_parent_locations( - location, None - ) - containing_subsection = modulestore().get_item(containing_subsection_locs[0]) - containing_section_locs = modulestore().get_parent_locations( + + # TODO (cpennington): If we share units between courses, + # this will need to change to check permissions correctly so as + # to pick the correct parent subsection + + containing_subsection_locs = modulestore().get_parent_locations(old_location, None) + containing_subsection = modulestore().get_item(containing_subsection_locs[0]) + containing_section_locs = modulestore().get_parent_locations( containing_subsection.location, None - ) - containing_section = modulestore().get_item(containing_section_locs[0]) + ) + containing_section = modulestore().get_item(containing_section_locs[0]) - # cdodge hack. We're having trouble previewing drafts via jump_to redirect - # so let's generate the link url here + # cdodge hack. We're having trouble previewing drafts via jump_to redirect + # so let's generate the link url here - # need to figure out where this item is in the list of children as the - # preview will need this - index = 1 - for child in containing_subsection.get_children(): - if child.location == item.location: - break - index = index + 1 + # need to figure out where this item is in the list of children as the + # preview will need this + index = 1 + for child in containing_subsection.get_children(): + if child.location == item.location: + break + index = index + 1 - preview_lms_base = settings.MITX_FEATURES.get('PREVIEW_LMS_BASE') + preview_lms_base = settings.MITX_FEATURES.get('PREVIEW_LMS_BASE') - preview_lms_link = ( + preview_lms_link = ( '//{preview_lms_base}/courses/{org}/{course}/' '{course_name}/courseware/{section}/{subsection}/{index}' ).format( @@ -299,102 +268,46 @@ def edit_unit(request, location): index=index ) - return render_to_response('unit.html', { - 'context_course': course, - 'unit': item, - # Still needed for creating a draft. - 'unit_location': location, - 'unit_locator': unit_locator, - 'components': components, - 'component_templates': component_templates, - 'draft_preview_link': preview_lms_link, - 'published_preview_link': lms_link, - 'subsection': containing_subsection, - 'release_date': ( - get_default_time_display(containing_subsection.start) - if containing_subsection.start is not None else None - ), - 'section': containing_section, - 'new_unit_category': 'vertical', - 'unit_state': compute_unit_state(item), - 'published_date': ( - get_default_time_display(item.published_date) - if item.published_date is not None else None - ), - }) - - -@expect_json -@login_required -@require_http_methods(("GET", "POST", "PUT")) -@ensure_csrf_cookie -def assignment_type_update(request, org, course, category, name): - """ - CRUD operations on assignment types for sections and subsections and - anything else gradable. - """ - location = Location(['i4x', org, course, category, name]) - if not has_access(request.user, location): - return HttpResponseForbidden() - - if request.method == 'GET': - rsp = CourseGradingModel.get_section_grader_type(location) - elif request.method in ('POST', 'PUT'): # post or put, doesn't matter. - rsp = CourseGradingModel.update_section_grader_type( - location, request.json - ) - return JsonResponse(rsp) - - -@login_required -@expect_json -def create_draft(request): - "Create a draft" - location = request.json['id'] - - # check permissions for this user within this course - if not has_access(request.user, location): - raise PermissionDenied() - - # This clones the existing item location to a draft location (the draft is - # implicit, because modulestore is a Draft modulestore) - modulestore().convert_to_draft(location) - - return HttpResponse() + return render_to_response('unit.html', { + 'context_course': course, + 'unit': item, + 'unit_locator': locator, + 'components': components, + 'component_templates': component_templates, + 'draft_preview_link': preview_lms_link, + 'published_preview_link': lms_link, + 'subsection': containing_subsection, + 'release_date': ( + get_default_time_display(containing_subsection.start) + if containing_subsection.start is not None else None + ), + 'section': containing_section, + 'new_unit_category': 'vertical', + 'unit_state': compute_unit_state(item), + 'published_date': ( + get_default_time_display(item.published_date) + if item.published_date is not None else None + ), + }) + else: + return HttpResponseBadRequest("Only supports html requests") @login_required -@expect_json -def publish_draft(request): - """ - Publish a draft +def _get_item_in_course(request, locator): """ - location = request.json['id'] - - # check permissions for this user within this course - if not has_access(request.user, location): - raise PermissionDenied() - - item = modulestore().get_item(location) - _xmodule_recurse( - item, - lambda i: modulestore().publish(i.location, request.user.id) - ) + Helper method for getting the old location, containing course, + item, and lms_link for a given locator. - return HttpResponse() - - -@login_required -@expect_json -def unpublish_unit(request): - "Unpublish a unit" - location = request.json['id'] - - # check permissions for this user within this course - if not has_access(request.user, location): + Verifies that the caller has permission to access this item. + """ + if not has_access(request.user, locator): raise PermissionDenied() - item = modulestore().get_item(location) - _xmodule_recurse(item, lambda i: modulestore().unpublish(i.location)) + old_location = loc_mapper().translate_locator_to_location(locator) + course_location = loc_mapper().translate_locator_to_location(locator, True) + course = modulestore().get_item(course_location) + item = modulestore().get_item(old_location, depth=1) + lms_link = get_lms_link_for_item(old_location, course_id=course.location.course_id) - return HttpResponse() + return old_location, course, item, lms_link diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 187ee9343b38..c7e379869b30 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -27,13 +27,11 @@ ItemNotFoundError, InvalidLocationError) from xmodule.modulestore import Location -from contentstore.course_info_model import ( - get_course_updates, update_course_updates, delete_course_update) +from contentstore.course_info_model import get_course_updates, update_course_updates, delete_course_update from contentstore.utils import ( get_lms_link_for_item, add_extra_panel_tab, remove_extra_panel_tab, get_modulestore) -from models.settings.course_details import ( - CourseDetails, CourseSettingsEncoder) +from models.settings.course_details import CourseDetails, CourseSettingsEncoder from models.settings.course_grading import CourseGradingModel from models.settings.course_metadata import CourseMetadata @@ -53,14 +51,13 @@ from xmodule.html_module import AboutDescriptor from xmodule.modulestore.locator import BlockUsageLocator from course_creators.views import get_course_creator_status, add_user_with_status_unrequested +from contentstore import utils __all__ = ['course_info_handler', 'course_handler', 'course_info_update_handler', - 'get_course_settings', - 'course_config_graders_page', - 'course_config_advanced_page', - 'course_settings_updates', - 'course_grader_updates', - 'course_advanced_updates', 'textbook_index', 'textbook_by_id', + 'settings_handler', + 'grading_handler', + 'advanced_settings_handler', + 'textbook_index', 'textbook_by_id', 'create_textbook'] @@ -177,7 +174,6 @@ def course_index(request, course_id, branch, version_guid, block): if not has_access(request.user, location): raise PermissionDenied() - old_location = loc_mapper().translate_locator_to_location(location) lms_link = get_lms_link_for_item(old_location) @@ -190,10 +186,8 @@ def course_index(request, course_id, branch, version_guid, block): 'lms_link': lms_link, 'sections': sections, 'course_graders': json.dumps( - CourseGradingModel.fetch(course.location).graders + CourseGradingModel.fetch(location).graders ), - # This is used by course grader, which has not yet been updated. - 'parent_location': course.location, 'parent_locator': location, 'new_section_category': 'chapter', 'new_subsection_category': 'sequential', @@ -232,14 +226,20 @@ def create_new_course(request): pass if existing_course is not None: return JsonResponse({ - 'ErrMsg': _('There is already a course defined with the same ' + 'ErrMsg': _( + 'There is already a course defined with the same ' 'organization, course number, and course run. Please ' 'change either organization or course number to be ' - 'unique.'), - 'OrgErrMsg': _('Please change either the organization or ' - 'course number so that it is unique.'), - 'CourseErrMsg': _('Please change either the organization or ' - 'course number so that it is unique.'), + 'unique.' + ), + 'OrgErrMsg': _( + 'Please change either the organization or ' + 'course number so that it is unique.' + ), + 'CourseErrMsg': _( + 'Please change either the organization or ' + 'course number so that it is unique.' + ), }) # dhm: this query breaks the abstraction, but I'll fix it when I do my suspended refactoring of this @@ -254,12 +254,15 @@ def create_new_course(request): courses = modulestore().collection.find(course_search_location, fields=('_id')) if courses.count() > 0: return JsonResponse({ - 'ErrMsg': _('There is already a course defined with the same ' + 'ErrMsg': _( + 'There is already a course defined with the same ' 'organization and course number. Please ' 'change at least one field to be unique.'), - 'OrgErrMsg': _('Please change either the organization or ' + 'OrgErrMsg': _( + 'Please change either the organization or ' 'course number so that it is unique.'), - 'CourseErrMsg': _('Please change either the organization or ' + 'CourseErrMsg': _( + 'Please change either the organization or ' 'course number so that it is unique.'), }) @@ -289,7 +292,8 @@ def create_new_course(request): initialize_course_tabs(new_course) - create_all_course_groups(request.user, new_course.location) + new_location = loc_mapper().translate_location(new_course.location.course_id, new_course.location, False, True) + create_all_course_groups(request.user, new_location) # seed the forums seed_permissions_roles(new_course.location.course_id) @@ -298,7 +302,6 @@ def create_new_course(request): # work. CourseEnrollment.enroll(request.user, new_course.location.course_id) - new_location = loc_mapper().translate_location(new_course.location.course_id, new_course.location, False, True) return JsonResponse({'url': new_location.url_reverse("course/", "")}) @@ -347,9 +350,8 @@ def course_info_handler(request, tag=None, course_id=None, branch=None, version_ @ensure_csrf_cookie @require_http_methods(("GET", "POST", "PUT", "DELETE")) @expect_json -def course_info_update_handler( - request, tag=None, course_id=None, branch=None, version_guid=None, block=None, provided_id=None - ): +def course_info_update_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None, + provided_id=None): """ restful CRUD operations on course_info updates. provided_id should be none if it's new (create) and index otherwise. @@ -394,232 +396,206 @@ def course_info_update_handler( @login_required @ensure_csrf_cookie -def get_course_settings(request, org, course, name): - """ - Send models and views as well as html for editing the course settings to - the client. - - org, course, name: Attributes of the Location for the item to edit +@require_http_methods(("GET", "PUT", "POST")) +@expect_json +def settings_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): """ - location = get_location_and_verify_access(request, org, course, name) - - course_module = modulestore().get_item(location) - - new_loc = loc_mapper().translate_location(location.course_id, location, False, True) - upload_asset_url = new_loc.url_reverse('assets/', '') - - return render_to_response('settings.html', { - 'context_course': course_module, - 'course_location': location, - 'details_url': reverse(course_settings_updates, - kwargs={"org": org, - "course": course, - "name": name, - "section": "details"}), - 'about_page_editable': not settings.MITX_FEATURES.get( - 'ENABLE_MKTG_SITE', False - ), - 'upload_asset_url': upload_asset_url - }) - - -@login_required -@ensure_csrf_cookie -def course_config_graders_page(request, org, course, name): + Course settings for dates and about pages + GET + html: get the page + json: get the CourseDetails model + PUT + json: update the Course and About xblocks through the CourseDetails model """ - Send models and views as well as html for editing the course settings to - the client. + locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + if not has_access(request.user, locator): + raise PermissionDenied() - org, course, name: Attributes of the Location for the item to edit - """ - location = get_location_and_verify_access(request, org, course, name) + if 'text/html' in request.META.get('HTTP_ACCEPT', '') and request.method == 'GET': + course_old_location = loc_mapper().translate_locator_to_location(locator) + course_module = modulestore().get_item(course_old_location) - course_module = modulestore().get_item(location) - course_details = CourseGradingModel.fetch(location) + upload_asset_url = locator.url_reverse('assets/') - return render_to_response('settings_graders.html', { - 'context_course': course_module, - 'course_location': location, - 'course_details': json.dumps(course_details, cls=CourseSettingsEncoder) - }) + return render_to_response('settings.html', { + 'context_course': course_module, + 'course_locator': locator, + 'lms_link_for_about_page': utils.get_lms_link_for_about_page(course_old_location), + 'course_image_url': utils.course_image_url(course_module), + 'details_url': locator.url_reverse('/settings/details/'), + 'about_page_editable': not settings.MITX_FEATURES.get( + 'ENABLE_MKTG_SITE', False + ), + 'upload_asset_url': upload_asset_url + }) + elif 'application/json' in request.META.get('HTTP_ACCEPT', ''): + if request.method == 'GET': + return JsonResponse( + CourseDetails.fetch(locator), + # encoder serializes dates, old locations, and instances + encoder=CourseSettingsEncoder + ) + else: # post or put, doesn't matter. + return JsonResponse( + CourseDetails.update_from_json(locator, request.json), + encoder=CourseSettingsEncoder + ) @login_required @ensure_csrf_cookie -def course_config_advanced_page(request, org, course, name): - """ - Send models and views as well as html for editing the advanced course - settings to the client. - - org, course, name: Attributes of the Location for the item to edit - """ - location = get_location_and_verify_access(request, org, course, name) - - course_module = modulestore().get_item(location) - - return render_to_response('settings_advanced.html', { - 'context_course': course_module, - 'course_location': location, - 'advanced_dict': json.dumps(CourseMetadata.fetch(location)), - }) - - +@require_http_methods(("GET", "POST", "PUT", "DELETE")) @expect_json -@login_required -@ensure_csrf_cookie -def course_settings_updates(request, org, course, name, section): +def grading_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None, grader_index=None): """ - Restful CRUD operations on course settings. This differs from - get_course_settings by communicating purely through json (not rendering any - html) and handles section level operations rather than whole page. - - org, course: Attributes of the Location for the item to edit - section: one of details, faculty, grading, problems, discussions + Course Grading policy configuration + GET + html: get the page + json no grader_index: get the CourseGrading model (graceperiod, cutoffs, and graders) + json w/ grader_index: get the specific grader + PUT + json no grader_index: update the Course through the CourseGrading model + json w/ grader_index: create or update the specific grader (create if index out of range) """ - get_location_and_verify_access(request, org, course, name) - - if section == 'details': - manager = CourseDetails - elif section == 'grading': - manager = CourseGradingModel - else: - return - - if request.method == 'GET': - # Cannot just do a get w/o knowing the course name :-( - return JsonResponse( - manager.fetch(Location(['i4x', org, course, 'course', name])), - encoder=CourseSettingsEncoder - ) - elif request.method in ('POST', 'PUT'): # post or put, doesn't matter. - return JsonResponse( - manager.update_from_json(request.json), - encoder=CourseSettingsEncoder - ) - + locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + if not has_access(request.user, locator): + raise PermissionDenied() -@expect_json -@require_http_methods(("GET", "POST", "PUT", "DELETE")) -@login_required -@ensure_csrf_cookie -def course_grader_updates(request, org, course, name, grader_index=None): - """ - Restful CRUD operations on course_info updates. This differs from - get_course_settings by communicating purely through json (not rendering any - html) and handles section level operations rather than whole page. + if 'text/html' in request.META.get('HTTP_ACCEPT', '') and request.method == 'GET': + course_old_location = loc_mapper().translate_locator_to_location(locator) + course_module = modulestore().get_item(course_old_location) + course_details = CourseGradingModel.fetch(locator) - org, course: Attributes of the Location for the item to edit - """ + return render_to_response('settings_graders.html', { + 'context_course': course_module, + 'course_locator': locator, + 'course_details': json.dumps(course_details, cls=CourseSettingsEncoder), + 'grading_url': locator.url_reverse('/settings/grading/'), + }) + elif 'application/json' in request.META.get('HTTP_ACCEPT', ''): + if request.method == 'GET': + if grader_index is None: + return JsonResponse( + CourseGradingModel.fetch(locator), + # encoder serializes dates, old locations, and instances + encoder=CourseSettingsEncoder + ) + else: + return JsonResponse(CourseGradingModel.fetch_grader(locator, grader_index)) + elif request.method in ('POST', 'PUT'): # post or put, doesn't matter. + # None implies update the whole model (cutoffs, graceperiod, and graders) not a specific grader + if grader_index is None: + return JsonResponse( + CourseGradingModel.update_from_json(locator, request.json), + encoder=CourseSettingsEncoder + ) + else: + return JsonResponse( + CourseGradingModel.update_grader_from_json(locator, request.json) + ) + elif request.method == "DELETE" and grader_index is not None: + CourseGradingModel.delete_grader(locator, grader_index) + return JsonResponse() + + +# pylint: disable=invalid-name +def _config_course_advanced_components(request, course_module): + """ + Check to see if the user instantiated any advanced components. This + is a hack that does the following : + 1) adds/removes the open ended panel tab to a course automatically + if the user has indicated that they want to edit the + combinedopendended or peergrading module + 2) adds/removes the notes panel tab to a course automatically if + the user has indicated that they want the notes module enabled in + their course + """ + # TODO refactor the above into distinct advanced policy settings + filter_tabs = True # Exceptional conditions will pull this to False + if ADVANCED_COMPONENT_POLICY_KEY in request.json: # Maps tab types to components + tab_component_map = { + 'open_ended':OPEN_ENDED_COMPONENT_TYPES, + 'notes':NOTE_COMPONENT_TYPES, + } + # Check to see if the user instantiated any notes or open ended + # components + for tab_type in tab_component_map.keys(): + component_types = tab_component_map.get(tab_type) + found_ac_type = False + for ac_type in component_types: + if ac_type in request.json[ADVANCED_COMPONENT_POLICY_KEY]: + # Add tab to the course if needed + changed, new_tabs = add_extra_panel_tab(tab_type, course_module) + # If a tab has been added to the course, then send the + # metadata along to CourseMetadata.update_from_json + if changed: + course_module.tabs = new_tabs + request.json.update({'tabs': new_tabs}) + # Indicate that tabs should not be filtered out of + # the metadata + filter_tabs = False # Set this flag to avoid the tab removal code below. + found_ac_type = True #break - location = get_location_and_verify_access(request, org, course, name) + # If we did not find a module type in the advanced settings, + # we may need to remove the tab from the course. + if not found_ac_type: # Remove tab from the course if needed + changed, new_tabs = remove_extra_panel_tab(tab_type, course_module) + if changed: + course_module.tabs = new_tabs + request.json.update({'tabs':new_tabs}) + # Indicate that tabs should *not* be filtered out of + # the metadata + filter_tabs = False - if request.method == 'GET': - # Cannot just do a get w/o knowing the course name :-( - return JsonResponse(CourseGradingModel.fetch_grader( - Location(location), grader_index - )) - elif request.method == "DELETE": - # ??? Should this return anything? Perhaps success fail? - CourseGradingModel.delete_grader(Location(location), grader_index) - return JsonResponse() - else: # post or put, doesn't matter. - return JsonResponse(CourseGradingModel.update_grader_from_json( - Location(location), - request.json - )) + return filter_tabs -@require_http_methods(("GET", "POST", "PUT", "DELETE")) @login_required @ensure_csrf_cookie +@require_http_methods(("GET", "POST", "PUT")) @expect_json -def course_advanced_updates(request, org, course, name): +def advanced_settings_handler(request, course_id=None, branch=None, version_guid=None, block=None, tag=None): """ - Restful CRUD operations on metadata. The payload is a json rep of the - metadata dicts. For delete, otoh, the payload is either a key or a list of - keys to delete. + Course settings configuration + GET + html: get the page + json: get the model + PUT, POST + json: update the Course's settings. The payload is a json rep of the + metadata dicts. The dict can include a "unsetKeys" entry which is a list + of keys whose values to unset: i.e., revert to default + """ + locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + if not has_access(request.user, locator): + raise PermissionDenied() - org, course: Attributes of the Location for the item to edit - """ - location = get_location_and_verify_access(request, org, course, name) + course_old_location = loc_mapper().translate_locator_to_location(locator) + course_module = modulestore().get_item(course_old_location) - if request.method == 'GET': - return JsonResponse(CourseMetadata.fetch(location)) - elif request.method == 'DELETE': - return JsonResponse(CourseMetadata.delete_key( - location, - json.loads(request.body) - )) - else: - # Whether or not to filter the tabs key out of the settings metadata - filter_tabs = True - - # Check to see if the user instantiated any advanced components. This - # is a hack that does the following : - # 1) adds/removes the open ended panel tab to a course automatically - # if the user has indicated that they want to edit the - # combinedopendended or peergrading module - # 2) adds/removes the notes panel tab to a course automatically if - # the user has indicated that they want the notes module enabled in - # their course - # TODO refactor the above into distinct advanced policy settings - if ADVANCED_COMPONENT_POLICY_KEY in request.json: - # Get the course so that we can scrape current tabs - course_module = modulestore().get_item(location) - - # Maps tab types to components - tab_component_map = { - 'open_ended': OPEN_ENDED_COMPONENT_TYPES, - 'notes': NOTE_COMPONENT_TYPES, - } + if 'text/html' in request.META.get('HTTP_ACCEPT', '') and request.method == 'GET': - # Check to see if the user instantiated any notes or open ended - # components - for tab_type in tab_component_map.keys(): - component_types = tab_component_map.get(tab_type) - found_ac_type = False - for ac_type in component_types: - if ac_type in request.json[ADVANCED_COMPONENT_POLICY_KEY]: - # Add tab to the course if needed - changed, new_tabs = add_extra_panel_tab( - tab_type, - course_module - ) - # If a tab has been added to the course, then send the - # metadata along to CourseMetadata.update_from_json - if changed: - course_module.tabs = new_tabs - request.json.update({'tabs': new_tabs}) - # Indicate that tabs should not be filtered out of - # the metadata - filter_tabs = False - # Set this flag to avoid the tab removal code below. - found_ac_type = True - break - # If we did not find a module type in the advanced settings, - # we may need to remove the tab from the course. - if not found_ac_type: - # Remove tab from the course if needed - changed, new_tabs = remove_extra_panel_tab( - tab_type, course_module - ) - if changed: - course_module.tabs = new_tabs - request.json.update({'tabs': new_tabs}) - # Indicate that tabs should *not* be filtered out of - # the metadata - filter_tabs = False - try: - return JsonResponse(CourseMetadata.update_from_json( - location, - request.json, - filter_tabs=filter_tabs - )) - except (TypeError, ValueError) as err: - return HttpResponseBadRequest( - "Incorrect setting format. " + str(err), - content_type="text/plain" - ) + return render_to_response('settings_advanced.html', { + 'context_course': course_module, + 'advanced_dict': json.dumps(CourseMetadata.fetch(course_module)), + 'advanced_settings_url': locator.url_reverse('settings/advanced') + }) + elif 'application/json' in request.META.get('HTTP_ACCEPT', ''): + if request.method == 'GET': + return JsonResponse(CourseMetadata.fetch(course_module)) + else: + # Whether or not to filter the tabs key out of the settings metadata + filter_tabs = _config_course_advanced_components(request, course_module) + try: + return JsonResponse(CourseMetadata.update_from_json( + course_module, + request.json, + filter_tabs=filter_tabs + )) + except (TypeError, ValueError) as err: + return HttpResponseBadRequest( + "Incorrect setting format. {}".format(err), + content_type="text/plain" + ) class TextbookValidationError(Exception): diff --git a/cms/djangoapps/contentstore/views/import_export.py b/cms/djangoapps/contentstore/views/import_export.py index 5d1b26ec3dfb..f740d10707e5 100644 --- a/cms/djangoapps/contentstore/views/import_export.py +++ b/cms/djangoapps/contentstore/views/import_export.py @@ -14,7 +14,6 @@ from django.http import HttpResponse from django.contrib.auth.decorators import login_required from django_future.csrf import ensure_csrf_cookie -from django.core.urlresolvers import reverse from django.core.servers.basehttp import FileWrapper from django.core.files.temp import NamedTemporaryFile from django.core.exceptions import SuspiciousOperation, PermissionDenied @@ -140,7 +139,7 @@ def import_handler(request, tag=None, course_id=None, branch=None, version_guid= "size": size, "deleteUrl": "", "deleteType": "", - "url": location.url_reverse('import/', ''), + "url": location.url_reverse('import'), "thumbnailUrl": "" }] }) @@ -252,8 +251,8 @@ def get_dir_for_fname(directory, filename): course_module = modulestore().get_item(old_location) return render_to_response('import.html', { 'context_course': course_module, - 'successful_import_redirect_url': location.url_reverse("course/", ""), - 'import_status_url': location.url_reverse("import_status/", "fillerName"), + 'successful_import_redirect_url': location.url_reverse("course"), + 'import_status_url': location.url_reverse("import_status", "fillerName"), }) else: return HttpResponseNotFound() @@ -313,7 +312,7 @@ def export_handler(request, tag=None, course_id=None, branch=None, version_guid= # an _accept URL parameter will be preferred over HTTP_ACCEPT in the header. requested_format = request.REQUEST.get('_accept', request.META.get('HTTP_ACCEPT', 'text/html')) - export_url = location.url_reverse('export/', '') + '?_accept=application/x-tgz' + export_url = location.url_reverse('export') + '?_accept=application/x-tgz' if 'application/x-tgz' in requested_format: name = old_location.name export_file = NamedTemporaryFile(prefix=name + '.', suffix=".tar.gz") @@ -339,16 +338,16 @@ def export_handler(request, tag=None, course_id=None, branch=None, version_guid= # if we have a nested exception, then we'll show the more generic error message pass + unit_locator = loc_mapper().translate_location(old_location.course_id, parent.location, False, True) + return render_to_response('export.html', { 'context_course': course_module, 'in_err': True, 'raw_err_msg': str(e), 'failed_module': failed_item, 'unit': unit, - 'edit_unit_url': reverse('edit_unit', kwargs={ - 'location': parent.location - }) if parent else '', - 'course_home_url': location.url_reverse("course/", ""), + 'edit_unit_url': unit_locator.url_reverse("unit") if parent else "", + 'course_home_url': location.url_reverse("course"), 'export_url': export_url }) @@ -359,7 +358,7 @@ def export_handler(request, tag=None, course_id=None, branch=None, version_guid= 'in_err': True, 'unit': None, 'raw_err_msg': str(e), - 'course_home_url': location.url_reverse("course/", ""), + 'course_home_url': location.url_reverse("course"), 'export_url': export_url }) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 97bfde3b8285..220da038a781 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -3,7 +3,9 @@ import logging from uuid import uuid4 +from functools import partial from static_replace import replace_static_urls +from xmodule_modifiers import wrap_xblock from django.core.exceptions import PermissionDenied from django.contrib.auth.decorators import login_required @@ -27,6 +29,9 @@ from student.models import CourseEnrollment from django.http import HttpResponseBadRequest from xblock.fields import Scope +from preview import handler_prefix, get_preview_html +from mitxmako.shortcuts import render_to_response, render_to_string +from models.settings.course_grading import CourseGradingModel __all__ = ['orphan_handler', 'xblock_handler'] @@ -51,17 +56,21 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid= all children and "all_versions" to delete from all (mongo) versions. GET json: returns representation of the xblock (locator id, data, and metadata). + if ?fields=graderType, it returns the graderType for the unit instead of the above. + html: returns HTML for rendering the xblock (which includes both the "preview" view and the "editor" view) PUT or POST - json: if xblock location is specified, update the xblock instance. The json payload can contain + json: if xblock locator is specified, update the xblock instance. The json payload can contain these fields, all optional: :data: the new value for the data. :children: the locator ids of children for this xblock. :metadata: new values for the metadata fields. Any whose values are None will be deleted not set to None! Absent ones will be left alone. :nullout: which metadata fields to set to None + :graderType: change how this unit is graded + :publish: can be one of three values, 'make_public, 'make_private', or 'create_draft' The JSON representation on the updated xblock (minus children) is returned. - if xblock location is not specified, create a new xblock instance. The json playload can contain + if xblock locator is not specified, create a new xblock instance. The json playload can contain these fields: :parent_locator: parent for new xblock, required :category: type of xblock, required @@ -70,14 +79,38 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid= The locator (and old-style id) for the created xblock (minus children) is returned. """ if course_id is not None: - location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) - if not has_access(request.user, location): + locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + if not has_access(request.user, locator): raise PermissionDenied() - old_location = loc_mapper().translate_locator_to_location(location) + old_location = loc_mapper().translate_locator_to_location(locator) if request.method == 'GET': - rsp = _get_module_info(location) - return JsonResponse(rsp) + if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): + fields = request.REQUEST.get('fields', '').split(',') + if 'graderType' in fields: + # right now can't combine output of this w/ output of _get_module_info, but worthy goal + return JsonResponse(CourseGradingModel.get_section_grader_type(locator)) + # TODO: pass fields to _get_module_info and only return those + rsp = _get_module_info(locator) + return JsonResponse(rsp) + else: + component = modulestore().get_item(old_location) + # Wrap the generated fragment in the xmodule_editor div so that the javascript + # can bind to it correctly + component.runtime.wrappers.append(partial(wrap_xblock, handler_prefix)) + + try: + content = component.render('studio_view').content + # catch exceptions indiscriminately, since after this point they escape the + # dungeon and surface as uneditable, unsaveable, and undeletable + # component-goblins. + except Exception as exc: # pylint: disable=W0703 + content = render_to_string('html_error.html', {'message': str(exc)}) + + return render_to_response('component.html', { + 'preview': get_preview_html(request, component), + 'editor': content + }) elif request.method == 'DELETE': delete_children = str_to_bool(request.REQUEST.get('recurse', 'False')) delete_all_versions = str_to_bool(request.REQUEST.get('all_versions', 'False')) @@ -85,12 +118,15 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid= return _delete_item_at_location(old_location, delete_children, delete_all_versions) else: # Since we have a course_id, we are updating an existing xblock. return _save_item( - location, + request, + locator, old_location, data=request.json.get('data'), children=request.json.get('children'), metadata=request.json.get('metadata'), - nullout=request.json.get('nullout') + nullout=request.json.get('nullout'), + grader_type=request.json.get('graderType'), + publish=request.json.get('publish'), ) elif request.method in ('PUT', 'POST'): return _create_item(request) @@ -101,11 +137,14 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid= ) -def _save_item(usage_loc, item_location, data=None, children=None, metadata=None, nullout=None): +def _save_item(request, usage_loc, item_location, data=None, children=None, metadata=None, nullout=None, + grader_type=None, publish=None): """ - Saves certain properties (data, children, metadata, nullout) for a given xblock item. + Saves xblock w/ its fields. Has special processing for grader_type, publish, and nullout and Nones in metadata. + nullout means to truly set the field to None whereas nones in metadata mean to unset them (so they revert + to default). - The item_location is still the old-style location. + The item_location is still the old-style location whereas usage_loc is a BlockUsageLocator """ store = get_modulestore(item_location) @@ -123,6 +162,14 @@ def _save_item(usage_loc, item_location, data=None, children=None, metadata=None log.error("Can't find item by location.") return JsonResponse({"error": "Can't find item by location: " + str(item_location)}, 404) + if publish: + if publish == 'make_private': + _xmodule_recurse(existing_item, lambda i: modulestore().unpublish(i.location)) + elif publish == 'create_draft': + # This clones the existing item location to a draft location (the draft is + # implicit, because modulestore is a Draft modulestore) + modulestore().convert_to_draft(item_location) + if data: store.update_item(item_location, data) else: @@ -170,12 +217,25 @@ def _save_item(usage_loc, item_location, data=None, children=None, metadata=None if existing_item.category == 'video': manage_video_subtitles_save(existing_item, existing_item) - # Note that children aren't being returned until we have a use case. - return JsonResponse({ + result = { 'id': unicode(usage_loc), 'data': data, 'metadata': own_metadata(existing_item) - }) + } + + if grader_type is not None: + result.update(CourseGradingModel.update_section_grader_type(existing_item, grader_type)) + + # Make public after updating the xblock, in case the caller asked + # for both an update and a publish. + if publish and publish == 'make_public': + _xmodule_recurse( + existing_item, + lambda i: modulestore().publish(i.location, request.user.id) + ) + + # Note that children aren't being returned until we have a use case. + return JsonResponse(result) @login_required @@ -192,10 +252,7 @@ def _create_item(request): raise PermissionDenied() parent = get_modulestore(category).get_item(parent_location) - # Necessary to set revision=None or else metadata inheritance does not work - # (the ID with @draft will be used as the key in the inherited metadata map, - # and that is not expected by the code that later references it). - dest_location = parent_location.replace(category=category, name=uuid4().hex, revision=None) + dest_location = parent_location.replace(category=category, name=uuid4().hex) # get the metadata, display_name, and definition from the request metadata = {} @@ -224,7 +281,7 @@ def _create_item(request): course_location = loc_mapper().translate_locator_to_location(parent_locator, get_course=True) locator = loc_mapper().translate_location(course_location.course_id, dest_location, False, True) - return JsonResponse({'id': dest_location.url(), "locator": unicode(locator)}) + return JsonResponse({"locator": unicode(locator)}) def _delete_item_at_location(item_location, delete_children=False, delete_all_versions=False): diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index ab1375554d60..123d7fbadbdd 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -3,7 +3,7 @@ from django.conf import settings from django.core.urlresolvers import reverse -from django.http import Http404, HttpResponseBadRequest, HttpResponseForbidden +from django.http import Http404, HttpResponseBadRequest from django.contrib.auth.decorators import login_required from mitxmako.shortcuts import render_to_response, render_to_string @@ -24,10 +24,9 @@ import static_replace from .session_kv_store import SessionKeyValueStore from .helpers import render_from_lms -from .access import has_access from ..utils import get_course_for_item -__all__ = ['preview_handler', 'preview_component'] +__all__ = ['preview_handler'] log = logging.getLogger(__name__) @@ -53,13 +52,13 @@ def preview_handler(request, usage_id, handler, suffix=''): usage_id: The usage-id of the block to dispatch to, passed through `quote_slashes` handler: The handler to execute - suffix: The remaineder of the url to be passed to the handler + suffix: The remainder of the url to be passed to the handler """ location = unquote_slashes(usage_id) descriptor = modulestore().get_item(location) - instance = load_preview_module(request, descriptor) + instance = _load_preview_module(request, descriptor) # Let the module handle the AJAX req = django_to_webob_request(request) try: @@ -85,32 +84,6 @@ def preview_handler(request, usage_id, handler, suffix=''): return webob_to_django_response(resp) -@login_required -def preview_component(request, location): - "Return the HTML preview of a component" - # TODO (vshnayder): change name from id to location in coffee+html as well. - if not has_access(request.user, location): - return HttpResponseForbidden() - - component = modulestore().get_item(location) - # Wrap the generated fragment in the xmodule_editor div so that the javascript - # can bind to it correctly - component.runtime.wrappers.append(partial(wrap_xblock, handler_prefix)) - - try: - content = component.render('studio_view').content - # catch exceptions indiscriminately, since after this point they escape the - # dungeon and surface as uneditable, unsaveable, and undeletable - # component-goblins. - except Exception as exc: # pylint: disable=W0703 - content = render_to_string('html_error.html', {'message': str(exc)}) - - return render_to_response('component.html', { - 'preview': get_preview_html(request, component), - 'editor': content - }) - - class PreviewModuleSystem(ModuleSystem): # pylint: disable=abstract-method """ An XModule ModuleSystem for use in Studio previews @@ -119,7 +92,7 @@ def handler_url(self, block, handler_name, suffix='', query='', thirdparty=False return handler_prefix(block, handler_name, suffix) + '?' + query -def preview_module_system(request, descriptor): +def _preview_module_system(request, descriptor): """ Returns a ModuleSystem for the specified descriptor that is specialized for rendering module previews. @@ -135,7 +108,7 @@ def preview_module_system(request, descriptor): # TODO (cpennington): Do we want to track how instructors are using the preview problems? track_function=lambda event_type, event: None, filestore=descriptor.runtime.resources_fs, - get_module=partial(load_preview_module, request), + get_module=partial(_load_preview_module, request), render_template=render_from_lms, debug=True, replace_urls=partial(static_replace.replace_static_urls, data_directory=None, course_id=course_id), @@ -162,7 +135,7 @@ def preview_module_system(request, descriptor): ) -def load_preview_module(request, descriptor): +def _load_preview_module(request, descriptor): """ Return a preview XModule instantiated from the supplied descriptor. @@ -171,7 +144,7 @@ def load_preview_module(request, descriptor): """ student_data = DbModel(SessionKeyValueStore(request)) descriptor.bind_for_student( - preview_module_system(request, descriptor), + _preview_module_system(request, descriptor), LmsFieldData(descriptor._field_data, student_data), # pylint: disable=protected-access ) return descriptor @@ -182,7 +155,7 @@ def get_preview_html(request, descriptor): Returns the HTML returned by the XModule's student_view, specified by the descriptor and idx. """ - module = load_preview_module(request, descriptor) + module = _load_preview_module(request, descriptor) try: content = module.render("student_view").content except Exception as exc: # pylint: disable=W0703 diff --git a/cms/djangoapps/contentstore/views/public.py b/cms/djangoapps/contentstore/views/public.py index de0a1899b3ac..9ab03a409331 100644 --- a/cms/djangoapps/contentstore/views/public.py +++ b/cms/djangoapps/contentstore/views/public.py @@ -10,7 +10,7 @@ from external_auth.views import ssl_login_shortcut -__all__ = ['signup', 'old_login_redirect', 'login_page', 'howitworks'] +__all__ = ['signup', 'login_page', 'howitworks'] @ensure_csrf_cookie @@ -22,13 +22,6 @@ def signup(request): return render_to_response('signup.html', {'csrf': csrf_token}) -def old_login_redirect(request): - ''' - Redirect to the active login url. - ''' - return redirect('login', permanent=True) - - @ssl_login_shortcut @ensure_csrf_cookie def login_page(request): diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index 277445e3b929..46791ddc26f3 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -2,12 +2,13 @@ Views related to course tabs """ from access import has_access -from util.json_request import expect_json +from util.json_request import expect_json, JsonResponse -from django.http import HttpResponse, HttpResponseBadRequest +from django.http import HttpResponseNotFound from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied from django_future.csrf import ensure_csrf_cookie +from django.views.decorators.http import require_http_methods from mitxmako.shortcuts import render_to_response from xmodule.modulestore import Location from xmodule.modulestore.inheritance import own_metadata @@ -19,7 +20,7 @@ from django.utils.translation import ugettext as _ -__all__ = ['edit_tabs', 'reorder_static_tabs'] +__all__ = ['tabs_handler'] def initialize_course_tabs(course): @@ -43,107 +44,113 @@ def initialize_course_tabs(course): modulestore('direct').update_metadata(course.location.url(), own_metadata(course)) - -@login_required @expect_json -def reorder_static_tabs(request): - "Order the static tabs in the requested order" - def get_location_for_tab(tab): - tab_locator = BlockUsageLocator(tab) - return loc_mapper().translate_locator_to_location(tab_locator) - - tabs = request.json['tabs'] - course_location = loc_mapper().translate_locator_to_location(BlockUsageLocator(tabs[0]), get_course=True) - - if not has_access(request.user, course_location): - raise PermissionDenied() - - course = get_modulestore(course_location).get_item(course_location) - - # get list of existing static tabs in course - # make sure they are the same lengths (i.e. the number of passed in tabs equals the number - # that we know about) otherwise we can drop some! - - existing_static_tabs = [t for t in course.tabs if t['type'] == 'static_tab'] - if len(existing_static_tabs) != len(tabs): - return HttpResponseBadRequest() - - # load all reference tabs, return BadRequest if we can't find any of them - tab_items = [] - for tab in tabs: - item = modulestore('direct').get_item(get_location_for_tab(tab)) - if item is None: - return HttpResponseBadRequest() - - tab_items.append(item) - - # now just go through the existing course_tabs and re-order the static tabs - reordered_tabs = [] - static_tab_idx = 0 - for tab in course.tabs: - if tab['type'] == 'static_tab': - reordered_tabs.append({'type': 'static_tab', - 'name': tab_items[static_tab_idx].display_name, - 'url_slug': tab_items[static_tab_idx].location.name}) - static_tab_idx += 1 - else: - reordered_tabs.append(tab) - - # OK, re-assemble the static tabs in the new order - course.tabs = reordered_tabs - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - course.save() - modulestore('direct').update_metadata(course.location, own_metadata(course)) - # TODO: above two lines are used for the primitive-save case. Maybe factor them out? - return HttpResponse() - - @login_required @ensure_csrf_cookie -def edit_tabs(request, org, course, coursename): - "Edit tabs" - location = ['i4x', org, course, 'course', coursename] - store = get_modulestore(location) - course_item = store.get_item(location) - - # check that logged in user has permissions to this item - if not has_access(request.user, location): - raise PermissionDenied() +@require_http_methods(("GET", "POST", "PUT")) +def tabs_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): + """ + The restful handler for static tabs. - # see tabs have been uninitialized (e.g. supporing courses created before tab support in studio) - if course_item.tabs is None or len(course_item.tabs) == 0: - initialize_course_tabs(course_item) + GET + html: return page for editing static tabs + json: not supported + PUT or POST + json: update the tab order. It is expected that the request body contains a JSON-encoded dict with entry "tabs". + The value for "tabs" is an array of tab locators, indicating the desired order of the tabs. - # first get all static tabs from the tabs list - # we do this because this is also the order in which items are displayed in the LMS - static_tabs_refs = [t for t in course_item.tabs if t['type'] == 'static_tab'] + Creating a tab, deleting a tab, or changing its contents is not supported through this method. + Instead use the general xblock URL (see item.xblock_handler). + """ + locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + if not has_access(request.user, locator): + raise PermissionDenied() - static_tabs = [] - for static_tab_ref in static_tabs_refs: - static_tab_loc = Location(location)._replace(category='static_tab', name=static_tab_ref['url_slug']) - static_tabs.append(modulestore('direct').get_item(static_tab_loc)) + old_location = loc_mapper().translate_locator_to_location(locator) + store = get_modulestore(old_location) + course_item = store.get_item(old_location) - components = [ - [ - static_tab.location.url(), + if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): + if request.method == 'GET': + raise NotImplementedError('coming soon') + else: + if 'tabs' in request.json: + def get_location_for_tab(tab): + """ Returns the location (old-style) for a tab. """ + return loc_mapper().translate_locator_to_location(BlockUsageLocator(tab)) + + tabs = request.json['tabs'] + + # get list of existing static tabs in course + # make sure they are the same lengths (i.e. the number of passed in tabs equals the number + # that we know about) otherwise we will inadvertently drop some! + existing_static_tabs = [t for t in course_item.tabs if t['type'] == 'static_tab'] + if len(existing_static_tabs) != len(tabs): + return JsonResponse( + {"error": "number of tabs must be {}".format(len(existing_static_tabs))}, status=400 + ) + + # load all reference tabs, return BadRequest if we can't find any of them + tab_items = [] + for tab in tabs: + item = modulestore('direct').get_item(get_location_for_tab(tab)) + if item is None: + return JsonResponse( + {"error": "no tab for found location {}".format(tab)}, status=400 + ) + + tab_items.append(item) + + # now just go through the existing course_tabs and re-order the static tabs + reordered_tabs = [] + static_tab_idx = 0 + for tab in course_item.tabs: + if tab['type'] == 'static_tab': + reordered_tabs.append( + {'type': 'static_tab', + 'name': tab_items[static_tab_idx].display_name, + 'url_slug': tab_items[static_tab_idx].location.name, + } + ) + static_tab_idx += 1 + else: + reordered_tabs.append(tab) + + # OK, re-assemble the static tabs in the new order + course_item.tabs = reordered_tabs + modulestore('direct').update_metadata(course_item.location, own_metadata(course_item)) + return JsonResponse() + else: + raise NotImplementedError('Creating or changing tab content is not supported.') + elif request.method == 'GET': # assume html + # see tabs have been uninitialized (e.g. supporting courses created before tab support in studio) + if course_item.tabs is None or len(course_item.tabs) == 0: + initialize_course_tabs(course_item) + + # first get all static tabs from the tabs list + # we do this because this is also the order in which items are displayed in the LMS + static_tabs_refs = [t for t in course_item.tabs if t['type'] == 'static_tab'] + + static_tabs = [] + for static_tab_ref in static_tabs_refs: + static_tab_loc = old_location.replace(category='static_tab', name=static_tab_ref['url_slug']) + static_tabs.append(modulestore('direct').get_item(static_tab_loc)) + + components = [ loc_mapper().translate_location( course_item.location.course_id, static_tab.location, False, True ) + for static_tab + in static_tabs ] - for static_tab - in static_tabs - ] - course_locator = loc_mapper().translate_location( - course_item.location.course_id, course_item.location, False, True - ) - - return render_to_response('edit-tabs.html', { - 'context_course': course_item, - 'components': components, - 'locator': course_locator - }) + return render_to_response('edit-tabs.html', { + 'context_course': course_item, + 'components': components, + 'course_locator': locator + }) + else: + return HttpResponseNotFound() # "primitive" tab edit functions driven by the command line. @@ -167,7 +174,7 @@ def primitive_delete(course, num): # Note for future implementations: if you delete a static_tab, then Chris Dodge # points out that there's other stuff to delete beyond this element. # This code happens to not delete static_tab so it doesn't come up. - primitive_save(course) + modulestore('direct').update_metadata(course.location, own_metadata(course)) def primitive_insert(course, num, tab_type, name): @@ -176,11 +183,5 @@ def primitive_insert(course, num, tab_type, name): new_tab = {u'type': unicode(tab_type), u'name': unicode(name)} tabs = course.tabs tabs.insert(num, new_tab) - primitive_save(course) - - -def primitive_save(course): - "Saves the course back to modulestore." - # This code copied from reorder_static_tabs above - course.save() modulestore('direct').update_metadata(course.location, own_metadata(course)) + diff --git a/cms/djangoapps/contentstore/views/transcripts_ajax.py b/cms/djangoapps/contentstore/views/transcripts_ajax.py index 9db2cb9c1305..9d5cdf8e80f7 100644 --- a/cms/djangoapps/contentstore/views/transcripts_ajax.py +++ b/cms/djangoapps/contentstore/views/transcripts_ajax.py @@ -18,11 +18,12 @@ from xmodule.contentstore.content import StaticContent from xmodule.exceptions import NotFoundError -from xmodule.modulestore.django import modulestore +from xmodule.modulestore.django import modulestore, loc_mapper from xmodule.contentstore.django import contentstore -from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError +from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError from util.json_request import JsonResponse +from xmodule.modulestore.locator import BlockUsageLocator from ..transcripts_utils import ( generate_subs_from_source, @@ -77,20 +78,14 @@ def upload_transcripts(request): 'subs': '', } - item_location = request.POST.get('id') - if not item_location: - return error_response(response, 'POST data without "id" form data.') + locator = request.POST.get('locator') + if not locator: + return error_response(response, 'POST data without "locator" form data.') - # This is placed before has_access() to validate item_location, - # because has_access() raises InvalidLocationError if location is invalid. try: - item = modulestore().get_item(item_location) - except (ItemNotFoundError, InvalidLocationError): - return error_response(response, "Can't find item by location.") - - # Check permissions for this user within this course. - if not has_access(request.user, item_location): - raise PermissionDenied() + item = _get_item(request, request.POST) + except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError): + return error_response(response, "Can't find item by locator.") if 'file' not in request.FILES: return error_response(response, 'POST data without "file" form data.') @@ -156,23 +151,17 @@ def download_transcripts(request): Raises Http404 if unsuccessful. """ - item_location = request.GET.get('id') - if not item_location: - log.debug('GET data without "id" property.') + locator = request.GET.get('locator') + if not locator: + log.debug('GET data without "locator" property.') raise Http404 - # This is placed before has_access() to validate item_location, - # because has_access() raises InvalidLocationError if location is invalid. try: - item = modulestore().get_item(item_location) - except (ItemNotFoundError, InvalidLocationError): - log.debug("Can't find item by location.") + item = _get_item(request, request.GET) + except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError): + log.debug("Can't find item by locator.") raise Http404 - # Check permissions for this user within this course. - if not has_access(request.user, item_location): - raise PermissionDenied() - subs_id = request.GET.get('subs_id') if not subs_id: log.debug('GET data without "subs_id" property.') @@ -240,7 +229,7 @@ def check_transcripts(request): 'status': 'Error', } try: - __, videos, item = validate_transcripts_data(request) + __, videos, item = _validate_transcripts_data(request) except TranscriptsRequestValidationException as e: return error_response(transcripts_presence, e.message) @@ -303,7 +292,7 @@ def check_transcripts(request): if len(html5_subs) == 2: # check html5 transcripts for equality transcripts_presence['html5_equal'] = json.loads(html5_subs[0]) == json.loads(html5_subs[1]) - command, subs_to_use = transcripts_logic(transcripts_presence, videos) + command, subs_to_use = _transcripts_logic(transcripts_presence, videos) transcripts_presence.update({ 'command': command, 'subs': subs_to_use, @@ -311,7 +300,7 @@ def check_transcripts(request): return JsonResponse(transcripts_presence) -def transcripts_logic(transcripts_presence, videos): +def _transcripts_logic(transcripts_presence, videos): """ By `transcripts_presence` content, figure what show to user: @@ -386,7 +375,7 @@ def choose_transcripts(request): } try: - data, videos, item = validate_transcripts_data(request) + data, videos, item = _validate_transcripts_data(request) except TranscriptsRequestValidationException as e: return error_response(response, e.message) @@ -416,7 +405,7 @@ def replace_transcripts(request): response = {'status': 'Error', 'subs': ''} try: - __, videos, item = validate_transcripts_data(request) + __, videos, item = _validate_transcripts_data(request) except TranscriptsRequestValidationException as e: return error_response(response, e.message) @@ -435,7 +424,7 @@ def replace_transcripts(request): return JsonResponse(response) -def validate_transcripts_data(request): +def _validate_transcripts_data(request): """ Validates, that request contains all proper data for transcripts processing. @@ -452,18 +441,10 @@ def validate_transcripts_data(request): if not data: raise TranscriptsRequestValidationException('Incoming video data is empty.') - item_location = data.get('id') - - # This is placed before has_access() to validate item_location, - # because has_access() raises InvalidLocationError if location is invalid. try: - item = modulestore().get_item(item_location) - except (ItemNotFoundError, InvalidLocationError): - raise TranscriptsRequestValidationException("Can't find item by location.") - - # Check permissions for this user within this course. - if not has_access(request.user, item_location): - raise PermissionDenied() + item = _get_item(request, data) + except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError): + raise TranscriptsRequestValidationException("Can't find item by locator.") if item.category != 'video': raise TranscriptsRequestValidationException('Transcripts are supported only for "video" modules.') @@ -492,7 +473,7 @@ def rename_transcripts(request): response = {'status': 'Error', 'subs': ''} try: - __, videos, item = validate_transcripts_data(request) + __, videos, item = _validate_transcripts_data(request) except TranscriptsRequestValidationException as e: return error_response(response, e.message) @@ -525,11 +506,10 @@ def save_transcripts(request): if not data: return error_response(response, 'Incoming video data is empty.') - item_location = data.get('id') try: - item = modulestore().get_item(item_location) - except (ItemNotFoundError, InvalidLocationError): - return error_response(response, "Can't find item by location.") + item = _get_item(request, data) + except (ItemNotFoundError, InvalidLocationError, InsufficientSpecificationError): + return error_response(response, "Can't find item by locator.") metadata = data.get('metadata') if metadata is not None: @@ -553,3 +533,24 @@ def save_transcripts(request): response['status'] = 'Success' return JsonResponse(response) + + +def _get_item(request, data): + """ + Obtains from 'data' the locator for an item. + Next, gets that item from the modulestore (allowing any errors to raise up). + Finally, verifies that the user has access to the item. + + Returns the item. + """ + locator = BlockUsageLocator(data.get('locator')) + old_location = loc_mapper().translate_locator_to_location(locator) + + # This is placed before has_access() to validate the location, + # because has_access() raises InvalidLocationError if location is invalid. + item = modulestore().get_item(old_location) + + if not has_access(request.user, locator): + raise PermissionDenied() + + return item diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index 6f2a2fbdec9f..d68aaeff7b6b 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -1,5 +1,4 @@ import json -from django.conf import settings from django.core.exceptions import PermissionDenied from django.contrib.auth.models import User, Group from django.contrib.auth.decorators import login_required @@ -10,9 +9,11 @@ from mitxmako.shortcuts import render_to_response from xmodule.modulestore.django import modulestore, loc_mapper -from util.json_request import JsonResponse +from util.json_request import JsonResponse, expect_json from auth.authz import ( - STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME, get_course_groupname_for_role) + STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME, get_course_groupname_for_role, + get_course_role_users +) from course_creators.views import user_requested_access from .access import has_access @@ -35,6 +36,7 @@ def request_course_creator(request): return JsonResponse({"Status": "OK"}) +# pylint: disable=unused-argument @login_required @ensure_csrf_cookie @require_http_methods(("GET", "POST", "PUT", "DELETE")) @@ -62,38 +64,39 @@ def course_team_handler(request, tag=None, course_id=None, branch=None, version_ return HttpResponseNotFound() -def _manage_users(request, location): +def _manage_users(request, locator): """ This view will return all CMS users who are editors for the specified course """ - old_location = loc_mapper().translate_locator_to_location(location) + old_location = loc_mapper().translate_locator_to_location(locator) # check that logged in user has permissions to this item - if not has_access(request.user, location, role=INSTRUCTOR_ROLE_NAME) and not has_access(request.user, location, role=STAFF_ROLE_NAME): + if not has_access(request.user, locator): raise PermissionDenied() course_module = modulestore().get_item(old_location) - - staff_groupname = get_course_groupname_for_role(location, "staff") - staff_group, __ = Group.objects.get_or_create(name=staff_groupname) - inst_groupname = get_course_groupname_for_role(location, "instructor") - inst_group, __ = Group.objects.get_or_create(name=inst_groupname) + instructors = get_course_role_users(locator, INSTRUCTOR_ROLE_NAME) + # the page only lists staff and assumes they're a superset of instructors. Do a union to ensure. + staff = set(get_course_role_users(locator, STAFF_ROLE_NAME)).union(instructors) return render_to_response('manage_users.html', { 'context_course': course_module, - 'staff': staff_group.user_set.all(), - 'instructors': inst_group.user_set.all(), - 'allow_actions': has_access(request.user, location, role=INSTRUCTOR_ROLE_NAME), + 'staff': staff, + 'instructors': instructors, + 'allow_actions': has_access(request.user, locator, role=INSTRUCTOR_ROLE_NAME), }) -def _course_team_user(request, location, email): - old_location = loc_mapper().translate_locator_to_location(location) +@expect_json +def _course_team_user(request, locator, email): + """ + Handle the add, remove, promote, demote requests ensuring the requester has authority + """ # check that logged in user has permissions to this item - if has_access(request.user, location, role=INSTRUCTOR_ROLE_NAME): + if has_access(request.user, locator, role=INSTRUCTOR_ROLE_NAME): # instructors have full permissions pass - elif has_access(request.user, location, role=STAFF_ROLE_NAME) and email == request.user.email: + elif has_access(request.user, locator, role=STAFF_ROLE_NAME) and email == request.user.email: # staff can only affect themselves pass else: @@ -123,7 +126,7 @@ def _course_team_user(request, location, email): # what's the highest role that this user has? groupnames = set(g.name for g in user.groups.all()) for role in roles: - role_groupname = get_course_groupname_for_role(old_location, role) + role_groupname = get_course_groupname_for_role(locator, role) if role_groupname in groupnames: msg["role"] = role break @@ -139,7 +142,7 @@ def _course_team_user(request, location, email): # make sure that the role groups exist groups = {} for role in roles: - groupname = get_course_groupname_for_role(old_location, role) + groupname = get_course_groupname_for_role(locator, role) group, __ = Group.objects.get_or_create(name=groupname) groups[role] = group @@ -162,22 +165,13 @@ def _course_team_user(request, location, email): return JsonResponse() # all other operations require the requesting user to specify a role - if request.META.get("CONTENT_TYPE", "").startswith("application/json") and request.body: - try: - payload = json.loads(request.body) - except: - return JsonResponse({"error": _("malformed JSON")}, 400) - try: - role = payload["role"] - except KeyError: - return JsonResponse({"error": _("`role` is required")}, 400) - else: - if not "role" in request.POST: - return JsonResponse({"error": _("`role` is required")}, 400) - role = request.POST["role"] + role = request.json.get("role", request.POST.get("role")) + if role is None: + return JsonResponse({"error": _("`role` is required")}, 400) + old_location = loc_mapper().translate_locator_to_location(locator) if role == "instructor": - if not has_access(request.user, location, role=INSTRUCTOR_ROLE_NAME): + if not has_access(request.user, locator, role=INSTRUCTOR_ROLE_NAME): msg = { "error": _("Only instructors may create other instructors") } @@ -203,4 +197,3 @@ def _course_team_user(request, location, email): CourseEnrollment.enroll(user, old_location.course_id) return JsonResponse() - diff --git a/cms/djangoapps/models/settings/course_details.py b/cms/djangoapps/models/settings/course_details.py index 99ce00b891cf..dd8582ba7648 100644 --- a/cms/djangoapps/models/settings/course_details.py +++ b/cms/djangoapps/models/settings/course_details.py @@ -1,20 +1,25 @@ +import re +import logging +import datetime +import json +from json.encoder import JSONEncoder + from xmodule.modulestore import Location from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata -import json -from json.encoder import JSONEncoder from contentstore.utils import get_modulestore, course_image_url from models.settings import course_grading from contentstore.utils import update_item from xmodule.fields import Date -import re -import logging -import datetime +from xmodule.modulestore.django import loc_mapper class CourseDetails(object): - def __init__(self, location): - self.course_location = location # a Location obj + def __init__(self, org, course_id, run): + # still need these for now b/c the client's screen shows these 3 fields + self.org = org + self.course_id = course_id + self.run = run self.start_date = None # 'start' self.end_date = None # 'end' self.enrollment_start = None @@ -27,16 +32,13 @@ def __init__(self, location): self.course_image_asset_path = "" # URL of the course image @classmethod - def fetch(cls, course_location): + def fetch(cls, course_locator): """ Fetch the course details for the given course from persistence and return a CourseDetails model. """ - if not isinstance(course_location, Location): - course_location = Location(course_location) - - course = cls(course_location) - - descriptor = get_modulestore(course_location).get_item(course_location) + course_old_location = loc_mapper().translate_locator_to_location(course_locator) + descriptor = get_modulestore(course_old_location).get_item(course_old_location) + course = cls(course_old_location.org, course_old_location.course, course_old_location.name) course.start_date = descriptor.start course.end_date = descriptor.end @@ -45,7 +47,7 @@ def fetch(cls, course_location): course.course_image_name = descriptor.course_image course.course_image_asset_path = course_image_url(descriptor) - temploc = course_location.replace(category='about', name='syllabus') + temploc = course_old_location.replace(category='about', name='syllabus') try: course.syllabus = get_modulestore(temploc).get_item(temploc).data except ItemNotFoundError: @@ -73,14 +75,12 @@ def fetch(cls, course_location): return course @classmethod - def update_from_json(cls, jsondict): + def update_from_json(cls, course_locator, jsondict): """ Decode the json into CourseDetails and save any changed attrs to the db """ - # TODO make it an error for this to be undefined & for it to not be retrievable from modulestore - course_location = Location(jsondict['course_location']) - # Will probably want to cache the inflight courses because every blur generates an update - descriptor = get_modulestore(course_location).get_item(course_location) + course_old_location = loc_mapper().translate_locator_to_location(course_locator) + descriptor = get_modulestore(course_old_location).get_item(course_old_location) dirty = False @@ -134,11 +134,11 @@ def update_from_json(cls, jsondict): # MongoKeyValueStore before we update the mongo datastore. descriptor.save() - get_modulestore(course_location).update_metadata(course_location, own_metadata(descriptor)) + get_modulestore(course_old_location).update_metadata(course_old_location, own_metadata(descriptor)) # NOTE: below auto writes to the db w/o verifying that any of the fields actually changed # to make faster, could compare against db or could have client send over a list of which fields changed. - temploc = Location(course_location).replace(category='about', name='syllabus') + temploc = Location(course_old_location).replace(category='about', name='syllabus') update_item(temploc, jsondict['syllabus']) temploc = temploc.replace(name='overview') @@ -151,9 +151,9 @@ def update_from_json(cls, jsondict): recomposed_video_tag = CourseDetails.recompose_video_tag(jsondict['intro_video']) update_item(temploc, recomposed_video_tag) - # Could just generate and return a course obj w/o doing any db reads, but I put the reads in as a means to confirm + # Could just return jsondict w/o doing any db reads, but I put the reads in as a means to confirm # it persisted correctly - return CourseDetails.fetch(course_location) + return CourseDetails.fetch(course_locator) @staticmethod def parse_video_tag(raw_video): @@ -188,6 +188,9 @@ def recompose_video_tag(video_key): # TODO move to a more general util? class CourseSettingsEncoder(json.JSONEncoder): + """ + Serialize CourseDetails, CourseGradingModel, datetime, and old Locations + """ def default(self, obj): if isinstance(obj, (CourseDetails, course_grading.CourseGradingModel)): return obj.__dict__ diff --git a/cms/djangoapps/models/settings/course_grading.py b/cms/djangoapps/models/settings/course_grading.py index 578961fad6cd..fbbb37450cb1 100644 --- a/cms/djangoapps/models/settings/course_grading.py +++ b/cms/djangoapps/models/settings/course_grading.py @@ -1,6 +1,7 @@ -from xmodule.modulestore import Location -from contentstore.utils import get_modulestore from datetime import timedelta +from contentstore.utils import get_modulestore +from xmodule.modulestore.django import loc_mapper +from xblock.fields import Scope class CourseGradingModel(object): @@ -9,22 +10,20 @@ class CourseGradingModel(object): """ # Within this class, allow access to protected members of client classes. # This comes up when accessing kvs data and caches during kvs saves and modulestore writes. - # pylint: disable=W0212 def __init__(self, course_descriptor): - self.course_location = course_descriptor.location - self.graders = [CourseGradingModel.jsonize_grader(i, grader) for i, grader in enumerate(course_descriptor.raw_grader)] # weights transformed to ints [0..100] + self.graders = [ + CourseGradingModel.jsonize_grader(i, grader) for i, grader in enumerate(course_descriptor.raw_grader) + ] # weights transformed to ints [0..100] self.grade_cutoffs = course_descriptor.grade_cutoffs self.grace_period = CourseGradingModel.convert_set_grace_period(course_descriptor) @classmethod - def fetch(cls, course_location): + def fetch(cls, course_locator): """ - Fetch the course details for the given course from persistence and return a CourseDetails model. + Fetch the course grading policy for the given course from persistence and return a CourseGradingModel. """ - if not isinstance(course_location, Location): - course_location = Location(course_location) - - descriptor = get_modulestore(course_location).get_item(course_location) + course_old_location = loc_mapper().translate_locator_to_location(course_locator) + descriptor = get_modulestore(course_old_location).get_item(course_old_location) model = cls(descriptor) return model @@ -35,12 +34,8 @@ def fetch_grader(course_location, index): Fetch the course's nth grader Returns an empty dict if there's no such grader. """ - if not isinstance(course_location, Location): - course_location = Location(course_location) - - descriptor = get_modulestore(course_location).get_item(course_location) - # # ??? it would be good if these had the course_location in them so that they stand alone sufficiently - # # but that would require not using CourseDescriptor's field directly. Opinions? + course_old_location = loc_mapper().translate_locator_to_location(course_location) + descriptor = get_modulestore(course_old_location).get_item(course_old_location) index = int(index) if len(descriptor.raw_grader) > index: @@ -57,48 +52,26 @@ def fetch_grader(course_location, index): } @staticmethod - def fetch_cutoffs(course_location): - """ - Fetch the course's grade cutoffs. - """ - if not isinstance(course_location, Location): - course_location = Location(course_location) - - descriptor = get_modulestore(course_location).get_item(course_location) - return descriptor.grade_cutoffs - - @staticmethod - def fetch_grace_period(course_location): - """ - Fetch the course's default grace period. - """ - if not isinstance(course_location, Location): - course_location = Location(course_location) - - descriptor = get_modulestore(course_location).get_item(course_location) - return {'grace_period': CourseGradingModel.convert_set_grace_period(descriptor)} - - @staticmethod - def update_from_json(jsondict): + def update_from_json(course_locator, jsondict): """ Decode the json into CourseGradingModel and save any changes. Returns the modified model. Probably not the usual path for updates as it's too coarse grained. """ - course_location = Location(jsondict['course_location']) - descriptor = get_modulestore(course_location).get_item(course_location) + course_old_location = loc_mapper().translate_locator_to_location(course_locator) + descriptor = get_modulestore(course_old_location).get_item(course_old_location) + graders_parsed = [CourseGradingModel.parse_grader(jsonele) for jsonele in jsondict['graders']] descriptor.raw_grader = graders_parsed descriptor.grade_cutoffs = jsondict['grade_cutoffs'] - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - descriptor.save() - get_modulestore(course_location).update_item(course_location, descriptor.xblock_kvs._data) + get_modulestore(course_old_location).update_item( + course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.content) + ) - CourseGradingModel.update_grace_period_from_json(course_location, jsondict['grace_period']) + CourseGradingModel.update_grace_period_from_json(course_locator, jsondict['grace_period']) - return CourseGradingModel.fetch(course_location) + return CourseGradingModel.fetch(course_locator) @staticmethod def update_grader_from_json(course_location, grader): @@ -106,12 +79,8 @@ def update_grader_from_json(course_location, grader): Create or update the grader of the given type (string key) for the given course. Returns the modified grader which is a full model on the client but not on the server (just a dict) """ - if not isinstance(course_location, Location): - course_location = Location(course_location) - - descriptor = get_modulestore(course_location).get_item(course_location) - # # ??? it would be good if these had the course_location in them so that they stand alone sufficiently - # # but that would require not using CourseDescriptor's field directly. Opinions? + course_old_location = loc_mapper().translate_locator_to_location(course_location) + descriptor = get_modulestore(course_old_location).get_item(course_old_location) # parse removes the id; so, grab it before parse index = int(grader.get('id', len(descriptor.raw_grader))) @@ -122,10 +91,9 @@ def update_grader_from_json(course_location, grader): else: descriptor.raw_grader.append(grader) - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - descriptor.save() - get_modulestore(course_location).update_item(course_location, descriptor._field_data._kvs._data) + get_modulestore(course_old_location).update_item( + course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.content) + ) return CourseGradingModel.jsonize_grader(index, descriptor.raw_grader[index]) @@ -135,16 +103,13 @@ def update_cutoffs_from_json(course_location, cutoffs): Create or update the grade cutoffs for the given course. Returns sent in cutoffs (ie., no extra db fetch). """ - if not isinstance(course_location, Location): - course_location = Location(course_location) - - descriptor = get_modulestore(course_location).get_item(course_location) + course_old_location = loc_mapper().translate_locator_to_location(course_location) + descriptor = get_modulestore(course_old_location).get_item(course_old_location) descriptor.grade_cutoffs = cutoffs - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - descriptor.save() - get_modulestore(course_location).update_item(course_location, descriptor._field_data._kvs._data) + get_modulestore(course_old_location).update_item( + course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.content) + ) return cutoffs @@ -155,8 +120,8 @@ def update_grace_period_from_json(course_location, graceperiodjson): grace_period entry in an enclosing dict. It is also safe to call this method with a value of None for graceperiodjson. """ - if not isinstance(course_location, Location): - course_location = Location(course_location) + course_old_location = loc_mapper().translate_locator_to_location(course_location) + descriptor = get_modulestore(course_old_location).get_item(course_old_location) # Before a graceperiod has ever been created, it will be None (once it has been # created, it cannot be set back to None). @@ -164,81 +129,67 @@ def update_grace_period_from_json(course_location, graceperiodjson): if 'grace_period' in graceperiodjson: graceperiodjson = graceperiodjson['grace_period'] - # lms requires these to be in a fixed order grace_timedelta = timedelta(**graceperiodjson) - - descriptor = get_modulestore(course_location).get_item(course_location) descriptor.graceperiod = grace_timedelta - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - descriptor.save() - get_modulestore(course_location).update_metadata(course_location, descriptor._field_data._kvs._metadata) + get_modulestore(course_old_location).update_metadata( + course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.settings) + ) @staticmethod def delete_grader(course_location, index): """ Delete the grader of the given type from the given course. """ - if not isinstance(course_location, Location): - course_location = Location(course_location) + course_old_location = loc_mapper().translate_locator_to_location(course_location) + descriptor = get_modulestore(course_old_location).get_item(course_old_location) - descriptor = get_modulestore(course_location).get_item(course_location) index = int(index) if index < len(descriptor.raw_grader): del descriptor.raw_grader[index] # force propagation to definition descriptor.raw_grader = descriptor.raw_grader - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - descriptor.save() - get_modulestore(course_location).update_item(course_location, descriptor._field_data._kvs._data) + get_modulestore(course_old_location).update_item( + course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.content) + ) @staticmethod def delete_grace_period(course_location): """ - Delete the course's default grace period. + Delete the course's grace period. """ - if not isinstance(course_location, Location): - course_location = Location(course_location) + course_old_location = loc_mapper().translate_locator_to_location(course_location) + descriptor = get_modulestore(course_old_location).get_item(course_old_location) - descriptor = get_modulestore(course_location).get_item(course_location) del descriptor.graceperiod - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - descriptor.save() - get_modulestore(course_location).update_metadata(course_location, descriptor._field_data._kvs._metadata) + get_modulestore(course_old_location).update_metadata( + course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.settings) + ) @staticmethod def get_section_grader_type(location): - if not isinstance(location, Location): - location = Location(location) - - descriptor = get_modulestore(location).get_item(location) - return {"graderType": descriptor.format if descriptor.format is not None else 'Not Graded', - "location": location, - "id": 99 # just an arbitrary value to - } + old_location = loc_mapper().translate_locator_to_location(location) + descriptor = get_modulestore(old_location).get_item(old_location) + return { + "graderType": descriptor.format if descriptor.format is not None else 'Not Graded', + "location": unicode(location), + } @staticmethod - def update_section_grader_type(location, jsondict): - if not isinstance(location, Location): - location = Location(location) - - descriptor = get_modulestore(location).get_item(location) - if 'graderType' in jsondict and jsondict['graderType'] != u"Not Graded": - descriptor.format = jsondict.get('graderType') + def update_section_grader_type(descriptor, grader_type): + if grader_type is not None and grader_type != u"Not Graded": + descriptor.format = grader_type descriptor.graded = True else: del descriptor.format del descriptor.graded - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - descriptor.save() - get_modulestore(location).update_metadata(location, descriptor._field_data._kvs._metadata) + get_modulestore(descriptor.location).update_metadata( + descriptor.location, descriptor.get_explicitly_set_fields_by_scope(Scope.settings) + ) + return {'graderType': grader_type} @staticmethod def convert_set_grace_period(descriptor): diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index 603865b8846b..ddb4814511b9 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -1,7 +1,7 @@ -from xmodule.modulestore import Location +from xblock.fields import Scope + from contentstore.utils import get_modulestore from xmodule.modulestore.inheritance import own_metadata -from xblock.fields import Scope from cms.xmodule_namespace import CmsBlockMixin @@ -20,21 +20,18 @@ class CourseMetadata(object): 'tabs', 'graceperiod', 'checklists', - 'show_timezone' + 'show_timezone', + 'format', + 'graded', ] @classmethod - def fetch(cls, course_location): + def fetch(cls, descriptor): """ Fetch the key:value editable course details for the given course from persistence and return a CourseMetadata model. """ - if not isinstance(course_location, Location): - course_location = Location(course_location) - - course = {} - - descriptor = get_modulestore(course_location).get_item(course_location) + result = {} for field in descriptor.fields.values(): if field.name in CmsBlockMixin.fields: @@ -46,19 +43,17 @@ def fetch(cls, course_location): if field.name in cls.FILTERED_LIST: continue - course[field.name] = field.read_json(descriptor) + result[field.name] = field.read_json(descriptor) - return course + return result @classmethod - def update_from_json(cls, course_location, jsondict, filter_tabs=True): + def update_from_json(cls, descriptor, jsondict, filter_tabs=True): """ Decode the json into CourseMetadata and save any changed attrs to the db. Ensures none of the fields are in the blacklist. """ - descriptor = get_modulestore(course_location).get_item(course_location) - dirty = False # Copy the filtered list to avoid permanently changing the class attribute. @@ -72,39 +67,17 @@ def update_from_json(cls, course_location, jsondict, filter_tabs=True): if key in filtered_list: continue + if key == "unsetKeys": + dirty = True + for unset in val: + descriptor.fields[unset].delete_from(descriptor) + if hasattr(descriptor, key) and getattr(descriptor, key) != val: dirty = True value = descriptor.fields[key].from_json(val) setattr(descriptor, key, value) if dirty: - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - descriptor.save() - get_modulestore(course_location).update_metadata(course_location, - own_metadata(descriptor)) - - # Could just generate and return a course obj w/o doing any db reads, - # but I put the reads in as a means to confirm it persisted correctly - return cls.fetch(course_location) - - @classmethod - def delete_key(cls, course_location, payload): - ''' - Remove the given metadata key(s) from the course. payload can be a - single key or [key..] - ''' - descriptor = get_modulestore(course_location).get_item(course_location) - - for key in payload['deleteKeys']: - if hasattr(descriptor, key): - delattr(descriptor, key) - - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - descriptor.save() - - get_modulestore(course_location).update_metadata(course_location, - own_metadata(descriptor)) + get_modulestore(descriptor.location).update_metadata(descriptor.location, own_metadata(descriptor)) - return cls.fetch(course_location) + return cls.fetch(descriptor) diff --git a/cms/envs/aws.py b/cms/envs/aws.py index 592415f61ae7..1b0c0ef6482f 100644 --- a/cms/envs/aws.py +++ b/cms/envs/aws.py @@ -166,9 +166,14 @@ if SEGMENT_IO_KEY: MITX_FEATURES['SEGMENT_IO'] = ENV_TOKENS.get('SEGMENT_IO', False) - AWS_ACCESS_KEY_ID = AUTH_TOKENS["AWS_ACCESS_KEY_ID"] +if AWS_ACCESS_KEY_ID == "": + AWS_ACCESS_KEY_ID = None + AWS_SECRET_ACCESS_KEY = AUTH_TOKENS["AWS_SECRET_ACCESS_KEY"] +if AWS_SECRET_ACCESS_KEY == "": + AWS_SECRET_ACCESS_KEY = None + DATABASES = AUTH_TOKENS['DATABASES'] MODULESTORE = AUTH_TOKENS['MODULESTORE'] CONTENTSTORE = AUTH_TOKENS['CONTENTSTORE'] diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 4c69172170be..e25f092c9a84 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -23,7 +23,8 @@ ################################# LMS INTEGRATION ############################# -MITX_FEATURES['PREVIEW_LMS_BASE'] = "preview.localhost:8000" +LMS_BASE = "localhost:8000" +MITX_FEATURES['PREVIEW_LMS_BASE'] = "preview." + LMS_BASE ################################# CELERY ###################################### diff --git a/cms/static/coffee/spec/main.coffee b/cms/static/coffee/spec/main.coffee index 2ee8d1797958..c84b60be61a8 100644 --- a/cms/static/coffee/spec/main.coffee +++ b/cms/static/coffee/spec/main.coffee @@ -197,7 +197,8 @@ define([ "js/spec/transcripts/videolist_spec", "js/spec/transcripts/message_manager_spec", "js/spec/transcripts/file_uploader_spec", - "js/spec/utils/module_spec" + "js/spec/utils/module_spec", + "js/spec/models/explicit_url_spec" # these tests are run separate in the cms-squire suite, due to process # isolation issues with Squire.js diff --git a/cms/static/coffee/spec/views/course_info_spec.coffee b/cms/static/coffee/spec/views/course_info_spec.coffee index 3c388fa5936d..1e843d59fbb0 100644 --- a/cms/static/coffee/spec/views/course_info_spec.coffee +++ b/cms/static/coffee/spec/views/course_info_spec.coffee @@ -196,3 +196,22 @@ define ["js/views/course_info_handout", "js/views/course_info_update", "js/model @handoutsEdit.$el.find('.edit-button').click() expect(@handoutsEdit.$codeMirror.getValue().trim()).toEqual('/static/fromServer.jpg') + it "can open course handouts with bad html on edit", -> + # Enter some bad html in handouts section, verifying that the + # model/handoutform opens when "Edit" is clicked + + @model = new ModuleInfo({ + id: 'handouts-id', + data: '

[LINK TEXT]

') + expect($('.edit-handouts-form').is(':hidden')).toEqual(false) \ No newline at end of file diff --git a/cms/static/coffee/spec/views/module_edit_spec.coffee b/cms/static/coffee/spec/views/module_edit_spec.coffee index 22d1052fa3a0..36716668d34c 100644 --- a/cms/static/coffee/spec/views/module_edit_spec.coffee +++ b/cms/static/coffee/spec/views/module_edit_spec.coffee @@ -1,12 +1,9 @@ -define ["coffee/src/views/module_edit", "xmodule"], (ModuleEdit) -> +define ["coffee/src/views/module_edit", "js/models/module_info", "xmodule"], (ModuleEdit, ModuleModel) -> describe "ModuleEdit", -> beforeEach -> - @stubModule = jasmine.createSpy("Module") - @stubModule.id = 'stub-id' - @stubModule.get = (param)-> - if param == 'old_id' - return 'stub-old-id' + @stubModule = new ModuleModel + id: "stub-id" setFixtures """
  • @@ -62,7 +59,7 @@ define ["coffee/src/views/module_edit", "xmodule"], (ModuleEdit) -> @moduleEdit.render() it "loads the module preview and editor via ajax on the view element", -> - expect(@moduleEdit.$el.load).toHaveBeenCalledWith("/preview_component/#{@moduleEdit.model.get('old_id')}", jasmine.any(Function)) + expect(@moduleEdit.$el.load).toHaveBeenCalledWith("/xblock/#{@moduleEdit.model.id}", jasmine.any(Function)) @moduleEdit.$el.load.mostRecentCall.args[1]() expect(@moduleEdit.loadDisplay).toHaveBeenCalled() expect(@moduleEdit.delegateEvents).toHaveBeenCalled() diff --git a/cms/static/coffee/spec/views/overview_spec.coffee b/cms/static/coffee/spec/views/overview_spec.coffee index 9ece1b0059e4..cbc08212137a 100644 --- a/cms/static/coffee/spec/views/overview_spec.coffee +++ b/cms/static/coffee/spec/views/overview_spec.coffee @@ -36,7 +36,7 @@ define ["js/views/overview", "js/views/feedback_notification", "sinon", "js/base appendSetFixtures """
    -
    diff --git a/cms/static/coffee/src/views/module_edit.coffee b/cms/static/coffee/src/views/module_edit.coffee index a13e572887c8..729a17615e18 100644 --- a/cms/static/coffee/src/views/module_edit.coffee +++ b/cms/static/coffee/src/views/module_edit.coffee @@ -69,15 +69,13 @@ define ["backbone", "jquery", "underscore", "gettext", "xblock/runtime.v1", payload (data) => @model.set(id: data.locator) - @model.set(old_id: data.id) - @$el.data('id', data.id) @$el.data('locator', data.locator) @render() ) render: -> - if @model.get('old_id') - @$el.load("/preview_component/#{@model.get('old_id')}", => + if @model.id + @$el.load(@model.url(), => @loadDisplay() @delegateEvents() ) diff --git a/cms/static/coffee/src/views/tabs.coffee b/cms/static/coffee/src/views/tabs.coffee index 0f72e8bddbf0..83ca7dc2fedc 100644 --- a/cms/static/coffee/src/views/tabs.coffee +++ b/cms/static/coffee/src/views/tabs.coffee @@ -6,8 +6,7 @@ define ["jquery", "jquery.ui", "backbone", "js/views/feedback_prompt", "js/views initialize: => @$('.component').each((idx, element) => model = new ModuleModel({ - id: $(element).data('locator'), - old_id:$(element).data('id') + id: $(element).data('locator') }) new ModuleEditView( @@ -38,14 +37,17 @@ define ["jquery", "jquery.ui", "backbone", "js/views/feedback_prompt", "js/views analytics.track "Reordered Static Pages", course: course_location_analytics + saving = new NotificationView.Mini({title: gettext("Saving…")}) + saving.show() + $.ajax({ type:'POST', - url: '/reorder_static_tabs', + url: @model.url(), data: JSON.stringify({ tabs : tabs }), contentType: 'application/json' - }) + }).success(=> saving.hide()) addNewTab: (event) => event.preventDefault() diff --git a/cms/static/coffee/src/views/unit.coffee b/cms/static/coffee/src/views/unit.coffee index 075b56d1b0a0..c4ff25c309ca 100644 --- a/cms/static/coffee/src/views/unit.coffee +++ b/cms/static/coffee/src/views/unit.coffee @@ -63,7 +63,6 @@ define ["jquery", "jquery.ui", "gettext", "backbone", @$('.component').each (idx, element) => model = new ModuleModel id: $(element).data('locator') - old_id: $(element).data('id') new ModuleEditView el: element, onDelete: @deleteComponent, @@ -167,7 +166,7 @@ define ["jquery", "jquery.ui", "gettext", "backbone", @wait(true) $.ajax({ type: 'DELETE', - url: @model.urlRoot + "/" + @$el.data('locator') + "?" + $.param({recurse: true}) + url: @model.url() + "?" + $.param({recurse: true}) }).success(=> analytics.track "Deleted Draft", @@ -180,8 +179,8 @@ define ["jquery", "jquery.ui", "gettext", "backbone", createDraft: (event) -> @wait(true) - $.postJSON('/create_draft', { - id: @$el.data('id') + $.postJSON(@model.url(), { + publish: 'create_draft' }, => analytics.track "Created Draft", course: course_location_analytics @@ -194,8 +193,8 @@ define ["jquery", "jquery.ui", "gettext", "backbone", @wait(true) @saveDraft() - $.postJSON('/publish_draft', { - id: @$el.data('id') + $.postJSON(@model.url(), { + publish: 'make_public' }, => analytics.track "Published Draft", course: course_location_analytics @@ -206,16 +205,16 @@ define ["jquery", "jquery.ui", "gettext", "backbone", setVisibility: (event) -> if @$('.visibility-select').val() == 'private' - target_url = '/unpublish_unit' + action = 'make_private' visibility = "private" else - target_url = '/publish_draft' + action = 'make_public' visibility = "public" @wait(true) - $.postJSON(target_url, { - id: @$el.data('id') + $.postJSON(@model.url(), { + publish: action }, => analytics.track "Set Unit Visibility", course: course_location_analytics diff --git a/cms/static/js/base.js b/cms/static/js/base.js index bc7260cf6401..174ab10c89cc 100644 --- a/cms/static/js/base.js +++ b/cms/static/js/base.js @@ -237,7 +237,7 @@ function createNewUnit(e) { function(data) { // redirect to the edit page - window.location = "/edit/" + data['id']; + window.location = "/unit/" + data['locator']; }); } diff --git a/cms/static/js/collections/course_grader.js b/cms/static/js/collections/course_grader.js index c4adf64e1f3f..7dde698cb27d 100644 --- a/cms/static/js/collections/course_grader.js +++ b/cms/static/js/collections/course_grader.js @@ -2,10 +2,6 @@ define(["backbone", "js/models/settings/course_grader"], function(Backbone, Cour var CourseGraderCollection = Backbone.Collection.extend({ model : CourseGrader, - course_location : null, // must be set to a Location object - url : function() { - return '/' + this.course_location.get('org') + "/" + this.course_location.get('course') + '/settings-grading/' + this.course_location.get('name') + '/'; - }, sumWeights : function() { return this.reduce(function(subtotal, grader) { return subtotal + grader.get('weight'); }, 0); } diff --git a/cms/static/js/models/assignment_grade.js b/cms/static/js/models/assignment_grade.js index 4c3d54b976bb..83f00a7d106c 100644 --- a/cms/static/js/models/assignment_grade.js +++ b/cms/static/js/models/assignment_grade.js @@ -1,26 +1,14 @@ -define(["backbone", "underscore", "js/models/location"], function(Backbone, _, Location) { +define(["backbone", "underscore"], function(Backbone, _) { var AssignmentGrade = Backbone.Model.extend({ defaults : { - graderType : null, // the type label (string). May be "Not Graded" which implies None. I'd like to use id but that's ephemeral - location : null // A location object + graderType : null, // the type label (string). May be "Not Graded" which implies None. + locator : null // locator for the block }, - initialize : function(attrs) { - if (attrs['assignmentUrl']) { - this.set('location', new Location(attrs['assignmentUrl'], {parse: true})); - } - }, - parse : function(attrs) { - if (attrs && attrs['location']) { - attrs.location = new Location(attrs['location'], {parse: true}); - } - }, - urlRoot : function() { - if (this.has('location')) { - var location = this.get('location'); - return '/' + location.get('org') + "/" + location.get('course') + '/' + location.get('category') + '/' - + location.get('name') + '/gradeas/'; - } - else return ""; + idAttribute: 'locator', + urlRoot : '/xblock/', + url: function() { + // add ?fields=graderType to the request url (only needed for fetch, but innocuous for others) + return Backbone.Model.prototype.url.apply(this) + '?' + $.param({fields: 'graderType'}); } }); return AssignmentGrade; diff --git a/cms/static/js/models/course_info.js b/cms/static/js/models/course_info.js index e5a6114dff08..e4c816ccf3e1 100644 --- a/cms/static/js/models/course_info.js +++ b/cms/static/js/models/course_info.js @@ -5,12 +5,9 @@ define(["backbone"], function(Backbone) { url: '', defaults: { - "courseId": "", // the location url "updates" : null, // UpdateCollection "handouts": null // HandoutCollection - }, - - idAttribute : "courseId" + } }); return CourseInfo; }); diff --git a/cms/static/js/models/explicit_url.js b/cms/static/js/models/explicit_url.js new file mode 100644 index 000000000000..aae69608af85 --- /dev/null +++ b/cms/static/js/models/explicit_url.js @@ -0,0 +1,14 @@ +/** + * A model that simply allows the update URL to be passed + * in as an argument. + */ +define(["backbone"], function(Backbone){ + return Backbone.Model.extend({ + defaults: { + "explicit_url": "" + }, + url: function() { + return this.get("explicit_url"); + } + }); +}); diff --git a/cms/static/js/models/settings/course_details.js b/cms/static/js/models/settings/course_details.js index 13cc4ce692ff..058cacadd70e 100644 --- a/cms/static/js/models/settings/course_details.js +++ b/cms/static/js/models/settings/course_details.js @@ -1,8 +1,10 @@ -define(["backbone", "underscore", "gettext", "js/models/location"], function(Backbone, _, gettext, Location) { +define(["backbone", "underscore", "gettext"], function(Backbone, _, gettext) { var CourseDetails = Backbone.Model.extend({ defaults: { - location : null, // the course's Location model, required + org : '', + course_id: '', + run: '', start_date: null, // maps to 'start' end_date: null, // maps to 'end' enrollment_start: null, @@ -17,9 +19,6 @@ var CourseDetails = Backbone.Model.extend({ // When init'g from html script, ensure you pass {parse: true} as an option (2nd arg to reset) parse: function(attributes) { - if (attributes['course_location']) { - attributes.location = new Location(attributes.course_location, {parse:true}); - } if (attributes['start_date']) { attributes.start_date = new Date(attributes.start_date); } diff --git a/cms/static/js/models/settings/course_grading_policy.js b/cms/static/js/models/settings/course_grading_policy.js index 1e23a4ecf45e..d034aa2cef14 100644 --- a/cms/static/js/models/settings/course_grading_policy.js +++ b/cms/static/js/models/settings/course_grading_policy.js @@ -3,15 +3,11 @@ define(["backbone", "js/models/location", "js/collections/course_grader"], var CourseGradingPolicy = Backbone.Model.extend({ defaults : { - course_location : null, graders : null, // CourseGraderCollection grade_cutoffs : null, // CourseGradeCutoff model grace_period : null // either null or { hours: n, minutes: m, ...} }, parse: function(attributes) { - if (attributes['course_location']) { - attributes.course_location = new Location(attributes.course_location, {parse:true}); - } if (attributes['graders']) { var graderCollection; // interesting race condition: if {parse:true} when newing, then parse called before .attributes created @@ -21,7 +17,6 @@ var CourseGradingPolicy = Backbone.Model.extend({ } else { graderCollection = new CourseGraderCollection(attributes.graders, {parse:true}); - graderCollection.course_location = attributes['course_location'] || this.get('course_location'); } attributes.graders = graderCollection; } @@ -35,10 +30,6 @@ var CourseGradingPolicy = Backbone.Model.extend({ } return attributes; }, - url : function() { - var location = this.get('course_location'); - return '/' + location.get('org') + "/" + location.get('course') + '/settings-details/' + location.get('name') + '/section/grading'; - }, gracePeriodToDate : function() { var newDate = new Date(); if (this.has('grace_period') && this.get('grace_period')['hours']) diff --git a/cms/static/js/spec/models/explicit_url_spec.js b/cms/static/js/spec/models/explicit_url_spec.js new file mode 100644 index 000000000000..df70d47b63ff --- /dev/null +++ b/cms/static/js/spec/models/explicit_url_spec.js @@ -0,0 +1,12 @@ +define(['js/models/explicit_url'], + function (Model) { + describe('Model ', function () { + it('allows url to be passed in constructor', function () { + expect(new Model({'explicit_url': '/fancy/url'}).url()).toBe('/fancy/url'); + }); + it('returns empty string if url not set', function () { + expect(new Model().url()).toBe(''); + }); + }); + } +); diff --git a/cms/static/js/spec/transcripts/file_uploader_spec.js b/cms/static/js/spec/transcripts/file_uploader_spec.js index c896ad811ae2..20b653527cf6 100644 --- a/cms/static/js/spec/transcripts/file_uploader_spec.js +++ b/cms/static/js/spec/transcripts/file_uploader_spec.js @@ -48,7 +48,7 @@ function ($, _, Utils, FileUploader) { el: $container, messenger: messenger, videoListObject: videoListObject, - component_id: 'component_id' + component_locator: 'component_locator' }); }); diff --git a/cms/static/js/spec/transcripts/message_manager_spec.js b/cms/static/js/spec/transcripts/message_manager_spec.js index c6d3bc790967..40eef7f02e00 100644 --- a/cms/static/js/spec/transcripts/message_manager_spec.js +++ b/cms/static/js/spec/transcripts/message_manager_spec.js @@ -52,7 +52,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) { view = new MessageManager({ el: $container, parent: videoList, - component_id: 'component_id' + component_locator: 'component_locator' }); }); @@ -60,7 +60,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) { expect(fileUploader.initialize).toHaveBeenCalledWith({ el: view.$el, messenger: view, - component_id: view.component_id, + component_locator: view.component_locator, videoListObject: view.options.parent }); }); @@ -215,7 +215,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) { function() { expect(Utils.command).toHaveBeenCalledWith( action, - view.component_id, + view.component_locator, videoList, void(0) ); @@ -245,7 +245,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) { function () { expect(Utils.command).toHaveBeenCalledWith( action, - view.component_id, + view.component_locator, videoList, { html5_id: extraParamas @@ -268,7 +268,7 @@ function ($, _, Utils, MessageManager, FileUploader, sinon) { function () { expect(Utils.command).toHaveBeenCalledWith( action, - view.component_id, + view.component_locator, videoList, void(0) ); diff --git a/cms/static/js/spec/transcripts/videolist_spec.js b/cms/static/js/spec/transcripts/videolist_spec.js index cf189c254978..de850eb3eed3 100644 --- a/cms/static/js/spec/transcripts/videolist_spec.js +++ b/cms/static/js/spec/transcripts/videolist_spec.js @@ -11,7 +11,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s 'transcripts/metadata-videolist-entry.underscore' ), abstractEditor = AbstractEditor.prototype, - component_id = 'component_id', + component_locator = 'component_locator', videoList = [ { mode: "youtube", @@ -62,7 +62,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s var tpl = sandbox({ 'class': 'component', - 'data-id': component_id + 'data-locator': component_locator }), model = new MetadataModel(modelStub), videoList, $el; @@ -157,7 +157,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s waitsForResponse(function () { expect(abstractEditor.initialize).toHaveBeenCalled(); expect(messenger.initialize).toHaveBeenCalled(); - expect(view.component_id).toBe(component_id); + expect(view.component_locator).toBe(component_locator); expect(view.$el).toHandle('input'); }); }); @@ -167,7 +167,7 @@ function ($, _, Utils, VideoList, MetadataView, MetadataModel, AbstractEditor, s expect(abstractEditor.render).toHaveBeenCalled(); expect(Utils.command).toHaveBeenCalledWith( 'check', - component_id, + component_locator, videoList ); diff --git a/cms/static/js/views/course_info_handout.js b/cms/static/js/views/course_info_handout.js index f9804d03a445..9309deda1b47 100644 --- a/cms/static/js/views/course_info_handout.js +++ b/cms/static/js/views/course_info_handout.js @@ -30,6 +30,7 @@ define(["backbone", "underscore", "codemirror", "js/views/feedback_notification" model: this.model })) ); + $('.handouts-content').html(this.model.get('data')); this.$preview = this.$el.find('.handouts-content'); this.$form = this.$el.find(".edit-handouts-form"); this.$editor = this.$form.find('.handouts-content-editor'); @@ -50,32 +51,43 @@ define(["backbone", "underscore", "codemirror", "js/views/feedback_notification" }, onSave: function(event) { - this.model.set('data', this.$codeMirror.getValue()); - var saving = new NotificationView.Mini({ - title: gettext('Saving…') - }); - saving.show(); - this.model.save({}, { - success: function() { - saving.hide(); - } - }); - this.render(); - this.$form.hide(); - this.closeEditor(); - - analytics.track('Saved Course Handouts', { - 'course': course_location_analytics - }); + $('#handout_error').removeClass('is-shown'); + $('.save-button').removeClass('is-disabled'); + if ($('.CodeMirror-lines').find('.cm-error').length == 0){ + this.model.set('data', this.$codeMirror.getValue()); + var saving = new NotificationView.Mini({ + title: gettext('Saving…') + }); + saving.show(); + this.model.save({}, { + success: function() { + saving.hide(); + } + }); + this.render(); + this.$form.hide(); + this.closeEditor(); + analytics.track('Saved Course Handouts', { + 'course': course_location_analytics + }); + }else{ + $('#handout_error').addClass('is-shown'); + $('.save-button').addClass('is-disabled'); + event.preventDefault(); + } }, onCancel: function(event) { + $('#handout_error').removeClass('is-shown'); + $('.save-button').removeClass('is-disabled'); this.$form.hide(); this.closeEditor(); }, closeEditor: function() { + $('#handout_error').removeClass('is-shown'); + $('.save-button').removeClass('is-disabled'); this.$form.hide(); ModalUtils.hideModalCover(); this.$form.find('.CodeMirror').remove(); diff --git a/cms/static/js/views/course_info_helper.js b/cms/static/js/views/course_info_helper.js index ec4a6ba55075..fb3474cdb0ca 100644 --- a/cms/static/js/views/course_info_helper.js +++ b/cms/static/js/views/course_info_helper.js @@ -6,7 +6,10 @@ define(["codemirror", "utility"], var $codeMirror = CodeMirror.fromTextArea(textArea, { mode: "text/html", lineNumbers: true, - lineWrapping: true + lineWrapping: true, + onChange: function () { + $('.save-button').removeClass('is-disabled'); + } }); $codeMirror.setValue(content); $codeMirror.clearHistory(); diff --git a/cms/static/js/views/overview_assignment_grader.js b/cms/static/js/views/overview_assignment_grader.js index 40e93496930c..b7b501f572eb 100644 --- a/cms/static/js/views/overview_assignment_grader.js +++ b/cms/static/js/views/overview_assignment_grader.js @@ -21,7 +21,7 @@ define(["backbone", "underscore", "gettext", "js/models/assignment_grade", "js/v '
  • Not Graded
    • ' + ''); this.assignmentGrade = new AssignmentGrade({ - assignmentUrl : this.$el.closest('.id-holder').data('id'), + locator : this.$el.closest('.id-holder').data('locator'), graderType : this.$el.data('initial-status')}); // TODO throw exception if graders is null this.graders = this.options['graders']; diff --git a/cms/static/js/views/settings/main.js b/cms/static/js/views/settings/main.js index ded2781f6666..63776829c3bd 100644 --- a/cms/static/js/views/settings/main.js +++ b/cms/static/js/views/settings/main.js @@ -21,9 +21,9 @@ var DetailsView = ValidatingView.extend({ initialize : function() { this.fileAnchorTemplate = _.template(' <%= filename %>'); // fill in fields - this.$el.find("#course-name").val(this.model.get('location').get('name')); - this.$el.find("#course-organization").val(this.model.get('location').get('org')); - this.$el.find("#course-number").val(this.model.get('location').get('course')); + this.$el.find("#course-organization").val(this.model.get('org')); + this.$el.find("#course-number").val(this.model.get('course_id')); + this.$el.find("#course-name").val(this.model.get('run')); this.$el.find('.set-date').datepicker({ 'dateFormat': 'm/d/yy' }); // Avoid showing broken image on mistyped/nonexistent image diff --git a/cms/static/js/views/transcripts/editor.js b/cms/static/js/views/transcripts/editor.js index 21e48a03b04b..78280a0d66c9 100644 --- a/cms/static/js/views/transcripts/editor.js +++ b/cms/static/js/views/transcripts/editor.js @@ -72,7 +72,7 @@ function($, Backbone, _, Utils, MetadataView, MetadataCollection) { syncBasicTab: function (metadataCollection, metadataView) { var result = [], getField = Utils.getField, - component_id = this.$el.closest('.component').data('id'), + component_locator = this.$el.closest('.component').data('locator'), subs = getField(metadataCollection, 'sub'), values = {}, videoUrl, metadata, modifiedValues; @@ -99,7 +99,7 @@ function($, Backbone, _, Utils, MetadataView, MetadataCollection) { if (isSubsModified) { metadata = $.extend(true, {}, modifiedValues); // Save module state - Utils.command('save', component_id, null, { + Utils.command('save', component_locator, null, { metadata: metadata, current_subs: _.pluck( Utils.getVideoList(videoUrl.getDisplayValue()), @@ -110,18 +110,16 @@ function($, Backbone, _, Utils, MetadataView, MetadataCollection) { // Get values from `Advanced` tab fields (`html5_sources`, // `youtube_id_1_0`) that should be synchronized. - html5Sources = getField(metadataCollection, 'html5_sources') -                                    .getDisplayValue(); + var html5Sources = getField(metadataCollection, 'html5_sources').getDisplayValue(); -            values.youtube = getField(metadataCollection, 'youtube_id_1_0') -                                    .getDisplayValue(); + values.youtube = getField(metadataCollection, 'youtube_id_1_0').getDisplayValue(); -            values.html5Sources = _.filter(html5Sources, function (value) { -                var link = Utils.parseLink(value), + values.html5Sources = _.filter(html5Sources, function (value) { + var link = Utils.parseLink(value), mode = link && link.mode; -                return mode === 'html5' && mode; -            }); + return mode === 'html5' && mode; + }); // The length of youtube video_id should be 11 characters. diff --git a/cms/static/js/views/transcripts/file_uploader.js b/cms/static/js/views/transcripts/file_uploader.js index c9c33820e43b..d307c7c85e39 100644 --- a/cms/static/js/views/transcripts/file_uploader.js +++ b/cms/static/js/views/transcripts/file_uploader.js @@ -39,7 +39,7 @@ function($, Backbone, _, Utils) { tplContainer.html(this.template({ ext: this.validFileExtensions, - component_id: this.options.component_id, + component_locator: this.options.component_locator, video_list: videoList })); diff --git a/cms/static/js/views/transcripts/message_manager.js b/cms/static/js/views/transcripts/message_manager.js index 80f70752cfee..506f3143ac2b 100644 --- a/cms/static/js/views/transcripts/message_manager.js +++ b/cms/static/js/views/transcripts/message_manager.js @@ -31,12 +31,12 @@ function($, Backbone, _, Utils, FileUploader, gettext) { initialize: function () { _.bindAll(this); - this.component_id = this.$el.closest('.component').data('id'); + this.component_locator = this.$el.closest('.component').data('locator'); this.fileUploader = new FileUploader({ el: this.$el, messenger: this, - component_id: this.component_id, + component_locator: this.component_locator, videoListObject: this.options.parent }); }, @@ -76,7 +76,7 @@ function($, Backbone, _, Utils, FileUploader, gettext) { this.$el.find('.transcripts-status') .removeClass('is-invisible') .find(this.elClass).html(template({ - component_id: encodeURIComponent(this.component_id), + component_locator: encodeURIComponent(this.component_locator), html5_list: html5List, grouped_list: groupedList, subs_id: (params) ? params.subs: '' @@ -204,7 +204,7 @@ function($, Backbone, _, Utils, FileUploader, gettext) { */ processCommand: function (action, errorMessage, videoId) { var self = this, - component_id = this.component_id, + component_locator = this.component_locator, videoList = this.options.parent.getVideoObjectsList(), extraParam, xhr; @@ -212,7 +212,7 @@ function($, Backbone, _, Utils, FileUploader, gettext) { extraParam = { html5_id: videoId }; } - xhr = Utils.command(action, component_id, videoList, extraParam) + xhr = Utils.command(action, component_locator, videoList, extraParam) .done(function (resp) { var sub = resp.subs; diff --git a/cms/static/js/views/transcripts/metadata_videolist.js b/cms/static/js/views/transcripts/metadata_videolist.js index 6e7e82232b10..7e7dac7f1956 100644 --- a/cms/static/js/views/transcripts/metadata_videolist.js +++ b/cms/static/js/views/transcripts/metadata_videolist.js @@ -46,7 +46,7 @@ function($, Backbone, _, AbstractEditor, Utils, MessageManager, MetadataView) { _.debounce(_.bind(this.inputHandler, this), this.inputDelay) ); - this.component_id = this.$el.closest('.component').data('id'); + this.component_locator = this.$el.closest('.component').data('locator'); }, render: function () { @@ -55,7 +55,7 @@ function($, Backbone, _, AbstractEditor, Utils, MessageManager, MetadataView) { .apply(this, arguments); var self = this, - component_id = this.$el.closest('.component').data('id'), + component_locator = this.$el.closest('.component').data('locator'), videoList = this.getVideoObjectsList(), showServerError = function (response) { @@ -82,7 +82,7 @@ function($, Backbone, _, AbstractEditor, Utils, MessageManager, MetadataView) { } // Check current state of Timed Transcripts. - Utils.command('check', component_id, videoList) + Utils.command('check', component_locator, videoList) .done(function (resp) { var params = resp, len = videoList.length, diff --git a/cms/static/js/views/transcripts/utils.js b/cms/static/js/views/transcripts/utils.js index 7965d0b9e306..a6865906e57b 100644 --- a/cms/static/js/views/transcripts/utils.js +++ b/cms/static/js/views/transcripts/utils.js @@ -295,7 +295,7 @@ define(["jquery", "underscore", "jquery.ajaxQueue"], function($, _) { * * @param {string} action Action that will be invoked on server. Is a part * of url. - * @param {string} component_id Id of component. + * @param {string} component_locator the locator of component. * @param {array} videoList List of object with information about inserted * urls. * @param {object} extraParams Extra parameters that can be send to the @@ -314,7 +314,7 @@ define(["jquery", "underscore", "jquery.ajaxQueue"], function($, _) { // _command() function. var xhr = null; - return function (action, component_id, videoList, extraParams) { + return function (action, component_locator, videoList, extraParams) { var params, data; if (extraParams) { @@ -326,7 +326,7 @@ define(["jquery", "underscore", "jquery.ajaxQueue"], function($, _) { } data = $.extend( - { id: component_id }, + { locator: component_locator }, { videos: videoList }, params ); diff --git a/cms/static/sass/contexts/_ie.scss b/cms/static/sass/contexts/_ie.scss index 4599ec3e6897..ed9484f010bd 100644 --- a/cms/static/sass/contexts/_ie.scss +++ b/cms/static/sass/contexts/_ie.scss @@ -10,6 +10,10 @@ &.is-shown { bottom: 0; } + + &.is-hiding { + bottom: -($ui-notification-height); + } } } diff --git a/cms/static/sass/elements/_xmodules.scss b/cms/static/sass/elements/_xmodules.scss index 576fd9549b55..8ffccfef2da8 100644 --- a/cms/static/sass/elements/_xmodules.scss +++ b/cms/static/sass/elements/_xmodules.scss @@ -1,4 +1,15 @@ -// studio - elements - xmodules +// studio - elements - xmodules & xblocks +// ==================== + +// general - display mode (xblock-student_view or xmodule_display) +.xmodule_display, .xblock-student_view { + + // font styling + i, em { + font-style: italic; + } +} + // ==================== // Video Alpha diff --git a/cms/templates/asset_index.html b/cms/templates/asset_index.html index 5576664df7f3..4f6f14a4669c 100644 --- a/cms/templates/asset_index.html +++ b/cms/templates/asset_index.html @@ -187,7 +187,7 @@

    ${_("What can I do on this page?")}

    ${_('close')}

    ${_("Upload New File")}

    -

    +

    diff --git a/cms/templates/course_info.html b/cms/templates/course_info.html index 401d32ccb21f..8cddb8a8c569 100644 --- a/cms/templates/course_info.html +++ b/cms/templates/course_info.html @@ -33,7 +33,6 @@ var editor = new CourseInfoEditView({ el: $('.main-wrapper'), model : new CourseInfoModel({ - courseId : '${context_course.location}', updates : course_updates, base_asset_url : '${base_asset_url}', handouts : course_handouts diff --git a/cms/templates/edit-tabs.html b/cms/templates/edit-tabs.html index 2dba71c2793f..70c3e61801db 100644 --- a/cms/templates/edit-tabs.html +++ b/cms/templates/edit-tabs.html @@ -9,12 +9,15 @@ <%block name="jsextra"> - + But in this there should be
    Great ideas require offsetting. - + bad tests require drivel
    - + [code] Code should be nicely monospaced. [/code] @@ -322,7 +363,7 @@ describe 'MarkdownEditingDescriptor', -> expect(data).toEqual("""

    Not a header

    A header

    - +

    Multiple choice w/ parentheticals

    @@ -332,7 +373,7 @@ describe 'MarkdownEditingDescriptor', -> no space b4 close paren - +

    Choice checks

    @@ -343,25 +384,25 @@ describe 'MarkdownEditingDescriptor', -> no space - +

    Option with multiple correct ones

    - + - +

    Option with embedded parens

    - + - +

    What happens w/ empty correct options?

    - + - +

    Explanation

    @@ -379,14 +420,14 @@ describe 'MarkdownEditingDescriptor', -> console.log(two * 2); - +

    But in this there should be

    Great ideas require offsetting.

    - +

    bad tests require drivel

    - + 
    
         Code should be nicely monospaced.
    diff --git a/common/lib/xmodule/xmodule/js/spec/video/general_spec.js b/common/lib/xmodule/xmodule/js/spec/video/general_spec.js index 8921e819e4fc..72b5e4e3b226 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/general_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/general_spec.js @@ -270,7 +270,7 @@ } }); - // Disabled 10/29/13 due to flakiness in master + // Disabled 11/25/13 due to flakiness in master xdescribe('multiple YT on page', function () { var state1, state2, state3; diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js index 29f4567c8a1b..4e45d3283883 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js @@ -456,7 +456,7 @@ expect(videoCaption.currentIndex).toEqual(5); }); - // Disabled 10/25/13 due to flakiness in master + // Disabled 11/25/13 due to flakiness in master xit('scroll caption to new position', function () { expect($.fn.scrollTo).toHaveBeenCalled(); }); @@ -537,7 +537,7 @@ }); }); - // Disabled 10/23/13 due to flakiness in master + // Disabled 11/25/13 due to flakiness in master xdescribe('scrollCaption', function () { beforeEach(function () { initialize(); @@ -682,7 +682,7 @@ .toHaveAttr('title', 'Turn off captions'); }); - // Test turned off due to flakiness (30.10.2013). + // Test turned off due to flakiness (11/25/13) xit('scroll the caption', function () { // After transcripts are shown, and the video plays for a // bit. diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_focus_grabber_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_focus_grabber_spec.js index 6f33d978fb19..71cf4f45c430 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_focus_grabber_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_focus_grabber_spec.js @@ -72,7 +72,17 @@ expect(state.focusGrabber.disableFocusGrabber).toHaveBeenCalled(); }); - it('after controls hide focus grabbers are enabled', function () { + // Disabled on 18.11.2013 due to flakiness on local dev machine. + // + // Video FocusGrabber: after controls hide focus grabbers are + // enabled [fail] + // Expected spy enableFocusGrabber to have been called. + // + // Approximately 1 in 8 times this test fails. + // + // TODO: Most likely, focusGrabber will be disabled in the future. This + // test could become unneeded in the future. + xit('after controls hide focus grabbers are enabled', function () { runs(function () { // Captions should not be "sticky" for the autohide mechanism // to work. diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js index 88da768f84b3..62e14753568a 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js @@ -4,11 +4,21 @@ videoProgressSlider, videoSpeedControl, videoVolumeControl, oldOTBD; - function initialize(fixture) { - if (typeof fixture === 'undefined') { - loadFixtures('video_all.html'); - } else { + function initialize(fixture, params) { + if (_.isString(fixture)) { loadFixtures(fixture); + } else { + if (_.isObject(fixture)) { + params = fixture; + } + + loadFixtures('video_all.html'); + } + + if (_.isObject(params)) { + $('#example') + .find('#video_id') + .data(params); } state = new Video('#example'); @@ -532,8 +542,54 @@ }); }); - // Disabled 10/24/13 due to flakiness in master - xdescribe('updatePlayTime', function () { + describe('update with start & end time', function () { + var START_TIME = 1, END_TIME = 2; + + beforeEach(function () { + initialize({start: START_TIME, end: END_TIME}); + + spyOn(videoPlayer, 'update').andCallThrough(); + spyOn(videoPlayer, 'pause').andCallThrough(); + spyOn(videoProgressSlider, 'notifyThroughHandleEnd') + .andCallThrough(); + }); + + it('video is paused on first endTime, start & end time are reset', function () { + var checkForStartEndTimeSet = true; + + videoProgressSlider.notifyThroughHandleEnd.reset(); + videoPlayer.pause.reset(); + videoPlayer.play(); + + waitsFor(function () { + if ( + !isFinite(videoPlayer.currentTime) || + videoPlayer.currentTime <= 0 + ) { + return false; + } + + if (checkForStartEndTimeSet) { + checkForStartEndTimeSet = false; + + expect(videoPlayer.startTime).toBe(START_TIME); + expect(videoPlayer.endTime).toBe(END_TIME); + } + + return videoPlayer.pause.calls.length === 1 + }, 5000, 'pause() has been called'); + + runs(function () { + expect(videoPlayer.startTime).toBe(0); + expect(videoPlayer.endTime).toBe(null); + + expect(videoProgressSlider.notifyThroughHandleEnd) + .toHaveBeenCalledWith({end: true}); + }); + }); + }); + + describe('updatePlayTime', function () { beforeEach(function () { initialize(); @@ -548,7 +604,7 @@ duration = videoPlayer.duration(); if (duration > 0) { - return true; + return true; } return false; @@ -612,6 +668,74 @@ }); }); + describe('updatePlayTime when start & end times are defined', function () { + var START_TIME = 1, + END_TIME = 2; + + beforeEach(function () { + initialize({start: START_TIME, end: END_TIME}); + + spyOn(videoPlayer, 'updatePlayTime').andCallThrough(); + spyOn(videoPlayer.player, 'seekTo').andCallThrough(); + spyOn(videoProgressSlider, 'updateStartEndTimeRegion') + .andCallThrough(); + }); + + it('when duration becomes available, updatePlayTime() is called', function () { + var duration; + + expect(videoPlayer.initialSeekToStartTime).toBeTruthy(); + expect(videoPlayer.seekToStartTimeOldSpeed).toBe('void'); + + videoPlayer.play(); + + waitsFor(function () { + duration = videoPlayer.duration(); + + return duration > 0 && + videoPlayer.initialSeekToStartTime === false; + }, 'duration becomes available', 1000); + + runs(function () { + expect(videoPlayer.startTime).toBe(START_TIME); + expect(videoPlayer.endTime).toBe(END_TIME); + + expect(videoPlayer.player.seekTo).toHaveBeenCalledWith(START_TIME); + + expect(videoProgressSlider.updateStartEndTimeRegion) + .toHaveBeenCalledWith({duration: duration}); + + expect(videoPlayer.seekToStartTimeOldSpeed).toBe(state.speed); + }); + }); + }); + + describe('updatePlayTime with invalid endTime', function () { + beforeEach(function () { + initialize({end: 100000}); + + spyOn(videoPlayer, 'updatePlayTime').andCallThrough(); + }); + + it('invalid endTime is reset to null', function () { + var duration; + + videoPlayer.updatePlayTime.reset(); + videoPlayer.play(); + + waitsFor(function () { + duration = videoPlayer.duration(); + + return duration > 0 && + videoPlayer.initialSeekToStartTime === false; + }, 'updatePlayTime was invoked and duration is set', 5000); + + runs(function () { + expect(videoPlayer.endTime).toBe(null); + }); + }); + }); + describe('toggleFullScreen', function () { describe('when the video player is not full screen', function () { beforeEach(function () { diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js index 751506b410bb..a8450c39cdf5 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js @@ -153,7 +153,7 @@ }); }); - // Turned off test due to flakiness (30.10.2013). + // Turned off test due to flakiness (11/25/13) xit('trigger seek event', function() { runs(function () { videoProgressSlider.onSlide( @@ -219,7 +219,7 @@ }); }); - // Turned off test due to flakiness (30.10.2013). + // Turned off test due to flakiness (11/25/13) xit('trigger seek event', function() { runs(function () { videoProgressSlider.onStop( @@ -285,6 +285,55 @@ expect(params).toEqual(expectedParams); }); }); + + describe('notifyThroughHandleEnd', function () { + beforeEach(function () { + initialize(); + + spyOnEvent(videoProgressSlider.handle, 'focus'); + spyOn(videoProgressSlider, 'notifyThroughHandleEnd') + .andCallThrough(); + }); + + it('params.end = true', function () { + videoProgressSlider.notifyThroughHandleEnd({end: true}); + + expect(videoProgressSlider.handle.attr('title')) + .toBe('video ended'); + + expect('focus').toHaveBeenTriggeredOn(videoProgressSlider.handle); + }); + + it('params.end = false', function () { + videoProgressSlider.notifyThroughHandleEnd({end: false}); + + expect(videoProgressSlider.handle.attr('title')) + .toBe('video position'); + + expect('focus').not.toHaveBeenTriggeredOn(videoProgressSlider.handle); + }); + + it('is called when video plays', function () { + videoPlayer.play(); + + waitsFor(function () { + var duration = videoPlayer.duration(), + currentTime = videoPlayer.currentTime; + + return ( + isFinite(duration) && + duration > 0 && + isFinite(currentTime) && + currentTime > 0 + ); + }, 'duration is set, video is playing', 5000); + + runs(function () { + expect(videoProgressSlider.notifyThroughHandleEnd) + .toHaveBeenCalledWith({end: false}); + }); + }); + }); }); }).call(this); diff --git a/common/lib/xmodule/xmodule/js/src/collapsible.coffee b/common/lib/xmodule/xmodule/js/src/collapsible.coffee index 2f6b2c978190..837231c482be 100644 --- a/common/lib/xmodule/xmodule/js/src/collapsible.coffee +++ b/common/lib/xmodule/xmodule/js/src/collapsible.coffee @@ -1,7 +1,7 @@ class @Collapsible # Set of library functions that provide a simple way to add collapsible - # functionality to elements. + # functionality to elements. # setCollapsibles: # Scan element's content for generic collapsible containers @@ -9,12 +9,15 @@ class @Collapsible ### el: container ### + linkTop = 'See full output' + linkBottom = 'See full output' + # standard longform + shortfom pattern el.find('.longform').hide() - el.find('.shortform').append('See full output') + el.find('.shortform').append(linkTop, linkBottom) # custom longform + shortform text pattern - short_custom = el.find('.shortform-custom') + short_custom = el.find('.shortform-custom') # set up each one individually short_custom.each (index, elt) => open_text = $(elt).data('open-text') @@ -31,13 +34,18 @@ class @Collapsible @toggleFull: (event, open_text, close_text) => event.preventDefault() - $(event.target).parent().siblings().slideToggle() - $(event.target).parent().parent().toggleClass('open') + parent = $(event.target).parent() + parent.siblings().slideToggle() + parent.parent().toggleClass('open') if $(event.target).text() == open_text new_text = close_text else new_text = open_text - $(event.target).text(new_text) + if $(event.target).hasClass('full') + el = parent.find('.full') + else + el = $(event.target) + el.text(new_text) @toggleHint: (event) => event.preventDefault() diff --git a/common/lib/xmodule/xmodule/js/src/problem/edit.coffee b/common/lib/xmodule/xmodule/js/src/problem/edit.coffee index ba85db574b00..d50cbae86574 100644 --- a/common/lib/xmodule/xmodule/js/src/problem/edit.coffee +++ b/common/lib/xmodule/xmodule/js/src/problem/edit.coffee @@ -228,24 +228,35 @@ class @MarkdownEditingDescriptor extends XModule.Descriptor }); // replace string and numerical - xml = xml.replace(/^\=\s*(.*?$)/gm, function(match, p) { - var string; - var floatValue = parseFloat(p); + xml = xml.replace(/(^\=\s*(.*?$)(\n*or\=\s*(.*?$))*)+/gm, function(match, p) { + var string, + answersList = p.replace(/^(or)?=\s*/gm, '').split('\n'), + floatValue = parseFloat(answersList[0]); + if(!isNaN(floatValue)) { - var params = /(.*?)\+\-\s*(.*?$)/.exec(p); + // Tries to extract parameters from string like 'expr +- tolerance' + var params = /(.*?)\+\-\s*(.*?$)/.exec(answersList[0]), + answer = answersList[0].replace(/\s+/g, ''); if(params) { - string = '\n'; + answer = params[1].replace(/\s+/g, ''); + string = '\n'; string += ' \n'; } else { - string = '\n'; + string = '\n'; } string += ' \n'; string += '\n\n'; } else { - string = '\n \n\n\n'; + var answers = []; + + for(var i = 0; i < answersList.length; i++) { + answers.push(answersList[i]) + } + + string = '\n \n\n\n'; } return string; - }); + }); // replace selects xml = xml.replace(/\[\[(.+?)\]\]/g, function(match, p) { @@ -262,13 +273,13 @@ class @MarkdownEditingDescriptor extends XModule.Descriptor selectString += '\n\n'; return selectString; }); - + // replace explanations xml = xml.replace(/\[explanation\]\n?([^\]]*)\[\/?explanation\]/gmi, function(match, p1) { var selectString = '\n
    \nExplanation\n\n' + p1 + '\n
    \n    '; return selectString; }); - + // replace code blocks xml = xml.replace(/\[code\]\n?([^\]]*)\[\/?code\]/gmi, function(match, p1) { var selectString = '
    \n' + p1 + '
    '; @@ -293,7 +304,7 @@ class @MarkdownEditingDescriptor extends XModule.Descriptor // rid white space xml = xml.replace(/\n\n\n/g, '\n'); - + // surround w/ problem tag xml = '\n' + xml + '\n'; diff --git a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js index c5dab0af85f0..50eff093ee65 100644 --- a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js +++ b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js @@ -3,7 +3,7 @@ // VideoPlayer module. define( 'video/03_video_player.js', -['video/02_html5_video.js', 'video/00_resizer.js' ], +['video/02_html5_video.js', 'video/00_resizer.js'], function (HTML5Video, Resizer) { var dfd = $.Deferred(); @@ -83,11 +83,9 @@ function (HTML5Video, Resizer) { state.videoPlayer.initialSeekToStartTime = true; - state.videoPlayer.oneTimePauseAtEndTime = true; - - // The initial value of the variable `seekToStartTimeOldSpeed` - // should always differ from the value returned by the duration - // function. + // At the start, the initial value of the variable + // `seekToStartTimeOldSpeed` should always differ from the value + // returned by the duration function. state.videoPlayer.seekToStartTimeOldSpeed = 'void'; state.videoPlayer.playerVars = { @@ -215,8 +213,7 @@ function (HTML5Video, Resizer) { // This function gets the video's current play position in time // (currentTime) and its duration. - // It is called at a regular interval when the video is playing (see - // below). + // It is called at a regular interval when the video is playing. function update() { this.videoPlayer.currentTime = this.videoPlayer.player .getCurrentTime(); @@ -224,22 +221,28 @@ function (HTML5Video, Resizer) { if (isFinite(this.videoPlayer.currentTime)) { this.videoPlayer.updatePlayTime(this.videoPlayer.currentTime); - // We need to pause the video is current time is smaller (or equal) - // than end time. Also, we must make sure that the end time is the - // one that was set in the configuration parameter. If it differs, - // this means that it was either reset to the end, or the duration - // changed it's value. + // We need to pause the video if current time is smaller (or equal) + // than end time. Also, we must make sure that this is only done + // once. // - // In the case of YouTube Flash mode, we must remember that the - // start and end times are rescaled based on the current speed of - // the video. + // If `endTime` is not `null`, then we are safe to pause the + // video. `endTime` will be set to `null`, and this if statement + // will not be executed on next runs. if ( - this.videoPlayer.endTime <= this.videoPlayer.currentTime && - this.videoPlayer.oneTimePauseAtEndTime + this.videoPlayer.endTime != null && + this.videoPlayer.endTime <= this.videoPlayer.currentTime ) { - this.videoPlayer.oneTimePauseAtEndTime = false; this.videoPlayer.pause(); - this.videoPlayer.endTime = this.videoPlayer.duration(); + + // After the first time the video reached the `endTime`, + // `startTime` and `endTime` are disabled. The video will play + // from start to the end on subsequent runs. + this.videoPlayer.startTime = 0; + this.videoPlayer.endTime = null; + + this.trigger('videoProgressSlider.notifyThroughHandleEnd', { + end: true + }); } } } @@ -321,8 +324,10 @@ function (HTML5Video, Resizer) { } ); + // After the user seeks, startTime and endTime are disabled. The video + // will play from start to the end on subsequent runs. this.videoPlayer.startTime = 0; - this.videoPlayer.endTime = duration; + this.videoPlayer.endTime = null; this.videoPlayer.player.seekTo(newTime, true); @@ -344,11 +349,21 @@ function (HTML5Video, Resizer) { var time = this.videoPlayer.duration(); this.trigger('videoControl.pause', null); + this.trigger('videoProgressSlider.notifyThroughHandleEnd', { + end: true + }); if (this.config.show_captions) { this.trigger('videoCaption.pause', null); } + // When only `startTime` is set, the video will play to the end + // starting at `startTime`. After the first time the video reaches the + // end, `startTime` and `endTime` are disabled. The video will play + // from start to the end on subsequent runs. + this.videoPlayer.startTime = 0; + this.videoPlayer.endTime = null; + // Sometimes `onEnded` events fires when `currentTime` not equal // `duration`. In this case, slider doesn't reach the end point of // timeline. @@ -391,6 +406,10 @@ function (HTML5Video, Resizer) { this.trigger('videoControl.play', null); + this.trigger('videoProgressSlider.notifyThroughHandleEnd', { + end: false + }); + if (this.config.show_captions) { this.trigger('videoCaption.play', null); } @@ -531,7 +550,7 @@ function (HTML5Video, Resizer) { function updatePlayTime(time) { var duration = this.videoPlayer.duration(), - durationChange; + durationChange, tempStartTime, tempEndTime; if ( duration > 0 && @@ -545,13 +564,23 @@ function (HTML5Video, Resizer) { this.videoPlayer.initialSeekToStartTime === false ) { durationChange = true; - } else { + } else { // this.videoPlayer.initialSeekToStartTime === true + this.videoPlayer.initialSeekToStartTime = false; + durationChange = false; } - this.videoPlayer.initialSeekToStartTime = false; this.videoPlayer.seekToStartTimeOldSpeed = this.speed; + // Current startTime and endTime could have already been reset. + // We will remember their current values, and reset them at the + // end. We need to perform the below calculations on start and end + // times so that the range on the slider gets correctly updated in + // the case of speed change in Flash player mode (for YouTube + // videos). + tempStartTime = this.videoPlayer.startTime; + tempEndTime = this.videoPlayer.endTime; + // We retrieve the original times. They could have been changed due // to the fact of speed change (duration change). This happens when // in YouTube Flash mode. There each speed is a different video, @@ -566,31 +595,33 @@ function (HTML5Video, Resizer) { this.videoPlayer.startTime /= Number(this.speed); } } + + // An `endTime` of `null` means that either the user didn't set + // and `endTime`, or it was set to a value greater than the + // duration of the video. + // + // If `endTime` is `null`, the video will play to the end. We do + // not set the `endTime` to the duration of the video because + // sometimes in YouTube mode the duration changes slightly during + // the course of playback. This would cause the video to pause just + // before the actual end of the video. if ( - this.videoPlayer.endTime === null || + this.videoPlayer.endTime !== null && this.videoPlayer.endTime > duration ) { - this.videoPlayer.endTime = duration; - } else { + this.videoPlayer.endTime = null; + } else if (this.videoPlayer.endTime !== null) { if (this.currentPlayerMode === 'flash') { this.videoPlayer.endTime /= Number(this.speed); } } - // If this is not a duration change (if it is, we continue playing - // from current time), then we need to seek the video to the start - // time. - // - // We seek only if start time differs from zero. - if (durationChange === false && this.videoPlayer.startTime > 0) { - this.videoPlayer.player.seekTo(this.videoPlayer.startTime); - } - // Rebuild the slider start-end range (if it doesn't take up the - // whole slider). + // whole slider). Remember that endTime === null means the end time + // is set to the end of video by default. if (!( this.videoPlayer.startTime === 0 && - this.videoPlayer.endTime === duration + this.videoPlayer.endTime === null )) { this.trigger( 'videoProgressSlider.updateStartEndTimeRegion', @@ -599,6 +630,28 @@ function (HTML5Video, Resizer) { } ); } + + // If this is not a duration change (if it is, we continue playing + // from current time), then we need to seek the video to the start + // time. + // + // We seek only if start time differs from zero, and we haven't + // performed already such a seek. + if ( + durationChange === false && + this.videoPlayer.startTime > 0 && + !(tempStartTime === 0 && tempEndTime === null) + ) { + this.videoPlayer.player.seekTo(this.videoPlayer.startTime); + } + + // Reset back the actual startTime and endTime if they have been + // already reset (a seek event happened, the video already ended + // once, or endTime has already been reached once). + if (tempStartTime === 0 && tempEndTime === null) { + this.videoPlayer.startTime = 0; + this.videoPlayer.endTime = null; + } } this.trigger( diff --git a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js index 59750a0f11f5..e10c4fbb243f 100644 --- a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js +++ b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js @@ -41,7 +41,8 @@ function () { onSlide: onSlide, onStop: onStop, updatePlayTime: updatePlayTime, - updateStartEndTimeRegion: updateStartEndTimeRegion + updateStartEndTimeRegion: updateStartEndTimeRegion, + notifyThroughHandleEnd: notifyThroughHandleEnd }; state.bindTo(methodsDict, state.videoProgressSlider, state); @@ -111,11 +112,6 @@ function () { duration = params.duration; } - // If the range spans the entire length of video, we don't do anything. - if (!this.videoPlayer.startTime && !this.videoPlayer.endTime) { - return; - } - start = this.videoPlayer.startTime; // If end is set to null, then we set it to the end of the video. We @@ -199,8 +195,6 @@ function () { }, 200); } - // Changed for tests -- JM: Check if it is the cause of Chrome Bug Valera - // noticed function updatePlayTime(params) { var time = Math.floor(params.time), duration = Math.floor(params.duration); @@ -215,6 +209,33 @@ function () { } } + // When the video stops playing (either because the end was reached, or + // because endTime was reached), the screen reader must be notified that + // the video is no longer playing. We do this by a little trick. Setting + // the title attribute of the slider know to "video ended", and focusing + // on it. The screen reader will read the attr text. + // + // The user can then tab his way forward, landing on the next control + // element, the Play button. + // + // @param params - object with property `end`. If set to true, the + // function must set the title attribute to + // `video ended`; + // if set to false, the function must reset the attr to + // it's original state. + // + // This function will be triggered from VideoPlayer methods onEnded(), + // onPlay(), and update() (update method handles endTime). + function notifyThroughHandleEnd(params) { + if (params.end) { + this.videoProgressSlider.handle + .attr('title', 'video ended') + .focus(); + } else { + this.videoProgressSlider.handle.attr('title', 'video position'); + } + } + // Returns a string describing the current time of video in hh:mm:ss // format. function getTimeDescription(time) { diff --git a/common/lib/xmodule/xmodule/lti_module.py b/common/lib/xmodule/xmodule/lti_module.py index 013d5e7ecb7b..24f17ddb05a4 100644 --- a/common/lib/xmodule/xmodule/lti_module.py +++ b/common/lib/xmodule/xmodule/lti_module.py @@ -83,6 +83,7 @@ class LTIFields(object): https://github.com/idan/oauthlib/blob/master/oauthlib/oauth1/rfc5849/signature.py#L136 """ + display_name = String(display_name="Display Name", help="Display name for this module", scope=Scope.settings, default="LTI") lti_id = String(help="Id of the tool", default='', scope=Scope.settings) launch_url = String(help="URL of the tool", default='http://www.example.com', scope=Scope.settings) custom_parameters = List(help="Custom parameters (vbid, book_location, etc..)", scope=Scope.settings) diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index 33b65b8b1053..c7355c2f29a3 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -204,21 +204,16 @@ def translate_locator_to_location(self, locator, get_course=False): self._decode_from_mongo(old_name), None) elif usage_id == locator.usage_id: - # figure out revision - # enforce the draft only if category in [..] logic - if category in draft.DIRECT_ONLY_CATEGORIES: - revision = None - elif locator.branch == candidate['draft_branch']: - revision = draft.DRAFT - else: - revision = None + # Always return revision=None because the + # old draft module store wraps locations as draft before + # trying to access things. return Location( 'i4x', candidate['_id']['org'], candidate['_id']['course'], category, self._decode_from_mongo(old_name), - revision) + None) return None def add_block_location_translator(self, location, old_course_id=None, usage_id=None): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index f368032f174e..2f6fabfce0d4 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -778,11 +778,7 @@ def update_children(self, location, children): children: A list of child item identifiers """ - # We expect the children IDs to always be the non-draft version. With view refactoring - # for split, we are now passing the draft version in some cases. - children_ids = [Location(child).replace(revision=None).url() for child in children] - - self._update_single_item(location, {'definition.children': children_ids}) + self._update_single_item(location, {'definition.children': children}) # recompute (and update) the metadata inheritance tree which is cached self.refresh_cached_metadata_inheritance_tree(Location(location)) # fire signal that we've written to DB diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 6318c5e68f19..f7a025015552 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -81,7 +81,7 @@ def get_item(self, location, depth=0): try: return wrap_draft(super(DraftModuleStore, self).get_item(as_draft(location), depth=depth)) except ItemNotFoundError: - return wrap_draft(super(DraftModuleStore, self).get_item(as_published(location), depth=depth)) + return wrap_draft(super(DraftModuleStore, self).get_item(location, depth=depth)) def get_instance(self, course_id, location, depth=0): """ @@ -169,7 +169,7 @@ def update_item(self, location, data, allow_not_found=False): try: draft_item = self.get_item(location) if not getattr(draft_item, 'is_draft', False): - self.convert_to_draft(as_published(location)) + self.convert_to_draft(location) except ItemNotFoundError, e: if not allow_not_found: raise e @@ -187,7 +187,7 @@ def update_children(self, location, children): draft_loc = as_draft(location) draft_item = self.get_item(location) if not getattr(draft_item, 'is_draft', False): - self.convert_to_draft(as_published(location)) + self.convert_to_draft(location) return super(DraftModuleStore, self).update_children(draft_loc, children) @@ -203,7 +203,7 @@ def update_metadata(self, location, metadata): draft_item = self.get_item(location) if not getattr(draft_item, 'is_draft', False): - self.convert_to_draft(as_published(location)) + self.convert_to_draft(location) if 'is_draft' in metadata: del metadata['is_draft'] @@ -262,7 +262,7 @@ def unpublish(self, location): """ Turn the published version into a draft, removing the published version """ - self.convert_to_draft(as_published(location)) + self.convert_to_draft(location) super(DraftModuleStore, self).delete_item(location) def _query_children_for_cache_children(self, items): diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index 27a758c0839d..355f5bfa62d3 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -88,7 +88,7 @@ def _copy_published_modules_to_course(self, new_course, old_course_loc, old_cour index_info = self.split_modulestore.get_course_index_info(course_version_locator) versions = index_info['versions'] versions['draft'] = versions['published'] - self.split_modulestore.update_course_index(course_version_locator, {'versions': versions}, update_versions=True) + self.split_modulestore.update_course_index(index_info) # clean up orphans in published version: in old mongo, parents pointed to the union of their published and draft # children which meant some pointers were to non-existent locations in 'direct' diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py index 9b2a652a95ef..ded67104b4d1 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py @@ -22,5 +22,4 @@ def fetch(self): Fetch the definition. Note, the caller should replace this lazy loader pointer with the result so as not to fetch more than once """ - return self.modulestore.definitions.find_one( - {'_id': self.definition_locator.definition_id}) + return self.modulestore.db_connection.get_definition(self.definition_locator.definition_id) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py new file mode 100644 index 000000000000..510c100048fc --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -0,0 +1,116 @@ +""" +Segregation of pymongo functions from the data modeling mechanisms for split modulestore. +""" +import pymongo + +class MongoConnection(object): + """ + Segregation of pymongo functions from the data modeling mechanisms for split modulestore. + """ + def __init__( + self, db, collection, host, port=27017, tz_aware=True, user=None, password=None, **kwargs + ): + """ + Create & open the connection, authenticate, and provide pointers to the collections + """ + self.database = pymongo.database.Database( + pymongo.MongoClient( + host=host, + port=port, + tz_aware=tz_aware, + **kwargs + ), + db + ) + + if user is not None and password is not None: + self.database.authenticate(user, password) + + self.course_index = self.database[collection + '.active_versions'] + self.structures = self.database[collection + '.structures'] + self.definitions = self.database[collection + '.definitions'] + + # every app has write access to the db (v having a flag to indicate r/o v write) + # Force mongo to report errors, at the expense of performance + # pymongo docs suck but explanation: + # http://api.mongodb.org/java/2.10.1/com/mongodb/WriteConcern.html + self.course_index.write_concern = {'w': 1} + self.structures.write_concern = {'w': 1} + self.definitions.write_concern = {'w': 1} + + def get_structure(self, key): + """ + Get the structure from the persistence mechanism whose id is the given key + """ + return self.structures.find_one({'_id': key}) + + def find_matching_structures(self, query): + """ + Find the structure matching the query. Right now the query must be a legal mongo query + :param query: a mongo-style query of {key: [value|{$in ..}|..], ..} + """ + return self.structures.find(query) + + def insert_structure(self, structure): + """ + Create the structure in the db + """ + self.structures.insert(structure) + + def update_structure(self, structure): + """ + Update the db record for structure + """ + self.structures.update({'_id': structure['_id']}, structure) + + def get_course_index(self, key): + """ + Get the course_index from the persistence mechanism whose id is the given key + """ + return self.course_index.find_one({'_id': key}) + + def find_matching_course_indexes(self, query): + """ + Find the course_index matching the query. Right now the query must be a legal mongo query + :param query: a mongo-style query of {key: [value|{$in ..}|..], ..} + """ + return self.course_index.find(query) + + def insert_course_index(self, course_index): + """ + Create the course_index in the db + """ + self.course_index.insert(course_index) + + def update_course_index(self, course_index): + """ + Update the db record for course_index + """ + self.course_index.update({'_id': course_index['_id']}, course_index) + + def delete_course_index(self, key): + """ + Delete the course_index from the persistence mechanism whose id is the given key + """ + return self.course_index.remove({'_id': key}) + + def get_definition(self, key): + """ + Get the definition from the persistence mechanism whose id is the given key + """ + return self.definitions.find_one({'_id': key}) + + def find_matching_definitions(self, query): + """ + Find the definitions matching the query. Right now the query must be a legal mongo query + :param query: a mongo-style query of {key: [value|{$in ..}|..], ..} + """ + return self.definitions.find(query) + + def insert_definition(self, definition): + """ + Create the definition in the db + """ + self.definitions.insert(definition) + + diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 61524165968f..a6349e6113b6 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -1,7 +1,6 @@ import threading import datetime import logging -import pymongo import re from importlib import import_module from path import path @@ -21,6 +20,7 @@ from xblock.fields import Scope from xblock.runtime import Mixologist from bson.objectid import ObjectId +from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection log = logging.getLogger(__name__) #============================================================================== @@ -49,7 +49,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): A Mongodb backed ModuleStore supporting versions, inheritance, and sharing. """ - # pylint: disable=W0201 def __init__(self, doc_store_config, fs_root, render_template, default_class=None, error_tracker=null_error_tracker, @@ -62,44 +61,13 @@ def __init__(self, doc_store_config, fs_root, render_template, super(SplitMongoModuleStore, self).__init__(**kwargs) self.loc_mapper = loc_mapper - def do_connection( - db, collection, host, port=27017, tz_aware=True, user=None, password=None, **kwargs - ): - """ - Create & open the connection, authenticate, and provide pointers to the collections - """ - self.db = pymongo.database.Database( - pymongo.MongoClient( - host=host, - port=port, - tz_aware=tz_aware, - **kwargs - ), - db - ) - - if user is not None and password is not None: - self.db.authenticate(user, password) - - self.course_index = self.db[collection + '.active_versions'] - self.structures = self.db[collection + '.structures'] - self.definitions = self.db[collection + '.definitions'] - - do_connection(**doc_store_config) + self.db_connection = MongoConnection(**doc_store_config) + self.db = self.db_connection.database # Code review question: How should I expire entries? # _add_cache could use a lru mechanism to control the cache size? self.thread_cache = threading.local() - - # every app has write access to the db (v having a flag to indicate r/o v write) - # Force mongo to report errors, at the expense of performance - # pymongo docs suck but explanation: - # http://api.mongodb.org/java/2.10.1/com/mongodb/WriteConcern.html - self.course_index.write_concern = {'w': 1} - self.structures.write_concern = {'w': 1} - self.definitions.write_concern = {'w': 1} - if default_class is not None: module_path, _, class_name = default_class.rpartition('.') class_ = getattr(import_module(module_path), class_name) @@ -138,7 +106,7 @@ def cache_items(self, system, base_usage_ids, depth=0, lazy=True): block['definition'] = DefinitionLazyLoader(self, block['definition']) else: # Load all descendants by id - descendent_definitions = self.definitions.find({ + descendent_definitions = self.db_connection.find_matching_definitions({ '_id': {'$in': [block['definition'] for block in new_module_data.itervalues()]}}) # turn into a map @@ -226,7 +194,7 @@ def _lookup_course(self, course_locator): if course_locator.course_id is not None and course_locator.branch is not None: # use the course_id - index = self.course_index.find_one({'_id': course_locator.course_id}) + index = self.db_connection.get_course_index(course_locator.course_id) if index is None: raise ItemNotFoundError(course_locator) if course_locator.branch not in index['versions']: @@ -241,7 +209,7 @@ def _lookup_course(self, course_locator): # cast string to ObjectId if necessary version_guid = course_locator.as_object_id(version_guid) - entry = self.structures.find_one({'_id': version_guid}) + entry = self.db_connection.get_structure(version_guid) # b/c more than one course can use same structure, the 'course_id' and 'branch' are not intrinsic to structure # and the one assoc'd w/ it by another fetch may not be the one relevant to this fetch; so, @@ -269,7 +237,7 @@ def get_courses(self, branch='published', qualifiers=None): if qualifiers is None: qualifiers = {} qualifiers.update({"versions.{}".format(branch): {"$exists": True}}) - matching = self.course_index.find(qualifiers) + matching = self.db_connection.find_matching_course_indexes(qualifiers) # collect ids and then query for those version_guids = [] @@ -279,7 +247,7 @@ def get_courses(self, branch='published', qualifiers=None): version_guids.append(version_guid) id_version_map[version_guid] = structure['_id'] - course_entries = self.structures.find({'_id': {'$in': version_guids}}) + course_entries = self.db_connection.find_matching_structures({'_id': {'$in': version_guids}}) # get the block for the course element (s/b the root) result = [] @@ -455,7 +423,7 @@ def get_course_index_info(self, course_locator): """ if course_locator.course_id is None: return None - index = self.course_index.find_one({'_id': course_locator.course_id}) + index = self.db_connection.get_course_index(course_locator.course_id) return index # TODO figure out a way to make this info accessible from the course descriptor @@ -487,7 +455,7 @@ def get_definition_history_info(self, definition_locator): 'edited_on': when the change was made } """ - definition = self.definitions.find_one({'_id': definition_locator.definition_id}) + definition = self.db_connection.get_definition(definition_locator.definition_id) if definition is None: return None return definition['edit_info'] @@ -509,14 +477,14 @@ def get_course_successors(self, course_locator, version_history_depth=1): # TODO if depth is significant, it may make sense to get all that have the same original_version # and reconstruct the subtree from version_guid - next_entries = self.structures.find({'previous_version' : version_guid}) + next_entries = self.db_connection.find_matching_structures({'previous_version' : version_guid}) # must only scan cursor's once next_versions = [struct for struct in next_entries] result = {version_guid: [CourseLocator(version_guid=struct['_id']) for struct in next_versions]} depth = 1 while depth < version_history_depth and len(next_versions) > 0: depth += 1 - next_entries = self.structures.find({'previous_version': + next_entries = self.db_connection.find_matching_structures({'previous_version': {'$in': [struct['_id'] for struct in next_versions]}}) next_versions = [struct for struct in next_entries] for course_structure in next_versions: @@ -537,7 +505,7 @@ def get_block_generations(self, block_locator): course_struct = self._lookup_course(block_locator.version_agnostic())['structure'] usage_id = block_locator.usage_id update_version_field = 'blocks.{}.edit_info.update_version'.format(usage_id) - all_versions_with_block = self.structures.find({'original_version': course_struct['original_version'], + all_versions_with_block = self.db_connection.find_matching_structures({'original_version': course_struct['original_version'], update_version_field: {'$exists': True}}) # find (all) root versions and build map previous: [successors] possible_roots = [] @@ -596,7 +564,7 @@ def create_definition_from_data(self, new_def_data, category, user_id): "original_version": new_id, } } - self.definitions.insert(document) + self.db_connection.insert_definition(document) definition_locator = DefinitionLocator(new_id) return definition_locator @@ -618,7 +586,7 @@ def needs_saved(): # if this looks in cache rather than fresh fetches, then it will probably not detect # actual change b/c the descriptor and cache probably point to the same objects - old_definition = self.definitions.find_one({'_id': definition_locator.definition_id}) + old_definition = self.db_connection.get_definition(definition_locator.definition_id) if old_definition is None: raise ItemNotFoundError(definition_locator.url()) @@ -630,7 +598,7 @@ def needs_saved(): old_definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) # previous version id old_definition['edit_info']['previous_version'] = definition_locator.definition_id - self.definitions.insert(old_definition) + self.db_connection.insert_definition(old_definition) return DefinitionLocator(old_definition['_id']), True else: return definition_locator, False @@ -657,7 +625,7 @@ def _generate_course_id(self, id_root): :param course_blocks: the current list of blocks. :param category: """ - existing_uses = self.course_index.find({"_id": {"$regex": id_root}}) + existing_uses = self.db_connection.find_matching_course_indexes({"_id": {"$regex": id_root}}) if existing_uses.count() > 0: max_found = 0 matcher = re.compile(id_root + r'(\d+)') @@ -779,11 +747,11 @@ def create_item( parent['edit_info']['update_version'] = new_id if continue_version: # db update - self.structures.update({'_id': new_id}, new_structure) + self.db_connection.update_structure(new_structure) # clear cache so things get refetched and inheritance recomputed self._clear_cache(new_id) else: - self.structures.insert(new_structure) + self.db_connection.insert_structure(new_structure) # update the index entry if appropriate if index_entry is not None: @@ -856,7 +824,7 @@ def create_course( 'original_version': definition_id, } } - self.definitions.insert(definition_entry) + self.db_connection.insert_definition(definition_entry) new_id = ObjectId() draft_structure = { @@ -880,7 +848,7 @@ def create_course( } } } - self.structures.insert(draft_structure) + self.db_connection.insert_structure(draft_structure) if versions_dict is None: versions_dict = {master_branch: new_id} @@ -898,20 +866,20 @@ def create_course( if block_fields is not None: root_block['fields'].update(block_fields) if definition_fields is not None: - definition = self.definitions.find_one({'_id': root_block['definition']}) + definition = self.db_connection.get_definition(root_block['definition']) definition['fields'].update(definition_fields) definition['edit_info']['previous_version'] = definition['_id'] definition['edit_info']['edited_by'] = user_id definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) definition['_id'] = ObjectId() - self.definitions.insert(definition) + self.db_connection.insert_definition(definition) root_block['definition'] = definition['_id'] root_block['edit_info']['edited_on'] = datetime.datetime.now(UTC) root_block['edit_info']['edited_by'] = user_id root_block['edit_info']['previous_version'] = root_block['edit_info'].get('update_version') root_block['edit_info']['update_version'] = new_id - self.structures.insert(draft_structure) + self.db_connection.insert_structure(draft_structure) versions_dict[master_branch] = new_id # create the index entry @@ -926,7 +894,7 @@ def create_course( 'edited_by': user_id, 'edited_on': datetime.datetime.now(UTC), 'versions': versions_dict} - self.course_index.insert(index_entry) + self.db_connection.insert_course_index(index_entry) return self.get_course(CourseLocator(course_id=new_id, branch=master_branch)) def update_item(self, descriptor, user_id, force=False): @@ -978,7 +946,7 @@ def update_item(self, descriptor, user_id, force=False): 'previous_version': block_data['edit_info']['update_version'], 'update_version': new_id, } - self.structures.insert(new_structure) + self.db_connection.insert_structure(new_structure) # update the index entry if appropriate if index_entry is not None: self._update_head(index_entry, descriptor.location.branch, new_id) @@ -1016,7 +984,7 @@ def persist_xblock_dag(self, xblock, user_id, force=False): is_updated = self._persist_subdag(xblock, user_id, new_structure['blocks'], new_id) if is_updated: - self.structures.insert(new_structure) + self.db_connection.insert_structure(new_structure) # update the index entry if appropriate if index_entry is not None: @@ -1115,31 +1083,18 @@ def update_metadata(self, location, metadata): '''Deprecated, use update_item.''' raise NotImplementedError('use update_item') - def update_course_index(self, course_locator, new_values_dict, update_versions=False): + def update_course_index(self, updated_index_entry): """ - Change the given course's index entry for the given fields. new_values_dict - should be a subset of the dict returned by get_course_index_info. - It cannot include '_id' (will raise IllegalArgument). - Provide update_versions=True if you intend this to replace the versions hash. - Note, this operation can be dangerous and break running courses. + Change the given course's index entry. - If the dict includes versions and not update_versions, it will raise an exception. - - If the dict includes edited_on or edited_by, it will raise an exception + Note, this operation can be dangerous and break running courses. Does not return anything useful. """ # TODO how should this log the change? edited_on and edited_by for this entry # has the semantic of who created the course and when; so, changing those will lose # that information. - if '_id' in new_values_dict: - raise ValueError("Cannot override _id") - if 'edited_on' in new_values_dict or 'edited_by' in new_values_dict: - raise ValueError("Cannot set edited_on or edited_by") - if not update_versions and 'versions' in new_values_dict: - raise ValueError("Cannot override versions without setting update_versions") - self.course_index.update({'_id': course_locator.course_id}, - {'$set': new_values_dict}) + self.db_connection.update_course_index(updated_index_entry) def delete_item(self, usage_locator, user_id, delete_children=False, force=False): """ @@ -1182,7 +1137,7 @@ def remove_subtree(usage_id): remove_subtree(usage_locator.usage_id) # update index if appropriate and structures - self.structures.insert(new_structure) + self.db_connection.insert_structure(new_structure) result = CourseLocator(version_guid=new_id) @@ -1204,11 +1159,11 @@ def delete_course(self, course_id): :param course_id: uses course_id rather than locator to emphasize its global effect """ - index = self.course_index.find_one({'_id': course_id}) + index = self.db_connection.get_course_index(course_id) if index is None: raise ItemNotFoundError(course_id) # this is the only real delete in the system. should it do something else? - self.course_index.remove(index['_id']) + self.db_connection.delete_course_index(index['_id']) def get_errored_courses(self): """ @@ -1296,7 +1251,7 @@ def internal_clean_children(self, course_locator): block['fields']["children"] = [ usage_id for usage_id in block['fields']["children"] if usage_id in original_structure['blocks'] ] - self.structures.update({'_id': original_structure['_id']}, original_structure) + self.db_connection.update_structure(original_structure) # clear cache again b/c inheritance may be wrong over orphans self._clear_cache(original_structure['_id']) @@ -1379,7 +1334,7 @@ def _get_index_if_valid(self, locator, force=False, continue_version=False): else: return None else: - index_entry = self.course_index.find_one({'_id': locator.course_id}) + index_entry = self.db_connection.get_course_index(locator.course_id) is_head = ( locator.version_guid is None or index_entry['versions'][locator.branch] == locator.version_guid @@ -1424,9 +1379,8 @@ def _update_head(self, index_entry, branch, new_id): :param course_locator: :param new_id: """ - self.course_index.update( - {"_id": index_entry["_id"]}, - {"$set": {"versions.{}".format(branch): new_id}}) + index_entry['versions'][branch] = new_id + self.db_connection.update_course_index(index_entry) def _partition_fields_by_scope(self, category, fields): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 2a1c16599361..2fbff1042389 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py @@ -274,7 +274,9 @@ def test_translate_locator(self): course_id=prob_locator.course_id, branch='draft', usage_id=prob_locator.usage_id ) prob_location = loc_mapper().translate_locator_to_location(prob_locator) - self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', 'draft')) + # Even though the problem was set as draft, we always return revision=None to work + # with old mongo/draft modulestores. + self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', None)) prob_locator = BlockUsageLocator( course_id=new_style_course_id, usage_id='problem2', branch='production' ) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py index 049fbd2ef800..74975f28961b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py @@ -59,9 +59,9 @@ def tearDown(self): dbref = self.loc_mapper.db dbref.drop_collection(self.loc_mapper.location_map) split_db = self.split_mongo.db - split_db.drop_collection(split_db.course_index) - split_db.drop_collection(split_db.structures) - split_db.drop_collection(split_db.definitions) + split_db.drop_collection(self.split_mongo.db_connection.course_index) + split_db.drop_collection(self.split_mongo.db_connection.structures) + split_db.drop_collection(self.split_mongo.db_connection.definitions) # old_mongo doesn't give a db attr, but all of the dbs are the same dbref.drop_collection(self.old_mongo.collection) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index aa095d8e4c4a..92de35e39f78 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -1018,41 +1018,29 @@ def test_update_course_index(self): Test changing the org, pretty id, etc of a course. Test that it doesn't allow changing the id, etc. """ locator = CourseLocator(course_id="GreekHero", branch='draft') - modulestore().update_course_index(locator, {'org': 'funkyU'}) + course_info = modulestore().get_course_index_info(locator) + course_info['org'] = 'funkyU' + modulestore().update_course_index(course_info) course_info = modulestore().get_course_index_info(locator) self.assertEqual(course_info['org'], 'funkyU') - modulestore().update_course_index(locator, {'org': 'moreFunky', 'prettyid': 'Ancient Greek Demagods'}) + course_info['org'] = 'moreFunky' + course_info['prettyid'] = 'Ancient Greek Demagods' + modulestore().update_course_index(course_info) course_info = modulestore().get_course_index_info(locator) self.assertEqual(course_info['org'], 'moreFunky') self.assertEqual(course_info['prettyid'], 'Ancient Greek Demagods') - self.assertRaises(ValueError, modulestore().update_course_index, locator, {'_id': 'funkygreeks'}) - - with self.assertRaises(ValueError): - modulestore().update_course_index( - locator, - {'edited_on': datetime.datetime.now(UTC)} - ) - with self.assertRaises(ValueError): - modulestore().update_course_index( - locator, - {'edited_by': 'sneak'} - ) - - self.assertRaises(ValueError, modulestore().update_course_index, locator, - {'versions': {'draft': self.GUID_D1}}) - # an allowed but not necessarily recommended way to revert the draft version versions = course_info['versions'] versions['draft'] = self.GUID_D1 - modulestore().update_course_index(locator, {'versions': versions}, update_versions=True) + modulestore().update_course_index(course_info) course = modulestore().get_course(locator) self.assertEqual(str(course.location.version_guid), self.GUID_D1) # an allowed but not recommended way to publish a course versions['published'] = self.GUID_D1 - modulestore().update_course_index(locator, {'versions': versions}, update_versions=True) + modulestore().update_course_index(course_info) course = modulestore().get_course(CourseLocator(course_id=locator.course_id, branch="published")) self.assertEqual(str(course.location.version_guid), self.GUID_D1) @@ -1068,9 +1056,9 @@ def test_create_with_root(self): self.assertEqual(new_course.location.usage_id, 'top') self.assertEqual(new_course.category, 'chapter') # look at db to verify - db_structure = modulestore().structures.find_one({ - '_id': new_course.location.as_object_id(new_course.location.version_guid) - }) + db_structure = modulestore().db_connection.get_structure( + new_course.location.as_object_id(new_course.location.version_guid) + ) self.assertIsNotNone(db_structure, "Didn't find course") self.assertNotIn('course', db_structure['blocks']) self.assertIn('top', db_structure['blocks']) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 5512af21431d..9d055356fcb3 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -79,7 +79,7 @@ def make_name_unique(xml_data): # tags that really need unique names--they store (or should store) state. need_uniq_names = ('problem', 'sequential', 'video', 'course', 'chapter', - 'videosequence', 'poll_question', 'timelimit') + 'videosequence', 'poll_question', 'vertical') attr = xml_data.attrib tag = xml_data.tag diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index 6e33a54323f8..2b5efcb87f12 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -97,16 +97,15 @@ def export_to_xml(modulestore, contentstore, course_location, root_dir, course_d if len(draft_verticals) > 0: draft_course_dir = export_fs.makeopendir('drafts') for draft_vertical in draft_verticals: - if getattr(draft_vertical, 'is_draft', False): - parent_locs = draft_modulestore.get_parent_locations(draft_vertical.location, course.location.course_id) - # Don't try to export orphaned items. - if len(parent_locs) > 0: - logging.debug('parent_locs = {0}'.format(parent_locs)) - draft_vertical.xml_attributes['parent_sequential_url'] = Location(parent_locs[0]).url() - sequential = modulestore.get_item(Location(parent_locs[0])) - index = sequential.children.index(draft_vertical.location.url()) - draft_vertical.xml_attributes['index_in_children_list'] = str(index) - draft_vertical.export_to_xml(draft_course_dir) + parent_locs = draft_modulestore.get_parent_locations(draft_vertical.location, course.location.course_id) + # Don't try to export orphaned items. + if len(parent_locs) > 0: + logging.debug('parent_locs = {0}'.format(parent_locs)) + draft_vertical.xml_attributes['parent_sequential_url'] = Location(parent_locs[0]).url() + sequential = modulestore.get_item(Location(parent_locs[0])) + index = sequential.children.index(draft_vertical.location.url()) + draft_vertical.xml_attributes['index_in_children_list'] = str(index) + draft_vertical.export_to_xml(draft_course_dir) def export_extra_content(export_fs, modulestore, course_id, course_location, category_type, dirname, file_suffix=''): diff --git a/common/lib/xmodule/xmodule/tests/rendering/__init__.py b/common/lib/xmodule/xmodule/tests/rendering/__init__.py index 9a3a52262ecf..d93c168c8c58 100644 --- a/common/lib/xmodule/xmodule/tests/rendering/__init__.py +++ b/common/lib/xmodule/xmodule/tests/rendering/__init__.py @@ -1,2 +1 @@ import core -import xmodule_asserts \ No newline at end of file diff --git a/common/lib/xmodule/xmodule/tests/rendering/xmodule_asserts.py b/common/lib/xmodule/xmodule/tests/rendering/xmodule_asserts.py deleted file mode 100644 index fa4dd66b06d9..000000000000 --- a/common/lib/xmodule/xmodule/tests/rendering/xmodule_asserts.py +++ /dev/null @@ -1,31 +0,0 @@ -""" -View assertion functions for XModules -""" - -from __future__ import absolute_import - -from nose.tools import assert_equals, assert_not_equals # pylint: disable=no-name-in-module - -from xmodule.timelimit_module import TimeLimitModule, TimeLimitDescriptor - -from xmodule.tests.rendering.core import assert_student_view_valid_html, assert_student_view_invalid_html - - -@assert_student_view_valid_html.register(TimeLimitModule) -@assert_student_view_valid_html.register(TimeLimitDescriptor) -def _(block, html): - """ - Assert that a TimeLimitModule renders student_view html correctly - """ - assert_not_equals(0, block.get_display_items()) - assert_student_view_valid_html(block.get_children()[0], html) - - -@assert_student_view_invalid_html.register(TimeLimitModule) -@assert_student_view_invalid_html.register(TimeLimitDescriptor) -def _(block, html): - """ - Assert that a TimeLimitModule renders student_view html correctly - """ - assert_equals(0, len(block.get_display_items())) - assert_equals(u"", html) diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 04a4e84f73cd..c1f4bb2ee2e5 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -92,6 +92,16 @@ def test_unique_url_names(self): self.assertNotEqual(descriptor1.location, descriptor2.location) + # Check that each vertical gets its very own url_name + bad_xml = '''''' + bad_xml2 = '''''' + + descriptor1 = system.process_xml(bad_xml) + descriptor2 = system.process_xml(bad_xml2) + + self.assertNotEqual(descriptor1.location, descriptor2.location) + + def test_reimport(self): '''Make sure an already-exported error xml tag loads properly''' diff --git a/common/lib/xmodule/xmodule/timelimit_module.py b/common/lib/xmodule/xmodule/timelimit_module.py deleted file mode 100644 index 73744b5e8bd7..000000000000 --- a/common/lib/xmodule/xmodule/timelimit_module.py +++ /dev/null @@ -1,136 +0,0 @@ -import logging - -from lxml import etree -from time import time - -from xmodule.editing_module import XMLEditingDescriptor -from xmodule.xml_module import XmlDescriptor -from xmodule.x_module import XModule -from xmodule.progress import Progress -from xmodule.exceptions import NotFoundError -from xblock.fields import Float, String, Boolean, Scope -from xblock.fragment import Fragment - - -log = logging.getLogger(__name__) - - -class TimeLimitFields(object): - has_children = True - - beginning_at = Float(help="The time this timer was started", scope=Scope.user_state) - ending_at = Float(help="The time this timer will end", scope=Scope.user_state) - accomodation_code = String(help="A code indicating accommodations to be given the student", scope=Scope.user_state) - time_expired_redirect_url = String(help="Url to redirect users to after the timelimit has expired", scope=Scope.settings) - duration = Float(help="The length of this timer", scope=Scope.settings) - suppress_toplevel_navigation = Boolean(help="Whether the toplevel navigation should be suppressed when viewing this module", scope=Scope.settings) - - -class TimeLimitModule(TimeLimitFields, XModule): - ''' - Wrapper module which imposes a time constraint for the completion of its child. - ''' - - # For a timed activity, we are only interested here - # in time-related accommodations, and these should be disjoint. - # (For proctored exams, it is possible to have multiple accommodations - # apply to an exam, so they require accommodating a multi-choice.) - TIME_ACCOMMODATION_CODES = (('NONE', 'No Time Accommodation'), - ('ADDHALFTIME', 'Extra Time - 1 1/2 Time'), - ('ADD30MIN', 'Extra Time - 30 Minutes'), - ('DOUBLE', 'Extra Time - Double Time'), - ('TESTING', 'Extra Time -- Large amount for testing purposes') - ) - - def _get_accommodated_duration(self, duration): - ''' - Get duration for activity, as adjusted for accommodations. - Input and output are expressed in seconds. - ''' - if self.accommodation_code is None or self.accommodation_code == 'NONE': - return duration - elif self.accommodation_code == 'ADDHALFTIME': - # TODO: determine what type to return - return int(duration * 1.5) - elif self.accommodation_code == 'ADD30MIN': - return (duration + (30 * 60)) - elif self.accommodation_code == 'DOUBLE': - return (duration * 2) - elif self.accommodation_code == 'TESTING': - # when testing, set timer to run for a week at a time. - return 3600 * 24 * 7 - - @property - def has_begun(self): - return self.beginning_at is not None - - @property - def has_ended(self): - if not self.ending_at: - return False - return self.ending_at < time() - - def begin(self, duration): - ''' - Sets the starting time and ending time for the activity, - based on the duration provided (in seconds). - ''' - self.beginning_at = time() - modified_duration = self._get_accommodated_duration(duration) - self.ending_at = self.beginning_at + modified_duration - - def get_remaining_time_in_ms(self): - return int((self.ending_at - time()) * 1000) - - def student_view(self, context): - # assumes there is one and only one child, so it only renders the first child - children = self.get_display_items() - if children: - child = children[0] - return child.render('student_view', context) - else: - return Fragment() - - def get_progress(self): - ''' Return the total progress, adding total done and total available. - (assumes that each submodule uses the same "units" for progress.) - ''' - # TODO: Cache progress or children array? - children = self.get_children() - progresses = [child.get_progress() for child in children] - progress = reduce(Progress.add_counts, progresses) - return progress - - def handle_ajax(self, _dispatch, _data): - raise NotFoundError('Unexpected dispatch type') - - def get_icon_class(self): - children = self.get_children() - if children: - return children[0].get_icon_class() - else: - return "other" - -class TimeLimitDescriptor(TimeLimitFields, XMLEditingDescriptor, XmlDescriptor): - - module_class = TimeLimitModule - - @classmethod - def definition_from_xml(cls, xml_object, system): - children = [] - for child in xml_object: - try: - children.append(system.process_xml(etree.tostring(child, encoding='unicode')).location.url()) - except Exception as e: - log.exception("Unable to load child when parsing TimeLimit wrapper. Continuing...") - if system.error_tracker is not None: - system.error_tracker("ERROR: " + str(e)) - continue - return {}, children - - def definition_to_xml(self, resource_fs): - xml_object = etree.Element('timelimit') - for child in self.get_children(): - xml_object.append( - etree.fromstring(child.export_to_xml(resource_fs))) - return xml_object diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index c64f1e0be3dd..e692dfb21ca4 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -3,7 +3,6 @@ import logging import os import sys -from collections import namedtuple from lxml import etree from xblock.fields import Dict, Scope, ScopeIds @@ -133,15 +132,12 @@ class XmlDescriptor(XModuleDescriptor): 'ispublic', # if True, then course is listed for all users; see 'xqa_key', # for xqaa server access 'giturl', # url of git server for origin of file - # information about testcenter exams is a dict (of dicts), not a string, - # so it cannot be easily exportable as a course element's attribute. - 'testcenter_info', # VS[compat] Remove once unused. 'name', 'slug') metadata_to_strip = ('data_dir', 'tabs', 'grading_policy', 'published_by', 'published_date', - 'discussion_blackouts', 'testcenter_info', + 'discussion_blackouts', # VS[compat] -- remove the below attrs once everything is in the CMS 'course', 'org', 'url_name', 'filename', # Used for storing xml attributes between import and export, for roundtrips diff --git a/common/static/coffee/src/discussion/discussion.coffee b/common/static/coffee/src/discussion/discussion.coffee index 5a52cd4de016..954dd8012944 100644 --- a/common/static/coffee/src/discussion/discussion.coffee +++ b/common/static/coffee/src/discussion/discussion.coffee @@ -25,9 +25,8 @@ if Backbone? @add model model - retrieveAnotherPage: (mode, options={}, sort_options={})-> - @current_page += 1 - data = { page: @current_page } + retrieveAnotherPage: (mode, options={}, sort_options={}, error=null)-> + data = { page: @current_page + 1 } switch mode when 'search' url = DiscussionUtil.urlFor 'search' @@ -59,6 +58,7 @@ if Backbone? @reset new_collection @pages = response.num_pages @current_page = response.page + error: error sortByDate: (thread) -> # diff --git a/common/static/coffee/src/discussion/discussion_module_view.coffee b/common/static/coffee/src/discussion/discussion_module_view.coffee index 3dde9bf950ad..e23215561e80 100644 --- a/common/static/coffee/src/discussion/discussion_module_view.coffee +++ b/common/static/coffee/src/discussion/discussion_module_view.coffee @@ -2,6 +2,8 @@ if Backbone? class @DiscussionModuleView extends Backbone.View events: "click .discussion-show": "toggleDiscussion" + "keypress .discussion-show": + (event) -> DiscussionUtil.activateOnEnter(event, toggleDiscussion) "click .new-post-btn": "toggleNewPost" "click .new-post-cancel": "hideNewPost" "click .discussion-paginator a": "navigateToPage" @@ -36,12 +38,15 @@ if Backbone? event.preventDefault() @newPostForm.slideUp(300) + hideDiscussion: -> + @$("section.discussion").slideUp() + @toggleDiscussionBtn.removeClass('shown') + @toggleDiscussionBtn.find('.button-text').html("Show Discussion") + @showed = false + toggleDiscussion: (event) -> if @showed - @$("section.discussion").slideUp() - @toggleDiscussionBtn.removeClass('shown') - @toggleDiscussionBtn.find('.button-text').html("Show Discussion") - @showed = false + @hideDiscussion() else @toggleDiscussionBtn.addClass('shown') @toggleDiscussionBtn.find('.button-text').html("Hide Discussion") @@ -51,20 +56,31 @@ if Backbone? @showed = true else $elem = @toggleDiscussionBtn - @loadPage $elem + @loadPage( + $elem, + => + @hideDiscussion() + DiscussionUtil.discussionAlert( + "Sorry", + "We had some trouble loading the discussion. Please try again." + ) + ) - loadPage: ($elem)=> + loadPage: ($elem, error) => discussionId = @$el.data("discussion-id") url = DiscussionUtil.urlFor('retrieve_discussion', discussionId) + "?page=#{@page}" DiscussionUtil.safeAjax $elem: $elem $loading: $elem + takeFocus: true url: url type: "GET" dataType: 'json' success: (response, textStatus, jqXHR) => @renderDiscussion($elem, response, textStatus, discussionId) + error: error renderDiscussion: ($elem, response, textStatus, discussionId) => + $elem.focus() window.user = new DiscussionUser(response.user_info) Content.loadContentInfos(response.annotated_content_info) DiscussionUtil.loadRoles(response.roles) @@ -131,5 +147,14 @@ if Backbone? navigateToPage: (event) => event.preventDefault() window.history.pushState({}, window.document.title, event.target.href) + currPage = @page @page = $(event.target).data('page-number') - @loadPage($(event.target)) + @loadPage( + $(event.target), + => + @page = currPage + DiscussionUtil.discussionAlert( + "Sorry", + "We had some trouble loading the threads you requested. Please try again." + ) + ) diff --git a/common/static/coffee/src/discussion/utils.coffee b/common/static/coffee/src/discussion/utils.coffee index 73cfde8a0672..466beff4191b 100644 --- a/common/static/coffee/src/discussion/utils.coffee +++ b/common/static/coffee/src/discussion/utils.coffee @@ -2,9 +2,12 @@ $ -> if !window.$$contents window.$$contents = {} $.fn.extend - loading: -> - @$_loading = $("
    Loading content
    ") + loading: (takeFocus) -> + @$_loading = $("
    Loading content
    ") $(this).after(@$_loading) + if takeFocus + DiscussionUtil.makeFocusTrap(@$_loading) + @$_loading.focus() loaded: -> @$_loading.remove() @@ -87,6 +90,18 @@ class @DiscussionUtil "notifications_status" : "/notification_prefs/status" }[name] + @activateOnEnter: (event, func) -> + if event.which == 13 + e.preventDefault() + func(event) + + @makeFocusTrap: (elem) -> + elem.keydown( + (event) -> + if event.which == 9 # Tab + event.preventDefault() + ) + @discussionAlert: (header, body) -> if $("#discussion-alert").length == 0 alertDiv = $("" ) - # Capture focus - alertDiv.find("button").keydown( - (event) -> - if event.which == 9 # Tab - event.preventDefault() - ) + @makeFocusTrap(alertDiv.find("button")) alertTrigger = $("").css("display", "none") alertTrigger.leanModal({closeButton: "#discussion-alert .dismiss", overlay: 1, top: 200}) $("body").append(alertDiv).append(alertTrigger) @@ -125,7 +135,7 @@ class @DiscussionUtil if params["loadingCallback"]? params["loadingCallback"].apply(params["$loading"]) else - params["$loading"].loading() + params["$loading"].loading(params["takeFocus"]) if !params["error"] params["error"] = => @discussionAlert( diff --git a/common/static/coffee/src/discussion/views/discussion_content_view.coffee b/common/static/coffee/src/discussion/views/discussion_content_view.coffee index c69595589720..9c3c4a01f53e 100644 --- a/common/static/coffee/src/discussion/views/discussion_content_view.coffee +++ b/common/static/coffee/src/discussion/views/discussion_content_view.coffee @@ -4,7 +4,8 @@ if Backbone? events: "click .discussion-flag-abuse": "toggleFlagAbuse" - "keypress .discussion-flag-abuse": "toggleFlagAbuseKeypress" + "keypress .discussion-flag-abuse": + (event) -> DiscussionUtil.activateOnEnter(event, toggleFlagAbuse) attrRenderer: endorsed: (endorsed) -> @@ -106,11 +107,6 @@ if Backbone? @model.bind('change', @renderPartialAttrs, @) - toggleFollowingKeypress: (event) -> - # Activate on spacebar or enter - if event.which == 32 or event.which == 13 - @toggleFollowing(event) - toggleFollowing: (event) -> event.preventDefault() $elem = $(event.target) @@ -126,11 +122,6 @@ if Backbone? url: url type: "POST" - toggleFlagAbuseKeypress: (event) -> - # Activate on spacebar or enter - if event.which == 32 or event.which == 13 - @toggleFlagAbuse(event) - toggleFlagAbuse: (event) -> event.preventDefault() if window.user.id in @model.get("abuse_flaggers") or (DiscussionUtil.isFlagModerator and @model.get("abuse_flaggers").length > 0) diff --git a/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee b/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee index 5a1e3fa9ae12..57385c15bd7e 100644 --- a/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee +++ b/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee @@ -124,8 +124,11 @@ if Backbone? loadMorePages: (event) -> if event event.preventDefault() - @$(".more-pages").html('
    Loading more threads
    ') + @$(".more-pages").html('
    Loading more threads
    ') @$(".more-pages").addClass("loading") + loadingDiv = @$(".more-pages .loading-animation") + DiscussionUtil.makeFocusTrap(loadingDiv) + loadingDiv.focus() options = {} switch @mode when 'search' @@ -156,7 +159,11 @@ if Backbone? $(".post-list a").first()?.focus() ) - @collection.retrieveAnotherPage(@mode, options, {sort_key: @sortBy}) + error = => + @renderThreads() + DiscussionUtil.discussionAlert("Sorry", "We had some trouble loading more threads. Please try again.") + + @collection.retrieveAnotherPage(@mode, options, {sort_key: @sortBy}, error) renderThread: (thread) => content = $(_.template($("#thread-list-item-template").html())(thread.toJSON())) diff --git a/common/static/coffee/src/discussion/views/discussion_thread_profile_view.coffee b/common/static/coffee/src/discussion/views/discussion_thread_profile_view.coffee index b8cecbf39b34..7130ac555c71 100644 --- a/common/static/coffee/src/discussion/views/discussion_thread_profile_view.coffee +++ b/common/static/coffee/src/discussion/views/discussion_thread_profile_view.coffee @@ -4,7 +4,8 @@ if Backbone? events: "click .discussion-vote": "toggleVote" "click .action-follow": "toggleFollowing" - "keypress .action-follow": "toggleFollowingKeypress" + "keypress .action-follow": + (event) -> DiscussionUtil.activateOnEnter(event, toggleFollowing) "click .expand-post": "expandPost" "click .collapse-post": "collapsePost" diff --git a/common/static/coffee/src/discussion/views/discussion_thread_show_view.coffee b/common/static/coffee/src/discussion/views/discussion_thread_show_view.coffee index b5f478a8afb9..1a3f8929e131 100644 --- a/common/static/coffee/src/discussion/views/discussion_thread_show_view.coffee +++ b/common/static/coffee/src/discussion/views/discussion_thread_show_view.coffee @@ -4,10 +4,12 @@ if Backbone? events: "click .discussion-vote": "toggleVote" "click .discussion-flag-abuse": "toggleFlagAbuse" - "keypress .discussion-flag-abuse": "toggleFlagAbuseKeypress" + "keypress .discussion-flag-abuse": + (event) -> DiscussionUtil.activateOnEnter(event, toggleFlagAbuse) "click .admin-pin": "togglePin" "click .action-follow": "toggleFollowing" - "keypress .action-follow": "toggleFollowingKeypress" + "keypress .action-follow": + (event) -> DiscussionUtil.activateOnEnter(event, toggleFollowing) "click .action-edit": "edit" "click .action-delete": "_delete" "click .action-openclose": "toggleClosed" diff --git a/common/static/coffee/src/discussion/views/thread_response_show_view.coffee b/common/static/coffee/src/discussion/views/thread_response_show_view.coffee index aaff4a4a3911..eaed0568c2dd 100644 --- a/common/static/coffee/src/discussion/views/thread_response_show_view.coffee +++ b/common/static/coffee/src/discussion/views/thread_response_show_view.coffee @@ -6,7 +6,8 @@ if Backbone? "click .action-delete": "_delete" "click .action-edit": "edit" "click .discussion-flag-abuse": "toggleFlagAbuse" - "keypress .discussion-flag-abuse": "toggleFlagAbuseKeypress" + "keypress .discussion-flag-abuse": + (event) -> DiscussionUtil.activateOnEnter(event, toggleFlagAbuse) $: (selector) -> @$el.find(selector) diff --git a/common/static/js/capa/choicetextinput.js b/common/static/js/capa/choicetextinput.js index 4d7540f93807..514e3f67f5e3 100644 --- a/common/static/js/capa/choicetextinput.js +++ b/common/static/js/capa/choicetextinput.js @@ -1,13 +1,13 @@ (function () { var update = function () { // Whenever a value changes create a new serialized version of this - // problem's inputs and set the hidden input fields value to equal it. - var parent = $(this).closest('.problems-wrapper'); + // problem's inputs and set the hidden input field's value to equal it. + var parent = $(this).closest('section.choicetextinput'); // find the closest parent problems-wrapper and use that as the problem // grab the input id from the input // real_input is the hidden input field var real_input = $('input.choicetextvalue', parent); - var all_inputs = $('.choicetextinput .ctinput', parent); + var all_inputs = $('input.ctinput', parent); var user_inputs = {}; $(all_inputs).each(function (index, elt) { var node = $(elt); diff --git a/common/static/js/vendor/backbone-min.js b/common/static/js/vendor/backbone-min.js index fe80cd78df3f..bce4fbc1b163 100644 --- a/common/static/js/vendor/backbone-min.js +++ b/common/static/js/vendor/backbone-min.js @@ -1,4 +1 @@ (function(){var t=this;var e=t.Backbone;var i=[];var r=i.push;var s=i.slice;var n=i.splice;var a;if(typeof exports!=="undefined"){a=exports}else{a=t.Backbone={}}a.VERSION="1.0.0";var h=t._;if(!h&&typeof require!=="undefined")h=require("underscore");a.$=t.jQuery||t.Zepto||t.ender||t.$;a.noConflict=function(){t.Backbone=e;return this};a.emulateHTTP=false;a.emulateJSON=false;var o=a.Events={on:function(t,e,i){if(!l(this,"on",t,[e,i])||!e)return this;this._events||(this._events={});var r=this._events[t]||(this._events[t]=[]);r.push({callback:e,context:i,ctx:i||this});return this},once:function(t,e,i){if(!l(this,"once",t,[e,i])||!e)return this;var r=this;var s=h.once(function(){r.off(t,s);e.apply(this,arguments)});s._callback=e;return this.on(t,s,i)},off:function(t,e,i){var r,s,n,a,o,u,c,f;if(!this._events||!l(this,"off",t,[e,i]))return this;if(!t&&!e&&!i){this._events={};return this}a=t?[t]:h.keys(this._events);for(o=0,u=a.length;o").attr(t);this.setElement(e,false)}else{this.setElement(h.result(this,"el"),false)}}});a.sync=function(t,e,i){var r=k[t];h.defaults(i||(i={}),{emulateHTTP:a.emulateHTTP,emulateJSON:a.emulateJSON});var s={type:r,dataType:"json"};if(!i.url){s.url=h.result(e,"url")||U()}if(i.data==null&&e&&(t==="create"||t==="update"||t==="patch")){s.contentType="application/json";s.data=JSON.stringify(i.attrs||e.toJSON(i))}if(i.emulateJSON){s.contentType="application/x-www-form-urlencoded";s.data=s.data?{model:s.data}:{}}if(i.emulateHTTP&&(r==="PUT"||r==="DELETE"||r==="PATCH")){s.type="POST";if(i.emulateJSON)s.data._method=r;var n=i.beforeSend;i.beforeSend=function(t){t.setRequestHeader("X-HTTP-Method-Override",r);if(n)return n.apply(this,arguments)}}if(s.type!=="GET"&&!i.emulateJSON){s.processData=false}if(s.type==="PATCH"&&window.ActiveXObject&&!(window.external&&window.external.msActiveXFilteringEnabled)){s.xhr=function(){return new ActiveXObject("Microsoft.XMLHTTP")}}var o=i.xhr=a.ajax(h.extend(s,i));e.trigger("request",e,o,i);return o};var k={create:"POST",update:"PUT",patch:"PATCH","delete":"DELETE",read:"GET"};a.ajax=function(){return a.$.ajax.apply(a.$,arguments)};var S=a.Router=function(t){t||(t={});if(t.routes)this.routes=t.routes;this._bindRoutes();this.initialize.apply(this,arguments)};var $=/\((.*?)\)/g;var T=/(\(\?)?:\w+/g;var H=/\*\w+/g;var A=/[\-{}\[\]+?.,\\\^$|#\s]/g;h.extend(S.prototype,o,{initialize:function(){},route:function(t,e,i){if(!h.isRegExp(t))t=this._routeToRegExp(t);if(h.isFunction(e)){i=e;e=""}if(!i)i=this[e];var r=this;a.history.route(t,function(s){var n=r._extractParameters(t,s);i&&i.apply(r,n);r.trigger.apply(r,["route:"+e].concat(n));r.trigger("route",e,n);a.history.trigger("route",r,e,n)});return this},navigate:function(t,e){a.history.navigate(t,e);return this},_bindRoutes:function(){if(!this.routes)return;this.routes=h.result(this,"routes");var t,e=h.keys(this.routes);while((t=e.pop())!=null){this.route(t,this.routes[t])}},_routeToRegExp:function(t){t=t.replace(A,"\\$&").replace($,"(?:$1)?").replace(T,function(t,e){return e?t:"([^/]+)"}).replace(H,"(.*?)");return new RegExp("^"+t+"$")},_extractParameters:function(t,e){var i=t.exec(e).slice(1);return h.map(i,function(t){return t?decodeURIComponent(t):null})}});var I=a.History=function(){this.handlers=[];h.bindAll(this,"checkUrl");if(typeof window!=="undefined"){this.location=window.location;this.history=window.history}};var N=/^[#\/]|\s+$/g;var P=/^\/+|\/+$/g;var O=/msie [\w.]+/;var C=/\/$/;I.started=false;h.extend(I.prototype,o,{interval:50,getHash:function(t){var e=(t||this).location.href.match(/#(.*)$/);return e?e[1]:""},getFragment:function(t,e){if(t==null){if(this._hasPushState||!this._wantsHashChange||e){t=this.location.pathname;var i=this.root.replace(C,"");if(!t.indexOf(i))t=t.substr(i.length)}else{t=this.getHash()}}return t.replace(N,"")},start:function(t){if(I.started)throw new Error("Backbone.history has already been started");I.started=true;this.options=h.extend({},{root:"/"},this.options,t);this.root=this.options.root;this._wantsHashChange=this.options.hashChange!==false;this._wantsPushState=!!this.options.pushState;this._hasPushState=!!(this.options.pushState&&this.history&&this.history.pushState);var e=this.getFragment();var i=document.documentMode;var r=O.exec(navigator.userAgent.toLowerCase())&&(!i||i<=7);this.root=("/"+this.root+"/").replace(P,"/");if(r&&this._wantsHashChange){this.iframe=a.$('
    @@ -34,16 +61,13 @@ {% trans "Save changes" %} - + {% trans "Back to editor" %}
    - + {% include "wiki/includes/cheatsheet.html" %} {% endblock %} - - - diff --git a/lms/templates/wiki/history.html b/lms/templates/wiki/history.html index 5488abb97db2..798b030c0367 100644 --- a/lms/templates/wiki/history.html +++ b/lms/templates/wiki/history.html @@ -10,7 +10,7 @@ {% endaddtoblock %} @@ -81,7 +95,7 @@ {% trans "Click each revision to see a list of edited lines. Click the Preview button to see how the article looked at this stage. At the bottom of this page, you can change to a particular revision or merge an old revision with the current one." %}

    -
    +
    {% for revision in revisions %}
    @@ -107,16 +121,29 @@
    {% if not revision == article.current_revision %} - - {% endif %} - - {% if article|can_write:user %} - + + + {% trans "Preview this revision" %} + + + {% if article|can_write:user %} + + {% endif %} + {% endif %} - +
    @@ -140,84 +167,103 @@ {% endfor %} - + {% include "wiki/includes/pagination.html" %} - + {% if revisions.count > 1 %}
    {% if article|can_write:user %} - + {% else %} {% endif %} -
    - {% endif %} - + -