handle scenario accept comes through twice and the review no longer exists

Author: kylehoehns

src/database/repos/activeReviewsRepo.ts

@@ -95,6 +95,14 @@ export const activeReviewRepo = {
     return mapRowToActiveReview(row);
   },
 
+  /**
+   * @returns the review with the given threadId, or undefined if not found
+   */
+  async getReviewByThreadIdOrUndefined(threadId: string): Promise<ActiveReview | undefined> {
+    const row = await this.getRowByThreadId(threadId);
+    return row ? mapRowToActiveReview(row) : undefined;
+  },
+
   /**
    * Creates a new active review
    * @returns The resulting active review

src/services/ReviewCloser.ts

@@ -5,10 +5,20 @@ import { App } from '@slack/bolt';
 import { chatService } from '@/services/ChatService';
 import { ActiveReview } from '@models/ActiveReview';
 import { closeRequest } from '@/services/RequestService';
+import log from '@/utils/log';
 
 export const reviewCloser = {
   async closeReviewIfComplete(app: App, threadId: string): Promise<void> {
-    const review = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
+    const review = await activeReviewRepo.getReviewByThreadIdOrUndefined(threadId);
+
+    // Review may have already been closed by another concurrent accept/decline
+    if (!review) {
+      log.d(
+        'reviewCloser.closeReviewIfComplete',
+        `Review ${threadId} not found - likely already closed by concurrent action`,
+      );
+      return;
+    }
 
     if (isCompleted(review)) {
       for (const user of review.pendingReviewers) {

src/services/__tests__/reviewCloser.test.ts

@@ -38,7 +38,7 @@ describe('reviewCloser', () => {
         pendingReviewers: [],
         pdfIdentifier: '',
       };
-      activeReviewRepo.getReviewByThreadIdOrFail = jest.fn().mockResolvedValue(review);
+      activeReviewRepo.getReviewByThreadIdOrUndefined = jest.fn().mockResolvedValue(review);
 
       await reviewCloser.closeReviewIfComplete(app, threadId);
 
@@ -71,7 +71,7 @@ describe('reviewCloser', () => {
         pendingReviewers: [],
         pdfIdentifier: '',
       };
-      activeReviewRepo.getReviewByThreadIdOrFail = jest.fn().mockResolvedValue(review);
+      activeReviewRepo.getReviewByThreadIdOrUndefined = jest.fn().mockResolvedValue(review);
 
       await reviewCloser.closeReviewIfComplete(app, threadId);
 
@@ -98,7 +98,7 @@ describe('reviewCloser', () => {
         pendingReviewers: [{ userId: '123', expiresAt: 1, messageTimestamp: '456' }],
         pdfIdentifier: '',
       };
-      activeReviewRepo.getReviewByThreadIdOrFail = jest.fn().mockResolvedValue(review);
+      activeReviewRepo.getReviewByThreadIdOrUndefined = jest.fn().mockResolvedValue(review);
 
       await reviewCloser.closeReviewIfComplete(app, threadId);
 
@@ -127,7 +127,7 @@ describe('reviewCloser', () => {
         pdfIdentifier: '',
       };
 
-      activeReviewRepo.getReviewByThreadIdOrFail = jest.fn().mockResolvedValue(review);
+      activeReviewRepo.getReviewByThreadIdOrUndefined = jest.fn().mockResolvedValue(review);
 
       await reviewCloser.closeReviewIfComplete(app, threadId);
 
@@ -144,6 +144,17 @@ describe('reviewCloser', () => {
       );
       expect(activeReviewRepo.remove).toHaveBeenCalledWith(threadId);
     });
+
+    it('should gracefully handle when review has already been closed by concurrent action', async () => {
+      const threadId = '111';
+      activeReviewRepo.getReviewByThreadIdOrUndefined = jest.fn().mockResolvedValue(undefined);
+
+      await reviewCloser.closeReviewIfComplete(app, threadId);
+
+      expect(chatService.replyToReviewThread).not.toHaveBeenCalled();
+      expect(activeReviewRepo.remove).not.toHaveBeenCalled();
+      expect(closeRequest).not.toHaveBeenCalled();
+    });
   });
 });