refactor: apply first feedback comments

openedx/features/survey_report/application.py → openedx/features/survey_report/api.py

@@ -7,18 +7,18 @@
 from openedx.features.survey_report.models import SurveyReport
 from openedx.features.survey_report.queries import (
     get_course_enrollments,
-    get_currently_learners,
+    get_recently_active_users,
     get_generated_certificates,
-    get_learners_registered,
+    get_registered_learners,
     get_unique_courses_offered
 )
 
 
 def generate_report() -> None:
     """ Generate a report with relevant data."""
     courses_offered = get_unique_courses_offered()
-    learners = get_currently_learners()
-    registered = get_learners_registered()
+    learners = get_recently_active_users(weeks=3)
+    registered = get_registered_learners()
     certificates = get_generated_certificates()
     enrollments = get_course_enrollments()
     extra_data = settings.SURVEY_REPORT_EXTRA_DATA

openedx/features/survey_report/management/commands/generate_report.py

@@ -4,7 +4,7 @@
 
 from django.core.management.base import BaseCommand, CommandError
 
-from openedx.features.survey_report.application import generate_report
+from openedx.features.survey_report.api import generate_report
 
 
 class Command(BaseCommand):

openedx/features/survey_report/management/commands/tests/test_generate_report.py

@@ -18,18 +18,18 @@ class GenerateReportTest(TestCase):
     @override_settings(SURVEY_REPORT_EXTRA_DATA={'extra_data': 'extra_data'})
     @mock.patch('openedx.features.survey_report.queries.get_course_enrollments')
     @mock.patch('openedx.features.survey_report.queries.get_generated_certificates')
-    @mock.patch('openedx.features.survey_report.queries.get_learners_registered')
-    @mock.patch('openedx.features.survey_report.queries.get_currently_learners')
+    @mock.patch('openedx.features.survey_report.queries.get_registered_learners')
+    @mock.patch('openedx.features.survey_report.queries.get_recently_active_users')
     @mock.patch('openedx.features.survey_report.queries.get_unique_courses_offered')
