diff --git a/common/djangoapps/django_comment_common/models.py b/common/djangoapps/django_comment_common/models.py index 03372a9422..6ea796da4b 100644 --- a/common/djangoapps/django_comment_common/models.py +++ b/common/djangoapps/django_comment_common/models.py @@ -6,6 +6,7 @@ from django.contrib.auth.models import User from django.dispatch import receiver from django.db.models.signals import post_save from django.utils.translation import ugettext_noop + from student.models import CourseEnrollment from xmodule.modulestore.django import modulestore @@ -84,15 +85,14 @@ class Role(models.Model): self.permissions.add(Permission.objects.get_or_create(name=permission)[0]) def has_permission(self, permission): + """Returns True if this role has the given permission, False otherwise.""" course = modulestore().get_course(self.course_id) if course is None: raise ItemNotFoundError(self.course_id) - if self.name == FORUM_ROLE_STUDENT and \ - (permission.startswith('edit') or permission.startswith('update') or permission.startswith('create')) and \ - (not course.forum_posts_allowed): + if permission_blacked_out(course, {self.name}, permission): return False - return self.permissions.filter(name=permission).exists() + return self.permissions.filter(name=permission).exists() # pylint: disable=no-member class Permission(models.Model): @@ -105,3 +105,35 @@ class Permission(models.Model): def __unicode__(self): return self.name + + +def permission_blacked_out(course, role_names, permission_name): + """Returns true if a user in course with the given roles would have permission_name blacked out. + + This will return true if it is a permission that the user might have normally had for the course, but does not have + right this moment because we are in a discussion blackout period (as defined by the settings on the course module). + Namely, they can still view, but they can't edit, update, or create anything. This only applies to students, as + moderators of any kind still have posting privileges during discussion blackouts. + """ + return ( + not course.forum_posts_allowed and + role_names == {FORUM_ROLE_STUDENT} and + any([permission_name.startswith(prefix) for prefix in ['edit', 'update', 'create']]) + ) + + +def all_permissions_for_user_in_course(user, course_id): # pylint: disable=invalid-name + """Returns all the permissions the user has in the given course.""" + course = modulestore().get_course(course_id) + if course is None: + raise ItemNotFoundError(course_id) + + all_roles = {role.name for role in Role.objects.filter(users=user, course_id=course_id)} + + permissions = { + permission.name + for permission + in Permission.objects.filter(roles__users=user, roles__course_id=course_id) + if not permission_blacked_out(course, all_roles, permission.name) + } + return permissions diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 6d7034b747..b7b5e27b04 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1215,6 +1215,7 @@ class MetricsMixin(object): finally: end_time = time.time() + duration = end_time - start_time course_id = getattr(self, 'course_id', '') tags = [ u'view_name:{}'.format(view_name), @@ -1227,10 +1228,17 @@ class MetricsMixin(object): dog_stats_api.increment(XMODULE_METRIC_NAME, tags=tags, sample_rate=XMODULE_METRIC_SAMPLE_RATE) dog_stats_api.histogram( XMODULE_DURATION_METRIC_NAME, - end_time - start_time, + duration, tags=tags, sample_rate=XMODULE_METRIC_SAMPLE_RATE, ) + log.debug( + "%.3fs - render %s.%s (%s)", + duration, + block.__class__.__name__, + view_name, + getattr(block, 'location', ''), + ) def handle(self, block, handler_name, request, suffix=''): start_time = time.time() @@ -1244,6 +1252,7 @@ class MetricsMixin(object): finally: end_time = time.time() + duration = end_time - start_time course_id = getattr(self, 'course_id', '') tags = [ u'handler_name:{}'.format(handler_name), @@ -1256,10 +1265,17 @@ class MetricsMixin(object): dog_stats_api.increment(XMODULE_METRIC_NAME, tags=tags, sample_rate=XMODULE_METRIC_SAMPLE_RATE) dog_stats_api.histogram( XMODULE_DURATION_METRIC_NAME, - end_time - start_time, + duration, tags=tags, sample_rate=XMODULE_METRIC_SAMPLE_RATE ) + log.debug( + "%.3fs - handle %s.%s (%s)", + duration, + block.__class__.__name__, + handler_name, + getattr(block, 'location', ''), + ) class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # pylint: disable=abstract-method diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index 5670c0be43..10e98655e1 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -29,7 +29,7 @@ from django_comment_client.utils import ( get_discussion_categories_ids, get_discussion_id_map, ) -from django_comment_client.permissions import check_permissions_by_view, cached_has_permission +from django_comment_client.permissions import check_permissions_by_view, has_permission from eventtracking import tracker import lms.lib.comment_client as cc @@ -490,7 +490,10 @@ def un_flag_abuse_for_thread(request, course_id, thread_id): course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) course = get_course_by_id(course_key) thread = cc.Thread.find(thread_id) - remove_all = cached_has_permission(request.user, 'openclose_thread', course_key) or has_access(request.user, 'staff', course) + remove_all = ( + has_permission(request.user, 'openclose_thread', course_key) or + has_access(request.user, 'staff', course) + ) thread.unFlagAbuse(user, thread, remove_all) return JsonResponse(prepare_content(thread.to_dict(), course_key)) @@ -522,7 +525,10 @@ def un_flag_abuse_for_comment(request, course_id, comment_id): user = cc.User.from_django_user(request.user) course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) course = get_course_by_id(course_key) - remove_all = cached_has_permission(request.user, 'openclose_thread', course_key) or has_access(request.user, 'staff', course) + remove_all = ( + has_permission(request.user, 'openclose_thread', course_key) or + has_access(request.user, 'staff', course) + ) comment = cc.Comment.find(comment_id) comment.unFlagAbuse(user, comment, remove_all) return JsonResponse(prepare_content(comment.to_dict(), course_key)) diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 59dc5a2106..e9d0ff75b4 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -316,12 +316,12 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase): MODULESTORE = TEST_DATA_MONGO_MODULESTORE @ddt.data( - # old mongo with cache: 15 - (ModuleStoreEnum.Type.mongo, 1, 21, 15, 40, 27), - (ModuleStoreEnum.Type.mongo, 50, 315, 15, 628, 27), + # old mongo with cache + (ModuleStoreEnum.Type.mongo, 1, 7, 5, 12, 7), + (ModuleStoreEnum.Type.mongo, 50, 7, 5, 12, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, 1, 3, 3, 40, 27), - (ModuleStoreEnum.Type.split, 50, 3, 3, 628, 27), + (ModuleStoreEnum.Type.split, 1, 3, 3, 12, 7), + (ModuleStoreEnum.Type.split, 50, 3, 3, 12, 7), ) @ddt.unpack def test_number_of_mongo_queries( @@ -363,27 +363,15 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase): self.assertEquals(response.status_code, 200) self.assertEquals(len(json.loads(response.content)["content"]["children"]), num_thread_responses) - # TODO: update this once django cache is disabled in tests - # Test with and without cache, clearing before and after use. - single_thread_local_cache = cache.get_cache( - backend='default', - LOCATION='single_thread_local_cache' - ) - single_thread_dummy_cache = cache.get_cache( - backend='django.core.cache.backends.dummy.DummyCache', - LOCATION='single_thread_local_cache' - ) + # Test uncached first, then cached now that the cache is warm. cached_calls = [ - [single_thread_dummy_cache, num_uncached_mongo_calls, num_uncached_sql_queries], - [single_thread_local_cache, num_cached_mongo_calls, num_cached_sql_queries] + [num_uncached_mongo_calls, num_uncached_sql_queries], + [num_cached_mongo_calls, num_cached_sql_queries], ] - for single_thread_cache, expected_mongo_calls, expected_sql_queries in cached_calls: - single_thread_cache.clear() - with patch("django_comment_client.permissions.CACHE", single_thread_cache): - with self.assertNumQueries(expected_sql_queries): - with check_mongo_calls(expected_mongo_calls): - call_single_thread() - single_thread_cache.clear() + for expected_mongo_calls, expected_sql_queries in cached_calls: + with self.assertNumQueries(expected_sql_queries): + with check_mongo_calls(expected_mongo_calls): + call_single_thread() @patch('requests.request') diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index d415b682bf..9c9d362f37 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -30,7 +30,7 @@ from courseware.access import has_access from xmodule.modulestore.django import modulestore from ccx.overrides import get_current_ccx -from django_comment_client.permissions import cached_has_permission +from django_comment_client.permissions import has_permission from django_comment_client.utils import ( merge_dict, extract, @@ -209,7 +209,7 @@ def inline_discussion(request, course_key, discussion_id): with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info) - is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) + is_staff = has_permission(request.user, 'openclose_thread', course.id) threads = [utils.prepare_content(thread, course_key, is_staff) for thread in threads] with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"): add_courseware_context(threads, course, request.user) @@ -241,7 +241,7 @@ def forum_form_discussion(request, course_key): try: unsafethreads, query_params = get_threads(request, course) # This might process a search query - is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) + is_staff = has_permission(request.user, 'openclose_thread', course.id) threads = [utils.prepare_content(thread, course_key, is_staff) for thread in unsafethreads] except cc.utils.CommentClientMaintenanceError: log.warning("Forum is in maintenance mode") @@ -275,11 +275,14 @@ def forum_form_discussion(request, course_key): 'threads': _attr_safe_json(threads), 'thread_pages': query_params['num_pages'], 'user_info': _attr_safe_json(user_info), - 'flag_moderator': cached_has_permission(request.user, 'openclose_thread', course.id) or has_access(request.user, 'staff', course), + 'flag_moderator': ( + has_permission(request.user, 'openclose_thread', course.id) or + has_access(request.user, 'staff', course) + ), 'annotated_content_info': _attr_safe_json(annotated_content_info), 'course_id': course.id.to_deprecated_string(), 'roles': _attr_safe_json(utils.get_role_ids(course_key)), - 'is_moderator': cached_has_permission(request.user, "see_all_cohorts", course_key), + 'is_moderator': has_permission(request.user, "see_all_cohorts", course_key), 'cohorts': course_settings["cohorts"], # still needed to render _thread_list_template 'user_cohort': user_cohort_id, # read from container in NewPostView 'is_course_cohorted': is_course_cohorted(course_key), # still needed to render _thread_list_template @@ -304,7 +307,7 @@ def single_thread(request, course_key, discussion_id, thread_id): course_settings = make_course_settings(course, request.user) cc_user = cc.User.from_django_user(request.user) user_info = cc_user.to_dict() - is_moderator = cached_has_permission(request.user, "see_all_cohorts", course_key) + is_moderator = has_permission(request.user, "see_all_cohorts", course_key) # Verify that the student has access to this thread if belongs to a discussion module if discussion_id not in utils.get_discussion_categories_ids(course, request.user): @@ -331,7 +334,7 @@ def single_thread(request, course_key, discussion_id, thread_id): if getattr(thread, "group_id", None) is not None and user_group_id != thread.group_id: raise Http404 - is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) + is_staff = has_permission(request.user, 'openclose_thread', course.id) if request.is_ajax(): with newrelic.agent.FunctionTrace(nr_transaction, "get_annotated_content_infos"): annotated_content_info = utils.get_annotated_content_infos(course_key, thread, request.user, user_info=user_info) @@ -381,7 +384,10 @@ def single_thread(request, course_key, discussion_id, thread_id): 'is_moderator': is_moderator, 'thread_pages': query_params['num_pages'], 'is_course_cohorted': is_course_cohorted(course_key), - 'flag_moderator': cached_has_permission(request.user, 'openclose_thread', course.id) or has_access(request.user, 'staff', course), + 'flag_moderator': ( + has_permission(request.user, 'openclose_thread', course.id) or + has_access(request.user, 'staff', course) + ), 'cohorts': course_settings["cohorts"], 'user_cohort': user_cohort, 'sort_preference': cc_user.default_sort_key, @@ -428,7 +434,7 @@ def user_profile(request, course_key, user_id): with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info) - is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) + is_staff = has_permission(request.user, 'openclose_thread', course.id) threads = [utils.prepare_content(thread, course_key, is_staff) for thread in threads] if request.is_ajax(): return utils.JsonResponse({ @@ -509,7 +515,7 @@ def followed_threads(request, course_key, user_id): with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info) if request.is_ajax(): - is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) + is_staff = has_permission(request.user, 'openclose_thread', course.id) return utils.JsonResponse({ 'annotated_content_info': annotated_content_info, 'discussion_data': [utils.prepare_content(thread, course_key, is_staff) for thread in threads], diff --git a/lms/djangoapps/django_comment_client/permissions.py b/lms/djangoapps/django_comment_client/permissions.py index 1ee08bcca3..39101f73f1 100644 --- a/lms/djangoapps/django_comment_client/permissions.py +++ b/lms/djangoapps/django_comment_client/permissions.py @@ -5,34 +5,27 @@ Module for checking permissions with the comment_client backend import logging from types import NoneType from django.core import cache + +from request_cache.middleware import RequestCache from lms.lib.comment_client import Thread from opaque_keys.edx.keys import CourseKey -CACHE = cache.get_cache('default') -CACHE_LIFESPAN = 60 - - -def cached_has_permission(user, permission, course_id=None): - """ - Call has_permission if it's not cached. A change in a user's role or - a role's permissions will only become effective after CACHE_LIFESPAN seconds. - """ - assert isinstance(course_id, (NoneType, CourseKey)) - key = u"permission_{user_id:d}_{course_id}_{permission}".format( - user_id=user.id, course_id=course_id, permission=permission) - val = CACHE.get(key, None) - if val not in [True, False]: - val = has_permission(user, permission, course_id=course_id) - CACHE.set(key, val, CACHE_LIFESPAN) - return val +from django_comment_common.models import all_permissions_for_user_in_course def has_permission(user, permission, course_id=None): assert isinstance(course_id, (NoneType, CourseKey)) - for role in user.roles.filter(course_id=course_id): - if role.has_permission(permission): - return True - return False + request_cache_dict = RequestCache.get_request_cache().data + cache_key = "django_comment_client.permissions.has_permission.all_permissions.{}.{}".format( + user.id, course_id + ) + if cache_key in request_cache_dict: + all_permissions = request_cache_dict[cache_key] + else: + all_permissions = all_permissions_for_user_in_course(user, course_id) + request_cache_dict[cache_key] = all_permissions + + return permission in all_permissions CONDITIONS = ['is_open', 'is_author', 'is_question_author'] @@ -84,7 +77,7 @@ def _check_conditions_permissions(user, permissions, course_id, content): if isinstance(per, basestring): if per in CONDITIONS: return _check_condition(user, per, content) - return cached_has_permission(user, per, course_id=course_id) + return has_permission(user, per, course_id=course_id) elif isinstance(per, list) and operator in ["and", "or"]: results = [test(user, x, operator="and") for x in per] if operator == "or": diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index d502a4c128..153a3b2b12 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -15,7 +15,7 @@ from opaque_keys.edx.keys import CourseKey from xmodule.modulestore.django import modulestore from django_comment_common.models import Role, FORUM_ROLE_STUDENT -from django_comment_client.permissions import check_permissions_by_view, cached_has_permission +from django_comment_client.permissions import check_permissions_by_view, has_permission from edxmako import lookup_template from courseware.access import has_access @@ -506,8 +506,8 @@ def prepare_content(content, course_key, is_staff=False, course_is_cohorted=None # Only reveal endorser if requester can see author or if endorser is staff if ( - endorser and - ("username" in fields or cached_has_permission(endorser, "endorse_comment", course_key)) + endorser and + ("username" in fields or has_permission(endorser, "endorse_comment", course_key)) ): endorsement["username"] = endorser.username else: @@ -552,7 +552,7 @@ def get_group_id_for_comments_service(request, course_key, commentable_id=None): requested_group_id = request.GET.get('group_id') elif request.method == "POST": requested_group_id = request.POST.get('group_id') - if cached_has_permission(request.user, "see_all_cohorts", course_key): + if has_permission(request.user, "see_all_cohorts", course_key): if not requested_group_id: return None try: