cdodge/filter-out-observers: remove observer (or other roles) from aggregate API calls

make sure the social stats API endpoint accept an new source key format, but the forum service needs the old style. Plus add tests

lms/djangoapps/api_manager/courses/tests.py

@@ -6,6 +6,7 @@ Run these tests @ Devstack:
 from datetime import datetime
 import json
 import uuid
+import mock
 from random import randint
 from urllib import urlencode
 
@@ -27,7 +28,8 @@ from .content import TEST_COURSE_OVERVIEW_CONTENT, TEST_COURSE_UPDATES_CONTENT,
 from .content import TEST_STATIC_TAB1_CONTENT, TEST_STATIC_TAB2_CONTENT
 
 TEST_API_KEY = str(uuid.uuid4())
-USER_COUNT = 4
+USER_COUNT = 5
+SAMPLE_GRADE_DATA_COUNT = 4
 
 
 class SecureClient(Client):
@@ -141,9 +143,10 @@ class CoursesApiTests(TestCase):
             user_profile = user.profile
             user_profile.avatar_url = 'http://example.com/{}.png'.format(user.id)
             user_profile.title = 'Software Engineer {}'.format(user.id)
+            user_profile.city = 'Cambridge'
             user_profile.save()
 
-        for i in xrange(USER_COUNT - 1):
+        for i in xrange(SAMPLE_GRADE_DATA_COUNT - 1):
             category = 'mentoring'
             module_type = 'mentoring'
             if i % 2 is 0:
@@ -1480,7 +1483,36 @@ class CoursesApiTests(TestCase):
         response = self.do_get(completion_uri)
         self.assertEqual(response.status_code, 404)
 
+    def _fake_get_get_course_social_stats(course_id):
+        return {
+            '1': {'foo':'bar'},
+            '2': {'one': 'two'}
+        }
+
+    @mock.patch("lms.lib.comment_client.user.get_course_social_stats", _fake_get_get_course_social_stats)
+    def test_social_metrics(self):
+        test_uri = '{}/{}/metrics/social/'.format(self.base_courses_uri, self.test_course_id)
+        response = self.do_get(test_uri)
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(len(response.data.keys()), 2)
+        self.assertIn('1', response.data)
+        self.assertIn('2', response.data)
+
+        # make the first user an observer to asset that its content is being filtered out from
+        # the aggregates
+        allow_access(self.course, self.users[0], 'observer')
+
+        response = self.do_get(test_uri)
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(len(response.data.keys()), 1)
+        self.assertNotIn('1', response.data)
+        self.assertIn('2', response.data)
+
     def test_courses_leaders_list_get(self):
+        # make the last user an observer to asset that its content is being filtered out from
+        # the aggregates
+        allow_access(self.course, self.users[USER_COUNT-1], 'observer')
+
         test_uri = '{}/{}/metrics/proficiency/leaders/'.format(self.base_courses_uri, self.test_course_id)
         response = self.do_get(test_uri)
         self.assertEqual(response.status_code, 200)
@@ -1535,6 +1567,7 @@ class CoursesApiTests(TestCase):
         self.assertEqual(response.status_code, 400)
 
     def test_courses_completions_leaders_list_get(self):
+
         completion_uri = '{}/{}/completions/'.format(self.base_courses_uri, unicode(self.course.id))
         users = []
         for i in xrange(1, 5):
@@ -1549,6 +1582,9 @@ class CoursesApiTests(TestCase):
             self.assertEqual(response.status_code, 201)
             users.append(response.data['id'])
 
