From 46ce3e7551d1b0921327a0b992e660f689828317 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Mon, 14 Dec 2020 18:19:24 -0500 Subject: [PATCH] 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 | 2 +- organizations/api.py | 40 +++++++++++++++--- organizations/data.py | 74 +++++++++++++++++---------------- organizations/tests/test_api.py | 69 +++++++++++++----------------- 4 files changed, 104 insertions(+), 81 deletions(-) diff --git a/organizations/__init__.py b/organizations/__init__.py index c2193b9e..756a5a74 100644 --- a/organizations/__init__.py +++ b/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 diff --git a/organizations/api.py b/organizations/api.py index 85d03d1d..67c39ffe 100644 --- a/organizations/api.py +++ b/organizations/api.py @@ -80,12 +80,23 @@ 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) @@ -93,7 +104,7 @@ def bulk_add_organizations(organization_data_items, dry_run=False): 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): diff --git a/organizations/data.py b/organizations/data.py index ddd4ae23..fea448f4 100644 --- a/organizations/data.py +++ b/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): diff --git a/organizations/tests/test_api.py b/organizations/tests/test_api.py index 812cdf28..dde4a98e 100644 --- a/organizations/tests/test_api.py +++ b/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,8 +626,7 @@ 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). @@ -642,14 +634,15 @@ def test_dry_run(self, mock_log_info): 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