diff --git a/fedmsg.d/sync2jira.py b/fedmsg.d/sync2jira.py index 359ce2eb..44d12139 100644 --- a/fedmsg.d/sync2jira.py +++ b/fedmsg.d/sync2jira.py @@ -65,24 +65,64 @@ }, 'map': { 'github': { - 'GITHUB_USERNAME/Demo_project': {'project': 'FACTORY', 'component': 'gitbz', - 'issue_updates': [ - 'comments', - 'upstream_id', - 'title', - 'description', - 'github_markdown', - 'upstream_id', - 'url', - {'transition': 'Closed'}, - {'assignee': {'overwrite': False}}, - 'github_project_fields'], - 'github_project_number': '1', - 'github_project_fields': {'storypoints': {'gh_field': 'Estimate'}, - 'priority': {'gh_field': 'Priority', 'options': - {'P0': 'Blocker', 'P1': 'Critical', 'P2': 'Major', - 'P3': 'Minor', 'P4': 'Optional', 'P5': 'Trivial'}}}, - 'sync': ['pullrequest', 'issue']}, + 'GITHUB_USERNAME/Demo_project': { + 'project': 'FACTORY', + 'component': 'gitbz', + + # ----- Issue synchronization ----- + 'issue_updates': [ + 'comments', # Sync GitHub issue comments to Jira + 'title', # Sync issue title + 'description', # Sync issue description/body + 'github_markdown', # Convert GitHub Markdown to Jira wiki format + 'upstream_id', # Add comment with upstream issue link on create + 'url', # Include upstream URL in description + 'github_project_fields', # Sync storypoints & priority from GitHub Projects + {'fixVersion': {'overwrite': False}}, # Sync milestone as fixVersion + {'transition': 'Closed'}, # Transition Jira when upstream issue closes + {'assignee': {'overwrite': False}}, # Sync assignee (don't overwrite existing) + {'on_close': {'apply_labels': ['closed-upstream']}}, # Label on close + ], + + # ----- PR synchronization ----- + # pr_updates supports the same options as issue_updates, + # plus merge_transition and link_transition. + 'pr_updates': [ + 'comments', # Sync GitHub PR comments to Jira + 'title', # Sync PR title + 'description', # Sync PR description/body + 'github_markdown', # Convert GitHub Markdown to Jira wiki format + 'upstream_id', # Add comment with upstream PR link on create + 'url', # Include upstream URL in description + 'github_project_fields', # Sync storypoints & priority from GitHub Projects + {'fixVersion': {'overwrite': False}}, # Sync milestone as fixVersion + {'transition': 'Closed'}, # Transition Jira when upstream issue closes + {'merge_transition': 'Closed'}, # Transition Jira when PR is merged + {'link_transition': 'In Progress'}, # Transition Jira when PR is first linked + {'assignee': {'overwrite': False}}, # Sync assignee (don't overwrite existing) + {'on_close': {'apply_labels': ['closed-upstream']}}, # Label on close + ], + + # ----- GitHub Projects (shared by issue & PR) ----- + 'github_project_number': '1', + 'github_project_fields': { + 'storypoints': {'gh_field': 'Estimate'}, + 'priority': { + 'gh_field': 'Priority', + 'options': { + 'P0': 'Blocker', + 'P1': 'Critical', + 'P2': 'Major', + 'P3': 'Minor', + 'P4': 'Optional', + 'P5': 'Trivial', + }, + }, + }, + + # What to sync: 'issue', 'pullrequest', or both + 'sync': ['issue', 'pullrequest'], + }, }, }, 'filters': { diff --git a/sync2jira/downstream_issue.py b/sync2jira/downstream_issue.py index 63cdadf8..173b1b68 100644 --- a/sync2jira/downstream_issue.py +++ b/sync2jira/downstream_issue.py @@ -963,7 +963,7 @@ def _create_jira_issue(client, issue, config): # Update relevant information (i.e., tags, assignees, etc.) if the User # opted in - _update_jira_issue(downstream, issue, client, config) + update_jira_issue(downstream, issue, client, config) return downstream @@ -985,21 +985,25 @@ def _label_matching(jira_labels, issue_labels): return updated_labels -def _update_jira_issue(existing, issue, client, config): +def update_jira_issue(existing, issue, client, config, updates_key="issue_updates"): """ Updates an existing JIRA issue (i.e., tags, assignee, comments, etc.). + Works for both Issue and PR intermediary objects. The ``updates_key`` + parameter selects which downstream config list to read + ("issue_updates" or "pr_updates"). + :param jira.resources.Issue existing: Existing JIRA issue that was found - :param sync2jira.intermediary.Issue issue: Upstream issue we're pulling data from + :param issue: Upstream Issue or PR we're pulling data from :param jira.client.JIRA client: JIRA Client + :param dict config: Config dict + :param str updates_key: Config key for the updates list :returns: Nothing """ - # Start with comments - # Only synchronize comments for listings that op-in - log.info("Updating information for upstream issue: %s", issue.url) + log.info("Updating information for upstream %s: %s", updates_key, issue.url) # Get a list of what the user wants to update for the upstream issue - updates = issue.downstream.get("issue_updates", []) + updates = issue.downstream.get(updates_key, []) # Update relevant data if needed. # If the user has specified nothing, just return. @@ -1008,7 +1012,7 @@ def _update_jira_issue(existing, issue, client, config): # Get fields representing project item fields in GitHub and Jira github_project_fields = issue.downstream.get("github_project_fields", {}) - # Only synchronize comments for listings that op-in + # Only synchronize github_project_fields for listings that op-in if "github_project_fields" in updates and github_project_fields: log.info("Looking for GitHub project fields") _update_github_project_fields( @@ -1026,7 +1030,8 @@ def _update_jira_issue(existing, issue, client, config): _update_tags(updates, existing, issue) # Only synchronize fixVersion for listings that op-in - if issue.fixVersion and any("fixVersion" in item for item in updates): + fix_version = getattr(issue, "fixVersion", None) + if fix_version and any("fixVersion" in item for item in updates): log.info("Looking for new fixVersions") _update_fixVersion(updates, existing, issue, client) @@ -1043,7 +1048,7 @@ def _update_jira_issue(existing, issue, client, config): # Only synchronize descriptions for listings that op-in if "description" in updates: log.info("Looking for new description") - _update_description(existing, issue) + _update_description(existing, issue, updates_key) # Only synchronize title for listings that op-in if "title" in updates: @@ -1055,10 +1060,10 @@ def _update_jira_issue(existing, issue, client, config): # Only synchronize transition (status) for listings that op-in if any("transition" in item for item in updates): log.info("Looking for new transition(s)") - _update_transition(client, existing, issue) + _update_transition(client, existing, issue, updates_key) - # Only execute 'on_close' events for listings that opt-in - # and when the issue is closed. + # Execute 'on_close' events when the issue is closed + # (opt-in is checked inside _update_on_close). if issue.status == "Closed": log.info("Attempting to update downstream issue on upstream closed event") _update_on_close(existing, updates) @@ -1066,33 +1071,34 @@ def _update_jira_issue(existing, issue, client, config): log.info("Done updating %s!", issue.url) -def _update_transition(client, existing, issue): +def _update_transition(client, existing, issue, updates_key="issue_updates"): """ Helper function to update the transition of a downstream JIRA issue. Supports an optional ``issue_types`` filter on transition entries in - ``issue_updates``. When present, the transition only fires if the + the updates list. When present, the transition only fires if the downstream JIRA issue's type is in the list. :param jira.client.JIRA client: JIRA client :param jira.resource.Issue existing: Existing JIRA issue :param sync2jira.intermediary.Issue issue: Upstream issue + :param str updates_key: Config key for the updates list :returns: Nothing """ - for entry in issue.downstream.get("issue_updates", []): + for entry in issue.downstream.get(updates_key, []): if not isinstance(entry, dict) or "transition" not in entry: continue closed_status = entry["transition"] - # Normalize legacy True value to "Closed" if closed_status is True: closed_status = "Closed" if not isinstance(closed_status, str): log.warning( "Ignoring malformed transition value %r (expected a string) in " - "issue_updates config for %s", + "%s config for %s", closed_status, + updates_key, existing.key, ) continue @@ -1395,19 +1401,18 @@ def _update_tags(updates, existing, issue): _update_jira_labels(existing, updated_labels) -def _build_description(issue): +def _build_description(issue, updates_key="issue_updates"): # Build the description of the JIRA issue. # # Truncate issue.content *before* wrapping it with {quote} and the # metadata prefix so the closing {quote} and decoration are never lost. - issue_updates = issue.downstream.get("issue_updates", []) + issue_updates = issue.downstream.get(updates_key, []) # Build the prefix lines that will appear above the quoted content. prefix_parts = [] - if issue.reporter: - prefix_parts.append( - f"[{issue.id}] Upstream Reporter: {issue.reporter['fullname']}" - ) + reporter = getattr(issue, "reporter", None) + if reporter: + prefix_parts.append(f"[{issue.id}] Upstream Reporter: {reporter}") if issue.status and any("transition" in item for item in issue_updates): prefix_parts.append("Upstream issue status: " + issue.status) @@ -1433,16 +1438,17 @@ def _build_description(issue): return description -def _update_description(existing, issue): +def _update_description(existing, issue, updates_key="issue_updates"): """ Helper function to sync description between upstream issue and downstream JIRA issue. :param jira.resource.Issue existing: Existing JIRA issue - :param sync2jira.intermediary.Issue issue: Upstream issue + :param sync2jira.intermediary.Issue or sync2jira.intermediary.PR issue: Upstream Issue or PR + :param str updates_key: Config key for the updates list :returns: Nothing """ - new_description = _build_description(issue) + new_description = _build_description(issue, updates_key) # Now we can update the JIRA issue if we need to if new_description != existing.fields.description: @@ -1574,19 +1580,24 @@ def convert_content(content: str) -> str: return content +def maybe_convert_markdown(issue, updates_key="issue_updates"): + """Apply GitHub-markdown-to-Jira conversion if the config opts in. + + Works for both Issue and PR objects. + """ + updates = issue.downstream.get(updates_key) + if updates and issue.source == "github" and issue.content: + if "github_markdown" in updates: + issue.content = convert_content(issue.content) + + def update_jira(client, config, issue): # Check the status of the JIRA client if not config["sync2jira"]["develop"] and not check_jira_status(client): log.warning("The JIRA server looks like its down. Shutting down...") raise RuntimeError("Jira server status check failed; aborting...") - if issue.downstream.get("issue_updates"): - if ( - issue.source == "github" - and issue.content - and "github_markdown" in issue.downstream["issue_updates"] - ): - issue.content = convert_content(issue.content) + maybe_convert_markdown(issue) # First, check to see if we have a matching issue using the new method. # If we do, then bail out. No sync needed. @@ -1599,7 +1610,7 @@ def update_jira(client, config, issue): log.info("Testing flag is true. Skipping actual update.") return # Update relevant metadata (i.e. tags, assignee, etc) - _update_jira_issue(existing, issue, client, config) + update_jira_issue(existing, issue, client, config) return # If we're *not* configured to do legacy matching (upgrade mode), then diff --git a/sync2jira/downstream_pr.py b/sync2jira/downstream_pr.py index 2b3e830b..76ff13f5 100644 --- a/sync2jira/downstream_pr.py +++ b/sync2jira/downstream_pr.py @@ -31,6 +31,8 @@ log = logging.getLogger("sync2jira") +UPDATES_KEY = "pr_updates" + def format_comment(pr, pr_suffix, client): """ @@ -94,22 +96,21 @@ def comment_exists(client, existing: JIRAIssue, new_comment): comments = client.comments(existing) for comment in comments: if new_comment == comment.body: - # If the comment was return True return False -def update_jira_issue(existing, pr, client): - """ - Updates an existing JIRA issue (i.e. tags, assignee, comments etc.). +def _update_pr_transitions(client, existing, pr): + """Handle merge_transition and link_transition for a PR. + These are PR-only concepts that have no Issue counterpart. + :param jira.client.JIRA client: JIRA Client :param jira.resources.Issue existing: Existing JIRA issue that was found :param sync2jira.intermediary.PR pr: Upstream issue we're pulling data from - :param jira.client.JIRA client: JIRA Client :returns: Nothing """ # Get our updates array - updates = pr.downstream.get("pr_updates", {}) + updates = pr.downstream.get(UPDATES_KEY, {}) # Format and add comment to indicate PR has been linked new_comment = format_comment(pr, pr.suffix, client) @@ -126,19 +127,19 @@ def update_jira_issue(existing, pr, client): d_issue.attach_link(client, existing, remote_link) # Only synchronize merge_transition for listings that opt-in - if any("merge_transition" in item for item in updates) and "merged" in pr.suffix: + if "merged" in pr.suffix and any("merge_transition" in item for item in updates): log.info("Looking for new merged_transition") - update_transition(client, existing, pr, "merge_transition") + _update_pr_transition(client, existing, pr, "merge_transition") # Only synchronize link_transition for listings that opt-in # and a link comment has been created if ( - any("link_transition" in item for item in updates) - and "mentioned" in new_comment + "mentioned" in new_comment and not exists + and any("link_transition" in item for item in updates) ): log.info("Looking for new link_transition") - update_transition(client, existing, pr, "link_transition") + _update_pr_transition(client, existing, pr, "link_transition") def _matches_transition_filters(transition_config, pr, existing): @@ -183,7 +184,7 @@ def _matches_transition_filters(transition_config, pr, existing): return True -def update_transition(client, existing, pr, transition_type): +def _update_pr_transition(client, existing, pr, transition_type): """ Helper function to update the transition of a downstream JIRA issue. @@ -196,7 +197,7 @@ def update_transition(client, existing, pr, transition_type): :param string transition_type: Transition type (link vs merged) :returns: Nothing """ - pr_updates = pr.downstream.get("pr_updates") + pr_updates = pr.downstream.get(UPDATES_KEY) if not pr_updates: return @@ -217,6 +218,28 @@ def update_transition(client, existing, pr, transition_type): ) +def update_jira_issue(existing, pr, client, config): + """ + Updates an existing JIRA issue for a PR event. + + First applies PR-specific steps (link comment, remote link, + merge/link transitions), then delegates to the shared update + pipeline in downstream_issue for common operations (comments, + description, title, assignee, tags, etc.). + + :param jira.resources.Issue existing: Existing JIRA issue that was found + :param sync2jira.intermediary.PR pr: Upstream PR we're pulling data from + :param jira.client.JIRA client: JIRA Client + :param dict config: Config dict + :returns: Nothing + """ + # PR-specific: link comment, remote link, merge/link transitions + _update_pr_transitions(client, existing, pr) + + # Shared pipeline: comments, description, title, assignee, tags, etc. + d_issue.update_jira_issue(existing, pr, client, config, UPDATES_KEY) + + def sync_with_jira(pr, config): """ Attempts to sync an upstream PR with JIRA (i.e. by finding @@ -249,18 +272,13 @@ def sync_with_jira(pr, config): update_jira(client, config, pr) break except JIRAError: - # We got an error from Jira; if this was a re-try attempt, let the - # exception propagate (and crash the run). if retry: log.info("[PR] Jira retry failed; aborting") raise - # The error may be due to expired/revoked auth. Ask get_jira_client to - # invalidate OAuth2 cache so the next call fetches a new token (no-op for PAT). log.info("[PR] Jira request failed; refreshing the Jira client") client = d_issue.get_jira_client(pr, config, invalidate_oauth2_cache=True) - # Retry the update retry = True @@ -270,6 +288,9 @@ def update_jira(client, config, pr): log.warning("The JIRA server looks like its down. Shutting down...") raise RuntimeError("Jira server status check failed; aborting...") + # Apply GitHub markdown conversion if configured + d_issue.maybe_convert_markdown(pr, UPDATES_KEY) + # Find our JIRA issue if one exists if isinstance(pr, Issue): # Instances of the `PR` type already have an initialized `jira_key` @@ -282,7 +303,7 @@ def update_jira(client, config, pr): if create_pr_issue: if existing := d_issue.get_existing_jira_issue(client, pr, config): log.info(f"Found existing JIRA issue {existing.key} for PR {pr.url}") - update_jira_issue(existing, pr, client) + update_jira_issue(existing, pr, client, config) else: _create_jira_issue_from_pr(client, pr, config) else: @@ -293,7 +314,7 @@ def update_jira(client, config, pr): if len(response) == 1: # Syncing relevant information log.info(f"Syncing PR {pr.url}") - update_jira_issue(response[0], pr, client) + update_jira_issue(response[0], pr, client, config) log.info(f"Done syncing PR {pr.url}") elif len(response) == 0: log.info(f"No issue found for referenced key, {pr.jira_key}") @@ -315,13 +336,9 @@ def _create_jira_issue_from_pr(client, pr, config): """ pr_content = pr.content or f"PR: {pr.url}" - if pr.downstream.get("issue_updates"): - if ( - pr.source == "github" - and pr_content - and "github_markdown" in pr.downstream["issue_updates"] - ): - pr_content = d_issue.convert_content(pr_content) + pr_updates = pr.downstream.get(UPDATES_KEY, []) + if pr.source == "github" and pr_content and "github_markdown" in pr_updates: + pr_content = d_issue.convert_content(pr_content) # Convert PR to Issue-like object for creation # PR and Issue share similar structure, but we need to adapt it @@ -332,15 +349,11 @@ def _create_jira_issue_from_pr(client, pr, config): upstream=pr.upstream, comments=pr.comments, config=config, - tags=[], # PRs don't have tags in the same way - fixVersion=[], + tags=pr.tags, + fixVersion=pr.fixVersion, priority=pr.priority, content=pr_content, - reporter=( - pr.reporter - if isinstance(pr.reporter, dict) - else {"fullname": str(pr.reporter)} - ), + reporter=pr.reporter, assignee=pr.assignee or [], status=pr.status, id_=pr.id, diff --git a/sync2jira/intermediary.py b/sync2jira/intermediary.py index 3a8fdfa5..efd37c70 100644 --- a/sync2jira/intermediary.py +++ b/sync2jira/intermediary.py @@ -122,7 +122,7 @@ def from_github(cls, upstream, issue, config): fixVersion=[issue["milestone"]], priority=issue.get("priority"), content=issue["body"] or "", - reporter=issue["user"], + reporter=issue["user"]["fullname"], assignee=issue["assignees"], status=issue["state"], id_=issue["id"], @@ -147,6 +147,8 @@ def __init__( upstream, config, comments, + tags, + fixVersion, priority, content, reporter, @@ -164,8 +166,8 @@ def __init__( self.url = url self.upstream = upstream self.comments = comments - # self.tags = tags - # self.fixVersion = fixVersion + self.tags = tags + self.fixVersion = fixVersion self.priority = priority self.base_branch = base_branch @@ -241,8 +243,8 @@ def from_github(cls, upstream, pr, suffix, config, action=None): upstream=upstream, config=config, comments=comments, - # tags=issue['labels'], - # fixVersion=[issue['milestone']], + tags=pr.get("labels", []), + fixVersion=[pr.get("milestone")], priority=None, content=pr.get("body"), reporter=pr["user"]["fullname"], diff --git a/sync2jira/main.py b/sync2jira/main.py index 86d7dd77..dd3c5e9a 100644 --- a/sync2jira/main.py +++ b/sync2jira/main.py @@ -321,10 +321,8 @@ def handle_msg(body, suffix, config): if not (pr := handler(body, config, is_pr=True)): log.info("Not handling PR issue update -- not configured") return - # PRs require additional handling (Issues do not have suffix, and - # reporter needs to be reformatted). + # PRs require additional handling (Issues do not have suffix). pr.suffix = suffix - pr.reporter = pr.reporter.get("fullname") setattr(pr, "match", matcher(pr.content, pr.comments)) d_pr.sync_with_jira(pr, config) else: diff --git a/tests/test_downstream_issue.py b/tests/test_downstream_issue.py index c408d483..2b32660f 100644 --- a/tests/test_downstream_issue.py +++ b/tests/test_downstream_issue.py @@ -74,7 +74,7 @@ def setUp(self): "owner": "mock_owner", } self.mock_issue.content = "mock_content" - self.mock_issue.reporter = {"fullname": "mock_user"} + self.mock_issue.reporter = "mock_user" self.mock_issue.url = "mock_url" self.mock_issue.title = "mock_title" self.mock_issue.comments = "mock_comments" @@ -794,7 +794,7 @@ def common_test_create_jira_issue( ) self.assertEqual(response, self.mock_downstream) - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "attach_link") @mock.patch("jira.client.JIRA") def test_create_jira_issue( @@ -809,7 +809,7 @@ def test_create_jira_issue( mock_client.add_comment.assert_not_called() - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "attach_link") @mock.patch("jira.client.JIRA") def test_create_jira_issue_failed_epic_link( @@ -829,7 +829,7 @@ def test_create_jira_issue_failed_epic_link( self.mock_downstream, f"Error adding Epic-Link: DUMMY-1234" ) - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "attach_link") @mock.patch("jira.client.JIRA") def test_create_jira_issue_failed_exd_service( @@ -853,7 +853,7 @@ def test_create_jira_issue_failed_exd_service( f"Value: {self.mock_issue.downstream['EXD-Service']['value']}", ) - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "attach_link") @mock.patch(PATH + "_get_preferred_issue_types") @mock.patch("jira.client.JIRA") @@ -880,7 +880,7 @@ def test_create_jira_issue_multiple_types( f"Some labels look like issue types but were not considered: {issue_types[1:]}", ) - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "attach_link") @mock.patch("jira.client.JIRA") def test_create_jira_issue_no_updates( @@ -1003,7 +1003,7 @@ def update_state( @mock.patch(PATH + "get_jira_client") @mock.patch(PATH + "get_existing_jira_issue") - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "_create_jira_issue") @mock.patch("jira.client.JIRA") @mock.patch(PATH + "_get_existing_jira_issue_legacy") @@ -1040,7 +1040,7 @@ def test_sync_with_jira_matching( @mock.patch(PATH + "get_jira_client") @mock.patch(PATH + "get_existing_jira_issue") - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "_create_jira_issue") @mock.patch("jira.client.JIRA") @mock.patch(PATH + "_get_existing_jira_issue_legacy") @@ -1075,7 +1075,7 @@ def test_sync_with_jira_down( @mock.patch(PATH + "get_jira_client") @mock.patch(PATH + "get_existing_jira_issue") - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "_create_jira_issue") @mock.patch("jira.client.JIRA") @mock.patch(PATH + "_get_existing_jira_issue_legacy") @@ -1110,6 +1110,74 @@ def test_sync_with_jira_no_matching( ) mock_existing_jira_issue_legacy.assert_not_called() + @mock.patch(PATH + "convert_content") + def test_maybe_convert_markdown(self, mock_convert): + """maybe_convert_markdown: converts when opted-in, skips otherwise.""" + + # Converts when source=github, content non-empty, github_markdown in updates + mock_convert.return_value = "h1. Hello" + issue = MagicMock() + issue.downstream = {"issue_updates": ["description", "github_markdown"]} + issue.source = "github" + issue.content = "# Hello" + d.maybe_convert_markdown(issue) + self.assertEqual(issue.content, "h1. Hello") + + # Converts with explicit "issue_updates" parameter + mock_convert.reset_mock() + mock_convert.return_value = "h1. Hello" + issue.downstream = {"issue_updates": ["github_markdown"]} + issue.content = "# Hello" + d.maybe_convert_markdown(issue, "issue_updates") + self.assertEqual(issue.content, "h1. Hello") + + # Works with pr_updates key + mock_convert.reset_mock() + mock_convert.return_value = "*bold*" + issue.downstream = {"pr_updates": ["github_markdown", "description"]} + issue.content = "**bold**" + d.maybe_convert_markdown(issue, "pr_updates") + self.assertEqual(issue.content, "*bold*") + + # Skips when github_markdown not in updates + mock_convert.reset_mock() + issue.downstream = {"issue_updates": ["description"]} + issue.content = "# Hello" + d.maybe_convert_markdown(issue) + self.assertEqual(issue.content, "# Hello") + + # Skips when content is empty + mock_convert.reset_mock() + issue.downstream = {"issue_updates": ["github_markdown"]} + issue.content = "" + d.maybe_convert_markdown(issue) + self.assertEqual(issue.content, "") + + # Skips when source is not github + mock_convert.reset_mock() + issue.source = "pagure" + issue.downstream = {"issue_updates": ["github_markdown"]} + issue.content = "# Hello" + d.maybe_convert_markdown(issue) + self.assertEqual(issue.content, "# Hello") + self.assertEqual(mock_convert.call_count, 0) + + # Skips when updates_key is not present in downstream config + mock_convert.reset_mock() + issue.source = "github" + issue.downstream = {} + issue.content = "# Hello" + d.maybe_convert_markdown(issue) + self.assertEqual(issue.content, "# Hello") + self.assertEqual(mock_convert.call_count, 0) + + # Skips when updates list is empty + issue.downstream = {"issue_updates": []} + issue.content = "# Hello" + d.maybe_convert_markdown(issue) + self.assertEqual(issue.content, "# Hello") + self.assertEqual(mock_convert.call_count, 0) + @mock.patch(PATH + "pypandoc") def test_convert_content(self, mock_pypandoc): """convert_content(): normal success, TeX fallback, and double failure.""" @@ -1175,13 +1243,13 @@ def test_update_jira_issue_closed( mock_update_title, ): """ - This tests '_update_jira_issue' function when the issue is closed + This tests 'update_jira_issue' function when the issue is closed """ self.mock_issue.status = "Closed" # Call the function - d._update_jira_issue( + d.update_jira_issue( existing=self.mock_downstream, issue=self.mock_issue, client=mock_client, @@ -1203,11 +1271,14 @@ def test_update_jira_issue_closed( ) mock_update_assignee.assert_called_once() mock_update_description.assert_called_with( - self.mock_downstream, self.mock_issue + self.mock_downstream, self.mock_issue, "issue_updates" ) mock_update_title.assert_called_with(self.mock_issue, self.mock_downstream) mock_update_transition.assert_called_with( - mock_client, self.mock_downstream, self.mock_issue + mock_client, + self.mock_downstream, + self.mock_issue, + "issue_updates", ) mock_update_on_close.assert_called_once() @@ -1233,10 +1304,10 @@ def test_update_jira_issue_open( mock_update_title, ): """ - This tests '_update_jira_issue' function when the issue is not closed + This tests 'update_jira_issue' function when the issue is not closed """ # Call the function - d._update_jira_issue( + d.update_jira_issue( existing=self.mock_downstream, issue=self.mock_issue, client=mock_client, @@ -1258,11 +1329,14 @@ def test_update_jira_issue_open( ) mock_update_assignee.assert_called_once() mock_update_description.assert_called_with( - self.mock_downstream, self.mock_issue + self.mock_downstream, self.mock_issue, "issue_updates" ) mock_update_title.assert_called_with(self.mock_issue, self.mock_downstream) mock_update_transition.assert_called_with( - mock_client, self.mock_downstream, self.mock_issue + mock_client, + self.mock_downstream, + self.mock_issue, + "issue_updates", ) mock_update_on_close.assert_not_called() @@ -1828,7 +1902,7 @@ def test_update_description_add_reporter(self): self.mock_downstream.fields.description = "[123] Upstream issue status: Open\n" self.mock_issue.status = "Open" self.mock_issue.id = "123" - self.mock_issue.reporter = {"fullname": "mock_user"} + self.mock_issue.reporter = "mock_user" # Call the function d._update_description(existing=self.mock_downstream, issue=self.mock_issue) @@ -1875,7 +1949,7 @@ def test_update_description_add_description(self, mock_datetime): ) self.mock_issue.status = "Open" self.mock_issue.id = "123" - self.mock_issue.reporter = {"fullname": "mock_user"} + self.mock_issue.reporter = "mock_user" mock_datetime.today.return_value = self.mock_today # Call the function @@ -1890,6 +1964,22 @@ def test_update_description_add_description(self, mock_datetime): } ) + def test_build_description_string_reporter(self): + """ + _build_description should include the reporter in the prefix. + """ + issue = MagicMock() + issue.downstream = {"pr_updates": ["description"]} + issue.id = "42" + issue.status = None + issue.reporter = "some_user" + issue.url = "https://github.com/org/repo/pull/42" + issue.content = "PR body text" + + result = d._build_description(issue, "pr_updates") + self.assertIn("[42] Upstream Reporter: some_user", result) + self.assertIn("PR body text", result) + def test_build_description_truncates_long_content(self): """ When the assembled description exceeds the Jira limit, @@ -2474,7 +2564,7 @@ def test_get_field_id_by_name_exception(self): } self.assertDictEqual(d.field_name_cache, expected_cache) - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "attach_link") @mock.patch("jira.client.JIRA") def test_create_jira_issue_epic_link_field_not_found( @@ -2535,7 +2625,7 @@ def test_update_github_project_fields_storypoints_resolution_failure( "storypoints" ] = original_storypoints - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "attach_link") @mock.patch(PATH + "change_status") @mock.patch("jira.client.JIRA") @@ -2570,7 +2660,7 @@ def test_create_jira_issue_with_component_and_labels( self.assertEqual(call_kwargs["labels"], ["label1", "label2"]) self.assertEqual(response, self.mock_downstream) - @mock.patch(PATH + "_update_jira_issue") + @mock.patch(PATH + "update_jira_issue") @mock.patch(PATH + "attach_link") @mock.patch(PATH + "change_status") @mock.patch("jira.client.JIRA") @@ -2783,7 +2873,7 @@ def test_update_jira_issue_github_project_fields_early_exit( mock_update_github_project_fields, ): """ - Test '_update_jira_issue' early exit when github_project_fields is not in updates + Test 'update_jira_issue' early exit when github_project_fields is not in updates or when github_project_fields is empty/None. """ scenarios = ( @@ -2806,7 +2896,7 @@ def test_update_jira_issue_github_project_fields_early_exit( for s in scenarios: self.mock_issue.downstream = s - d._update_jira_issue( + d.update_jira_issue( existing=self.mock_downstream, issue=self.mock_issue, client=mock_client, diff --git a/tests/test_downstream_pr.py b/tests/test_downstream_pr.py index 6c1d6daa..1a2dc795 100644 --- a/tests/test_downstream_pr.py +++ b/tests/test_downstream_pr.py @@ -2,6 +2,7 @@ import unittest.mock as mock from unittest.mock import MagicMock +import sync2jira.downstream_issue as d_issue import sync2jira.downstream_pr as d PATH = "sync2jira.downstream_pr." @@ -70,7 +71,7 @@ def test_sync_with_jira_link(self, mock_d_issue, mock_update_jira_issue): # Assert everything was called correctly mock_update_jira_issue.assert_called_with( - "mock_existing", self.mock_pr, self.mock_client + "mock_existing", self.mock_pr, self.mock_client, self.mock_config ) self.mock_client.search_issues.assert_called_with("Key = JIRA-1234") mock_d_issue.get_jira_client.assert_called_with(self.mock_pr, self.mock_config) @@ -92,7 +93,7 @@ def test_sync_with_jira_merged(self, mock_d_issue, mock_update_jira_issue): # Assert everything was called correctly mock_update_jira_issue.assert_called_with( - "mock_existing", self.mock_pr, mock_client + "mock_existing", self.mock_pr, mock_client, self.mock_config ) mock_client.search_issues.assert_called_with("Key = JIRA-1234") mock_d_issue.get_jira_client.assert_called_with(self.mock_pr, self.mock_config) @@ -153,6 +154,7 @@ def test_sync_with_jira_testing(self, mock_d_issue, mock_update_jira_issue): mock_client.search_issues.assert_not_called() mock_d_issue.get_jira_client.assert_not_called() + @mock.patch(PATH + "d_issue.update_jira_issue") @mock.patch(PATH + "comment_exists") @mock.patch(PATH + "format_comment") @mock.patch(PATH + "d_issue.attach_link") @@ -163,6 +165,7 @@ def test_update_jira_issue_link( mock_attach_link, mock_format_comment, mock_comment_exists, + mock_shared_update, ): """ This function tests 'update_jira_issue' @@ -173,9 +176,11 @@ def test_update_jira_issue_link( mock_issue_link_exists.return_value = False # Call the function - d.update_jira_issue("mock_existing", self.mock_pr, self.mock_client) + d.update_jira_issue( + "mock_existing", self.mock_pr, self.mock_client, self.mock_config + ) - # Assert everything was called correctly + # Assert PR-specific steps were called self.mock_client.add_comment.assert_called_with( "mock_existing", "mock_formatted_comment" ) @@ -190,6 +195,14 @@ def test_update_jira_issue_link( "mock_existing", {"url": "mock_url", "title": "[PR] mock_title"}, ) + # Assert shared update pipeline was called + mock_shared_update.assert_called_with( + "mock_existing", + self.mock_pr, + self.mock_client, + self.mock_config, + "pr_updates", + ) def test_issue_link_exists_false(self): """ @@ -223,6 +236,7 @@ def test_issue_link_exists_true(self): self.mock_client.remote_links.assert_called_with(self.mock_existing) self.assertEqual(ret, True) + @mock.patch(PATH + "d_issue.update_jira_issue") @mock.patch(PATH + "format_comment") @mock.patch(PATH + "comment_exists") @mock.patch(PATH + "d_issue.attach_link") @@ -233,6 +247,7 @@ def test_update_jira_issue_exists( mock_attach_link, mock_comment_exists, mock_format_comment, + mock_shared_update, ): """ This function tests 'update_jira_issue' where the comment already exists @@ -243,9 +258,11 @@ def test_update_jira_issue_exists( mock_issue_link_exists.return_value = True # Call the function - d.update_jira_issue("mock_existing", self.mock_pr, self.mock_client) + d.update_jira_issue( + "mock_existing", self.mock_pr, self.mock_client, self.mock_config + ) - # Assert everything was called correctly + # Assert PR-specific steps were called self.mock_client.add_comment.assert_not_called() mock_format_comment.assert_called_with( self.mock_pr, self.mock_pr.suffix, self.mock_client @@ -257,6 +274,14 @@ def test_update_jira_issue_exists( mock_issue_link_exists.assert_called_with( self.mock_client, "mock_existing", self.mock_pr ) + # Assert shared update pipeline was still called + mock_shared_update.assert_called_with( + "mock_existing", + self.mock_pr, + self.mock_client, + self.mock_config, + "pr_updates", + ) def test_comment_exists_false(self): """ @@ -356,16 +381,16 @@ def test_format_comment_open_no_user_found(self): ) @mock.patch(PATH + "d_issue") - def test_update_transition(self, mock_d_issue): + def test_update_pr_transition(self, mock_d_issue): """ - This function tests 'update_transition' + This function tests '_update_pr_transition' """ # Set up return values mock_client = MagicMock() self.mock_existing.fields.issuetype.name = "Bug" # Call the function - d.update_transition( + d._update_pr_transition( mock_client, self.mock_existing, self.mock_pr, "merge_transition" ) @@ -466,7 +491,7 @@ def test_matches_transition_filters(self): @mock.patch(PATH + "d_issue") def test_update_transition_selects_first_matching_entry(self, mock_d_issue): - """Test that update_transition iterates entries and selects the first match.""" + """Test that _update_pr_transition iterates entries and selects the first match.""" mock_client = MagicMock() self.mock_existing.fields.issuetype.name = "Story" self.mock_pr.base_branch = "main" @@ -484,7 +509,7 @@ def test_update_transition_selects_first_matching_entry(self, mock_d_issue): ] } - d.update_transition( + d._update_pr_transition( mock_client, self.mock_existing, self.mock_pr, "merge_transition" ) @@ -507,7 +532,7 @@ def test_update_transition_no_matching_entry(self, mock_d_issue): ] } - d.update_transition( + d._update_pr_transition( mock_client, self.mock_existing, self.mock_pr, "merge_transition" ) @@ -525,7 +550,7 @@ def test_update_transition_skips_entry_without_transition_type(self, mock_d_issu ] } - d.update_transition( + d._update_pr_transition( mock_client, self.mock_existing, self.mock_pr, "merge_transition" ) @@ -535,11 +560,11 @@ def test_update_transition_skips_entry_without_transition_type(self, mock_d_issu @mock.patch(PATH + "d_issue") def test_update_transition_no_pr_updates(self, mock_d_issue): - """Test that update_transition returns silently when pr_updates is absent.""" + """Test that _update_pr_transition returns silently when pr_updates is absent.""" mock_client = MagicMock() self.mock_pr.downstream = {} - d.update_transition( + d._update_pr_transition( mock_client, self.mock_existing, self.mock_pr, "merge_transition" ) @@ -547,16 +572,120 @@ def test_update_transition_no_pr_updates(self, mock_d_issue): @mock.patch(PATH + "d_issue") def test_update_transition_empty_pr_updates(self, mock_d_issue): - """Test that update_transition returns silently when pr_updates is empty.""" + """Test that _update_pr_transition returns silently when pr_updates is empty.""" mock_client = MagicMock() self.mock_pr.downstream = {"pr_updates": []} - d.update_transition( + d._update_pr_transition( mock_client, self.mock_existing, self.mock_pr, "merge_transition" ) mock_d_issue.change_status.assert_not_called() + @mock.patch(PATH + "d_issue.change_status") + @mock.patch(PATH + "d_issue.update_jira_issue", wraps=d_issue.update_jira_issue) + @mock.patch(PATH + "comment_exists", return_value=True) + @mock.patch(PATH + "format_comment", return_value="mock_comment") + @mock.patch(PATH + "d_issue.attach_link") + @mock.patch(PATH + "issue_link_exists", return_value=True) + def test_update_jira_issue_transition_noop_when_status_none( + self, + mock_issue_link_exists, + mock_attach_link, + mock_format_comment, + mock_comment_exists, + mock_shared_update, + mock_change_status, + ): + """github.pull_request path: PR has status=None, so _update_transition + is a no-op even when {'transition': 'Closed'} is in pr_updates. + """ + self.mock_pr.status = None + self.mock_pr.downstream = { + "pr_updates": [{"transition": "Closed"}], + } + + d.update_jira_issue( + self.mock_existing, self.mock_pr, self.mock_client, self.mock_config + ) + + mock_change_status.assert_not_called() + + @mock.patch(PATH + "d_issue.change_status") + @mock.patch(PATH + "comment_exists", return_value=True) + @mock.patch(PATH + "format_comment", return_value="mock_comment") + @mock.patch(PATH + "d_issue.attach_link") + @mock.patch(PATH + "issue_link_exists", return_value=True) + def test_update_jira_issue_merged_pr_no_duplicate_transition( + self, + mock_issue_link_exists, + mock_attach_link, + mock_format_comment, + mock_comment_exists, + mock_change_status, + ): + """github.issues path: merged PR with status='Closed'. + + merge_transition fires first (via _update_pr_transitions), then + _update_transition finds Jira already in target state and skips. + change_status is called once (for merge_transition) not twice. + """ + self.mock_pr.suffix = "merged" + self.mock_pr.status = "Closed" + self.mock_pr.downstream = { + "pr_updates": [ + {"merge_transition": "Closed"}, + {"transition": "Closed"}, + ], + } + self.mock_existing.fields.status.name = "Closed" + + d.update_jira_issue( + self.mock_existing, self.mock_pr, self.mock_client, self.mock_config + ) + + mock_change_status.assert_called_once_with( + self.mock_client, self.mock_existing, "Closed", self.mock_pr + ) + + @mock.patch(PATH + "d_issue.change_status") + @mock.patch(PATH + "comment_exists", return_value=True) + @mock.patch(PATH + "format_comment", return_value="mock_comment") + @mock.patch(PATH + "d_issue.attach_link") + @mock.patch(PATH + "issue_link_exists", return_value=True) + def test_update_jira_issue_closed_without_merge_transitions( + self, + mock_issue_link_exists, + mock_attach_link, + mock_format_comment, + mock_comment_exists, + mock_change_status, + ): + """github.issues path: PR closed without merge, status='Closed'. + + merge_transition skips (suffix is 'closed', not 'merged'). + _update_transition sees status=='Closed' and fires, transitioning + the Jira issue — covering a case merge_transition cannot handle. + """ + self.mock_pr.suffix = "closed" + self.mock_pr.status = "Closed" + self.mock_pr.url = "mock_url" + self.mock_pr.downstream = { + "pr_updates": [ + {"merge_transition": "Closed"}, + {"transition": "Closed"}, + ], + } + self.mock_existing.fields.status.name = "Open" + + d.update_jira_issue( + self.mock_existing, self.mock_pr, self.mock_client, self.mock_config + ) + + mock_change_status.assert_called_once_with( + self.mock_client, self.mock_existing, "Closed", self.mock_pr + ) + @mock.patch(PATH + "update_jira") @mock.patch(PATH + "d_issue") def test_sync_with_jira_create_pr_issue_enabled( @@ -792,6 +921,8 @@ def _setup_pr_for_issue_creation(self, **overrides): self.mock_pr.url = "https://github.com/test/repo/pull/1" self.mock_pr.upstream = "test/repo" self.mock_pr.comments = [] + self.mock_pr.tags = ["bug", "enhancement"] + self.mock_pr.fixVersion = ["v1.0"] self.mock_pr.priority = None self.mock_pr.content = "PR description" self.mock_pr.reporter = "testuser" @@ -817,16 +948,12 @@ def _assert_issue_created_with_pr_fields(self, mock_issue_class, **field_overrid self.assertEqual(kwargs["title"], self.mock_pr._title) self.assertEqual(kwargs["url"], self.mock_pr.url) self.assertEqual(kwargs["upstream"], self.mock_pr.upstream) + self.assertEqual(kwargs["tags"], self.mock_pr.tags) + self.assertEqual(kwargs["fixVersion"], self.mock_pr.fixVersion) self.assertEqual(kwargs["status"], self.mock_pr.status) self.assertEqual(kwargs["id_"], self.mock_pr.id) - # Handle reporter conversion - if isinstance(self.mock_pr.reporter, dict): - self.assertEqual(kwargs["reporter"], self.mock_pr.reporter) - else: - self.assertEqual( - kwargs["reporter"], {"fullname": str(self.mock_pr.reporter)} - ) + self.assertEqual(kwargs["reporter"], self.mock_pr.reporter) # Apply field-specific overrides for key, expected_value in field_overrides.items(): @@ -878,27 +1005,3 @@ def test_create_jira_issue_from_pr_no_content( self._assert_issue_created_with_pr_fields( mock_issue_class, content=f"PR: {self.mock_pr.url}" ) - - @mock.patch(PATH + "d_issue._create_jira_issue") - @mock.patch(PATH + "Issue") - def test_create_jira_issue_from_pr_reporter_dict( - self, mock_issue_class, mock_create_jira_issue - ): - """ - Test '_create_jira_issue_from_pr' when reporter is already a dict (not a string). - """ - # Set up return values - mock_client = MagicMock() - mock_created_issue = MagicMock() - mock_create_jira_issue.return_value = mock_created_issue - - # Set up PR object with dict reporter - self._setup_pr_for_issue_creation( - reporter={"fullname": "testuser", "email": "test@example.com"} - ) - - # Call the function - d._create_jira_issue_from_pr(mock_client, self.mock_pr, self.mock_config) - - # Assert Issue was created with dict reporter (not wrapped) - self._assert_issue_created_with_pr_fields(mock_issue_class) diff --git a/tests/test_intermediary.py b/tests/test_intermediary.py index dd5dbbe3..eb0835e7 100644 --- a/tests/test_intermediary.py +++ b/tests/test_intermediary.py @@ -35,7 +35,7 @@ def setUp(self): "milestone": "mock_milestone", "priority": "mock_priority", "body": "mock_content", - "user": "mock_reporter", + "user": {"fullname": "mock_reporter"}, "assignees": "mock_assignee", "state": "open", "date_created": "mock_date", @@ -131,7 +131,7 @@ def test_from_github_open_without_priority(self): "labels": "mock_tags", "milestone": "mock_milestone", "body": "mock_content", - "user": "mock_reporter", + "user": {"fullname": "mock_reporter"}, "assignees": "mock_assignee", "state": "open", "date_created": "mock_date", @@ -243,6 +243,8 @@ def test_from_github_pr_reopen(self, mock_matcher): self.assertEqual(response.suffix, "reopened") self.assertEqual(response.status, None) + self.assertEqual(response.tags, "mock_tags") + self.assertEqual(response.fixVersion, ["mock_milestone"]) self.assertEqual(response.downstream, {"mock_downstream": "mock_key"}) self.assertEqual(response.jira_key, "JIRA-1234") self.mock_github_pr["comments"][0]["changed"] = None @@ -278,6 +280,7 @@ def test_from_github_pr_flat_topic_normalizes_suffix(self, mock_matcher): base_kw["action"] = action response = i.PR.from_github(**base_kw) self.assertEqual(response.suffix, expected) + self.assertEqual(response.status, None) def test_matcher(self): """This tests the matcher function"""