Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from xmodule.contentstore.django import contentstore
from xmodule.course_module import CourseDescriptor
from .prompt import query_yes_no
from django_comment_common.utils import unseed_permissions_roles

from auth.authz import _delete_course_group

Expand Down Expand Up @@ -40,6 +41,9 @@ def handle(self, *args, **options):
if query_yes_no("Are you sure. This action cannot be undone!", default="no"):
loc = CourseDescriptor.id_to_location(course_id)
if delete_course(ms, cs, loc, commit):
print 'removing forums permissions and roles...'
unseed_permissions_roles(course_id)

print 'removing User permissions from course....'
# in the django layer, we need to remove all the user permissions groups associated with this course
if commit:
Expand Down
34 changes: 34 additions & 0 deletions cms/djangoapps/contentstore/tests/test_contentstore.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@
from pymongo import MongoClient
from student.models import CourseEnrollment

from django_comment_common.utils import unseed_permissions_roles

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be combined with the other import from this module

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actually, this should not be invoked directly in this test.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I do think it is worth testing the unseed_permissions_roles method in isolation, though perhaps not in this file (in addition to testing that the deletion unseeds permissions).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agreed (including that it doesn't belong in this file)


TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % uuid4().hex

Expand Down Expand Up @@ -1292,6 +1294,38 @@ def test_create_course_check_forum_seeding(self):
test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
self.assertTrue(are_permissions_roles_seeded(self._get_course_id(test_course_data)))

def test_forum_unseeding(self):
"""Test new course creation and verify forum unseeding """
test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
self.assertTrue(are_permissions_roles_seeded(self._get_course_id(test_course_data)))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Move this down and use course_id

course_id = self._get_course_id(test_course_data)
unseed_permissions_roles(course_id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe check first that permissions are seeded? Or perhaps that is well covered in another test.

If so, change the comment line to say "verify form unseeding".

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The test should be verifying that role deletion happens as part of course delettion

self.assertFalse(are_permissions_roles_seeded(course_id))

def test_forum_unseeding_with_different_casing(self):
"""Make sure that we honor case sensitivity when unseeding forums"""
test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
# make sure we don't delete a forum permissions set with different casing
# than the passed in course_id. This is because Mongo and MySQL are using different collations
course_id = self._get_course_id(test_course_data)
unseed_permissions_roles(course_id.upper())
# permissions should still be there!
self.assertTrue(are_permissions_roles_seeded(course_id))

def test_forum_unseeding_with_multiple_courses(self):
"""Test new course creation and verify forum unseeding when there are multiple courses"""
test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
second_course_data = self.assert_created_course(number_suffix=uuid4().hex)

# unseed the forums for the first course
course_id = self._get_course_id(test_course_data)
unseed_permissions_roles(course_id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should delete course

self.assertFalse(are_permissions_roles_seeded(course_id))

second_course_id = self._get_course_id(second_course_data)
# permissions should still be there for the other course
self.assertTrue(are_permissions_roles_seeded(second_course_id))

def _get_course_id(self, test_course_data):
"""Returns the course ID (org/number/run)."""
return "{org}/{number}/{run}".format(**test_course_data)
Expand Down
24 changes: 23 additions & 1 deletion common/djangoapps/django_comment_common/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,30 @@ def seed_permissions_roles(course_id):
administrator_role.inherit_permissions(moderator_role)


def are_permissions_roles_seeded(course_id):
def _remove_permission_role(course_id, name):
try:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would create a helper function (defined within this one) that does

try:
if role.course_id == course_id:
role.delete()
except Role.DoesNotExist
pass

Then you can just call that function with each role type.

role = Role.objects.get(name=name, course_id=course_id)
if role.course_id == course_id:
role.delete()
except Role.DoesNotExist:
pass


def unseed_permissions_roles(course_id):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah, this looks better! But (ducking...) can you also add a test for unseed_permissions_roles in isolation? It would be in the test file for django_comment_common/utils.py.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yea, I agree. But when I try to do this, I see that in django_comment_common/module.py, there's a dependency on a course to be in the modulestore (line 70) so that it can dereference course.forum_posts_allowed. I'm sure there's a way to get a course factory or something else in there, but I'd like to timebox this...

"""
A utility method to clean up all forum related permissions and roles
"""
_remove_permission_role(name="Administrator", course_id=course_id)
_remove_permission_role(name="Moderator", course_id=course_id)
_remove_permission_role(name="Community TA", course_id=course_id)
_remove_permission_role(name="Student", course_id=course_id)


def are_permissions_roles_seeded(course_id):
"""
Returns whether the forums permissions for a course have been provisioned in
the database
"""
try:
administrator_role = Role.objects.get(name="Administrator", course_id=course_id)
moderator_role = Role.objects.get(name="Moderator", course_id=course_id)
Expand Down