Merge pull request #8528 from edx/benmcmorran/discussion-caching

TNL-2291 Discussion forums permission caching

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

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

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))

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')

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],

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":

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: