Return bulk-add results instead of logging them (#155)

`api.bulk_add_organization` and `api.bulk_add_organization_courses`
previously logged the organizations and organization-course linkages
they were about to create/reactivate.

This was somewhat helpful, but parsing the log output by
hand is more time-consuming and error prone than
simply returning the data and allowing the caller decide
how to process it

TNL-7774

organizations/__init__.py

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

organizations/api.py

@@ -80,20 +80,31 @@ def bulk_add_organizations(organization_data_items, dry_run=False):
 
         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.
+            If True, don't apply changes, but still return organizations
+            that would have been created or reactivated.
 
     Raises:
         InvalidOrganizationException: One or more organization dictionaries
             have missing or invalid data; no organizations were created.
+
+    Returns: tuple[set[str], set[str]]
+
+        A tuple in the form: (
+            short names of organizations that were newly created,
+            short names of organizations that we reactivated
+        )
+        From an API layer point of view, the organizations that were "added"
+        is the union of the organizations that were *newly created* and those
+        that were *reactivated*. We distinguish between them in the return
+        value to allow for richer reporting by users of this function.
     """
     for organization_data in organization_data_items:
         _validate_organization_data(organization_data)
         if "short_name" not in organization_data:
             raise exceptions.InvalidOrganizationException(
                 "Organization is missing short_name: {}".format(organization_data)
             )
-    data.bulk_create_organizations(organization_data_items, dry_run)
+    return data.bulk_create_organizations(organization_data_items, dry_run)
 
 
 def edit_organization(organization_data):
@@ -167,14 +178,31 @@ def bulk_add_organization_courses(organization_course_pairs, dry_run=False):
 
         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.
+            If True, don't apply changes, but still return organization-course
+            linkages that would have been created or reactivated.
 
     Raises:
         InvalidOrganizationException: One or more organization dictionaries
             have missing or invalid data.
         InvalidCourseKeyException: One or more course keys could not be parsed.
         (in case of either exception, no org-course linkages are created).
+
+    Returns: tuple[
+                set[tuple[str, CourseKey],
+                set[tuple[str, CourseKey]
+            ]
+
+        A tuple in the form: (
+            organization-course linkages that we newly created,
+            organization-course linkages that we reactivated
+        )
+        where the `str` objects are organization short names.
+
+        From an API layer point of view, the organization-
+        course linkages that were "added" is the union of the linkages that were
+        *newly created* and those that were *reactivated*.
+        We distinguish between them in the return value to allow for richer
+        reporting by users of this function.
     """
     for organization_data, course_key in organization_course_pairs:
         _validate_organization_data(organization_data)
@@ -183,7 +211,7 @@ def bulk_add_organization_courses(organization_course_pairs, dry_run=False):
                 "Organization is missing short_name: {}".format(organization_data)
             )
         _validate_course_key(course_key)
-    data.bulk_create_organization_courses(organization_course_pairs, dry_run)
+    return data.bulk_create_organization_courses(organization_course_pairs, dry_run)
 
 
 def get_organization_courses(organization_data):

organizations/data.py

@@ -164,8 +164,15 @@ def bulk_create_organizations(organizations, dry_run=False):
 
         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.
+            If True, don't apply changes, but still return organizations
+            that would have been created or reactivated.
+
+    Returns: tuple[set[str], set[str]]
+
+        A tuple in the form: (
+            short names of organizations that were newly created,
+            short names of organizations that we reactivated
+        )
     """
     # Collect organizations by short name, dropping conflicts as necessary.
     organization_objs = [
@@ -205,30 +212,25 @@ def bulk_create_organizations(organizations, dry_run=False):
     ]
     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(
+    # Collect sets of orgs that will be reactivated and created,
+    # so that we can have an informative return value.
+    short_names_of_organizations_to_reactivate = set(
         organizations_to_reactivate.values_list("short_name", flat=True)
     )
-    short_names_of_organizations_to_create = [
+    short_names_of_organizations_to_create = {
         org.short_name for org in organizations_to_create
-    ]
-    log.info(
-        "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),
+    }
+
+    # If not a dry run,
+    # re-activate existing organizations, and create the new ones.
+    if not dry_run:
+        organizations_to_reactivate.update(active=True)
+        internal.Organization.objects.bulk_create(organizations_to_create)
+
+    return (
         short_names_of_organizations_to_create,
+        short_names_of_organizations_to_reactivate,
     )
-    if dry_run:
-        return
-
-    # Activate existing organizations, and create the new ones.
-    organizations_to_reactivate.update(active=True)
-    internal.Organization.objects.bulk_create(organizations_to_create)
 
 
 def update_organization(organization):
@@ -337,8 +339,20 @@ def bulk_create_organization_courses(organization_course_pairs, dry_run=False):
 
         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.
+            If True, don't apply changes, but still return organization-course
+            linkages that would have been created or reactivated.
+
+    Returns: tuple[
+                set[tuple[str, str]],
+                set[tuple[str, str]]
+             ]
+
+        A tuple in the form: (
+            organization-course linkages that we newly created,
+            organization-course linkages that we reactivated
+        )
+        where an "organization-course" linkage is a tuple in the form:
+            (organization short name, course key string).
     """
     def linkage_to_pair(linkage):
         """
@@ -395,24 +409,13 @@ def linkage_to_pair(linkage):
         )
     ]
 
-    # Log what we're about to do.
     # If this is a dry run, return before applying any changes to the db.
     linkage_pairs_to_reactivate = {
         linkage_to_pair(linkage)
         for linkage in linkages_to_reactivate
     }
-    log.info(
-        "Organization-course linkages to be bulk-reactivated (n=%i): %r. ",
-        len(linkage_pairs_to_reactivate),
-        list(linkage_pairs_to_reactivate),
-    )
-    log.info(
-        "Organization-course linkages to be bulk-created (n=%i): %r.",
-        len(linkage_pairs_to_create),
-        list(linkage_pairs_to_create),
-    )
     if dry_run:
-        return
+        return linkage_pairs_to_create, linkage_pairs_to_reactivate
 
     # Bulk-reactivate existing organization-course linkages.
     ids_of_linkages_to_reactivate = {linkage.id for linkage in linkages_to_reactivate}
@@ -443,6 +446,7 @@ def linkage_to_pair(linkage):
         for org_short_name, course_id
         in linkage_pairs_to_create
     ])
+    return linkage_pairs_to_create, linkage_pairs_to_reactivate
 
 
 def query_organizations_by_short_name(short_names):

organizations/tests/test_api.py

@@ -457,20 +457,19 @@ def test_add_no_organizations(self):
         api.bulk_add_organizations([])
         assert len(api.get_organizations()) == 0
 
-    @patch.object(data_module_logger, 'info', autospec=True)
-    def test_dry_run(self, mock_log_info):
+    def test_dry_run(self):
         """
         Test that `bulk_add_organizations` does nothing when `dry_run` is
         specified (except logging).
         """
-        api.bulk_add_organizations(
+        would_create, would_reactivate = api.bulk_add_organizations(
             [self.make_organization_data("org_a")],
             dry_run=True,
         )
 
         assert api.get_organizations() == []
-        # One for reactivations, one for creations.
-        assert mock_log_info.call_count == 2
+        assert would_create == {"org_a"}
+        assert would_reactivate == set()
 
     @patch.object(data_module_logger, 'info', autospec=True)
     def test_edge_cases(self, mock_log_info):
@@ -496,7 +495,7 @@ def test_edge_cases(self, mock_log_info):
         # 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([
+            created, reactivated = api.bulk_add_organizations([
 
                 # New organization.
                 self.make_organization_data("org_X"),
@@ -529,6 +528,12 @@ def test_edge_cases(self, mock_log_info):
             "EXISTING_ORG", "org_to_reactivate", "org_X", "org_Y"
         }
 
+        # Based on the return value,
+        #    created vs. reactivated vs. not touched
+        # is true for the organizations passed to `bulk_add_organizations`.
+        assert reactivated == {"org_to_reactivate"}
+        assert created == {"org_X", "org_Y"}
+
         # Organization dicts with already-taken short_names shouldn't have modified
         # the existing orgs.
         assert "this name should be ignored" not in {
@@ -547,18 +552,6 @@ def test_edge_cases(self, mock_log_info):
             self.make_organization_data("org_X")["short_name"]
         )
 
-        # Based on logging messages, make sure the expected breakdown of
-        #    created vs. reactivated vs. not touched
-        # is true for the organizations passed to `bulk_add_organizations`.
-        logged_orgs_to_reactivate = mock_log_info.call_args_list[1][0][2]
-        assert set(logged_orgs_to_reactivate) == {
-            "org_to_reactivate"
-        }
-        logged_orgs_to_create = mock_log_info.call_args_list[2][0][2]
-        assert set(logged_orgs_to_create) == {
-            "org_X", "org_Y"
-        }
-
     def test_add_several_organizations(self):
         """
         Test that the query_count of bulk_add_organizations does not increase
