Improve support for draft PRs (#771)

This addresses two separate feature requests for draft PRs: - Always show the "Draft" badge on PRs, not just for your own PRs. - Allow "Mute until draft". Fixes #398.
fwouts · Jun 2, 2020 · 9a11359 · 9a11359
1 parent 97c2e58
commit 9a11359
Show file tree

Hide file tree

Showing 7 changed files with 161 additions and 77 deletions.
diff --git a/src/components/PullRequestItem.tsx b/src/components/PullRequestItem.tsx
@@ -4,11 +4,11 @@ import { observer } from "mobx-react-lite";
 import React from "react";
 import { Dropdown } from "react-bootstrap";
 import { EnrichedPullRequest } from "../filtering/enriched-pull-request";
+import { isRunningAsPopup } from "../popup-environment";
 import { PullRequest } from "../storage/loaded-state";
 import { MuteType } from "../storage/mute-configuration";
 import { SmallButton } from "./design/Button";
 import { PullRequestStatus } from "./PullRequestStatus";
-import { isRunningAsPopup } from "../popup-environment";
 
 const PullRequestBox = styled.a`
   display: flex;
@@ -137,6 +137,11 @@ export const PullRequestItem = observer((props: PullRequestItemProps) => {
                 <Dropdown.Item onSelect={createMuteHandler("next-update")}>
                   Mute until next update by author
                 </Dropdown.Item>
+                {props.pullRequest.draft && (
+                  <Dropdown.Item onSelect={createMuteHandler("not-draft")}>
+                    Mute until not draft
+                  </Dropdown.Item>
+                )}
                 <Dropdown.Item onSelect={createMuteHandler("1-hour")}>
                   Mute for 1 hour
                 </Dropdown.Item>

diff --git a/src/components/PullRequestStatus.tsx b/src/components/PullRequestStatus.tsx
@@ -13,138 +13,120 @@ const StateBox = styled.div`
   padding: 0 8px;
 `;
 
+const SpacedBadge = styled(Badge)`
+  margin-right: 4px;
+`;
+
 const UNREVIEWED = (
-  <Badge pill variant="danger">
+  <SpacedBadge pill variant="danger" key="unreviewed">
     Unreviewed
-  </Badge>
+  </SpacedBadge>
 );
 
 const AUTHOR_REPLIED = (
-  <Badge pill variant="secondary">
+  <SpacedBadge pill variant="secondary" key="author-replied">
     Author replied
-  </Badge>
+  </SpacedBadge>
 );
 
 const NEW_COMMIT = (
-  <Badge pill variant="primary">
+  <SpacedBadge pill variant="primary" key="new-commit">
     New commit
-  </Badge>
+  </SpacedBadge>
 );
 
 const DRAFT = (
-  <Badge pill variant="dark">
+  <SpacedBadge pill variant="dark" key="draft">
     Draft
-  </Badge>
+  </SpacedBadge>
 );
 
 const MERGEABLE = (
-  <Badge pill variant="success">
+  <SpacedBadge pill variant="success" key="mergeable">
     Mergeable
-  </Badge>
+  </SpacedBadge>
 );
 
 const APPROVED_BY_EVERONE = (
-  <Badge pill variant="success">
+  <SpacedBadge pill variant="success" key="approved-by-everyone">
     Approved by everyone
-  </Badge>
+  </SpacedBadge>
 );
 
 const CHANGES_REQUESTED = (
-  <Badge pill variant="warning">
+  <SpacedBadge pill variant="warning" key="changes-requested">
     Changes requested
-  </Badge>
+  </SpacedBadge>
 );
 
 const WAITING_FOR_REVIEW = (
-  <Badge pill variant="info">
+  <SpacedBadge pill variant="info" key="waiting-for-review">
     Waiting for review
-  </Badge>
+  </SpacedBadge>
 );
 
 const NO_REVIEWER_ASSIGNED = (
-  <Badge pill variant="light">
+  <SpacedBadge pill variant="light" key="no-reviewer-assigned">
     No reviewer assigned
-  </Badge>
+  </SpacedBadge>
 );
 
 export const PullRequestStatus = observer(
   ({ pullRequest }: { pullRequest: EnrichedPullRequest }) => {
-    const state = renderState(pullRequest.state);
-    if (state) {
-      return <StateBox>{state}</StateBox>;
+    const badges = getBadges(pullRequest.state);
+    if (badges.length > 0) {
+      return <StateBox>{badges}</StateBox>;
     }
     return <></>;
   }
 );
 
-function renderState(state: PullRequestState) {
+function getBadges(state: PullRequestState): JSX.Element[] {
+  const badges: JSX.Element[] = [];
+  if (state.draft) {
+    badges.push(DRAFT);
+  }
   switch (state.kind) {
     case "incoming":
-      return renderIncomingState(state);
+      badges.push(...getIncomingStateBadges(state));
+      break;
     case "outgoing":
-      return addDraftTag(
-        state,
-        addMergeableTag(state, renderOutgoingState(state))
-      );
+      badges.push(...getOutgoingStateBadges(state));
+      break;
     default:
-      return null;
+    // Do nothing.
   }
+  return badges;
 }
 
-function renderIncomingState(state: IncomingState) {
+function getIncomingStateBadges(state: IncomingState): JSX.Element[] {
   if (state.newReviewRequested) {
-    return UNREVIEWED;
-  } else if (state.authorResponded && state.newCommit) {
-    return (
-      <>
-        {AUTHOR_REPLIED} {NEW_COMMIT}
-      </>
-    );
-  } else if (state.authorResponded) {
-    return AUTHOR_REPLIED;
-  } else if (state.newCommit) {
-    return NEW_COMMIT;
-  } else {
-    return null;
+    return [UNREVIEWED];
   }
-}
-
-function renderOutgoingState(state: OutgoingState): JSX.Element {
-  if (state.approvedByEveryone) {
-    return APPROVED_BY_EVERONE;
-  } else if (state.changesRequested) {
-    return CHANGES_REQUESTED;
-  } else if (state.noReviewers) {
-    return (
-      <>
-        {WAITING_FOR_REVIEW} {NO_REVIEWER_ASSIGNED}
-      </>
-    );
-  } else {
-    return WAITING_FOR_REVIEW;
+  const badges: JSX.Element[] = [];
+  if (state.authorResponded) {
+    badges.push(AUTHOR_REPLIED);
+  }
+  if (state.newCommit) {
+    badges.push(NEW_COMMIT);
   }
+  return badges;
 }
 
-function addMergeableTag(state: OutgoingState, otherTags: JSX.Element) {
+function getOutgoingStateBadges(state: OutgoingState): JSX.Element[] {
+  const badges: JSX.Element[] = [];
   if (state.mergeable) {
-    return (
-      <>
-        {MERGEABLE} {otherTags}
-      </>
-    );
-  } else {
-    return otherTags;
+    badges.push(MERGEABLE);
   }
-}
-
-function addDraftTag(state: OutgoingState, otherTags: JSX.Element) {
-  if (state.draft) {
-    return (
-      <>
-        {DRAFT} {otherTags}
-      </>
-    );
+  if (state.approvedByEveryone) {
+    badges.push(APPROVED_BY_EVERONE);
+  } else if (state.changesRequested) {
+    badges.push(CHANGES_REQUESTED);
   } else {
-    return otherTags;
+    badges.push(WAITING_FOR_REVIEW);
+    if (state.noReviewers) {
+      badges.push(NO_REVIEWER_ASSIGNED);
+    }
   }
+  return badges;
 }
diff --git a/src/filtering/filters.spec.ts b/src/filtering/filters.spec.ts
@@ -226,6 +226,35 @@ describe("filters (incoming)", () => {
       )
     ).toEqual([Filter.MUTED]);
   });
+  it("is MUTED when the PR is muted until not draft and the PR is still a draft", () => {
+    const env = buildTestingEnvironment();
+    expect(
+      getFilteredBucket(
+        env,
+        "kevin",
+        {
+          mutedPullRequests: [
+            {
+              repo: {
+                owner: "zenclabs",
+                name: "prmonitor",
+              },
+              number: 1,
+              until: {
+                kind: "not-draft",
+              },
+            },
+          ],
+        },
+        fakePullRequest()
+          .draft()
+          .author("fwouts")
+          .seenAs("kevin")
+          .reviewRequested(["kevin"])
+          .build()
+      )
+    ).toEqual([Filter.MUTED]);
+  });
   it("is MUTED when the PR is muted until a specific time that hasn't been reached yet", () => {
     const env = buildTestingEnvironment();
     env.currentTime = 50;
@@ -450,6 +479,34 @@ describe("filters (incoming)", () => {
       )
     ).toEqual([Filter.INCOMING]);
   });
+  it("is INCOMING when the PR was muted until not draft and the PR is no longer a draft", () => {
+    const env = buildTestingEnvironment();
+    expect(
+      getFilteredBucket(
+        env,
+        "kevin",
+        {
+          mutedPullRequests: [
+            {
+              repo: {
+                owner: "zenclabs",
+                name: "prmonitor",
+              },
+              number: 1,
+              until: {
+                kind: "not-draft",
+              },
+            },
+          ],
+        },
+        fakePullRequest()
+          .author("fwouts")
+          .seenAs("kevin")
+          .reviewRequested(["kevin"])
+          .build()
+      )
+    ).toEqual([Filter.INCOMING]);
+  });
   it("is INCOMING for a PR that needs review when an unrelated PR is muted", () => {
     const env = buildTestingEnvironment();
     expect(

diff --git a/src/filtering/muted.ts b/src/filtering/muted.ts
@@ -40,6 +40,8 @@ export function isMuted(
           const updatedSince =
             getLastAuthorUpdateTimestamp(pr) > muted.until.mutedAtTimestamp;
           return updatedSince ? MutedResult.VISIBLE : MutedResult.MUTED;
+        case "not-draft":
+          return pr.draft ? MutedResult.MUTED : MutedResult.VISIBLE;
         case "specific-time":
           return currentTime >= muted.until.unmuteAtTimestamp
             ? MutedResult.VISIBLE

diff --git a/src/filtering/status.spec.ts b/src/filtering/status.spec.ts
@@ -14,6 +14,7 @@ describe("pullRequestState", () => {
       )
     ).toEqual({
       kind: "incoming",
+      draft: false,
       newReviewRequested: true,
       newCommit: false,
       authorResponded: false,
@@ -31,6 +32,7 @@ describe("pullRequestState", () => {
       )
     ).toEqual({
       kind: "incoming",
+      draft: false,
       newReviewRequested: false,
       newCommit: false,
       authorResponded: true,
@@ -48,6 +50,7 @@ describe("pullRequestState", () => {
       )
     ).toEqual({
       kind: "incoming",
+      draft: false,
       newReviewRequested: false,
       newCommit: true,
       authorResponded: false,
@@ -66,6 +69,7 @@ describe("pullRequestState", () => {
       )
     ).toEqual({
       kind: "incoming",
+      draft: false,
       newReviewRequested: false,
       newCommit: true,
       authorResponded: true,
@@ -82,6 +86,7 @@ describe("pullRequestState", () => {
       )
     ).toEqual({
       kind: "incoming",
+      draft: false,
       newReviewRequested: false,
       newCommit: false,
       authorResponded: false,
@@ -98,6 +103,7 @@ describe("pullRequestState", () => {
       )
     ).toEqual({
       kind: "incoming",
+      draft: false,
       newReviewRequested: false,
       newCommit: false,
       authorResponded: false,
@@ -116,6 +122,7 @@ describe("pullRequestState", () => {
       )
     ).toEqual({
       kind: "not-involved",
+      draft: false,
     });
   });
 

diff --git a/src/filtering/status.ts b/src/filtering/status.ts
@@ -19,6 +19,7 @@ export function pullRequestState(
   if (!pr.reviewRequested && !userPreviouslyReviewed(pr, currentUserLogin)) {
     return {
       kind: "not-involved",
+      draft: pr.draft === true,
     };
   }
   return incomingPullRequestState(pr, currentUserLogin);
@@ -39,6 +40,7 @@ function incomingPullRequestState(
   const hasReviewed = lastReviewOrCommentFromCurrentUser > 0;
   return {
     kind: "incoming",
+    draft: pr.draft === true,
     newReviewRequested: !hasReviewed,
     authorResponded: hasReviewed && hasNewCommentByAuthor,
     newCommit: hasReviewed && hasNewCommit,
@@ -120,6 +122,11 @@ export type PullRequestState = IncomingState | NotInvolvedState | OutgoingState;
 export interface IncomingState {
   kind: "incoming";
 
+  /**
+   * True if the PR is a draft.
+   */
+  draft: boolean;
+
   /**
    * True if a review has been requested from the user, but they haven't
    * submitted any review or comments on the PR yet.
@@ -145,6 +152,11 @@ export interface IncomingState {
  */
 export interface NotInvolvedState {
   kind: "not-involved";
+
+  /**
+   * True if the PR is a draft.
+   */
+  draft: boolean;
 }
 
 /**