Fix history label creation for anonymous users (#20200)

* Remove decaffeination artifacts in LabelsTests * Remove decaffeination artifacts in LabelsManagerTests * Fix label creation for anonymous users * Update label creation route in MockProjectHistoryApi tests * Support both endpoints for backwards compatibility GitOrigin-RevId: 50ce1ba49388e50f147fb620e0425fea83301c9d
2024-11-21 20:47:08 -05:00 · 2024-10-01 14:36:38 +02:00 · 2024-10-01 14:36:38 +02:00 · cd6631c105
commit cd6631c105
parent 3ff142d478
9 changed files with 170 additions and 131 deletions
--- a/services/project-history/app/js/HttpController.js
+++ b/services/project-history/app/js/HttpController.js
@ -455,13 +455,19 @@ export function getLabels(req, res, next) {
 }

 export function createLabel(req, res, next) {
-  const { project_id: projectId, user_id: userId } = req.params
+  const { project_id: projectId, user_id: userIdParam } = req.params
  const {
    version,
    comment,
+    user_id: userIdBody,
    created_at: createdAt,
    validate_exists: validateExists,
  } = req.body
+
+  // Temporarily looking up both params and body while rolling out changes
+  // in the router path - https://github.com/overleaf/internal/pull/20200
+  const userId = userIdParam || userIdBody
+
  HistoryApiManager.shouldUseProjectHistory(
    projectId,
    (error, shouldUseProjectHistory) => {
--- a/services/project-history/app/js/LabelsManager.js
+++ b/services/project-history/app/js/LabelsManager.js
@ -53,9 +53,11 @@ export function createLabel(
        project_id: new ObjectId(projectId),
        comment,
        version,
-        user_id: new ObjectId(userId),
        created_at: createdAt,
      }
+      if (userId) {
+        label.user_id = userId
+      }
      db.projectHistoryLabels.insertOne(label, function (error, confirmation) {
        if (error) {
          return callback(OError.tag(error))
@ -125,7 +127,13 @@ function _toObjectId(...args1) {
  const args = args1.slice(0, adjustedLength - 1)
  const callback = args1[adjustedLength - 1]
  try {
-    const ids = args.map(id => new ObjectId(id))
+    const ids = args.map(id => {
+      if (id) {
+        return new ObjectId(id)
+      } else {
+        return undefined
+      }
+    })
    callback(null, ...ids)
  } catch (error) {
    callback(error)
--- a/services/project-history/app/js/Router.js
+++ b/services/project-history/app/js/Router.js
@ -125,6 +125,23 @@ export function initialize(app) {

  app.get('/project/:project_id/labels', HttpController.getLabels)

+  app.post(
+    '/project/:project_id/labels',
+    validate({
+      body: {
+        version: Joi.number().integer().required(),
+        comment: Joi.string().required(),
+        created_at: Joi.string(),
+        validate_exists: Joi.boolean().default(true),
+        user_id: Joi.string(),
+      },
+    }),
+
+    HttpController.createLabel
+  )
+
+  // Temporarily maintaining both paths for createLabel to support backwards
+  // compatibility while rolling out - https://github.com/overleaf/internal/pull/20200
  app.post(
    '/project/:project_id/user/:user_id/labels',
    validate({
--- a/services/project-history/test/acceptance/js/LabelsTests.js
+++ b/services/project-history/test/acceptance/js/LabelsTests.js
@ -1,18 +1,4 @@
-/* eslint-disable
-    no-undef,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-import sinon from 'sinon'
 import { expect } from 'chai'
-import Settings from '@overleaf/settings'
 import mongodb from 'mongodb-legacy'
 import nock from 'nock'
 import * as ProjectHistoryClient from './helpers/ProjectHistoryClient.js'
@ -20,14 +6,13 @@ import * as ProjectHistoryApp from './helpers/ProjectHistoryApp.js'
 const { ObjectId } = mongodb

 const MockHistoryStore = () => nock('http://127.0.0.1:3100')
-const MockFileStore = () => nock('http://127.0.0.1:3009')
 const MockWeb = () => nock('http://127.0.0.1:3000')

 const fixture = path => new URL(`../fixtures/${path}`, import.meta.url)

 describe('Labels', function () {
  beforeEach(function (done) {
-    return ProjectHistoryApp.ensureRunning(error => {
+    ProjectHistoryApp.ensureRunning(error => {
      if (error != null) {
        throw error
      }
@ -37,7 +22,7 @@ describe('Labels', function () {
        projectId: this.historyId,
      })

-      return ProjectHistoryClient.initializeProject(
+      ProjectHistoryClient.initializeProject(
        this.historyId,
        (error, olProject) => {
          if (error != null) {
@ -68,18 +53,18 @@ describe('Labels', function () {
          this.comment2 = 'another saved version comment'
          this.user_id = new ObjectId().toString()
          this.created_at = new Date(1)
-          return done()
+          done()
        }
      )
    })
  })

  afterEach(function () {
-    return nock.cleanAll()
+    nock.cleanAll()
  })

  it('can create and get labels', function (done) {
-    return ProjectHistoryClient.createLabel(
+    ProjectHistoryClient.createLabel(
      this.project_id,
      this.user_id,
      7,
@ -89,22 +74,42 @@ describe('Labels', function () {
        if (error != null) {
          throw error
        }
-        return ProjectHistoryClient.getLabels(
-          this.project_id,
-          (error, labels) => {
+        ProjectHistoryClient.getLabels(this.project_id, (error, labels) => {
          if (error != null) {
            throw error
          }
          expect(labels).to.deep.equal([label])
-            return done()
+          done()
+        })
      }
    )
+  })
+
+  it('can create and get labels with no user id', function (done) {
+    const userId = undefined
+    ProjectHistoryClient.createLabel(
+      this.project_id,
+      userId,
+      7,
+      this.comment,
+      this.created_at,
+      (error, label) => {
+        if (error != null) {
+          throw error
+        }
+        ProjectHistoryClient.getLabels(this.project_id, (error, labels) => {
+          if (error != null) {
+            throw error
+          }
+          expect(labels).to.deep.equal([label])
+          done()
+        })
      }
    )
  })

  it('can delete labels', function (done) {
-    return ProjectHistoryClient.createLabel(
+    ProjectHistoryClient.createLabel(
      this.project_id,
      this.user_id,
      7,
@ -114,31 +119,24 @@ describe('Labels', function () {
        if (error != null) {
          throw error
        }
-        return ProjectHistoryClient.deleteLabel(
-          this.project_id,
-          label.id,
-          error => {
+        ProjectHistoryClient.deleteLabel(this.project_id, label.id, error => {
          if (error != null) {
            throw error
          }
-            return ProjectHistoryClient.getLabels(
-              this.project_id,
-              (error, labels) => {
+          ProjectHistoryClient.getLabels(this.project_id, (error, labels) => {
            if (error != null) {
              throw error
            }
            expect(labels).to.deep.equal([])
-                return done()
-              }
-            )
-          }
-        )
+            done()
+          })
+        })
      }
    )
  })

  it('can delete labels for the current user', function (done) {
-    return ProjectHistoryClient.createLabel(
+    ProjectHistoryClient.createLabel(
      this.project_id,
      this.user_id,
      7,
@ -148,7 +146,7 @@ describe('Labels', function () {
        if (error != null) {
          throw error
        }
-        return ProjectHistoryClient.deleteLabelForUser(
+        ProjectHistoryClient.deleteLabelForUser(
          this.project_id,
          this.user_id,
          label.id,
@ -156,16 +154,13 @@ describe('Labels', function () {
            if (error != null) {
              throw error
            }
-            return ProjectHistoryClient.getLabels(
-              this.project_id,
-              (error, labels) => {
+            ProjectHistoryClient.getLabels(this.project_id, (error, labels) => {
              if (error != null) {
                throw error
              }
              expect(labels).to.deep.equal([])
-                return done()
-              }
-            )
+              done()
+            })
          }
        )
      }
@ -175,7 +170,7 @@ describe('Labels', function () {
  it('can transfer ownership of labels', function (done) {
    const fromUser = new ObjectId().toString()
    const toUser = new ObjectId().toString()
-    return ProjectHistoryClient.createLabel(
+    ProjectHistoryClient.createLabel(
      this.project_id,
      fromUser,
      7,
@ -185,7 +180,7 @@ describe('Labels', function () {
        if (error != null) {
          throw error
        }
-        return ProjectHistoryClient.createLabel(
+        ProjectHistoryClient.createLabel(
          this.project_id,
          fromUser,
          7,
@ -195,14 +190,14 @@ describe('Labels', function () {
            if (error != null) {
              throw error
            }
-            return ProjectHistoryClient.transferLabelOwnership(
+            ProjectHistoryClient.transferLabelOwnership(
              fromUser,
              toUser,
              error => {
                if (error != null) {
                  throw error
                }
-                return ProjectHistoryClient.getLabels(
+                ProjectHistoryClient.getLabels(
                  this.project_id,
                  (error, labels) => {
                    if (error != null) {
@ -224,7 +219,7 @@ describe('Labels', function () {
                        user_id: toUser,
                      },
                    ])
-                    return done()
+                    done()
                  }
                )
              }
@ -235,8 +230,8 @@ describe('Labels', function () {
    )
  })

-  return it('should return labels with summarized updates', function (done) {
-    return ProjectHistoryClient.createLabel(
+  it('should return labels with summarized updates', function (done) {
+    ProjectHistoryClient.createLabel(
      this.project_id,
      this.user_id,
      8,
@ -246,7 +241,7 @@ describe('Labels', function () {
        if (error != null) {
          throw error
        }
-        return ProjectHistoryClient.getSummarizedUpdates(
+        ProjectHistoryClient.getSummarizedUpdates(
          this.project_id,
          { min_count: 1 },
          (error, updates) => {
@ -278,7 +273,7 @@ describe('Labels', function () {
                },
              ],
            })
-            return done()
+            done()
          }
        )
      }
--- a/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js
+++ b/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js
@ -259,8 +259,8 @@ export function createLabel(
 ) {
  request.post(
    {
-      url: `http://127.0.0.1:3054/project/${projectId}/user/${userId}/labels`,
-      json: { comment, version, created_at: createdAt },
+      url: `http://127.0.0.1:3054/project/${projectId}/labels`,
+      json: { comment, version, created_at: createdAt, user_id: userId },
    },
    (error, res, body) => {
      if (error) {
--- a/services/project-history/test/unit/js/HttpController/HttpControllerTests.js
+++ b/services/project-history/test/unit/js/HttpController/HttpControllerTests.js
@ -412,13 +412,13 @@ describe('HttpController', function () {
      this.req = {
        params: {
          project_id: this.projectId,
-          user_id: this.userId,
        },
        body: {
          version: (this.version = 'label-1'),
          comment: (this.comment = 'a comment'),
          created_at: (this.created_at = Date.now().toString()),
          validate_exists: true,
+          user_id: this.userId,
        },
      }
      this.label = { _id: new ObjectId() }
--- a/services/project-history/test/unit/js/LabelsManager/LabelsManagerTests.js
+++ b/services/project-history/test/unit/js/LabelsManager/LabelsManagerTests.js
@ -1,14 +1,3 @@
-/* eslint-disable
-    no-return-assign,
-    no-undef,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 import sinon from 'sinon'
 import { expect } from 'chai'
 import mongodb from 'mongodb-legacy'
@ -53,11 +42,11 @@ describe('LabelsManager', function () {
    this.historyId = 123
    this.user_id = new ObjectId().toString()
    this.label_id = new ObjectId().toString()
-    return (this.callback = sinon.stub())
+    this.callback = sinon.stub()
  })

  afterEach(function () {
-    return tk.reset()
+    tk.reset()
  })

  describe('getLabels', function () {
@ -77,17 +66,17 @@ describe('LabelsManager', function () {

    describe('with valid project id', function () {
      beforeEach(function () {
-        return this.LabelsManager.getLabels(this.project_id, this.callback)
+        this.LabelsManager.getLabels(this.project_id, this.callback)
      })

      it('gets the labels state from mongo', function () {
-        return expect(
-          this.db.projectHistoryLabels.find
-        ).to.have.been.calledWith({ project_id: new ObjectId(this.project_id) })
+        expect(this.db.projectHistoryLabels.find).to.have.been.calledWith({
+          project_id: new ObjectId(this.project_id),
+        })
      })

-      return it('returns formatted labels', function () {
-        return expect(this.callback).to.have.been.calledWith(null, [
+      it('returns formatted labels', function () {
+        expect(this.callback).to.have.been.calledWith(null, [
          sinon.match({
            id: this.label._id,
            comment: this.label.comment,
@ -99,11 +88,11 @@ describe('LabelsManager', function () {
      })
    })

-    return describe('with invalid project id', function () {
-      return it('returns an error', function (done) {
-        return this.LabelsManager.getLabels('invalid id', error => {
+    describe('with invalid project id', function () {
+      it('returns an error', function (done) {
+        this.LabelsManager.getLabels('invalid id', error => {
          expect(error).to.exist
-          return done()
+          done()
        })
      })
    })
@ -122,7 +111,7 @@ describe('LabelsManager', function () {
        this.db.projectHistoryLabels.insertOne.yields(null, {
          insertedId: new ObjectId(this.label_id),
        })
-        return this.LabelsManager.createLabel(
+        this.LabelsManager.createLabel(
          this.project_id,
          this.user_id,
          this.version,
@ -134,27 +123,25 @@ describe('LabelsManager', function () {
      })

      it('flushes unprocessed updates', function () {
-        return expect(
+        expect(
          this.UpdatesProcessor.processUpdatesForProject
        ).to.have.been.calledWith(this.project_id)
      })

      it('finds the V1 project id', function () {
-        return expect(this.WebApiManager.getHistoryId).to.have.been.calledWith(
+        expect(this.WebApiManager.getHistoryId).to.have.been.calledWith(
          this.project_id
        )
      })

      it('checks there is a chunk for the project + version', function () {
-        return expect(
+        expect(
          this.HistoryStoreManager.getChunkAtVersion
        ).to.have.been.calledWith(this.project_id, this.historyId, this.version)
      })

      it('create the label in mongo', function () {
-        return expect(
-          this.db.projectHistoryLabels.insertOne
-        ).to.have.been.calledWith(
+        expect(this.db.projectHistoryLabels.insertOne).to.have.been.calledWith(
          sinon.match({
            project_id: new ObjectId(this.project_id),
            comment: this.comment,
@ -166,8 +153,8 @@ describe('LabelsManager', function () {
        )
      })

-      return it('returns the label', function () {
-        return expect(this.callback).to.have.been.calledWith(null, {
+      it('returns the label', function () {
+        expect(this.callback).to.have.been.calledWith(null, {
          id: new ObjectId(this.label_id),
          comment: this.comment,
          version: this.version,
@ -182,7 +169,7 @@ describe('LabelsManager', function () {
        this.db.projectHistoryLabels.insertOne.yields(null, {
          insertedId: new ObjectId(this.label_id),
        })
-        return this.LabelsManager.createLabel(
+        this.LabelsManager.createLabel(
          this.project_id,
          this.user_id,
          this.version,
@ -193,10 +180,8 @@ describe('LabelsManager', function () {
        )
      })

-      return it('create the label with the current date', function () {
-        return expect(
-          this.db.projectHistoryLabels.insertOne
-        ).to.have.been.calledWith(
+      it('create the label with the current date', function () {
+        expect(this.db.projectHistoryLabels.insertOne).to.have.been.calledWith(
          sinon.match({
            project_id: new ObjectId(this.project_id),
            comment: this.comment,
@ -208,13 +193,13 @@ describe('LabelsManager', function () {
      })
    })

-    return describe('with shouldValidateExists = false', function () {
+    describe('with shouldValidateExists = false', function () {
      beforeEach(function () {
        this.createdAt = new Date(1)
        this.db.projectHistoryLabels.insertOne.yields(null, {
          insertedId: new ObjectId(this.label_id),
        })
-        return this.LabelsManager.createLabel(
+        this.LabelsManager.createLabel(
          this.project_id,
          this.user_id,
          this.version,
@ -225,9 +210,39 @@ describe('LabelsManager', function () {
        )
      })

-      return it('checks there is a chunk for the project + version', function () {
-        return expect(this.HistoryStoreManager.getChunkAtVersion).to.not.have
-          .been.called
+      it('checks there is a chunk for the project + version', function () {
+        expect(this.HistoryStoreManager.getChunkAtVersion).to.not.have.been
+          .called
+      })
+    })
+
+    describe('with no userId', function () {
+      beforeEach(function () {
+        this.db.projectHistoryLabels.insertOne.yields(null, {
+          insertedId: new ObjectId(this.label_id),
+        })
+        const userId = undefined
+        this.LabelsManager.createLabel(
+          this.project_id,
+          userId,
+          this.version,
+          this.comment,
+          this.createdAt,
+          false,
+          this.callback
+        )
+      })
+
+      it('creates the label without user_id', function () {
+        expect(this.db.projectHistoryLabels.insertOne).to.have.been.calledWith(
+          sinon.match({
+            project_id: new ObjectId(this.project_id),
+            comment: this.comment,
+            version: this.version,
+            user_id: undefined,
+            created_at: this.now,
+          })
+        )
      })
    })
  })
@ -235,7 +250,7 @@ describe('LabelsManager', function () {
  describe('deleteLabelForUser', function () {
    beforeEach(function () {
      this.db.projectHistoryLabels.deleteOne.yields()
-      return this.LabelsManager.deleteLabelForUser(
+      this.LabelsManager.deleteLabelForUser(
        this.project_id,
        this.user_id,
        this.label_id,
@ -243,10 +258,8 @@ describe('LabelsManager', function () {
      )
    })

-    return it('removes the label from the database', function () {
-      return expect(
-        this.db.projectHistoryLabels.deleteOne
-      ).to.have.been.calledWith(
+    it('removes the label from the database', function () {
+      expect(this.db.projectHistoryLabels.deleteOne).to.have.been.calledWith(
        {
          _id: new ObjectId(this.label_id),
          project_id: new ObjectId(this.project_id),
@ -257,20 +270,18 @@ describe('LabelsManager', function () {
    })
  })

-  return describe('deleteLabel', function () {
+  describe('deleteLabel', function () {
    beforeEach(function () {
      this.db.projectHistoryLabels.deleteOne.yields()
-      return this.LabelsManager.deleteLabel(
+      this.LabelsManager.deleteLabel(
        this.project_id,
        this.label_id,
        this.callback
      )
    })

-    return it('removes the label from the database', function () {
-      return expect(
-        this.db.projectHistoryLabels.deleteOne
-      ).to.have.been.calledWith(
+    it('removes the label from the database', function () {
+      expect(this.db.projectHistoryLabels.deleteOne).to.have.been.calledWith(
        {
          _id: new ObjectId(this.label_id),
          project_id: new ObjectId(this.project_id),
--- a/services/web/app/src/Features/History/HistoryController.js
+++ b/services/web/app/src/Features/History/HistoryController.js
@ -196,8 +196,8 @@ module.exports = HistoryController = {
    HistoryController._makeRequest(
      {
        method: 'POST',
-        url: `${settings.apis.project_history.url}/project/${projectId}/user/${userId}/labels`,
-        json: { comment, version },
+        url: `${settings.apis.project_history.url}/project/${projectId}/labels`,
+        json: { comment, version, user_id: userId },
      },
      function (err, label) {
        if (err) {
@ -215,7 +215,9 @@ module.exports = HistoryController = {

  _enrichLabel(label, callback) {
    if (!label.user_id) {
-      return callback(null, label)
+      const newLabel = Object.assign({}, label)
+      newLabel.user_display_name = HistoryController._displayNameForUser(null)
+      return callback(null, newLabel)
    }
    UserGetter.getUser(
      label.user_id,
@ -237,7 +239,8 @@ module.exports = HistoryController = {
    }
    const uniqueUsers = new Set(labels.map(label => label.user_id))

-    // For backwards compatibility expect missing user_id fields
+    // For backwards compatibility, and for anonymously created labels in SP
+    // expect missing user_id fields
    uniqueUsers.delete(undefined)

    if (!uniqueUsers.size) {
@ -255,7 +258,6 @@ module.exports = HistoryController = {

        labels.forEach(label => {
          const user = users.get(label.user_id)
-          if (!user) return
          label.user_display_name = HistoryController._displayNameForUser(user)
        })
        callback(null, labels)
--- a/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js
+++ b/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js
@ -103,7 +103,7 @@ class MockProjectHistoryApi extends AbstractMockApi {
      }
    })

-    this.app.post('/project/:projectId/user/:user_id/labels', (req, res) => {
+    this.app.post('/project/:projectId/labels', (req, res) => {
      const { projectId } = req.params
      const { comment, version } = req.body
      const labelId = new ObjectId().toString()