Repository: carpentries/amy
Issue: Remove double-counting in display of instructors counts

Looking at https://amy.software-carpentry.org/workshops/reports/instructors_over_time, there's a big spike early in 2016 when a lot of SWC instructors were given DC badges as well.  That's misleading: people shouldn't be double-counted.  Looking in api/views.py at `instructors_over_time`, it looks like we're counting everything in `self.queryset2` (that member name needs to be changed to something more meaningful...) and only then applying `.distinct()` to remove duplicates


Changed files:
api/test/test_reports.py, api/views.py, api/serializers.py

diff --git a/api/serializers.py b/api/serializers.py
index 983b7a6c..295d17f8 100644
--- a/api/serializers.py
+++ b/api/serializers.py
@@ -107,7 +107,7 @@ class WorkshopsOverTimeSerializer(serializers.Serializer):
 
 
 class InstructorsOverTimeSerializer(serializers.Serializer):
-    date = serializers.DateField(format=None, source='awarded')
+    date = serializers.DateField(format=None)
     count = serializers.IntegerField()
 
 
diff --git a/api/test/test_reports.py b/api/test/test_reports.py
index 3794eeb6..d9b64b08 100644
--- a/api/test/test_reports.py
+++ b/api/test/test_reports.py
@@ -149,3 +149,42 @@ class TestCSVYAMLListSerialization(BaseReportingTest):
         data = rvs.all_activity_over_time(mock_request, format=format_).data
         self.assertEqual(type(data['missing']['attendance']), type([]))
         self.assertEqual(type(data['missing']['instructors']), type([]))
+
+
+class TestNotCountingInstructorsTwice(BaseReportingTest):
+    def setUp(self):
+        super().setUp()
+
+        # Get instructor badges
+        swc_instructor, _ = Badge.objects.get_or_create(name='swc-instructor')
+        dc_instructor, _ = Badge.objects.get_or_create(name='dc-instructor')
+
+        # Create instructors
+        instructor = Person.objects.create(
+            username='harrypotter', personal='Harry', family='Potter',
+            email='user1@name.org',
+        )
+        another_instructor = Person.objects.create(
+            username='bobsmith', personal='Bob', family='Smith',
+            email='bob.smith@name.org',
+        )
+
+        # Award badges (harrypotter is a double-instructor)
+        Award.objects.create(person=instructor, badge=swc_instructor,
+                             awarded=datetime.date(2016, 10, 3))
+        Award.objects.create(person=instructor, badge=dc_instructor,
+                             awarded=datetime.date(2016, 10, 2))
+        Award.objects.create(person=another_instructor, badge=dc_instructor,
+                             awarded=datetime.date(2016, 10, 4))
+
+    def test_instructors_over_time(self):
+        """Make sure we don't count double-instructor twice. Regression against
+        #978."""
+        url = reverse('api:reports-instructors-over-time')
+        response = self.client.get(url, {'format': 'json'})
+        content = response.content.decode('utf-8')
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(json.loads(content), [
+            {'count': 1, 'date': '2016-10-02'},
+            {'count': 2, 'date': '2016-10-04'},
+        ])
diff --git a/api/views.py b/api/views.py
index 201c5a7e..9127a3fa 100644
--- a/api/views.py
+++ b/api/views.py
@@ -1,7 +1,7 @@
 import datetime
 from itertools import accumulate
 
-from django.db.models import Count, Sum, Case, F, When, Value, IntegerField
+from django.db.models import Count, Sum, Case, F, When, Value, IntegerField, Min
 from rest_framework import viewsets
 from rest_framework.decorators import list_route
 from rest_framework.filters import DjangoFilterBackend
@@ -260,11 +260,13 @@ class ReportsViewSet(ViewSet):
             prev_ = next(it)
         except StopIteration:
             return
+        next_ = None
         for next_ in it:
             if prev_['date'] != next_['date']:
                 yield prev_
             prev_ = next_
-        yield next_
+        if next_ is not None:
+            yield next_
 
     def listify(self, iterable, request, format=None):
         """Some renderers require lists instead of any iterables for rendering.
@@ -312,9 +314,15 @@ class ReportsViewSet(ViewSet):
     def instructors_over_time(self, request, format=None):
         """Cumulative number of instructor appearances on workshops over
         time."""
+
         badges = Badge.objects.instructor_badges()
-        qs = self.queryset2.filter(badge__in=badges) \\
-                           .annotate(count=Count('person__id')).distinct()
+
+        qs = Person.objects.filter(badges__in=badges)  \\
+                           .annotate(date=Min('award__awarded'),
+                                     count=Value(1,
+                                                 output_field=IntegerField())) \\
+                           .order_by('date')
+
         serializer = InstructorsOverTimeSerializer(qs, many=True)
 
         # run a cumulative generator over the data