Merge pull request #655 from omnivore-app/fix/update-label

Fix not updating labels in elastic after updating in postgres

packages/api/src/elastic/labels.ts

@@ -136,9 +136,8 @@ export const deleteLabelInPages = async (
       refresh: ctx.refresh,
     })
 
-    if (body.updated === 0) return false
-
-    await ctx.pubsub.entityDeleted(EntityType.LABEL, label, ctx.uid)
+    body.updated > 0 &&
+      (await ctx.pubsub.entityDeleted(EntityType.LABEL, label, ctx.uid))
 
     return true
   } catch (e) {
@@ -146,3 +145,56 @@ export const deleteLabelInPages = async (
     return false
   }
 }
+
+export const updateLabelInPage = async (
+  label: Label,
+  ctx: PageContext
+): Promise<boolean> => {
+  try {
+    const { body } = await client.updateByQuery({
+      index: INDEX_ALIAS,
+      body: {
+        script: {
+          source: `ctx._source.labels.removeIf(l -> l.id == params.label.id);
+                   ctx._source.labels.add(params.label)`,
+          lang: 'painless',
+          params: {
+            label: label,
+          },
+        },
+        query: {
+          bool: {
+            filter: [
+              {
+                term: {
+                  userId: ctx.uid,
+                },
+              },
+              {
+                nested: {
+                  path: 'labels',
+                  query: {
+                    term: {
+                      'labels.id': label.id,
+                    },
+                  },
+                },
+              },
+            ],
+          },
+        },
+      },
+      refresh: ctx.refresh,
+      conflicts: 'proceed', // ignore conflicts
+    })
+
+    body.updated > 0 &&
+      (await ctx.pubsub.entityUpdated(EntityType.LABEL, label, ctx.uid))
+
+    return true
+  } catch (e) {
+    console.error('failed to update label in elastic', e)
+
+    return false
+  }
+}

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

@@ -29,7 +29,11 @@ import { getRepository, setClaims } from '../../entity/utils'
 import { createPubSubClient } from '../../datalayer/pubsub'
 import { AppDataSource } from '../../server'
 import { getPageById } from '../../elastic/pages'
