Skip to content

Commit

Permalink
Improve team/outsider detection
Browse files Browse the repository at this point in the history 
1. People with just "triage" permission are still considered team
   members
2. Issue reporters are always considered "outsiders" for the purposes of
   this app.
  • Loading branch information
@yrodiere
yrodiere committed Dec 9, 2024
1 parent ca7b13e commit 713bee0
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 34 deletions.
18 changes: 15 additions & 3 deletions src/main/java/io/quarkus/github/lottery/github/GitHubRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.kohsuke.github.GHIssueQueryBuilder;
import org.kohsuke.github.GHIssueState;
import org.kohsuke.github.GHRepository;
import org.kohsuke.github.GHUser;
import org.kohsuke.github.GitHub;

import io.quarkiverse.githubapp.ConfigFile;
Expand Down Expand Up @@ -202,9 +203,20 @@ private IssueActionSide lastActionSide(GHIssue ghIssue, Set<String> initialActio
// No action since the label was assigned.
return IssueActionSide.TEAM;
}
return switch (repository().getPermission(lastComment.get().getUser())) {
case ADMIN, WRITE -> IssueActionSide.TEAM;
case READ, UNKNOWN, NONE -> IssueActionSide.OUTSIDER;
return getIssueActionSide(ghIssue, lastComment.get().getUser());
}

private IssueActionSide getIssueActionSide(GHIssue issue, GHUser user) throws IOException {
if (issue.getUser().getLogin().equals(user.getLogin())) {
// This is the reporter; even if part of the team,
// we'll consider he's acting as an outsider here,
// because he's unlikely to ask for feedback from himself.
return IssueActionSide.OUTSIDER;
}

return switch (repository().getPermission(user)) {
case ADMIN, WRITE, UNKNOWN -> IssueActionSide.TEAM; // "Unknown" includes "triage"
case READ, NONE -> IssueActionSide.OUTSIDER;
};
}

Expand Down
108 changes: 77 additions & 31 deletions src/test/java/io/quarkus/github/lottery/GitHubServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ void issuesLastActedOnByAndLastUpdatedBefore_team() throws IOException {
Date issue1ActionLabelEvent = Date.from(cutoff.minus(1, ChronoUnit.DAYS));
Date issue2ActionLabelEvent = Date.from(cutoff.minus(2, ChronoUnit.DAYS));
Date issue7ActionLabelEvent = Date.from(cutoff.minus(2, ChronoUnit.DAYS));
Date issue8ActionLabelEvent = Date.from(cutoff.minus(2, ChronoUnit.DAYS));

var queryIssuesNeedsFeedbackBuilderMock = Mockito.mock(GHIssueQueryBuilder.ForRepository.class,
withSettings().defaultAnswer(Answers.RETURNS_SELF));
Expand All @@ -402,25 +403,12 @@ void issuesLastActedOnByAndLastUpdatedBefore_team() throws IOException {
withSettings().defaultAnswer(Answers.RETURNS_SELF));
var issue7QueryCommentsBuilderMock = Mockito.mock(GHIssueCommentQueryBuilder.class,
withSettings().defaultAnswer(Answers.RETURNS_SELF));
var issue8QueryCommentsBuilderMock = Mockito.mock(GHIssueCommentQueryBuilder.class,
withSettings().defaultAnswer(Answers.RETURNS_SELF));
given()
.github(mocks -> {
var repositoryMock = mocks.repository(repoRef.repositoryName());

when(repositoryMock.queryIssues()).thenReturn(queryIssuesNeedsFeedbackBuilderMock)
.thenReturn(queryIssuesNeedsReproducerBuilderMock);
var prMock = mockPullRequestForLotteryFilteredOutByRepository(mocks, 0);
var issue1Mock = mockIssueForLottery(mocks, 1, beforeCutoff);
var issue2Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 2, beforeCutoff);
var issue3Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 3, beforeCutoff);
var issue4Mock = mockIssueForLottery(mocks, 4, beforeCutoff);
var issue5Mock = mockIssueForLottery(mocks, 5, beforeCutoff);
var issue6Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 6, afterCutoff);
var issue7Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 7, beforeCutoff);
var issuesNeedsFeedbackMocks = mockPagedIterable(issue1Mock, issue3Mock, issue5Mock);
when(queryIssuesNeedsFeedbackBuilderMock.list()).thenReturn(issuesNeedsFeedbackMocks);
var issuesNeedsReproducerMocks = mockPagedIterable(prMock, issue2Mock, issue4Mock, issue6Mock, issue7Mock);
when(queryIssuesNeedsReproducerBuilderMock.list()).thenReturn(issuesNeedsReproducerMocks);

