From cd6631c1050c377595c1c4aa7b52a32e8fa6a2b5 Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Tue, 1 Oct 2024 14:36:38 +0200 Subject: [PATCH] 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 --- .../project-history/app/js/HttpController.js | 8 +- .../project-history/app/js/LabelsManager.js | 12 +- services/project-history/app/js/Router.js | 17 +++ .../test/acceptance/js/LabelsTests.js | 127 +++++++++--------- .../js/helpers/ProjectHistoryClient.js | 4 +- .../js/HttpController/HttpControllerTests.js | 2 +- .../js/LabelsManager/LabelsManagerTests.js | 117 ++++++++-------- .../src/Features/History/HistoryController.js | 12 +- .../src/mocks/MockProjectHistoryApi.js | 2 +- 9 files changed, 170 insertions(+), 131 deletions(-) diff --git a/services/project-history/app/js/HttpController.js b/services/project-history/app/js/HttpController.js index 934f1de781..91cb297289 100644 --- 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) => { diff --git a/services/project-history/app/js/LabelsManager.js b/services/project-history/app/js/LabelsManager.js index 6aa9049193..fe3dd4eed2 100644 --- 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) diff --git a/services/project-history/app/js/Router.js b/services/project-history/app/js/Router.js index 42fe4c244f..329cc501fb 100644 --- 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({ diff --git a/services/project-history/test/acceptance/js/LabelsTests.js b/services/project-history/test/acceptance/js/LabelsTests.js index e872cf7d48..2812add814 100644 --- 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) => { - if (error != null) { - throw error - } - expect(labels).to.deep.equal([label]) - return done() + ProjectHistoryClient.getLabels(this.project_id, (error, labels) => { + if (error != null) { + throw error } - ) + expect(labels).to.deep.equal([label]) + 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 + } + ProjectHistoryClient.getLabels(this.project_id, (error, labels) => { if (error != null) { throw error } - return ProjectHistoryClient.getLabels( - this.project_id, - (error, labels) => { - if (error != null) { - throw error - } - expect(labels).to.deep.equal([]) - return done() - } - ) - } - ) + expect(labels).to.deep.equal([]) + 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) => { - if (error != null) { - throw error - } - expect(labels).to.deep.equal([]) - return done() + ProjectHistoryClient.getLabels(this.project_id, (error, labels) => { + if (error != null) { + throw error } - ) + expect(labels).to.deep.equal([]) + 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() } ) } diff --git a/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js b/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js index e0b7eb6acf..cadc7969a7 100644 --- 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) { diff --git a/services/project-history/test/unit/js/HttpController/HttpControllerTests.js b/services/project-history/test/unit/js/HttpController/HttpControllerTests.js index 834c3b8183..683fd9cea8 100644 --- 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() } diff --git a/services/project-history/test/unit/js/LabelsManager/LabelsManagerTests.js b/services/project-history/test/unit/js/LabelsManager/LabelsManagerTests.js index 5353b8b212..3916f457d0 100644 --- 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), diff --git a/services/web/app/src/Features/History/HistoryController.js b/services/web/app/src/Features/History/HistoryController.js index eadf71f1b2..5fb9990dc2 100644 --- 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) diff --git a/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js b/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js index 900ecb56d5..4f53277923 100644 --- 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()