Merge 92b541a into b43f7e3

organizations/__init__.py

@@ -1,4 +1,4 @@
 """
 edx-organizations app initialization module
 """
-__version__ = '6.1.0'  # pragma: no cover
+__version__ = '6.2.0'  # pragma: no cover

organizations/api.py

@@ -51,7 +51,7 @@ def add_organization(organization_data):
     return organization
 
 
-def bulk_add_organizations(organization_data_items):
+def bulk_add_organizations(organization_data_items, dry_run=False):
     """
     Efficiently store multiple organizations.
 
@@ -78,6 +78,11 @@ def bulk_add_organizations(organization_data_items):
             If multiple organizations share a `short_name`, the first organization
             in `organization_data_items` will be used, and the latter ones ignored.
 
+        dry_run (bool):
+            Optional, defaulting to False.
+            If True, log organizations that would be created or activated,
+            but do not actually apply the changes to the database.
+
     Raises:
         InvalidOrganizationException: One or more organization dictionaries
             have missing or invalid data; no organizations were created.
@@ -88,7 +93,7 @@ def bulk_add_organizations(organization_data_items):
             raise exceptions.InvalidOrganizationException(
                 "Organization is missing short_name: {}".format(organization_data)
             )
-    data.bulk_create_organizations(organization_data_items)
+    data.bulk_create_organizations(organization_data_items, dry_run)
 
 
 def edit_organization(organization_data):
@@ -142,7 +147,7 @@ def add_organization_course(organization_data, course_key):
     )
 
 
-def bulk_add_organization_courses(organization_course_pairs):
+def bulk_add_organization_courses(organization_course_pairs, dry_run=False):
     """
     Efficiently store multiple organization-course relationships.
 
@@ -160,6 +165,11 @@ def bulk_add_organization_courses(organization_course_pairs):
 
             Assumption: All provided organizations already exist in storage.
 
+        dry_run (bool):
+            Optional, defaulting to False.
+            If True, log organizations-course linkages that would be created or
+            activated, but do not actually apply the changes to the database.
+
     Raises:
         InvalidOrganizationException: One or more organization dictionaries
             have missing or invalid data.
@@ -173,7 +183,7 @@ def bulk_add_organization_courses(organization_course_pairs):
                 "Organization is missing short_name: {}".format(organization_data)
             )
         _validate_course_key(course_key)
-    data.bulk_create_organization_courses(organization_course_pairs)
+    data.bulk_create_organization_courses(organization_course_pairs, dry_run)
 
 
 def get_organization_courses(organization_data):

organizations/data.py

@@ -137,7 +137,7 @@ def create_organization(organization):
     return serializers.serialize_organization(organization)
 
 
