Merge pull request #298 from edx-solutions/ziafazal/api-fix-company-proficiency-avg

ziafazal/api-fix-company-proficiency-avg: fix for incorrect average calculation

lms/djangoapps/api_manager/courses/views.py

@@ -38,7 +38,7 @@ from student.roles import CourseRole, CourseAccessRole, CourseInstructorRole, Co
 from xmodule.modulestore.django import modulestore
 
 from api_manager.courseware_access import get_course, get_course_child, get_course_leaf_nodes, get_course_key, \
-    course_exists, get_modulestore, get_course_descriptor
+    course_exists, get_modulestore, get_course_descriptor, get_aggregate_exclusion_user_ids
 from api_manager.models import CourseGroupRelationship, CourseContentGroupRelationship, GroupProfile, \
     CourseModuleCompletion
 from api_manager.permissions import SecureAPIView, SecureListAPIView
@@ -323,27 +323,6 @@ 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
-
-
 def _get_course_data(request, course_key, course_descriptor, depth=0):
     """
     creates a dict of course attributes
@@ -1459,7 +1438,7 @@ class CoursesMetricsGradesList(SecureListAPIView):
         if not course_exists(request, request.user, course_id):
             return Response({}, status=status.HTTP_404_NOT_FOUND)
         course_key = get_course_key(course_id)
-        exclude_users = _get_aggregate_exclusion_user_ids(course_key)
+        exclude_users = get_aggregate_exclusion_user_ids(course_key)
         queryset = StudentGradebook.objects.filter(course_id__exact=course_key,
                                                    user__is_active=True,
                                                    user__courseenrollment__is_active=True,
@@ -1533,7 +1512,7 @@ class CoursesMetrics(SecureAPIView):
             return Response({}, status=status.HTTP_404_NOT_FOUND)
         course_descriptor, course_key, course_content = get_course(request, request.user, course_id)
         slash_course_id = get_course_key(course_id, slashseparated=True)
-        exclude_users = _get_aggregate_exclusion_user_ids(course_key)
+        exclude_users = get_aggregate_exclusion_user_ids(course_key)
         users_enrolled_qs = CourseEnrollment.users_enrolled_in(course_key).exclude(id__in=exclude_users)
         organization = request.QUERY_PARAMS.get('organization', None)
         org_ids = None
@@ -1599,7 +1578,7 @@ class CoursesTimeSeriesMetrics(SecureAPIView):
         start_dt = parse_datetime(start)
         end_dt = parse_datetime(end)
         course_key = get_course_key(course_id)
-        exclude_users = _get_aggregate_exclusion_user_ids(course_key)
+        exclude_users = get_aggregate_exclusion_user_ids(course_key)
         grade_complete_match_range = getattr(settings, 'GRADEBOOK_GRADE_COMPLETE_PROFORMA_MATCH_RANGE', 0.01)
         grades_qs = StudentGradebook.objects.filter(course_id__exact=course_key, user__is_active=True,
                                                     user__courseenrollment__is_active=True,
@@ -1695,7 +1674,7 @@ class CoursesMetricsGradesLeadersList(SecureListAPIView):
             return Response({}, status=status.HTTP_404_NOT_FOUND)
         course_key = get_course_key(course_id)
         # Users having certain roles (such as an Observer) are excluded from aggregations
-        exclude_users = _get_aggregate_exclusion_user_ids(course_key)
+        exclude_users = get_aggregate_exclusion_user_ids(course_key)
         leaderboard_data = StudentGradebook.generate_leaderboard(course_key, user_id=user_id, count=count, exclude_users=exclude_users)
         serializer = CourseLeadersSerializer(leaderboard_data['queryset'], many=True)
         data['leaders'] = serializer.data  # pylint: disable=E1101
@@ -1743,7 +1722,7 @@ class CoursesMetricsCompletionsLeadersList(SecureAPIView):
             return Response({}, status=status.HTTP_404_NOT_FOUND)
         course_key = get_course_key(course_id)
         total_possible_completions = float(len(get_course_leaf_nodes(course_key)))
-        exclude_users = _get_aggregate_exclusion_user_ids(course_key)
+        exclude_users = get_aggregate_exclusion_user_ids(course_key)
         orgs_filter = self.request.QUERY_PARAMS.get('organizations', None)
         if orgs_filter:
             upper_bound = getattr(settings, 'API_LOOKUP_UPPER_BOUND', 100)
@@ -1815,7 +1794,7 @@ class CoursesMetricsSocial(SecureListAPIView):
             course_key = get_course_key(course_id)
             # remove any excluded users from the aggregate
 
-            exclude_users = _get_aggregate_exclusion_user_ids(course_key)
+            exclude_users = get_aggregate_exclusion_user_ids(course_key)
 
             for user_id in exclude_users:
                 if str(user_id) in data:
@@ -1863,7 +1842,7 @@ class CoursesMetricsCities(SecureListAPIView):
         if not course_exists(self.request, self.request.user, course_id):
             raise Http404
         course_key = get_course_key(course_id)
-        exclude_users = _get_aggregate_exclusion_user_ids(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]

lms/djangoapps/api_manager/courseware_access.py

@@ -5,6 +5,7 @@ from django.conf import settings
 
 from courseware import courses, module_render
 from courseware.model_data import FieldDataCache
+from student.roles import CourseRole, CourseObserverRole
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey, UsageKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location
@@ -149,3 +150,24 @@ def get_course_child_content(request, user, course_key, child_descriptor):
         field_data_cache,
         course_key)
     return child_content
+
+
+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

lms/djangoapps/organizations/tests.py

@@ -443,24 +443,29 @@ class OrganizationsApiTests(TestCase):
         response = self.do_get(metrics_uri)
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data['users_grade_complete_count'], 6)
+        self.assertEqual(response.data['users_grade_average'], 0.751)
+
 
         courses = {'courses': unicode(course1.id)}
         filtered_metrics_uri = '{}?{}'.format(metrics_uri, urlencode(courses))
         response = self.do_get(filtered_metrics_uri)
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data['users_grade_complete_count'], 2)
+        self.assertEqual(response.data['users_grade_average'], 0.845)
 
         courses = {'courses': unicode(course2.id)}
         filtered_metrics_uri = '{}?{}'.format(metrics_uri, urlencode(courses))
         response = self.do_get(filtered_metrics_uri)
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data['users_grade_complete_count'], 4)
+        self.assertEqual(response.data['users_grade_average'], 0.688)
 
         courses = {'courses': '{},{}'.format(course1.id, course2.id)}
         filtered_metrics_uri = '{}?{}'.format(metrics_uri, urlencode(courses))
         response = self.do_get(filtered_metrics_uri)
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data['users_grade_complete_count'], 6)
+        self.assertEqual(response.data['users_grade_average'], 0.758)
 
         courses = {'courses': '{}'.format(self.course.id)}
         filtered_metrics_uri = '{}?{}'.format(metrics_uri, urlencode(courses))

lms/djangoapps/organizations/views.py

@@ -4,13 +4,13 @@
 from django.conf import settings
 from django.contrib.auth.models import User, Group
 from django.core.exceptions import ObjectDoesNotExist
-from django.db.models import Avg, F
+from django.db.models import Sum, F, Count
 
 from rest_framework import status, viewsets
 from rest_framework.decorators import action
 from rest_framework.response import Response
 
-from api_manager.courseware_access import get_course_key
+from api_manager.courseware_access import get_course_key, get_aggregate_exclusion_user_ids
 from organizations.models import Organization
 from api_manager.users.serializers import UserSerializer
 from api_manager.groups.serializers import GroupSerializer
@@ -39,18 +39,29 @@ class OrganizationsViewSet(viewsets.ModelViewSet):
         org_user_grades = StudentGradebook.objects.filter(user__organizations=pk, user__is_active=True,
                                                           user__courseenrollment__is_active=True)
         courses_filter = request.QUERY_PARAMS.get('courses', None)
+        courses = []
         if courses_filter:
             upper_bound = getattr(settings, 'API_LOOKUP_UPPER_BOUND', 100)
             courses_filter = courses_filter.split(",")[:upper_bound]
-            courses = []
             for course_string in courses_filter:
                 courses.append(get_course_key(course_string))
             org_user_grades = org_user_grades.filter(course_id__in=courses,
                                                      user__courseenrollment__course_id__in=courses)
 
-        users_grade_average = org_user_grades.aggregate(Avg('grade'))
-        if users_grade_average['grade__avg']:
-            grade_avg = float('{0:.3f}'.format(float(users_grade_average['grade__avg'])))
+        users_grade_sum = org_user_grades.aggregate(Sum('grade'))
+        if users_grade_sum['grade__sum']:
+            exclude_users = set()
+            for course_key in courses:
+                exclude_users.union(get_aggregate_exclusion_user_ids(course_key))
+            users_enrolled_qs = CourseEnrollment.objects.filter(user__is_active=True, is_active=True,
+                                                                user__organizations=pk)\
+                .exclude(user_id__in=exclude_users)
+            if courses:
+                users_enrolled_qs = users_enrolled_qs.filter(course_id__in=courses)
+            users_enrolled = users_enrolled_qs.aggregate(Count('user', distinct=True))
+            total_users = users_enrolled['user__count']
+            if total_users:
+                grade_avg = float('{0:.3f}'.format(float(users_grade_sum['grade__sum']) / total_users))
         response_data['users_grade_average'] = grade_avg
 
         users_grade_complete_count = org_user_grades\