Merge pull request #3822 from omnivore-app/fix/create-label

fix: LABEL_ALREADY_EXIST bug occured during label creation

packages/api/src/repository/label.ts

@@ -46,9 +46,12 @@ export const labelRepository = appDataSource.getRepository(Label).extend({
     return this.findOneBy({ id })
   },
 
-  findByName(name: string) {
+  findByName(name: string, userId: string) {
     return this.createQueryBuilder()
-      .where('LOWER(name) = LOWER(:name)', { name }) // case insensitive
+      .where('user_id = :userId AND LOWER(name) = LOWER(:name)', {
+        name,
+        userId,
+      }) // case insensitive
       .getOne()
   },
 

packages/api/src/resolvers/highlight/index.ts

@@ -81,7 +81,7 @@ export const mergeHighlightResolver = authorized<
   MergeHighlightSuccess,
   MergeHighlightError,
   MutationMergeHighlightArgs
->(async (_, { input }, { log, pubsub, uid }) => {
+>(async (_, { input }, { authTrx, log, pubsub, uid }) => {
   const { overlapHighlightIdList, ...newHighlightInput } = input
 
   /* Compute merged annotation form the order of highlights appearing on page */
@@ -90,9 +90,10 @@ export const mergeHighlightResolver = authorized<
   const mergedColors: string[] = []
 
   try {
-    const existingHighlights = await highlightRepository.findByLibraryItemId(
-      input.articleId,
-      uid
+    const existingHighlights = await authTrx((tx) =>
+      tx
+        .withRepository(highlightRepository)
+        .findByLibraryItemId(input.articleId, uid)
     )
 
     existingHighlights.forEach((highlight) => {

packages/api/src/resolvers/labels/index.ts

@@ -90,17 +90,22 @@ export const createLabelResolver = authorized<
   CreateLabelError,
   MutationCreateLabelArgs
 >(async (_, { input }, { authTrx, uid }) => {
-  const existingLabel = await labelRepository.findByName(input.name)
-  if (existingLabel) {
+  const label = await authTrx(async (tx) => {
+    const repo = tx.withRepository(labelRepository)
+    const existingLabel = await repo.findByName(input.name, uid)
+    if (existingLabel) {
+      return null
+    }
+
+    return repo.createLabel(input, uid)
+  })
+
+  if (!label) {
     return {
       errorCodes: [CreateLabelErrorCode.LabelAlreadyExists],
     }
   }
 
-  const label = await authTrx(async (tx) =>
-    tx.withRepository(labelRepository).createLabel(input, uid)
-  )
-
   analytics.capture({
     distinctId: uid,
     event: 'label_created',

packages/api/test/resolvers/highlight.test.ts

@@ -1,10 +1,9 @@
+/* eslint-disable @typescript-eslint/restrict-template-expressions */
 import * as chai from 'chai'
 import { expect } from 'chai'
 import chaiString from 'chai-string'
 import 'mocha'
-import { Highlight } from '../../src/entity/highlight'
 import { User } from '../../src/entity/user'
-import { getRepository } from '../../src/repository'
 import {
   createHighlight,
   deleteHighlightById,
@@ -157,7 +156,7 @@ describe('Highlights API', () => {
       .post('/local/debug/fake-user-login')
       .send({ fakeEmail: user.email })
 
-    authToken = res.body.authToken
+    authToken = res.body.authToken as string
     itemId = (await createTestLibraryItem(user.id)).id
   })
 

packages/api/test/resolvers/labels.test.ts

@@ -145,7 +145,7 @@ describe('Labels API', () => {
       })
     })
 
-    context('when name exists', () => {
+    context('when name exists in the user library', () => {
       let existingLabel: Label
 
       before(async () => {
@@ -177,6 +177,32 @@ describe('Labels API', () => {
       })
     })
 
+    context('when name exists in the other user library', () => {
+      let existingLabel: Label
+      let otherUser: User
+
+      before(async () => {
+        otherUser = await createTestUser('otherUser')
+        existingLabel = await createLabel('label3', '#ffffff', otherUser.id)
+      })
+
+      after(async () => {
+        // delete other user will also delete the label
+        await deleteUser(otherUser.id)
+      })
+
+      it('creates the label', async () => {
+        const res = await graphqlRequest(query, authToken, {
+          input: { name: existingLabel.name },
+        }).expect(200)
+        const label = await findLabelById(
+          res.body.data.createLabel.label.id,
+          user.id
+        )
+        expect(label).to.exist
+      })
+    })
+
     it('responds status code 400 when invalid query', async () => {
       const invalidQuery = `
         mutation {

packages/db/migrations/0173.do.alter_labels_table_policy.sql

@@ -0,0 +1,11 @@
+-- Type: DO
+-- Name: alter_labels_table_policy
+-- Description: Alter labels table select policy to check user_id
+
+BEGIN;
+
+ALTER POLICY read_labels ON omnivore.labels
+    TO omnivore_user
+    USING (user_id = omnivore.get_current_user_id());
+
+COMMIT;

packages/db/migrations/0173.undo.alter_labels_table_policy.sql

@@ -0,0 +1,11 @@
+-- Type: UNDO
+-- Name: alter_labels_table_policy
+-- Description: Alter labels table select policy to check user_id
+
+BEGIN;
+
+ALTER POLICY read_labels ON omnivore.labels
+    TO omnivore_user
+    USING (true);
+
+COMMIT;