Merge pull request #16545 from overleaf/dp-project-owner-delete-labels

Allow project owners to delete history labels

GitOrigin-RevId: 16111337681ac4085db2cf48e9d4c2fa87993b77

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)
     }

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,

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
   )
 

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,

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 } },

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)
     })
 

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
+      )
+    })
+  })
 })

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',
+        owner_ref: true,
-        url: `${settings.apis.project_history.url}/project/${projectId}/user/${userId}/labels/${labelId}`,
       },
-      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)
+          }
+        )
       }
     )
   },

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={<Icon type="tag" fw />}
         onClose={showConfirmationModal}
         closeButton={Boolean(
-          isOwnedByCurrentUser && !isPseudoCurrentStateLabel
+          (isOwnedByCurrentUser || isProjectOwner) && !isPseudoCurrentStateLabel
         )}
         closeBtnProps={{ 'aria-label': t('delete') }}
         className="history-version-badge"

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)
     })

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()