Skip to content

E
edx-platform
Commit b92ad0ad by David Ormsbee Committed by Ben McMorran
Options

TNL-2291 Add caching to discussion forum permissions 

Caches all permissions per user per course. Adds caching functionality to has_permission and replaces all instances of cached_has_permission with has_permission.
parent e3d820a7
Showing with 110 additions and 69 deletions
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:
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment