Skip to content

Code review fixes for PR #448 #450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Bala-Sakabattula wants to merge 3 commits into
base: main
Choose a base branch
from
+50 −45
Open

Code review fixes for PR #448 #450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ada3382
fixes
Bala-Sakabattula Apr 5, 2026
8fce53b
displayName consistency
Bala-Sakabattula Apr 5, 2026
f207f08
increasing coverage
Bala-Sakabattula Apr 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 22 additions & 23 deletions sync2jira/downstream_issue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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_query(issue)
if not issue_keys:
return None
jql = f"key in ({','.join(issue_keys)})"
Comment on lines -319 to +322
Copy link
Copy Markdown
Collaborator

@webbnh webbnh Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_get_existing_jira_issue_query() no longer returns a JQL query; you don't want to rename the function?

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)
Expand All @@ -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,
Expand Down Expand Up @@ -370,27 +370,25 @@ 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_query(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
upstream issue. Return empty tuple if no matches were found in either our local
Comment on lines 375 to +376
Copy link
Copy Markdown
Collaborator

@webbnh webbnh Apr 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function now returns just the list of keys -- it does not "generate a JQL query" -- so the docstring (new) line 375 should presumably be updated.

cache or in the Dataverse.

: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]]
:rtype: Tuple[tuple[str, ...]
"""
if result := jira_cache.get(issue.url):
issue_keys = (result,)
else:
results = execute_snowflake_query(issue)
if not results:
return (), None
return None
Comment on lines 387 to +388
Copy link
Copy Markdown
Collaborator

@webbnh webbnh Apr 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is contrary to both the function signature type hint and the docstring. Presumably it should be returning an empty tuple instead of None:

Suggested change
if not results:
return (), None
return None
if not results:
return ()

Alternatively, you can presumably just omit the whole if statement: if execute_snowflake_query() returns an empty list, then the construction at (new) line 389 will produce an empty tuple, and we'll return the same result.

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(
Expand Down Expand Up @@ -446,6 +444,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", getattr(user, "name", None))
Comment on lines +447 to +451
Copy link
Copy Markdown
Collaborator

@webbnh webbnh Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bala, this encapsulation is an excellent step!

Nevertheless, what do you think of Qodo's second comment from yesterday? Can the display name be an empty string (or an explicit None), and, if so, do we want to try to fall back to using "name" when it is?

(While I like and prefer the nested getattr() call, if we need/want to cover that case, then your original code was better, modulo what Black did to it....)

Copy link
Copy Markdown
Collaborator Author

@Bala-Sakabattula Bala-Sakabattula Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about it so should we go back to old type ??

getattr(user, "displayName", None) or getattr( user, "name", None)

Copy link
Copy Markdown
Collaborator

@webbnh webbnh Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I suppose so. But, thanks to this encapsulation, you only have to change it in one place! 🙂

The trick is structuring it in a way which doesn't tempt Black to spread it over extra lines making it less readable. 😛



def check_comments_for_duplicate(client, result, username):
"""
Checks comment of JIRA issue to see if it has been
Expand All @@ -459,10 +464,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)
Expand Down Expand Up @@ -1141,9 +1143,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.
Expand All @@ -1152,14 +1153,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.
Expand Down
50 changes: 28 additions & 22 deletions tests/test_downstream_issue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from datetime import timedelta
import os
import types
from typing import Any, Optional
import unittest
import unittest.mock as mock
Expand Down Expand Up @@ -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(()),
"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)
),
Expand All @@ -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)
),
Expand All @@ -354,10 +350,8 @@ def _q(keys):
mock_get_query.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
Expand All @@ -372,17 +366,17 @@ def test_get_existing_jira_issue_query(self, mock_snowflake):
{
"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": None,
},
{
"jira_cache": {},
"snowflake": (("issue_key",),),
"expected": (("issue_key",), "key in (issue_key)"),
"expected": ("issue_key",),
},
{
"jira_cache": {},
Expand All @@ -391,10 +385,7 @@ 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"),
},
)

Expand Down Expand Up @@ -1715,6 +1706,21 @@ 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",
)
Comment on lines +1709 to +1722
Copy link
Copy Markdown
Collaborator

@webbnh webbnh Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing the case where the parameter is not false-y but contains neither displayName nor name. Also, it would be good to have a case where the parameter contains both fields, and we show that it returns the correct one. (That is, since there are two possible field-inputs, there should be four cases: "A", "B", both, and neither. 🙂)


@mock.patch("jira.client.JIRA")
def test_check_comments_for_duplicates(self, mock_client):
"""
Expand Down
Loading