@@ -633,23 +626,23 @@ def test_add_no_organization_courses(self):
         with self.assertNumQueries(1):
             api.bulk_add_organization_courses([])
 
-    @patch.object(data_module_logger, 'info', autospec=True)
-    def test_dry_run(self, mock_log_info):
+    def test_dry_run(self):
         """
         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)
+        would_create, would_reactivate = api.bulk_add_organization_courses(
+            [(org_a, course_key_x)], dry_run=True
+        )
 
         assert api.get_organization_courses(org_a) == []
-        # One for reactivations, one for creations.
-        assert mock_log_info.call_count == 2
+        assert would_create == {("org_a", "course-v1:x+x+x")}
+        assert would_reactivate == set()
 
-    @patch.object(data_module_logger, 'info', autospec=True)
-    def test_edge_cases(self, mock_log_info):
+    def test_edge_cases(self):
         """
         Test that bulk_add_organization_courses handles a few edge cases as expected.
         """
@@ -677,7 +670,7 @@ def test_edge_cases(self, mock_log_info):
         # 1 query to get organiations for new linkages,
         # 1 query to create new linkages.
         with self.assertNumQueries(5):
-            api.bulk_add_organization_courses([
+            created, reactivated = api.bulk_add_organization_courses([
 
                 # A->X: Existing linkage, should be a no-op.
                 (org_a, course_key_x),
@@ -708,6 +701,17 @@ def test_edge_cases(self, mock_log_info):
             "course-v1:x+x+x", "course-v1:y+y+y"
         }
 
+        # Based on return value, make sure the expected breakdown of
+        #    created vs. reactivated vs. not touched
+        # is true for the org-course linkages passed to `bulk_add_organization_courses`.
+        assert created == {
+            ("org_b", str(course_key_y)),
+            ("org_b", str(course_key_z)),
+        }
+        assert reactivated == {
+            ("org_a", str(course_key_y)),
+        }
+
         # Org B was linked to courses Y and Z.
         org_b_courses = api.get_organization_courses(org_b)
         assert {
@@ -716,19 +720,6 @@ def test_edge_cases(self, mock_log_info):
             "course-v1:y+y+y", "course-v1:z+z+z"
         }
 
-        # Based on logging messages, make sure the expected breakdown of
-        #    created vs. reactivated vs. not touched
-        # is true for the org-course linkages passed to `bulk_add_organization_courses`.
-        logged_linkages_to_reactivate = mock_log_info.call_args_list[0][0][2]
-        assert set(logged_linkages_to_reactivate) == {
-            ("org_a", str(course_key_y)),
-        }
-        logged_linkages_to_create = mock_log_info.call_args_list[1][0][2]
-        assert set(logged_linkages_to_create) == {
-            ("org_b", str(course_key_y)),
-            ("org_b", str(course_key_z)),
-        }
-
     def test_add_several_organization_courses(self):
         """
         Test that the query_count of bulk_add_organization_courses does not increase