From 98c66d90355fb314f147fc203c6dab27bae7d395 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 12 Feb 2014 15:59:58 -0500 Subject: [PATCH 1/5] Added get_courses_for_wiki method. LMS-2136 --- .../xmodule/xmodule/modulestore/mongo/base.py | 9 +++++++ .../xmodule/modulestore/tests/test_mongo.py | 25 ++++++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 0770852d31..587cc69753 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -878,6 +878,15 @@ class MongoModuleStore(ModuleStoreWriteBase): item_locs -= all_reachable return list(item_locs) + def get_courses_for_wiki(self, wiki_slug): + """ + Return the list of courses which use this wiki + :param wiki_slug: the wiki id + :return: list of course locations + """ + courses = self.collection.find({'definition.data.wiki_slug': wiki_slug}) + return [Location(course['_id']) for course in courses] + def _create_new_field_data(self, _category, _location, definition_data, metadata): """ To instantiate a new xmodule which will be saved latter, set up the dbModel and kvs diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index ad43c59e2e..169405e134 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -37,6 +37,9 @@ RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': '' class TestMongoModuleStore(object): '''Tests!''' + # Explicitly list the courses to load (don't want the big one) + courses = ['toy', 'simple', 'simple_with_draft', 'test_unicode'] + @classmethod def setupClass(cls): cls.connection = pymongo.MongoClient( @@ -74,9 +77,7 @@ class TestMongoModuleStore(object): # Also test draft store imports # draft_store = DraftModuleStore(doc_store_config, FS_ROOT, RENDER_TEMPLATE, default_class=DEFAULT_CLASS) - # Explicitly list the courses to load (don't want the big one) - courses = ['toy', 'simple', 'simple_with_draft', 'test_unicode'] - import_from_xml(store, DATA_DIR, courses, draft_store=draft_store, static_content_store=content_store) + import_from_xml(store, DATA_DIR, TestMongoModuleStore.courses, draft_store=draft_store, static_content_store=content_store) # also test a course with no importing of static content import_from_xml( @@ -273,6 +274,24 @@ class TestMongoModuleStore(object): {'displayname': 'hello'} ) + def test_get_courses_for_wiki(self): + """ + Test the get_courses_for_wiki method + """ + for course_id in self.courses: + courses = self.store.get_courses_for_wiki(course_id) + assert_equals(len(courses), 1) + assert_equals(Location('i4x', 'edX', course_id, 'course', '2012_Fall'), courses[0]) + course_locs = self.store.get_courses_for_wiki('no_such_wiki') + assert_equals(len(course_locs), 0) + # now set two to have same wiki + course = self.store.get_course('edX/toy/2012_Fall') + course.wiki_slug = 'simple' + self.store.save_xmodule(course) + courses = self.store.get_courses_for_wiki('simple') + assert_equals(len(courses), 2) + for course_id in ['toy', 'simple']: + assert_in(Location('i4x', 'edX', course_id, 'course', '2012_Fall'), courses) class TestMongoKeyValueStore(object): """ From 3c55c91c5a566c3b43e71f466be3b33232abca55 Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Tue, 18 Feb 2014 15:14:56 +0500 Subject: [PATCH 2/5] Added get_courses_for_wiki(self, wiki_slug) to mongo, xml and mixed modulestores. This method returns a list of courses which use the particular wiki_slug. It is used for checking permissions in the course_wiki app. LMS-2136 --- .../lib/xmodule/xmodule/modulestore/mixed.py | 11 +++++ .../xmodule/xmodule/modulestore/mongo/base.py | 4 +- .../xmodule/modulestore/split_mongo/split.py | 11 +++++ .../tests/test_mixed_modulestore.py | 18 ++++++++ .../xmodule/modulestore/tests/test_mongo.py | 46 +++++++++++++------ .../xmodule/modulestore/tests/test_xml.py | 31 +++++++++++-- common/lib/xmodule/xmodule/modulestore/xml.py | 9 ++++ 7 files changed, 110 insertions(+), 20 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 5152ea9231..a3757b42d4 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -417,6 +417,17 @@ class MixedModuleStore(ModuleStoreWriteBase): for course in store.get_courses(): loc_mapper().translate_location(course.location.course_id, course.location) + def get_courses_for_wiki(self, wiki_slug): + """ + Return the list of courses which use this wiki_slug + :param wiki_slug: the course wiki root slug + :return: list of course locations + """ + courses = [] + for modulestore in self.modulestores.values(): + courses.extend(modulestore.get_courses_for_wiki(wiki_slug)) + return courses + def _compare_stores(left, right): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 587cc69753..116a48b31c 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -880,8 +880,8 @@ class MongoModuleStore(ModuleStoreWriteBase): def get_courses_for_wiki(self, wiki_slug): """ - Return the list of courses which use this wiki - :param wiki_slug: the wiki id + Return the list of courses which use this wiki_slug + :param wiki_slug: the course wiki root slug :return: list of course locations """ courses = self.collection.find({'definition.data.wiki_slug': wiki_slug}) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index da61e061a1..cdc12655db 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -1645,3 +1645,14 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): be a json dict key. """ structure['blocks'][LocMapperStore.encode_key_for_mongo(block_id)] = content + + def get_courses_for_wiki(self, wiki_slug): + """ + Return the list of courses which use this wiki_slug + :param wiki_slug: the course wiki root slug + :return: list of course locations + + Todo: Needs to be implemented. + """ + courses = [] + return courses \ No newline at end of file diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index d2e31cb831..1bc868f982 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -368,6 +368,24 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID], None) self.assertEqual(len(orphans), 0, "unexpected orphans: {}".format(orphans)) + @ddt.data('direct') + def test_get_courses_for_wiki(self, default_ms): + """ + Test the get_courses_for_wiki method + """ + self.initdb(default_ms) + course_locations = self.store.get_courses_for_wiki('toy') + self.assertEqual(len(course_locations), 1) + self.assertIn(Location('i4x', 'edX', 'toy', 'course', '2012_Fall'), course_locations) + + course_locations = self.store.get_courses_for_wiki('simple') + self.assertEqual(len(course_locations), 1) + self.assertIn(Location('i4x', 'edX', 'simple', 'course', '2012_Fall'), course_locations) + + self.assertEqual(len(self.store.get_courses_for_wiki('edX.simple.2012_Fall')), 0) + self.assertEqual(len(self.store.get_courses_for_wiki('no_such_wiki')), 0) + + #============================================================================================================= # General utils for not using django settings #============================================================================================================= diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 169405e134..730f700bb7 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -278,20 +278,38 @@ class TestMongoModuleStore(object): """ Test the get_courses_for_wiki method """ - for course_id in self.courses: - courses = self.store.get_courses_for_wiki(course_id) - assert_equals(len(courses), 1) - assert_equals(Location('i4x', 'edX', course_id, 'course', '2012_Fall'), courses[0]) - course_locs = self.store.get_courses_for_wiki('no_such_wiki') - assert_equals(len(course_locs), 0) - # now set two to have same wiki - course = self.store.get_course('edX/toy/2012_Fall') - course.wiki_slug = 'simple' - self.store.save_xmodule(course) - courses = self.store.get_courses_for_wiki('simple') - assert_equals(len(courses), 2) - for course_id in ['toy', 'simple']: - assert_in(Location('i4x', 'edX', course_id, 'course', '2012_Fall'), courses) + for course_number in self.courses: + course_locations = self.store.get_courses_for_wiki(course_number) + assert_equals(len(course_locations), 1) + assert_equals(Location('i4x', 'edX', course_number, 'course', '2012_Fall'), course_locations[0]) + + course_locations = self.store.get_courses_for_wiki('no_such_wiki') + assert_equals(len(course_locations), 0) + + # set toy course to share the wiki with simple course + toy_course = self.store.get_course('edX/toy/2012_Fall') + toy_course.wiki_slug = 'simple' + self.store.update_item(toy_course) + + # now toy_course should not be retrievable with old wiki_slug + course_locations = self.store.get_courses_for_wiki('toy') + assert_equals(len(course_locations), 0) + + # but there should be two courses with wiki_slug 'simple' + course_locations = self.store.get_courses_for_wiki('simple') + assert_equals(len(course_locations), 2) + for course_number in ['toy', 'simple']: + assert_in(Location('i4x', 'edX', course_number, 'course', '2012_Fall'), course_locations) + + # configure simple course to use unique wiki_slug. + simple_course = self.store.get_course('edX/simple/2012_Fall') + simple_course.wiki_slug = 'edX.simple.2012_Fall' + self.store.update_item(simple_course) + # it should be retrievable with its new wiki_slug + course_locations = self.store.get_courses_for_wiki('edX.simple.2012_Fall') + assert_equals(len(course_locations), 1) + assert_in(Location('i4x', 'edX', 'simple', 'course', '2012_Fall'), course_locations) + class TestMongoKeyValueStore(object): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py index 71274aae5f..8cbdbbeffe 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py @@ -7,8 +7,6 @@ import unittest from glob import glob from mock import patch -from nose.tools import assert_raises, assert_equals # pylint: disable=E0611 - from xmodule.course_module import CourseDescriptor from xmodule.modulestore.xml import XMLModuleStore from xmodule.modulestore import Location, XML_MODULESTORE_TYPE @@ -43,7 +41,7 @@ class TestXMLModuleStore(unittest.TestCase): def test_xml_modulestore_type(self): store = XMLModuleStore(DATA_DIR, course_dirs=['toy', 'simple']) - assert_equals(store.get_modulestore_type('foo/bar/baz'), XML_MODULESTORE_TYPE) + self.assertEqual(store.get_modulestore_type('foo/bar/baz'), XML_MODULESTORE_TYPE) def test_unicode_chars_in_xml_content(self): # edX/full/6.002_Spring_2012 has non-ASCII chars, and during @@ -52,7 +50,7 @@ class TestXMLModuleStore(unittest.TestCase): # Ensure that there really is a non-ASCII character in the course. with open(os.path.join(DATA_DIR, "toy/sequential/vertical_sequential.xml")) as xmlf: xml = xmlf.read() - with assert_raises(UnicodeDecodeError): + with self.assertRaises(UnicodeDecodeError): xml.decode('ascii') # Load the course, but don't make error modules. This will succeed, @@ -78,3 +76,28 @@ class TestXMLModuleStore(unittest.TestCase): about_module = course_module[about_location] self.assertIn("GREEN", about_module.data) self.assertNotIn("RED", about_module.data) + + def test_get_courses_for_wiki(self): + """ + Test the get_courses_for_wiki method + """ + store = XMLModuleStore(DATA_DIR, course_dirs=['toy', 'simple']) + for course in store.get_courses(): + course_locations = store.get_courses_for_wiki(course.wiki_slug) + self.assertEqual(len(course_locations), 1) + self.assertIn(Location('i4x', 'edX', course.location.course, 'course', '2012_Fall'), course_locations) + + course_locations = store.get_courses_for_wiki('no_such_wiki') + self.assertEqual(len(course_locations), 0) + + # now set toy course to share the wiki with simple course + toy_course = store.get_course('edX/toy/2012_Fall') + toy_course.wiki_slug = 'simple' + + course_locations = store.get_courses_for_wiki('toy') + self.assertEqual(len(course_locations), 0) + + course_locations = store.get_courses_for_wiki('simple') + self.assertEqual(len(course_locations), 2) + for course_number in ['toy', 'simple']: + self.assertIn(Location('i4x', 'edX', course_number, 'course', '2012_Fall'), course_locations) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index cdaf446cdc..445ecee039 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -790,3 +790,12 @@ class XMLModuleStore(ModuleStoreReadBase): "split" for new-style split MongoDB backed courses. """ return XML_MODULESTORE_TYPE + + def get_courses_for_wiki(self, wiki_slug): + """ + Return the list of courses which use this wiki_slug + :param wiki_slug: the course wiki root slug + :return: list of course locations + """ + courses = self.get_courses() + return [course.location for course in courses if (course.wiki_slug == wiki_slug)] From 7b185db7514e2c8edbb36a757edb4450cfac1f05 Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Tue, 11 Feb 2014 18:17:39 +0500 Subject: [PATCH 3/5] Refactored permissions check for course_wiki pages. CourseRole names have a new format (type_org.number.run). Previously when checking if a user was staff for a course wiki type_org/number/run and type_number format role names were checked by parsing user group names. This logic has been refactored to first fetch all courses which use the particular wiki_slug and then use courseware.access.has_access to check if the user has staff permissions on any of the courses. LMS-2136 --- .../course_wiki/tests/test_access.py | 73 ++++++++++++------- lms/djangoapps/course_wiki/utils.py | 41 +++-------- 2 files changed, 57 insertions(+), 57 deletions(-) diff --git a/lms/djangoapps/course_wiki/tests/test_access.py b/lms/djangoapps/course_wiki/tests/test_access.py index aca2223d13..5d56c228d8 100644 --- a/lms/djangoapps/course_wiki/tests/test_access.py +++ b/lms/djangoapps/course_wiki/tests/test_access.py @@ -4,6 +4,7 @@ Tests for wiki permissions from django.contrib.auth.models import Group from student.tests.factories import UserFactory +from xmodule.modulestore.django import loc_mapper from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -24,21 +25,37 @@ class TestWikiAccessBase(ModuleStoreTestCase): self.wiki = get_or_create_root() - self.course_math101 = CourseFactory.create(org='org', number='math101', display_name='Course') - self.course_math101_staff = [ - InstructorFactory(course=self.course_math101.location), - StaffFactory(course=self.course_math101.location) - ] + self.course_math101 = CourseFactory.create(org='org', number='math101', display_name='Course', metadata={'use_unique_wiki_id': 'false'}) + self.course_math101_staff = self.create_staff_for_course(self.course_math101) wiki_math101 = self.create_urlpath(self.wiki, course_wiki_slug(self.course_math101)) wiki_math101_page = self.create_urlpath(wiki_math101, 'Child') wiki_math101_page_page = self.create_urlpath(wiki_math101_page, 'Grandchild') self.wiki_math101_pages = [wiki_math101, wiki_math101_page, wiki_math101_page_page] + self.course_math101b = CourseFactory.create(org='org', number='math101b', display_name='Course', metadata={'use_unique_wiki_id': 'true'}) + self.course_math101b_staff = self.create_staff_for_course(self.course_math101b) + + wiki_math101b = self.create_urlpath(self.wiki, course_wiki_slug(self.course_math101b)) + wiki_math101b_page = self.create_urlpath(wiki_math101b, 'Child') + wiki_math101b_page_page = self.create_urlpath(wiki_math101b_page, 'Grandchild') + self.wiki_math101b_pages = [wiki_math101b, wiki_math101b_page, wiki_math101b_page_page] + def create_urlpath(self, parent, slug): """Creates an article at /parent/slug and returns its URLPath""" return URLPath.create_article(parent, slug, title=slug) + def create_staff_for_course(self, course): + """Creates and returns users with instructor and staff access to course.""" + + course_locator = loc_mapper().translate_location(course.id, course.location) + return [ + InstructorFactory(course=course.location), # Creates instructor_org/number/run role name + StaffFactory(course=course.location), # Creates staff_org/number/run role name + InstructorFactory(course=course_locator), # Creates instructor_org.number.run role name + StaffFactory(course=course_locator), # Creates staff_org.number.run role name + ] + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class TestWikiAccess(TestWikiAccessBase): @@ -47,21 +64,15 @@ class TestWikiAccess(TestWikiAccessBase): super(TestWikiAccess, self).setUp() self.course_310b = CourseFactory.create(org='org', number='310b', display_name='Course') - self.course_310b_staff = [ - InstructorFactory(course=self.course_310b.location), - StaffFactory(course=self.course_310b.location) - ] - self.course_310b_ = CourseFactory.create(org='org', number='310b_', display_name='Course') - self.course_310b__staff = [ - InstructorFactory(course=self.course_310b_.location), - StaffFactory(course=self.course_310b_.location) - ] + self.course_310b_staff = self.create_staff_for_course(self.course_310b) + self.course_310b2 = CourseFactory.create(org='org', number='310b_', display_name='Course') + self.course_310b2_staff = self.create_staff_for_course(self.course_310b2) self.wiki_310b = self.create_urlpath(self.wiki, course_wiki_slug(self.course_310b)) - self.wiki_310b_ = self.create_urlpath(self.wiki, course_wiki_slug(self.course_310b_)) + self.wiki_310b2 = self.create_urlpath(self.wiki, course_wiki_slug(self.course_310b2)) def test_no_one_is_root_wiki_staff(self): - all_course_staff = self.course_math101_staff + self.course_310b_staff + self.course_310b__staff + all_course_staff = self.course_math101_staff + self.course_310b_staff + self.course_310b2_staff for course_staff in all_course_staff: self.assertFalse(user_is_article_course_staff(course_staff, self.wiki.article)) @@ -70,6 +81,10 @@ class TestWikiAccess(TestWikiAccessBase): for course_staff in self.course_math101_staff: self.assertTrue(user_is_article_course_staff(course_staff, page.article)) + for page in self.wiki_math101b_pages: + for course_staff in self.course_math101b_staff: + self.assertTrue(user_is_article_course_staff(course_staff, page.article)) + def test_settings(self): for page in self.wiki_math101_pages: for course_staff in self.course_math101_staff: @@ -79,15 +94,27 @@ class TestWikiAccess(TestWikiAccessBase): self.assertTrue(settings.CAN_ASSIGN(page.article, course_staff)) self.assertTrue(settings.CAN_ASSIGN_OWNER(page.article, course_staff)) + for page in self.wiki_math101b_pages: + for course_staff in self.course_math101b_staff: + self.assertTrue(settings.CAN_DELETE(page.article, course_staff)) + self.assertTrue(settings.CAN_MODERATE(page.article, course_staff)) + self.assertTrue(settings.CAN_CHANGE_PERMISSIONS(page.article, course_staff)) + self.assertTrue(settings.CAN_ASSIGN(page.article, course_staff)) + self.assertTrue(settings.CAN_ASSIGN_OWNER(page.article, course_staff)) + def test_other_course_staff_is_not_course_wiki_staff(self): + for page in self.wiki_math101_pages: + for course_staff in self.course_math101b_staff: + self.assertFalse(user_is_article_course_staff(course_staff, page.article)) + for page in self.wiki_math101_pages: for course_staff in self.course_310b_staff: self.assertFalse(user_is_article_course_staff(course_staff, page.article)) for course_staff in self.course_310b_staff: - self.assertFalse(user_is_article_course_staff(course_staff, self.wiki_310b_.article)) + self.assertFalse(user_is_article_course_staff(course_staff, self.wiki_310b2.article)) - for course_staff in self.course_310b__staff: + for course_staff in self.course_310b2_staff: self.assertFalse(user_is_article_course_staff(course_staff, self.wiki_310b.article)) @@ -114,10 +141,7 @@ class TestWikiAccessForNumericalCourseNumber(TestWikiAccessBase): super(TestWikiAccessForNumericalCourseNumber, self).setUp() self.course_200 = CourseFactory.create(org='org', number='200', display_name='Course') - self.course_200_staff = [ - InstructorFactory(course=self.course_200.location), - StaffFactory(course=self.course_200.location) - ] + self.course_200_staff = self.create_staff_for_course(self.course_200) wiki_200 = self.create_urlpath(self.wiki, course_wiki_slug(self.course_200)) wiki_200_page = self.create_urlpath(wiki_200, 'Child') @@ -138,10 +162,7 @@ class TestWikiAccessForOldFormatCourseStaffGroups(TestWikiAccessBase): self.course_math101c = CourseFactory.create(org='org', number='math101c', display_name='Course') Group.objects.get_or_create(name='instructor_math101c') - self.course_math101c_staff = [ - InstructorFactory(course=self.course_math101c.location), - StaffFactory(course=self.course_math101c.location) - ] + self.course_math101c_staff = self.create_staff_for_course(self.course_math101c) wiki_math101c = self.create_urlpath(self.wiki, course_wiki_slug(self.course_math101c)) wiki_math101c_page = self.create_urlpath(wiki_math101c, 'Child') diff --git a/lms/djangoapps/course_wiki/utils.py b/lms/djangoapps/course_wiki/utils.py index 2fb5412d35..9f9956a7b2 100644 --- a/lms/djangoapps/course_wiki/utils.py +++ b/lms/djangoapps/course_wiki/utils.py @@ -3,7 +3,8 @@ Utility functions for course_wiki. """ from django.core.exceptions import ObjectDoesNotExist - +from xmodule import modulestore +import courseware def user_is_article_course_staff(user, article): """ @@ -20,24 +21,24 @@ def user_is_article_course_staff(user, article): so this will return True. """ - course_slug = article_course_wiki_root_slug(article) + wiki_slug = article_course_wiki_root_slug(article) - if course_slug is None: + if wiki_slug is None: return False - user_groups = user.groups.all() - # The wiki expects article slugs to contain at least one non-digit so if # the course number is just a number the course wiki root slug is set to # be '_'. This means slug '202_' can belong to either # course numbered '202_' or '202' and so we need to consider both. - if user_is_staff_on_course_number(user_groups, course_slug): + courses = modulestore.django.modulestore().get_courses_for_wiki(wiki_slug) + if any(courseware.access.has_access(user, course, 'staff', course.course_id) for course in courses): return True - if (course_slug.endswith('_') and slug_is_numerical(course_slug[:-1]) and - user_is_staff_on_course_number(user_groups, course_slug[:-1])): - return True + if (wiki_slug.endswith('_') and slug_is_numerical(wiki_slug[:-1])): + courses = modulestore.django.modulestore().get_courses_for_wiki(wiki_slug[:-1]) + if any(courseware.access.has_access(user, course, 'staff', course.course_id) for course in courses): + return True return False @@ -64,28 +65,6 @@ def course_wiki_slug(course): return slug -def user_is_staff_on_course_number(user_groups, course_number): - """Returns whether the groups contain a staff group for the course number""" - - # Course groups have format 'instructor_' and 'staff_' where - # course_id = org/course_number/run. So check if user's groups contain a group - # whose name starts with 'instructor_' or 'staff_' and contains '/course_number/'. - course_number_fragment = '/{0}/'.format(course_number) - if [group for group in user_groups if (group.name.startswith(('instructor_', 'staff_')) and - course_number_fragment in group.name)]: - return True - - # Old course groups had format 'instructor_' and 'staff_' - # Check if user's groups contain either of these. - old_instructor_group_name = 'instructor_{0}'.format(course_number) - old_staff_group_name = 'staff_{0}'.format(course_number) - if [group for group in user_groups if (group.name == old_instructor_group_name or - group.name == old_staff_group_name)]: - return True - - return False - - def article_course_wiki_root_slug(article): """ We assume the second level ancestor is the course wiki root. Examples: From 8f3cfa5a5f9eee4c19126ed0e3b6b91710a9a87c Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Tue, 4 Mar 2014 00:32:27 +0500 Subject: [PATCH 4/5] When creating a course in studio set a unique wiki_slug for it. Currently wiki_slug is set to course number. However, since multiple courses can have the same number this may lead to clashes. So wiki_slug will be set to org.course.name. To maintain the active wiki_slugs for xml courses this cannot be changed in the CourseDescriptor. LMS-2136 --- .../contentstore/tests/test_contentstore.py | 31 +++++++++++++++++-- cms/djangoapps/contentstore/views/course.py | 7 +++++ .../xmodule/modulestore/xml_importer.py | 19 +++++++++--- .../data/two_toys/course/TT_2012_Fall.xml | 1 + 4 files changed, 51 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 300e338cbe..8c321f03dd 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1761,8 +1761,27 @@ class ContentStoreTest(ModuleStoreTestCase): self.assertEquals(course_module.pdf_textbooks[0]["chapters"][0]["url"], '/c4x/MITx/999/asset/Chapter1.pdf') self.assertEquals(course_module.pdf_textbooks[0]["chapters"][1]["url"], '/c4x/MITx/999/asset/Chapter2.pdf') - # check that URL slug got updated to new course slug - self.assertEquals(course_module.wiki_slug, '999') + def test_import_into_new_course_id_wiki_slug_renamespacing(self): + module_store = modulestore('direct') + target_location = Location('i4x', 'MITx', '999', 'course', '2013_Spring') + course_data = { + 'org': target_location.org, + 'number': target_location.course, + 'display_name': 'Robot Super Course', + 'run': target_location.name + } + target_course_id = '{0}/{1}/{2}'.format(target_location.org, target_location.course, target_location.name) + _create_course(self, course_data) + + # Import a course with wiki_slug == location.course + import_from_xml(module_store, 'common/test/data/', ['toy'], target_location_namespace=target_location) + course_module = module_store.get_instance(target_location.course_id, target_location) + self.assertEquals(course_module.wiki_slug, 'MITx.999.2013_Spring') + + # Now try importing a course with wiki_slug == '{0}{1}{2}'.format(location.org, location.course, location.name) + import_from_xml(module_store, 'common/test/data/', ['two_toys'], target_location_namespace=target_location) + course_module = module_store.get_instance(target_course_id, target_location) + self.assertEquals(course_module.wiki_slug, 'MITx.999.2013_Spring') def test_import_metadata_with_attempts_empty_string(self): module_store = modulestore('direct') @@ -1915,6 +1934,14 @@ class ContentStoreTest(ModuleStoreTestCase): _test_no_locations(self, resp) return resp + def test_wiki_slug(self): + """When creating a course a unique wiki_slug should be set.""" + + course_location = Location(['i4x', 'MITx', '999', 'course', '2013_Spring']) + _create_course(self, self.course_data) + course_module = modulestore('direct').get_item(course_location) + self.assertEquals(course_module.wiki_slug, 'MITx.999.2013_Spring') + @override_settings(MODULESTORE=TEST_MODULESTORE) class MetadataSaveTestCase(ModuleStoreTestCase): diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 138e323d1c..af7f1324de 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -376,8 +376,15 @@ def create_new_course(request): metadata = {} else: metadata = {'display_name': display_name} + + # Set a unique wiki_slug for newly created courses. To maintain active wiki_slugs for existing xml courses this + # cannot be changed in CourseDescriptor. + wiki_slug = "{0}.{1}.{2}".format(dest_location.org, dest_location.course, dest_location.name) + definition_data = {'wiki_slug': wiki_slug} + modulestore('direct').create_and_save_xmodule( dest_location, + definition_data=definition_data, metadata=metadata ) new_course = modulestore('direct').get_item(dest_location) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 61ca0b62f1..cd82f97aec 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -514,11 +514,20 @@ def remap_namespace(module, target_location_namespace): chapter['url'], target_location_namespace ) - # if there is a wiki_slug which is the same as the original location - # (aka default value), then remap that so the wiki doesn't point to - # the old Wiki. - if module.wiki_slug == original_location.course: - module.wiki_slug = target_location_namespace.course + # Original wiki_slugs had value location.course. To make them unique this was changed to 'org.course.name'. + # If the wiki_slug is equal to either of these default values then remap that so that the wiki does not point + # to the old wiki. + original_unique_wiki_slug = '{0}.{1}.{2}'.format( + original_location.org, + original_location.course, + original_location.name + ) + if module.wiki_slug == original_unique_wiki_slug or module.wiki_slug == original_location.course: + module.wiki_slug = '{0}.{1}.{2}'.format( + target_location_namespace.org, + target_location_namespace.course, + target_location_namespace.name, + ) module.save() diff --git a/common/test/data/two_toys/course/TT_2012_Fall.xml b/common/test/data/two_toys/course/TT_2012_Fall.xml index b6dbd48b4c..107612deec 100644 --- a/common/test/data/two_toys/course/TT_2012_Fall.xml +++ b/common/test/data/two_toys/course/TT_2012_Fall.xml @@ -1,3 +1,4 @@ + From d335713da5065164ada78468cef4d354b6a1bf08 Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Tue, 4 Mar 2014 18:34:24 +0500 Subject: [PATCH 5/5] Export CourseDescriptor wiki_slug field to xml. LMS-2136 --- common/lib/xmodule/xmodule/course_module.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index bb9b3508aa..78ca359de6 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -603,6 +603,11 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): xml_object.append(textbook_xml_object) + if self.wiki_slug is not None: + wiki_xml_object = etree.Element('wiki') + wiki_xml_object.set('slug', self.wiki_slug) + xml_object.append(wiki_xml_object) + return xml_object def has_ended(self):