-    def test_generate_report(self, mock_get_unique_courses_offered, mock_get_currently_learners,
-                             mock_get_learners_registered, mock_get_generated_certificates,
+    def test_generate_report(self, mock_get_unique_courses_offered, mock_get_recently_active_users,
+                             mock_get_registered_learners, mock_get_generated_certificates,
                              mock_get_course_enrollments):
         """
         Test that generate_report command creates a survey report.
         """
         mock_get_unique_courses_offered.return_value = 1
-        mock_get_currently_learners.return_value = 2
-        mock_get_learners_registered.return_value = 3
+        mock_get_recently_active_users.return_value = 2
+        mock_get_registered_learners.return_value = 3
         mock_get_generated_certificates.return_value = 4
         mock_get_course_enrollments.return_value = 5
         out = StringIO()

openedx/features/survey_report/migrations/0001_initial.py

@@ -1,4 +1,4 @@
-# Generated by Django 3.2.16 on 2022-10-21 14:54
+# Generated by Django 3.2.16 on 2022-11-03 20:07
 
 from django.db import migrations, models
 import jsonfield.fields
@@ -16,15 +16,13 @@ class Migration(migrations.Migration):
             name='SurveyReport',
             fields=[
                 ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
-                ('courses_offered', models.IntegerField()),
-                ('learners', models.IntegerField()),
-                ('registered_learners', models.IntegerField()),
-                ('enrollments', models.IntegerField()),
-                ('generated_certificates', models.IntegerField()),
-                ('extra_data', jsonfield.fields.JSONField(blank=True, default={}, help_text='Extra information for instance data')),
-                ('request_details', models.CharField(blank=True, max_length=255, null=True)),
+                ('courses_offered', models.BigIntegerField()),
+                ('learners', models.BigIntegerField()),
+                ('registered_learners', models.BigIntegerField()),
+                ('enrollments', models.BigIntegerField()),
+                ('generated_certificates', models.BigIntegerField()),
+                ('extra_data', jsonfield.fields.JSONField(blank=True, default=dict, help_text='Extra information for instance data')),
                 ('created_at', models.DateTimeField(auto_now=True)),
-                ('sent_at', models.DateTimeField(blank=True, null=True)),
             ],
             options={
                 'ordering': ['-created_at'],

openedx/features/survey_report/models.py

@@ -12,19 +12,17 @@ class SurveyReport(models.Model):
 
     .. no_pii:
     """
-    courses_offered = models.IntegerField()
-    learners = models.IntegerField()
-    registered_learners = models.IntegerField()
-    enrollments = models.IntegerField()
-    generated_certificates = models.IntegerField()
+    courses_offered = models.BigIntegerField()
+    learners = models.BigIntegerField()
+    registered_learners = models.BigIntegerField()
+    enrollments = models.BigIntegerField()
+    generated_certificates = models.BigIntegerField()
     extra_data = JSONField(
         blank=True,
-        default={},
+        default=dict,
         help_text="Extra information for instance data",
     )
-    request_details = models.CharField(max_length=255, null=True, blank=True)
     created_at = models.DateTimeField(auto_now=True)
-    sent_at = models.DateTimeField(null=True, blank=True)
 
     class Meta:
         ordering = ["-created_at"]

openedx/features/survey_report/queries.py

@@ -7,6 +7,7 @@
 from django.contrib.auth.models import User  # lint-amnesty, pylint: disable=imported-auth-user
 from django.db.models import Count, OuterRef, Q, Subquery
 
+from common.djangoapps.util.query import read_replica_or_default
 from common.djangoapps.student.models import CourseEnrollment
 from lms.djangoapps.grades.models import PersistentCourseGrade
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
@@ -27,32 +28,44 @@ def get_unique_courses_offered() -> int:
         .filter(count__gt=5)\
         .filter(start__lt=datetime.now())\
         .filter(Q(end__isnull=True) | Q(end__gt=datetime.now()))\
+        .using(read_replica_or_default())\
         .count()
 
 
-def get_currently_learners() -> int:
+def get_recently_active_users(weeks: int) -> int:
     """
-    Get total number of learners with last login in the last 3 weeks.
+    Get total number of users with last login in the last weeks.
+
+    Args:
+        weeks (int): number of weeks since the last login to considerate as an active learner.
     """
-    return User.objects.filter(last_login__gte=datetime.now() - timedelta(weeks=3)).count()
+    return User.objects.filter(last_login__gte=datetime.now() - timedelta(weeks=weeks))\
+        .using(read_replica_or_default())\
+        .count()
 
 
-def get_learners_registered() -> int:
+def get_registered_learners() -> int:
     """
     Get total number of active learners registered.
     """
-    return User.objects.filter(is_active=True).count()
+    return User.objects.filter(is_active=True)\
+        .using(read_replica_or_default())\
+        .count()
 
 
 def get_generated_certificates() -> int:
     """
     Get total number of generated certificates.
     """
-    return PersistentCourseGrade.objects.filter(passed_timestamp__isnull=False).count()
+    return PersistentCourseGrade.objects.filter(passed_timestamp__isnull=False)\
+        .using(read_replica_or_default())\
+        .count()
 
 
 def get_course_enrollments() -> int:
     """
-    Get total number of enrollments from users that aren't staff.
+    Get total number of enrollments from users that aren't staff. Course staff members will be included.
     """
-    return CourseEnrollment.objects.filter(user__is_superuser=False, user__is_staff=False).count()
+    return CourseEnrollment.objects.filter(is_active=True, user__is_superuser=False, user__is_staff=False)\
+        .using(read_replica_or_default())\
+        .count()

openedx/features/survey_report/tests/test_query_methods.py

@@ -10,9 +10,9 @@
 from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory
 from openedx.features.survey_report.queries import (
     get_course_enrollments,
-    get_currently_learners,
+    get_recently_active_users,
     get_generated_certificates,
-    get_learners_registered,
+    get_registered_learners,
     get_unique_courses_offered
 )
 from xmodule.modulestore import ModuleStoreEnum  # lint-amnesty, pylint: disable=wrong-import-order
@@ -57,7 +57,7 @@ def test_get_unique_courses_offered(self):
             mock_datetime.now.return_value = datetime.now()
             assert get_unique_courses_offered() == 1
 
-    def test_get_currently_learners(self):
+    def test_get_recently_active_users(self):
         """
         Test that get_currently_learners returns the correct number of learners.
         """
@@ -69,13 +69,13 @@ def test_get_currently_learners(self):
         self.user2.save()
         with patch('openedx.features.survey_report.queries.datetime') as mock_datetime:
             mock_datetime.now.return_value = datetime.now()
-            assert get_currently_learners() == 2
+            assert get_recently_active_users(weeks=3) == 2
 
     def test_get_learners_registered(self):
         """
         Test that get_learners_registered returns the correct number of learners.
         """
-        assert get_learners_registered() == 7
+        assert get_registered_learners() == 7
 
     def test_get_generated_certificates(self):
         """