From 6672ca0109a47b9ab27787ff5eabc9f53591b5eb Mon Sep 17 00:00:00 2001
From: Hongbo Wu <hongbo@omnivore.app>
Date: Thu, 7 Sep 2023 22:21:20 +0800
Subject: [PATCH] fix labels tests

---
 packages/api/src/entity/highlight.ts        |  4 +-
 packages/api/src/resolvers/labels/index.ts  | 64 ++++++++-------------
 packages/api/src/resolvers/rules/index.ts   | 58 ++++---------------
 packages/api/src/services/labels.ts         | 18 ++++++
 packages/api/src/services/rules.ts          | 34 +++++++++--
 packages/api/test/resolvers/article.test.ts |  1 +
 packages/api/test/resolvers/labels.test.ts  | 30 +++++-----
 packages/api/test/resolvers/rules.test.ts   | 40 +++++--------
 8 files changed, 114 insertions(+), 135 deletions(-)

diff --git a/packages/api/src/entity/highlight.ts b/packages/api/src/entity/highlight.ts
index f4bae6d38..8147fa2d5 100644
--- a/packages/api/src/entity/highlight.ts
+++ b/packages/api/src/entity/highlight.ts
@@ -31,7 +31,7 @@ export class Highlight {
   @JoinColumn({ name: 'user_id' })
   user!: User
 
-  @ManyToOne(() => LibraryItem, { onDelete: 'CASCADE', eager: true })
+  @ManyToOne(() => LibraryItem, { onDelete: 'CASCADE' })
   @JoinColumn({ name: 'library_item_id' })
   libraryItem!: LibraryItem
 
@@ -80,7 +80,7 @@ export class Highlight {
   @Column('text', { nullable: true })
   color?: string | null
 
-  @ManyToMany(() => Label, { cascade: true })
+  @ManyToMany(() => Label, { cascade: true, eager: true })
   @JoinTable({
     name: 'entity_labels',
     joinColumn: { name: 'highlight_id' },
diff --git a/packages/api/src/resolvers/labels/index.ts b/packages/api/src/resolvers/labels/index.ts
index ad17b4b3d..4890a5987 100644
--- a/packages/api/src/resolvers/labels/index.ts
+++ b/packages/api/src/resolvers/labels/index.ts
@@ -32,6 +32,7 @@ import {
   findOrCreateLabels,
   saveLabelsInHighlight,
   saveLabelsInLibraryItem,
+  updateLabel,
 } from '../../services/labels'
 import { analytics } from '../../utils/analytics'
 import { authorized } from '../../utils/helpers'
@@ -40,7 +41,7 @@ export const labelsResolver = authorized<LabelsSuccess, LabelsError>(
   async (_obj, _params, { log, authTrx }) => {
     try {
       const labels = await authTrx(async (tx) => {
-        return tx.find(Label, {
+        return tx.withRepository(labelRepository).find({
           order: {
             position: 'ASC',
           },
@@ -96,7 +97,7 @@ export const createLabelResolver = authorized<
   } catch (error) {
     log.error('createLabelResolver', error)
     return {
-      errorCodes: [CreateLabelErrorCode.LabelAlreadyExists],
+      errorCodes: [CreateLabelErrorCode.BadRequest],
     }
   }
 })
@@ -170,7 +171,14 @@ export const setLabelsResolver = authorized<
         labelsSet = await authTrx(async (tx) => {
           return tx.withRepository(labelRepository).findLabelsById(labelIds)
         })
+
+        if (labelsSet.length !== labelIds.length) {
+          return {
+            errorCodes: [SetLabelsErrorCode.NotFound],
+          }
+        }
       }
+
       // save labels in the library item
       await saveLabelsInLibraryItem(labelsSet, pageId, uid, pubsub)
 
@@ -190,7 +198,7 @@ export const setLabelsResolver = authorized<
     } catch (error) {
       log.error('setLabelsResolver error', error)
       return {
-        errorCodes: [SetLabelsErrorCode.NotFound],
+        errorCodes: [SetLabelsErrorCode.BadRequest],
       }
     }
   }
@@ -200,44 +208,18 @@ export const updateLabelResolver = authorized<
   UpdateLabelSuccess,
   UpdateLabelError,
   MutationUpdateLabelArgs
->(
-  async (
-    _,
-    { input: { name, color, description, labelId } },
-    { authTrx, log }
-  ) => {
-    try {
-      log.info('Updating a label', {
-        labels: {
-          source: 'resolver',
-          resolver: 'updateLabelResolver',
-        },
-      })
+>(async (_, { input: { name, color, description, labelId } }, { uid, log }) => {
+  try {
+    const label = await updateLabel(labelId, { name, color, description }, uid)
 
-      const result = await authTrx(async (tx) => {
-        return tx.withRepository(labelRepository).updateLabel(labelId, {
-          name,
-          color,
-          description,
-        })
-      })
-
-      if (!result.affected) {
-        log.error('failed to update')
-        return {
-          errorCodes: [UpdateLabelErrorCode.NotFound],
-        }
-      }
-
-      return { label: result.raw as Label }
-    } catch (error) {
-      log.error('error updating label', error)
-      return {
-        errorCodes: [UpdateLabelErrorCode.BadRequest],
-      }
+    return { label }
+  } catch (error) {
+    log.error('error updating label', error)
+    return {
+      errorCodes: [UpdateLabelErrorCode.BadRequest],
     }
   }
-)
+})
 
 export const setLabelsForHighlightResolver = authorized<
   SetLabelsSuccess,
@@ -291,7 +273,7 @@ export const setLabelsForHighlightResolver = authorized<
   } catch (error) {
     log.error('setLabelsForHighlightResolver error', error)
     return {
-      errorCodes: [SetLabelsErrorCode.NotFound],
+      errorCodes: [SetLabelsErrorCode.BadRequest],
     }
   }
 })
@@ -323,7 +305,7 @@ export const moveLabelResolver = authorized<
     let newPosition = 1
     if (afterLabelId) {
       const afterLabel = await authTrx(async (tx) => {
-        return tx.withRepository(labelRepository).findById(labelId)
+        return tx.withRepository(labelRepository).findById(afterLabelId)
       })
       if (!afterLabel) {
         return {
@@ -356,7 +338,7 @@ export const moveLabelResolver = authorized<
 
       // update the position of the label
       return labelRepo.save({
-        ...label,
+        id: labelId,
         position: newPosition,
       })
     })
diff --git a/packages/api/src/resolvers/rules/index.ts b/packages/api/src/resolvers/rules/index.ts
index c60bee99f..fbc6b7926 100644
--- a/packages/api/src/resolvers/rules/index.ts
+++ b/packages/api/src/resolvers/rules/index.ts
@@ -13,7 +13,7 @@ import {
   SetRuleErrorCode,
   SetRuleSuccess,
 } from '../../generated/graphql'
-import { getRepository } from '../../repository'
+import { deleteRule } from '../../services/rules'
 import { authorized } from '../../utils/helpers'
 
 export const setRuleResolver = authorized<
@@ -46,25 +46,19 @@ export const rulesResolver = authorized<
   RulesSuccess,
   RulesError,
   QueryRulesArgs
->(async (_, { enabled }, { claims, log }) => {
+>(async (_, { enabled }, { authTrx, log }) => {
   try {
-    const rules = await getRepository(Rule).findBy({
-      user: { id: claims.uid },
-      enabled: enabled === null ? undefined : enabled,
-    })
+    const rules = await authTrx((t) =>
+      t.getRepository(Rule).findBy({
+        enabled: enabled === null ? undefined : enabled,
+      })
+    )
 
     return {
       rules,
     }
   } catch (error) {
-    log.error('Error getting rules', {
-      error,
-      labels: {
-        source: 'resolver',
-        resolver: 'rulesResolver',
-        uid: claims.uid,
-      },
-    })
+    log.error('Error getting rules', error)
 
     return {
       errorCodes: [RulesErrorCode.BadRequest],
@@ -76,46 +70,18 @@ export const deleteRuleResolver = authorized<
   DeleteRuleSuccess,
   DeleteRuleError,
   MutationDeleteRuleArgs
->(async (_, { id }, { claims, log }) => {
-  log.info('Deleting rule', {
-    id,
-    labels: {
-      source: 'resolver',
-      resolver: 'deleteRuleResolver',
-      uid: claims.uid,
-    },
-  })
-
+>(async (_, { id }, { uid, log }) => {
   try {
-    const rule = await getRepository(Rule).findOneBy({
-      id,
-      user: { id: claims.uid },
-    })
-    if (!rule) {
-      return {
-        errorCodes: [DeleteRuleErrorCode.NotFound],
-      }
-    }
-
-    await getRepository(Rule).delete({
-      id: rule.id,
-    })
+    const rule = await deleteRule(id, uid)
 
     return {
       rule,
     }
   } catch (error) {
-    log.error('Error deleting rule', {
-      error,
-      labels: {
-        source: 'resolver',
-        resolver: 'deleteRuleResolver',
-        uid: claims.uid,
-      },
-    })
+    log.error('Error deleting rule', error)
 
     return {
-      errorCodes: [DeleteRuleErrorCode.BadRequest],
+      errorCodes: [DeleteRuleErrorCode.NotFound],
     }
   }
 })
diff --git a/packages/api/src/services/labels.ts b/packages/api/src/services/labels.ts
index 85881d27e..fa6eb8dd5 100644
--- a/packages/api/src/services/labels.ts
+++ b/packages/api/src/services/labels.ts
@@ -1,4 +1,5 @@
 import { FindOptionsWhere, In } from 'typeorm'
+import { QueryDeepPartialEntity } from 'typeorm/query-builder/QueryPartialEntity'
 import { EntityLabel } from '../entity/entity_label'
 import { Label } from '../entity/label'
 import { createPubSubClient, EntityType } from '../pubsub'
@@ -172,3 +173,20 @@ export const deleteLabels = async (
     userId
   )
 }
+
+export const updateLabel = async (
+  id: string,
+  label: QueryDeepPartialEntity<Label>,
+  userId: string
+) => {
+  return authTrx(
+    async (t) => {
+      const repo = t.withRepository(labelRepository)
+      await repo.updateLabel(id, label)
+
+      return repo.findOneByOrFail({ id })
+    },
+    undefined,
+    userId
+  )
+}
diff --git a/packages/api/src/services/rules.ts b/packages/api/src/services/rules.ts
index 47975eea4..e33024755 100644
--- a/packages/api/src/services/rules.ts
+++ b/packages/api/src/services/rules.ts
@@ -21,10 +21,34 @@ export const createRule = async (
     return existingRule
   }
 
-  return authTrx((t) =>
-    t.getRepository(Rule).save({
-      ...rule,
-      user: { id: userId },
-    })
+  return authTrx(
+    (t) =>
+      t.getRepository(Rule).save({
+        ...rule,
+        user: { id: userId },
+      }),
+    undefined,
+    userId
+  )
+}
+
+export const deleteRule = async (id: string, userId?: string) => {
+  return authTrx(
+    async (t) => {
+      const repo = t.getRepository(Rule)
+      await repo.delete(id)
+
+      return repo.findOneByOrFail({ id })
+    },
+    undefined,
+    userId
+  )
+}
+
+export const deleteRules = async (userId: string) => {
+  return authTrx(
+    (t) => t.getRepository(Rule).delete({ user: { id: userId } }),
+    undefined,
+    userId
   )
 }
diff --git a/packages/api/test/resolvers/article.test.ts b/packages/api/test/resolvers/article.test.ts
index daac10f6b..c0473180b 100644
--- a/packages/api/test/resolvers/article.test.ts
+++ b/packages/api/test/resolvers/article.test.ts
@@ -816,6 +816,7 @@ describe('Article API', () => {
           shortId: `test shortId${i}`,
           user,
           quote: '<p>search highlight</p>',
+          libraryItem: item,
         }
         const highlight = await createHighlight(
           highlightToSave,
diff --git a/packages/api/test/resolvers/labels.test.ts b/packages/api/test/resolvers/labels.test.ts
index 1bc1a3e74..249c2b6c9 100644
--- a/packages/api/test/resolvers/labels.test.ts
+++ b/packages/api/test/resolvers/labels.test.ts
@@ -10,7 +10,7 @@ import {
   createHighlight,
   findHighlightById,
 } from '../../src/services/highlights'
-import { createLabel, deleteLabels } from '../../src/services/labels'
+import { createLabel, deleteLabels, saveLabelsInHighlight } from '../../src/services/labels'
 import {
   deleteLibraryItemById,
   findLibraryItemById,
@@ -167,11 +167,11 @@ describe('Labels API', () => {
         await deleteLabels([existingLabel.id], user.id)
       })
 
-      it('should return error code LABEL_ALREADY_EXISTS', async () => {
+      it('should return error code BAD_REQUEST', async () => {
         const res = await graphqlRequest(query, authToken).expect(200)
 
         expect(res.body.data.createLabel.errorCodes).to.eql([
-          'LABEL_ALREADY_EXISTS',
+          'BAD_REQUEST',
         ])
       })
     })
@@ -265,16 +265,16 @@ describe('Labels API', () => {
             '#ffffff',
             user.id
           )
-          labelId = toDeleteLabel.id
           const highlight: DeepPartial<Highlight> = {
             id: highlightId,
             patch: 'test patch',
             quote: 'test quote',
             shortId: 'test shortId',
-            labels: [toDeleteLabel],
             user,
+            libraryItem: item,
           }
           await createHighlight(highlight, item.id, user.id)
+          await saveLabelsInHighlight([toDeleteLabel], highlightId, user.id)
         })
 
         after(async () => {
@@ -398,9 +398,9 @@ describe('Labels API', () => {
         labelIds = [labels[0].id, labels[1].id]
       })
 
-      it('should return error code NOT_FOUND', async () => {
+      it('should return error code BAD_REQUEST', async () => {
         const res = await graphqlRequest(query, authToken).expect(200)
-        expect(res.body.data.setLabels.errorCodes).to.eql(['NOT_FOUND'])
+        expect(res.body.data.setLabels.errorCodes).to.eql(['BAD_REQUEST'])
       })
     })
 
@@ -513,9 +513,9 @@ describe('Labels API', () => {
         labelId = generateFakeUuid()
       })
 
-      it('should return error code NOT_FOUND', async () => {
+      it('should return error code BAD_REQUEST', async () => {
         const res = await graphqlRequest(query, authToken).expect(200)
-        expect(res.body.data.updateLabel.errorCodes).to.eql(['NOT_FOUND'])
+        expect(res.body.data.updateLabel.errorCodes).to.eql(['BAD_REQUEST'])
       })
     })
   })
@@ -576,8 +576,9 @@ describe('Labels API', () => {
           id: highlightId,
           patch: 'test patch',
           quote: 'test quote',
-          shortId: generateFakeUuid(),
+          shortId: 'test shortId 2',
           user,
+          libraryItem: item,
         }
         highlightId = (await createHighlight(highlight, item.id, user.id)).id
         labelIds = [labels[0].id, labels[1].id]
@@ -596,8 +597,9 @@ describe('Labels API', () => {
         const highlight: DeepPartial<Highlight> = {
           patch: 'test patch',
           quote: 'test quote',
-          shortId: generateFakeUuid(),
+          shortId: 'test shortId 3',
           user,
+          libraryItem: item,
         }
         highlightId = (await createHighlight(highlight, item.id, user.id)).id
         labelIds = [generateFakeUuid(), generateFakeUuid()]
@@ -613,14 +615,14 @@ describe('Labels API', () => {
 
     context('when highlight not exist', () => {
       before(() => {
-        highlightId = generateFakeUuid()
+        highlightId = 'fake_highlight_id'
         labelIds = [labels[0].id, labels[1].id]
       })
 
-      it('should return error code NOT_FOUND', async () => {
+      it('should return error code BAD_REQUEST', async () => {
         const res = await graphqlRequest(query, authToken).expect(200)
         expect(res.body.data.setLabelsForHighlight.errorCodes).to.eql([
-          'NOT_FOUND',
+          'BAD_REQUEST',
         ])
       })
     })
diff --git a/packages/api/test/resolvers/rules.test.ts b/packages/api/test/resolvers/rules.test.ts
index 1cb96aeef..763e36215 100644
--- a/packages/api/test/resolvers/rules.test.ts
+++ b/packages/api/test/resolvers/rules.test.ts
@@ -2,7 +2,7 @@ import { expect } from 'chai'
 import 'mocha'
 import { Rule, RuleAction, RuleActionType } from '../../src/entity/rule'
 import { User } from '../../src/entity/user'
-import { authTrx, getRepository } from '../../src/repository'
+import { createRule, deleteRules } from '../../src/services/rules'
 import { deleteUser } from '../../src/services/user'
 import { createTestUser } from '../db'
 import { graphqlRequest, request } from '../util'
@@ -72,7 +72,7 @@ describe('Rules Resolver', () => {
     `
 
     after(async () => {
-      await getRepository(Rule).delete({ user: { id: user.id } })
+      await deleteRules(user.id)
     })
 
     it('should set rules', async () => {
@@ -90,22 +90,15 @@ describe('Rules Resolver', () => {
 
   describe('get rules', () => {
     before(async () => {
-      await authTrx(
-        (t) =>
-          t.getRepository(Rule).save({
-            user: { id: user.id },
-            name: 'test rule',
-            filter: 'test filter 2',
-            actions: [{ type: RuleActionType.SendNotification, params: [] }],
-            enabled: true,
-          }),
-        undefined,
-        user.id
-      )
+      await createRule(user.id, {
+        name: 'test rule 2',
+        filter: 'test filter 2',
+        actions: [{ type: RuleActionType.SendNotification, params: [] }],
+      })
     })
 
     after(async () => {
-      await getRepository(Rule).delete({ user: { id: user.id } })
+      await deleteRules(user.id)
     })
 
     const getRulesQuery = (enabled: boolean | null = null) => `
@@ -143,18 +136,11 @@ describe('Rules Resolver', () => {
     let rule: Rule
 
     before(async () => {
-      rule = await authTrx(
-        (t) =>
-          t.getRepository(Rule).save({
-            user: { id: user.id },
-            name: 'test rule',
-            filter: 'test filter 3',
-            actions: [{ type: RuleActionType.SendNotification, params: [] }],
-            enabled: true,
-          }),
-        undefined,
-        user.id
-      )
+      rule = await createRule(user.id, {
+        name: 'test rule 3',
+        filter: 'test filter 3',
+        actions: [{ type: RuleActionType.SendNotification, params: [] }],
+      })
     })
 
     const deleteRulesQuery = (id: string) => `