diff --git a/src/components/PullRequestItem.tsx b/src/components/PullRequestItem.tsx index 222292e6..0f893f20 100644 --- a/src/components/PullRequestItem.tsx +++ b/src/components/PullRequestItem.tsx @@ -2,8 +2,10 @@ import styled from "@emotion/styled"; import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; import { observer } from "mobx-react-lite"; import React from "react"; +import { EnrichedPullRequest } from "../filtering/enriched-pull-request"; import { PullRequest } from "../storage/loaded-state"; import { SmallButton } from "./design/Button"; +import { PullRequestStatus } from "./PullRequestStatus"; const PullRequestBox = styled.a` display: flex; @@ -71,7 +73,7 @@ const AuthorLogin = styled.div` `; export interface PullRequestItemProps { - pullRequest: PullRequest; + pullRequest: EnrichedPullRequest; allowMuting: boolean; onOpen(pullRequestUrl: string): void; onMute(pullRequest: PullRequest): void; @@ -100,6 +102,7 @@ export const PullRequestItem = observer((props: PullRequestItemProps) => { )} + {props.pullRequest.repoOwner}/{props.pullRequest.repoName} (# {props.pullRequest.pullRequestNumber}) diff --git a/src/components/PullRequestList.tsx b/src/components/PullRequestList.tsx index b9315577..fd7a10c4 100644 --- a/src/components/PullRequestList.tsx +++ b/src/components/PullRequestList.tsx @@ -1,6 +1,7 @@ import styled from "@emotion/styled"; import { observer } from "mobx-react-lite"; import React from "react"; +import { EnrichedPullRequest } from "../filtering/enriched-pull-request"; import { PullRequest } from "../storage/loaded-state"; import { Paragraph } from "./design/Paragraph"; import { Loader } from "./Loader"; @@ -14,7 +15,7 @@ export const List = styled.div` `; export interface PullRequestListProps { - pullRequests: PullRequest[] | null; + pullRequests: EnrichedPullRequest[] | null; emptyMessage: string; allowMuting: boolean; onOpen(pullRequestUrl: string): void; diff --git a/src/components/PullRequestStatus.tsx b/src/components/PullRequestStatus.tsx new file mode 100644 index 00000000..88edc736 --- /dev/null +++ b/src/components/PullRequestStatus.tsx @@ -0,0 +1,46 @@ +import styled from "@emotion/styled"; +import { observer } from "mobx-react-lite"; +import React from "react"; +import { Badge } from "react-bootstrap"; +import { PullRequestStatus as Status } from "../filtering/status"; + +const StatusBox = styled.div` + padding: 0 8px; +`; + +const UNREVIEWED = ( + + Unreviewed + +); + +const AUTHOR_REPLIED = ( + + Author replied + +); + +const NEW_COMMIT = ( + + New commit + +); + +export const PullRequestStatus = observer((props: { status: Status }) => { + switch (props.status) { + case Status.INCOMING_NEW_REVIEW_REQUESTED: + return {UNREVIEWED}; + case Status.INCOMING_REVIEWED_NEW_COMMENT_BY_AUTHOR: + return {AUTHOR_REPLIED}; + case Status.INCOMING_REVIEWED_NEW_COMMIT: + return {NEW_COMMIT}; + case Status.INCOMING_REVIEWED_NEW_COMMIT_AND_NEW_COMMENT_BY_AUTHOR: + return ( + + {AUTHOR_REPLIED} {NEW_COMMIT} + + ); + default: + return <>; + } +}); diff --git a/src/filtering/enriched-pull-request.ts b/src/filtering/enriched-pull-request.ts new file mode 100644 index 00000000..8e94439e --- /dev/null +++ b/src/filtering/enriched-pull-request.ts @@ -0,0 +1,6 @@ +import { PullRequest } from "../storage/loaded-state"; +import { PullRequestStatus } from "./status"; + +export interface EnrichedPullRequest extends PullRequest { + status: PullRequestStatus; +} diff --git a/src/filtering/filters-mine.spec.ts b/src/filtering/filters-mine.spec.ts deleted file mode 100644 index d56980ee..00000000 --- a/src/filtering/filters-mine.spec.ts +++ /dev/null @@ -1,39 +0,0 @@ -import { PullRequest } from "../storage/loaded-state"; -import { NOTHING_MUTED } from "../storage/mute-configuration"; -import { Filter, filterPredicate } from "./filters"; - -const DUMMY_PR: PullRequest = { - author: { - login: "fwouts", - avatarUrl: "http://url" - }, - repoOwner: "zenclabs", - repoName: "prmonitor", - pullRequestNumber: 1, - title: "Dummy PR", - updatedAt: "5 May 2019", - htmlUrl: "https://github.com/zenclabs/prmonitor/pull/1", - nodeId: "fake", - requestedReviewers: [], - reviews: [], - comments: [] -}; - -describe("filters (reviewed)", () => { - it("is true for the user's own PRs", () => { - expect( - filterPredicate("fwouts", NOTHING_MUTED, Filter.MINE)({ - ...DUMMY_PR, - requestedReviewers: ["fwouts"] - }) - ).toBe(true); - }); - it("is false for other PRs", () => { - expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.MINE)({ - ...DUMMY_PR, - requestedReviewers: ["fwouts"] - }) - ).toBe(false); - }); -}); diff --git a/src/filtering/filters-muted.spec.ts b/src/filtering/filters-muted.spec.ts deleted file mode 100644 index b34ff27e..00000000 --- a/src/filtering/filters-muted.spec.ts +++ /dev/null @@ -1,106 +0,0 @@ -import { PullRequest } from "../storage/loaded-state"; -import { NOTHING_MUTED } from "../storage/mute-configuration"; -import { Filter, filterPredicate } from "./filters"; - -const DUMMY_PR: PullRequest = { - author: { - login: "fwouts", - avatarUrl: "http://url" - }, - repoOwner: "zenclabs", - repoName: "prmonitor", - pullRequestNumber: 1, - title: "Dummy PR", - updatedAt: "5 May 2019", - htmlUrl: "https://github.com/zenclabs/prmonitor/pull/1", - nodeId: "fake", - requestedReviewers: [], - reviews: [], - comments: [] -}; - -describe("filters (muted)", () => { - it("is false for the user's own PRs", () => { - expect( - filterPredicate("fwouts", NOTHING_MUTED, Filter.MUTED)({ - ...DUMMY_PR, - requestedReviewers: ["fwouts"] - }) - ).toBe(false); - }); - it("is false when the user is not a reviewer and hasn't commented", () => { - expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.MUTED)({ - ...DUMMY_PR, - requestedReviewers: [] - }) - ).toBe(false); - }); - it("is false when the user is a reviewer, hasn't reviewed or commented and has not muted the PR", () => { - expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.MUTED)({ - ...DUMMY_PR, - requestedReviewers: ["kevin"] - }) - ).toBe(false); - }); - it("is true when the user is a reviewer, hasn't reviewed or commented and has muted the PR", () => { - expect( - filterPredicate( - "kevin", - { - mutedPullRequests: [ - { - repo: { - owner: "zenclabs", - name: "prmonitor" - }, - number: 1, - until: { - kind: "next-update", - mutedAtTimestamp: 1000 - } - } - ] - }, - Filter.INCOMING - )({ - ...DUMMY_PR, - requestedReviewers: ["kevin"] - }) - ).toBe(false); - }); - it("is false when the PR was muted but was since updated", () => { - expect( - filterPredicate( - "kevin", - { - mutedPullRequests: [ - { - repo: { - owner: "zenclabs", - name: "prmonitor" - }, - number: 1, - until: { - kind: "next-update", - mutedAtTimestamp: 1000 - } - } - ] - }, - Filter.MUTED - )({ - ...DUMMY_PR, - requestedReviewers: ["kevin"], - reviews: [ - { - authorLogin: "fwouts", - state: "COMMENTED", - submittedAt: "2019-02-15T17:00:11Z" - } - ] - }) - ).toBe(false); - }); -}); diff --git a/src/filtering/filters-reviewed.spec.ts b/src/filtering/filters-reviewed.spec.ts deleted file mode 100644 index b833200b..00000000 --- a/src/filtering/filters-reviewed.spec.ts +++ /dev/null @@ -1,95 +0,0 @@ -import { PullRequest } from "../storage/loaded-state"; -import { NOTHING_MUTED } from "../storage/mute-configuration"; -import { Filter, filterPredicate } from "./filters"; - -const DUMMY_PR: PullRequest = { - author: { - login: "fwouts", - avatarUrl: "http://url" - }, - repoOwner: "zenclabs", - repoName: "prmonitor", - pullRequestNumber: 1, - title: "Dummy PR", - updatedAt: "5 May 2019", - htmlUrl: "https://github.com/zenclabs/prmonitor/pull/1", - nodeId: "fake", - requestedReviewers: [], - reviews: [], - comments: [] -}; - -describe("filters (reviewed)", () => { - it("is false for the user's own PRs", () => { - expect( - filterPredicate("fwouts", NOTHING_MUTED, Filter.REVIEWED)({ - ...DUMMY_PR, - requestedReviewers: ["fwouts"] - }) - ).toBe(false); - }); - it("is false when the user is not a reviewer and hasn't commented", () => { - expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.REVIEWED)({ - ...DUMMY_PR, - requestedReviewers: [] - }) - ).toBe(false); - }); - it("is false when the user needs to review the PR", () => { - expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.REVIEWED)({ - ...DUMMY_PR, - requestedReviewers: ["kevin"] - }) - ).toBe(false); - }); - it("is true when the user is a reviewer and has already reviewed", () => { - expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.REVIEWED)({ - ...DUMMY_PR, - requestedReviewers: [], - reviews: [ - { - authorLogin: "kevin", - state: "COMMENTED", - submittedAt: "2019-02-15T17:00:11Z" - } - ] - }) - ).toBe(true); - }); - it("is true when the user is a reviewer and has already reviewed, even if the PR is muted", () => { - expect( - filterPredicate( - "kevin", - { - mutedPullRequests: [ - { - repo: { - owner: "zenclabs", - name: "prmonitor" - }, - number: 1, - until: { - kind: "next-update", - mutedAtTimestamp: 1000 - } - } - ] - }, - Filter.REVIEWED - )({ - ...DUMMY_PR, - requestedReviewers: [], - reviews: [ - { - authorLogin: "kevin", - state: "COMMENTED", - submittedAt: "2019-02-15T17:00:11Z" - } - ] - }) - ).toBe(true); - }); -}); diff --git a/src/filtering/filters-incoming.spec.ts b/src/filtering/filters.spec.ts similarity index 61% rename from src/filtering/filters-incoming.spec.ts rename to src/filtering/filters.spec.ts index ffbdddfa..d9c16b5f 100644 --- a/src/filtering/filters-incoming.spec.ts +++ b/src/filtering/filters.spec.ts @@ -1,6 +1,7 @@ import { PullRequest } from "../storage/loaded-state"; import { NOTHING_MUTED } from "../storage/mute-configuration"; -import { Filter, filterPredicate } from "./filters"; +import { Filter } from "./filters"; +import { getFilteredBucket } from "./testing"; const DUMMY_PR: PullRequest = { author: { @@ -20,33 +21,33 @@ const DUMMY_PR: PullRequest = { }; describe("filters (incoming)", () => { - it("is false for the user's own PRs", () => { + it("is MINE for the user's own PRs", () => { expect( - filterPredicate("fwouts", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("fwouts", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: ["fwouts"] }) - ).toBe(false); + ).toEqual([Filter.MINE]); }); - it("is false when the user is not a reviewer and hasn't commented", () => { + it("is NOTHING when the user is not a reviewer and hasn't commented", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: [] }) - ).toBe(false); + ).toEqual([]); }); - it("is true when the user is a reviewer and hasn't reviewed or commented", () => { + it("is INCOMING when the user is a reviewer and hasn't reviewed or commented", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: ["kevin"] }) - ).toBe(true); + ).toEqual([Filter.INCOMING]); }); - it("is true when the user is not a reviewer but had reviewed, and the author responds", () => { + it("is INCOMING when the user is not a reviewer but had reviewed, and the author responds", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: [], reviews: [ @@ -63,11 +64,11 @@ describe("filters (incoming)", () => { } ] }) - ).toBe(true); + ).toEqual([Filter.INCOMING]); }); - it("is true when the user is not a reviewer but had commented, and the author responds", () => { + it("is INCOMING when the user is not a reviewer but had commented, and the author responds", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: [], comments: [ @@ -81,11 +82,11 @@ describe("filters (incoming)", () => { } ] }) - ).toBe(true); + ).toEqual([Filter.INCOMING]); }); - it("is false when the user has reviewed and the author hasn't responded", () => { + it("is REVIEWED when the user has reviewed and the author hasn't responded", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: [], reviews: [ @@ -96,11 +97,11 @@ describe("filters (incoming)", () => { } ] }) - ).toBe(false); + ).toEqual([Filter.REVIEWED]); }); - it("is false when the user is a reviewer, has commented and the author hasn't responded", () => { + it("is REVIEWED when the user is a reviewer, has commented and the author hasn't responded", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: ["kevin"], comments: [ @@ -118,11 +119,11 @@ describe("filters (incoming)", () => { } ] }) - ).toBe(false); + ).toEqual([Filter.REVIEWED]); }); - it("is true when the author responded with a comment", () => { + it("is INCOMING when the author responded with a comment", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: [], comments: [ @@ -136,11 +137,11 @@ describe("filters (incoming)", () => { } ] }) - ).toBe(true); + ).toEqual([Filter.INCOMING]); }); - it("is true when the author responded with a review", () => { + it("is INCOMING when the author responded with a review", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: [], comments: [ @@ -157,11 +158,11 @@ describe("filters (incoming)", () => { } ] }) - ).toBe(true); + ).toEqual([Filter.INCOMING]); }); - it("is true when the PR was previously reviewed but the author responded", () => { + it("is INCOMING when the PR was previously reviewed but the author responded", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: [], reviews: [ @@ -178,11 +179,11 @@ describe("filters (incoming)", () => { } ] }) - ).toBe(true); + ).toEqual([Filter.INCOMING]); }); - it("is true when the PR was approved but the author responded", () => { + it("is INCOMING when the PR was approved but the author responded", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: [], reviews: [ @@ -199,11 +200,11 @@ describe("filters (incoming)", () => { } ] }) - ).toBe(true); + ).toEqual([Filter.INCOMING]); }); - it("ignores pending review comments", () => { + it("is still INCOMING when there are pending review comments", () => { expect( - filterPredicate("kevin", NOTHING_MUTED, Filter.INCOMING)({ + getFilteredBucket("kevin", NOTHING_MUTED, { ...DUMMY_PR, requestedReviewers: ["kevin"], reviews: [ @@ -214,11 +215,11 @@ describe("filters (incoming)", () => { } ] }) - ).toBe(true); + ).toEqual([Filter.INCOMING]); }); - it("ignores muted PRs that the author did not add new comments or reviews to", () => { + it("is MUTED when the PR is muted and the author did not add new comments or reviews to", () => { expect( - filterPredicate( + getFilteredBucket( "kevin", { mutedPullRequests: [ @@ -235,24 +236,24 @@ describe("filters (incoming)", () => { } ] }, - Filter.INCOMING - )({ - ...DUMMY_PR, - requestedReviewers: ["kevin"], - reviews: [ - // Another user posted a review after we muted. - { - authorLogin: "dries", - state: "CHANGES_REQUESTED", - submittedAt: "2019-03-18T17:00:11Z" - } - ] - }) - ).toBe(false); + { + ...DUMMY_PR, + requestedReviewers: ["kevin"], + reviews: [ + // Another user posted a review after we muted. + { + authorLogin: "dries", + state: "CHANGES_REQUESTED", + submittedAt: "2019-03-18T17:00:11Z" + } + ] + } + ) + ).toEqual([Filter.MUTED]); }); - it("does not ignore muted PRs that the author added comments to since muting", () => { + it("is INCOMING when the PR was muted but the author added comments since muting", () => { expect( - filterPredicate( + getFilteredBucket( "kevin", { mutedPullRequests: [ @@ -269,22 +270,22 @@ describe("filters (incoming)", () => { } ] }, - Filter.INCOMING - )({ - ...DUMMY_PR, - requestedReviewers: ["kevin"], - comments: [ - { - authorLogin: "fwouts", - createdAt: "2019-03-18T17:00:11Z" - } - ] - }) - ).toBe(true); + { + ...DUMMY_PR, + requestedReviewers: ["kevin"], + comments: [ + { + authorLogin: "fwouts", + createdAt: "2019-03-18T17:00:11Z" + } + ] + } + ) + ).toEqual([Filter.INCOMING]); }); - it("is true for a PR that needs review when an unrelated PR is muted", () => { + it("is INCOMING for a PR that needs review when an unrelated PR is muted", () => { expect( - filterPredicate( + getFilteredBucket( "kevin", { mutedPullRequests: [ @@ -301,11 +302,11 @@ describe("filters (incoming)", () => { } ] }, - Filter.INCOMING - )({ - ...DUMMY_PR, - requestedReviewers: ["kevin"] - }) - ).toBe(true); + { + ...DUMMY_PR, + requestedReviewers: ["kevin"] + } + ) + ).toEqual([Filter.INCOMING]); }); }); diff --git a/src/filtering/filters.ts b/src/filtering/filters.ts index b484ed5b..7e855f8e 100644 --- a/src/filtering/filters.ts +++ b/src/filtering/filters.ts @@ -1,9 +1,12 @@ -import assertNever from "assert-never"; import { PullRequest } from "../storage/loaded-state"; import { MuteConfiguration } from "../storage/mute-configuration"; +import { EnrichedPullRequest } from "./enriched-pull-request"; import { isMuted } from "./muted"; -import { isReviewNeeded } from "./review-needed"; -import { userPreviouslyReviewed } from "./reviewed"; +import { + isReviewRequired, + pullRequestStatus, + PullRequestStatus +} from "./status"; export enum Filter { /** @@ -29,47 +32,29 @@ export enum Filter { MINE = "mine" } -export type FilteredPullRequests = { [filter in Filter]: PullRequest[] }; +export type FilteredPullRequests = { + [filter in Filter]: EnrichedPullRequest[] +}; export function filterPullRequests( userLogin: string, openPullRequests: PullRequest[], muteConfiguration: MuteConfiguration ): FilteredPullRequests { + const enrichedPullRequests = openPullRequests.map(pr => ({ + status: pullRequestStatus(pr, userLogin), + ...pr + })); return { - incoming: openPullRequests.filter( - filterPredicate(userLogin, muteConfiguration, Filter.INCOMING) + incoming: enrichedPullRequests.filter( + pr => isReviewRequired(pr.status) && !isMuted(pr, muteConfiguration) ), - muted: openPullRequests.filter( - filterPredicate(userLogin, muteConfiguration, Filter.MUTED) + muted: enrichedPullRequests.filter( + pr => isReviewRequired(pr.status) && isMuted(pr, muteConfiguration) ), - reviewed: openPullRequests.filter( - filterPredicate(userLogin, muteConfiguration, Filter.REVIEWED) + reviewed: enrichedPullRequests.filter( + pr => pr.status === PullRequestStatus.INCOMING_REVIEWED_PENDING_REPLY ), - mine: openPullRequests.filter( - filterPredicate(userLogin, muteConfiguration, Filter.MINE) - ) + mine: enrichedPullRequests.filter(pr => pr.author.login === userLogin) }; } - -export function filterPredicate( - userLogin: string, - muteConfiguration: MuteConfiguration, - filter: Filter -): (pr: PullRequest) => boolean { - switch (filter) { - case Filter.INCOMING: - return pr => - isReviewNeeded(pr, userLogin) && !isMuted(pr, muteConfiguration); - case Filter.MUTED: - return pr => - isReviewNeeded(pr, userLogin) && isMuted(pr, muteConfiguration); - case Filter.REVIEWED: - return pr => - userPreviouslyReviewed(pr, userLogin) && !isReviewNeeded(pr, userLogin); - case Filter.MINE: - return pr => pr.author.login === userLogin; - default: - throw assertNever(filter); - } -} diff --git a/src/filtering/muted.ts b/src/filtering/muted.ts index 1ece2987..00e76f3e 100644 --- a/src/filtering/muted.ts +++ b/src/filtering/muted.ts @@ -1,6 +1,6 @@ import { PullRequest } from "../storage/loaded-state"; import { MuteConfiguration } from "../storage/mute-configuration"; -import { getLastAuthorCommentTimestamp } from "./timestamps"; +import { getLastChangeTimestamp } from "./timestamps"; /** * Returns whether the pull request is muted. @@ -16,7 +16,7 @@ export function isMuted(pr: PullRequest, muteConfiguration: MuteConfiguration) { switch (muted.until.kind) { case "next-update": const updatedSince = - getLastAuthorCommentTimestamp(pr) > muted.until.mutedAtTimestamp; + getLastChangeTimestamp(pr) > muted.until.mutedAtTimestamp; return !updatedSince; } } diff --git a/src/filtering/review-needed.ts b/src/filtering/review-needed.ts deleted file mode 100644 index c024930f..00000000 --- a/src/filtering/review-needed.ts +++ /dev/null @@ -1,45 +0,0 @@ -import { PullRequest } from "../storage/loaded-state"; -import { userPreviouslyReviewed } from "./reviewed"; -import { - getLastAuthorCommentTimestamp, - getLastReviewOrCommentTimestamp -} from "./timestamps"; - -/** - * Returns whether another review is needed for a PR: - * - either a review is explicitly requested - * - or the PR has been updated by the author (new commit or comment) since the last review - */ -export function isReviewNeeded( - pr: PullRequest, - currentUserLogin: string -): boolean { - return ( - pr.author.login !== currentUserLogin && - (reviewRequested(pr, currentUserLogin) || - userPreviouslyReviewed(pr, currentUserLogin)) && - isNewReviewNeeded(pr, currentUserLogin) - ); -} - -/** - * Returns whether a review is specifically requested from the user. - */ -function reviewRequested(pr: PullRequest, currentUserLogin: string): boolean { - return pr.requestedReviewers.includes(currentUserLogin); -} - -/** - * Returns whether the user, who previously wrote a review, needs to take another look. - */ -function isNewReviewNeeded(pr: PullRequest, currentUserLogin: string): boolean { - const lastReviewOrCommentFromCurrentUser = getLastReviewOrCommentTimestamp( - pr, - currentUserLogin - ); - const lastCommentFromAuthor = getLastAuthorCommentTimestamp(pr); - return ( - lastReviewOrCommentFromCurrentUser === 0 || - lastReviewOrCommentFromCurrentUser < lastCommentFromAuthor - ); -} diff --git a/src/filtering/status.ts b/src/filtering/status.ts new file mode 100644 index 00000000..c7b07628 --- /dev/null +++ b/src/filtering/status.ts @@ -0,0 +1,85 @@ +import { PullRequest } from "../storage/loaded-state"; +import { userPreviouslyReviewed } from "./reviewed"; +import { + getLastAuthorCommentTimestamp, + getLastCommitTimestamp, + getLastReviewOrCommentTimestamp +} from "./timestamps"; + +/** + * Returns whether another review is needed for a PR: + * - either a review is explicitly requested + * - or the PR has been updated by the author (new commit or comment) since the last review + */ +export function pullRequestStatus( + pr: PullRequest, + currentUserLogin: string +): PullRequestStatus { + if (pr.author.login === currentUserLogin) { + return PullRequestStatus.OUTGOING; + } + if ( + !reviewRequested(pr, currentUserLogin) && + !userPreviouslyReviewed(pr, currentUserLogin) + ) { + return PullRequestStatus.NOT_INVOLVED; + } + return incomingPullRequestStatus(pr, currentUserLogin); +} + +/** + * Returns whether a review is specifically requested from the user. + */ +function reviewRequested(pr: PullRequest, currentUserLogin: string): boolean { + return pr.requestedReviewers.includes(currentUserLogin); +} + +/** + * Returns whether the user, who previously wrote a review, needs to take another look. + */ +function incomingPullRequestStatus( + pr: PullRequest, + currentUserLogin: string +): PullRequestStatus { + const lastReviewOrCommentFromCurrentUser = getLastReviewOrCommentTimestamp( + pr, + currentUserLogin + ); + const hasNewCommentByAuthor = + lastReviewOrCommentFromCurrentUser < getLastAuthorCommentTimestamp(pr); + const hasNewCommit = + lastReviewOrCommentFromCurrentUser < getLastCommitTimestamp(pr); + if (lastReviewOrCommentFromCurrentUser === 0) { + return PullRequestStatus.INCOMING_NEW_REVIEW_REQUESTED; + } else if (hasNewCommentByAuthor && hasNewCommit) { + return PullRequestStatus.INCOMING_REVIEWED_NEW_COMMIT_AND_NEW_COMMENT_BY_AUTHOR; + } else if (hasNewCommit) { + return PullRequestStatus.INCOMING_REVIEWED_NEW_COMMIT; + } else if (hasNewCommentByAuthor) { + return PullRequestStatus.INCOMING_REVIEWED_NEW_COMMENT_BY_AUTHOR; + } else { + return PullRequestStatus.INCOMING_REVIEWED_PENDING_REPLY; + } +} + +export enum PullRequestStatus { + INCOMING_NEW_REVIEW_REQUESTED, + INCOMING_REVIEWED_NEW_COMMENT_BY_AUTHOR, + INCOMING_REVIEWED_NEW_COMMIT, + INCOMING_REVIEWED_NEW_COMMIT_AND_NEW_COMMENT_BY_AUTHOR, + INCOMING_REVIEWED_PENDING_REPLY, + NOT_INVOLVED, + OUTGOING +} + +export function isReviewRequired(status: PullRequestStatus) { + switch (status) { + case PullRequestStatus.INCOMING_NEW_REVIEW_REQUESTED: + case PullRequestStatus.INCOMING_REVIEWED_NEW_COMMENT_BY_AUTHOR: + case PullRequestStatus.INCOMING_REVIEWED_NEW_COMMIT: + case PullRequestStatus.INCOMING_REVIEWED_NEW_COMMIT_AND_NEW_COMMENT_BY_AUTHOR: + return true; + default: + return false; + } +} diff --git a/src/filtering/testing.ts b/src/filtering/testing.ts new file mode 100644 index 00000000..0dd894ac --- /dev/null +++ b/src/filtering/testing.ts @@ -0,0 +1,29 @@ +import { PullRequest } from "../storage/loaded-state"; +import { MuteConfiguration } from "../storage/mute-configuration"; +import { Filter, filterPullRequests } from "./filters"; + +export function getFilteredBucket( + userLogin: string, + muteConfiguration: MuteConfiguration, + pr: PullRequest +) { + const filteredPullRequests = filterPullRequests( + userLogin, + [pr], + muteConfiguration + ); + const filters: Filter[] = []; + if (filteredPullRequests.incoming.length > 0) { + filters.push(Filter.INCOMING); + } + if (filteredPullRequests.muted.length > 0) { + filters.push(Filter.MUTED); + } + if (filteredPullRequests.reviewed.length > 0) { + filters.push(Filter.REVIEWED); + } + if (filteredPullRequests.mine.length > 0) { + filters.push(Filter.MINE); + } + return filters; +} diff --git a/src/filtering/timestamps.ts b/src/filtering/timestamps.ts index 9f0a9c18..69b5367b 100644 --- a/src/filtering/timestamps.ts +++ b/src/filtering/timestamps.ts @@ -1,5 +1,12 @@ import { PullRequest } from "../storage/loaded-state"; +export function getLastChangeTimestamp(pr: PullRequest): number { + return Math.max( + getLastAuthorCommentTimestamp(pr), + getLastCommitTimestamp(pr) + ); +} + export function getLastAuthorCommentTimestamp(pr: PullRequest): number { return getLastReviewOrCommentTimestamp(pr, pr.author.login); } @@ -28,3 +35,12 @@ export function getLastReviewOrCommentTimestamp( } return lastCommentedTime; } + +export function getLastCommitTimestamp(pr: PullRequest): number { + let lastCommitTime = 0; + for (const commit of pr.commits || []) { + const createdAt = new Date(commit.createdAt).getTime(); + lastCommitTime = Math.max(lastCommitTime, createdAt); + } + return lastCommitTime; +} diff --git a/src/notifications/api.ts b/src/notifications/api.ts index 308ee359..febcd8fa 100644 --- a/src/notifications/api.ts +++ b/src/notifications/api.ts @@ -1,8 +1,8 @@ -import { PullRequest } from "../storage/loaded-state"; +import { EnrichedPullRequest } from "../filtering/enriched-pull-request"; export interface Notifier { notify( - unreviewedPullRequests: PullRequest[], + unreviewedPullRequests: EnrichedPullRequest[], alreadyNotifiedPullRequestUrls: Set ): void; registerClickListener(clickListener: NotifierClickListener): void; diff --git a/src/notifications/implementation.ts b/src/notifications/implementation.ts index c1cbf01c..7cbcaae8 100644 --- a/src/notifications/implementation.ts +++ b/src/notifications/implementation.ts @@ -1,5 +1,6 @@ import { ChromeApi } from "../chrome/api"; -import { PullRequest } from "../storage/loaded-state"; +import { EnrichedPullRequest } from "../filtering/enriched-pull-request"; +import { PullRequestStatus } from "../filtering/status"; import { Notifier } from "./api"; export function buildNotifier(chromeApi: ChromeApi): Notifier { @@ -26,7 +27,7 @@ export function buildNotifier(chromeApi: ChromeApi): Notifier { */ async function showNotificationForNewPullRequests( chromeApi: ChromeApi, - unreviewedPullRequests: PullRequest[], + unreviewedPullRequests: EnrichedPullRequest[], notifiedPullRequestUrls: Set ) { if (!unreviewedPullRequests) { @@ -46,7 +47,10 @@ async function showNotificationForNewPullRequests( /** * Shows a notification if the pull request is new. */ -function showNotification(chromeApi: ChromeApi, pullRequest: PullRequest) { +function showNotification( + chromeApi: ChromeApi, + pullRequest: EnrichedPullRequest +) { // We set the notification ID to the URL so that we simply cannot have duplicate // notifications about the same pull request and we can easily open a browser tab // to this pull request just by knowing the notification ID. @@ -58,8 +62,26 @@ function showNotification(chromeApi: ChromeApi, pullRequest: PullRequest) { chromeApi.notifications.create(notificationId, { type: "basic", iconUrl: "images/GitHub-Mark-120px-plus.png", - title: "New pull request", - message: pullRequest.title, + title: getTitle(pullRequest), + message: getMessage(pullRequest), ...(supportsRequireInteraction ? { requireInteraction: true } : {}) }); } + +function getTitle(pullRequest: EnrichedPullRequest): string { + switch (pullRequest.status) { + case PullRequestStatus.INCOMING_NEW_REVIEW_REQUESTED: + return "New pull request"; + case PullRequestStatus.INCOMING_REVIEWED_NEW_COMMENT_BY_AUTHOR: + return `${pullRequest.author.login} commented`; + case PullRequestStatus.INCOMING_REVIEWED_NEW_COMMIT: + case PullRequestStatus.INCOMING_REVIEWED_NEW_COMMIT_AND_NEW_COMMENT_BY_AUTHOR: + return `Pull request updated`; + default: + throw new Error(`Well, this should never happen.`); + } +} + +function getMessage(pullRequest: EnrichedPullRequest): string { + return pullRequest.title; +} diff --git a/src/state/core.ts b/src/state/core.ts index 805ff703..5a041137 100644 --- a/src/state/core.ts +++ b/src/state/core.ts @@ -1,6 +1,7 @@ import { computed, observable } from "mobx"; import { BadgeState } from "../badge/api"; import { Environment } from "../environment/api"; +import { EnrichedPullRequest } from "../filtering/enriched-pull-request"; import { Filter, FilteredPullRequests, @@ -167,7 +168,7 @@ export class Core { } @computed - get unreviewedPullRequests(): PullRequest[] | null { + get unreviewedPullRequests(): EnrichedPullRequest[] | null { return this.filteredPullRequests ? this.filteredPullRequests[Filter.INCOMING] : null;