From a096d98956cad5ca931c26b630d83736c9d81374 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 7 Oct 2020 15:16:54 +0200 Subject: [PATCH] Merge pull request #3193 from overleaf/jpa-mongodb-native-acceptance-tests [misc] migrate the acceptance tests to the native mongo driver GitOrigin-RevId: 5ec8605cafb28cc9cfeb85d7ee0d1b567cfe49ba --- .../acceptance/src/ConvertArchivedState.js | 2 +- .../web/test/acceptance/src/DeletionTests.js | 2 +- .../web/test/acceptance/src/HistoryTests.js | 8 +- services/web/test/acceptance/src/Init.js | 31 +---- .../test/acceptance/src/PasswordResetTests.js | 14 +- .../test/acceptance/src/TokenAccessTests.js | 10 +- .../test/acceptance/src/UserEmailsTests.js | 129 ++++++++---------- .../acceptance/src/UserOnboardingTests.js | 13 +- .../acceptance/src/helpers/Subscription.js | 2 +- .../web/test/acceptance/src/helpers/User.js | 68 +++------ 10 files changed, 100 insertions(+), 179 deletions(-) diff --git a/services/web/test/acceptance/src/ConvertArchivedState.js b/services/web/test/acceptance/src/ConvertArchivedState.js index 2be3b60033..ae35145f67 100644 --- a/services/web/test/acceptance/src/ConvertArchivedState.js +++ b/services/web/test/acceptance/src/ConvertArchivedState.js @@ -1,6 +1,6 @@ const { expect } = require('chai') const { exec } = require('child_process') -const { ObjectId } = require('../../../app/src/infrastructure/mongojs') +const { ObjectId } = require('mongodb') const User = require('./helpers/User').promises diff --git a/services/web/test/acceptance/src/DeletionTests.js b/services/web/test/acceptance/src/DeletionTests.js index 7be10bca76..48d4fd4b56 100644 --- a/services/web/test/acceptance/src/DeletionTests.js +++ b/services/web/test/acceptance/src/DeletionTests.js @@ -3,7 +3,7 @@ const request = require('./helpers/request') const async = require('async') const { expect } = require('chai') const settings = require('settings-sharelatex') -const { db, ObjectId } = require('../../../app/src/infrastructure/mongojs') +const { db, ObjectId } = require('../../../app/src/infrastructure/mongodb') const { Subscription } = require('../../../app/src/models/Subscription') const SubscriptionViewModelBuilder = require('../../../app/src/Features/Subscription/SubscriptionViewModelBuilder') const MockDocstoreApi = require('./helpers/MockDocstoreApi') diff --git a/services/web/test/acceptance/src/HistoryTests.js b/services/web/test/acceptance/src/HistoryTests.js index 81a4500f7e..ad856189e1 100644 --- a/services/web/test/acceptance/src/HistoryTests.js +++ b/services/web/test/acceptance/src/HistoryTests.js @@ -13,7 +13,7 @@ */ const { expect } = require('chai') -const { db, ObjectId } = require('../../../app/src/infrastructure/mongojs') +const { db, ObjectId } = require('../../../app/src/infrastructure/mongodb') const MockV1HistoryApi = require('./helpers/MockV1HistoryApi') const User = require('./helpers/User') @@ -33,7 +33,7 @@ describe('History', function() { return done(error) } this.v1_history_id = 42 - return db.projects.update( + return db.projects.updateOne( { _id: ObjectId(this.project_id) }, @@ -83,7 +83,7 @@ describe('History', function() { return done(error) } this.v1_history_id = 42 - db.projects.update( + db.projects.updateOne( { _id: ObjectId(this.project_id) }, { $set: { @@ -193,7 +193,7 @@ describe('History', function() { if (error != null) { return done(error) } - return db.projects.update( + return db.projects.updateOne( { _id: ObjectId(this.project_id) }, diff --git a/services/web/test/acceptance/src/Init.js b/services/web/test/acceptance/src/Init.js index 5ae3b62240..5b437f1658 100644 --- a/services/web/test/acceptance/src/Init.js +++ b/services/web/test/acceptance/src/Init.js @@ -1,7 +1,6 @@ const App = require('../../../app.js') const { exec } = require('child_process') -const { waitForDb } = require('../../../app/src/infrastructure/mongodb') -const { db } = require('../../../app/src/infrastructure/mongojs') +const { waitForDb, db } = require('../../../app/src/infrastructure/mongodb') require('logger-sharelatex').logger.level('error') @@ -18,28 +17,8 @@ before(function(done) { }) }) -afterEach(function(done) { - db.getCollectionNames((error, names) => { - if (error) { - throw error - } - Promise.all( - names.map(name => { - return new Promise((resolve, reject) => { - db[name].remove({}, err => { - if (err) { - reject(err) - } else { - resolve() - } - }) - }) - }) - ).then( - () => done(), - err => { - throw err - } - ) - }) +afterEach(async function() { + return Promise.all( + Object.values(db).map(collection => collection.deleteMany({})) + ) }) diff --git a/services/web/test/acceptance/src/PasswordResetTests.js b/services/web/test/acceptance/src/PasswordResetTests.js index 3d54ed5eb9..71f9afb707 100644 --- a/services/web/test/acceptance/src/PasswordResetTests.js +++ b/services/web/test/acceptance/src/PasswordResetTests.js @@ -1,7 +1,7 @@ const { expect } = require('chai') const RateLimiter = require('../../../app/src/infrastructure/RateLimiter') const UserHelper = require('./helpers/UserHelper') -const { db } = require('../../../app/src/infrastructure/mongojs') +const { db } = require('../../../app/src/infrastructure/mongodb') describe('PasswordReset', function() { let email, response, user, userHelper, token @@ -25,15 +25,9 @@ describe('PasswordReset', function() { } }) - token = await new Promise(resolve => { - db.tokens.findOne( - { 'data.user_id': user._id.toString() }, - (error, tokenData) => { - expect(error).to.not.exist - resolve(tokenData.token) - } - ) - }) + token = (await db.tokens.findOne({ + 'data.user_id': user._id.toString() + })).token }) describe('with a valid token', function() { describe('when logged in', function() { diff --git a/services/web/test/acceptance/src/TokenAccessTests.js b/services/web/test/acceptance/src/TokenAccessTests.js index 803dce63ce..beb3ea7a81 100644 --- a/services/web/test/acceptance/src/TokenAccessTests.js +++ b/services/web/test/acceptance/src/TokenAccessTests.js @@ -4,7 +4,7 @@ const MockV1Api = require('./helpers/MockV1Api') const User = require('./helpers/User') const request = require('./helpers/request') const settings = require('settings-sharelatex') -const { db, ObjectId } = require('../../../app/src/infrastructure/mongojs') +const { db, ObjectId } = require('../../../app/src/infrastructure/mongodb') const expectErrorResponse = require('./helpers/expectErrorResponse') @@ -1023,7 +1023,7 @@ describe('TokenAccess', function() { expect(err).not.to.exist this.tokens = project.tokens this.owner.makePrivate(this.projectId, () => { - db.projects.update( + db.projects.updateOne( { _id: project._id }, { $set: { @@ -1259,7 +1259,7 @@ describe('TokenAccess', function() { return done(err) } this.projectId = projectId - db.users.update( + db.users.updateOne( { _id: ObjectId(this.owner._id.toString()) }, { $set: { 'overleaf.id': 321321 } }, err => { @@ -1270,7 +1270,7 @@ describe('TokenAccess', function() { if (err != null) { return done(err) } - db.projects.update( + db.projects.updateOne( { _id: ObjectId(projectId) }, { $set: { overleaf: { id: 1234 } } }, err => { @@ -1290,7 +1290,7 @@ describe('TokenAccess', function() { } MockV1Api.setDocInfo(this.tokens.readAndWrite, docInfo) MockV1Api.setDocInfo(this.tokens.readOnly, docInfo) - db.projects.remove({ _id: ObjectId(projectId) }, done) + db.projects.deleteOne({ _id: ObjectId(projectId) }, done) }) } ) diff --git a/services/web/test/acceptance/src/UserEmailsTests.js b/services/web/test/acceptance/src/UserEmailsTests.js index 300afe60b1..982a9eb02c 100644 --- a/services/web/test/acceptance/src/UserEmailsTests.js +++ b/services/web/test/acceptance/src/UserEmailsTests.js @@ -2,7 +2,7 @@ const { expect } = require('chai') const async = require('async') const User = require('./helpers/User') const UserHelper = require('./helpers/UserHelper') -const { db, ObjectId } = require('../../../app/src/infrastructure/mongojs') +const { db, ObjectId } = require('../../../app/src/infrastructure/mongodb') const MockV1Api = require('./helpers/MockV1Api') const expectErrorResponse = require('./helpers/expectErrorResponse') @@ -47,13 +47,13 @@ describe('UserEmails', function() { ) }, cb => { - db.tokens.find( - { + db.tokens + .find({ use: 'email_confirmation', 'data.user_id': this.user._id, usedAt: { $exists: false } - }, - (error, tokens) => { + }) + .toArray((error, tokens) => { expect(error).to.not.exist // There should only be one confirmation token at the moment expect(tokens.length).to.equal(1) @@ -63,8 +63,7 @@ describe('UserEmails', function() { expect(tokens[0].data.user_id).to.equal(this.user._id) ;({ token } = tokens[0]) cb() - } - ) + }) }, cb => { this.user.request( @@ -95,19 +94,18 @@ describe('UserEmails', function() { ) }, cb => { - db.tokens.find( - { + db.tokens + .find({ use: 'email_confirmation', 'data.user_id': this.user._id, usedAt: { $exists: false } - }, - (error, tokens) => { + }) + .toArray((error, tokens) => { expect(error).to.not.exist // Token should be deleted after use expect(tokens.length).to.equal(0) cb() - } - ) + }) } ], done @@ -134,13 +132,13 @@ describe('UserEmails', function() { ) }, cb => { - db.tokens.find( - { + db.tokens + .find({ use: 'email_confirmation', 'data.user_id': this.user._id, usedAt: { $exists: false } - }, - (error, tokens) => { + }) + .toArray((error, tokens) => { expect(error).to.not.exist // There should only be one confirmation token at the moment expect(tokens.length).to.equal(1) @@ -148,8 +146,7 @@ describe('UserEmails', function() { expect(tokens[0].data.user_id).to.equal(this.user._id) token1 = tokens[0].token cb() - } - ) + }) }, cb => { // Delete the email from the first user @@ -191,13 +188,13 @@ describe('UserEmails', function() { ) }, cb => { - db.tokens.find( - { + db.tokens + .find({ use: 'email_confirmation', 'data.user_id': this.user2._id, usedAt: { $exists: false } - }, - (error, tokens) => { + }) + .toArray((error, tokens) => { expect(error).to.not.exist // The first token has been used, so this should be token2 now expect(tokens.length).to.equal(1) @@ -205,8 +202,7 @@ describe('UserEmails', function() { expect(tokens[0].data.user_id).to.equal(this.user2._id) token2 = tokens[0].token cb() - } - ) + }) }, cb => { // Second user should be able to confirm the email @@ -265,13 +261,13 @@ describe('UserEmails', function() { ) }, cb => { - db.tokens.find( - { + db.tokens + .find({ use: 'email_confirmation', 'data.user_id': this.user._id, usedAt: { $exists: false } - }, - (error, tokens) => { + }) + .toArray((error, tokens) => { expect(error).to.not.exist // There should only be one confirmation token at the moment expect(tokens.length).to.equal(1) @@ -279,8 +275,7 @@ describe('UserEmails', function() { expect(tokens[0].data.user_id).to.equal(this.user._id) ;({ token } = tokens[0]) cb() - } - ) + }) }, cb => { db.tokens.update( @@ -338,13 +333,13 @@ describe('UserEmails', function() { ) }, cb => { - db.tokens.find( - { + db.tokens + .find({ use: 'email_confirmation', 'data.user_id': this.user._id, usedAt: { $exists: false } - }, - (error, tokens) => { + }) + .toArray((error, tokens) => { expect(error).to.not.exist // There should only be one confirmation token at the moment expect(tokens.length).to.equal(1) @@ -353,8 +348,7 @@ describe('UserEmails', function() { ) expect(tokens[0].data.user_id).to.equal(this.user._id) cb() - } - ) + }) }, cb => { this.user.request( @@ -373,13 +367,13 @@ describe('UserEmails', function() { ) }, cb => { - db.tokens.find( - { + db.tokens + .find({ use: 'email_confirmation', 'data.user_id': this.user._id, usedAt: { $exists: false } - }, - (error, tokens) => { + }) + .toArray((error, tokens) => { expect(error).to.not.exist // There should be two tokens now expect(tokens.length).to.equal(2) @@ -392,8 +386,7 @@ describe('UserEmails', function() { ) expect(tokens[1].data.user_id).to.equal(this.user._id) cb() - } - ) + }) } ], done @@ -432,21 +425,20 @@ describe('UserEmails', function() { ) }, cb => { - db.tokens.find( - { + db.tokens + .find({ use: 'email_confirmation', 'data.user_id': this.user._id, usedAt: { $exists: false } - }, - (error, tokens) => { + }) + .toArray((error, tokens) => { expect(error).to.not.exist // There should still only be one confirmation token expect(tokens.length).to.equal(1) expect(tokens[0].data.email).to.equal(this.user.email) expect(tokens[0].data.user_id).to.equal(this.user._id) cb() - } - ) + }) } ], done @@ -473,18 +465,17 @@ describe('UserEmails', function() { ) }, cb => { - db.tokens.find( - { + db.tokens + .find({ use: 'email_confirmation', 'data.user_id': this.user._id, usedAt: { $exists: false } - }, - (error, tokens) => { + }) + .toArray((error, tokens) => { expect(error).to.not.exist expect(tokens.length).to.equal(0) cb() - } - ) + }) } ], done @@ -514,7 +505,7 @@ describe('UserEmails', function() { }, cb => { // Mark the email as confirmed - db.users.update( + db.users.updateOne( { 'emails.email': 'new-confirmed-default@example.com' }, @@ -565,7 +556,7 @@ describe('UserEmails', function() { async.series( [ cb => { - db.users.update( + db.users.updateOne( { _id: ObjectId(this.user._id) }, @@ -629,7 +620,7 @@ describe('UserEmails', function() { async.series( [ cb => { - db.users.update( + db.users.updateOne( { _id: ObjectId(this.user._id) }, @@ -659,7 +650,7 @@ describe('UserEmails', function() { }, cb => { // Mark the email as confirmed - db.users.update( + db.users.updateOne( { 'emails.email': 'new-confirmed-default-in-v1@example.com' }, @@ -701,7 +692,7 @@ describe('UserEmails', function() { async.series( [ cb => { - db.users.update( + db.users.updateOne( { _id: ObjectId(this.user._id) }, @@ -731,7 +722,7 @@ describe('UserEmails', function() { }, cb => { // Mark the email as confirmed - db.users.update( + db.users.updateOne( { 'emails.email': 'exists-in-v1@example.com' }, @@ -797,18 +788,10 @@ describe('UserEmails', function() { uri: '/user/emails' }) expect(response.statusCode).to.equal(204) - const token = await new Promise(resolve => { - db.tokens.findOne( - { - 'data.user_id': userId.toString(), - 'data.email': otherEmail - }, - (error, tokenData) => { - expect(error).to.not.exist - resolve(tokenData.token) - } - ) - }) + const token = (await db.tokens.findOne({ + 'data.user_id': userId.toString(), + 'data.email': otherEmail + })).token response = await userHelper.request.post(`/user/emails/confirm`, { form: { token diff --git a/services/web/test/acceptance/src/UserOnboardingTests.js b/services/web/test/acceptance/src/UserOnboardingTests.js index 40218ab7a7..b818238c03 100644 --- a/services/web/test/acceptance/src/UserOnboardingTests.js +++ b/services/web/test/acceptance/src/UserOnboardingTests.js @@ -2,7 +2,7 @@ const { expect } = require('chai') const async = require('async') const User = require('./helpers/User') const request = require('./helpers/request') -const { db, ObjectId } = require('../../../app/src/infrastructure/mongojs') +const { db, ObjectId } = require('../../../app/src/infrastructure/mongodb') const _ = require('underscore') describe('UserOnboardingTests', function() { @@ -46,11 +46,11 @@ describe('UserOnboardingTests', function() { // user 3 should still not have had an email sent const user3 = this.user3 - db.users.find( - { + db.users + .find({ onboardingEmailSentAt: null - }, - (error, users) => { + }) + .toArray((error, users) => { if (error != null) { throw error } @@ -58,8 +58,7 @@ describe('UserOnboardingTests', function() { expect(ids.length).to.equal(1) expect(ids).to.include(user3._id.toString()) done() - } - ) + }) } ) }) diff --git a/services/web/test/acceptance/src/helpers/Subscription.js b/services/web/test/acceptance/src/helpers/Subscription.js index 7fab941213..b1b7fd86d9 100644 --- a/services/web/test/acceptance/src/helpers/Subscription.js +++ b/services/web/test/acceptance/src/helpers/Subscription.js @@ -1,4 +1,4 @@ -const { db, ObjectId } = require('../../../../app/src/infrastructure/mongojs') +const { db, ObjectId } = require('../../../../app/src/infrastructure/mongodb') const { expect } = require('chai') const SubscriptionUpdater = require('../../../../app/src/Features/Subscription/SubscriptionUpdater') const SubscriptionModel = require('../../../../app/src/models/Subscription') diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index 2a08c1bf45..8e5d42df48 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -1,6 +1,6 @@ const request = require('./request') const settings = require('settings-sharelatex') -const { db, ObjectId } = require('../../../../app/src/infrastructure/mongojs') +const { db, ObjectId } = require('../../../../app/src/infrastructure/mongodb') const UserModel = require('../../../../app/src/models/User').User const UserUpdater = require('../../../../app/src/Features/User/UserUpdater') const AuthenticationManager = require('../../../../app/src/Features/Authentication/AuthenticationManager') @@ -43,7 +43,7 @@ class User { } mongoUpdate(updateOp, callback) { - db.users.update({ _id: ObjectId(this._id) }, updateOp, callback) + db.users.updateOne({ _id: ObjectId(this._id) }, updateOp, callback) } register(callback) { @@ -202,17 +202,13 @@ class User { } ensureAdmin(callback) { - db.users.update( - { _id: ObjectId(this.id) }, - { $set: { isAdmin: true } }, - callback - ) + this.mongoUpdate({ $set: { isAdmin: true } }, callback) } ensureStaffAccess(flag, callback) { const update = { $set: {} } update.$set[`staffAccess.${flag}`] = true - db.users.update({ _id: ObjectId(this.id) }, update, callback) + this.mongoUpdate(update, callback) } upgradeFeatures(callback) { @@ -227,11 +223,7 @@ class User { trackChanges: true, trackChangesVisible: true } - db.users.update( - { _id: ObjectId(this.id) }, - { $set: { features } }, - callback - ) + this.mongoUpdate({ $set: { features } }, callback) } downgradeFeatures(callback) { @@ -246,26 +238,18 @@ class User { trackChanges: false, trackChangesVisible: false } - db.users.update( - { _id: ObjectId(this.id) }, - { $set: { features } }, - callback - ) + this.mongoUpdate({ $set: { features } }, callback) } defaultFeatures(callback) { const features = settings.defaultFeatures - db.users.update( - { _id: ObjectId(this.id) }, - { $set: { features } }, - callback - ) + this.mongoUpdate({ $set: { features } }, callback) } getFeatures(callback) { db.users.findOne( { _id: ObjectId(this.id) }, - { features: 1 }, + { projection: { features: 1 } }, (error, user) => callback(error, user && user.features) ) } @@ -279,16 +263,12 @@ class User { return callback() } const userId = user._id - db.projects.remove( - { owner_ref: ObjectId(userId) }, - { multi: true }, - err => { - if (err != null) { - callback(err) - } - db.users.remove({ _id: ObjectId(userId) }, callback) + db.projects.deleteMany({ owner_ref: ObjectId(userId) }, err => { + if (err != null) { + callback(err) } - ) + db.users.deleteOne({ _id: ObjectId(userId) }, callback) + }) }) } @@ -323,7 +303,7 @@ class User { } saveProject(project, callback) { - db.projects.update({ _id: project._id }, project, callback) + db.projects.updateOne({ _id: project._id }, { $set: project }, callback) } createProject(name, options, callback) { @@ -375,9 +355,7 @@ class User { } deleteProjects(callback) { - db.projects.remove({ owner_ref: ObjectId(this.id) }, { multi: true }, err => - callback(err) - ) + db.projects.deleteMany({ owner_ref: ObjectId(this.id) }, callback) } openProject(projectId, callback) { @@ -430,9 +408,7 @@ class User { } else if (privileges === 'readOnly') { updateOp = { $addToSet: { readOnly_refs: user._id } } } - db.projects.update({ _id: db.ObjectId(projectId) }, updateOp, err => - callback(err) - ) + db.projects.updateOne({ _id: ObjectId(projectId) }, updateOp, callback) } makePublic(projectId, level, callback) { @@ -520,17 +496,7 @@ class User { newPassword2: this.password } }, - (error, response, body) => { - if (error != null) { - return callback(error) - } - db.users.findOne({ email: this.email }, (error, user) => { - if (error != null) { - return callback(error) - } - callback() - }) - } + callback ) }) }