-def bulk_create_organizations(organizations):
+def bulk_create_organizations(organizations, dry_run=False):
     """
     Efficiently insert multiple organizations into the database.
 
@@ -159,6 +159,11 @@ def bulk_create_organizations(organizations):
 
             If multiple organizations share a `short_name`, the first organization
             in `organizations` will be used, and the latter ones ignored.
+
+        dry_run (bool):
+            Optional, defaulting to False.
+            If True, log organizations that would be created or activated,
+            but do not actually apply the changes to the database.
     """
     # Collect organizations by short name, dropping conflicts as necessary.
     organization_objs = [
@@ -184,29 +189,45 @@ def bulk_create_organizations(organizations):
             continue
         organizations_by_short_name[short_name_lower] = organization
 
-    # Find out which organizations we need to create vs. activate.
-    organizations_to_activate = internal.Organization.objects.filter(
+    # Find out which organizations we need to reactivate vs. create.
+    existing_organizations = internal.Organization.objects.filter(
         short_name__in=organizations_by_short_name
     )
-    organization_short_names_to_activate = {
+    existing_organization_short_names = {
         short_name.lower()
         for short_name
-        in organizations_to_activate.values_list("short_name", flat=True)
+        in existing_organizations.values_list("short_name", flat=True)
     }
     organizations_to_create = [
         organization
         for short_name, organization in organizations_by_short_name.items()
-        if short_name.lower() not in organization_short_names_to_activate
+        if short_name.lower() not in existing_organization_short_names
+    ]
+    organizations_to_reactivate = existing_organizations.filter(active=False)
+
+    # Log what we're about to do.
+    # If this is a dry run, return before applying any changes to the db.
+    short_names_of_organizations_to_reactivate = list(
+        organizations_to_reactivate.values_list("short_name", flat=True)
+    )
+    short_names_of_organizations_to_create = [
+        org.short_name for org in organizations_to_create
     ]
     log.info(
-        "Organizations to be bulk-reactivated: %r. "
-        "Organizations to be bulk-created: %r.",
-        organization_short_names_to_activate,
-        {org.short_name for org in organizations_to_create},
+        "Organizations to be bulk-reactivated (n=%i): %r. ",
+        len(short_names_of_organizations_to_reactivate),
+        short_names_of_organizations_to_reactivate,
     )
+    log.info(
+        "Organizations to be bulk-created (n=%i): %r.",
+        len(short_names_of_organizations_to_create),
+        short_names_of_organizations_to_create,
+    )
+    if dry_run:
+        return
 
     # Activate existing organizations, and create the new ones.
-    organizations_to_activate.update(active=True)
+    organizations_to_reactivate.update(active=True)
     internal.Organization.objects.bulk_create(organizations_to_create)
 
 
@@ -299,7 +320,7 @@ def create_organization_course(organization, course_key):
         )
 
 
-def bulk_create_organization_courses(organization_course_pairs):
+def bulk_create_organization_courses(organization_course_pairs, dry_run=False):
     """
     Efficiently insert multiple organization-course relationships into the database.
 
@@ -313,6 +334,11 @@ def bulk_create_organization_courses(organization_course_pairs):
             Organization-course linkages that DO already exist will be activated, if inactive.
 
             Assumption: All provided organizations already exist in the DB.
+
+        dry_run (bool):
+            Optional, defaulting to False.
+            If True, log organizations-course linkaages that would be created or
+            activated, but do not actually apply the changes to the database.
     """
     # For sake of having sane variable names, please understand
     #   * "orgslug" to mean "lowercase organization short name" and
@@ -327,39 +353,64 @@ def bulk_create_organization_courses(organization_course_pairs):
         in organization_course_pairs
     }
 
-    # Grab all existing (lowercased org short name, course key string) pairs from db,
-    # filtering for the ones requested for creation in `organization_course_pairs`, and
-    # indexing by db `id` of org_course object (we need this later for the bulk update).
-    orgslug_courseid_pairs_to_activate_by_id = {
-        org_course_id: (short_name.lower(), course_id)
-        for org_course_id, short_name, course_id
+    # Grab all existing (lowercased org short name, course key string, is_active) triples
+    # from db, filtering for the ones requested for creation in `organization_course_pairs`,
+    # and indexing by db `id` of org_course object (we need this later for the bulk update).
+    existing_orgslug_courseid_triples_by_id = {
+        org_course_id: (short_name.lower(), course_id, is_active)
+        for org_course_id, short_name, course_id, is_active
         in internal.OrganizationCourse.objects.values_list(
-            "id", "organization__short_name", "course_id"
+            "id", "organization__short_name", "course_id", "active"
         )
         if (short_name.lower(), course_id) in orgslug_courseid_pairs
     }
-
-    # Working backwards from the set of pairs we need to *activate*,
-    # find the set of pairs we need to *create*.
-    orgslug_courseid_pairs_to_activate = set(
-        orgslug_courseid_pairs_to_activate_by_id.values()
+    existing_orgslug_courseid_triples = set(
+        existing_orgslug_courseid_triples_by_id.values()
     )
+
+    # Now that we have the set of orgslug/courseid linkages that *already exist*,
+    # determine which ones need to be *reactivated*
+    # and which linkages from our original set we still need to *create*.
     orgslug_courseid_pairs_to_create = {
         (orgslug, courseid)
         for orgslug, courseid in orgslug_courseid_pairs
-        if (orgslug, courseid) not in orgslug_courseid_pairs_to_activate
+        if not (
+            # If an organization-course linkage already exists as either
+            # active OR inactive, we do not need to create it.
+            (orgslug, courseid, True) in existing_orgslug_courseid_triples or (
+                (orgslug, courseid, False) in existing_orgslug_courseid_triples
+            )
+        )
+    }
+    orgslug_courseid_pairs_to_reactivate_by_id = {
+        org_course_id: (orgslug, courseid)
+        for org_course_id, (orgslug, courseid, is_active)
+        in existing_orgslug_courseid_triples_by_id.items()
+        if not is_active
     }
+    orgslug_courseid_pairs_to_reactivate = set(
+        orgslug_courseid_pairs_to_reactivate_by_id.values()
+    )
+
+    # Log what we're about to do.
+    # If this is a dry run, return before applying any changes to the db.
+    log.info(
+        "Organization-course linkages to be bulk-reactivated (n=%i): %r. ",
+        len(orgslug_courseid_pairs_to_reactivate),
+        list(orgslug_courseid_pairs_to_reactivate),
+    )
     log.info(
-        "Organization-course linkages to be bulk-reactivated: %r. "
-        "Organization-course linkages to be bulk-created: %r.",
-        orgslug_courseid_pairs_to_activate,
-        orgslug_courseid_pairs_to_create,
+        "Organization-course linkages to be bulk-created (n=%i): %r.",
+        len(orgslug_courseid_pairs_to_create),
+        list(orgslug_courseid_pairs_to_create),
     )
+    if dry_run:
+        return
 
     # Activate existing organization-course linkages.
-    ids_of_org_courses_to_activate = set(orgslug_courseid_pairs_to_activate_by_id)
+    ids_of_org_courses_to_reactivate = set(orgslug_courseid_pairs_to_reactivate_by_id)
     internal.OrganizationCourse.objects.filter(
-        id__in=ids_of_org_courses_to_activate
+        id__in=ids_of_org_courses_to_reactivate
     ).update(
         active=True
     )

organizations/tests/test_api.py

@@ -11,6 +11,7 @@
 import organizations.api as api
 import organizations.exceptions as exceptions
 import organizations.tests.utils as utils
+from organizations.data import log as data_module_logger
 
 
 @ddt.ddt
@@ -464,6 +465,20 @@ def test_add_no_organizations(self):
         api.bulk_add_organizations([])
         assert len(api.get_organizations()) == 0
 
+    @patch.object(data_module_logger, 'info')
+    def test_dry_run(self, mock_log_info):
+        """
+        Test that `bulk_add_organizations` does nothing when `dry_run` is
+        specified (except logging).
+        """
+        api.bulk_add_organizations(
+            [self.make_organization_data("org_a")],
+            dry_run=True,
+        )
+
+        assert api.get_organizations() == []
+        assert mock_log_info.call_count == 1
+
     def test_edge_cases(self):
         """
         Test that bulk_add_organizations handles a few edge cases as expected.
@@ -481,8 +496,10 @@ def test_edge_cases(self):
             )["id"]
         )
 
-        # 1 query to load list of existing orgs, 1 query for create, and 1 query for update.
-        with self.assertNumQueries(3):
+        # 1 query to load list of existing orgs,
+        # 1 query to filter for only inactive existing orgs,
+        # 1 query for create, and 1 query for update.
+        with self.assertNumQueries(4):
             api.bulk_add_organizations([
 
                 # New organization.
@@ -528,14 +545,21 @@ def test_add_several_organizations(self):
         when given more organizations.
         """
         existing_org = api.add_organization(self.make_organization_data("existing_org"))
+        api.remove_organization(
+            api.add_organization(
+                self.make_organization_data("org_to_reactivate")
+            )["id"]
+        )
 
         # 1 query to load list of existing orgs,
+        # 1 query to filter for only inactive existing orgs,
         # 1 query for activate-existing, and 1 query for create-new.
-        with self.assertNumQueries(3):
+        with self.assertNumQueries(4):
             api.bulk_add_organizations([
                 existing_org,
                 existing_org,
                 existing_org,
+                self.make_organization_data("org_to_reactivate"),
                 self.make_organization_data("new_org_1"),
                 self.make_organization_data("new_org_2"),
                 self.make_organization_data("new_org_3"),
@@ -548,7 +572,7 @@ def test_add_several_organizations(self):
                 self.make_organization_data("new_org_9"),  # Redundant.
                 self.make_organization_data("new_org_9"),  # Redundant.
             ])
-        assert len(api.get_organizations()) == 10
+        assert len(api.get_organizations()) == 11
 
 
 class BulkAddOrganizationCoursesTestCase(utils.OrganizationsTestCaseBase):
@@ -581,14 +605,28 @@ def test_validation_errors(self):
         # In either case, no data should've been written for `valid_org`.
         assert len(api.get_organization_courses(valid_org)) == 0
 
-    def test_add_no_organizations(self):
+    def test_add_no_organization_courses(self):
         """
         Test that `bulk_add_organization_courses` works given an an empty list.
         """
         # 1 query to load list of existing org-courses.
         with self.assertNumQueries(1):
             api.bulk_add_organization_courses([])
 
+    @patch.object(data_module_logger, 'info')
+    def test_dry_run(self, mock_log_info):
+        """
+        Test that `bulk_add_organization_courses` does nothing when `dry_run` is
+        specified (except logging).
+        """
+        org_a = api.add_organization(self.make_organization_data("org_a"))
+        course_key_x = CourseKey.from_string("course-v1:x+x+x")
+
+        api.bulk_add_organization_courses([(org_a, course_key_x)], dry_run=True)
+
+        assert api.get_organization_courses(org_a) == []
+        assert mock_log_info.call_count == 1
+
     def test_edge_cases(self):
         """
         Test that bulk_add_organization_courses handles a few edge cases as expected.
@@ -667,14 +705,18 @@ def test_add_several_organization_courses(self):
         # Add linkage A->X.
         api.add_organization_course(org_a, course_key_x)
 
+        # Add linkage A->Y, and then remove (actually: deactivate).
+        api.add_organization_course(org_a, course_key_y)
+        api.remove_organization_course(org_a, course_key_y)
+
         # 1 query to load list of existing org-courses,
         # 1 query for activate-existing, and 1 query for create-new.
         with self.assertNumQueries(3):
             api.bulk_add_organization_courses([
                 (org_a, course_key_x),  # Already existing.
                 (org_a, course_key_x),  # Already existing.
-                (org_a, course_key_y),  # The rest are new.
-                (org_a, course_key_z),
+                (org_a, course_key_y),  # Reactivation.
+                (org_a, course_key_z),  # The rest are new.
                 (org_b, course_key_x),
                 (org_b, course_key_y),
                 (org_b, course_key_z),