diff --git a/docs/dynamodb.md b/docs/dynamodb.md index b636835d..e332bb3d 100644 --- a/docs/dynamodb.md +++ b/docs/dynamodb.md @@ -26,8 +26,3 @@ For example: * Github Comment / Review Comment -> Asana comment All Asana objects created or used by SGTM should be tracked in this table. When handling incoming webhooks, SGTM will fetch relevant objects to update if they exist, otherwise create a new object and add it to the table. - -#### sgtm-users -A mapping of GitHub users (github handle string) to Asana users (asana user id string). - -This table is read by SGTM to convert GitHub users to corresponding Asana users. The mapping is currently kept updated via [a lambda](/src/sync_users/handler.py). Webhook events handled by SGTM should never write to this table. diff --git a/src/asana/helpers.py b/src/asana/helpers.py index ff52d221..e4b68346 100644 --- a/src/asana/helpers.py +++ b/src/asana/helpers.py @@ -105,7 +105,7 @@ def _build_status_from_pull_request(pull_request: PullRequest) -> Optional[str]: def _author_asana_user_id_from_pull_request(pull_request: PullRequest) -> Optional[str]: return dynamodb_client.get_asana_domain_user_id_from_github_handle( - github_handle=pull_request.author_handle() + pull_request.author_handle() ) @@ -172,9 +172,7 @@ def _task_assignee_from_pull_request(pull_request: PullRequest) -> Optional[str] if asana_logic.should_set_task_owner_to_pr_author(pull_request): return _author_asana_user_id_from_pull_request(pull_request) assignee = pull_request.assignee() - return dynamodb_client.get_asana_domain_user_id_from_github_handle( - github_handle=assignee.login - ) + return dynamodb_client.get_asana_domain_user_id_from_github_handle(assignee.login) def _asana_display_name_for_github_user(github_user: User) -> str: diff --git a/src/config.py b/src/config.py index 110763c8..eba96265 100644 --- a/src/config.py +++ b/src/config.py @@ -11,10 +11,8 @@ ENV = os.getenv("ENV", "dev") LOCK_TABLE = os.getenv("LOCK_TABLE", "sgtm-lock") OBJECTS_TABLE = os.getenv("OBJECTS_TABLE", "sgtm-objects") -USERS_TABLE = os.getenv("USERS_TABLE", "sgtm-users") -ASANA_USERS_PROJECT_ID = os.getenv("ASANA_USERS_PROJECT_ID", "") GITHUB_USERNAMES_TO_ASANA_GIDS_S3_PATH = os.getenv( - "GITHUB_USERNAMES_TO_ASANA_GIDS_S3_PATH", "" + "GITHUB_USERNAMES_TO_ASANA_GIDS_S3_PATH", ) # Feature flags diff --git a/src/dynamodb/client.py b/src/dynamodb/client.py index fd401de7..b0c50a3b 100644 --- a/src/dynamodb/client.py +++ b/src/dynamodb/client.py @@ -9,7 +9,6 @@ from src.config import ( OBJECTS_TABLE, - USERS_TABLE, GITHUB_USERNAMES_TO_ASANA_GIDS_S3_PATH, ) from src.logger import logger @@ -120,64 +119,6 @@ def bulk_insert_github_node_to_asana_id_mapping( ] return self.bulk_insert_items_in_batches(OBJECTS_TABLE, items) - # USERS TABLE - - def DEPRECATED_bulk_insert_github_handle_to_asana_user_id_mapping( - self, gh_and_asana_ids: List[Tuple[str, str]] - ): - """Insert multiple mappings from github handle to Asana user ids. - - This is marked as deprecated since we'd like to read SGTM user info from S3 instead of - DynamoDB. - """ - items = [ - { - self.GITHUB_HANDLE_KEY: {"S": gh_handle}, - self.USER_ID_KEY: {"S": asana_user_id}, - } - for gh_handle, asana_user_id in gh_and_asana_ids - ] - return self.bulk_insert_items_in_batches(USERS_TABLE, items) - - @memoize - def DEPRECATED_get_asana_domain_user_id_from_github_handle( - self, github_handle: str - ) -> Optional[str]: - """ - Retrieves the Asana domain user-id associated with a specific GitHub user login, or None, - if no such association exists. User-id associations are created manually via an external process. - TODO: document this process, and create scripts to encapsulate it - - This is marked as deprecated since we'd like to read SGTM user info from S3 instead of - DynamoDB. - """ - response = self.client.get_item( - TableName=USERS_TABLE, - Key={self.GITHUB_HANDLE_KEY: {"S": github_handle.lower()}}, - ) - if "Item" in response: - return response["Item"][self.USER_ID_KEY]["S"] - else: - return None - - def DEPRECATED_get_all_user_items(self) -> Iterator[DynamoDbUserItem]: - """ - Get all DynamoDb items from the USERS_TABLE - - This is marked as deprecated since we'd like to read SGTM user info from S3 instead of - DynamoDB. - """ - response = self.client.scan(TableName=USERS_TABLE) - yield from response["Items"] - - # May need to paginate, if the first page of data is > 1MB - # (https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Scan.html#Scan.Pagination) - while response.get("LastEvaluatedKey"): - response = self.client.scan( - TableName=USERS_TABLE, ExclusiveStartKey=response["LastEvaluatedKey"] - ) - yield from response["Items"] - @staticmethod def _create_client(): # Encapsulates creating a boto3 client connection for DynamoDb with a more user-friendly error case @@ -213,8 +154,9 @@ def __init__(self): self.github_user_mapping_key_name, ) = GITHUB_USERNAMES_TO_ASANA_GIDS_S3_PATH.split("/", 1) else: - self.github_user_mapping_bucket_name = None - self.github_user_mapping_key_name = None + raise ConfigurationError( + "Configuration error: GITHUB_USERNAMES_TO_ASANA_GIDS_S3_PATH is not set to a valid S3 path" + ) # getter for the singleton @classmethod @@ -295,51 +237,6 @@ def insert_github_node_to_asana_id_mapping(gh_node_id: str, asana_id: str): ) -def get_asana_domain_user_id_from_github_handle(github_handle: str) -> Optional[str]: - """ - Using the singleton instance of S3Client, creating it if necessary: - - Retrieves the Asana domain user-id associated with a specific GitHub user login, or None, - if no such association exists. User-id associations are created manually via an external - process. - - If the S3 pathway fails, we fall back to the DynamoDb pathway. - """ - - def fallback_to_dynamo_pathway() -> Optional[str]: - dynamodb_returns = DynamoDbClient.singleton().DEPRECATED_get_asana_domain_user_id_from_github_handle( - github_handle - ) - if dynamodb_returns is not None: - # we only want to emit a warning about the S3 pathway if the dynamoDB lookup returns - # something that's not null - logger.warning( - "S3 lookup failed to find the Asana domain user id for github handle %s. The DynamoDB table returned %s. Error: %s", - github_handle, - dynamodb_returns, - traceback.format_exc(), - ) - return dynamodb_returns - - try: - if user_id := S3Client.singleton().get_asana_domain_user_id_from_github_username( - github_handle - ): - return user_id - else: - return fallback_to_dynamo_pathway() - except Exception: - return fallback_to_dynamo_pathway() - - -def DEPRECATED_get_all_user_items() -> List[dict]: - """ - This function is marked as deprecated since it's only used by the `sync_users` Lambda function, - which we are planning to deprecate in favor of reading SGTM user info from S3 instead of DynamoDB. - """ - return DynamoDbClient.singleton().DEPRECATED_get_all_user_items() - - def bulk_insert_github_node_to_asana_id_mapping( gh_and_asana_ids: List[Tuple[str, str]] ): @@ -353,16 +250,17 @@ def bulk_insert_github_node_to_asana_id_mapping( ) -def DEPRECATED_bulk_insert_github_handle_to_asana_user_id_mapping( - gh_and_asana_ids: List[Tuple[str, str]] -): +def get_asana_domain_user_id_from_github_handle(github_handle: str) -> Optional[str]: """ - This function is marked as deprecated since it's only used by the `sync_users` Lambda function, - which we are planning to deprecate in favor of reading SGTM user info from S3 instead of DynamoDB. + Using the singleton instance of S3Client, creating it if necessary: - Insert multiple mappings from github handle to Asana user ids. + Retrieves the Asana domain user-id associated with a specific GitHub user login, or None, + if no such association exists. User-id associations are created manually via an external + process. + + If the S3 pathway fails, we fall back to the DynamoDb pathway. """ - # todo why/where lol - DynamoDbClient.singleton().DEPRECATED_bulk_insert_github_handle_to_asana_user_id_mapping( - gh_and_asana_ids + + return S3Client.singleton().get_asana_domain_user_id_from_github_username( + github_handle ) diff --git a/src/sync_users/README.md b/src/sync_users/README.md deleted file mode 100644 index f2da6d63..00000000 --- a/src/sync_users/README.md +++ /dev/null @@ -1 +0,0 @@ -# todo delete this entire lambda once we are successfully pulling users from S3 instead of Asana \ No newline at end of file diff --git a/src/sync_users/__init__.py b/src/sync_users/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/src/sync_users/handler.py b/src/sync_users/handler.py deleted file mode 100644 index 0d43fb5d..00000000 --- a/src/sync_users/handler.py +++ /dev/null @@ -1,52 +0,0 @@ -from src.asana import client as asana_client -from src.config import USERS_TABLE, ASANA_USERS_PROJECT_ID -from src.dynamodb import client as dynamodb_client -from src.logger import logger -from src.sync_users.sgtm_user import SgtmUser - - -def handler(event: dict, context: dict) -> None: - """ - Entrypoint for Lambda function that syncs the mapping of Github handles - to Asana user ids with the dynamodb table USERS_TABLE. This happens - through an Asana project with custom fields - one task per user with - custom fields defined in SgtmUser (GITHUB_HANDLE_CUSTOM_FIELD_NAME, - USER_ID_CUSTOM_FIELD_NAME) - - `event` and `context` are passed into the Lambda function, but we don't - really care what they are for this function, and they are ignored - """ - logger.info( - "Starting sync from Asana project to Dynamodb {} table".format(USERS_TABLE) - ) - - users_in_dynamodb = set( - [ - SgtmUser.from_dynamodb_item(item) - for item in dynamodb_client.DEPRECATED_get_all_user_items() - ] - ) - logger.info("Found {} users in dynamodb".format(len(users_in_dynamodb))) - - asana_user_tasks_from_asana_project = asana_client.find_all_tasks_for_project( - ASANA_USERS_PROJECT_ID, opt_fields=["custom_fields"] - ) - users_in_asana = [ - u - for u in [ - SgtmUser.from_custom_fields_list(task["custom_fields"]) - for task in asana_user_tasks_from_asana_project - ] - if u is not None - ] - logger.info("Found {} users in Asana".format(len(users_in_asana))) - - users_to_add = [user for user in users_in_asana if user not in users_in_dynamodb] - logger.info("{} users to add to DynamoDb".format(len(users_to_add))) - - # Batch write the users - if len(users_to_add) > 0: - dynamodb_client.DEPRECATED_bulk_insert_github_handle_to_asana_user_id_mapping( - [(u.github_handle, u.domain_user_id) for u in users_to_add] - ) - logger.info("Done writing user mappings to DynamoDb") diff --git a/src/sync_users/sgtm_user.py b/src/sync_users/sgtm_user.py deleted file mode 100644 index 16b1f350..00000000 --- a/src/sync_users/sgtm_user.py +++ /dev/null @@ -1,74 +0,0 @@ -""" -Class representing a "User" for SGTM, which comprises of a Github handle and an -Asana domain user id. In practice, these should be actual developers in your -organization that are contributing to your repository, which have an Asana -account and a Github account. -""" -from __future__ import annotations -from typing import Any, List, Optional -from src.dynamodb.client import DynamoDbClient - - -class SgtmUser(object): - # TODO: These should be configurable probably - GITHUB_HANDLE_CUSTOM_FIELD_NAME = "Github Username" - USER_ID_CUSTOM_FIELD_NAME = "user_id" - - def __init__(self, github_handle: str, domain_user_id: str): - self.github_handle = github_handle - - # always lower-case github handles - if self.github_handle: - self.github_handle = self.github_handle.lower().strip() - - self.domain_user_id = domain_user_id - - @classmethod - def from_dynamodb_item(cls, item: dict) -> SgtmUser: - return cls( - item.get(DynamoDbClient.GITHUB_HANDLE_KEY, {}).get("S", ""), - item.get(DynamoDbClient.USER_ID_KEY, {}).get("S", ""), - ) - - @staticmethod - def _get_custom_field_value(custom_field: dict): - if custom_field["type"] == "text": - return custom_field["text_value"] - elif custom_field["type"] == "number": - return custom_field["number_value"] - elif custom_field["type"] == "enum": - return custom_field["enum_value"] - else: - raise Exception( - "Unknown custom field type: {}".format(custom_field["type"]) - ) - - @classmethod - def from_custom_fields_list( - cls, custom_fields_list: List[dict] - ) -> Optional[SgtmUser]: - github_handle = None - asana_user_id = None - for cf in custom_fields_list: - if cf["name"] == cls.USER_ID_CUSTOM_FIELD_NAME: - asana_user_id = cls._get_custom_field_value(cf) - elif cf["name"] == cls.GITHUB_HANDLE_CUSTOM_FIELD_NAME: - github_handle = cls._get_custom_field_value(cf) - - if github_handle and asana_user_id: - return cls(github_handle, str(asana_user_id)) - else: - return None - - def __eq__(self, other: Any) -> bool: - if isinstance(other, SgtmUser): - return (self.github_handle == other.github_handle) and ( - self.domain_user_id == other.domain_user_id - ) - return False - - def __ne__(self, other: Any) -> bool: - return not self.__eq__(other) - - def __hash__(self) -> int: - return hash(self.github_handle) ^ hash(self.domain_user_id) diff --git a/terraform/main.tf b/terraform/main.tf index 61da9508..57165e69 100644 --- a/terraform/main.tf +++ b/terraform/main.tf @@ -53,8 +53,7 @@ resource "aws_iam_policy" "lambda-function-cloudwatch-policy" { "logs:PutLogEvents" ], "Resource": [ - "arn:aws:logs:${var.aws_region}:*:log-group:/aws/lambda/${aws_lambda_function.sgtm.function_name}:*", - "arn:aws:logs:${var.aws_region}:*:log-group:/aws/lambda/${aws_lambda_function.sgtm_sync_users.function_name}:*" + "arn:aws:logs:${var.aws_region}:*:log-group:/aws/lambda/${aws_lambda_function.sgtm.function_name}:*" ], "Effect": "Allow" } @@ -219,46 +218,6 @@ resource "aws_lambda_function" "sgtm" { } } -resource "aws_lambda_function" "sgtm_sync_users" { - # TODO delete this once we have fully tested the new pathway - s3_bucket = aws_s3_bucket.lambda_code_s3_bucket.bucket - s3_key = aws_s3_bucket_object.lambda_code_bundle.key - function_name = "sgtm_sync_users" - role = aws_iam_role.iam_for_lambda_function.arn - handler = "src.sync_users.handler.handler" - source_code_hash = data.archive_file.create_dist_pkg.output_base64sha256 - - runtime = var.lambda_runtime - - timeout = 900 - environment { - variables = { - API_KEYS_S3_BUCKET = var.api_key_s3_bucket_name, - API_KEYS_S3_KEY = var.api_key_s3_object - ASANA_USERS_PROJECT_ID = var.asana_users_project_id - } - } -} - -resource "aws_cloudwatch_event_rule" "execute_sgtm_sync_users_event_rule" { - name = "execute_sgtm_sync_users" - description = "Execute Lambda function sgtm_sync_users on a cron-style schedule" - schedule_expression = "rate(1 hour)" -} - -resource "aws_lambda_permission" "lambda_permission_for_sgtm_sync_users_schedule_event" { - statement_id = "AllowSGTMSyncUsersInvoke" - action = "lambda:InvokeFunction" - function_name = aws_lambda_function.sgtm_sync_users.function_name - principal = "events.amazonaws.com" - source_arn = aws_cloudwatch_event_rule.execute_sgtm_sync_users_event_rule.arn -} - -resource "aws_cloudwatch_event_target" "execute_sgtm_sync_users_event_target" { - target_id = "execute_sgtm_sync_users_event_target" - rule = aws_cloudwatch_event_rule.execute_sgtm_sync_users_event_rule.name - arn = aws_lambda_function.sgtm_sync_users.arn -} ### API diff --git a/terraform/variables.tf b/terraform/variables.tf index 47730697..6c4fe7a2 100644 --- a/terraform/variables.tf +++ b/terraform/variables.tf @@ -63,11 +63,6 @@ variable "terraform_backend_tfc_workspace" { description = "The Terraform Cloud workspace to use as the remote backend. Must be provided if terraform_backend_use_tfc is true." } -variable "asana_users_project_id" { - type = string - description = "Project ID that holds the tasks that map Github handles to Asana user ids" -} - variable "github_usernames_to_asana_gids_s3_path" { description = "The S3 path, in the form bucket/key, to the .json file that maps Github usernames to email addresses associated with Asana users." type = string diff --git a/test/asana/helpers/test_asana_comment_from_github_comment.py b/test/asana/helpers/test_asana_comment_from_github_comment.py index dd3fa5a7..765d05e3 100644 --- a/test/asana/helpers/test_asana_comment_from_github_comment.py +++ b/test/asana/helpers/test_asana_comment_from_github_comment.py @@ -1,18 +1,25 @@ from html import escape +from unittest.mock import MagicMock, patch import src.asana.helpers from test.impl.mock_dynamodb_test_case import MockDynamoDbTestCase from test.impl.builders import builder, build +from test.test_utils import magic_mock_with_return_type_value +@patch( + "src.dynamodb.client.get_asana_domain_user_id_from_github_handle", + magic_mock_with_return_type_value( + {"github_test_user_login": "TEST_USER_ASANA_DOMAIN_USER_ID"} + ), +) class TestAsanaCommentFromGitHubComment(MockDynamoDbTestCase): @classmethod def setUpClass(cls): MockDynamoDbTestCase.setUpClass() - cls.test_data.insert_user_into_user_table( - "github_test_user_login", "TEST_USER_ASANA_DOMAIN_USER_ID" - ) - def test_includes_comment_text(self): + def test_includes_comment_text( + self, + ): github_comment = build( builder.comment() .author(builder.user("github_unknown_user_login")) @@ -23,7 +30,9 @@ def test_includes_comment_text(self): ) self.assertContainsStrings(asana_comment, ["GITHUB_COMMENT_TEXT"]) - def test_transforms_urls_from_comment_tect(self): + def test_transforms_urls_from_comment_tect( + self, + ): url = "https://www.foo.bar/?a=1&b=2" github_comment = build( builder.comment() @@ -37,7 +46,9 @@ def test_transforms_urls_from_comment_tect(self): asana_comment, ['{}'.format(escape(url), url)] ) - def test_includes_asana_comment_author(self): + def test_includes_asana_comment_author( + self, + ): github_comment = build( builder.comment().author(builder.user("github_test_user_login")) ) @@ -46,7 +57,9 @@ def test_includes_asana_comment_author(self): ) self.assertContainsStrings(asana_comment, ["TEST_USER_ASANA_DOMAIN_USER_ID"]) - def test_handles_non_asana_comment_author_gracefully(self): + def test_handles_non_asana_comment_author_gracefully( + self, + ): github_comment = build( builder.comment().author( builder.user("github_unknown_user_login", "GITHUB_UNKNOWN_USER_NAME") @@ -59,7 +72,9 @@ def test_handles_non_asana_comment_author_gracefully(self): asana_comment, ["github_unknown_user_login", "GITHUB_UNKNOWN_USER_NAME"] ) - def test_handles_non_asana_comment_author_that_has_no_name_gracefully(self): + def test_handles_non_asana_comment_author_that_has_no_name_gracefully( + self, + ): github_comment = build( builder.comment().author(builder.user("github_unknown_user_login")) ) @@ -68,7 +83,9 @@ def test_handles_non_asana_comment_author_that_has_no_name_gracefully(self): ) self.assertContainsStrings(asana_comment, ["github_unknown_user_login"]) - def test_does_not_inject_unsafe_html(self): + def test_does_not_inject_unsafe_html( + self, + ): placeholder = "💣" github_placeholder_comment = build( builder.comment() @@ -97,7 +114,9 @@ def test_does_not_inject_unsafe_html(self): f"Expected the {unsafe_character} character to be escaped", ) - def test_considers_double_quotes_safe_in_comment_text(self): + def test_considers_double_quotes_safe_in_comment_text( + self, + ): github_author = builder.user("github_unknown_user_login") placeholder = "💣" github_placeholder_comment = build( @@ -121,7 +140,9 @@ def test_considers_double_quotes_safe_in_comment_text(self): f"Did not expected the {safe_character} character to be escaped", ) - def test_transforms_github_at_mentions_to_asana_at_mentions(self): + def test_transforms_github_at_mentions_to_asana_at_mentions( + self, + ): github_comment = build( builder.comment() .author(builder.user("github_unknown_user_login")) @@ -132,7 +153,9 @@ def test_transforms_github_at_mentions_to_asana_at_mentions(self): ) self.assertContainsStrings(asana_comment, ["TEST_USER_ASANA_DOMAIN_USER_ID"]) - def test_handles_non_asana_comment_at_mention_gracefully(self): + def test_handles_non_asana_comment_at_mention_gracefully( + self, + ): github_comment = build( builder.comment() .author(builder.user("github_unknown_user_login")) @@ -143,7 +166,9 @@ def test_handles_non_asana_comment_at_mention_gracefully(self): ) self.assertContainsStrings(asana_comment, ["@github_unknown_user_login"]) - def test_handles_at_sign_in_comment_gracefully(self): + def test_handles_at_sign_in_comment_gracefully( + self, + ): github_comment = build( builder.comment() .author(builder.user("github_unknown_user_login")) @@ -154,7 +179,9 @@ def test_handles_at_sign_in_comment_gracefully(self): ) self.assertContainsStrings(asana_comment, ["hello@world.asana.com"]) - def test_includes_url_in_comment(self): + def test_includes_url_in_comment( + self, + ): url = "https://github.com/Asana/SGTM/pull/31#issuecomment-626850667" github_comment = build(builder.comment().url(url)) asana_comment = src.asana.helpers.asana_comment_from_github_comment( diff --git a/test/asana/helpers/test_asana_comment_from_github_review.py b/test/asana/helpers/test_asana_comment_from_github_review.py index cdef6ae0..518be869 100644 --- a/test/asana/helpers/test_asana_comment_from_github_review.py +++ b/test/asana/helpers/test_asana_comment_from_github_review.py @@ -1,7 +1,9 @@ +from unittest.mock import patch import src.asana.helpers from src.github.models import ReviewState from test.impl.builders import builder, build from test.impl.mock_dynamodb_test_case import MockDynamoDbTestCase +from test.test_utils import magic_mock_with_return_type_value """ Extracts the GitHub author and comments from a GitHub Review, and transforms them into @@ -11,13 +13,16 @@ """ +@patch( + "src.dynamodb.client.get_asana_domain_user_id_from_github_handle", + magic_mock_with_return_type_value( + {"github_test_user_login": "TEST_USER_ASANA_DOMAIN_USER_ID"} + ), +) class TestAsanaCommentFromGitHubReview(MockDynamoDbTestCase): @classmethod def setUpClass(cls): MockDynamoDbTestCase.setUpClass() - cls.test_data.insert_user_into_user_table( - "github_test_user_login", "TEST_USER_ASANA_DOMAIN_USER_ID" - ) def test_includes_review_text(self): github_review = build( diff --git a/test/asana/helpers/test_extract_task_fields_from_pull_request.py b/test/asana/helpers/test_extract_task_fields_from_pull_request.py index b687edc5..a31a2355 100644 --- a/test/asana/helpers/test_extract_task_fields_from_pull_request.py +++ b/test/asana/helpers/test_extract_task_fields_from_pull_request.py @@ -9,6 +9,7 @@ from src.github.models import Commit from src.github.logic import ApprovedBeforeMergeStatus from src.asana import logic as asana_logic +from test.test_utils import magic_mock_with_return_type_value followup_bot = builder.user("follow_up").build() @@ -82,9 +83,17 @@ class BaseClass(MockDynamoDbTestCase): @classmethod def setUpClass(cls): MockDynamoDbTestCase.setUpClass() - cls.test_data.insert_user_into_user_table( - "github_test_user_login", "TEST_USER_ASANA_DOMAIN_USER_ID" + + def setUp(self) -> None: + super(BaseClass, self).setUp() + patch_get_asana_domain_user_id_from_github_handle = patch( + "src.dynamodb.client.get_asana_domain_user_id_from_github_handle", + magic_mock_with_return_type_value( + {"github_test_user_login": "TEST_USER_ASANA_DOMAIN_USER_ID"} + ), ) + patch_get_asana_domain_user_id_from_github_handle.start() + self.addCleanup(patch_get_asana_domain_user_id_from_github_handle.stop) class TestExtractsMiscellaneousFieldsFromPullRequest(BaseClass): @@ -368,16 +377,20 @@ def test_html_body_status_merged_not_approved( self.assertContainsStrings(actual, expected_strings) +@patch( + "src.dynamodb.client.get_asana_domain_user_id_from_github_handle", + magic_mock_with_return_type_value( + { + "github_assignee_login_annie": "ANNIE_ASANA_DOMAIN_USER_ID", + "github_assignee_login_billy": "BILLY_ASANA_DOMAIN_USER_ID", + "github_test_user_login": "TEST_USER_ASANA_DOMAIN_USER_ID", + } + ), +) class TestExtractsAssigneeFromPullRequest(BaseClass): @classmethod def setUpClass(cls): BaseClass.setUpClass() - cls.test_data.insert_user_into_user_table( - "github_assignee_login_annie", "ANNIE_ASANA_DOMAIN_USER_ID" - ) - cls.test_data.insert_user_into_user_table( - "github_assignee_login_billy", "BILLY_ASANA_DOMAIN_USER_ID" - ) @patch("src.asana.logic.SGTM_FEATURE__ALLOW_PERSISTENT_TASK_ASSIGNEE", False) def test_assignee(self): diff --git a/test/dynamodb/test_client.py b/test/dynamodb/test_client.py index 01cb8ee8..ac499932 100644 --- a/test/dynamodb/test_client.py +++ b/test/dynamodb/test_client.py @@ -1,5 +1,7 @@ +from unittest.mock import patch from test.impl.mock_dynamodb_test_case import MockDynamoDbTestCase import src.dynamodb.client as dynamodb_client +from test.test_utils import magic_mock_with_return_type_value class DynamodbClientTest(MockDynamoDbTestCase): @@ -20,31 +22,6 @@ def test_get_asana_id_from_github_node_id_and_insert_github_node_to_asana_id_map dynamodb_client.get_asana_id_from_github_node_id(gh_node_id), asana_id ) - def test_get_asana_domain_user_id_from_github_handle(self): - gh_handle = "Elaine Benes" - asana_user_id = "12345" - self.test_data.insert_user_into_user_table(gh_handle, asana_user_id) - self.assertEqual( - dynamodb_client.get_asana_domain_user_id_from_github_handle(gh_handle), - asana_user_id, - ) - - def test_bulk_insert_github_handle_to_asana_user_id_mapping(self): - mappings = [("user1", "1"), ("user2", "2")] - dynamodb_client.DEPRECATED_bulk_insert_github_handle_to_asana_user_id_mapping( - mappings - ) - self.assertEqual( - dynamodb_client.get_asana_domain_user_id_from_github_handle("user1"), - "1", - ) - self.assertEqual( - dynamodb_client.get_asana_domain_user_id_from_github_handle("user2"), - "2", - ) - user_items = dynamodb_client.DEPRECATED_get_all_user_items() - self.assertEqual(len(list(user_items)), 2) - if __name__ == "__main__": from unittest import main as run_tests diff --git a/test/github/test_controller.py b/test/github/test_controller.py index 7963cd69..7baefccd 100644 --- a/test/github/test_controller.py +++ b/test/github/test_controller.py @@ -1,4 +1,4 @@ -from unittest.mock import patch +from unittest.mock import patch, ANY from uuid import uuid4 from test.impl.mock_dynamodb_test_case import MockDynamoDbTestCase import src.github.client as github_client @@ -8,11 +8,15 @@ from test.impl.builders import builder +@patch("src.dynamodb.client.get_asana_domain_user_id_from_github_handle") class GithubControllerTest(MockDynamoDbTestCase): @patch.object(asana_controller, "update_task") @patch.object(asana_controller, "create_task") def test_upsert_pull_request_when_task_id_not_found_in_dynamodb( - self, create_task_mock, update_task_mock + self, + create_task_mock, + update_task_mock, + _get_asana_domain_id_mock, ): # If the task id is not found in dynamodb, then we assume the task # doesn't exist and create a new task @@ -27,7 +31,7 @@ def test_upsert_pull_request_when_task_id_not_found_in_dynamodb( add_asana_task_to_pr_mock.assert_called_with(pull_request, new_task_id) create_task_mock.assert_called_with(pull_request.repository_id()) - update_task_mock.assert_called_with(pull_request, new_task_id, []) + update_task_mock.assert_called_with(pull_request, new_task_id, ANY) # Assert that the new task id was inserted into the table task_id = dynamodb_client.get_asana_id_from_github_node_id(pull_request.id()) @@ -36,7 +40,10 @@ def test_upsert_pull_request_when_task_id_not_found_in_dynamodb( @patch.object(asana_controller, "update_task") @patch.object(asana_controller, "create_task") def test_upsert_pull_request_when_task_id_already_found_in_dynamodb( - self, create_task_mock, update_task_mock + self, + create_task_mock, + update_task_mock, + _get_asana_domain_id_mock, ): # If the task id is found in dynamodb, then we just update (don't # attempt to create) @@ -51,10 +58,14 @@ def test_upsert_pull_request_when_task_id_already_found_in_dynamodb( github_controller.upsert_pull_request(pull_request) create_task_mock.assert_not_called() - update_task_mock.assert_called_with(pull_request, existing_task_id, []) + update_task_mock.assert_called_with(pull_request, existing_task_id, ANY) @patch.object(github_client, "edit_pr_description") - def test_add_asana_task_to_pull_request(self, edit_pr_mock): + def test_add_asana_task_to_pull_request( + self, + edit_pr_mock, + _get_asana_domain_id_mock, + ): pull_request = builder.pull_request("original body").build() task_id = uuid4().hex @@ -70,7 +81,10 @@ def test_add_asana_task_to_pull_request(self, edit_pr_mock): @patch.object(asana_controller, "update_task") @patch.object(asana_controller, "upsert_github_comment_to_task") def test_upsert_comment_when_task_id_already_found_in_dynamodb( - self, add_comment_mock, update_task_mock + self, + add_comment_mock, + update_task_mock, + _get_asana_domain_id_mock, ): # If the task id is found in dynamodb, then we just update (don't # attempt to create) @@ -86,12 +100,15 @@ def test_upsert_comment_when_task_id_already_found_in_dynamodb( github_controller.upsert_comment(pull_request, comment) add_comment_mock.assert_called_with(comment, existing_task_id) - update_task_mock.assert_called_with(pull_request, existing_task_id, []) + update_task_mock.assert_called_with(pull_request, existing_task_id, ANY) @patch.object(asana_controller, "update_task") @patch.object(asana_controller, "upsert_github_comment_to_task") def test_upsert_comment_when_task_id_not_found_in_dynamodb( - self, add_comment_mock, update_task_mock + self, + add_comment_mock, + update_task_mock, + _get_asana_domain_id_mock, ): pull_request = builder.pull_request().build() comment = builder.comment().build() @@ -100,7 +117,9 @@ def test_upsert_comment_when_task_id_not_found_in_dynamodb( # TODO: Test that a full sync was performed @patch.object(github_client, "set_pull_request_assignee") - def test_assign_pull_request_to_author(self, set_pr_assignee_mock): + def test_assign_pull_request_to_author( + self, set_pr_assignee_mock, _get_asana_domain_id_mock + ): user = builder.user().login("the_author").name("dont-care") pull_request = builder.pull_request().author(user).build() with patch.object(pull_request, "set_assignees") as set_assignees_mock: diff --git a/test/impl/mock_dynamodb_test_case.py b/test/impl/mock_dynamodb_test_case.py index e9ed462d..6c8624ff 100644 --- a/test/impl/mock_dynamodb_test_case.py +++ b/test/impl/mock_dynamodb_test_case.py @@ -5,9 +5,8 @@ import os import boto3 # type: ignore from moto import mock_dynamodb # type: ignore -from src.config import OBJECTS_TABLE, USERS_TABLE, LOCK_TABLE +from src.config import OBJECTS_TABLE, LOCK_TABLE from .base_test_case_class import BaseClass -from .mock_dynamodb_test_data_helper import MockDynamoDbTestDataHelper # "Mock" the AWS credentials as they can't be mocked in Botocore currently os.environ.setdefault("AWS_ACCESS_KEY_ID", "foobar_key") @@ -27,13 +26,11 @@ class MockDynamoDbTestCase(BaseClass): The test data helper, which knows how to interact with our dynamodb tables for the purpose of test data """ - test_data = None READ_CAPACITY_UNITS = 123 WRITE_CAPACITY_UNITS = 123 @classmethod def tearDownClass(cls): - cls.test_data = None cls.client = None mock_dynamodb().__exit__() @@ -64,26 +61,6 @@ def setUpClass(cls): }, ) - client.create_table( - AttributeDefinitions=[ - { - "AttributeName": "github/handle", - "AttributeType": "S", - }, - ], - TableName=USERS_TABLE, - KeySchema=[ - { - "AttributeName": "github/handle", - "KeyType": "HASH", - } - ], - ProvisionedThroughput={ - "ReadCapacityUnits": cls.READ_CAPACITY_UNITS, - "WriteCapacityUnits": cls.WRITE_CAPACITY_UNITS, - }, - ) - client.create_table( AttributeDefinitions=[ { @@ -112,4 +89,3 @@ def setUpClass(cls): }, ) cls.client = client - cls.test_data = MockDynamoDbTestDataHelper(client) diff --git a/test/impl/mock_dynamodb_test_data_helper.py b/test/impl/mock_dynamodb_test_data_helper.py deleted file mode 100644 index 5a0d5f62..00000000 --- a/test/impl/mock_dynamodb_test_data_helper.py +++ /dev/null @@ -1,20 +0,0 @@ -""" -Test case that should be used for tests that require integration with dynamodb -or other external resources. -""" -from src.config import USERS_TABLE - - -class MockDynamoDbTestDataHelper(object): - def __init__(self, client): - self.client = client - - def insert_user_into_user_table(self, gh_handle: str, asana_domain_user_id: str): - # #DynamoDbSchema - self.client.put_item( - TableName=USERS_TABLE, - Item={ - "github/handle": {"S": gh_handle.lower()}, - "asana/domain-user-id": {"S": asana_domain_user_id}, - }, - ) diff --git a/test/sync_users/__init__.py b/test/sync_users/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/test/sync_users/test_handler.py b/test/sync_users/test_handler.py deleted file mode 100644 index 68a97fde..00000000 --- a/test/sync_users/test_handler.py +++ /dev/null @@ -1,199 +0,0 @@ -import unittest -from unittest.mock import patch - -from src.asana import client as asana_client -from src.dynamodb import client as dynamodb_client -from src.sync_users.handler import handler -from src.sync_users.sgtm_user import SgtmUser - - -GITHUB_HANDLE_KEY = dynamodb_client.DynamoDbClient.GITHUB_HANDLE_KEY -USER_ID_KEY = dynamodb_client.DynamoDbClient.USER_ID_KEY - - -@patch.object(asana_client, "find_all_tasks_for_project") -@patch.object( - dynamodb_client, "DEPRECATED_bulk_insert_github_handle_to_asana_user_id_mapping" -) -@patch.object(dynamodb_client, "DEPRECATED_get_all_user_items") -class TestHandler(unittest.TestCase): - def test_no_users_to_sync__all_already_synced( - self, get_all_user_items_mock, bulk_insert_mock, find_tasks_mock - ): - gh_handle = "torvalds" - asana_user_id = "12345" - get_all_user_items_mock.return_value = [ - { - GITHUB_HANDLE_KEY: {"S": gh_handle}, - USER_ID_KEY: {"S": asana_user_id}, - } - ] - - find_tasks_mock.return_value = [ - { - "gid": "1", - "custom_fields": [ - { - "name": SgtmUser.GITHUB_HANDLE_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": gh_handle, - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": asana_user_id, - }, - ], - } - ] - - handler({}, {}) - - # No writes necessary, because all users are in DynamoDb table already - self.assertEqual(bulk_insert_mock.call_count, 0) - - def test_no_users_to_sync__incomplete_user_info( - self, get_all_user_items_mock, bulk_insert_mock, find_tasks_mock - ): - get_all_user_items_mock.return_value = [] - - find_tasks_mock.return_value = [ - { - "gid": "1", - "custom_fields": [ - { - "name": SgtmUser.GITHUB_HANDLE_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "", # missing Github Handle - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "12345", - }, - ], - }, - { - "gid": "2", - "custom_fields": [ - { - "name": SgtmUser.GITHUB_HANDLE_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "torvalds", - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "", # missing Asana user id - }, - ], - }, - {"gid": "3", "custom_fields": []}, - ] - - handler({}, {}) - - # No writes necessary, because no users have the mapping values that - # are required - self.assertEqual(bulk_insert_mock.call_count, 0) - - def test_sync_users__none_already( - self, get_all_user_items_mock, bulk_insert_mock, find_tasks_mock - ): - # No existing dynamodb user mappings - get_all_user_items_mock.return_value = [] - - find_tasks_mock.return_value = [ - { - "gid": "1", - "custom_fields": [ - { - "name": SgtmUser.GITHUB_HANDLE_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "user1", - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "123", - }, - ], - }, - { - "gid": "2", - "custom_fields": [ - { - "name": SgtmUser.GITHUB_HANDLE_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "user2", - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "456", - }, - ], - }, - ] - - handler({}, {}) - - # All users should be written - self.assertEqual(bulk_insert_mock.call_count, 1) - bulk_insert_mock.assert_called_with([("user1", "123"), ("user2", "456")]) - - def test_sync_users__partial( - self, get_all_user_items_mock, bulk_insert_mock, find_tasks_mock - ): - # user1 already exists in the DynamoDb mapping table - get_all_user_items_mock.return_value = [ - { - GITHUB_HANDLE_KEY: {"S": "user1"}, - USER_ID_KEY: {"S": "123"}, - } - ] - - find_tasks_mock.return_value = [ - { - "gid": "1", - "custom_fields": [ - { - "name": SgtmUser.GITHUB_HANDLE_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "user1", - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "123", - }, - ], - }, - { - "gid": "2", - "custom_fields": [ - { - "name": SgtmUser.GITHUB_HANDLE_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "user2", - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "456", - }, - ], - }, - ] - - handler({}, {}) - - # only user2 should be written - self.assertEqual(bulk_insert_mock.call_count, 1) - bulk_insert_mock.assert_called_with([("user2", "456")]) - - -if __name__ == "__main__": - from unittest import main as run_tests - - run_tests() diff --git a/test/sync_users/test_sgtm_user.py b/test/sync_users/test_sgtm_user.py deleted file mode 100644 index 315d99b3..00000000 --- a/test/sync_users/test_sgtm_user.py +++ /dev/null @@ -1,96 +0,0 @@ -import unittest -from unittest.mock import patch - -from src.sync_users.sgtm_user import SgtmUser -from src.dynamodb.client import DynamoDbClient - - -class TestSgtmUser(unittest.TestCase): - def test_constructor_lower_cases_github_handle(self): - user = SgtmUser("JerrySeinfeld", "123") - self.assertEqual(user.github_handle, "jerryseinfeld") - - def test_constructor_removes_whitespace_from_github_handle(self): - user = SgtmUser(" JerrySeinfeld\n ", "123") - self.assertEqual(user.github_handle, "jerryseinfeld") - - def test_constructor_handles_None(self): - user = SgtmUser(None, None) - self.assertEqual(user.github_handle, None) - self.assertEqual(user.domain_user_id, None) - - def test_from_custom_fields_list__creates_user(self): - custom_fields = [ - { - "name": SgtmUser.GITHUB_HANDLE_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "elainebenes", - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "number", - "number_value": 123, - }, - ] - user = SgtmUser.from_custom_fields_list(custom_fields) - self.assertEqual(user.github_handle, "elainebenes") - self.assertEqual(user.domain_user_id, "123") - - def test_from_custom_fields_list__empty_value_returns_None(self): - custom_fields = [ - { - "name": SgtmUser.GITHUB_HANDLE_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "", # Empty string - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "number", - "number_value": 123, - }, - ] - user = SgtmUser.from_custom_fields_list(custom_fields) - self.assertEqual(user, None) - - def test_from_custom_fields_list__missing_custom_fields_returns_None(self): - custom_fields = [ - { - "name": "some-unknown_custom-field", - "type": "text", - "text_value": "foo", - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "number", - "number_value": 123, - }, - ] - user = SgtmUser.from_custom_fields_list(custom_fields) - self.assertEqual(user, None) - - def test_from_custom_fields_list__None_custom_field_value_returns_None(self): - custom_fields = [ - { - "name": SgtmUser.GITHUB_HANDLE_CUSTOM_FIELD_NAME, - "type": "text", - "text_value": "elainebenes", - }, - { - "name": SgtmUser.USER_ID_CUSTOM_FIELD_NAME, - "type": "number", - "number_value": None, - }, - ] - user = SgtmUser.from_custom_fields_list(custom_fields) - self.assertEqual(user, None) - - def test_equality(self): - user1 = SgtmUser("Foo", "123") - user2 = SgtmUser("fOO", "123") - self.assertEqual(user1, user2) - - -if __name__ == "__main__": - from unittest import main as run_tests - - run_tests() diff --git a/test/test_utils.py b/test/test_utils.py index 4cc31b1c..ceccc327 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -1,4 +1,6 @@ +from typing import Any import unittest +from unittest.mock import MagicMock from src.utils import memoize, parse_date_string @@ -38,6 +40,28 @@ def remember_me(): ) +def magic_mock_with_return_type_value( + arg_to_return_values_dict: dict[Any, Any], return_none_if_not_found: bool = True +): + """ + This is a function that returns a MagicMock object that will set the return value of the function being mocked + based on the argument passed to the function. You should pass a dictionary that maps arguments to return values. + This function will handle the side_effect function for you. + """ + + def _side_effect_function(arg): + try: + return arg_to_return_values_dict[arg] + except KeyError as exc: + if return_none_if_not_found: + return None + raise ValueError( + f"Mock behavior is undefined for arg {arg}. Please provide a return value for this arg." + ) from exc + + return MagicMock(side_effect=_side_effect_function) + + if __name__ == "__main__": from unittest import main as run_tests