Conversation
Originally implemented by Keerthana in packit#361
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new UAT test script for the Jira Cloud API, which covers various functionalities like issue creation, commenting, labeling, status changes, attachments, and JQL searches, including a new test for Jotnar tags. The jira_utils.py file was updated to better support both Jira Cloud and Server APIs, introducing an is_jira_cloud helper and modifying API calls (jira_api_get, jira_api_post, jira_api_put) to allow specifying the API version (v2 or v3) for better compatibility. However, several critical issues were identified: the URL construction logic in jira_api_get, jira_api_post, and jira_api_put was simplified in a way that breaks existing callers providing full REST paths. The get_current_issues function has a critical bug where the jira_api_post call was removed, leading to an UnboundLocalError, and its pagination logic is broken. The get_user_account_id function, when used with Jira Server, will likely fail because it defaults to API v3, which is only available on Jira Cloud. Additionally, there's a minor inconsistency in the UAT test script's progress messages, showing x/14 instead of x/15 in some places.
| def jira_api_get(path: str, *, params: dict | None = None) -> Any: | ||
| url = f"{jira_url()}/{path}" if path.startswith("rest/") else f"{jira_url()}/rest/api/2/{path}" | ||
| def jira_api_get(path: str, *, params: dict | None = None, api_version: Literal["2", "3"] = "3") -> Any: | ||
| url = f"{jira_url()}/rest/api/{api_version}/{path}" |
There was a problem hiding this comment.
The new URL construction logic breaks callers that provide a full REST path. For example, JIRA_SEARCH_PATH is rest/api/3/search/jql, and calling this function with it will result in a malformed URL like .../rest/api/3/rest/api/3/search/jql. You should retain the logic to handle paths that already start with rest/.
This issue also exists in jira_api_post, jira_api_put, and jira_api_upload.
| url = f"{jira_url()}/rest/api/{api_version}/{path}" | |
| url = f"{jira_url()}/{path}" if path.startswith("rest/") else f"{jira_url()}/rest/api/{api_version}/{path}" |
| body: dict[str, Any] = { | ||
| "jql": jql, | ||
| "maxResults": max_results, | ||
| "fields": _fields(full), | ||
| "fields": [] if full else _fields(False), | ||
| } | ||
|
|
||
| if next_page_token: | ||
| body["nextPageToken"] = next_page_token | ||
|
|
||
| logger.debug("Fetching JIRA issues, max=%d, nextPageToken=%s", max_results, next_page_token) | ||
| response_data = jira_api_post(JIRA_SEARCH_PATH, json=body, decode_response=True) | ||
| logger.debug("Got %d issues", len(response_data["issues"])) | ||
|
|
||
| for issue_data in response_data["issues"]: | ||
| yield decode_issue(issue_data, full) | ||
|
|
||
| next_page_token = response_data.get("nextPageToken") | ||
| if not next_page_token or len(response_data["issues"]) == 0: | ||
| break | ||
| if response_data.get("isLast", True): | ||
| break | ||
| next_page_token = response_data.get("nextPageToken") |
There was a problem hiding this comment.
This loop appears to be broken due to incomplete refactoring:
- The
response_datavariable is used but never assigned because thejira_api_postcall has been removed. This will cause anUnboundLocalError. - The pagination logic is broken. The
next_page_tokenis retrieved but not passed in the body of the subsequent request, which would likely cause an infinite loop if there are multiple pages of results. next_page_tokenis assigned twice (lines 391 and 396), which is redundant.
Please restore the API call and fix the pagination logic.
| if is_jira_cloud(): | ||
| users = jira_api_get("user/search", params={"query": email}, api_version="3") | ||
| else: | ||
| users = jira_api_get("user/search", params={"username": email}) |
There was a problem hiding this comment.
According to the comment at the top of the file, Jira API v3 only exists on Jira Cloud. For Jira Server, you must use v2. This call does not specify the API version, so it will default to '3' and likely fail on Jira Server.
| users = jira_api_get("user/search", params={"username": email}) | |
| users = jira_api_get("user/search", params={"username": email}, api_version="2") |
| print("UAT Test Script") | ||
|
|
||
| # Test 1: Get custom fields | ||
| print("\n[1/14] Get custom fields") |
There was a problem hiding this comment.
The total number of tests in the progress message is inconsistent. This one says 1/14, but later tests say x/15. Please update this and the other occurrences on lines 53 and 64 to use /15 for consistency.
print("\n[1/15] Get custom fields")2 participants
Originally implemented by Keerthana in #361
I am carrying this over so we can get all the benefits from #361.
The rebase is not complete, I'll continue tomorrow and make sure the tests pass.