From 119b4206a4b6e4012fae64c1a68f25e62b77f6b0 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 5 Feb 2013 18:42:01 -0500 Subject: [PATCH 1/2] Revert "Fix permissions bug and add test cases for django comment client permissions." This reverts commit e7450874557b48c5479dad6bc9f008c6c50100ce. --- .../django_comment_client/permissions.py | 7 +- lms/djangoapps/django_comment_client/tests.py | 111 +++++++++++++++ .../django_comment_client/tests/__init__.py | 0 .../tests/test_permissions.py | 128 ------------------ 4 files changed, 113 insertions(+), 133 deletions(-) create mode 100644 lms/djangoapps/django_comment_client/tests.py delete mode 100644 lms/djangoapps/django_comment_client/tests/__init__.py delete mode 100644 lms/djangoapps/django_comment_client/tests/test_permissions.py diff --git a/lms/djangoapps/django_comment_client/permissions.py b/lms/djangoapps/django_comment_client/permissions.py index b583c3fe74..b95a890dda 100644 --- a/lms/djangoapps/django_comment_client/permissions.py +++ b/lms/djangoapps/django_comment_client/permissions.py @@ -29,7 +29,6 @@ def has_permission(user, permission, course_id=None): CONDITIONS = ['is_open', 'is_author'] -# data may be a json file def check_condition(user, condition, course_id, data): def check_open(user, condition, course_id, data): try: @@ -62,10 +61,8 @@ def check_conditions_permissions(user, permissions, course_id, **kwargs): def test(user, per, operator="or"): if isinstance(per, basestring): if per in CONDITIONS: - return check_condition(user, per, course_id, kwargs['data']) + return check_condition(user, per, course_id, kwargs) return cached_has_permission(user, per, course_id=course_id) - # TODO: refactor this to be more clear. - # e.g. the "and operator in" bit on the next line is not needed? elif isinstance(per, list) and operator in ["and", "or"]: results = [test(user, x, operator="and") for x in per] if operator == "or": @@ -105,4 +102,4 @@ def check_permissions_by_view(user, course_id, content, name): p = VIEW_PERMISSIONS[name] except KeyError: logging.warning("Permission for view named %s does not exist in permissions.py" % name) - return check_conditions_permissions(user, p, course_id, data=content) + return check_conditions_permissions(user, p, course_id, content=content) diff --git a/lms/djangoapps/django_comment_client/tests.py b/lms/djangoapps/django_comment_client/tests.py new file mode 100644 index 0000000000..ac059a1e3f --- /dev/null +++ b/lms/djangoapps/django_comment_client/tests.py @@ -0,0 +1,111 @@ +from django.contrib.auth.models import User, Group +from django.core.urlresolvers import reverse +from django.test import TestCase +from django.test.client import RequestFactory +from django.conf import settings + +from mock import Mock + +from override_settings import override_settings + +import xmodule.modulestore.django + +from student.models import CourseEnrollment + +from django.db.models.signals import m2m_changed, pre_delete, pre_save, post_delete, post_save +from django.dispatch.dispatcher import _make_id +import string +import random +from .permissions import has_permission +from .models import Role, Permission + +from xmodule.modulestore.django import modulestore +from xmodule.modulestore import Location +from xmodule.modulestore.xml_importer import import_from_xml +from xmodule.modulestore.xml import XMLModuleStore + +import comment_client + +from courseware.tests.tests import PageLoader, TEST_DATA_XML_MODULESTORE + +#@override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) +#class TestCohorting(PageLoader): +# """Check that cohorting works properly""" +# +# def setUp(self): +# xmodule.modulestore.django._MODULESTORES = {} +# +# # Assume courses are there +# self.toy = modulestore().get_course("edX/toy/2012_Fall") +# +# # Create two accounts +# self.student = 'view@test.com' +# self.student2 = 'view2@test.com' +# self.password = 'foo' +# self.create_account('u1', self.student, self.password) +# self.create_account('u2', self.student2, self.password) +# self.activate_user(self.student) +# self.activate_user(self.student2) +# +# def test_create_thread(self): +# my_save = Mock() +# comment_client.perform_request = my_save +# +# resp = self.client.post( +# reverse('django_comment_client.base.views.create_thread', +# kwargs={'course_id': 'edX/toy/2012_Fall', +# 'commentable_id': 'General'}), +# {'some': "some", +# 'data': 'data'}) +# self.assertTrue(my_save.called) +# +# #self.assertEqual(resp.status_code, 200) +# #self.assertEqual(my_save.something, "expected", "complaint if not true") +# +# self.toy.metadata["cohort_config"] = {"cohorted": True} +# +# # call the view again ... +# +# # assert that different things happened + + + +class PermissionsTestCase(TestCase): + def random_str(self, length=15, chars=string.ascii_uppercase + string.digits): + return ''.join(random.choice(chars) for x in range(length)) + + def setUp(self): + + self.course_id = "edX/toy/2012_Fall" + + self.moderator_role = Role.objects.get_or_create(name="Moderator", course_id=self.course_id)[0] + self.student_role = Role.objects.get_or_create(name="Student", course_id=self.course_id)[0] + + self.student = User.objects.create(username=self.random_str(), + password="123456", email="john@yahoo.com") + self.moderator = User.objects.create(username=self.random_str(), + password="123456", email="staff@edx.org") + self.moderator.is_staff = True + self.moderator.save() + self.student_enrollment = CourseEnrollment.objects.create(user=self.student, course_id=self.course_id) + self.moderator_enrollment = CourseEnrollment.objects.create(user=self.moderator, course_id=self.course_id) + + def tearDown(self): + self.student_enrollment.delete() + self.moderator_enrollment.delete() + +# Do we need to have this? We shouldn't be deleting students, ever +# self.student.delete() +# self.moderator.delete() + + def testDefaultRoles(self): + self.assertTrue(self.student_role in self.student.roles.all()) + self.assertTrue(self.moderator_role in self.moderator.roles.all()) + + def testPermission(self): + name = self.random_str() + self.moderator_role.add_permission(name) + self.assertTrue(has_permission(self.moderator, name, self.course_id)) + + self.student_role.add_permission(name) + self.assertTrue(has_permission(self.student, name, self.course_id)) diff --git a/lms/djangoapps/django_comment_client/tests/__init__.py b/lms/djangoapps/django_comment_client/tests/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/lms/djangoapps/django_comment_client/tests/test_permissions.py b/lms/djangoapps/django_comment_client/tests/test_permissions.py deleted file mode 100644 index 44e1a3a128..0000000000 --- a/lms/djangoapps/django_comment_client/tests/test_permissions.py +++ /dev/null @@ -1,128 +0,0 @@ -import string -import random -import collections - -import factory -from django.test import TestCase - -from django.contrib.auth.models import User -from student.models import UserProfile, CourseEnrollment -from django_comment_client.models import Role, Permission -import django_comment_client.permissions as p - -class UserFactory(factory.Factory): - FACTORY_FOR = User - username = 'robot' - password = '123456' - email = 'robot@edx.org' - is_active = True - is_staff = False - -class CourseEnrollmentFactory(factory.Factory): - FACTORY_FOR = CourseEnrollment - user = factory.SubFactory(UserFactory) - course_id = 'edX/toy/2012_Fall' - -class RoleFactory(factory.Factory): - FACTORY_FOR = Role - name = 'Student' - course_id = 'edX/toy/2012_Fall' - -class PermissionFactory(factory.Factory): - FACTORY_FOR = Permission - name = 'create_comment' - - -class PermissionsTestCase(TestCase): - def setUp(self): - self.course_id = "edX/toy/2012_Fall" - - self.student_role = RoleFactory(name='Student') - self.moderator_role = RoleFactory(name='Moderator') - self.student = UserFactory(username='student', email='student@edx.org') - self.moderator = UserFactory(username='moderator', email='staff@edx.org', is_staff=True) - self.update_thread_permission = PermissionFactory(name='update_thread') - self.update_thread_permission.roles.add(self.student_role) - self.update_thread_permission.roles.add(self.moderator_role) - self.manage_moderator_permission = PermissionFactory(name='manage_moderator') - self.manage_moderator_permission.roles.add(self.moderator_role) - self.student_enrollment = CourseEnrollmentFactory(user=self.student) - self.moderator_enrollment = CourseEnrollmentFactory(user=self.moderator) - - self.student_open_thread = {'content': { - 'closed': False, - 'user_id': str(self.student.id)} - } - self.student_closed_thread = {'content': { - 'closed': True, - 'user_id': str(self.student.id)} - } - - def test_user_has_permission(self): - s_ut = p.has_permission(self.student, 'update_thread', self.course_id) - m_ut = p.has_permission(self.moderator, 'update_thread', self.course_id) - s_mm = p.has_permission(self.student, 'manage_moderator', self.course_id) - m_mm = p.has_permission(self.moderator, 'manage_moderator', self.course_id) - self.assertTrue(s_ut) - self.assertTrue(m_ut) - self.assertFalse(s_mm) - self.assertTrue(m_mm) - - def test_check_conditions(self): - # Checks whether the discussion thread is open, or whether the author is user - s_o = p.check_condition(self.student, 'is_open', self.course_id, self.student_open_thread) - s_a = p.check_condition(self.student, 'is_author', self.course_id, self.student_open_thread) - m_c = p.check_condition(self.moderator, 'is_open', self.course_id, self.student_closed_thread) - m_a = p.check_condition(self.moderator,'is_author', self.course_id, self.student_open_thread) - self.assertTrue(s_o) - self.assertTrue(s_a) - self.assertFalse(m_c) - self.assertFalse(m_a) - - def test_check_conditions_and_permissions(self): - # Check conditions - ret = p.check_conditions_permissions(self.student, - 'is_open', - self.course_id, - data=self.student_open_thread) - self.assertTrue(ret) - - # Check permissions - ret = p.check_conditions_permissions(self.student, - 'update_thread', - self.course_id, - data=self.student_open_thread) - self.assertTrue(ret) - - # Check that a list of permissions/conditions will be OR'd - ret = p.check_conditions_permissions(self.moderator, - ['is_open','manage_moderator'], - self.course_id, - data=self.student_open_thread) - self.assertTrue(ret) - - # Check that a list of permissions will be OR'd - ret = p.check_conditions_permissions(self.student, - ['update_thread','manage_moderator'], - self.course_id, - data=self.student_open_thread) - self.assertTrue(ret) - - # Check that a list of list of permissions will be AND'd - ret = p.check_conditions_permissions(self.student, - [['update_thread','manage_moderator']], - self.course_id, - data=self.student_open_thread) - self.assertFalse(ret) - - def test_check_permissions_by_view(self): - ret = p.check_permissions_by_view(self.student, self.course_id, - self.student_open_thread, 'openclose_thread') - self.assertFalse(ret) - - # Check a view permission that includes both a condition and a permission - self.vote_permission = PermissionFactory(name='vote') - self.vote_permission.roles.add(self.student_role) - ret = p.check_permissions_by_view(self.student, self.course_id, - self.student_open_thread, 'vote_for_comment') - self.assertTrue(ret) \ No newline at end of file From a3a886bd0f8ece9388d13af7de16bc94f07851ec Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 5 Feb 2013 18:51:16 -0500 Subject: [PATCH 2/2] Revert "Add tests for django-comment-client models" (which actually had a model change as well) This reverts commit 053547453ec641feb755247a90cc61560a1f44c0. --- .../django_comment_client/models.py | 8 ++- .../tests/test_models.py | 54 ------------------- 2 files changed, 3 insertions(+), 59 deletions(-) delete mode 100644 lms/djangoapps/django_comment_client/tests/test_models.py diff --git a/lms/djangoapps/django_comment_client/models.py b/lms/djangoapps/django_comment_client/models.py index 3c3ab2bb53..10c05c75e9 100644 --- a/lms/djangoapps/django_comment_client/models.py +++ b/lms/djangoapps/django_comment_client/models.py @@ -46,13 +46,11 @@ class Role(models.Model): def add_permission(self, permission): self.permissions.add(Permission.objects.get_or_create(name=permission)[0]) - def has_permission(self, permission): course = get_course_by_id(self.course_id) - changing_comments = permission.startswith('edit') or \ - permission.startswith('update') or permission.startswith('create') - in_blackout_period = not course.forum_posts_allowed - if (self.name == FORUM_ROLE_STUDENT) and in_blackout_period and changing_comments: + if self.name == FORUM_ROLE_STUDENT and \ + (permission.startswith('edit') or permission.startswith('update') or permission.startswith('create')) and \ + (not course.forum_posts_allowed): return False return self.permissions.filter(name=permission).exists() diff --git a/lms/djangoapps/django_comment_client/tests/test_models.py b/lms/djangoapps/django_comment_client/tests/test_models.py deleted file mode 100644 index 3c9b05b202..0000000000 --- a/lms/djangoapps/django_comment_client/tests/test_models.py +++ /dev/null @@ -1,54 +0,0 @@ -import django_comment_client.models as models -import django_comment_client.permissions as permissions -from django.test import TestCase -from nose.plugins.skip import SkipTest -from courseware.courses import get_course_by_id - -class RoleClassTestCase(TestCase): - def setUp(self): - self.course_id = "edX/toy/2012_Fall" - self.student_role = models.Role.objects.create(name="Student", - course_id=self.course_id) - - def test_unicode(self): - self.assertEqual(str(self.student_role), "Student for edX/toy/2012_Fall") - - self.admin_for_all = models.Role.objects.create(name="Administrator") - self.assertEqual(str(self.admin_for_all), "Administrator for all courses") - - def test_has_permission(self): - self.student_role.add_permission("delete_thread") - self.TA_role = models.Role.objects.create(name="Community TA", - course_id=self.course_id) - self.assertTrue(self.student_role.has_permission("delete_thread")) - self.assertFalse(self.TA_role.has_permission("delete_thread")) - - # Toy course does not have a blackout period defined. - def test_students_can_create_if_not_during_blackout(self): - self.student_role.add_permission("create_comment") - self.assertTrue(self.student_role.has_permission("create_comment")) - - def test_students_cannot_create_during_blackout(self): - # Not sure how to set up these conditions - raise SkipTest() - - def test_inherit_permissions(self): - self.student_role.add_permission("delete_thread") - self.TA_role = models.Role.objects.create(name="Community TA", - course_id=self.course_id) - self.TA_role.inherit_permissions(self.student_role) - self.assertTrue(self.TA_role.has_permission("delete_thread")) - - # TODO: You should not be able to inherit permissions across courses? - def test_inherit_permissions_across_courses(self): - raise SkipTest() - self.student_role.add_permission("delete_thread") - self.course_id_2 = "MITx/6.002x/2012_Fall" - self.admin_role = models.Role.objects.create(name="Administrator", - course_id=self.course_id_2) - self.admin_role.inherit_permissions(self.student_role) - -class PermissionClassTestCase(TestCase): - def test_unicode(self): - self.permission = permissions.Permission.objects.create(name="test") - self.assertEqual(str(self.permission), "test")