Code review fixes for PR #448

sync2jira/downstream_issue.py

@@ -23,7 +23,7 @@
 import operator
 import os
 import re
-from typing import Any, Dict, Optional, Tuple, Union
+from typing import Any, Dict, Optional, Union
 import unicodedata
 
 from dotenv import load_dotenv
@@ -316,14 +316,14 @@ def get_existing_jira_issue(client, issue, config):
     :rtype: JIssue or None
     """
 
-    issue_keys, jql = _get_existing_jira_issue_query(issue)
-    if not jql:
+    issue_keys = _get_existing_jira_issue_keys(issue)
+    if not issue_keys:
         return None
+    jql = f"key in ({','.join(issue_keys)})"
     results: ResultList[JIssue] = client.search_issues(jql)
     if not results:
         # JQL/search index can lag right after create, or hide archived issues.
         # Resolve by issue key (direct GET) before giving up and duplicating.
-        results = ResultList[JIssue]()
         for key in issue_keys:
             try:
                 found = client.issue(key)
@@ -336,7 +336,7 @@ def get_existing_jira_issue(client, issue, config):
             )
             results = ResultList[JIssue]((found,))
             break
-        if not results:
+        else:
             log.warning(
                 "Downstream issue not found for upstream %s after JQL %r and direct fetch for keys %s.",
                 issue.url,
@@ -370,27 +370,26 @@ def get_existing_jira_issue(client, issue, config):
     return results[0]
 
 
-def _get_existing_jira_issue_query(
-    issue: Issue,
-) -> Tuple[tuple[str, ...], Optional[str]]:
+def _get_existing_jira_issue_keys(issue: Issue) -> tuple[str, ...]:
     """
-    Generate a JQL query to find downstream issues corresponding to a given
-    upstream issue.  Return empty tuple and None if no matches were found in either our local
-    cache or in the Dataverse.
+    Retrieve downstream Jira issue keys corresponding to a given upstream issue.
+
+    The function first checks the local cache; if no cached result is found,
+    it queries Snowflake. Returns empty tuple if no matches are found.
 
     :param sync2jira.intermediary.Issue issue: Issue object
-    :returns: A string containing the JQL query or None if no matches
-    :rtype: Tuple[tuple[str, ...], Optional[str]]
+    :returns: A tuple of Jira issue keys, empty if no matches are found
+    :rtype: Tuple[str, ...]
     """
     if result := jira_cache.get(issue.url):
         issue_keys = (result,)
     else:
         results = execute_snowflake_query(issue)
         if not results:
-            return (), None
+            return ()
         issue_keys = tuple(row[0] for row in results)
 
-    return issue_keys, f"key in ({','.join(issue_keys)})"
+    return issue_keys
 
 
 def _filter_downstream_issues(
@@ -446,6 +445,13 @@ def find_username(_issue, config):
     return config["sync2jira"]["jira_username"]
 
 
+def _jira_user_display_label(user) -> Optional[str]:
+    """Best-effort display string for a Jira User (Cloud: displayName, else name)."""
+    if not user:
+        return None
+    return getattr(user, "displayName", None) or getattr(user, "name", None)
+
+
 def check_comments_for_duplicate(client, result, username):
     """
     Checks comment of JIRA issue to see if it has been
