From c8977ab9d62150ad8411882cdb4f43545fd83c52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez=20Capel?= Date: Wed, 23 May 2018 15:23:46 +0100 Subject: [PATCH 01/30] Add overleaf id to user schema --- services/web/app/coffee/models/Subscription.coffee | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/web/app/coffee/models/Subscription.coffee b/services/web/app/coffee/models/Subscription.coffee index aaf78a4ab2..1e9ad71d83 100644 --- a/services/web/app/coffee/models/Subscription.coffee +++ b/services/web/app/coffee/models/Subscription.coffee @@ -18,6 +18,8 @@ SubscriptionSchema = new Schema downgraded: Boolean planCode: String allowed: {type: Boolean, default: true} + overleaf: + id: { type: Number } SubscriptionSchema.statics.findAndModify = (query, update, callback)-> From 8a55994f6484652e45b4e80d65da8eeea3ddacf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez=20Capel?= Date: Wed, 23 May 2018 16:11:28 +0100 Subject: [PATCH 02/30] Ensure a team overleaf id is unique --- services/web/app/coffee/models/Subscription.coffee | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/models/Subscription.coffee b/services/web/app/coffee/models/Subscription.coffee index 1e9ad71d83..a9ebb7d585 100644 --- a/services/web/app/coffee/models/Subscription.coffee +++ b/services/web/app/coffee/models/Subscription.coffee @@ -19,7 +19,11 @@ SubscriptionSchema = new Schema planCode: String allowed: {type: Boolean, default: true} overleaf: - id: { type: Number } + id: + type: Number + index: + unique: true, + partialFilterExpression: {'overleaf.id': {$exists: true}} SubscriptionSchema.statics.findAndModify = (query, update, callback)-> From 259d690f7cf6916ffcf9aab4b2aadb530660e709 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez=20Capel?= Date: Fri, 25 May 2018 15:45:33 +0100 Subject: [PATCH 03/30] Add method to update user references --- .../SubscriptionGroupHandler.coffee | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee index 5b7c2e0740..6cfe6b1e79 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee @@ -39,12 +39,27 @@ module.exports = SubscriptionGroupHandler = removeUserFromGroup: (adminUser_id, userToRemove_id, callback)-> SubscriptionUpdater.removeUserFromGroup adminUser_id, userToRemove_id, callback - + removeEmailInviteFromGroup: (adminUser_id, email, callback) -> SubscriptionUpdater.removeEmailInviteFromGroup adminUser_id, email, callback + + replaceUserReferencesInGroups: (oldId, newId, callback) -> + Subscription.update {admin_id: userStubId}, {admin_id: slUserId}, (error) -> + callback(error) if error? + + # Mongo won't let us pull and addToSet in the same query, so do it in + # two. Note we need to add first, since the query is based on the old user. + query = {member_ids: userStubId} + addNewUserUpdate = $addToSet: {member_ids: slUserId} + removeOldUserUpdate = $pull: {member_ids: userStubId} + + Subscription.update query, addNewUserUpdate, { multi: true }, (error) -> + return callback(error) if error? + Subscription.update query, removeOldUserUpdate, { multi: true }, callback + getPopulatedListOfMembers: (adminUser_id, callback)-> - SubscriptionLocator.getUsersSubscription adminUser_id, (err, subscription)-> + SubscriptionLocator.getUsersSubscription adminUser_id, (err, subscription)-> users = [] for email in subscription.invited_emails or [] users.push buildEmailInviteViewModel(email) @@ -111,7 +126,7 @@ module.exports = SubscriptionGroupHandler = async.series jobs, callback buildUserViewModel = (user)-> - u = + u = email: user.email first_name: user.first_name last_name: user.last_name @@ -123,4 +138,4 @@ buildEmailInviteViewModel = (email) -> return { email: email holdingAccount: true - } \ No newline at end of file + } From b52fbdbfa4539bf56b5593c3772d078ca3ad6dba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez=20Capel?= Date: Tue, 29 May 2018 15:35:46 +0100 Subject: [PATCH 04/30] Unit test SubscriptionGroupHandler.replaceUserReferencesInGroups --- .../SubscriptionGroupHandler.coffee | 9 ++-- .../SubscriptionGroupHandlerTests.coffee | 54 +++++++++++++++---- 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee index 6cfe6b1e79..fe1e78e5e3 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee @@ -2,6 +2,7 @@ async = require("async") _ = require("underscore") SubscriptionUpdater = require("./SubscriptionUpdater") SubscriptionLocator = require("./SubscriptionLocator") +Subscription = require("../../models/Subscription").Subscription UserLocator = require("../User/UserLocator") LimitationsManager = require("./LimitationsManager") logger = require("logger-sharelatex") @@ -45,14 +46,14 @@ module.exports = SubscriptionGroupHandler = replaceUserReferencesInGroups: (oldId, newId, callback) -> - Subscription.update {admin_id: userStubId}, {admin_id: slUserId}, (error) -> + Subscription.update {admin_id: oldId}, {admin_id: newId}, (error) -> callback(error) if error? # Mongo won't let us pull and addToSet in the same query, so do it in # two. Note we need to add first, since the query is based on the old user. - query = {member_ids: userStubId} - addNewUserUpdate = $addToSet: {member_ids: slUserId} - removeOldUserUpdate = $pull: {member_ids: userStubId} + query = { member_ids: oldId } + addNewUserUpdate = $addToSet: { member_ids: newId } + removeOldUserUpdate = $pull: { member_ids: oldId } Subscription.update query, addNewUserUpdate, { multi: true }, (error) -> return callback(error) if error? diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee index 1daf43bbd7..acb5bdea16 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee @@ -19,12 +19,12 @@ describe "SubscriptionGroupHandler", -> admin_id:@adminUser_id _id:@subscription_id - @SubscriptionLocator = + @SubscriptionLocator = getUsersSubscription: sinon.stub() getSubscriptionByMemberIdAndId: sinon.stub() getSubscription: sinon.stub() - @UserCreator = + @UserCreator = getUserOrCreateHoldingAccount: sinon.stub().callsArgWith(1, null, @user) @SubscriptionUpdater = @@ -47,7 +47,10 @@ describe "SubscriptionGroupHandler", -> @EmailHandler = sendEmail:sinon.stub() - @settings = + @Subscription = + update: sinon.stub().yields() + + @settings = siteUrl:"http://www.sharelatex.com" @readStub = sinon.stub() @@ -59,13 +62,14 @@ describe "SubscriptionGroupHandler", -> "../User/UserCreator": @UserCreator "./SubscriptionUpdater": @SubscriptionUpdater "./SubscriptionLocator": @SubscriptionLocator + "../../models/Subscription": Subscription: @Subscription "../User/UserLocator": @UserLocator "./LimitationsManager": @LimitationsManager "../Security/OneTimeTokenHandler":@OneTimeTokenHandler "../Email/EmailHandler":@EmailHandler "settings-sharelatex":@settings "../Notifications/NotificationsBuilder": @NotificationsBuilder - "logger-sharelatex": + "logger-sharelatex": err:-> log:-> warn:-> @@ -75,7 +79,7 @@ describe "SubscriptionGroupHandler", -> beforeEach -> @LimitationsManager.hasGroupMembersLimitReached.callsArgWith(1, null, false, @subscription) @UserLocator.findByEmail.callsArgWith(1, null, @user) - + it "should find the user", (done)-> @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> @UserLocator.findByEmail.calledWith(@newEmail).should.equal true @@ -85,7 +89,7 @@ describe "SubscriptionGroupHandler", -> @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> @SubscriptionUpdater.addUserToGroup.calledWith(@adminUser_id, @user._id).should.equal true done() - + it "should not add the user to the group if the limit has been reached", (done)-> @LimitationsManager.hasGroupMembersLimitReached.callsArgWith(1, null, true, @subscription) @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> @@ -103,7 +107,7 @@ describe "SubscriptionGroupHandler", -> @NotificationsBuilder.groupPlan.calledWith(@user, {subscription_id:@subscription._id}).should.equal true @readStub.called.should.equal true done() - + it "should add an email invite if no user is found", (done) -> @UserLocator.findByEmail.callsArgWith(1, null, null) @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> @@ -117,6 +121,35 @@ describe "SubscriptionGroupHandler", -> @SubscriptionUpdater.removeUserFromGroup.calledWith(@adminUser_id, @user._id).should.equal true done() + describe "replaceUserReferencesInGroups", -> + beforeEach -> + @oldId = "ba5eba11" + @newId = "5ca1ab1e" + + it "replaces the admin_id", (done) -> + @Handler.replaceUserReferencesInGroups @oldId, @newId, (err) => + + @Subscription.update.calledWith( + { admin_id: @oldId }, + { admin_id: @newId } + ).should.equal true + + done() + + it "replaces the member ids", (done) -> + @Handler.replaceUserReferencesInGroups @oldId, @newId, (err) => + + @Subscription.update.calledWith( + { member_ids: @oldId }, + { $addToSet: { member_ids: @newId } } + ).should.equal true + + @Subscription.update.calledWith( + { member_ids: @oldId }, + { $pull: { member_ids: @oldId } } + ).should.equal true + + done() describe "getPopulatedListOfMembers", -> beforeEach -> @@ -148,7 +181,7 @@ describe "SubscriptionGroupHandler", -> assert.deepEqual users[1], {_id:@subscription.member_ids[1]} assert.deepEqual users[2], {_id:@subscription.member_ids[2]} done() - + it "should return any invited users", (done) -> @subscription.invited_emails = ["jo@example.com", "charlie@example.com"] @Handler.getPopulatedListOfMembers @adminUser_id, (err, users)=> @@ -219,7 +252,7 @@ describe "SubscriptionGroupHandler", -> @Handler.convertEmailInvitesToMemberships @email, @user_id, (err) => @SubscriptionLocator.getGroupsWithEmailInvite.calledWith(@email).should.equal true done() - + it "should remove the email from each group", (done) -> @Handler.convertEmailInvitesToMemberships @email, @user_id, (err) => for group in @groups @@ -227,7 +260,7 @@ describe "SubscriptionGroupHandler", -> .calledWith(group.admin_id, @email) .should.equal true done() - + it "should add the user to each group", (done) -> @Handler.convertEmailInvitesToMemberships @email, @user_id, (err) => for group in @groups @@ -235,4 +268,3 @@ describe "SubscriptionGroupHandler", -> .calledWith(group.admin_id, @user_id) .should.equal true done() - From 301ae80f99a05869fe587b0b1b801baca5578af2 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 29 May 2018 17:21:42 +0100 Subject: [PATCH 05/30] Allow refreshFeatures to notify v1 to update its features --- .../Subscription/FeaturesUpdater.coffee | 15 ++++- .../Subscription/V1SubscriptionManager.coffee | 67 +++++++++++-------- ...ests.coffee => FeatureUpdaterTests.coffee} | 37 ++++++---- .../coffee/helpers/MockV1Api.coffee | 13 +++- .../V1SusbcriptionManagerTests.coffee | 60 ++++++++--------- 5 files changed, 114 insertions(+), 78 deletions(-) rename services/web/test/acceptance/coffee/{SubscriptionTests.coffee => FeatureUpdaterTests.coffee} (85%) diff --git a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee index 5c176c611f..28da0ef2f7 100644 --- a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee +++ b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee @@ -11,7 +11,16 @@ V1SubscriptionManager = require("./V1SubscriptionManager") oneMonthInSeconds = 60 * 60 * 24 * 30 module.exports = FeaturesUpdater = - refreshFeatures: (user_id, callback)-> + refreshFeatures: (user_id, notifyV1, callback)-> + if !callback? + callback = notifyV1 + notifyV1 = false + + if notifyV1 + V1SubscriptionManager.notifyV1OfFeaturesChange user_id, (error) -> + if error? + logger.err {err: error, user_id}, "error notifying v1 about updated features" + jobs = individualFeatures: (cb) -> FeaturesUpdater._getIndividualFeatures user_id, cb groupFeatureSets: (cb) -> FeaturesUpdater._getGroupFeatureSets user_id, cb @@ -80,4 +89,6 @@ module.exports = FeaturesUpdater = if !plan? return {} else - return plan.features \ No newline at end of file + return plan.features + + _notifyV1: (user_id, callback = (error) ->) -> diff --git a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee index 05dc140be2..f297984dd3 100644 --- a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee +++ b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee @@ -12,39 +12,50 @@ module.exports = V1SubscriptionManager = # - 'v1_free' getPlanCodeFromV1: (userId, callback=(err, planCode)->) -> logger.log {userId}, "[V1SubscriptionManager] fetching v1 plan for user" + V1SubscriptionManager._v1Request userId, { + method: 'GET', + url: (v1Id) -> "/api/v1/sharelatex/users/#{v1Id}/plan_code" + }, (error, body) -> + return callback(error) if error? + planName = body?.plan_name + logger.log {userId, planName, body}, "[V1SubscriptionManager] fetched v1 plan for user" + if planName in ['pro', 'pro_plus', 'student', 'free'] + planName = "v1_#{planName}" + else + # Throw away 'anonymous', etc as being equivalent to null + planName = null + return callback(null, planName) + + notifyV1OfFeaturesChange: (userId, callback = (error) ->) -> + V1SubscriptionManager._v1Request userId, { + method: 'POST', + url: (v1Id) -> "/api/v1/sharelatex/users/#{v1Id}/sync" + }, callback + + _v1Request: (userId, options, callback=(err, body)->) -> + if !settings?.apis?.v1 + return callback null, null UserGetter.getUser userId, {'overleaf.id': 1}, (err, user) -> return callback(err) if err? v1Id = user?.overleaf?.id if !v1Id? logger.log {userId}, "[V1SubscriptionManager] no v1 id found for user" return callback(null, null) - V1SubscriptionManager._v1PlanRequest v1Id, (err, body) -> - return callback(err) if err? - planName = body?.plan_name - logger.log {userId, planName, body}, "[V1SubscriptionManager] fetched v1 plan for user" - if planName in ['pro', 'pro_plus', 'student', 'free'] - planName = "v1_#{planName}" + options.url = options.url + request { + baseUrl: settings.apis.v1.url + url: options.url(v1Id) + method: options.method + auth: + user: settings.apis.v1.user + pass: settings.apis.v1.pass + sendImmediately: true + json: true, + timeout: 5 * 1000 + }, (error, response, body) -> + return callback(error) if error? + if 200 <= response.statusCode < 300 + return callback null, body else - # Throw away 'anonymous', etc as being equivalent to null - planName = null - return callback(null, planName) + return callback new Error("non-success code from v1: #{response.statusCode}") - _v1PlanRequest: (v1Id, callback=(err, body)->) -> - if !settings?.apis?.v1 - return callback null, null - request { - method: 'GET', - url: settings.apis.v1.url + - "/api/v1/sharelatex/users/#{v1Id}/plan_code" - auth: - user: settings.apis.v1.user - pass: settings.apis.v1.pass - sendImmediately: true - json: true, - timeout: 5 * 1000 - }, (error, response, body) -> - return callback(error) if error? - if 200 <= response.statusCode < 300 - return callback null, body - else - return callback new Error("non-success code from v1: #{response.statusCode}") \ No newline at end of file diff --git a/services/web/test/acceptance/coffee/SubscriptionTests.coffee b/services/web/test/acceptance/coffee/FeatureUpdaterTests.coffee similarity index 85% rename from services/web/test/acceptance/coffee/SubscriptionTests.coffee rename to services/web/test/acceptance/coffee/FeatureUpdaterTests.coffee index ce762ea0ee..3264f4aeae 100644 --- a/services/web/test/acceptance/coffee/SubscriptionTests.coffee +++ b/services/web/test/acceptance/coffee/FeatureUpdaterTests.coffee @@ -6,27 +6,22 @@ settings = require "settings-sharelatex" {ObjectId} = require("../../../app/js/infrastructure/mongojs") Subscription = require("../../../app/js/models/Subscription").Subscription User = require("../../../app/js/models/User").User +FeaturesUpdater = require("../../../app/js/Features/Subscription/FeaturesUpdater") MockV1Api = require "./helpers/MockV1Api" +logger = require "logger-sharelatex" +logger.logger.level("error") syncUserAndGetFeatures = (user, callback = (error, features) ->) -> - request { - method: 'POST', - url: "/user/#{user._id}/features/sync", - auth: - user: 'sharelatex' - pass: 'password' - sendImmediately: true - }, (error, response, body) -> - throw error if error? - expect(response.statusCode).to.equal 200 + FeaturesUpdater.refreshFeatures user._id, (error) -> + return callback(error) if error? User.findById user._id, (error, user) -> return callback(error) if error? features = user.toObject().features delete features.$init # mongoose internals return callback null, features -describe "Subscriptions", -> +describe "FeatureUpdater.refreshFeatures", -> beforeEach (done) -> @user = new UserClient() @user.ensureUserExists (error) -> @@ -148,4 +143,22 @@ describe "Subscriptions", -> throw error if error? plan = settings.plans.find (plan) -> plan.planCode == 'professional' expect(features).to.deep.equal(plan.features) - done() \ No newline at end of file + done() + + describe "when the notifyV1Flag is passed", -> + beforeEach -> + User.update { + _id: @user._id + }, { + overleaf: + id: 42 + } # returns a promise + + it "should ping the v1 API end point to sync", (done) -> + FeaturesUpdater.refreshFeatures @user._id, true, (error) => + setTimeout () => + expect( + MockV1Api.syncUserFeatures.calledWith('42') + ).to.equal true + done() + , 500 diff --git a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee index 5c2cf47ad9..389ecd1762 100644 --- a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee +++ b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee @@ -1,6 +1,7 @@ express = require("express") app = express() bodyParser = require('body-parser') +sinon = require 'sinon' app.use(bodyParser.json()) @@ -23,19 +24,25 @@ module.exports = MockV1Api = clearExportParams: () -> @exportParams = null + syncUserFeatures: sinon.stub() + run: () -> - app.get "/api/v1/sharelatex/users/:ol_user_id/plan_code", (req, res, next) => - user = @users[req.params.ol_user_id] + app.get "/api/v1/sharelatex/users/:v1_user_id/plan_code", (req, res, next) => + user = @users[req.params.v1_user_id] if user res.json user else res.sendStatus 404 + app.post "/api/v1/sharelatex/users/:v1_user_id/sync", (req, res, next) => + @syncUserFeatures(req.params.v1_user_id) + res.sendStatus 200 + app.post "/api/v1/sharelatex/exports", (req, res, next) => - #{project, version, pathname} @exportParams = Object.assign({}, req.body) res.json exportId: @exportId + app.listen 5000, (error) -> throw error if error? .on "error", (error) -> diff --git a/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee b/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee index ae6237e627..9d85d81f84 100644 --- a/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee @@ -16,10 +16,10 @@ describe 'V1SubscriptionManager', -> err: sinon.stub() warn: sinon.stub() "settings-sharelatex": - overleaf: - host: @host = "http://overleaf.example.com" + apis: + v1: + host: @host = "http://overleaf.example.com" "request": @request = sinon.stub() - @V1SubscriptionManager._v1PlanRequest = sinon.stub() @userId = 'abcd' @v1UserId = 42 @user = @@ -33,33 +33,20 @@ describe 'V1SubscriptionManager', -> @responseBody = id: 32, plan_name: 'pro' - @UserGetter.getUser = sinon.stub() - .yields(null, @user) - @V1SubscriptionManager._v1PlanRequest = sinon.stub() + @V1SubscriptionManager._v1Request = sinon.stub() .yields(null, @responseBody) @call = (cb) => @V1SubscriptionManager.getPlanCodeFromV1 @userId, cb describe 'when all goes well', -> - - it 'should call getUser', (done) -> + it 'should call _v1Request', (done) -> @call (err, planCode) => expect( - @UserGetter.getUser.callCount + @V1SubscriptionManager._v1Request.callCount ).to.equal 1 expect( - @UserGetter.getUser.calledWith(@userId) - ).to.equal true - done() - - it 'should call _v1PlanRequest', (done) -> - @call (err, planCode) => - expect( - @V1SubscriptionManager._v1PlanRequest.callCount - ).to.equal 1 - expect( - @V1SubscriptionManager._v1PlanRequest.calledWith( - @v1UserId + @V1SubscriptionManager._v1Request.calledWith( + @userId ) ).to.equal true done() @@ -80,49 +67,56 @@ describe 'V1SubscriptionManager', -> expect(planCode).to.equal null done() + describe '_v1Request', -> + beforeEach -> + @UserGetter.getUser = sinon.stub() + .yields(null, @user) + describe 'when getUser produces an error', -> beforeEach -> @UserGetter.getUser = sinon.stub() .yields(new Error('woops')) + @call = (cb) => + @V1SubscriptionManager._v1Request @user_id, { url: () -> '/foo' }, cb - it 'should not call _v1PlanRequest', (done) -> + it 'should not call request', (done) -> @call (err, planCode) => expect( - @V1SubscriptionManager._v1PlanRequest.callCount + @request.callCount ).to.equal 0 done() it 'should produce an error', (done) -> @call (err, planCode) => expect(err).to.exist - expect(planCode).to.not.exist done() describe 'when getUser does not find a user', -> beforeEach -> @UserGetter.getUser = sinon.stub() .yields(null, null) + @call = (cb) => + @V1SubscriptionManager._v1Request @user_id, { url: () -> '/foo' }, cb - it 'should not call _v1PlanRequest', (done) -> + it 'should not call request', (done) -> @call (err, planCode) => expect( - @V1SubscriptionManager._v1PlanRequest.callCount + @request.callCount ).to.equal 0 done() - it 'should produce a null plan-code, without error', (done) -> - @call (err, planCode) => + it 'should not error', (done) -> + @call (err) => expect(err).to.not.exist - expect(planCode).to.not.exist done() describe 'when the request to v1 fails', -> beforeEach -> - @V1SubscriptionManager._v1PlanRequest = sinon.stub() - .yields(new Error('woops')) + @request.yields(new Error('woops')) + @call = (cb) => + @V1SubscriptionManager._v1Request @user_id, { url: () -> '/foo' }, cb it 'should produce an error', (done) -> - @call (err, planCode) => + @call (err) => expect(err).to.exist - expect(planCode).to.not.exist done() From c5b553d4a69a87ce22e4c6e87a32bea968d614a2 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 29 May 2018 17:31:15 +0100 Subject: [PATCH 06/30] Notify v1 by default --- .../Subscription/FeaturesUpdater.coffee | 2 +- .../coffee/FeatureUpdaterTests.coffee | 2 +- .../Subscription/FeaturesUpdaterTests.coffee | 80 +++++++++++-------- 3 files changed, 50 insertions(+), 34 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee index 28da0ef2f7..3a505167d7 100644 --- a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee +++ b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee @@ -14,7 +14,7 @@ module.exports = FeaturesUpdater = refreshFeatures: (user_id, notifyV1, callback)-> if !callback? callback = notifyV1 - notifyV1 = false + notifyV1 = true if notifyV1 V1SubscriptionManager.notifyV1OfFeaturesChange user_id, (error) -> diff --git a/services/web/test/acceptance/coffee/FeatureUpdaterTests.coffee b/services/web/test/acceptance/coffee/FeatureUpdaterTests.coffee index 3264f4aeae..5bc3026fff 100644 --- a/services/web/test/acceptance/coffee/FeatureUpdaterTests.coffee +++ b/services/web/test/acceptance/coffee/FeatureUpdaterTests.coffee @@ -13,7 +13,7 @@ logger = require "logger-sharelatex" logger.logger.level("error") syncUserAndGetFeatures = (user, callback = (error, features) ->) -> - FeaturesUpdater.refreshFeatures user._id, (error) -> + FeaturesUpdater.refreshFeatures user._id, false, (error) -> return callback(error) if error? User.findById user._id, (error, user) -> return callback(error) if error? diff --git a/services/web/test/unit/coffee/Subscription/FeaturesUpdaterTests.coffee b/services/web/test/unit/coffee/Subscription/FeaturesUpdaterTests.coffee index c8f77bfd40..489c5676ff 100644 --- a/services/web/test/unit/coffee/Subscription/FeaturesUpdaterTests.coffee +++ b/services/web/test/unit/coffee/Subscription/FeaturesUpdaterTests.coffee @@ -22,6 +22,7 @@ describe "FeaturesUpdater", -> describe "refreshFeatures", -> beforeEach -> + @V1SubscriptionManager.notifyV1OfFeaturesChange = sinon.stub().yields() @UserFeaturesUpdater.updateFeatures = sinon.stub().yields() @FeaturesUpdater._getIndividualFeatures = sinon.stub().yields(null, { 'individual': 'features' }) @FeaturesUpdater._getGroupFeatureSets = sinon.stub().yields(null, [{ 'group': 'features' }, { 'group': 'features2' }]) @@ -29,48 +30,63 @@ describe "FeaturesUpdater", -> @ReferalFeatures.getBonusFeatures = sinon.stub().yields(null, { 'bonus': 'features' }) @FeaturesUpdater._mergeFeatures = sinon.stub().returns({'merged': 'features'}) @callback = sinon.stub() - @FeaturesUpdater.refreshFeatures @user_id, @callback - it "should get the individual features", -> - @FeaturesUpdater._getIndividualFeatures - .calledWith(@user_id) - .should.equal true + describe "normally", -> + beforeEach -> + @FeaturesUpdater.refreshFeatures @user_id, @callback - it "should get the group features", -> - @FeaturesUpdater._getGroupFeatureSets - .calledWith(@user_id) - .should.equal true + it "should get the individual features", -> + @FeaturesUpdater._getIndividualFeatures + .calledWith(@user_id) + .should.equal true - it "should get the v1 features", -> - @FeaturesUpdater._getV1Features - .calledWith(@user_id) - .should.equal true + it "should get the group features", -> + @FeaturesUpdater._getGroupFeatureSets + .calledWith(@user_id) + .should.equal true - it "should get the bonus features", -> - @ReferalFeatures.getBonusFeatures - .calledWith(@user_id) - .should.equal true + it "should get the v1 features", -> + @FeaturesUpdater._getV1Features + .calledWith(@user_id) + .should.equal true - it "should merge from the default features", -> - @FeaturesUpdater._mergeFeatures.calledWith(@Settings.defaultFeatures).should.equal true + it "should get the bonus features", -> + @ReferalFeatures.getBonusFeatures + .calledWith(@user_id) + .should.equal true - it "should merge the individual features", -> - @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'individual': 'features' }).should.equal true + it "should merge from the default features", -> + @FeaturesUpdater._mergeFeatures.calledWith(@Settings.defaultFeatures).should.equal true - it "should merge the group features", -> - @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'group': 'features' }).should.equal true - @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'group': 'features2' }).should.equal true + it "should merge the individual features", -> + @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'individual': 'features' }).should.equal true - it "should merge the v1 features", -> - @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'v1': 'features' }).should.equal true + it "should merge the group features", -> + @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'group': 'features' }).should.equal true + @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'group': 'features2' }).should.equal true - it "should merge the bonus features", -> - @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'bonus': 'features' }).should.equal true + it "should merge the v1 features", -> + @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'v1': 'features' }).should.equal true - it "should update the user with the merged features", -> - @UserFeaturesUpdater.updateFeatures - .calledWith(@user_id, {'merged': 'features'}) - .should.equal true + it "should merge the bonus features", -> + @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'bonus': 'features' }).should.equal true + + it "should update the user with the merged features", -> + @UserFeaturesUpdater.updateFeatures + .calledWith(@user_id, {'merged': 'features'}) + .should.equal true + + it "should notify v1", -> + @V1SubscriptionManager.notifyV1OfFeaturesChange + .called.should.equal true + + describe "with notifyV1 == false", -> + beforeEach -> + @FeaturesUpdater.refreshFeatures @user_id, false, @callback + + it "should not notify v1", -> + @V1SubscriptionManager.notifyV1OfFeaturesChange + .called.should.equal false describe "_mergeFeatures", -> it "should prefer priority over standard for compileGroup", -> From e0e88b25fe174c5f7d16862904e6ce649310b942 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 30 May 2018 12:48:08 +0100 Subject: [PATCH 07/30] Make agrument checking more robust --- .../app/coffee/Features/Subscription/FeaturesUpdater.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee index 3a505167d7..dc7571ea50 100644 --- a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee +++ b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee @@ -11,8 +11,8 @@ V1SubscriptionManager = require("./V1SubscriptionManager") oneMonthInSeconds = 60 * 60 * 24 * 30 module.exports = FeaturesUpdater = - refreshFeatures: (user_id, notifyV1, callback)-> - if !callback? + refreshFeatures: (user_id, notifyV1 = true, callback = () ->)-> + if typeof notifyV1 == 'function' callback = notifyV1 notifyV1 = true From cfbb5c8f24be4769f6804233d37946ee2e7eb7ff Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 1 Jun 2018 14:55:07 +0100 Subject: [PATCH 08/30] Remove some dead code --- .../web/app/coffee/Features/Subscription/FeaturesUpdater.coffee | 2 -- .../coffee/Features/Subscription/V1SubscriptionManager.coffee | 1 - 2 files changed, 3 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee index dc7571ea50..5627072c93 100644 --- a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee +++ b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee @@ -90,5 +90,3 @@ module.exports = FeaturesUpdater = return {} else return plan.features - - _notifyV1: (user_id, callback = (error) ->) -> diff --git a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee index f297984dd3..e920b94f6c 100644 --- a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee +++ b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee @@ -41,7 +41,6 @@ module.exports = V1SubscriptionManager = if !v1Id? logger.log {userId}, "[V1SubscriptionManager] no v1 id found for user" return callback(null, null) - options.url = options.url request { baseUrl: settings.apis.v1.url url: options.url(v1Id) From 4c5f186ca24bc213579fcfbc1879c6a5a994262b Mon Sep 17 00:00:00 2001 From: hugh-obrien Date: Mon, 4 Jun 2018 11:05:47 +0100 Subject: [PATCH 09/30] take custom first and last names from export UI --- .../Features/Exports/ExportsController.coffee | 12 +++++++++++- .../coffee/Features/Exports/ExportsHandler.coffee | 13 ++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/services/web/app/coffee/Features/Exports/ExportsController.coffee b/services/web/app/coffee/Features/Exports/ExportsController.coffee index b60f58ba20..c2a8d0d617 100644 --- a/services/web/app/coffee/Features/Exports/ExportsController.coffee +++ b/services/web/app/coffee/Features/Exports/ExportsController.coffee @@ -7,7 +7,17 @@ module.exports = exportProject: (req, res) -> {project_id, brand_variation_id} = req.params user_id = AuthenticationController.getLoggedInUserId(req) - ExportsHandler.exportProject project_id, user_id, brand_variation_id, (err, export_data) -> + export_params = { + project_id: project_id, + brand_variation_id: brand_variation_id, + user_id: user_id + } + + if req.body && req.body.firstName && req.body.firstName + export_params.first_name = req.body.firstName + export_params.last_name = req.body.lastName + + ExportsHandler.exportProject export_params, (err, export_data) -> return next(err) if err? logger.log user_id:user_id diff --git a/services/web/app/coffee/Features/Exports/ExportsHandler.coffee b/services/web/app/coffee/Features/Exports/ExportsHandler.coffee index 727b01a575..38357c129d 100644 --- a/services/web/app/coffee/Features/Exports/ExportsHandler.coffee +++ b/services/web/app/coffee/Features/Exports/ExportsHandler.coffee @@ -10,8 +10,8 @@ settings = require 'settings-sharelatex' module.exports = ExportsHandler = self = - exportProject: (project_id, user_id, brand_variation_id, callback=(error, export_data) ->) -> - self._buildExport project_id, user_id, brand_variation_id, (err, export_data) -> + exportProject: (export_params, callback=(error, export_data) ->) -> + self._buildExport export_params, (err, export_data) -> return callback(err) if err? self._requestExport export_data, (err, export_v1_id) -> return callback(err) if err? @@ -19,7 +19,10 @@ module.exports = ExportsHandler = self = # TODO: possibly store the export data in Mongo callback null, export_data - _buildExport: (project_id, user_id, brand_variation_id, callback=(err, export_data) ->) -> + _buildExport: (export_params, callback=(err, export_data) ->) -> + project_id = export_params.project_id + user_id = export_params.user_id + brand_variation_id = export_params.brand_variation_id jobs = project: (cb) -> ProjectGetter.getProject project_id, cb @@ -43,6 +46,10 @@ module.exports = ExportsHandler = self = logger.err err:err, project_id: project_id return callback(err) + if export_params.first_name && export_params.last_name + user.first_name = export_params.first_name + user.last_name = export_params.last_name + export_data = project: id: project_id From b02eea1e7e86dea533ebc9be89565101625eb8b9 Mon Sep 17 00:00:00 2001 From: hugh-obrien Date: Mon, 4 Jun 2018 11:07:47 +0100 Subject: [PATCH 10/30] update tests for exports name options --- .../coffee/Exports/ExportsHandlerTests.coffee | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee b/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee index 6333db8270..8589de25c4 100644 --- a/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee +++ b/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee @@ -27,6 +27,11 @@ describe 'ExportsHandler', -> @project_history_id = 987 @user_id = "user-id-456" @brand_variation_id = 789 + @export_params = { + project_id: @project_id, + brand_variation_id: @brand_variation_id, + user_id: @user_id + } @callback = sinon.stub() describe 'exportProject', -> @@ -35,13 +40,13 @@ describe 'ExportsHandler', -> @response_body = {iAmAResponseBody: true} @ExportsHandler._buildExport = sinon.stub().yields(null, @export_data) @ExportsHandler._requestExport = sinon.stub().yields(null, @response_body) - @ExportsHandler.exportProject @project_id, @user_id, @brand_variation_id, (error, export_data) => + @ExportsHandler.exportProject @export_params, (error, export_data) => @callback(error, export_data) done() it "should build the export", -> @ExportsHandler._buildExport - .calledWith(@project_id, @user_id, @brand_variation_id) + .calledWith(@export_params) .should.equal true it "should request the export", -> @@ -76,7 +81,7 @@ describe 'ExportsHandler', -> describe "when all goes well", -> beforeEach (done) -> - @ExportsHandler._buildExport @project_id, @user_id, @brand_variation_id, (error, export_data) => + @ExportsHandler._buildExport @export_params, (error, export_data) => @callback(error, export_data) done() @@ -104,10 +109,11 @@ describe 'ExportsHandler', -> @callback.calledWith(null, expected_export_data) .should.equal true + describe "when project is not found", -> beforeEach (done) -> @ProjectGetter.getProject = sinon.stub().yields(new Error("project not found")) - @ExportsHandler._buildExport @project_id, @user_id, @brand_variation_id, (error, export_data) => + @ExportsHandler._buildExport @export_params, (error, export_data) => @callback(error, export_data) done() @@ -118,7 +124,7 @@ describe 'ExportsHandler', -> describe "when project has no root doc", -> beforeEach (done) -> @ProjectLocator.findRootDoc = sinon.stub().yields(null, [null, null]) - @ExportsHandler._buildExport @project_id, @user_id, @brand_variation_id, (error, export_data) => + @ExportsHandler._buildExport @export_params, (error, export_data) => @callback(error, export_data) done() @@ -129,7 +135,7 @@ describe 'ExportsHandler', -> describe "when user is not found", -> beforeEach (done) -> @UserGetter.getUser = sinon.stub().yields(new Error("user not found")) - @ExportsHandler._buildExport @project_id, @user_id, @brand_variation_id, (error, export_data) => + @ExportsHandler._buildExport @export_params, (error, export_data) => @callback(error, export_data) done() @@ -140,7 +146,7 @@ describe 'ExportsHandler', -> describe "when project history request fails", -> beforeEach (done) -> @ExportsHandler._requestVersion = sinon.stub().yields(new Error("project history call failed")) - @ExportsHandler._buildExport @project_id, @user_id, @brand_variation_id, (error, export_data) => + @ExportsHandler._buildExport @export_params, (error, export_data) => @callback(error, export_data) done() From 54ce19650035b06daf0f938010ef3fd7bc02d87b Mon Sep 17 00:00:00 2001 From: hugh-obrien Date: Mon, 4 Jun 2018 11:11:40 +0100 Subject: [PATCH 11/30] test custom first and last name for exports --- .../Features/Exports/ExportsController.coffee | 4 +-- .../coffee/Exports/ExportsHandlerTests.coffee | 29 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/Exports/ExportsController.coffee b/services/web/app/coffee/Features/Exports/ExportsController.coffee index c2a8d0d617..393ca95fdb 100644 --- a/services/web/app/coffee/Features/Exports/ExportsController.coffee +++ b/services/web/app/coffee/Features/Exports/ExportsController.coffee @@ -14,8 +14,8 @@ module.exports = } if req.body && req.body.firstName && req.body.firstName - export_params.first_name = req.body.firstName - export_params.last_name = req.body.lastName + export_params.first_name = req.body.firstName.trim() + export_params.last_name = req.body.lastName.trim() ExportsHandler.exportProject export_params, (err, export_data) -> return next(err) if err? diff --git a/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee b/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee index 8589de25c4..f10f4631c1 100644 --- a/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee +++ b/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee @@ -109,6 +109,35 @@ describe 'ExportsHandler', -> @callback.calledWith(null, expected_export_data) .should.equal true + describe "when we send replacement user first and last name", -> + beforeEach (done) -> + @custom_first_name = "FIRST" + @custom_last_name = "LAST" + @export_params.first_name = @custom_first_name + @export_params.last_name = @custom_last_name + @ExportsHandler._buildExport @export_params, (error, export_data) => + @callback(error, export_data) + done() + + it "should send the data from the user input", -> + expected_export_data = + project: + id: @project_id + rootDocPath: @rootDocPath + historyId: @project_history_id + historyVersion: @historyVersion + user: + id: @user_id + firstName: @custom_first_name + lastName: @custom_last_name + email: @user.email + orcidId: null + destination: + brandVariationId: @brand_variation_id + options: + callbackUrl: null + @callback.calledWith(null, expected_export_data) + .should.equal true describe "when project is not found", -> beforeEach (done) -> From be0fd9a446f1f0a01170b52610f5774129e7351a Mon Sep 17 00:00:00 2001 From: Hayden Faulds Date: Tue, 5 Jun 2018 11:30:48 +0100 Subject: [PATCH 12/30] reduce container teardown timeout to 0 --- services/web/Makefile | 6 +++--- services/web/Makefile.module | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/services/web/Makefile b/services/web/Makefile index 469ab8e919..bfdfc0f52f 100644 --- a/services/web/Makefile +++ b/services/web/Makefile @@ -180,7 +180,7 @@ clean_css: rm -f public/stylesheets/*.css* clean_ci: - docker-compose down -v + docker-compose down -v -t 0 test: test_unit test_frontend test_acceptance @@ -204,7 +204,7 @@ test_acceptance_app_start_service: test_clean # stop service and clear dbs docker-compose ${DOCKER_COMPOSE_FLAGS} up -d test_acceptance test_acceptance_app_stop_service: - docker-compose ${DOCKER_COMPOSE_FLAGS} stop test_acceptance redis mongo + docker-compose ${DOCKER_COMPOSE_FLAGS} stop -t 0 test_acceptance redis mongo test_acceptance_app_run: docker-compose ${DOCKER_COMPOSE_FLAGS} exec -T test_acceptance npm -q run test:acceptance -- ${MOCHA_ARGS} @@ -224,7 +224,7 @@ test_acceptance_module: $(MODULE_MAKEFILES) fi test_clean: - docker-compose ${DOCKER_COMPOSE_FLAGS} down -v + docker-compose ${DOCKER_COMPOSE_FLAGS} down -v -t 0 ci: MOCHA_ARGS="--reporter tap" \ diff --git a/services/web/Makefile.module b/services/web/Makefile.module index 00bea5f8da..fa1ba41679 100644 --- a/services/web/Makefile.module +++ b/services/web/Makefile.module @@ -62,7 +62,7 @@ test_acceptance_start_service: test_acceptance_stop_service $(DOCKER_COMPOSE) up -d test_acceptance test_acceptance_stop_service: - $(DOCKER_COMPOSE) stop test_acceptance redis mongo + $(DOCKER_COMPOSE) stop -t 0 test_acceptance redis mongo test_acceptance_run: $(DOCKER_COMPOSE) exec -T test_acceptance npm -q run test:acceptance:dir -- ${MOCHA_ARGS} $(MODULE_DIR)/test/acceptance/js From c5530163f54893e3d6ca9cf8aae29ac9b1af3c88 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Mon, 28 May 2018 16:08:37 +0200 Subject: [PATCH 13/30] add emails attribute on user creation --- .../coffee/Features/User/UserCreator.coffee | 4 +++ .../coffee/Features/User/UserGetter.coffee | 9 ++++++- .../User/UserRegistrationHandler.coffee | 2 +- .../coffee/Features/User/UserUpdater.coffee | 11 +------- services/web/app/coffee/models/User.coffee | 4 +++ .../coffee/RegistrationTests.coffee | 14 ++++++++++ .../acceptance/coffee/helpers/User.coffee | 27 +++++++++++++++---- .../unit/coffee/User/UserCreatorTests.coffee | 8 ++++++ .../unit/coffee/User/UserGetterTests.coffee | 17 ++++++++++++ .../User/UserRegistrationHandlerTests.coffee | 8 +++--- .../unit/coffee/User/UserUpdaterTests.coffee | 17 +++--------- 11 files changed, 86 insertions(+), 35 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserCreator.coffee b/services/web/app/coffee/Features/User/UserCreator.coffee index 0a0cc8641e..4b4dd30ae2 100644 --- a/services/web/app/coffee/Features/User/UserCreator.coffee +++ b/services/web/app/coffee/Features/User/UserCreator.coffee @@ -18,6 +18,10 @@ module.exports = UserCreator = user.ace.syntaxValidation = true user.featureSwitches?.pdfng = true + user.emails = [ + email: user.email + createdAt: new Date() + ] user.save (err)-> callback(err, user) diff --git a/services/web/app/coffee/Features/User/UserGetter.coffee b/services/web/app/coffee/Features/User/UserGetter.coffee index 10a975613e..2d55d0e432 100644 --- a/services/web/app/coffee/Features/User/UserGetter.coffee +++ b/services/web/app/coffee/Features/User/UserGetter.coffee @@ -62,12 +62,19 @@ module.exports = UserGetter = return callback(null, user) if user? db.userstubs.findOne query, projection, callback + # check for duplicate email address. This is also enforced at the DB level + ensureUniqueEmailAddress: (newEmail, callback) -> + @getUserByAnyEmail newEmail, (error, user) -> + return callback(message: 'alread_exists') if user? + callback(error) + [ 'getUser', 'getUserEmail', 'getUserByMainEmail', 'getUserByAnyEmail', 'getUsers', - 'getUserOrUserStubById' + 'getUserOrUserStubById', + 'ensureUniqueEmailAddress', ].map (method) -> metrics.timeAsyncMethod UserGetter, method, 'mongo.UserGetter', logger diff --git a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index fab438ffa6..4a42d7f5fa 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee @@ -48,7 +48,7 @@ module.exports = UserRegistrationHandler = if !requestIsValid return callback(new Error("request is not valid")) userDetails.email = userDetails.email?.trim()?.toLowerCase() - UserGetter.getUserByMainEmail userDetails.email, (err, user) => + UserGetter.getUserByAnyEmail userDetails.email, (err, user) => if err? return callback err if user?.holdingAccount == false diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index fa9ee24450..22b31239bd 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -43,7 +43,7 @@ module.exports = UserUpdater = # Add a new email address for the user. Email cannot be already used by this # or any other user addEmailAddress: (userId, newEmail, callback) -> - @_ensureUniqueEmailAddress newEmail, (error) => + UserGetter.ensureUniqueEmailAddress newEmail, (error) => return callback(error) if error? update = $push: emails: email: newEmail, createdAt: new Date() @@ -81,20 +81,11 @@ module.exports = UserUpdater = return callback(new Error('Default email does not belong to user')) callback() - - # check for duplicate email address. This is also enforced at the DB level - _ensureUniqueEmailAddress: (newEmail, callback) -> - UserGetter.getUserByAnyEmail newEmail, (error, user) -> - return callback(message: 'alread_exists') if user? - callback() - - [ 'updateUser' 'changeEmailAddress' 'setDefaultEmailAddress' 'addEmailAddress' 'removeEmailAddress' - '_ensureUniqueEmailAddress' ].map (method) -> metrics.timeAsyncMethod(UserUpdater, method, 'mongo.UserUpdater', logger) diff --git a/services/web/app/coffee/models/User.coffee b/services/web/app/coffee/models/User.coffee index 009f582b2a..71982ba40b 100644 --- a/services/web/app/coffee/models/User.coffee +++ b/services/web/app/coffee/models/User.coffee @@ -8,6 +8,10 @@ ObjectId = Schema.ObjectId UserSchema = new Schema email : {type : String, default : ''} + emails: [{ + email: { type : String, default : '' }, + createdAt: { type : Date, default: () -> new Date() } + }], first_name : {type : String, default : ''} last_name : {type : String, default : ''} role : {type : String, default : ''} diff --git a/services/web/test/acceptance/coffee/RegistrationTests.coffee b/services/web/test/acceptance/coffee/RegistrationTests.coffee index 89d1bf3299..8bcf8c3685 100644 --- a/services/web/test/acceptance/coffee/RegistrationTests.coffee +++ b/services/web/test/acceptance/coffee/RegistrationTests.coffee @@ -128,6 +128,20 @@ describe "CSRF protection", -> expect(response.statusCode).to.equal 403 done() +describe "Register", -> + before -> + @user = new User() + + it 'Set emails attribute', (done) -> + @user.register (error, user) => + expect(error).to.not.exist + user.email.should.equal @user.email + user.emails.should.exist + user.emails.should.be.a 'array' + user.emails.length.should.equal 1 + user.emails[0].email.should.equal @user.email + done() + describe "LoginViaRegistration", -> before (done) -> diff --git a/services/web/test/acceptance/coffee/helpers/User.coffee b/services/web/test/acceptance/coffee/helpers/User.coffee index f83780b535..793ac6cd9f 100644 --- a/services/web/test/acceptance/coffee/helpers/User.coffee +++ b/services/web/test/acceptance/coffee/helpers/User.coffee @@ -22,9 +22,30 @@ class User jar: @jar }) + setExtraAttributes: (user) -> + throw new Error("User does not exist") unless user?._id? + @id = user._id.toString() + @_id = user._id.toString() + @first_name = user.first_name + @referal_id = user.referal_id + get: (callback = (error, user)->) -> db.users.findOne { _id: ObjectId(@_id) }, callback + register: (callback = (error, user) ->) -> + return callback(new Error('User already registered')) if @_id? + @getCsrfToken (error) => + return callback(error) if error? + @request.post { + url: '/register' + json: { @email, @password } + }, (error, response, body) => + return callback(error) if error? + db.users.findOne { email: @email }, (error, user) => + return callback(error) if error? + @setExtraAttributes user + callback(null, user) + login: (callback = (error) ->) -> @loginWith(@email, callback) @@ -47,11 +68,7 @@ class User return callback(error) if error? UserUpdater.updateUser user._id, $set: emails: @emails, (error) => return callback(error) if error? - @id = user?._id?.toString() - @_id = user?._id?.toString() - @first_name = user?.first_name - @referal_id = user?.referal_id - + @setExtraAttributes user callback(null, @password) setFeatures: (features, callback = (error) ->) -> diff --git a/services/web/test/unit/coffee/User/UserCreatorTests.coffee b/services/web/test/unit/coffee/User/UserCreatorTests.coffee index cc2b1ec150..3764c8a765 100644 --- a/services/web/test/unit/coffee/User/UserCreatorTests.coffee +++ b/services/web/test/unit/coffee/User/UserCreatorTests.coffee @@ -70,3 +70,11 @@ describe "UserCreator", -> assert.equal user.holdingAccount, true assert.equal user.last_name, "lastNammmmeee" done() + + it "should set emails attribute", (done)-> + @UserCreator.createNewUser email: @email, (err, user)=> + user.email.should.equal @email + user.emails.length.should.equal 1 + user.emails[0].email.should.equal @email + user.emails[0].createdAt.should.be.a 'date' + done() diff --git a/services/web/test/unit/coffee/User/UserGetterTests.coffee b/services/web/test/unit/coffee/User/UserGetterTests.coffee index cdc22772f3..9ce4c843ff 100644 --- a/services/web/test/unit/coffee/User/UserGetterTests.coffee +++ b/services/web/test/unit/coffee/User/UserGetterTests.coffee @@ -74,3 +74,20 @@ describe "UserGetter", -> @findOne.calledTwice.should.equal true @findOne.calledWith(email: email, projection).should.equal true done() + + describe 'ensureUniqueEmailAddress', -> + beforeEach -> + @UserGetter.getUserByAnyEmail = sinon.stub() + + it 'should return error if existing user is found', (done)-> + @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @fakeUser) + @UserGetter.ensureUniqueEmailAddress @newEmail, (err)=> + should.exist(err) + err.message.should.equal 'alread_exists' + done() + + it 'should return null if no user is found', (done)-> + @UserGetter.getUserByAnyEmail.callsArgWith(1) + @UserGetter.ensureUniqueEmailAddress @newEmail, (err)=> + should.not.exist(err) + done() diff --git a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee index 9411059022..e738947171 100644 --- a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee +++ b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee @@ -14,7 +14,7 @@ describe "UserRegistrationHandler", -> @User = update: sinon.stub().callsArgWith(2) @UserGetter = - getUserByMainEmail: sinon.stub() + getUserByAnyEmail: sinon.stub() @UserCreator = createNewUser:sinon.stub().callsArgWith(1, null, @user) @AuthenticationManager = @@ -72,7 +72,7 @@ describe "UserRegistrationHandler", -> beforeEach -> @user.holdingAccount = true @handler._registrationRequestIsValid = sinon.stub().returns true - @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) + @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user) it "should not create a new user if there is a holding account there", (done)-> @handler.registerNewUser @passingRequest, (err)=> @@ -96,7 +96,7 @@ describe "UserRegistrationHandler", -> done() it "should return email registered in the error if there is a non holdingAccount there", (done)-> - @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user = {holdingAccount:false}) + @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user = {holdingAccount:false}) @handler.registerNewUser @passingRequest, (err, user)=> err.should.deep.equal new Error("EmailAlreadyRegistered") user.should.deep.equal @user @@ -105,7 +105,7 @@ describe "UserRegistrationHandler", -> describe "validRequest", -> beforeEach -> @handler._registrationRequestIsValid = sinon.stub().returns true - @UserGetter.getUserByMainEmail.callsArgWith 1 + @UserGetter.getUserByAnyEmail.callsArgWith 1 it "should create a new user", (done)-> @handler.registerNewUser @passingRequest, (err)=> diff --git a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee index b952a688ae..7b3be3df2b 100644 --- a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee @@ -17,6 +17,7 @@ describe "UserUpdater", -> @UserGetter = getUserEmail: sinon.stub() getUserByAnyEmail: sinon.stub() + ensureUniqueEmailAddress: sinon.stub() @logger = err: sinon.stub(), log: -> @UserUpdater = SandboxedModule.require modulePath, requires: "settings-sharelatex":@settings @@ -60,13 +61,13 @@ describe "UserUpdater", -> describe 'addEmailAddress', -> beforeEach -> - @UserUpdater._ensureUniqueEmailAddress = sinon.stub().callsArgWith(1) + @UserGetter.ensureUniqueEmailAddress = sinon.stub().callsArgWith(1) it 'add email', (done)-> @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null) @UserUpdater.addEmailAddress @stubbedUser._id, @newEmail, (err)=> - @UserUpdater._ensureUniqueEmailAddress.called.should.equal true + @UserGetter.ensureUniqueEmailAddress.called.should.equal true should.not.exist(err) @UserUpdater.updateUser.calledWith( @stubbedUser._id, @@ -135,15 +136,3 @@ describe "UserUpdater", -> done() - describe '_ensureUniqueEmailAddress', -> - it 'should return error if existing user is found', (done)-> - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @stubbedUser) - @UserUpdater._ensureUniqueEmailAddress @newEmail, (err)=> - should.exist(err) - done() - - it 'should return null if no user is found', (done)-> - @UserGetter.getUserByAnyEmail.callsArgWith(1) - @UserUpdater._ensureUniqueEmailAddress @newEmail, (err)=> - should.not.exist(err) - done() From 813289f5de740128b44294fdf0699bfb758abaee Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Mon, 28 May 2018 16:09:22 +0200 Subject: [PATCH 14/30] use multiple emails when relevant --- .../CollaboratorsInviteController.coffee | 2 +- .../CollaboratorsInviteHandler.coffee | 2 +- .../SubscriptionGroupHandler.coffee | 2 +- .../CollaboratorsInviteControllerTests.coffee | 12 ++++++------ .../CollaboratorsInviteHandlerTests.coffee | 18 +++++++++--------- .../SubscriptionGroupHandlerTests.coffee | 8 ++++---- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee index f74a144bac..8b1b481994 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee @@ -27,7 +27,7 @@ module.exports = CollaboratorsInviteController = _checkShouldInviteEmail: (email, callback=(err, shouldAllowInvite)->) -> if Settings.restrictInvitesToExistingAccounts == true logger.log {email}, "checking if user exists with this email" - UserGetter.getUserByMainEmail email, {_id: 1}, (err, user) -> + UserGetter.getUserByAnyEmail email, {_id: 1}, (err, user) -> return callback(err) if err? userExists = user? and user?._id? callback(null, userExists) diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee index b511f56e53..58121dae2c 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee @@ -32,7 +32,7 @@ module.exports = CollaboratorsInviteHandler = _trySendInviteNotification: (projectId, sendingUser, invite, callback=(err)->) -> email = invite.email - UserGetter.getUserByMainEmail email, {_id: 1}, (err, existingUser) -> + UserGetter.getUserByAnyEmail email, {_id: 1}, (err, existingUser) -> if err? logger.err {projectId, email}, "error checking if user exists" return callback(err) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee index d6ce0dde59..9463538250 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee @@ -21,7 +21,7 @@ module.exports = SubscriptionGroupHandler = if limitReached logger.err adminUserId:adminUserId, newEmail:newEmail, "group subscription limit reached not adding user to group" return callback(limitReached:limitReached) - UserGetter.getUserByMainEmail newEmail, (err, user)-> + UserGetter.getUserByAnyEmail newEmail, (err, user)-> return callback(err) if err? if user? SubscriptionUpdater.addUserToGroup adminUserId, user._id, (err)-> diff --git a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee index 4b57ff3697..affef2dc31 100644 --- a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee +++ b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee @@ -25,7 +25,7 @@ describe "CollaboratorsInviteController", -> @LimitationsManager = {} @UserGetter = - getUserByMainEmail: sinon.stub() + getUserByAnyEmail: sinon.stub() getUser: sinon.stub() @CollaboratorsInviteController = SandboxedModule.require modulePath, requires: @@ -716,7 +716,7 @@ describe "CollaboratorsInviteController", -> beforeEach -> @user = {_id: ObjectId().toString()} - @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, @user) + @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, @user) it 'should callback with `true`', (done) -> @call (err, shouldAllow) => @@ -728,7 +728,7 @@ describe "CollaboratorsInviteController", -> beforeEach -> @user = null - @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, @user) + @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, @user) it 'should callback with `false`', (done) -> @call (err, shouldAllow) => @@ -738,15 +738,15 @@ describe "CollaboratorsInviteController", -> it 'should have called getUser', (done) -> @call (err, shouldAllow) => - @UserGetter.getUserByMainEmail.callCount.should.equal 1 - @UserGetter.getUserByMainEmail.calledWith(@email, {_id: 1}).should.equal true + @UserGetter.getUserByAnyEmail.callCount.should.equal 1 + @UserGetter.getUserByAnyEmail.calledWith(@email, {_id: 1}).should.equal true done() describe 'when getUser produces an error', -> beforeEach -> @user = null - @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, new Error('woops')) + @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, new Error('woops')) it 'should callback with an error', (done) -> @call (err, shouldAllow) => diff --git a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee index 58b373f61f..edad39a637 100644 --- a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee +++ b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee @@ -605,7 +605,7 @@ describe "CollaboratorsInviteHandler", -> _id: ObjectId() first_name: "jim" @existingUser = {_id: ObjectId()} - @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, @existingUser) + @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, @existingUser) @fakeProject = _id: @project_id name: "some project" @@ -626,8 +626,8 @@ describe "CollaboratorsInviteHandler", -> it 'should call getUser', (done) -> @call (err) => - @UserGetter.getUserByMainEmail.callCount.should.equal 1 - @UserGetter.getUserByMainEmail.calledWith(@invite.email).should.equal true + @UserGetter.getUserByAnyEmail.callCount.should.equal 1 + @UserGetter.getUserByAnyEmail.calledWith(@invite.email).should.equal true done() it 'should call getProject', (done) -> @@ -671,7 +671,7 @@ describe "CollaboratorsInviteHandler", -> describe 'when the user does not exist', -> beforeEach -> - @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, null) + @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, null) it 'should not produce an error', (done) -> @call (err) => @@ -680,8 +680,8 @@ describe "CollaboratorsInviteHandler", -> it 'should call getUser', (done) -> @call (err) => - @UserGetter.getUserByMainEmail.callCount.should.equal 1 - @UserGetter.getUserByMainEmail.calledWith(@invite.email).should.equal true + @UserGetter.getUserByAnyEmail.callCount.should.equal 1 + @UserGetter.getUserByAnyEmail.calledWith(@invite.email).should.equal true done() it 'should not call getProject', (done) -> @@ -698,7 +698,7 @@ describe "CollaboratorsInviteHandler", -> describe 'when the getUser produces an error', -> beforeEach -> - @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, new Error('woops')) + @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, new Error('woops')) it 'should produce an error', (done) -> @call (err) => @@ -707,8 +707,8 @@ describe "CollaboratorsInviteHandler", -> it 'should call getUser', (done) -> @call (err) => - @UserGetter.getUserByMainEmail.callCount.should.equal 1 - @UserGetter.getUserByMainEmail.calledWith(@invite.email).should.equal true + @UserGetter.getUserByAnyEmail.callCount.should.equal 1 + @UserGetter.getUserByAnyEmail.calledWith(@invite.email).should.equal true done() it 'should not call getProject', (done) -> diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee index bca9ac7600..9a70ca0411 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee @@ -32,7 +32,7 @@ describe "SubscriptionGroupHandler", -> @UserGetter = getUser: sinon.stub() - getUserByMainEmail: sinon.stub() + getUserByAnyEmail: sinon.stub() @LimitationsManager = hasGroupMembersLimitReached: sinon.stub() @@ -71,11 +71,11 @@ describe "SubscriptionGroupHandler", -> describe "addUserToGroup", -> beforeEach -> @LimitationsManager.hasGroupMembersLimitReached.callsArgWith(1, null, false, @subscription) - @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) + @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user) it "should find the user", (done)-> @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> - @UserGetter.getUserByMainEmail.calledWith(@newEmail).should.equal true + @UserGetter.getUserByAnyEmail.calledWith(@newEmail).should.equal true done() it "should add the user to the group", (done)-> @@ -102,7 +102,7 @@ describe "SubscriptionGroupHandler", -> done() it "should add an email invite if no user is found", (done) -> - @UserGetter.getUserByMainEmail.callsArgWith(1, null, null) + @UserGetter.getUserByAnyEmail.callsArgWith(1, null, null) @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> @SubscriptionUpdater.addEmailInviteToGroup.calledWith(@adminUser_id, @newEmail).should.equal true done() From 8f71b104c52d2e6f9e759e14898c22c26c62c1ab Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Wed, 6 Jun 2018 16:59:13 +0100 Subject: [PATCH 15/30] Fix bug where unowned project would show archive quick action instead of leave If the user does not own the project, the project can only be left, not archived. Previously the quick action button was only showing the archive icon but clicking the button would correctly leave the project. This is confusing, so this commit corrects to show the leave icon for projects not owned by the current user --- services/web/app/views/project/list/item.pug | 12 ++++++++++-- .../coffee/main/project-list/project-list.coffee | 5 ++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/services/web/app/views/project/list/item.pug b/services/web/app/views/project/list/item.pug index bc37683489..5a9cb2138d 100644 --- a/services/web/app/views/project/list/item.pug +++ b/services/web/app/views/project/list/item.pug @@ -64,13 +64,21 @@ if settings.overleaf ) i.icon.fa.fa-cloud-download button.btn.btn-link.action-btn( - ng-if="!project.archived" + ng-if="!project.archived && isOwner()" tooltip=translate('archive'), tooltip-placement="top", tooltip-append-to-body="true", - ng-click="archive($event)" + ng-click="archiveOrLeave($event)" ) i.icon.fa.fa-inbox + button.btn.btn-link.action-btn( + ng-if="!project.archived && !isOwner()" + tooltip=translate('leave'), + tooltip-placement="top", + tooltip-append-to-body="true", + ng-click="archiveOrLeave($event)" + ) + i.icon.fa.fa-sign-out button.btn.btn-link.action-btn( ng-if="project.archived" tooltip=translate('unarchive'), diff --git a/services/web/public/coffee/main/project-list/project-list.coffee b/services/web/public/coffee/main/project-list/project-list.coffee index 6696243905..841ccd6c2d 100644 --- a/services/web/public/coffee/main/project-list/project-list.coffee +++ b/services/web/public/coffee/main/project-list/project-list.coffee @@ -490,6 +490,9 @@ define [ else return "None" + $scope.isOwner = () -> + window.user_id == $scope.project.owner._id + $scope.$watch "project.selected", (value) -> if value? $scope.updateSelectedProjects() @@ -502,7 +505,7 @@ define [ e.stopPropagation() $scope.downloadProjectsById([$scope.project.id]) - $scope.archive = (e) -> + $scope.archiveOrLeave = (e) -> e.stopPropagation() $scope.archiveOrLeaveProjects([$scope.project]) From 0900559579e7b72fde509e59584acf94376b8ab1 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Wed, 6 Jun 2018 17:19:12 +0100 Subject: [PATCH 16/30] Fix blurrly autocomplete highlight on Chrome The blurrly text shadow is back on Chrome. I suspect it maybe intended, not a bug, so I've fixed it for all versions of Chrome. I've replaced with font-weight: bold, which visually has the same appearance --- services/web/public/coffee/ide.coffee | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/services/web/public/coffee/ide.coffee b/services/web/public/coffee/ide.coffee index 6c5b1d920e..9f910d857e 100644 --- a/services/web/public/coffee/ide.coffee +++ b/services/web/public/coffee/ide.coffee @@ -213,11 +213,10 @@ define [ try chromeVersion = parseFloat(navigator.userAgent.split(" Chrome/")[1]) || null; browserIsChrome61or62 = ( - chromeVersion? && - (chromeVersion == 61 || chromeVersion == 62) + chromeVersion? ) if browserIsChrome61or62 - document.styleSheets[0].insertRule(".ace_editor.ace_autocomplete .ace_completion-highlight { text-shadow: none !important; }", 1) + document.styleSheets[0].insertRule(".ace_editor.ace_autocomplete .ace_completion-highlight { text-shadow: none !important; font-weight: bold; }", 1) catch err console.error err From 458650d4566e2182617384c3d8e14f333a040c0b Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Wed, 6 Jun 2018 15:30:33 +0200 Subject: [PATCH 17/30] create script to backfill secondary emails --- services/web/scripts/add_multiple_emails.js | 61 +++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 services/web/scripts/add_multiple_emails.js diff --git a/services/web/scripts/add_multiple_emails.js b/services/web/scripts/add_multiple_emails.js new file mode 100644 index 0000000000..8da2d57dac --- /dev/null +++ b/services/web/scripts/add_multiple_emails.js @@ -0,0 +1,61 @@ +const mongojs = require('../app/js/infrastructure/mongojs') +const { db } = mongojs +const async = require('async') +const minilist = require('minimist') + +const updateUser = function (user, callback) { + console.log(`Updating user ${user._id}`) + const update = { + $set: { + emails: [{ + email: user.email, + createdAt: new Date() + }] + } + } + db.users.update({_id: user._id}, update, callback) +} + +const updateUsers = (users, callback) => + async.eachLimit(users, ASYNC_LIMIT, updateUser, function (error) { + if (error) { + callback(error) + return + } + counter += users.length + console.log(`${counter} users updated`) + loopForUsers(callback) + }) + +var loopForUsers = callback => + db.users.find( + { emails: {$exists: false} }, + { email: 1 } + ).limit(FETCH_LIMIT, function (error, users) { + if (error) { + callback(error) + return + } + if (users.length === 0) { + console.log(`DONE (${counter} users updated)`) + return callback() + } + updateUsers(users, callback) + }) + +var counter = 0 +var run = () => + loopForUsers(function (error) { + if (error) { throw error } + process.exit() + }) + +let FETCH_LIMIT, ASYNC_LIMIT +var setup = function () { + let args = minilist(process.argv.slice(2)) + FETCH_LIMIT = (args.fetch) ? args.fetch : 100 + ASYNC_LIMIT = (args.async) ? args.async : 10 +} + +setup() +run() From 4a1c2cf0e07af4804a25908f33b4624ba5f92794 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Wed, 6 Jun 2018 13:44:02 +0100 Subject: [PATCH 18/30] Set cloned project's owner to current user Fixes a bug where cloning a project then selecting to delete it, the wrong button for deletion is shown (leave instead of archive/delete). This is because we are using the owner object (which was undefined after cloning) to determine which button to show --- .../web/public/coffee/main/project-list/project-list.coffee | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/web/public/coffee/main/project-list/project-list.coffee b/services/web/public/coffee/main/project-list/project-list.coffee index 6696243905..6e40bb04c1 100644 --- a/services/web/public/coffee/main/project-list/project-list.coffee +++ b/services/web/public/coffee/main/project-list/project-list.coffee @@ -320,6 +320,9 @@ define [ name: cloneName id: data.project_id accessLevel: "owner" + owner: { + _id: user_id + } # TODO: Check access level if correct after adding it in # to the rest of the app } From c684fc3383386f1edc61e65b4a97e2610f71c8c1 Mon Sep 17 00:00:00 2001 From: hugh-obrien Date: Thu, 7 Jun 2018 12:57:01 +0100 Subject: [PATCH 19/30] fix first/last name check bug --- .../web/app/coffee/Features/Exports/ExportsController.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Exports/ExportsController.coffee b/services/web/app/coffee/Features/Exports/ExportsController.coffee index 393ca95fdb..40cb7bb507 100644 --- a/services/web/app/coffee/Features/Exports/ExportsController.coffee +++ b/services/web/app/coffee/Features/Exports/ExportsController.coffee @@ -13,7 +13,7 @@ module.exports = user_id: user_id } - if req.body && req.body.firstName && req.body.firstName + if req.body && req.body.firstName && req.body.lastName export_params.first_name = req.body.firstName.trim() export_params.last_name = req.body.lastName.trim() From bfaa6d8dcc04c10d2c7c9fb88dc9be283cc3b058 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 29 May 2018 11:41:08 +0100 Subject: [PATCH 20/30] Improve styling of buttons --- .../stylesheets/app/editor/toolbar.less | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/services/web/public/stylesheets/app/editor/toolbar.less b/services/web/public/stylesheets/app/editor/toolbar.less index a73f46abc3..f4521c1600 100644 --- a/services/web/public/stylesheets/app/editor/toolbar.less +++ b/services/web/public/stylesheets/app/editor/toolbar.less @@ -184,8 +184,12 @@ } } +/************************************** + Toggle Switch +***************************************/ + .toggle-wrapper { - width: 200px; + min-width: 200px; height: 24px; } @@ -241,3 +245,22 @@ transform: translate(100%); border-radius: 0 @btn-border-radius-base @btn-border-radius-base 0; } + +/************************************** + Formatting buttons +***************************************/ +.formatting-btn { + padding: 0; + min-width: 32px; + height: 100%; + display: flex; + align-items: center; + justify-content: center; + box-shadow: none; + border: none; + border-left: 1px solid #ccc; + + &:last-child { + border-right: 1px solid #ccc; + } +} From 7384cfba1a411149e0c550bfa5f2191823477329 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 29 May 2018 16:56:38 +0100 Subject: [PATCH 21/30] Style wrapped buttons so the toolbar can be resized --- services/web/public/stylesheets/app/editor.less | 1 + .../web/public/stylesheets/app/editor/toolbar.less | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/services/web/public/stylesheets/app/editor.less b/services/web/public/stylesheets/app/editor.less index 0e3d48cf87..af3063cf22 100644 --- a/services/web/public/stylesheets/app/editor.less +++ b/services/web/public/stylesheets/app/editor.less @@ -92,6 +92,7 @@ .toolbar-editor { height: @editor-toolbar-height; background-color: @editor-toolbar-bg; + overflow: hidden; } .loading-screen { diff --git a/services/web/public/stylesheets/app/editor/toolbar.less b/services/web/public/stylesheets/app/editor/toolbar.less index f4521c1600..e5601fe4d3 100644 --- a/services/web/public/stylesheets/app/editor/toolbar.less +++ b/services/web/public/stylesheets/app/editor/toolbar.less @@ -249,9 +249,14 @@ /************************************** Formatting buttons ***************************************/ +.button-measurer-wrapper { + display: flex; +} + .formatting-btn { padding: 0; min-width: 32px; + width: 32px; height: 100%; display: flex; align-items: center; @@ -259,8 +264,14 @@ box-shadow: none; border: none; border-left: 1px solid #ccc; + border-radius: 0; &:last-child { border-right: 1px solid #ccc; } } + +.formatting-btn-icon { + font-style: normal; + line-height: 1.5; +} From aaf5da877e07ff672cd74014e0561c2ad4289155 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 31 May 2018 13:31:24 +0100 Subject: [PATCH 22/30] Style different buttons --- .../stylesheets/app/editor/toolbar.less | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/services/web/public/stylesheets/app/editor/toolbar.less b/services/web/public/stylesheets/app/editor/toolbar.less index e5601fe4d3..008536152b 100644 --- a/services/web/public/stylesheets/app/editor/toolbar.less +++ b/services/web/public/stylesheets/app/editor/toolbar.less @@ -249,14 +249,15 @@ /************************************** Formatting buttons ***************************************/ -.button-measurer-wrapper { +formatting-buttons { display: flex; + width: 100%; + overflow: hidden; } .formatting-btn { padding: 0; - min-width: 32px; - width: 32px; + margin: 3px 0; height: 100%; display: flex; align-items: center; @@ -265,12 +266,23 @@ border: none; border-left: 1px solid #ccc; border-radius: 0; +} - &:last-child { +.formatting-btn-with-icon { + min-width: 32px; + width: 32px; + + &:nth-child(-1) { border-right: 1px solid #ccc; } } +.formatting-btn-overflowed { + margin-left: auto; + padding-left: 5px; + padding-right: 5px; +} + .formatting-btn-icon { font-style: normal; line-height: 1.5; From c5f62d3aa3ad341b90f63e0a0b2377832382a296 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 31 May 2018 16:10:40 +0100 Subject: [PATCH 23/30] Style dropdown & clean up naming --- .../stylesheets/app/editor/toolbar.less | 38 ++++++++++++------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/services/web/public/stylesheets/app/editor/toolbar.less b/services/web/public/stylesheets/app/editor/toolbar.less index 008536152b..55303a64c6 100644 --- a/services/web/public/stylesheets/app/editor/toolbar.less +++ b/services/web/public/stylesheets/app/editor/toolbar.less @@ -257,7 +257,6 @@ formatting-buttons { .formatting-btn { padding: 0; - margin: 3px 0; height: 100%; display: flex; align-items: center; @@ -266,24 +265,35 @@ formatting-buttons { border: none; border-left: 1px solid #ccc; border-radius: 0; -} -.formatting-btn-with-icon { - min-width: 32px; - width: 32px; + &--icon { + min-width: 32px; + width: 32px; - &:nth-child(-1) { - border-right: 1px solid #ccc; + &:last-of-type { + border-right: 1px solid @formatting-btn-border; + } + } + + &--more { + padding-left: 9px; + padding-right: 9px; } } -.formatting-btn-overflowed { - margin-left: auto; - padding-left: 5px; - padding-right: 5px; -} - -.formatting-btn-icon { +.formatting-icon { font-style: normal; line-height: 1.5; } + +.formatting-more { + margin-left: auto; +} + +.formatting-menu { + min-width: auto; + + .formatting-menu-item { + float: left; + } +} From 611a6f9c0bb18dcb93dd6a602886a6cd191ac888 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 31 May 2018 16:26:40 +0100 Subject: [PATCH 24/30] Reduce width of overflowed button menu --- services/web/public/stylesheets/app/editor/toolbar.less | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/services/web/public/stylesheets/app/editor/toolbar.less b/services/web/public/stylesheets/app/editor/toolbar.less index 55303a64c6..d4162aeb5a 100644 --- a/services/web/public/stylesheets/app/editor/toolbar.less +++ b/services/web/public/stylesheets/app/editor/toolbar.less @@ -292,8 +292,13 @@ formatting-buttons { .formatting-menu { min-width: auto; + max-width: 130px; .formatting-menu-item { float: left; + + &:nth-of-type(4n + 1) .formatting-btn { + border-left: none; + } } } From 12d7eb8a46f212853fc1d5efc883ab84aa9f4491 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 31 May 2018 17:47:55 +0100 Subject: [PATCH 25/30] Adjust styling to work with wrapper --- services/web/public/stylesheets/app/editor/toolbar.less | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/services/web/public/stylesheets/app/editor/toolbar.less b/services/web/public/stylesheets/app/editor/toolbar.less index d4162aeb5a..f1faddb7a9 100644 --- a/services/web/public/stylesheets/app/editor/toolbar.less +++ b/services/web/public/stylesheets/app/editor/toolbar.less @@ -249,12 +249,15 @@ /************************************** Formatting buttons ***************************************/ -formatting-buttons { - display: flex; +.formatting-buttons { width: 100%; overflow: hidden; } +.formatting-buttons-wrapper { + display: flex; +} + .formatting-btn { padding: 0; height: 100%; From ea18d606c41bec4b2be644598481377c352019ae Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 1 Jun 2018 13:19:00 +0100 Subject: [PATCH 26/30] Nicer v2 styles --- .../web/public/stylesheets/app/editor/toolbar.less | 13 ++++++++++++- .../public/stylesheets/core/_common-variables.less | 6 ++++++ .../web/public/stylesheets/core/ol-variables.less | 6 ++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/services/web/public/stylesheets/app/editor/toolbar.less b/services/web/public/stylesheets/app/editor/toolbar.less index f1faddb7a9..55e581c3d6 100644 --- a/services/web/public/stylesheets/app/editor/toolbar.less +++ b/services/web/public/stylesheets/app/editor/toolbar.less @@ -259,6 +259,8 @@ } .formatting-btn { + color: @formatting-btn-color; + background-color: @formatting-btn-bg; padding: 0; height: 100%; display: flex; @@ -266,9 +268,13 @@ justify-content: center; box-shadow: none; border: none; - border-left: 1px solid #ccc; + border-left: 1px solid @formatting-btn-border; border-radius: 0; + &:hover { + color: @formatting-btn-color; + } + &--icon { min-width: 32px; width: 32px; @@ -296,10 +302,15 @@ .formatting-menu { min-width: auto; max-width: 130px; + background-color: @formatting-menu-bg; .formatting-menu-item { float: left; + .formatting-btn { + border-right: none; + } + &:nth-of-type(4n + 1) .formatting-btn { border-left: none; } diff --git a/services/web/public/stylesheets/core/_common-variables.less b/services/web/public/stylesheets/core/_common-variables.less index 033ce9c1bf..5d08d9f95d 100644 --- a/services/web/public/stylesheets/core/_common-variables.less +++ b/services/web/public/stylesheets/core/_common-variables.less @@ -941,6 +941,12 @@ @toggle-switch-bg : @gray-lightest; @toggle-switch-highlight-color : @brand-primary; +// Formatting buttons +@formatting-btn-color : @btn-default-color; +@formatting-btn-bg : @btn-default-bg; +@formatting-btn-border : @btn-default-border; +@formatting-menu-bg : @btn-default-bg; + // Chat @chat-bg : transparent; @chat-message-color : @text-color; diff --git a/services/web/public/stylesheets/core/ol-variables.less b/services/web/public/stylesheets/core/ol-variables.less index 3d3d4468fd..114d07a5b1 100644 --- a/services/web/public/stylesheets/core/ol-variables.less +++ b/services/web/public/stylesheets/core/ol-variables.less @@ -244,6 +244,12 @@ @toggle-switch-radius-left : @btn-border-radius-base 0 0 @btn-border-radius-base; @toggle-switch-radius-right : 0 @btn-border-radius-base @btn-border-radius-base 0; +// Formatting buttons +@formatting-btn-color : #FFF; +@formatting-btn-bg : @ol-blue-gray-5; +@formatting-btn-border : @ol-blue-gray-4; +@formatting-menu-bg : @ol-blue-gray-5; + // Chat @chat-bg : @ol-blue-gray-5; @chat-message-color : #FFF; From 9ae92dbeef614436f3ecc21544577b318d87740b Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 1 Jun 2018 13:45:47 +0100 Subject: [PATCH 27/30] Small icon & math icon styling --- services/web/public/stylesheets/app/editor/toolbar.less | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/services/web/public/stylesheets/app/editor/toolbar.less b/services/web/public/stylesheets/app/editor/toolbar.less index 55e581c3d6..2f34e7a258 100644 --- a/services/web/public/stylesheets/app/editor/toolbar.less +++ b/services/web/public/stylesheets/app/editor/toolbar.less @@ -293,6 +293,15 @@ .formatting-icon { font-style: normal; line-height: 1.5; + + &--small { + font-size: small; + line-height: 1.9; + } + + &--serif { + font-family: @font-family-serif; + } } .formatting-more { From ad13eccfa71163714a1b4c07fc0ac1b024ba8d73 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 1 Jun 2018 15:09:15 +0100 Subject: [PATCH 28/30] Flatten rules for readability --- .../stylesheets/app/editor/toolbar.less | 61 ++++++++++--------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/services/web/public/stylesheets/app/editor/toolbar.less b/services/web/public/stylesheets/app/editor/toolbar.less index 2f34e7a258..45a0384381 100644 --- a/services/web/public/stylesheets/app/editor/toolbar.less +++ b/services/web/public/stylesheets/app/editor/toolbar.less @@ -274,34 +274,34 @@ &:hover { color: @formatting-btn-color; } +} - &--icon { - min-width: 32px; - width: 32px; +.formatting-btn--icon { + min-width: 32px; + width: 32px; +} - &:last-of-type { - border-right: 1px solid @formatting-btn-border; - } - } +.formatting-btn--icon:last-of-type { + border-right: 1px solid @formatting-btn-border; +} - &--more { - padding-left: 9px; - padding-right: 9px; - } +.formatting-btn--more { + padding-left: 9px; + padding-right: 9px; } .formatting-icon { font-style: normal; line-height: 1.5; +} - &--small { - font-size: small; - line-height: 1.9; - } +.formatting-icon--small { + font-size: small; + line-height: 1.9; +} - &--serif { - font-family: @font-family-serif; - } +.formatting-icon--serif { + font-family: @font-family-serif; } .formatting-more { @@ -312,16 +312,17 @@ min-width: auto; max-width: 130px; background-color: @formatting-menu-bg; - - .formatting-menu-item { - float: left; - - .formatting-btn { - border-right: none; - } - - &:nth-of-type(4n + 1) .formatting-btn { - border-left: none; - } - } +} + +.formatting-menu-item { + float: left; +} + +.formatting-menu-item > .formatting-btn { + border-right: none; +} + +// Disable border on left-most icon in menu +.formatting-menu-item:nth-of-type(4n + 1) > .formatting-btn { + border-left: none; } From c4c94419954ea6be10540a651742a7b28203cdab Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 5 Jun 2018 14:55:32 +0100 Subject: [PATCH 29/30] Adjust caret down --- services/web/public/stylesheets/app/editor/toolbar.less | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/services/web/public/stylesheets/app/editor/toolbar.less b/services/web/public/stylesheets/app/editor/toolbar.less index 45a0384381..4006b56a7b 100644 --- a/services/web/public/stylesheets/app/editor/toolbar.less +++ b/services/web/public/stylesheets/app/editor/toolbar.less @@ -288,6 +288,10 @@ .formatting-btn--more { padding-left: 9px; padding-right: 9px; + + .caret { + margin-top: 1px; + } } .formatting-icon { From 7cdcd725fd6cce231b933acba313ba93a1ec0c73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Thu, 7 Jun 2018 18:44:59 +0200 Subject: [PATCH 30/30] Revert "Use Multiple Emails" --- .../CollaboratorsInviteController.coffee | 2 +- .../CollaboratorsInviteHandler.coffee | 2 +- .../SubscriptionGroupHandler.coffee | 2 +- .../coffee/Features/User/UserCreator.coffee | 4 --- .../coffee/Features/User/UserGetter.coffee | 9 +------ .../User/UserRegistrationHandler.coffee | 2 +- .../coffee/Features/User/UserUpdater.coffee | 11 +++++++- services/web/app/coffee/models/User.coffee | 4 --- .../coffee/RegistrationTests.coffee | 14 ---------- .../acceptance/coffee/helpers/User.coffee | 27 ++++--------------- .../CollaboratorsInviteControllerTests.coffee | 12 ++++----- .../CollaboratorsInviteHandlerTests.coffee | 18 ++++++------- .../SubscriptionGroupHandlerTests.coffee | 8 +++--- .../unit/coffee/User/UserCreatorTests.coffee | 8 ------ .../unit/coffee/User/UserGetterTests.coffee | 17 ------------ .../User/UserRegistrationHandlerTests.coffee | 8 +++--- .../unit/coffee/User/UserUpdaterTests.coffee | 17 +++++++++--- 17 files changed, 57 insertions(+), 108 deletions(-) diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee index 8b1b481994..f74a144bac 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteController.coffee @@ -27,7 +27,7 @@ module.exports = CollaboratorsInviteController = _checkShouldInviteEmail: (email, callback=(err, shouldAllowInvite)->) -> if Settings.restrictInvitesToExistingAccounts == true logger.log {email}, "checking if user exists with this email" - UserGetter.getUserByAnyEmail email, {_id: 1}, (err, user) -> + UserGetter.getUserByMainEmail email, {_id: 1}, (err, user) -> return callback(err) if err? userExists = user? and user?._id? callback(null, userExists) diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee index 58121dae2c..b511f56e53 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsInviteHandler.coffee @@ -32,7 +32,7 @@ module.exports = CollaboratorsInviteHandler = _trySendInviteNotification: (projectId, sendingUser, invite, callback=(err)->) -> email = invite.email - UserGetter.getUserByAnyEmail email, {_id: 1}, (err, existingUser) -> + UserGetter.getUserByMainEmail email, {_id: 1}, (err, existingUser) -> if err? logger.err {projectId, email}, "error checking if user exists" return callback(err) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee index 9463538250..d6ce0dde59 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionGroupHandler.coffee @@ -21,7 +21,7 @@ module.exports = SubscriptionGroupHandler = if limitReached logger.err adminUserId:adminUserId, newEmail:newEmail, "group subscription limit reached not adding user to group" return callback(limitReached:limitReached) - UserGetter.getUserByAnyEmail newEmail, (err, user)-> + UserGetter.getUserByMainEmail newEmail, (err, user)-> return callback(err) if err? if user? SubscriptionUpdater.addUserToGroup adminUserId, user._id, (err)-> diff --git a/services/web/app/coffee/Features/User/UserCreator.coffee b/services/web/app/coffee/Features/User/UserCreator.coffee index 4b4dd30ae2..0a0cc8641e 100644 --- a/services/web/app/coffee/Features/User/UserCreator.coffee +++ b/services/web/app/coffee/Features/User/UserCreator.coffee @@ -18,10 +18,6 @@ module.exports = UserCreator = user.ace.syntaxValidation = true user.featureSwitches?.pdfng = true - user.emails = [ - email: user.email - createdAt: new Date() - ] user.save (err)-> callback(err, user) diff --git a/services/web/app/coffee/Features/User/UserGetter.coffee b/services/web/app/coffee/Features/User/UserGetter.coffee index 4ba750be17..2e6d4eada6 100644 --- a/services/web/app/coffee/Features/User/UserGetter.coffee +++ b/services/web/app/coffee/Features/User/UserGetter.coffee @@ -64,19 +64,12 @@ module.exports = UserGetter = return callback(null, user) if user? db.userstubs.findOne query, projection, callback - # check for duplicate email address. This is also enforced at the DB level - ensureUniqueEmailAddress: (newEmail, callback) -> - @getUserByAnyEmail newEmail, (error, user) -> - return callback(message: 'alread_exists') if user? - callback(error) - [ 'getUser', 'getUserEmail', 'getUserByMainEmail', 'getUserByAnyEmail', 'getUsers', - 'getUserOrUserStubById', - 'ensureUniqueEmailAddress', + 'getUserOrUserStubById' ].map (method) -> metrics.timeAsyncMethod UserGetter, method, 'mongo.UserGetter', logger diff --git a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index 4a42d7f5fa..fab438ffa6 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee @@ -48,7 +48,7 @@ module.exports = UserRegistrationHandler = if !requestIsValid return callback(new Error("request is not valid")) userDetails.email = userDetails.email?.trim()?.toLowerCase() - UserGetter.getUserByAnyEmail userDetails.email, (err, user) => + UserGetter.getUserByMainEmail userDetails.email, (err, user) => if err? return callback err if user?.holdingAccount == false diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index 22b31239bd..fa9ee24450 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -43,7 +43,7 @@ module.exports = UserUpdater = # Add a new email address for the user. Email cannot be already used by this # or any other user addEmailAddress: (userId, newEmail, callback) -> - UserGetter.ensureUniqueEmailAddress newEmail, (error) => + @_ensureUniqueEmailAddress newEmail, (error) => return callback(error) if error? update = $push: emails: email: newEmail, createdAt: new Date() @@ -81,11 +81,20 @@ module.exports = UserUpdater = return callback(new Error('Default email does not belong to user')) callback() + + # check for duplicate email address. This is also enforced at the DB level + _ensureUniqueEmailAddress: (newEmail, callback) -> + UserGetter.getUserByAnyEmail newEmail, (error, user) -> + return callback(message: 'alread_exists') if user? + callback() + + [ 'updateUser' 'changeEmailAddress' 'setDefaultEmailAddress' 'addEmailAddress' 'removeEmailAddress' + '_ensureUniqueEmailAddress' ].map (method) -> metrics.timeAsyncMethod(UserUpdater, method, 'mongo.UserUpdater', logger) diff --git a/services/web/app/coffee/models/User.coffee b/services/web/app/coffee/models/User.coffee index 71982ba40b..009f582b2a 100644 --- a/services/web/app/coffee/models/User.coffee +++ b/services/web/app/coffee/models/User.coffee @@ -8,10 +8,6 @@ ObjectId = Schema.ObjectId UserSchema = new Schema email : {type : String, default : ''} - emails: [{ - email: { type : String, default : '' }, - createdAt: { type : Date, default: () -> new Date() } - }], first_name : {type : String, default : ''} last_name : {type : String, default : ''} role : {type : String, default : ''} diff --git a/services/web/test/acceptance/coffee/RegistrationTests.coffee b/services/web/test/acceptance/coffee/RegistrationTests.coffee index 8bcf8c3685..89d1bf3299 100644 --- a/services/web/test/acceptance/coffee/RegistrationTests.coffee +++ b/services/web/test/acceptance/coffee/RegistrationTests.coffee @@ -128,20 +128,6 @@ describe "CSRF protection", -> expect(response.statusCode).to.equal 403 done() -describe "Register", -> - before -> - @user = new User() - - it 'Set emails attribute', (done) -> - @user.register (error, user) => - expect(error).to.not.exist - user.email.should.equal @user.email - user.emails.should.exist - user.emails.should.be.a 'array' - user.emails.length.should.equal 1 - user.emails[0].email.should.equal @user.email - done() - describe "LoginViaRegistration", -> before (done) -> diff --git a/services/web/test/acceptance/coffee/helpers/User.coffee b/services/web/test/acceptance/coffee/helpers/User.coffee index 793ac6cd9f..f83780b535 100644 --- a/services/web/test/acceptance/coffee/helpers/User.coffee +++ b/services/web/test/acceptance/coffee/helpers/User.coffee @@ -22,30 +22,9 @@ class User jar: @jar }) - setExtraAttributes: (user) -> - throw new Error("User does not exist") unless user?._id? - @id = user._id.toString() - @_id = user._id.toString() - @first_name = user.first_name - @referal_id = user.referal_id - get: (callback = (error, user)->) -> db.users.findOne { _id: ObjectId(@_id) }, callback - register: (callback = (error, user) ->) -> - return callback(new Error('User already registered')) if @_id? - @getCsrfToken (error) => - return callback(error) if error? - @request.post { - url: '/register' - json: { @email, @password } - }, (error, response, body) => - return callback(error) if error? - db.users.findOne { email: @email }, (error, user) => - return callback(error) if error? - @setExtraAttributes user - callback(null, user) - login: (callback = (error) ->) -> @loginWith(@email, callback) @@ -68,7 +47,11 @@ class User return callback(error) if error? UserUpdater.updateUser user._id, $set: emails: @emails, (error) => return callback(error) if error? - @setExtraAttributes user + @id = user?._id?.toString() + @_id = user?._id?.toString() + @first_name = user?.first_name + @referal_id = user?.referal_id + callback(null, @password) setFeatures: (features, callback = (error) ->) -> diff --git a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee index affef2dc31..4b57ff3697 100644 --- a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee +++ b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteControllerTests.coffee @@ -25,7 +25,7 @@ describe "CollaboratorsInviteController", -> @LimitationsManager = {} @UserGetter = - getUserByAnyEmail: sinon.stub() + getUserByMainEmail: sinon.stub() getUser: sinon.stub() @CollaboratorsInviteController = SandboxedModule.require modulePath, requires: @@ -716,7 +716,7 @@ describe "CollaboratorsInviteController", -> beforeEach -> @user = {_id: ObjectId().toString()} - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, @user) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, @user) it 'should callback with `true`', (done) -> @call (err, shouldAllow) => @@ -728,7 +728,7 @@ describe "CollaboratorsInviteController", -> beforeEach -> @user = null - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, @user) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, @user) it 'should callback with `false`', (done) -> @call (err, shouldAllow) => @@ -738,15 +738,15 @@ describe "CollaboratorsInviteController", -> it 'should have called getUser', (done) -> @call (err, shouldAllow) => - @UserGetter.getUserByAnyEmail.callCount.should.equal 1 - @UserGetter.getUserByAnyEmail.calledWith(@email, {_id: 1}).should.equal true + @UserGetter.getUserByMainEmail.callCount.should.equal 1 + @UserGetter.getUserByMainEmail.calledWith(@email, {_id: 1}).should.equal true done() describe 'when getUser produces an error', -> beforeEach -> @user = null - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, new Error('woops')) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, new Error('woops')) it 'should callback with an error', (done) -> @call (err, shouldAllow) => diff --git a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee index edad39a637..58b373f61f 100644 --- a/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee +++ b/services/web/test/unit/coffee/Collaborators/CollaboratorsInviteHandlerTests.coffee @@ -605,7 +605,7 @@ describe "CollaboratorsInviteHandler", -> _id: ObjectId() first_name: "jim" @existingUser = {_id: ObjectId()} - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, @existingUser) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, @existingUser) @fakeProject = _id: @project_id name: "some project" @@ -626,8 +626,8 @@ describe "CollaboratorsInviteHandler", -> it 'should call getUser', (done) -> @call (err) => - @UserGetter.getUserByAnyEmail.callCount.should.equal 1 - @UserGetter.getUserByAnyEmail.calledWith(@invite.email).should.equal true + @UserGetter.getUserByMainEmail.callCount.should.equal 1 + @UserGetter.getUserByMainEmail.calledWith(@invite.email).should.equal true done() it 'should call getProject', (done) -> @@ -671,7 +671,7 @@ describe "CollaboratorsInviteHandler", -> describe 'when the user does not exist', -> beforeEach -> - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, null, null) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, null, null) it 'should not produce an error', (done) -> @call (err) => @@ -680,8 +680,8 @@ describe "CollaboratorsInviteHandler", -> it 'should call getUser', (done) -> @call (err) => - @UserGetter.getUserByAnyEmail.callCount.should.equal 1 - @UserGetter.getUserByAnyEmail.calledWith(@invite.email).should.equal true + @UserGetter.getUserByMainEmail.callCount.should.equal 1 + @UserGetter.getUserByMainEmail.calledWith(@invite.email).should.equal true done() it 'should not call getProject', (done) -> @@ -698,7 +698,7 @@ describe "CollaboratorsInviteHandler", -> describe 'when the getUser produces an error', -> beforeEach -> - @UserGetter.getUserByAnyEmail = sinon.stub().callsArgWith(2, new Error('woops')) + @UserGetter.getUserByMainEmail = sinon.stub().callsArgWith(2, new Error('woops')) it 'should produce an error', (done) -> @call (err) => @@ -707,8 +707,8 @@ describe "CollaboratorsInviteHandler", -> it 'should call getUser', (done) -> @call (err) => - @UserGetter.getUserByAnyEmail.callCount.should.equal 1 - @UserGetter.getUserByAnyEmail.calledWith(@invite.email).should.equal true + @UserGetter.getUserByMainEmail.callCount.should.equal 1 + @UserGetter.getUserByMainEmail.calledWith(@invite.email).should.equal true done() it 'should not call getProject', (done) -> diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee index 9a70ca0411..bca9ac7600 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionGroupHandlerTests.coffee @@ -32,7 +32,7 @@ describe "SubscriptionGroupHandler", -> @UserGetter = getUser: sinon.stub() - getUserByAnyEmail: sinon.stub() + getUserByMainEmail: sinon.stub() @LimitationsManager = hasGroupMembersLimitReached: sinon.stub() @@ -71,11 +71,11 @@ describe "SubscriptionGroupHandler", -> describe "addUserToGroup", -> beforeEach -> @LimitationsManager.hasGroupMembersLimitReached.callsArgWith(1, null, false, @subscription) - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user) + @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) it "should find the user", (done)-> @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> - @UserGetter.getUserByAnyEmail.calledWith(@newEmail).should.equal true + @UserGetter.getUserByMainEmail.calledWith(@newEmail).should.equal true done() it "should add the user to the group", (done)-> @@ -102,7 +102,7 @@ describe "SubscriptionGroupHandler", -> done() it "should add an email invite if no user is found", (done) -> - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, null) + @UserGetter.getUserByMainEmail.callsArgWith(1, null, null) @Handler.addUserToGroup @adminUser_id, @newEmail, (err)=> @SubscriptionUpdater.addEmailInviteToGroup.calledWith(@adminUser_id, @newEmail).should.equal true done() diff --git a/services/web/test/unit/coffee/User/UserCreatorTests.coffee b/services/web/test/unit/coffee/User/UserCreatorTests.coffee index 3764c8a765..cc2b1ec150 100644 --- a/services/web/test/unit/coffee/User/UserCreatorTests.coffee +++ b/services/web/test/unit/coffee/User/UserCreatorTests.coffee @@ -70,11 +70,3 @@ describe "UserCreator", -> assert.equal user.holdingAccount, true assert.equal user.last_name, "lastNammmmeee" done() - - it "should set emails attribute", (done)-> - @UserCreator.createNewUser email: @email, (err, user)=> - user.email.should.equal @email - user.emails.length.should.equal 1 - user.emails[0].email.should.equal @email - user.emails[0].createdAt.should.be.a 'date' - done() diff --git a/services/web/test/unit/coffee/User/UserGetterTests.coffee b/services/web/test/unit/coffee/User/UserGetterTests.coffee index fbea0e8b53..7fb14a7f7d 100644 --- a/services/web/test/unit/coffee/User/UserGetterTests.coffee +++ b/services/web/test/unit/coffee/User/UserGetterTests.coffee @@ -85,20 +85,3 @@ describe "UserGetter", -> @findOne.calledTwice.should.equal true @findOne.calledWith(email: email, projection).should.equal true done() - - describe 'ensureUniqueEmailAddress', -> - beforeEach -> - @UserGetter.getUserByAnyEmail = sinon.stub() - - it 'should return error if existing user is found', (done)-> - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @fakeUser) - @UserGetter.ensureUniqueEmailAddress @newEmail, (err)=> - should.exist(err) - err.message.should.equal 'alread_exists' - done() - - it 'should return null if no user is found', (done)-> - @UserGetter.getUserByAnyEmail.callsArgWith(1) - @UserGetter.ensureUniqueEmailAddress @newEmail, (err)=> - should.not.exist(err) - done() diff --git a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee index e738947171..9411059022 100644 --- a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee +++ b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee @@ -14,7 +14,7 @@ describe "UserRegistrationHandler", -> @User = update: sinon.stub().callsArgWith(2) @UserGetter = - getUserByAnyEmail: sinon.stub() + getUserByMainEmail: sinon.stub() @UserCreator = createNewUser:sinon.stub().callsArgWith(1, null, @user) @AuthenticationManager = @@ -72,7 +72,7 @@ describe "UserRegistrationHandler", -> beforeEach -> @user.holdingAccount = true @handler._registrationRequestIsValid = sinon.stub().returns true - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user) + @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) it "should not create a new user if there is a holding account there", (done)-> @handler.registerNewUser @passingRequest, (err)=> @@ -96,7 +96,7 @@ describe "UserRegistrationHandler", -> done() it "should return email registered in the error if there is a non holdingAccount there", (done)-> - @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @user = {holdingAccount:false}) + @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user = {holdingAccount:false}) @handler.registerNewUser @passingRequest, (err, user)=> err.should.deep.equal new Error("EmailAlreadyRegistered") user.should.deep.equal @user @@ -105,7 +105,7 @@ describe "UserRegistrationHandler", -> describe "validRequest", -> beforeEach -> @handler._registrationRequestIsValid = sinon.stub().returns true - @UserGetter.getUserByAnyEmail.callsArgWith 1 + @UserGetter.getUserByMainEmail.callsArgWith 1 it "should create a new user", (done)-> @handler.registerNewUser @passingRequest, (err)=> diff --git a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee index 7b3be3df2b..b952a688ae 100644 --- a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee @@ -17,7 +17,6 @@ describe "UserUpdater", -> @UserGetter = getUserEmail: sinon.stub() getUserByAnyEmail: sinon.stub() - ensureUniqueEmailAddress: sinon.stub() @logger = err: sinon.stub(), log: -> @UserUpdater = SandboxedModule.require modulePath, requires: "settings-sharelatex":@settings @@ -61,13 +60,13 @@ describe "UserUpdater", -> describe 'addEmailAddress', -> beforeEach -> - @UserGetter.ensureUniqueEmailAddress = sinon.stub().callsArgWith(1) + @UserUpdater._ensureUniqueEmailAddress = sinon.stub().callsArgWith(1) it 'add email', (done)-> @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null) @UserUpdater.addEmailAddress @stubbedUser._id, @newEmail, (err)=> - @UserGetter.ensureUniqueEmailAddress.called.should.equal true + @UserUpdater._ensureUniqueEmailAddress.called.should.equal true should.not.exist(err) @UserUpdater.updateUser.calledWith( @stubbedUser._id, @@ -136,3 +135,15 @@ describe "UserUpdater", -> done() + describe '_ensureUniqueEmailAddress', -> + it 'should return error if existing user is found', (done)-> + @UserGetter.getUserByAnyEmail.callsArgWith(1, null, @stubbedUser) + @UserUpdater._ensureUniqueEmailAddress @newEmail, (err)=> + should.exist(err) + done() + + it 'should return null if no user is found', (done)-> + @UserGetter.getUserByAnyEmail.callsArgWith(1) + @UserUpdater._ensureUniqueEmailAddress @newEmail, (err)=> + should.not.exist(err) + done()