var adminUser = mockUserForInspectedComments(mocks, repositoryMock, 1L, "someadmin",
GHPermissionType.ADMIN);
var writeUser = mockUserForInspectedComments(mocks, repositoryMock, 2L, "somewriter",
Expand All @@ -429,6 +417,24 @@ void issuesLastActedOnByAndLastUpdatedBefore_team() throws IOException {
var noneUser = mockUserForInspectedComments(mocks, repositoryMock, 4L, "somestranger",
GHPermissionType.NONE);
var botUser = mockUserForInspectedComments(mocks, repositoryMock, 5L, "somebot[bot]");
var randomReporterUser = mockUserForInspectedComments(mocks, repositoryMock, 6L, "somereporter");

when(repositoryMock.queryIssues()).thenReturn(queryIssuesNeedsFeedbackBuilderMock)
.thenReturn(queryIssuesNeedsReproducerBuilderMock);
var prMock = mockPullRequestForLotteryFilteredOutByRepository(mocks, 0);
var issue1Mock = mockIssueForLottery(mocks, 1, beforeCutoff, randomReporterUser);
var issue2Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 2, beforeCutoff, randomReporterUser);
var issue3Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 3, beforeCutoff, randomReporterUser);
var issue4Mock = mockIssueForLottery(mocks, 4, beforeCutoff);
var issue5Mock = mockIssueForLottery(mocks, 5, beforeCutoff, randomReporterUser);
var issue6Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 6, afterCutoff);
var issue7Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 7, beforeCutoff, randomReporterUser);
var issue8Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 8, beforeCutoff, writeUser);
var issuesNeedsFeedbackMocks = mockPagedIterable(issue1Mock, issue3Mock, issue5Mock);
when(queryIssuesNeedsFeedbackBuilderMock.list()).thenReturn(issuesNeedsFeedbackMocks);
var issuesNeedsReproducerMocks = mockPagedIterable(prMock, issue2Mock, issue4Mock, issue6Mock, issue7Mock,
issue8Mock);
when(queryIssuesNeedsReproducerBuilderMock.list()).thenReturn(issuesNeedsReproducerMocks);

var needsReproducerLabelMock = mockLabel("triage/needs-reproducer");
var areaHibernateSearchLabelMock = mockLabel("area/hibernate-search");
Expand Down Expand Up @@ -501,6 +507,22 @@ void issuesLastActedOnByAndLastUpdatedBefore_team() throws IOException {
mockIssueComment(mocks, 703, botUser));
when(issue7Mock.queryComments()).thenReturn(issue7QueryCommentsBuilderMock);
when(issue7QueryCommentsBuilderMock.list()).thenReturn(issue7CommentsMocks);

