Code review fixes for PR #448

[Bala-Sakabattula]

Fixes for comments raised in PR #448.

[qodo-code-review]

Review Summary by Qodo

Refactor JQL query generation and simplify issue lookup logic

🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Refactor _get_existing_jira_issue_query to return only issue keys tuple
• Move JQL query construction into get_existing_jira_issue caller function
• Simplify control flow by replacing conditional with for-else pattern
• Remove unused Tuple import and simplify nested getattr calls
• Update test cases to match new function signature and behavior

Diagram

flowchart LR
  A["_get_existing_jira_issue_query<br/>returns tuple of keys"] -->|"issue_keys"| B["get_existing_jira_issue<br/>constructs JQL"]
  B -->|"JQL query"| C["client.search_issues"]
  C -->|"results"| D["Filter and cache<br/>downstream issue"]

Loading

File Changes

1. sync2jira/downstream_issue.py ✨ Enhancement +11/-15

Refactor query generation and simplify control flow

• Removed Tuple from imports as it's no longer used
• Refactored _get_existing_jira_issue_query to return only tuple[str, ...] instead of tuple with
 optional JQL string
• Moved JQL query construction from helper function to get_existing_jira_issue caller
• Replaced if not results: with else: block using for-else pattern for cleaner control flow
• Simplified nested getattr calls in check_comments_for_duplicate to single chained call
• Removed unnecessary ResultList initialization before loop

sync2jira/downstream_issue.py

---

2. tests/test_downstream_issue.py 🧪 Tests +12/-22

Update tests for simplified query function signature

• Removed _q helper function that was constructing tuples with JQL strings
• Updated all test scenarios to expect only issue key tuples from _get_existing_jira_issue_query
• Simplified mock setup by directly assigning side_effect instead of conditional assignment
• Changed expected return values from (keys, jql_string) to just keys tuple
• Updated test assertions to match new function signature

tests/test_downstream_issue.py

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

1. Query helper returns None ☑ 🐞 ≡

Description

_get_existing_jira_issue_query is annotated as returning tuple[str, ...] but returns None when
Snowflake has no results, and its docstring/:rtype are malformed/outdated. This breaks the helper’s
contract and can cause future callers (or type checking) to treat the result as always-iterable when
it is not.

Code

sync2jira/downstream_issue.py[R373-389]

+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
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
  issue_keys = tuple(row[0] for row in results)

Evidence

The helper is declared to return a tuple but returns None on the no-results path; tests also assert
None, confirming the intended (but undocumented/incorrectly typed) behavior. The sole production
callsite currently guards with if not issue_keys, but the helper’s signature and docstring still
advertise a tuple/JQL-returning contract, making this easy to misuse later.

sync2jira/downstream_issue.py[319-323]
sync2jira/downstream_issue.py[373-391]
tests/test_downstream_issue.py[362-395]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_get_existing_jira_issue_query` is typed as returning `tuple[str, ...]`, but it returns `None` when no keys are found. Its docstring is also inconsistent (mentions empty tuple / JQL string) and the `:rtype:` line is malformed.
### Issue Context
The only current caller (`get_existing_jira_issue`) guards with `if not issue_keys`, so runtime is OK today, but the helper’s contract is incorrect and easy to misuse.
### Fix Focus Areas
- sync2jira/downstream_issue.py[373-391]
### Suggested fix
Choose one consistent contract and update code + tests accordingly:
- Option A (recommended): return an empty tuple `()` instead of `None` and keep return type `tuple[str, ...]`.
- Option B: keep `None`, but change the annotation to `tuple[str, ...] | None` (or `Optional[tuple[str, ...]]`) and update the docstring to state it returns Jira keys (not a JQL string), and fix the malformed `:rtype:` line.
Then update `tests/test_downstream_issue.py` expectations to match.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Author name fallback changed ☑ 🐞 ≡

Description

check_comments_for_duplicate no longer falls back to author.name when author.displayName exists
but is falsy (e.g., None/""). This can prevent detecting duplicate-marking comments for such authors
and change downstream issue filtering behavior.

Code

sync2jira/downstream_issue.py[R458-463]

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 = getattr(author, "displayName", getattr(author, "name", None))
  if search and author_label == username:
      issue_id = search.groups()[0] + "-" + search.groups()[1]

Evidence

The new code uses the getattr(..., default) form, which only applies the default when the
attribute is missing—not when it exists but is None/empty—whereas the previous A or B form would
fall back for falsy values. The unit test only covers the case where displayName is set to a
non-empty string, so the falsy-displayName case is currently untested.

sync2jira/downstream_issue.py[458-465]
tests/test_downstream_issue.py[1708-1729]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`author_label = getattr(author, "displayName", getattr(author, "name", None))` only falls back to `name` if `displayName` is missing; it does not fall back when `displayName` exists but is falsy (e.g., `None` or `""`). This can break duplicate detection.
### Issue Context
`check_comments_for_duplicate` uses `author_label == username` to decide whether to interpret a comment as a duplicate marker.
### Fix Focus Areas
- sync2jira/downstream_issue.py[458-465]
- tests/test_downstream_issue.py[1708-1729]
### Suggested fix
Compute the label with falsy fallback, e.g.:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[webbnh]

/retest

[webbnh]

Looks good, but it needs a little polish.

The docstring for _get_existing_jira_issue_query() needs to be updated, and either it or the actual return need to be brought into alignment. (Qodo commented on this, too.) And, I recommend changing the function's name.

The encapsulation of _jira_user_display_label() test is excellent, but the test for it is missing a case or two. Also, Qodo has a comment about its code that we should consider.

[webbnh]

👍