do not attempt to close the main review for each pending reviewer

Author: kylehoehns

src/services/RequestService.ts

@@ -10,55 +10,78 @@ import { App } from '@slack/bolt';
 import { reviewCloser } from '@/services/ReviewCloser';
 import log from '@utils/log';
 
-export const expireRequest = moveOntoNextPerson(
-  'The request has expired. You will keep your spot in the queue.',
-);
+const expireMessage = 'The request has expired. You will keep your spot in the queue.';
+
+export const expireRequest = moveOntoNextPerson(expireMessage);
 
 export const declineRequest = moveOntoNextPerson('Thanks! You will keep your spot in the queue.');
 
-/**
- * Notify the user if necessary, and request the next person in line
- */
-function moveOntoNextPerson(closeMessage: string) {
-  return async (app: App, activeReview: Readonly<ActiveReview>, previousUserId: string) => {
-    const updatedReview: ActiveReview = {
-      ...activeReview,
-    };
+export const closeRequest = async (
+  app: App,
+  activeReview: Readonly<ActiveReview>,
+  previousUserId: string,
+): Promise<ActiveReview> => {
+  return closeRequestInternal(app, activeReview, previousUserId, expireMessage);
+};
+
+const closeRequestInternal = async (
+  app: App,
+  activeReview: Readonly<ActiveReview>,
+  previousUserId: string,
+  closeMessage: string,
+): Promise<ActiveReview> => {
+  const updatedReview: ActiveReview = {
+    ...activeReview,
+  };
+
+  log.d(
+    'requestService.moveOnToNextPerson',
+    `Moving on to next person for ${activeReview.threadId}`,
+  );
 
+  const priorPendingReviewer = updatedReview.pendingReviewers.find(
+    ({ userId }) => userId === previousUserId,
+  );
+  if (priorPendingReviewer) {
+    updatedReview.pendingReviewers = updatedReview.pendingReviewers.filter(
+      ({ userId }) => userId !== previousUserId,
+    );
+    updatedReview.declinedReviewers.push({
+      userId: previousUserId,
+      declinedAt: new Date().getTime(),
+    });
     log.d(
       'requestService.moveOnToNextPerson',
-      `Moving on to next person for ${activeReview.threadId}`,
+      `Adding ${previousUserId} to declined reviewers for ${activeReview.threadId}`,
+    );
+    await activeReviewRepo.update(updatedReview);
+    const contextBlock = requestBuilder.buildReviewSectionBlock(
+      { id: updatedReview.requestorId },
+      updatedReview.languages,
+      DeadlineLabel.get(updatedReview.dueBy) || 'Unknown',
+    );
+    const closeMessageBlock = textBlock(closeMessage);
+    await chatService.updateDirectMessage(
+      app.client,
+      priorPendingReviewer.userId,
+      priorPendingReviewer.messageTimestamp,
+      [contextBlock, closeMessageBlock],
     );
+  }
+  return updatedReview;
+};
 
-    const priorPendingReviewer = updatedReview.pendingReviewers.find(
-      ({ userId }) => userId === previousUserId,
+/**
+ * Notify the user if necessary, and request the next person in line
+ */
+function moveOntoNextPerson(closeMessage: string) {
+  return async (app: App, activeReview: Readonly<ActiveReview>, previousUserId: string) => {
+    const updatedReview = await closeRequestInternal(
+      app,
+      activeReview,
+      previousUserId,
+      closeMessage,
     );
-    if (priorPendingReviewer) {
-      updatedReview.pendingReviewers = updatedReview.pendingReviewers.filter(
-        ({ userId }) => userId !== previousUserId,
-      );
-      updatedReview.declinedReviewers.push({
-        userId: previousUserId,
-        declinedAt: new Date().getTime(),
-      });
-      log.d(
-        'requestService.moveOnToNextPerson',
-        `Adding ${previousUserId} to declined reviewers for ${activeReview.threadId}`,
-      );
-      await activeReviewRepo.update(updatedReview);
-      const contextBlock = requestBuilder.buildReviewSectionBlock(
-        { id: updatedReview.requestorId },
-        updatedReview.languages,
-        DeadlineLabel.get(updatedReview.dueBy) || 'Unknown',
-      );
-      const closeMessageBlock = textBlock(closeMessage);
-      await chatService.updateDirectMessage(
-        app.client,
-        priorPendingReviewer.userId,
-        priorPendingReviewer.messageTimestamp,
-        [contextBlock, closeMessageBlock],
-      );
-    }
 
     await requestNextUserReview(updatedReview, app.client);
 

src/services/ReviewCloser.ts

@@ -4,15 +4,15 @@ import { mention } from '@/utils/text';
 import { App } from '@slack/bolt';
 import { chatService } from '@/services/ChatService';
 import { ActiveReview } from '@models/ActiveReview';
-import { expireRequest } from '@/services/RequestService';
+import { closeRequest } from '@/services/RequestService';
 
 export const reviewCloser = {
   async closeReviewIfComplete(app: App, threadId: string): Promise<void> {
     const review = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
 
     if (isCompleted(review)) {
       for (const user of review.pendingReviewers) {
-        await expireRequest(app, review, user.userId);
+        await closeRequest(app, review, user.userId);
       }
       await closeReview(
         app,

src/services/__tests__/reviewCloser.test.ts

@@ -8,10 +8,10 @@ import { reviewCloser } from '@/services/ReviewCloser';
 
 jest.mock('@/services/RequestService', () => ({
   ...jest.requireActual('@/services/RequestService'),
-  expireRequest: jest.fn(),
+  closeRequest: jest.fn(),
 }));
 
-import { expireRequest } from '@/services/RequestService';
+import { closeRequest } from '@/services/RequestService';
 
 describe('reviewCloser', () => {
   let app: App;
@@ -131,9 +131,9 @@ describe('reviewCloser', () => {
 
       await reviewCloser.closeReviewIfComplete(app, threadId);
 
-      expect(expireRequest).toHaveBeenCalledTimes(pendingReviewers.length);
+      expect(closeRequest).toHaveBeenCalledTimes(pendingReviewers.length);
       pendingReviewers.forEach(pendingReviewer => {
-        expect(expireRequest).toHaveBeenCalledWith(app, review, pendingReviewer.userId);
+        expect(closeRequest).toHaveBeenCalledWith(app, review, pendingReviewer.userId);
       });
 
       // Verify the review is closed after expiring requests