From 693100358c8f7e663197fe3c93b9050348b47816 Mon Sep 17 00:00:00 2001 From: Hugh O'Brien Date: Tue, 3 Mar 2020 16:37:31 +0000 Subject: [PATCH] Merge pull request #2646 from overleaf/as-fix-trashing-old-archived-project Fix error thrown when trashing project with legacy boolean archived state GitOrigin-RevId: 0131a2294767e00d69c4dd80ed86dfd5d77339b8 --- .../src/Features/Project/ProjectDeleter.js | 8 +++- .../test/acceptance/src/ProjectCRUDTests.js | 47 +++++++++++++++++-- .../unit/src/Project/ProjectDeleterTests.js | 41 +++++++++++++++- 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/services/web/app/src/Features/Project/ProjectDeleter.js b/services/web/app/src/Features/Project/ProjectDeleter.js index 1b685daaa5..4ee383f24c 100644 --- a/services/web/app/src/Features/Project/ProjectDeleter.js +++ b/services/web/app/src/Features/Project/ProjectDeleter.js @@ -144,11 +144,17 @@ async function trashProject(projectId, userId) { throw new Errors.NotFoundError('project not found') } + const archived = ProjectHelper.calculateArchivedArray( + project, + userId, + 'UNARCHIVE' + ) + await Project.update( { _id: projectId }, { $addToSet: { trashed: ObjectId(userId) }, - $pull: { archived: ObjectId(userId) } + $set: { archived: archived } } ) } catch (err) { diff --git a/services/web/test/acceptance/src/ProjectCRUDTests.js b/services/web/test/acceptance/src/ProjectCRUDTests.js index 51161733c8..c4cd8ae86d 100644 --- a/services/web/test/acceptance/src/ProjectCRUDTests.js +++ b/services/web/test/acceptance/src/ProjectCRUDTests.js @@ -1,6 +1,7 @@ const { expect } = require('chai') const User = require('./helpers/User').promises const { Project } = require('../../../app/src/models/Project') +const { ObjectId } = require('../../../app/src/infrastructure/mongojs') describe('Project CRUD', function() { beforeEach(async function() { @@ -49,14 +50,52 @@ describe('Project CRUD', function() { const trashedProject = await Project.findById(this.projectId).exec() expectObjectIdArrayEqual(trashedProject.trashed, [this.user._id]) }) + + describe('with an array archived state', function() { + it('should mark the project as not archived for the user', async function() { + await Project.update( + { _id: this.projectId }, + { $set: { archived: [ObjectId(this.user._id)] } } + ).exec() + + const { response } = await this.user.doRequest( + 'POST', + `/project/${this.projectId}/trash` + ) + + expect(response.statusCode).to.equal(200) + + const trashedProject = await Project.findById(this.projectId).exec() + expectObjectIdArrayEqual(trashedProject.archived, []) + }) + }) + + describe('with a legacy boolean state', function() { + it('should mark the project as not archived for the user', async function() { + await Project.update( + { _id: this.projectId }, + { $set: { archived: true } } + ).exec() + + const { response } = await this.user.doRequest( + 'POST', + `/project/${this.projectId}/trash` + ) + + expect(response.statusCode).to.equal(200) + + const trashedProject = await Project.findById(this.projectId).exec() + expectObjectIdArrayEqual(trashedProject.archived, []) + }) + }) }) describe('when untrashing a project', function() { it('should mark the project as untrashed for the user', async function() { await Project.update( { _id: this.projectId }, - { trashed: [this.user._id] } - ) + { trashed: [ObjectId(this.user._id)] } + ).exec() const { response } = await this.user.doRequest( 'DELETE', `/project/${this.projectId}/trash` @@ -70,8 +109,8 @@ describe('Project CRUD', function() { it('does nothing if the user has already untrashed the project', async function() { await Project.update( { _id: this.projectId }, - { trashed: [this.user._id] } - ) + { trashed: [ObjectId(this.user._id)] } + ).exec() // Mark as untrashed the first time await this.user.doRequest('DELETE', `/project/${this.projectId}/trash`) diff --git a/services/web/test/unit/src/Project/ProjectDeleterTests.js b/services/web/test/unit/src/Project/ProjectDeleterTests.js index a1a4608ad7..939119cdfd 100644 --- a/services/web/test/unit/src/Project/ProjectDeleterTests.js +++ b/services/web/test/unit/src/Project/ProjectDeleterTests.js @@ -453,6 +453,18 @@ describe('ProjectDeleter', function() { ) this.ProjectMock.verify() }) + + it('calculates the archived array', async function() { + await this.ProjectDeleter.promises.archiveProject( + this.project._id, + this.user._id + ) + expect(this.ProjectHelper.calculateArchivedArray).to.have.been.calledWith( + this.project, + this.user._id, + 'ARCHIVE' + ) + }) }) describe('unarchiveProject', function() { @@ -477,10 +489,25 @@ describe('ProjectDeleter', function() { ) this.ProjectMock.verify() }) + + it('calculates the archived array', async function() { + await this.ProjectDeleter.promises.unarchiveProject( + this.project._id, + this.user._id + ) + expect(this.ProjectHelper.calculateArchivedArray).to.have.been.calledWith( + this.project, + this.user._id, + 'UNARCHIVE' + ) + }) }) describe('trashProject', function() { beforeEach(function() { + let archived = [ObjectId(this.user._id)] + this.ProjectHelper.calculateArchivedArray.returns(archived) + this.ProjectMock.expects('findOne') .withArgs({ _id: this.project._id }) .chain('exec') @@ -491,7 +518,7 @@ describe('ProjectDeleter', function() { { _id: this.project._id }, { $addToSet: { trashed: ObjectId(this.user._id) }, - $pull: { archived: ObjectId(this.user._id) } + $set: { archived: archived } } ) .resolves() @@ -504,6 +531,18 @@ describe('ProjectDeleter', function() { ) this.ProjectMock.verify() }) + + it('unarchives the project', async function() { + await this.ProjectDeleter.promises.trashProject( + this.project._id, + this.user._id + ) + expect(this.ProjectHelper.calculateArchivedArray).to.have.been.calledWith( + this.project, + this.user._id, + 'UNARCHIVE' + ) + }) }) describe('untrashProject', function() {