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

Tweak to InitialIssueTriage and PullRequestTriage rules #5830

Merged
merged 3 commits into from
Mar 28, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,18 @@ public class IssueProcessingTests : ProcessingTestBase
/// <param name="ruleState">Whether or not the rule is on/off</param>
/// <param name="AIServiceReturnsLabels">Whether or not the AI Service should return labels</param>
[Category("static")]
[TestCase(RulesConstants.InitialIssueTriage, "Tests.JsonEventPayloads/InitialIssueTriage_issue_opened_no_labels_no_assignee.json", RuleState.On, true)]
[TestCase(RulesConstants.InitialIssueTriage, "Tests.JsonEventPayloads/InitialIssueTriage_issue_opened_no_labels_no_assignee.json", RuleState.On, false)]
[TestCase(RulesConstants.InitialIssueTriage, "Tests.JsonEventPayloads/InitialIssueTriage_issue_opened_no_labels_no_assignee.json", RuleState.Off, false)]
public async Task TestInitialIssueTriage(string rule, string payloadFile, RuleState ruleState, bool AIServiceReturnsLabels)
[TestCase(RulesConstants.InitialIssueTriage, "Tests.JsonEventPayloads/InitialIssueTriage_issue_opened_no_labels_no_assignee.json", RuleState.On, true, true, true)]
[TestCase(RulesConstants.InitialIssueTriage, "Tests.JsonEventPayloads/InitialIssueTriage_issue_opened_no_labels_no_assignee.json", RuleState.On, false, true, true)]
[TestCase(RulesConstants.InitialIssueTriage, "Tests.JsonEventPayloads/InitialIssueTriage_issue_opened_no_labels_no_assignee.json", RuleState.Off, false, false, false)]
[TestCase(RulesConstants.InitialIssueTriage, "Tests.JsonEventPayloads/InitialIssueTriage_issue_opened_no_labels_no_assignee.json", RuleState.On, false, false, true)]
[TestCase(RulesConstants.InitialIssueTriage, "Tests.JsonEventPayloads/InitialIssueTriage_issue_opened_no_labels_no_assignee.json", RuleState.On, false, true, false)]
[TestCase(RulesConstants.InitialIssueTriage, "Tests.JsonEventPayloads/InitialIssueTriage_issue_opened_no_labels_no_assignee.json", RuleState.On, false, false, false)]
public async Task TestInitialIssueTriage(string rule, string payloadFile, RuleState ruleState, bool AIServiceReturnsLabels, bool isMemberOfOrg, bool hasWriteOrAdmin)
{
var mockGitHubEventClient = new MockGitHubEventClient(OrgConstants.ProductHeaderName);
mockGitHubEventClient.RulesConfiguration.Rules[rule] = ruleState;
mockGitHubEventClient.UserHasPermissionsReturn = hasWriteOrAdmin;
mockGitHubEventClient.IsUserMemberOfOrgReturn = isMemberOfOrg;
var rawJson = TestHelpers.GetTestEventPayload(payloadFile);
var issueEventPayload = SimpleJsonSerializer.Deserialize<IssueEventGitHubPayload>(rawJson);
List<string> expectedLabels = new List<string>
Expand Down Expand Up @@ -75,6 +80,19 @@ public async Task TestInitialIssueTriage(string rule, string payloadFile, RuleSt
// If the AI Label service doesn't predict labels, the label added is NeedsTriage
Assert.True(issueUpdate.Labels.Contains(LabelConstants.NeedsTriage), $"IssueUpdate does not contain {LabelConstants.NeedsTriage} which should have been added when no labels were predicted.");
}

// If the user is not part of the Azure org AND they don't have write or admin collaborator permissions
// then customer-reported and question labels should be added to the issue
if (!isMemberOfOrg && !hasWriteOrAdmin)
{
Assert.True(issueUpdate.Labels.Contains(LabelConstants.CustomerReported), $"IssueUpdate does not contain {LabelConstants.CustomerReported} which it should when the user is not part of the org and doesn't have write/admin collaborator permissions.");
Assert.True(issueUpdate.Labels.Contains(LabelConstants.Question), $"IssueUpdate does not contain {LabelConstants.Question} which it should when the user is not part of the org and doesn't have write/admin collaborator permissions.");
}
else
{
Assert.False(issueUpdate.Labels.Contains(LabelConstants.CustomerReported), $"IssueUpdate contains {LabelConstants.CustomerReported} and shouldn't when the user is part of the org or has write/admin collaborator permissions.");
Assert.False(issueUpdate.Labels.Contains(LabelConstants.Question), $"IssueUpdate contains {LabelConstants.Question} and shouldn't when the user is part of the org or has write/admin collaborator permissions.");
}
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,17 @@ public class PullRequestProcessingTests : ProcessingTestBase
/// <param name="hasWriteOrAdmin">Whether or not the PR creator has write or admin permissions</param>
[Category("static")]
[NonParallelizable]
[TestCase(RulesConstants.PullRequestTriage, "Tests.JsonEventPayloads/PullRequestTriage_pr_opened_no_labels.json", RuleState.On, true)]
[TestCase(RulesConstants.PullRequestTriage, "Tests.JsonEventPayloads/PullRequestTriage_pr_opened_no_labels.json", RuleState.On, false)]
[TestCase(RulesConstants.PullRequestTriage, "Tests.JsonEventPayloads/PullRequestTriage_pr_opened_no_labels.json", RuleState.Off, false)]
public async Task TestPullRequestTriage(string rule, string payloadFile, RuleState ruleState, bool hasWriteOrAdmin)
[TestCase(RulesConstants.PullRequestTriage, "Tests.JsonEventPayloads/PullRequestTriage_pr_opened_no_labels.json", RuleState.On, true, true)]
[TestCase(RulesConstants.PullRequestTriage, "Tests.JsonEventPayloads/PullRequestTriage_pr_opened_no_labels.json", RuleState.Off, false, false)]
[TestCase(RulesConstants.PullRequestTriage, "Tests.JsonEventPayloads/PullRequestTriage_pr_opened_no_labels.json", RuleState.On, true, false)]
[TestCase(RulesConstants.PullRequestTriage, "Tests.JsonEventPayloads/PullRequestTriage_pr_opened_no_labels.json", RuleState.On, false, true)]
[TestCase(RulesConstants.PullRequestTriage, "Tests.JsonEventPayloads/PullRequestTriage_pr_opened_no_labels.json", RuleState.On, false, false)]
public async Task TestPullRequestTriage(string rule, string payloadFile, RuleState ruleState, bool isMemberOfOrg, bool hasWriteOrAdmin)
{
var mockGitHubEventClient = new MockGitHubEventClient(OrgConstants.ProductHeaderName);
mockGitHubEventClient.RulesConfiguration.Rules[rule] = ruleState;
mockGitHubEventClient.UserHasPermissionsReturn = hasWriteOrAdmin;
mockGitHubEventClient.IsUserMemberOfOrgReturn = isMemberOfOrg;
var rawJson = TestHelpers.GetTestEventPayload(payloadFile);
var prEventPayload = PullRequestProcessing.DeserializePullRequest(rawJson, SimpleJsonSerializer);

Expand All @@ -69,18 +72,18 @@ public async Task TestPullRequestTriage(string rule, string payloadFile, RuleSta
// which means an issueUpdate will be created
int expectedUpdates = 1;

if (hasWriteOrAdmin)
if (hasWriteOrAdmin || isMemberOfOrg)
{
// There should be one update, an IssueUpdate with the NoRecentActivity label removed
Assert.AreEqual(expectedUpdates, totalUpdates, $"The number of updates for a user having Write or Admin permission should have been {expectedUpdates} but was instead, {totalUpdates}");
Assert.AreEqual(expectedUpdates, totalUpdates, $"The number of updates for a user having Write or Admin permission or being a member of Azure org should have been {expectedUpdates} but was instead, {totalUpdates}");
}
// If the user doesn't have Write or Admin permissions then "customer-reported" and "Community Contribution" labels
// will be added and a single comment will be created
else
{
expectedUpdates++;
// Along with the label updates, there should also be a comment added.
Assert.AreEqual(expectedUpdates, totalUpdates, $"The number of updates for a user without Write or Admin permission should have been {expectedUpdates} but was instead, {totalUpdates}");
Assert.AreEqual(expectedUpdates, totalUpdates, $"The number of updates for a user without Write or Admin permission or being a member of Azure org should have been {expectedUpdates} but was instead, {totalUpdates}");
}
// Retrieve the IssueUpdate and verify the expected changes
var issueUpdate = mockGitHubEventClient.GetIssueUpdate();
Expand All @@ -92,19 +95,20 @@ public async Task TestPullRequestTriage(string rule, string payloadFile, RuleSta
Assert.True(issueUpdate.Labels.Contains(label), $"label {label} should have been added because of the file paths in the PR but was not.");
}

if (hasWriteOrAdmin)
// If the user is not part of the Azure org AND they don't have write or admin collaborator permissions
// then customer-reported and community-contribution labels should have been added along with a comment
if (!isMemberOfOrg && !hasWriteOrAdmin)
{
Assert.False(issueUpdate.Labels.Contains(LabelConstants.CustomerReported), $"User has write or admin permission, IssueUpdate should not contain {LabelConstants.CustomerReported}.");
Assert.False(issueUpdate.Labels.Contains(LabelConstants.CommunityContribution), $"User has write or admin permission, IssueUpdate should not contain {LabelConstants.CommunityContribution}.");
}
else
{
// Without Admin or Write permissions there should be two additional labels and a comment added
Assert.True(issueUpdate.Labels.Contains(LabelConstants.CustomerReported), $"User does not have write or admin permission, IssueUpdate should contain {LabelConstants.CustomerReported}.");
Assert.True(issueUpdate.Labels.Contains(LabelConstants.CommunityContribution), $"User does not have write or admin permission, IssueUpdate should contain {LabelConstants.CommunityContribution}.");
Assert.AreEqual(1, mockGitHubEventClient.GetComments().Count, "Without admin or write permission there should have been a comment added.");
}

else
{
Assert.False(issueUpdate.Labels.Contains(LabelConstants.CustomerReported), $"User has write or admin permission or is a member of Azure, IssueUpdate should not contain {LabelConstants.CustomerReported}.");
Assert.False(issueUpdate.Labels.Contains(LabelConstants.CommunityContribution), $"User has write or admin permission or is a member of Azure, IssueUpdate should not contain {LabelConstants.CommunityContribution}.");
Assert.AreEqual(0, mockGitHubEventClient.GetComments().Count, "User has write or admin permission or is a member of Azure, there should not have been a comment added.");
}
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,10 @@ public static async Task ProcessIssueEvent(GitHubEventClient gitHubEventClient,
/// Resulting Actions:
/// Evaluate the user that created the issue:
/// IF creator is NOT an Azure SDK team owner
/// AND is NOT a member of the Azure organization
/// AND does NOT have write permission
/// AND does NOT have admin permission:
/// Add "customer-reported" label
/// Add "question" label
/// IF the user is NOT a member of the Azure Org
/// IF the user does not have Admin or Write Collaborator permission
/// Add "customer-reported" label
/// Add "question" label
/// Query AI label service for label suggestions:
/// IF labels were predicted:
/// Assign returned labels to the issue
Expand Down Expand Up @@ -101,6 +100,18 @@ public static async Task InitialIssueTriage(GitHubEventClient gitHubEventClient,
{
issueUpdate.AddLabel(LabelConstants.NeedsTriage);
}

// If the user is not a member of the Azure Org AND the user does not have write or admin collaborator permission
bool isMemberOfOrg = await gitHubEventClient.IsUserMemberOfOrg(OrgConstants.Azure, issueEventPayload.Issue.User.Login);
if (!isMemberOfOrg)
{
bool hasAdminOrWritePermission = await gitHubEventClient.DoesUserHaveAdminOrWritePermission(issueEventPayload.Repository.Id, issueEventPayload.Issue.User.Login);
if (!hasAdminOrWritePermission)
{
issueUpdate.AddLabel(LabelConstants.CustomerReported);
issueUpdate.AddLabel(LabelConstants.Question);
}
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,19 @@ public static async Task PullRequestTriage(GitHubEventClient gitHubEventClient,
}
}

// The sender will only have Write or Admin permssion if they are a collaborator
bool hasAdminOrWritePermission = await gitHubEventClient.DoesUserHaveAdminOrWritePermission(prEventPayload.Repository.Id, prEventPayload.PullRequest.User.Login);
// If the user doesn't have Write or Admin permissions
if (!hasAdminOrWritePermission)
// If the user is not a member of the Azure Org AND the user does not have write or admin collaborator permission
bool isMemberOfOrg = await gitHubEventClient.IsUserMemberOfOrg(OrgConstants.Azure, prEventPayload.PullRequest.User.Login);
if (!isMemberOfOrg)
{
var issueUpdate = gitHubEventClient.GetIssueUpdate(prEventPayload.PullRequest);
issueUpdate.AddLabel(LabelConstants.CustomerReported);
issueUpdate.AddLabel(LabelConstants.CommunityContribution);
string prComment = $"Thank you for your contribution @{prEventPayload.PullRequest.User.Login}! We will review the pull request and get back to you soon.";
gitHubEventClient.CreateComment(prEventPayload.Repository.Id, prEventPayload.PullRequest.Number, prComment);
bool hasAdminOrWritePermission = await gitHubEventClient.DoesUserHaveAdminOrWritePermission(prEventPayload.Repository.Id, prEventPayload.PullRequest.User.Login);
if (!hasAdminOrWritePermission)
{
var issueUpdate = gitHubEventClient.GetIssueUpdate(prEventPayload.PullRequest);
issueUpdate.AddLabel(LabelConstants.CustomerReported);
issueUpdate.AddLabel(LabelConstants.CommunityContribution);
string prComment = $"Thank you for your contribution @{prEventPayload.PullRequest.User.Login}! We will review the pull request and get back to you soon.";
gitHubEventClient.CreateComment(prEventPayload.Repository.Id, prEventPayload.PullRequest.Number, prComment);
}
}
}
}
Expand Down
21 changes: 11 additions & 10 deletions tools/github-event-processor/RULES.md
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,10 @@ This is a stand-alone service providing a REST API which requires a service key

```text
IF creator is NOT an Azure SDK team owner
AND is NOT a member of the Azure organization
AND is does NOT have a collaborator association
AND does NOT have write permission
AND does NOT have admin permission:

- Add "customer-reported" label
- Add "question" label
IF the user is NOT a member of the Azure Org
IF the user does not have Admin or Write Collaborator permission
- Add "customer-reported" label to the issue
- Add "question" label to the issue
```

- Query AI label service for suggestions:
Expand All @@ -126,6 +123,11 @@ This is a stand-alone service providing a REST API which requires a service key
- Add "needs-team-triage" label to the issue
ELSE
- Add "needs-triage" label to the issue

IF the user is NOT a member of the Azure Org
IF the user does not have Admin or Write Collaborator permission
- Add "customer-reported" label to the issue
- Add "question" label to the issue
```

- _This is what we'd like to get to. It requires changes to the CODEOWNERS file structure which have not been done yet_
Expand Down Expand Up @@ -446,9 +448,8 @@ OR
- Determine if this is a community contribution:

```text
IF the PR author does not have write permission
AND the PR author does not have write permission
AND the PR author does not have a collaborator association:
IF the user is NOT a member of the Azure Org
IF the user does not have Admin or Write Collaborator permission
- Add "customer-reported" label
- Add "Community Contribution" label
- Create the following comment
Expand Down