Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
33 changes: 33 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,37 @@ 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 seeding """
test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
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):
"""Test new course creation and verify forum 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.

Comment should be updated.

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
38 changes: 37 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,44 @@ def seed_permissions_roles(course_id):
administrator_role.inherit_permissions(moderator_role)


def are_permissions_roles_seeded(course_id):
def unseed_permissions_roles(course_id):
"""
A utility method to clean up all forum related permissions and roles
"""
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.

administrator_role = Role.objects.get(name="Administrator", course_id=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.

Just do Role.objects.filter(course_id=course_id).delete()

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, even better.

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.

Right, I think my only pushback on this (use of 'filter') is that there's uncertainty regarding the collation here and I have that sanity check before calling the delete() method.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If we do end up with courses with different casing of the same id, this will be the least of our problems.

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

try:
moderator_role = Role.objects.get(name="Moderator", course_id=course_id)
if moderator_role.course_id == course_id:
moderator_role.delete()
except Role.DoesNotExist:
pass

try:
community_ta_role = Role.objects.get(name="Community TA", course_id=course_id)
if community_ta_role.course_id == course_id:
community_ta_role.delete()
except Role.DoesNotExist:
pass

try:
student_role = Role.objects.get(name="Student", course_id=course_id)
if student_role.course_id == course_id:
student_role.delete()
except Role.DoesNotExist:
pass


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