Skip to content
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.

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

fix adding/removing team reviewers with gh pr edit #3701

Merged
merged 2 commits into from
May 27, 2021
Merged

fix adding/removing team reviewers with gh pr edit #3701

merged 2 commits into from
May 27, 2021

Conversation

mercimat
Copy link
Contributor

@mercimat mercimat commented May 24, 2021

Fixes #3705

The issue:

According to the documentation of gh pr edit, the --add-reviewer and --remove-reviewer flags accept team users with the following format: myorg/team-slug.
If the PR doesn't have any team user specified in the "reviewers" section, the existing code works.
But if the PR already has a team user specified in the "reviewers" section, the gh pr edit 1 --add-reviewer myorg/other-team command fails with the error message: '' not found.

Identifying the root cause:

This error is returned by the TeamsToIDs function in api/queries_repo.go, because one element in names is an empty string.
Going further in debugging this, I found out that the empty string was generated by the Logins function in api/queries_pr.go.
The Logins function was using the RequestedReviewer.Login attribute for both normal users and team users. But teams don't have a Login field (see Team graphql reference). So, when parsing an existing PR with a team as reviewer, the Logins function returns a list with an empty string element.

Fix proposal:

This fix updates the graphql query to get the slug and the organization login in case of a team element, and it adds a check on the typename in Logins to build the login for a team concatenating the org login and the team slug. That way the team login can be compared to values in --add-reviewer, --remove-reviewer flags and in the metadata of the repo.

Testing:

I added a simple unit test for the Logins function. I also built the gh command with the changes and manually tried the gh pr edit command with different combinations of the --add-reviewer and --remove-reviewer flags.

Thank you in advance for your inputs on this PR. Please let me know in case changes to the fix would be needed.

@cliAutomation
Copy link
Collaborator

Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message.

@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI May 24, 2021
@mercimat mercimat changed the title fix pr review requests for team users fix adding/removing team reviewers with gh pr edit May 24, 2021
@samcoe samcoe self-requested a review May 27, 2021 03:19
Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for the contribution. The thorough explanation of the details of the bug and the added tests made this very easy to follow 👍

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 May 27, 2021
@samcoe samcoe merged commit 35e5c75 into cli:trunk May 27, 2021
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 May 27, 2021
@mercimat mercimat deleted the fix-pr-team-reviewrequests branch May 27, 2021 16:58
@github-actions github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done 💤
Development

Successfully merging this pull request may close these issues.

'' not found error when adding/removing a team reviewer with gh pr edit
3 participants