From a55cd9b7016158be48e1b12e4dda4a4b80c06999 Mon Sep 17 00:00:00 2001
From: John Baldwin <jlbaldwin@gmail.com>
Date: Thu, 17 Dec 2020 16:25:31 +0100
Subject: [PATCH] Refactor: Moved a number of edx-platform imports to
 figures.compat

Improved abstraction: This commit refactors how Figures imports edx-platform
classes. It centralizes imports to figures.compat. The rest of Figures then
imports the platform classes from figures.compat.

This is preliminary work to help abstract platform dependencies for Open
edX named release structures and improve future testing efforts
---
 figures/compat.py                            |  9 ++--
 figures/filters.py                           |  5 +-
 figures/models.py                            |  2 +
 figures/pipeline/course_daily_metrics.py     |  7 +--
 figures/serializers.py                       |  9 ++--
 figures/sites.py                             | 50 ++++++++++++++------
 figures/tasks.py                             |  8 ++--
 figures/views.py                             |  6 +--
 setup.py                                     |  2 +-
 tests/pipeline/test_course_daily_metrics.py  |  4 +-
 tests/test_filters.py                        |  4 +-
 tests/test_serializers.py                    |  3 +-
 tests/test_sites.py                          |  7 +--
 tests/views/test_course_enrollment_view.py   |  3 +-
 tests/views/test_general_course_data_view.py |  8 +---
 tests/views/test_general_user_data_view.py   |  3 +-
 tests/views/test_learner_details_view.py     |  7 ++-
 17 files changed, 72 insertions(+), 65 deletions(-)

diff --git a/figures/compat.py b/figures/compat.py
index 50edafcf0..30fef7914 100644
--- a/figures/compat.py
+++ b/figures/compat.py
@@ -1,7 +1,9 @@
 """Figures compatability module
 
-This module serves to provide a common access point to functionality that
-differs from  different named Open edX releases
+This module serves to provide a common access point to edx-platform objects.
+
+It was originally intended to encapsulate functionality that differs from
+different named Open edX releases
 
 We can identify the Open edX named release for Ginkgo and later by getting the
 value from openedx.core.release.RELEASE_LINE. This will be the release name as
@@ -74,7 +76,8 @@ class CourseNotFound(Exception):
 # preemptive addition. Added it here to avoid adding to figures.models
 # In fact, we should probably do a refactoring that makes all Figures import it
 # from here
-from student.models import CourseEnrollment  # noqa pylint: disable=unused-import,import-error
+from student.models import CourseAccessRole, CourseEnrollment  # noqa pylint: disable=unused-import,import-error
+from openedx.core.djangoapps.content.course_overviews.models import CourseOverview  # noqa pylint: disable=unused-import,import-error
 
 
 def course_grade(learner, course):
diff --git a/figures/filters.py b/figures/filters.py
index 3263898fc..d9ddfdd8b 100644
--- a/figures/filters.py
+++ b/figures/filters.py
@@ -27,10 +27,7 @@
 
 from opaque_keys.edx.keys import CourseKey
 
-from openedx.core.djangoapps.content.course_overviews.models import CourseOverview  # noqa pylint: disable=import-error
-
-from student.models import CourseEnrollment  # pylint: disable=import-error
-
+from figures.compat import CourseEnrollment, CourseOverview
 from figures.pipeline.course_daily_metrics import get_enrolled_in_exclude_admins
 from figures.models import (
     CourseDailyMetrics,
diff --git a/figures/models.py b/figures/models.py
index 6012aeb6a..8ae1482eb 100644
--- a/figures/models.py
+++ b/figures/models.py
@@ -116,6 +116,8 @@ class SiteDailyMetrics(TimeStampedModel):
     # Should change this to default value of 0
     mau = models.IntegerField(blank=True, null=True)
 
+    # TODO: Add field for number of CDMs reported
+
     class Meta:
         """
         SiteDailyMetrics view and serializer tests fail when we include 'site'
diff --git a/figures/pipeline/course_daily_metrics.py b/figures/pipeline/course_daily_metrics.py
index 4842dfe95..448ca5fcd 100644
--- a/figures/pipeline/course_daily_metrics.py
+++ b/figures/pipeline/course_daily_metrics.py
@@ -19,11 +19,12 @@
 from django.db import transaction
 from django.utils.timezone import utc
 