// This is like issue 2, but the reporter is a team member -- so should be considered as an outsider.
var issue8Event1Mock = mockIssueEvent("created");
var issue8Event2Mock = mockIssueEvent("labeled");
when(issue8Event2Mock.getLabel()).thenReturn(needsReproducerLabelMock);
when(issue8Event2Mock.getCreatedAt()).thenReturn(issue8ActionLabelEvent);
var issue8Event3Mock = mockIssueEvent("labeled");
when(issue8Event3Mock.getLabel()).thenReturn(areaHibernateSearchLabelMock);
var issue8Event4Mock = mockIssueEvent("locked");
var issue8EventsMocks = mockPagedIterable(issue8Event1Mock,
issue8Event2Mock, issue8Event3Mock, issue8Event4Mock);
when(issue8Mock.listEvents()).thenReturn(issue8EventsMocks);
var issue8CommentsMocks = mockPagedIterable(mockIssueComment(mocks, 801, noneUser),
mockIssueComment(mocks, 802, writeUser));
when(issue8Mock.queryComments()).thenReturn(issue8QueryCommentsBuilderMock);
when(issue8QueryCommentsBuilderMock.list()).thenReturn(issue8CommentsMocks);
})
.when(() -> {
var repo = gitHubService.repository(repoRef);
Expand All @@ -526,6 +548,7 @@ void issuesLastActedOnByAndLastUpdatedBefore_team() throws IOException {
verify(issue1QueryCommentsBuilderMock).since(issue1ActionLabelEvent);
verify(issue2QueryCommentsBuilderMock).since(issue2ActionLabelEvent);
verify(issue7QueryCommentsBuilderMock).since(issue7ActionLabelEvent);
verify(issue8QueryCommentsBuilderMock).since(issue8ActionLabelEvent);

verifyNoMoreInteractions(mocks.ghObjects());
});
Expand All @@ -542,6 +565,7 @@ void issuesLastActedOnByAndLastUpdatedBefore_outsider() throws IOException {
Date issue1ActionLabelEvent = Date.from(cutoff.minus(1, ChronoUnit.DAYS));
Date issue2ActionLabelEvent = Date.from(cutoff.minus(2, ChronoUnit.DAYS));
Date issue7ActionLabelEvent = Date.from(cutoff.minus(2, ChronoUnit.DAYS));
Date issue8ActionLabelEvent = Date.from(cutoff.minus(2, ChronoUnit.DAYS));

var queryIssuesNeedsReproducerBuilderMock = Mockito.mock(GHIssueQueryBuilder.ForRepository.class,
withSettings().defaultAnswer(Answers.RETURNS_SELF));
Expand All @@ -559,36 +583,41 @@ void issuesLastActedOnByAndLastUpdatedBefore_outsider() throws IOException {
withSettings().defaultAnswer(Answers.RETURNS_SELF));
var issue7QueryCommentsBuilderMock = Mockito.mock(GHIssueCommentQueryBuilder.class,
withSettings().defaultAnswer(Answers.RETURNS_SELF));
var issue8QueryCommentsBuilderMock = Mockito.mock(GHIssueCommentQueryBuilder.class,
withSettings().defaultAnswer(Answers.RETURNS_SELF));
given()
.github(mocks -> {
var repositoryMock = mocks.repository(repoRef.repositoryName());

var adminUser = mockUserForInspectedComments(mocks, repositoryMock, 1L, "someadmin",
GHPermissionType.ADMIN);
var writeUser = mockUserForInspectedComments(mocks, repositoryMock, 2L, "somewriter",
GHPermissionType.WRITE);
var readUser = mockUserForInspectedComments(mocks, repositoryMock, 3L, "somereader", GHPermissionType.READ);
var noneUser = mockUserForInspectedComments(mocks, repositoryMock, 4L, "somestranger",
GHPermissionType.NONE);
var botUser = mockUserForInspectedComments(mocks, repositoryMock, 5L, "somebot[bot]");
var randomReporterUser = mockUserForInspectedComments(mocks, repositoryMock, 6L, "somereporter");

when(repositoryMock.queryIssues())
.thenReturn(queryIssuesNeedsFeedbackBuilderMock)
.thenReturn(queryIssuesNeedsReproducerBuilderMock);
// Pull requests should always be filtered out
var prMock = mockPullRequestForLotteryFilteredOutByRepository(mocks, 0);
var issue1Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 1, beforeCutoff);
var issue2Mock = mockIssueForLottery(mocks, 2, beforeCutoff);
var issue3Mock = mockIssueForLottery(mocks, 3, beforeCutoff);
var issue1Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 1, beforeCutoff, randomReporterUser);
var issue2Mock = mockIssueForLottery(mocks, 2, beforeCutoff, randomReporterUser);
var issue3Mock = mockIssueForLottery(mocks, 3, beforeCutoff, randomReporterUser);
var issue4Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 4, beforeCutoff);
var issue5Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 5, beforeCutoff);
var issue5Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 5, beforeCutoff, randomReporterUser);
var issue6Mock = mockIssueForLotteryFilteredOutByRepository(mocks, 6, afterCutoff);
var issue7Mock = mockIssueForLottery(mocks, 7, beforeCutoff);
var issuesNeedsFeedbackMocks = mockPagedIterable(prMock, issue2Mock, issue4Mock, issue6Mock, issue7Mock);
var issue7Mock = mockIssueForLottery(mocks, 7, beforeCutoff, randomReporterUser);
var issue8Mock = mockIssueForLottery(mocks, 8, beforeCutoff, writeUser);
var issuesNeedsFeedbackMocks = mockPagedIterable(prMock, issue2Mock, issue4Mock, issue6Mock, issue7Mock,
issue8Mock);
when(queryIssuesNeedsFeedbackBuilderMock.list()).thenReturn(issuesNeedsFeedbackMocks);
var issuesNeedsReproducerMocks = mockPagedIterable(issue1Mock, issue3Mock, issue5Mock);
when(queryIssuesNeedsReproducerBuilderMock.list()).thenReturn(issuesNeedsReproducerMocks);

var adminUser = mockUserForInspectedComments(mocks, repositoryMock, 1L, "someadmin",
GHPermissionType.ADMIN);
var writeUser = mockUserForInspectedComments(mocks, repositoryMock, 2L, "somewriter",
GHPermissionType.WRITE);
var readUser = mockUserForInspectedComments(mocks, repositoryMock, 3L, "somereader", GHPermissionType.READ);
var noneUser = mockUserForInspectedComments(mocks, repositoryMock, 4L, "somestranger",
GHPermissionType.NONE);
var botUser = mockUserForInspectedComments(mocks, repositoryMock, 5L, "somebot[bot]");

