diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js index ab16929bf9..05137a97f8 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js @@ -28,22 +28,34 @@ module.exports = { convertTrackChangesToExplicitFormat, }, } +// Forces null pendingReviewer_refs, readOnly_refs, and reviewer_refs to +// be empty arrays to avoid errors during $pull ops +// See https://github.com/overleaf/internal/issues/24610 +async function fixNullCollaboratorRefs(projectId) { + // Temporary cleanup for the case where pendingReviewer_refs is null + await Project.updateOne( + { _id: projectId, pendingReviewer_refs: { $type: 'null' } }, + { $set: { pendingReviewer_refs: [] } } + ).exec() + + // Temporary cleanup for the case where readOnly_refs is null + await Project.updateOne( + { _id: projectId, readOnly_refs: { $type: 'null' } }, + { $set: { readOnly_refs: [] } } + ).exec() + + // Temporary cleanup for the case where reviewer_refs is null + await Project.updateOne( + { _id: projectId, reviewer_refs: { $type: 'null' } }, + { $set: { reviewer_refs: [] } } + ).exec() +} async function removeUserFromProject(projectId, userId) { try { const project = await Project.findOne({ _id: projectId }).exec() - // Temporary workaround for the case where pendingReviewer_refs is null - await Project.updateOne( - { _id: projectId, pendingReviewer_refs: { $type: 'null' } }, - { $set: { pendingReviewer_refs: [] } } - ).exec() - - // Temporary workaround for the case where readOnly_refs is null - await Project.updateOne( - { _id: projectId, readOnly_refs: { $type: 'null' } }, - { $set: { readOnly_refs: [] } } - ).exec() + await fixNullCollaboratorRefs(projectId) // Deal with the old type of boolean value for archived // In order to clear it @@ -307,6 +319,9 @@ async function setCollaboratorPrivilegeLevel( ], } let update + + await fixNullCollaboratorRefs(projectId) + switch (privilegeLevel) { case PrivilegeLevels.READ_AND_WRITE: { update = { diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js index 7cf9adbb8e..8542bd8355 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js @@ -82,6 +82,46 @@ describe('CollaboratorsHandler', function () { './CollaboratorsGetter': this.CollaboratorsGetter, }, }) + + // Helper function to set up mock expectations for null reference cleanup + this.expectNullReferenceCleanup = projectId => { + this.ProjectMock.expects('updateOne') + .withArgs( + { + _id: projectId, + pendingReviewer_refs: { $type: 'null' }, + }, + { + $set: { pendingReviewer_refs: [] }, + } + ) + .chain('exec') + .resolves() + this.ProjectMock.expects('updateOne') + .withArgs( + { + _id: projectId, + readOnly_refs: { $type: 'null' }, + }, + { + $set: { readOnly_refs: [] }, + } + ) + .chain('exec') + .resolves() + this.ProjectMock.expects('updateOne') + .withArgs( + { + _id: projectId, + reviewer_refs: { $type: 'null' }, + }, + { + $set: { reviewer_refs: [] }, + } + ) + .chain('exec') + .resolves() + } }) afterEach(function () { @@ -100,30 +140,7 @@ describe('CollaboratorsHandler', function () { }) it('should remove the user from mongo', async function () { - this.ProjectMock.expects('updateOne') - .withArgs( - { - _id: this.project._id, - pendingReviewer_refs: { $type: 'null' }, - }, - { - $set: { pendingReviewer_refs: [] }, - } - ) - .chain('exec') - .resolves() - this.ProjectMock.expects('updateOne') - .withArgs( - { - _id: this.project._id, - readOnly_refs: { $type: 'null' }, - }, - { - $set: { readOnly_refs: [] }, - } - ) - .chain('exec') - .resolves() + this.expectNullReferenceCleanup(this.project._id) this.ProjectMock.expects('updateOne') .withArgs( { @@ -166,30 +183,7 @@ describe('CollaboratorsHandler', function () { }) it('should remove the user from mongo', async function () { - this.ProjectMock.expects('updateOne') - .withArgs( - { - _id: this.oldArchivedProject._id, - pendingReviewer_refs: { $type: 'null' }, - }, - { - $set: { pendingReviewer_refs: [] }, - } - ) - .chain('exec') - .resolves() - this.ProjectMock.expects('updateOne') - .withArgs( - { - _id: this.oldArchivedProject._id, - readOnly_refs: { $type: 'null' }, - }, - { - $set: { readOnly_refs: [] }, - } - ) - .chain('exec') - .resolves() + this.expectNullReferenceCleanup(this.oldArchivedProject._id) this.ProjectMock.expects('updateOne') .withArgs( { @@ -230,30 +224,7 @@ describe('CollaboratorsHandler', function () { }) it('should remove the user from mongo', async function () { - this.ProjectMock.expects('updateOne') - .withArgs( - { - _id: this.archivedProject._id, - pendingReviewer_refs: { $type: 'null' }, - }, - { - $set: { pendingReviewer_refs: [] }, - } - ) - .chain('exec') - .resolves() - this.ProjectMock.expects('updateOne') - .withArgs( - { - _id: this.archivedProject._id, - readOnly_refs: { $type: 'null' }, - }, - { - $set: { readOnly_refs: [] }, - } - ) - .chain('exec') - .resolves() + this.expectNullReferenceCleanup(this.archivedProject._id) this.ProjectMock.expects('updateOne') .withArgs( { @@ -541,30 +512,7 @@ describe('CollaboratorsHandler', function () { .chain('exec') .resolves({ _id: projectId }) - this.ProjectMock.expects('updateOne') - .withArgs( - { - _id: projectId, - pendingReviewer_refs: { $type: 'null' }, - }, - { - $set: { pendingReviewer_refs: [] }, - } - ) - .chain('exec') - .resolves() - this.ProjectMock.expects('updateOne') - .withArgs( - { - _id: projectId, - readOnly_refs: { $type: 'null' }, - }, - { - $set: { readOnly_refs: [] }, - } - ) - .chain('exec') - .resolves() + this.expectNullReferenceCleanup(projectId) this.ProjectMock.expects('updateOne') .withArgs( { @@ -727,6 +675,8 @@ describe('CollaboratorsHandler', function () { describe('setCollaboratorPrivilegeLevel', function () { it('sets a collaborator to read-only', async function () { + this.expectNullReferenceCleanup(this.project._id) + this.ProjectMock.expects('updateOne') .withArgs( { @@ -757,6 +707,8 @@ describe('CollaboratorsHandler', function () { }) it('sets a collaborator to read-write', async function () { + this.expectNullReferenceCleanup(this.project._id) + this.ProjectMock.expects('updateOne') .withArgs( { @@ -796,6 +748,8 @@ describe('CollaboratorsHandler', function () { }) }) it('should correctly update the project', async function () { + this.expectNullReferenceCleanup(this.project._id) + this.ProjectMock.expects('updateOne') .withArgs( { @@ -839,6 +793,8 @@ describe('CollaboratorsHandler', function () { }) }) it('should correctly update the project', async function () { + this.expectNullReferenceCleanup(this.project._id) + this.ProjectMock.expects('updateOne') .withArgs( { @@ -871,6 +827,8 @@ describe('CollaboratorsHandler', function () { }) it('sets a collaborator to read-only as a pendingEditor', async function () { + this.expectNullReferenceCleanup(this.project._id) + this.ProjectMock.expects('updateOne') .withArgs( { @@ -904,6 +862,8 @@ describe('CollaboratorsHandler', function () { }) it('sets a collaborator to read-only as a pendingReviewer', async function () { + this.expectNullReferenceCleanup(this.project._id) + this.ProjectMock.expects('updateOne') .withArgs( { @@ -937,6 +897,8 @@ describe('CollaboratorsHandler', function () { }) it('throws a NotFoundError if the project or collaborator does not exist', async function () { + this.expectNullReferenceCleanup(this.project._id) + this.ProjectMock.expects('updateOne') .chain('exec') .resolves({ matchedCount: 0 })