-from openedx.core.djangoapps.content.course_overviews.models import CourseOverview  # noqa pylint: disable=import-error
-from student.models import CourseEnrollment  # pylint: disable=import-error
 from student.roles import CourseCcxCoachRole, CourseInstructorRole, CourseStaffRole  # noqa pylint: disable=import-error
 
-from figures.compat import GeneratedCertificate, StudentModule
+from figures.compat import (CourseEnrollment,
+                            CourseOverview,
+                            GeneratedCertificate,
+                            StudentModule)
 from figures.helpers import as_course_key, as_datetime, next_day, prev_day
 import figures.metrics
 from figures.models import CourseDailyMetrics, PipelineError
diff --git a/figures/serializers.py b/figures/serializers.py
index c4f5dded5..0b415a4c9 100644
--- a/figures/serializers.py
+++ b/figures/serializers.py
@@ -27,12 +27,13 @@
 from rest_framework import serializers
 from rest_framework.fields import empty
 
-from openedx.core.djangoapps.content.course_overviews.models import CourseOverview  # noqa pylint: disable=import-error
 from openedx.core.djangoapps.user_api.accounts.serializers import AccountLegacyProfileSerializer  # noqa pylint: disable=import-error
 
-from student.models import CourseAccessRole, CourseEnrollment  # pylint: disable=import-error
-
-from figures.compat import RELEASE_LINE, GeneratedCertificate
+from figures.compat import (RELEASE_LINE,
+                            CourseAccessRole,
+                            CourseEnrollment,
+                            CourseOverview,
+                            GeneratedCertificate)
 from figures.helpers import as_course_key
 from figures.metrics import (
     get_course_enrolled_users_for_time_period,
diff --git a/figures/sites.py b/figures/sites.py
index a8221a7d7..208b191a1 100644
--- a/figures/sites.py
+++ b/figures/sites.py
@@ -18,13 +18,13 @@
 # TODO: Add exception handling
 import organizations
 
-from openedx.core.djangoapps.content.course_overviews.models import CourseOverview  # noqa pylint: disable=import-error
-
-from student.models import CourseEnrollment  # pylint: disable=import-error
-
-from figures.compat import StudentModule
-from figures.helpers import as_course_key
-import figures.helpers
+from figures.compat import (
+    CourseEnrollment,
+    CourseOverview,
+    GeneratedCertificate,
+    StudentModule,
+)
+from figures.helpers import as_course_key, is_multisite
 
 
 class CrossSiteResourceError(Exception):
@@ -106,7 +106,7 @@ def get_site_for_course(course_id):
     TODO: Figure out how we want to handle ``DoesNotExist``
     whether to let it raise back up raw or handle with a custom exception
     """
-    if figures.helpers.is_multisite():
+    if is_multisite():
         org_courses = organizations.models.OrganizationCourse.objects.filter(
             course_id=str(course_id))
         if org_courses:
@@ -152,7 +152,7 @@ def get_course_keys_for_site(site):
 
     We may also be able to reduce the queries here to also improve performance
     """
-    if figures.helpers.is_multisite():
+    if is_multisite():
         course_ids = site_course_ids(site)
     else:
         course_ids = CourseOverview.objects.all().values_list('id', flat=True)
@@ -164,7 +164,7 @@ def get_courses_for_site(site):
 
     This function relies on Appsembler's fork of edx-organizations
     """
-    if figures.helpers.is_multisite():
+    if is_multisite():
         course_keys = get_course_keys_for_site(site)
         courses = CourseOverview.objects.filter(id__in=course_keys)
     else:
@@ -173,7 +173,7 @@ def get_courses_for_site(site):
 
 
 def get_user_ids_for_site(site):
-    if figures.helpers.is_multisite():
+    if is_multisite():
         return get_users_for_site(site).values_list('id', flat=True)
     else:
         user_ids = get_user_model().objects.all().values_list('id', flat=True)
@@ -181,7 +181,7 @@ def get_user_ids_for_site(site):
 
 
 def get_users_for_site(site):
-    if figures.helpers.is_multisite():
+    if is_multisite():
         return get_user_model().objects.filter(organizations__sites__in=[site])
     else:
         users = get_user_model().objects.all()
@@ -189,14 +189,14 @@ def get_users_for_site(site):
 
 
 def get_course_enrollments_for_site(site):
-    if figures.helpers.is_multisite():
+    if is_multisite():
         return CourseEnrollment.objects.filter(user__organizations__sites__in=[site])
     else:
         return CourseEnrollment.objects.all()
 
 
 def get_student_modules_for_course_in_site(site, course_id):
-    if figures.helpers.is_multisite():
+    if is_multisite():
         site_id = site.id
         check_site = get_site_for_course(course_id)
         if not check_site or site_id != check_site.id:
@@ -241,3 +241,25 @@ def student_modules_for_course_enrollment(ce):
     Relies on the fact that course_ids are globally unique
     """
     return StudentModule.objects.filter(student=ce.user, course_id=ce.course_id)
+
+
+def site_certificates(site):
+    """
+    If we want to be clever, we can abstract a function:
+    ```
+    def site_user_related(site, model_class):
+        if is_multisite():
+            return model_class.objects.filter(user__organizations__sites__in=[site])
+        else:
+            return model_class.objects.all()
+
+    def site_certificates(site):
+        return site_user_related(GeneratedCertificate)
+    ```
+    Then:
+    """
+    if is_multisite():
+        return GeneratedCertificate.objects.filter(
+            user__organizations__sites__in=[site])
+    else:
+        return GeneratedCertificate.objects.all()
diff --git a/figures/tasks.py b/figures/tasks.py
index 94f051871..2fd13afec 100644
--- a/figures/tasks.py
+++ b/figures/tasks.py
@@ -6,6 +6,8 @@
 import datetime
 import time
 
+import six
+
 from django.contrib.sites.models import Site
 from django.utils.timezone import utc
 
@@ -13,11 +15,8 @@
 from celery.app import shared_task
 from celery.utils.log import get_task_logger
 
-# TODO: import CourseOverview from figures.compat
-from openedx.core.djangoapps.content.course_overviews.models import CourseOverview  # noqa pylint: disable=import-error
-from student.models import CourseEnrollment  # pylint: disable=import-error
-
 from figures.backfill import backfill_enrollment_data_for_site
+from figures.compat import CourseEnrollment, CourseOverview
 from figures.helpers import as_course_key, as_date
 from figures.log import log_exec_time
 from figures.models import PipelineError
@@ -27,7 +26,6 @@
 from figures.pipeline.mau_pipeline import collect_course_mau
 from figures.pipeline.site_monthly_metrics import fill_last_month as fill_last_smm_month
 from figures.pipeline.logger import log_error_to_db
-import six
 
 
 logger = get_task_logger(__name__)
diff --git a/figures/views.py b/figures/views.py
index 6cac0524d..4244fa344 100644
--- a/figures/views.py
+++ b/figures/views.py
@@ -39,11 +39,7 @@
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 
-# Directly including edx-platform objects for early development
-# Follow-on, we'll likely consolidate edx-platform model imports to an adapter
-from openedx.core.djangoapps.content.course_overviews.models import CourseOverview  # noqa pylint: disable=import-error
-
-from figures.compat import CourseEnrollment
+from figures.compat import CourseEnrollment, CourseOverview
 from figures.filters import (
     CourseDailyMetricsFilter,
     CourseEnrollmentFilter,
diff --git a/setup.py b/setup.py
index 6b620dc3a..61188562f 100644
--- a/setup.py
+++ b/setup.py
@@ -26,7 +26,7 @@
 
 setup(
     name='Figures',
-    version='0.4.dev6',
+    version='0.4.dev7',
     packages=find_packages(),
     include_package_data=True,
     license='MIT',
diff --git a/tests/pipeline/test_course_daily_metrics.py b/tests/pipeline/test_course_daily_metrics.py
index 100750d03..e5b74e48b 100644
--- a/tests/pipeline/test_course_daily_metrics.py
+++ b/tests/pipeline/test_course_daily_metrics.py
@@ -10,10 +10,8 @@
 
 from django.core.exceptions import PermissionDenied, ValidationError
 
-from student.models import CourseEnrollment, CourseAccessRole
-
+from figures.compat import CourseAccessRole, CourseEnrollment
 from figures.helpers import as_datetime, next_day, prev_day
-
 from figures.models import CourseDailyMetrics, PipelineError
 from figures.pipeline import course_daily_metrics as pipeline_cdm
 import figures.sites
diff --git a/tests/test_filters.py b/tests/test_filters.py
index 54994e3ed..c256f91c0 100644
--- a/tests/test_filters.py
+++ b/tests/test_filters.py
@@ -34,9 +34,7 @@
 from django.contrib.sites.models import Site
 from django.test import TestCase
 
-from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
-from student.models import CourseEnrollment
-
+from figures.compat import CourseEnrollment, CourseOverview
 from figures.filters import (
     CourseDailyMetricsFilter,
     CourseEnrollmentFilter,
diff --git a/tests/test_serializers.py b/tests/test_serializers.py
index 383918fe2..5b03bb8e1 100644
--- a/tests/test_serializers.py
+++ b/tests/test_serializers.py
@@ -15,8 +15,7 @@
 from django.utils.timezone import utc
 from rest_framework.exceptions import ValidationError
 
-from student.models import CourseEnrollment
-
+from figures.compat import CourseEnrollment
 from figures.models import (
     CourseDailyMetrics,
     CourseMauMetrics,
diff --git a/tests/test_sites.py b/tests/test_sites.py
index e9c755634..e14feceae 100644
--- a/tests/test_sites.py
+++ b/tests/test_sites.py
@@ -31,10 +31,11 @@
 
 import organizations
 
-from openedx.core.djangoapps.content.course_overviews.models import (
-    CourseOverview,
-)
+# from openedx.core.djangoapps.content.course_overviews.models import (
+#     CourseOverview,
+# )
 
+from figures.compat import CourseOverview
 import figures.helpers
 import figures.sites
 
diff --git a/tests/views/test_course_enrollment_view.py b/tests/views/test_course_enrollment_view.py
index f67e078f5..49d788811 100644
--- a/tests/views/test_course_enrollment_view.py
+++ b/tests/views/test_course_enrollment_view.py
@@ -14,8 +14,7 @@
 
 from opaque_keys.edx.keys import CourseKey
 
-from student.models import CourseEnrollment
-
+from figures.compat import CourseEnrollment
 from figures.helpers import is_multisite
 from figures.views import CourseEnrollmentViewSet
 
diff --git a/tests/views/test_general_course_data_view.py b/tests/views/test_general_course_data_view.py
index da2ccfbff..cb0a0172a 100644
--- a/tests/views/test_general_course_data_view.py
+++ b/tests/views/test_general_course_data_view.py
@@ -7,19 +7,13 @@
 from django.contrib.sites.models import Site
 from django.db.models import F
 
-
-
 from rest_framework.test import (
     APIRequestFactory,
     #RequestsClient, Not supported in older  rest_framework versions
     force_authenticate,
     )
 
-from openedx.core.djangoapps.content.course_overviews.models import (
-    CourseOverview,
-)
-from student.models import CourseEnrollment
-
+from figures.compat import CourseEnrollment, CourseOverview
 import figures.helpers
 from figures.helpers import as_course_key, is_multisite
 from figures.views import GeneralCourseDataViewSet
diff --git a/tests/views/test_general_user_data_view.py b/tests/views/test_general_user_data_view.py
index 6526b322b..4a3793151 100644
--- a/tests/views/test_general_user_data_view.py
+++ b/tests/views/test_general_user_data_view.py
@@ -41,8 +41,7 @@
     force_authenticate,
     )
 
-from student.models import CourseEnrollment
-
+from figures.compat import CourseEnrollment
 from figures.helpers import is_multisite
 from figures.views import GeneralUserDataViewSet
 
diff --git a/tests/views/test_learner_details_view.py b/tests/views/test_learner_details_view.py
index 6f8ad2ec2..27ab285fa 100644
--- a/tests/views/test_learner_details_view.py
+++ b/tests/views/test_learner_details_view.py
@@ -1,4 +1,4 @@
-'''Tests Figures UserIndexView class
+"""Tests Figures UserIndexView class
 
 TODO: make reasonable endpoint
 
@@ -27,7 +27,7 @@
 ]
 
 
-'''
+"""
 
 from __future__ import absolute_import
 import mock
@@ -42,8 +42,7 @@
     force_authenticate,
     )
 
-from student.models import CourseEnrollment
-
+from figures.compat import CourseEnrollment
 from figures.helpers import as_course_key
 from figures.serializers import LearnerCourseDetailsSerializer
 from figures.sites import get_course_enrollments_for_site