+        # make the last user an observer to make sure that data is being filtered out
+        allow_access(self.course, self.users[USER_COUNT-1], 'observer')
+
         for i in xrange(1, 26):
             local_content_name = 'Video_Sequence{}'.format(i)
             local_content = ItemFactory.create(
@@ -1565,11 +1601,18 @@ class CoursesApiTests(TestCase):
                 user_id = users[2]
             else:
                 user_id = users[3]
+
             content_id = unicode(local_content.scope_ids.usage_id)
             completions_data = {'content_id': content_id, 'user_id': user_id}
             response = self.do_post(completion_uri, completions_data)
             self.assertEqual(response.status_code, 201)
 
+            # observer should complete everything, so we can assert that it is filtered out
+            response = self.do_post(completion_uri, {
+                'content_id': content_id, 'user_id': self.users[USER_COUNT-1].id
+            })
+            self.assertEqual(response.status_code, 201)
+
         test_uri = '{}/{}/metrics/completions/leaders/?{}'.format(self.base_courses_uri, self.test_course_id, 'count=6')
         response = self.do_get(test_uri)
         self.assertEqual(response.status_code, 200)
@@ -1761,6 +1804,11 @@ class CoursesApiTests(TestCase):
             self.assertEqual(response.status_code, 200)
             self.assertEqual(response.data['city'], city)
 
+        # make all the classwide users an observer to assert that its content is being filtered out from
+        # the aggregates
+        for user in self.users:
+            allow_access(self.course, user, 'observer')
+
         response = self.do_get('{}{}{}'.format('/api/courses/', self.test_course_id, '/metrics/cities/'))
         self.assertEqual(response.status_code, 200)
         self.assertEqual(len(response.data['results']), 4)

lms/djangoapps/api_manager/courses/views.py

@@ -23,7 +23,7 @@ from courseware.views import get_static_tab_contents
 from django_comment_common.models import FORUM_ROLE_MODERATOR
 from instructor.access import revoke_access, update_forum_role
 from student.models import CourseEnrollment, CourseEnrollmentAllowed
-from student.roles import CourseAccessRole, CourseInstructorRole, CourseStaffRole, CourseObserverRole, UserBasedRole
+from student.roles import CourseRole, CourseAccessRole, CourseInstructorRole, CourseStaffRole, CourseObserverRole, UserBasedRole
 
 from xmodule.modulestore.django import modulestore
 
@@ -41,6 +41,8 @@ from .serializers import GradeSerializer, CourseLeadersSerializer, CourseComplet
 from lms.lib.comment_client.user import get_course_social_stats
 from lms.lib.comment_client.utils import CommentClientRequestError
 
+from opaque_keys.edx.keys import CourseKey
+
 log = logging.getLogger(__name__)
 
 
@@ -304,6 +306,27 @@ def _manage_role(course_descriptor, user, role, action):
                 update_forum_role(course_descriptor.id, user, FORUM_ROLE_MODERATOR, 'revoke')
 
 
+def _get_aggregate_exclusion_user_ids(course_key):
+    """
+    This helper method will return the list of user ids that are marked in roles
+    that can be excluded from certain aggregate queries. The list of roles to exclude
+    can be defined in a AGGREGATION_EXCLUDE_ROLES settings variable
+    """
+
+    exclude_user_ids = set()
+    exclude_role_list = getattr(settings, 'AGGREGATION_EXCLUDE_ROLES', [CourseObserverRole.ROLE])
+
+    for role in exclude_role_list:
+        users = CourseRole(role, course_key).users_with_role()
+        user_ids = set()
+        for user in users:
+            user_ids.add(user.id)
+
+        exclude_user_ids = exclude_user_ids.union(user_ids)
+
+    return exclude_user_ids
+
+
 class CourseContentList(SecureAPIView):
     """
     **Use Case**
@@ -1515,13 +1538,14 @@ class CoursesLeadersList(SecureListAPIView):
         course_descriptor, course_key, course_content = get_course(request, request.user, course_id)  # pylint: disable=W0612
         if not course_descriptor:
             return Response({}, status=status.HTTP_404_NOT_FOUND)
+
         queryset = StudentModule.objects.filter(
             course_id__exact=course_key,
             grade__isnull=False,
             max_grade__isnull=False,
             max_grade__gt=0,
             student__is_active=True
-        )
+        ).exclude(student__in=_get_aggregate_exclusion_user_ids(course_key))
 
         if content_id:
             content_descriptor, content_key, existing_content = get_course_child(request, request.user, course_key, content_id)  # pylint: disable=W0612
@@ -1580,7 +1604,10 @@ class CoursesCompletionsLeadersList(SecureAPIView):
         course_descriptor, course_key, course_content = get_course(request, request.user, course_id)  # pylint: disable=W0612
         if not course_descriptor:
             return Response({}, status=status.HTTP_404_NOT_FOUND)
-        queryset = CourseModuleCompletion.objects.filter(course_id=course_key)
+
+        exclude_users = _get_aggregate_exclusion_user_ids(course_key)
+        queryset = CourseModuleCompletion.objects.filter(course_id=course_key)\
+            .exclude(user__in=exclude_users)
 
         if user_id:
             user_completions = queryset.filter(user__id=user_id).count()
@@ -1591,6 +1618,7 @@ class CoursesCompletionsLeadersList(SecureAPIView):
 
         total_completions = queryset.filter(user__is_active=True).count()
         users = CourseModuleCompletion.objects.filter(user__is_active=True)\
+            .exclude(user__in=exclude_users)\
             .aggregate(total=Count('user__id', distinct=True))
 
         if users and users['total'] > 0:
@@ -1636,7 +1664,19 @@ class CoursesSocialMetrics(SecureListAPIView):
     def get(self, request, course_id): # pylint: disable=W0613
 
         try:
-            data = get_course_social_stats(course_id)
+            course_key = CourseKey.from_string(course_id)
+
+            # the forum service expects the legacy slash separated string format
+            data = get_course_social_stats(course_key.to_deprecated_string())
+
+            # remove any excluded users from the aggregate
+
+            exclude_users = _get_aggregate_exclusion_user_ids(course_key)
+
+            for user_id in exclude_users:
+                if str(user_id) in data:
+                    del data[str(user_id)]
+
             http_status = status.HTTP_200_OK
         except CommentClientRequestError, e:
             data = {
@@ -1666,7 +1706,9 @@ class CoursesCitiesMetrics(SecureListAPIView):
         course_descriptor, course_key, course_content = get_course(self.request, self.request.user, course_id)  # pylint: disable=W0612
         if not course_descriptor:
             raise Http404
-        queryset = CourseEnrollment.users_enrolled_in(course_key)
+
+        exclude_users = _get_aggregate_exclusion_user_ids(course_key)
+        queryset = CourseEnrollment.users_enrolled_in(course_key).exclude(id__in=exclude_users)
         if city:
             city = city.split(',')[:upper_bound]
             q_list = [Q(profile__city__iexact=item.strip()) for item in city]

lms/lib/comment_client/user.py

@@ -162,7 +162,9 @@ class User(models.Model):
 
 
 def get_course_social_stats(course_id):
-
+    """
+    Helper method to get the social stats from the comment service
+    """
     url = _url_for_course_social_stats()
     params = {'course_id': course_id}
     response = perform_request(