@@ -459,10 +465,7 @@ def check_comments_for_duplicate(client, result, username):
     """
     for comment in client.comments(result):
         search = re.search(r"Marking as duplicate of (\w*)-(\d*)", comment.body)
-        author = comment.author
-        author_label = getattr(author, "displayName", None) or getattr(
-            author, "name", None
-        )
+        author_label = _jira_user_display_label(comment.author)
         if search and author_label == username:
             issue_id = search.groups()[0] + "-" + search.groups()[1]
             return client.issue(issue_id)
@@ -1141,9 +1144,8 @@ def _update_assignee(client, existing, issue, overwrite):
     us_exists = bool(
         issue.assignee and issue.assignee[0] and issue.assignee[0].get("fullname")
     )
-    ds_exists = bool(existing.fields.assignee) and hasattr(
-        existing.fields.assignee, "displayName"
-    )
+    assignee = existing.fields.assignee
+    ds_exists = bool(assignee) and _jira_user_display_label(assignee) is not None
     if overwrite:
         if not ds_exists:
             # Let assign_user() figure out what to do.
@@ -1152,14 +1154,12 @@ def _update_assignee(client, existing, issue, overwrite):
             # Overwrite the downstream assignment only if it is different from
             # the upstream one.
             un = issue.assignee[0]["fullname"]
-            dn = existing.fields.assignee.displayName
+            dn = _jira_user_display_label(assignee)
             update = un != dn and remove_diacritics(un) != dn
         else:
             # Without an upstream owner, update only if the downstream is not
             # assigned to the project owner.
-            update = (
-                issue.downstream.get("owner") != existing.fields.assignee.displayName
-            )
+            update = issue.downstream.get("owner") != _jira_user_display_label(assignee)
     else:
         # We're not overwriting, so call assign_user() only if the downstream
         # doesn't already have an assignment.

tests/test_downstream_issue.py

@@ -1,5 +1,6 @@
 from datetime import timedelta
 import os
+import types
 from typing import Any, Optional
 import unittest
 import unittest.mock as mock
@@ -268,9 +269,9 @@ class MockIssue(object):
         )
 
     @mock.patch(PATH + "_filter_downstream_issues")
-    @mock.patch(PATH + "_get_existing_jira_issue_query")
+    @mock.patch(PATH + "_get_existing_jira_issue_keys")
     @mock.patch("jira.client.JIRA")
-    def test_get_existing_newstyle(self, mock_client, mock_get_query, mock_filter):
+    def test_get_existing_newstyle(self, mock_client, mock_get_keys, mock_filter):
         """
         This tests 'get_existing_jira_issue' function.
         """
@@ -287,47 +288,42 @@ def test_get_existing_newstyle(self, mock_client, mock_get_query, mock_filter):
         mock_issue_3.fields = MagicMock()
         mock_issue_3.fields.updated = "2025-11-30T00:00:00.0+0000"
 
-        def _q(keys):
-            if not keys:
-                return (), None
-            return keys, f"key in ({','.join(keys)})"
-
         scenarios = (
             {
-                "scenario": "_get_existing_jira_issue_query returns no keys",
-                "query_return": _q(()),
+                "scenario": "_get_existing_jira_issue_keys returns no keys",
+                "query_return": (),
                 "search_issues": None,
                 "filter_results": None,
                 "expected": None,
                 "issue_side_effect": None,
             },
             {
                 "scenario": "Jira search returns no items, direct fetch fails",
-                "query_return": _q(("MOCK-1",)),
+                "query_return": ("MOCK-1",),
                 "search_issues": ResultList[JIssue](()),
                 "filter_results": None,
                 "expected": None,
                 "issue_side_effect": JIRAError(),
             },
             {
                 "scenario": "Jira search returns no items, direct fetch succeeds",
-                "query_return": _q(("MOCK-1",)),
+                "query_return": ("MOCK-1",),
                 "search_issues": ResultList[JIssue](()),
                 "filter_results": None,
                 "expected": mock_issue_1,
                 "issue_side_effect": None,
             },
             {
                 "scenario": "Jira search returns one item",
-                "query_return": _q(("MOCK-1",)),
+                "query_return": ("MOCK-1",),
                 "search_issues": ResultList[JIssue]((mock_issue_1,)),
                 "filter_results": None,
                 "expected": mock_issue_1,
                 "issue_side_effect": None,
             },
             {
                 "scenario": "_filter_downstream_issues returns one item",
-                "query_return": _q(("MOCK-1", "MOCK-2", "MOCK-3")),
+                "query_return": ("MOCK-1", "MOCK-2", "MOCK-3"),
                 "search_issues": ResultList[JIssue](
                     (mock_issue_1, mock_issue_2, mock_issue_3)
                 ),
@@ -337,7 +333,7 @@ def _q(keys):
             },
             {
                 "scenario": "_filter_downstream_issues returns multiple items",
-                "query_return": _q(("MOCK-1", "MOCK-2", "MOCK-3")),
+                "query_return": ("MOCK-1", "MOCK-2", "MOCK-3"),
                 "search_issues": ResultList[JIssue](
                     (mock_issue_1, mock_issue_2, mock_issue_3)
                 ),
@@ -351,13 +347,11 @@ def _q(keys):
 
         for x in scenarios:
             d.jira_cache = d.UrlCache()  # Clear the cache
-            mock_get_query.return_value = x["query_return"]
+            mock_get_keys.return_value = x["query_return"]
             mock_client.search_issues.return_value = x["search_issues"]
             mock_filter.return_value = x["filter_results"]
-            if x["issue_side_effect"] is not None:
-                mock_client.issue.side_effect = x["issue_side_effect"]
-            else:
-                mock_client.issue.side_effect = None
+            mock_client.issue.side_effect = x["issue_side_effect"]
+            if not x["issue_side_effect"]:
                 mock_client.issue.return_value = mock_issue_1
             result = d.get_existing_jira_issue(
                 client=mock_client, issue=self.mock_issue, config=self.mock_config
@@ -367,22 +361,22 @@ def _q(keys):
                 self.assertEqual(d.jira_cache[self.mock_issue.url], x["expected"].key)
 
     @mock.patch(PATH + "execute_snowflake_query")
-    def test_get_existing_jira_issue_query(self, mock_snowflake):
+    def test_get_existing_jira_issue_keys(self, mock_snowflake):
         scenarios = (
             {
                 "jira_cache": {self.mock_issue.url: "issue_key"},
                 "snowflake": (),
-                "expected": (("issue_key",), "key in (issue_key)"),
+                "expected": ("issue_key",),
             },
             {
                 "jira_cache": {},
                 "snowflake": (),
-                "expected": ((), None),
+                "expected": (),
             },
             {
                 "jira_cache": {},
                 "snowflake": (("issue_key",),),
-                "expected": (("issue_key",), "key in (issue_key)"),
+                "expected": ("issue_key",),
             },
             {
                 "jira_cache": {},
@@ -391,17 +385,14 @@ def test_get_existing_jira_issue_query(self, mock_snowflake):
                     ("issue_key_2",),
                     ("issue_key_3",),
                 ),
-                "expected": (
-                    ("issue_key_1", "issue_key_2", "issue_key_3"),
-                    "key in (issue_key_1,issue_key_2,issue_key_3)",
-                ),
+                "expected": ("issue_key_1", "issue_key_2", "issue_key_3"),
             },
         )
 
         for x in scenarios:
             d.jira_cache = x["jira_cache"]
             mock_snowflake.return_value = x["snowflake"]
-            result = d._get_existing_jira_issue_query(self.mock_issue)
+            result = d._get_existing_jira_issue_keys(self.mock_issue)
             self.assertEqual(result, x["expected"])
 
     @mock.patch(PATH + "find_username")
@@ -1715,6 +1706,30 @@ def test_find_username(self):
         # Assert everything was called correctly
         self.assertEqual(response, "mock_user")
 
+    def test_jira_user_display_label(self):
+        """
+        Covers _jira_user_display_label: falsy user guard, displayName, name fallback.
+        """
+        self.assertIsNone(d._jira_user_display_label(None))
+        self.assertIsNone(d._jira_user_display_label(""))
+        self.assertEqual(
+            d._jira_user_display_label(types.SimpleNamespace(displayName="Alice")),
+            "Alice",
+        )
+        self.assertEqual(
+            d._jira_user_display_label(types.SimpleNamespace(name="bob_only")),
+            "bob_only",
+        )
+        self.assertEqual(
+            d._jira_user_display_label(
+                types.SimpleNamespace(displayName="Alice", name="bob")
+            ),
+            "Alice",
+        )
+        self.assertIsNone(
+            d._jira_user_display_label(types.SimpleNamespace()),
+        )
+
     @mock.patch("jira.client.JIRA")
     def test_check_comments_for_duplicates(self, mock_client):
         """