From e3513a9d50a43e1083b61e4aacf842425cd3ece6 Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Mon, 22 Jan 2024 09:02:51 +0000 Subject: [PATCH] Merge pull request #16545 from overleaf/dp-project-owner-delete-labels Allow project owners to delete history labels GitOrigin-RevId: 16111337681ac4085db2cf48e9d4c2fa87993b77 --- .../project-history/app/js/HttpController.js | 20 +++++++++-- .../project-history/app/js/LabelsManager.js | 17 +++++++++- services/project-history/app/js/Router.js | 18 ++++++++++ .../test/acceptance/js/LabelsTests.js | 34 +++++++++++++++++++ .../js/helpers/ProjectHistoryClient.js | 17 +++++++++- .../js/HttpController/HttpControllerTests.js | 30 ++++++++++++++-- .../js/LabelsManager/LabelsManagerTests.js | 27 +++++++++++++-- .../src/Features/History/HistoryController.js | 30 +++++++++++++--- .../components/change-list/tag-tooltip.tsx | 5 ++- .../src/mocks/MockProjectHistoryApi.js | 14 ++++++++ .../src/History/HistoryControllerTests.js | 1 + 11 files changed, 198 insertions(+), 15 deletions(-) diff --git a/services/project-history/app/js/HttpController.js b/services/project-history/app/js/HttpController.js index 3f80694f0a..ac21c3d95c 100644 --- a/services/project-history/app/js/HttpController.js +++ b/services/project-history/app/js/HttpController.js @@ -379,13 +379,29 @@ export function createLabel(req, res, next) { ) } -export function deleteLabel(req, res, next) { +/** + * This will delete a label if it is owned by the current user. If you wish to + * delete a label regardless of the current user, then use `deleteLabel` instead. + */ +export function deleteLabelForUser(req, res, next) { const { project_id: projectId, user_id: userId, label_id: labelId, } = req.params - LabelsManager.deleteLabel(projectId, userId, labelId, error => { + + LabelsManager.deleteLabelForUser(projectId, userId, labelId, error => { + if (error != null) { + return next(error) + } + res.sendStatus(204) + }) +} + +export function deleteLabel(req, res, next) { + const { project_id: projectId, label_id: labelId } = req.params + + LabelsManager.deleteLabel(projectId, labelId, error => { if (error != null) { return next(error) } diff --git a/services/project-history/app/js/LabelsManager.js b/services/project-history/app/js/LabelsManager.js index 1b3ce2afdd..07a53fae26 100644 --- a/services/project-history/app/js/LabelsManager.js +++ b/services/project-history/app/js/LabelsManager.js @@ -81,7 +81,7 @@ export function createLabel( }) } -export function deleteLabel(projectId, userId, labelId, callback) { +export function deleteLabelForUser(projectId, userId, labelId, callback) { return _toObjectId( projectId, userId, @@ -102,6 +102,21 @@ export function deleteLabel(projectId, userId, labelId, callback) { ) } +export function deleteLabel(projectId, labelId, callback) { + return _toObjectId(projectId, labelId, function (error, projectId, labelId) { + if (error != null) { + return callback(OError.tag(error)) + } + return db.projectHistoryLabels.deleteOne( + { + _id: new ObjectId(labelId), + project_id: new ObjectId(projectId), + }, + callback + ) + }) +} + export function transferLabels(fromUserId, toUserId, callback) { return _toObjectId( fromUserId, diff --git a/services/project-history/app/js/Router.js b/services/project-history/app/js/Router.js index 4eca0effc4..5d514b3fba 100644 --- a/services/project-history/app/js/Router.js +++ b/services/project-history/app/js/Router.js @@ -112,6 +112,24 @@ export function initialize(app) { app.delete( '/project/:project_id/user/:user_id/labels/:label_id', + validate({ + params: Joi.object({ + project_id: Joi.string().regex(/^[0-9a-f]{24}$/), + user_id: Joi.string().regex(/^[0-9a-f]{24}$/), + label_id: Joi.string().regex(/^[0-9a-f]{24}$/), + }), + }), + HttpController.deleteLabelForUser + ) + + app.delete( + '/project/:project_id/labels/:label_id', + validate({ + params: Joi.object({ + project_id: Joi.string().regex(/^[0-9a-f]{24}$/), + label_id: Joi.string().regex(/^[0-9a-f]{24}$/), + }), + }), HttpController.deleteLabel ) diff --git a/services/project-history/test/acceptance/js/LabelsTests.js b/services/project-history/test/acceptance/js/LabelsTests.js index 4962f769e0..a31e5c670f 100644 --- a/services/project-history/test/acceptance/js/LabelsTests.js +++ b/services/project-history/test/acceptance/js/LabelsTests.js @@ -115,6 +115,40 @@ describe('Labels', function () { throw error } return ProjectHistoryClient.deleteLabel( + this.project_id, + label.id, + error => { + 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() + } + ) + } + ) + } + ) + }) + + it('can delete labels for the current user', function (done) { + return ProjectHistoryClient.createLabel( + this.project_id, + this.user_id, + 7, + this.comment, + this.created_at, + (error, label) => { + if (error != null) { + throw error + } + return ProjectHistoryClient.deleteLabelForUser( this.project_id, this.user_id, label.id, diff --git a/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js b/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js index 965759f8a0..e1cc96f45b 100644 --- a/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js +++ b/services/project-history/test/acceptance/js/helpers/ProjectHistoryClient.js @@ -234,7 +234,7 @@ export function getLabels(projectId, callback) { ) } -export function deleteLabel(projectId, userId, labelId, callback) { +export function deleteLabelForUser(projectId, userId, labelId, callback) { request.delete( { url: `http://localhost:3054/project/${projectId}/user/${userId}/labels/${labelId}`, @@ -249,6 +249,21 @@ export function deleteLabel(projectId, userId, labelId, callback) { ) } +export function deleteLabel(projectId, labelId, callback) { + request.delete( + { + url: `http://localhost:3054/project/${projectId}/labels/${labelId}`, + }, + (error, res, body) => { + if (error) { + return callback(error) + } + expect(res.statusCode).to.equal(204) + callback(null, body) + } + ) +} + export function setFailure(failureEntry, callback) { db.projectHistoryFailures.deleteOne( { project_id: { $exists: true } }, diff --git a/services/project-history/test/unit/js/HttpController/HttpControllerTests.js b/services/project-history/test/unit/js/HttpController/HttpControllerTests.js index 4510d032a1..1fae6f503c 100644 --- a/services/project-history/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/project-history/test/unit/js/HttpController/HttpControllerTests.js @@ -45,6 +45,7 @@ describe('HttpController', function () { this.LabelsManager = { createLabel: sinon.stub(), deleteLabel: sinon.stub().yields(), + deleteLabelForUser: sinon.stub().yields(), getLabels: sinon.stub(), } this.HistoryApiManager = { @@ -75,6 +76,7 @@ describe('HttpController', function () { }) this.pathname = 'doc-id-123' this.projectId = new ObjectId().toString() + this.projectOwnerId = new ObjectId().toString() this.next = sinon.stub() this.userId = new ObjectId().toString() this.now = Date.now() @@ -473,7 +475,7 @@ describe('HttpController', function () { }) }) - describe('deleteLabel', function () { + describe('deleteLabelForUser', function () { beforeEach(function () { this.req = { params: { @@ -482,12 +484,34 @@ describe('HttpController', function () { label_id: (this.label_id = new ObjectId()), }, } + this.HttpController.deleteLabelForUser(this.req, this.res, this.next) + }) + + it('should delete a label for a project', function () { + this.LabelsManager.deleteLabelForUser + .calledWith(this.projectId, this.userId, this.label_id) + .should.equal(true) + }) + + it('should return 204', function () { + this.res.sendStatus.calledWith(204).should.equal(true) + }) + }) + + describe('deleteLabel', function () { + beforeEach(function () { + this.req = { + params: { + project_id: this.projectId, + label_id: (this.label_id = new ObjectId()), + }, + } this.HttpController.deleteLabel(this.req, this.res, this.next) }) - it('should create a label for a project', function () { + it('should delete a label for a project', function () { this.LabelsManager.deleteLabel - .calledWith(this.projectId, this.userId, this.label_id) + .calledWith(this.projectId, this.label_id) .should.equal(true) }) diff --git a/services/project-history/test/unit/js/LabelsManager/LabelsManagerTests.js b/services/project-history/test/unit/js/LabelsManager/LabelsManagerTests.js index 12d6f1d836..5353b8b212 100644 --- a/services/project-history/test/unit/js/LabelsManager/LabelsManagerTests.js +++ b/services/project-history/test/unit/js/LabelsManager/LabelsManagerTests.js @@ -232,10 +232,10 @@ describe('LabelsManager', function () { }) }) - return describe('deleteLabel', function () { + describe('deleteLabelForUser', function () { beforeEach(function () { this.db.projectHistoryLabels.deleteOne.yields() - return this.LabelsManager.deleteLabel( + return this.LabelsManager.deleteLabelForUser( this.project_id, this.user_id, this.label_id, @@ -256,4 +256,27 @@ describe('LabelsManager', function () { ) }) }) + + return describe('deleteLabel', function () { + beforeEach(function () { + this.db.projectHistoryLabels.deleteOne.yields() + return 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( + { + _id: new ObjectId(this.label_id), + project_id: new ObjectId(this.project_id), + }, + this.callback + ) + }) + }) }) diff --git a/services/web/app/src/Features/History/HistoryController.js b/services/web/app/src/Features/History/HistoryController.js index d13b9bd49b..560996ce17 100644 --- a/services/web/app/src/Features/History/HistoryController.js +++ b/services/web/app/src/Features/History/HistoryController.js @@ -6,6 +6,7 @@ const request = require('request') const settings = require('@overleaf/settings') const SessionManager = require('../Authentication/SessionManager') const UserGetter = require('../User/UserGetter') +const ProjectGetter = require('../Project/ProjectGetter') const Errors = require('../Errors/Errors') const HistoryManager = require('./HistoryManager') const ProjectDetailsHandler = require('../Project/ProjectDetailsHandler') @@ -218,16 +219,35 @@ module.exports = HistoryController = { deleteLabel(req, res, next) { const { Project_id: projectId, label_id: labelId } = req.params const userId = SessionManager.getLoggedInUserId(req.session) - HistoryController._makeRequest( + + ProjectGetter.getProject( + projectId, { - method: 'DELETE', - url: `${settings.apis.project_history.url}/project/${projectId}/user/${userId}/labels/${labelId}`, + owner_ref: true, }, - function (err) { + (err, project) => { if (err) { return next(err) } - res.sendStatus(204) + + // If the current user is the project owner, we can use the non-user-specific delete label endpoint. + // Otherwise, we have to use the user-specific version (which only deletes the label if it is owned by the user) + const deleteEndpointUrl = project.owner_ref.equals(userId) + ? `${settings.apis.project_history.url}/project/${projectId}/labels/${labelId}` + : `${settings.apis.project_history.url}/project/${projectId}/user/${userId}/labels/${labelId}` + + HistoryController._makeRequest( + { + method: 'DELETE', + url: deleteEndpointUrl, + }, + function (err) { + if (err) { + return next(err) + } + res.sendStatus(204) + } + ) } ) }, diff --git a/services/web/frontend/js/features/history/components/change-list/tag-tooltip.tsx b/services/web/frontend/js/features/history/components/change-list/tag-tooltip.tsx index d15b0bed75..8ad8038fea 100644 --- a/services/web/frontend/js/features/history/components/change-list/tag-tooltip.tsx +++ b/services/web/frontend/js/features/history/components/change-list/tag-tooltip.tsx @@ -15,6 +15,7 @@ import { isPseudoLabel } from '../../utils/label' import { LoadedLabel } from '../../services/types/label' import { debugConsole } from '@/utils/debugging' import { formatTimeBasedOnYear } from '@/features/utils/format-date' +import { useEditorContext } from '@/shared/context/editor-context' type TagProps = { label: LoadedLabel @@ -22,6 +23,8 @@ type TagProps = { } function Tag({ label, currentUserId, ...props }: TagProps) { + const { isProjectOwner } = useEditorContext() + const { t } = useTranslation() const [showDeleteModal, setShowDeleteModal] = useState(false) const { projectId } = useHistoryContext() @@ -67,7 +70,7 @@ function Tag({ label, currentUserId, ...props }: TagProps) { prepend={} onClose={showConfirmationModal} closeButton={Boolean( - isOwnedByCurrentUser && !isPseudoCurrentStateLabel + (isOwnedByCurrentUser || isProjectOwner) && !isPseudoCurrentStateLabel )} closeBtnProps={{ 'aria-label': t('delete') }} className="history-version-badge" diff --git a/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js b/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js index 63dd10c904..8c0ba5c444 100644 --- a/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js +++ b/services/web/test/acceptance/src/mocks/MockProjectHistoryApi.js @@ -128,6 +128,20 @@ class MockProjectHistoryApi extends AbstractMockApi { } ) + this.app.delete('/project/:projectId/labels/:labelId', (req, res) => { + const { projectId, labelId } = req.params + const label = + this.labels[projectId] != null + ? this.labels[projectId][labelId] + : undefined + if (label != null) { + this.deleteLabel(projectId, labelId) + res.sendStatus(204) + } else { + res.sendStatus(404) + } + }) + this.app.post('/project/:projectId/flush', (req, res) => { res.sendStatus(200) }) diff --git a/services/web/test/unit/src/History/HistoryControllerTests.js b/services/web/test/unit/src/History/HistoryControllerTests.js index 50fe94f32d..816a7427f3 100644 --- a/services/web/test/unit/src/History/HistoryControllerTests.js +++ b/services/web/test/unit/src/History/HistoryControllerTests.js @@ -38,6 +38,7 @@ describe('HistoryController', function () { '../Project/ProjectEntityUpdateHandler': (this.ProjectEntityUpdateHandler = {}), '../User/UserGetter': (this.UserGetter = {}), + '../Project/ProjectGetter': (this.ProjectGetter = {}), './RestoreManager': (this.RestoreManager = {}), '../../infrastructure/Features': (this.Features = sinon .stub()