var needsReproducerLabelMock = mockLabel("triage/needs-reproducer");
var areaHibernateSearchLabelMock = mockLabel("area/hibernate-search");

Expand Down Expand Up @@ -660,14 +689,30 @@ void issuesLastActedOnByAndLastUpdatedBefore_outsider() throws IOException {
mockIssueComment(mocks, 703, botUser));
when(issue7Mock.queryComments()).thenReturn(issue7QueryCommentsBuilderMock);
when(issue7QueryCommentsBuilderMock.list()).thenReturn(issue7CommentsMocks);

// This is like issue 2, but the reporter is a team member -- so should be considered as an outsider.
var issue8Event1Mock = mockIssueEvent("created");
var issue8Event2Mock = mockIssueEvent("labeled");
when(issue8Event2Mock.getLabel()).thenReturn(needsReproducerLabelMock);
when(issue8Event2Mock.getCreatedAt()).thenReturn(issue8ActionLabelEvent);
var issue8Event3Mock = mockIssueEvent("labeled");
when(issue8Event3Mock.getLabel()).thenReturn(areaHibernateSearchLabelMock);
var issue8Event4Mock = mockIssueEvent("locked");
var issue8EventsMocks = mockPagedIterable(issue8Event1Mock,
issue8Event2Mock, issue8Event3Mock, issue8Event4Mock);
when(issue8Mock.listEvents()).thenReturn(issue8EventsMocks);
var issue8CommentsMocks = mockPagedIterable(mockIssueComment(mocks, 801, noneUser),
mockIssueComment(mocks, 802, writeUser));
when(issue8Mock.queryComments()).thenReturn(issue8QueryCommentsBuilderMock);
when(issue8QueryCommentsBuilderMock.list()).thenReturn(issue8CommentsMocks);
})
.when(() -> {
var repo = gitHubService.repository(repoRef);

assertThat(repo.issuesLastActedOnByAndLastUpdatedBefore(
new LinkedHashSet<>(List.of("triage/needs-feedback", "triage/needs-reproducer")),
"area/hibernate-search", IssueActionSide.OUTSIDER, cutoff))
.containsExactlyElementsOf(stubIssueList(2, 3, 7));
.containsExactlyElementsOf(stubIssueList(2, 3, 7, 8));
})
.then().github(mocks -> {
verify(queryIssuesNeedsFeedbackBuilderMock).state(GHIssueState.OPEN);
Expand All @@ -685,6 +730,7 @@ void issuesLastActedOnByAndLastUpdatedBefore_outsider() throws IOException {
verify(issue1QueryCommentsBuilderMock).since(issue1ActionLabelEvent);
verify(issue2QueryCommentsBuilderMock).since(issue2ActionLabelEvent);
verify(issue7QueryCommentsBuilderMock).since(issue7ActionLabelEvent);
verify(issue8QueryCommentsBuilderMock).since(issue8ActionLabelEvent);

verifyNoMoreInteractions(mocks.ghObjects());
});
Expand Down
22 changes: 22 additions & 0 deletions src/test/java/io/quarkus/github/lottery/util/MockHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,18 @@ public static GHIssue mockIssueForLottery(GitHubMockContext context, int number,
return mock;
}

public static GHIssue mockIssueForLottery(GitHubMockContext context, int number, Date updatedAt, GHUser reporter)
throws IOException {
GHIssue mock = context.issue(10000L + number);
when(mock.isPullRequest()).thenReturn(false);
when(mock.getNumber()).thenReturn(number);
when(mock.getTitle()).thenReturn("Title for issue " + number);
when(mock.getHtmlUrl()).thenReturn(url(number));
when(mock.getUpdatedAt()).thenReturn(updatedAt);
when(mock.getUser()).thenReturn(reporter);
return mock;
}

public static GHIssue mockIssueForLotteryFilteredOutByRepository(GitHubMockContext context, int number, Date updatedAt)
throws IOException {
GHIssue mock = context.issue(10000L + number);
Expand All @@ -90,6 +102,16 @@ public static GHPullRequest mockPullRequestForLotteryFilteredOutByRepository(Git
return mock;
}

public static GHIssue mockIssueForLotteryFilteredOutByRepository(GitHubMockContext context, int number, Date updatedAt,
GHUser reporter)
throws IOException {
GHIssue mock = context.issue(10000L + number);
when(mock.isPullRequest()).thenReturn(false);
when(mock.getUpdatedAt()).thenReturn(updatedAt);
when(mock.getUser()).thenReturn(reporter);
return mock;
}

public static GHIssue mockIssueForNotification(GitHubMockContext context, long id, String title) {
GHIssue mock = context.issue(id);
when(mock.getTitle()).thenReturn(title);
Expand Down

0 comments on commit 713bee0

Please sign in to comment.