fix: deletion corner cases

backend/src/services/secret-reminder-recipients/secret-reminder-recipients-dal.ts

@@ -1,3 +1,5 @@
+import { Knex } from "knex";
+
 import { TDbClient } from "@app/db";
 import { TableName } from "@app/db/schemas";
 import { ormify, selectAllTableCols } from "@app/lib/knex";
@@ -7,9 +9,8 @@ export type TSecretReminderRecipientsDALFactory = ReturnType<typeof secretRemind
 export const secretReminderRecipientsDALFactory = (db: TDbClient) => {
   const secretReminderRecipientsOrm = ormify(db, TableName.SecretReminderRecipients);
 
-  const findUsersBySecretId = async (secretId: string) => {
+  const findUsersBySecretId = async (secretId: string, tx?: Knex) => {
-    const res = await db
+    const res = await (tx || db.replicaNode())(TableName.SecretReminderRecipients)
-      .replicaNode()(TableName.SecretReminderRecipients)
       .where({ secretId })
       .leftJoin(TableName.Users, `${TableName.SecretReminderRecipients}.userId`, `${TableName.Users}.id`)
       .leftJoin(TableName.Project, `${TableName.SecretReminderRecipients}.projectId`, `${TableName.Project}.id`)

backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts

@@ -346,7 +346,7 @@ export const fnSecretBulkDelete = async ({
     deletedSecrets
       .filter(({ reminderRepeatDays }) => Boolean(reminderRepeatDays))
       .map(({ id, reminderRepeatDays }) =>
-        secretQueueService.removeSecretReminder({ secretId: id, repeatDays: reminderRepeatDays as number })
+        secretQueueService.removeSecretReminder({ secretId: id, repeatDays: reminderRepeatDays as number }, tx)
       )
   );
 

backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts

@@ -221,7 +221,7 @@ export type TFnSecretBulkDelete = {
   tx?: Knex;
   secretDAL: Pick<TSecretV2BridgeDALFactory, "deleteMany">;
   secretQueueService: {
-    removeSecretReminder: (data: TRemoveSecretReminderDTO) => Promise<void>;
+    removeSecretReminder: (data: TRemoveSecretReminderDTO, tx?: Knex) => Promise<void>;
   };
 };
 

backend/src/services/secret/secret-fns.ts

@@ -737,7 +737,7 @@ export const fnSecretBulkDelete = async ({
     deletedSecrets
       .filter(({ secretReminderRepeatDays }) => Boolean(secretReminderRepeatDays))
       .map(({ id, secretReminderRepeatDays }) =>
-        secretQueueService.removeSecretReminder({ secretId: id, repeatDays: secretReminderRepeatDays as number })
+        secretQueueService.removeSecretReminder({ secretId: id, repeatDays: secretReminderRepeatDays as number }, tx)
       )
   );
 

backend/src/services/secret/secret-queue.ts

@@ -1,6 +1,7 @@
 /* eslint-disable no-await-in-loop */
 import opentelemetry from "@opentelemetry/api";
 import { AxiosError } from "axios";
+import { Knex } from "knex";
 
 import {
   ProjectMembershipRole,
@@ -185,8 +186,10 @@ export const secretQueueFactory = ({
     unit: "1"
   });
 
-  const removeSecretReminder = async (dto: TRemoveSecretReminderDTO) => {
+  const removeSecretReminder = async ({ deleteRecipients = true, ...dto }: TRemoveSecretReminderDTO, tx?: Knex) => {
-    await secretReminderRecipientsDAL.delete({ secretId: dto.secretId });
+    if (deleteRecipients) {
+      await secretReminderRecipientsDAL.delete({ secretId: dto.secretId }, tx);
+    }
 
     const appCfg = getConfig();
     await queueService.stopRepeatableJob(
@@ -233,7 +236,12 @@ export const secretQueueFactory = ({
       .replace(":", "-");
   };
 
-  const addSecretReminder = async ({ oldSecret, newSecret, projectId, recipients }: TCreateSecretReminderDTO) => {
+  const addSecretReminder = async ({
+    oldSecret,
+    newSecret,
+    projectId,
+    deleteRecipients = true
+  }: TCreateSecretReminderDTO) => {
     try {
       const appCfg = getConfig();
 
@@ -255,28 +263,11 @@ export const secretQueueFactory = ({
       if (oldSecret.secretReminderRepeatDays) {
         await removeSecretReminder({
           repeatDays: oldSecret.secretReminderRepeatDays,
-          secretId: oldSecret.id
+          secretId: oldSecret.id,
+          deleteRecipients
         });
       }
 
-      const existingRecipients = await secretReminderRecipientsDAL.findUsersBySecretId(newSecret.id);
-
-      if (recipients?.length) {
-        await secretReminderRecipientsDAL.transaction(async (tx) => {
-          await secretReminderRecipientsDAL.delete({ secretId: newSecret.id }, tx);
-          await secretReminderRecipientsDAL.insertMany(
-            recipients.map((r) => ({
-              secretId: newSecret.id,
-              userId: r,
-              projectId
-            })),
-            tx
-          );
-        });
-      } else if (existingRecipients) {
-        await secretReminderRecipientsDAL.delete({ secretId: newSecret.id });
-      }
-
       await queueService.queue(
         QueueName.SecretReminder,
         QueueJobs.SecretReminder,
@@ -315,33 +306,52 @@ export const secretQueueFactory = ({
     const recipientsUpdated =
       secretReminderRecipients?.some(
         (newId) => !oldSecret.secretReminderRecipients?.find((oldId) => newId === oldId)
-      ) ||
+      ) || secretReminderRecipients?.length !== oldSecret.secretReminderRecipients?.length;
-      (secretReminderRecipients?.length !== oldSecret.secretReminderRecipients?.length &&
-        secretReminderRepeatDays !== null);
 
-    if (newSecret.type !== SecretType.Personal && secretReminderRepeatDays !== undefined) {
+    await secretReminderRecipientsDAL.transaction(async (tx) => {
-      if (
+      if (newSecret.type !== SecretType.Personal && secretReminderRepeatDays !== undefined) {
-        (secretReminderRepeatDays && oldSecret.secretReminderRepeatDays !== secretReminderRepeatDays) ||
+        if (
-        (secretReminderNote && oldSecret.secretReminderNote !== secretReminderNote) ||
+          (secretReminderRepeatDays && oldSecret.secretReminderRepeatDays !== secretReminderRepeatDays) ||
-        recipientsUpdated
+          (secretReminderNote && oldSecret.secretReminderNote !== secretReminderNote)
-      ) {
+        ) {
-        await addSecretReminder({
+          await addSecretReminder({
-          oldSecret,
+            oldSecret,
-          newSecret,
+            newSecret,
-          projectId,
+            projectId,
-          recipients: newSecret.secretReminderRecipients
+            deleteRecipients: false
-        });
+          });
-      } else if (
+        } else if (
-        secretReminderRepeatDays === null &&
+          secretReminderRepeatDays === null &&
-        secretReminderNote === null &&
+          secretReminderNote === null &&
-        oldSecret.secretReminderRepeatDays
+          oldSecret.secretReminderRepeatDays
-      ) {
+        ) {
-        await removeSecretReminder({
+          await removeSecretReminder({
-          secretId: oldSecret.id,
+            secretId: oldSecret.id,
-          repeatDays: oldSecret.secretReminderRepeatDays
+            repeatDays: oldSecret.secretReminderRepeatDays
-        });
+          });
+        }
       }
-    }
+
+      if (recipientsUpdated) {
+        // if no recipients, delete all existing recipients
+        if (!secretReminderRecipients?.length) {
+          const existingRecipients = await secretReminderRecipientsDAL.findUsersBySecretId(newSecret.id, tx);
+          if (existingRecipients) {
+            await secretReminderRecipientsDAL.delete({ secretId: newSecret.id }, tx);
+          }
+        } else {
+          await secretReminderRecipientsDAL.delete({ secretId: newSecret.id }, tx);
+          await secretReminderRecipientsDAL.insertMany(
+            secretReminderRecipients.map((r) => ({
+              secretId: newSecret.id,
+              userId: r,
+              projectId
+            })),
+            tx
+          );
+        }
+      }
+    });
   };
   const createManySecretsRawFn = createManySecretsRawFnFactory({
     projectDAL,

backend/src/services/secret/secret-service.ts

@@ -546,10 +546,13 @@ export const secretServiceFactory = ({
 
       for await (const secret of secrets) {
         if (secret.secretReminderRepeatDays !== null && secret.secretReminderRepeatDays !== undefined) {
-          await secretQueueService.removeSecretReminder({
+          await secretQueueService.removeSecretReminder(
-            repeatDays: secret.secretReminderRepeatDays,
+            {
-            secretId: secret.id
+              repeatDays: secret.secretReminderRepeatDays,
-          });
+              secretId: secret.id
+            },
+            tx
+          );
         }
       }
 
@@ -1074,10 +1077,13 @@ export const secretServiceFactory = ({
 
       for await (const secret of secrets) {
         if (secret.secretReminderRepeatDays !== null && secret.secretReminderRepeatDays !== undefined) {
-          await secretQueueService.removeSecretReminder({
+          await secretQueueService.removeSecretReminder(
-            repeatDays: secret.secretReminderRepeatDays,
+            {
-            secretId: secret.id
+              repeatDays: secret.secretReminderRepeatDays,
-          });
+              secretId: secret.id
+            },
+            tx
+          );
         }
       }
       const secretValueHidden = !hasSecretReadValueOrDescribePermission(

backend/src/services/secret/secret-types.ts

@@ -379,7 +379,7 @@ export type TFnSecretBulkDelete = {
   tx?: Knex;
   secretDAL: Pick<TSecretDALFactory, "deleteMany">;
   secretQueueService: {
-    removeSecretReminder: (data: TRemoveSecretReminderDTO) => Promise<void>;
+    removeSecretReminder: (data: TRemoveSecretReminderDTO, tx?: Knex) => Promise<void>;
   };
 };
 
@@ -410,12 +410,14 @@ export type TCreateSecretReminderDTO = {
   oldSecret: TPartialSecret;
   newSecret: TPartialSecret;
   projectId: string;
-  recipients?: string[] | null;
+
+  deleteRecipients?: boolean;
 };
 
 export type TRemoveSecretReminderDTO = {
   secretId: string;
   repeatDays: number;
+  deleteRecipients?: boolean;
 };
 
 export type TBackFillSecretReferencesDTO = TProjectPermission;

frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/CreateReminderForm.tsx

@@ -87,6 +87,15 @@ export const CreateReminderForm = ({
     }
   }, [members, isOpen, recipients]);
 
+  useEffect(() => {
+    if (repeatDays) {
+      setValue("days", repeatDays);
+    }
+    if (note) {
+      setValue("note", note);
+    }
+  }, [repeatDays, note]);
+
   return (
     <Modal isOpen={isOpen} onOpenChange={onOpenChange}>
       <ModalContent