-import { deleteLabelInPages, updateLabelsInPage } from '../../elastic/labels'
+import {
+  deleteLabelInPages,
+  updateLabelInPage,
+  updateLabelsInPage,
+} from '../../elastic/labels'
 
 export const labelsResolver = authorized<LabelsSuccess, LabelsError>(
   async (_obj, _params, { claims: { uid }, log }) => {
@@ -222,7 +226,7 @@ export const setLabelsResolver = authorized<
 
     const labels = await getRepository(Label).find({
       where: { id: In(labelIds), user: { id: user.id } },
-      relations: ['user'],
+      select: ['id', 'name', 'color', 'description', 'createdAt'],
     })
     if (labels.length !== labelIds.length) {
       return {
@@ -266,7 +270,7 @@ export const updateLabelResolver = authorized<
   UpdateLabelSuccess,
   UpdateLabelError,
   MutationUpdateLabelArgs
->(async (_, { input }, { claims: { uid }, log }) => {
+>(async (_, { input }, { claims: { uid }, log, pubsub }) => {
   log.info('updateLabelResolver')
 
   try {
@@ -278,9 +282,9 @@ export const updateLabelResolver = authorized<
       }
     }
 
-    const label = await getRepository(Label).findOneBy({
-      id: labelId,
-      user: { id: uid },
+    const label = await getRepository(Label).findOne({
+      where: { id: labelId, user: { id: uid } },
+      select: ['id', 'name', 'color', 'description', 'createdAt'],
     })
     if (!label) {
       return {
@@ -308,7 +312,17 @@ export const updateLabelResolver = authorized<
     if (!result.affected) {
       log.error('failed to update')
       return {
-        errorCodes: [UpdateLabelErrorCode.NotFound],
+        errorCodes: [UpdateLabelErrorCode.BadRequest],
+      }
+    }
+
+    const updated = await updateLabelInPage(label, {
+      pubsub,
+      uid,
+    })
+    if (!updated) {
+      return {
+        errorCodes: [UpdateLabelErrorCode.BadRequest],
       }
     }
 

packages/api/src/services/labels.ts

@@ -54,5 +54,15 @@ export const addLabelToPage = async (
 
   console.log('adding label to page', label.name, pageId)
 
-  return addLabelInPage(pageId, labelEntity, ctx)
+  return addLabelInPage(
+    pageId,
+    {
+      id: labelEntity.id,
+      name: labelEntity.name,
+      color: labelEntity.color,
+      description: labelEntity.description,
+      createdAt: labelEntity.createdAt,
+    },
+    ctx
+  )
 }

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

@@ -9,9 +9,11 @@ import { Label } from '../../src/entity/label'
 import { expect } from 'chai'
 import 'mocha'
 import { User } from '../../src/entity/user'
-import { Page } from '../../src/elastic/types'
+import { Page, PageContext } from '../../src/elastic/types'
 import { getRepository } from '../../src/entity/utils'
 import { getPageById } from '../../src/elastic/pages'
+import { addLabelInPage } from '../../src/elastic/labels'
+import { createPubSubClient } from '../../src/datalayer/pubsub'
 
 describe('Labels API', () => {
   const username = 'fakeUser'
@@ -20,6 +22,7 @@ describe('Labels API', () => {
   let authToken: string
   let page: Page
   let labels: Label[]
+  let ctx: PageContext
 
   before(async () => {
     // create test user and login
@@ -41,7 +44,19 @@ describe('Labels API', () => {
       'different_label',
       '#dddddd'
     )
-    page = await createTestElasticPage(user, [existingLabelOfLink])
+    page = await createTestElasticPage(user, [
+      {
+        id: existingLabelOfLink.id,
+        name: existingLabelOfLink.name,
+        color: existingLabelOfLink.color,
+      },
+    ])
+
+    ctx = {
+      pubsub: createPubSubClient(),
+      refresh: true,
+      uid: user.id,
+    }
   })
 
   after(async () => {
@@ -198,8 +213,10 @@ describe('Labels API', () => {
     })
 
     context('when label exists', () => {
+      let toDeleteLabel: Label
+
       before(async () => {
-        const toDeleteLabel = await createTestLabel(user, 'label4', '#ffffff')
+        toDeleteLabel = await createTestLabel(user, 'label4', '#ffffff')
         labelId = toDeleteLabel.id
       })
 
@@ -208,6 +225,19 @@ describe('Labels API', () => {
         const label = await getRepository(Label).findOneBy({ id: labelId })
         expect(label).to.not.exist
       })
+
+      context('when a page has this label', () => {
+        before(async () => {
+          await addLabelInPage(page.id, toDeleteLabel, ctx)
+        })
+
+        it('should update page', async () => {
+          await graphqlRequest(query, authToken).expect(200)
+          const updatedPage = await getPageById(page.id)
+
+          expect(updatedPage?.labels).not.to.include(toDeleteLabel)
+        })
+      })
     })
 
     context('when label not exist', () => {
@@ -293,7 +323,7 @@ describe('Labels API', () => {
       })
     })
 
-    context('when link not exist', () => {
+    context('when page not exist', () => {
       before(() => {
         pageId = generateFakeUuid()
         labelIds = [labels[0].id, labels[1].id]
@@ -319,4 +349,95 @@ describe('Labels API', () => {
       return graphqlRequest(query, invalidAuthToken).expect(500)
     })
   })
+
+  describe('Update label', () => {
+    let query: string
+    let labelId: string
+    let name: string
+    let color: string
+
+    beforeEach(() => {
+      query = `
+        mutation {
+          updateLabel(
+            input: {
+              labelId: "${labelId}",
+              name: "${name}",
+              color: "${color}"
+            }
+          ) {
+            ... on UpdateLabelSuccess {
+              label {
+                id
+                name
+                color
+              }
+            }
+            ... on UpdateLabelError {
+              errorCodes
+            }
+          }
+        }
+      `
+    })
+
+    context('when labels exists', () => {
+      let toUpdateLabel: Label
+
+      before(async () => {
+        toUpdateLabel = await createTestLabel(user, 'label5', '#ffffff')
+        labelId = toUpdateLabel.id
+        name = 'Updated label'
+        color = '#aabbcc'
+      })
+
+      it('should return the updated label', async () => {
+        const res = await graphqlRequest(query, authToken).expect(200)
+        expect(res.body.data.updateLabel.label).to.eql({
+          id: labelId,
+          name,
+          color,
+        })
+      })
+
+      it('should update the label in db', async () => {
+        await graphqlRequest(query, authToken).expect(200)
+        const updatedLabel = await getRepository(Label).findOne({
+          where: { id: labelId },
+        })
+
+        expect(updatedLabel?.name).to.eql(name)
+        expect(updatedLabel?.color).to.eql(color)
+      })
+
+      context('when a page has the label', () => {
+        before(async () => {
+          await addLabelInPage(page.id, toUpdateLabel, ctx)
+        })
+
+        it('should update the page with the label', async () => {
+          await graphqlRequest(query, authToken).expect(200)
+
+          const updatedPage = await getPageById(page.id)
+          const updatedLabel = updatedPage?.labels?.filter(
+            (l) => l.id === labelId
+          )?.[0]
+
+          expect(updatedLabel?.name).to.eql(name)
+          expect(updatedLabel?.color).to.eql(color)
+        })
+      })
+    })
+
+    context('when labels not exist', () => {
+      before(() => {
+        labelId = generateFakeUuid()
+      })
+
+      it('should return error code NOT_FOUND', async () => {
+        const res = await graphqlRequest(query, authToken).expect(200)
+        expect(res.body.data.updateLabel.errorCodes).to.eql(['NOT_FOUND'])
+      })
+    })
+  })
 })

packages/api/test/services/save_newsletter_email.test.ts

@@ -44,16 +44,14 @@ describe('saveNewsletterEmail', () => {
       ctx
     )
 
-    setTimeout(async () => {
-      const page = await getPageByParam({ userId: user.id })
-      if (!page) {
-        expect.fail('page not found')
-      }
-      expect(page.url).to.equal('https://example.com')
-      expect(page.title).to.equal('fake title')
-      expect(page.author).to.equal('fake author')
-      expect(page.content).to.contain(fakeContent)
-    })
+    const page = await getPageByParam({ userId: user.id })
+    if (!page) {
+      expect.fail('page not found')
+    }
+    expect(page.url).to.equal('https://example.com')
+    expect(page.title).to.equal('fake title')
+    expect(page.author).to.equal('fake author')
+    expect(page.content).to.contain(fakeContent)
   })
 
   it('should adds a Newsletter label to that page', async () => {
@@ -73,9 +71,7 @@ describe('saveNewsletterEmail', () => {
       ctx
     )
 
-    setTimeout(async () => {
-      const page = await getPageByParam({ userId: user.id })
-      expect(page?.labels).to.deep.include(newLabel)
-    })
+    const page = await getPageByParam({ userId: user.id })
+    expect(page?.labels?.[0]).to.deep.include(newLabel)
   })
 })

packages/api/test/util.ts

@@ -2,10 +2,9 @@ import { createApp } from '../src/server'
 import supertest from 'supertest'
 import { v4 } from 'uuid'
 import { corsConfig } from '../src/utils/corsConfig'
-import { ArticleSavingRequestStatus, Page } from '../src/elastic/types'
+import { ArticleSavingRequestStatus, Label, Page } from '../src/elastic/types'
 import { PageType } from '../src/generated/graphql'
 import { User } from '../src/entity/user'
-import { Label } from '../src/entity/label'
 import { createPubSubClient } from '../src/datalayer/pubsub'
 import { createPage, getPageById } from '../src/elastic/pages'
 

packages/db/elastic_migrations/index_settings.json

@@ -50,9 +50,21 @@
       "labels": {
         "type": "nested",
         "properties": {
+          "id": {
+            "type": "keyword"
+          },
           "name": {
             "type": "keyword",
             "normalizer": "lowercase_normalizer"
+          },
+          "color": {
+            "type": "keyword"
+          },
+          "description": {
+            "type": "text"
+          },
+          "createdAt": {
+            "type": "date"
           }
         }
       },