From d108b11bc02cc5d3cce592661b86a32ab43bd6fa Mon Sep 17 00:00:00 2001 From: Nate Stemen Date: Mon, 7 May 2018 17:25:29 -0400 Subject: [PATCH 01/34] Add latexmkrc in FileTypeManager --- .../Features/Uploads/FileTypeManager.coffee | 9 +++------ .../coffee/Uploads/FileTypeManagerTests.coffee | 18 ++++++++++++++---- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/services/web/app/coffee/Features/Uploads/FileTypeManager.coffee b/services/web/app/coffee/Features/Uploads/FileTypeManager.coffee index 918f66df7b..524c6915e0 100644 --- a/services/web/app/coffee/Features/Uploads/FileTypeManager.coffee +++ b/services/web/app/coffee/Features/Uploads/FileTypeManager.coffee @@ -3,7 +3,7 @@ Path = require("path") module.exports = FileTypeManager = TEXT_EXTENSIONS : [ - "tex", "latex", "sty", "cls", "bst", "bib", "bibtex", "txt", "tikz", "rtex", "md", "asy" + "tex", "latex", "sty", "cls", "bst", "bib", "bibtex", "txt", "tikz", "rtex", "md", "asy", "latexmkrc" ] IGNORE_EXTENSIONS : [ @@ -34,7 +34,7 @@ module.exports = FileTypeManager = extension = parts.slice(-1)[0] if extension? extension = extension.toLowerCase() - binaryFile = (@TEXT_EXTENSIONS.indexOf(extension) == -1 or parts.length <= 1) + binaryFile = (@TEXT_EXTENSIONS.indexOf(extension) == -1 or parts.length <= 1) and parts[0] != 'latexmkrc' if binaryFile return callback null, true @@ -52,13 +52,10 @@ module.exports = FileTypeManager = if extension? extension = extension.toLowerCase() ignore = false - if name[0] == "." + if name[0] == "." and extension != 'latexmkrc' ignore = true if @IGNORE_EXTENSIONS.indexOf(extension) != -1 ignore = true if @IGNORE_FILENAMES.indexOf(name) != -1 ignore = true callback null, ignore - - - diff --git a/services/web/test/unit/coffee/Uploads/FileTypeManagerTests.coffee b/services/web/test/unit/coffee/Uploads/FileTypeManagerTests.coffee index c6fdf64829..be456c74a5 100644 --- a/services/web/test/unit/coffee/Uploads/FileTypeManagerTests.coffee +++ b/services/web/test/unit/coffee/Uploads/FileTypeManagerTests.coffee @@ -39,7 +39,7 @@ describe "FileTypeManager", -> beforeEach -> @stat = { size: 100 } @fs.stat = sinon.stub().callsArgWith(1, null, @stat) - + it "should return .tex files as not binary", -> @FileTypeManager.isBinary "file.tex", "/path/on/disk", (error, binary) -> binary.should.equal false @@ -80,10 +80,18 @@ describe "FileTypeManager", -> @FileTypeManager.isBinary "tex", "/path/on/disk", (error, binary) -> binary.should.equal true + it "should return .latexmkrc file as not binary", -> + @FileTypeManager.isBinary ".latexmkrc", "/path/on/disk", (error, binary) -> + binary.should.equal false + + it "should return latexmkrc file as not binary", -> + @FileTypeManager.isBinary "latexmkrc", "/path/on/disk", (error, binary) -> + binary.should.equal false + it "should ignore the case of an extension", -> @FileTypeManager.isBinary "file.TEX", "/path/on/disk", (error, binary) -> binary.should.equal false - + it "should return large text files as binary", -> @stat.size = 2 * 1024 * 1024 # 2Mb @FileTypeManager.isBinary "file.tex", "/path/on/disk", (error, binary) -> @@ -98,6 +106,10 @@ describe "FileTypeManager", -> @FileTypeManager.shouldIgnore "path/.git", (error, ignore) -> ignore.should.equal true + it "should not ignore .latexmkrc dotfile", -> + @FileTypeManager.shouldIgnore "path/.latexmkrc", (error, ignore) -> + ignore.should.equal false + it "should ignore __MACOSX", -> @FileTypeManager.shouldIgnore "path/__MACOSX", (error, ignore) -> ignore.should.equal true @@ -109,5 +121,3 @@ describe "FileTypeManager", -> it "should ignore the case of the extension", -> @FileTypeManager.shouldIgnore "file.AUX", (error, ignore) -> ignore.should.equal true - - From d1756436e83dc4e47ad278b2e8d9e2b5a956e1e4 Mon Sep 17 00:00:00 2001 From: hugh-obrien Date: Fri, 11 May 2018 15:34:22 +0100 Subject: [PATCH 02/34] compile react in production mode --- services/web/Jenkinsfile | 2 +- services/web/webpack.config.js | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/services/web/Jenkinsfile b/services/web/Jenkinsfile index 7953aa0dbc..7b44d70315 100644 --- a/services/web/Jenkinsfile +++ b/services/web/Jenkinsfile @@ -101,7 +101,7 @@ pipeline { } } steps { - sh 'make minify' + sh 'WEBPACK_ENV=production make minify' } } diff --git a/services/web/webpack.config.js b/services/web/webpack.config.js index da0dc8257e..95f3fe5f9e 100644 --- a/services/web/webpack.config.js +++ b/services/web/webpack.config.js @@ -1,7 +1,9 @@ const fs = require('fs') const path = require('path') +const webpack = require('webpack') const MODULES_PATH = path.join(__dirname, '/modules') +const webpackENV = process.env.WEBPACK_ENV || 'development' // Generate a hash of entry points, including modules const entryPoints = {} @@ -63,7 +65,16 @@ module.exports = { }] }, - // TODO - // plugins: {} + plugins: [ + new webpack.DefinePlugin({ + // Swaps out checks for NODE_ENV with the env. This is used by various + // libs to enable dev-only features. These checks then become something + // like `if ('production' == 'production')`. Minification will then strip + // the dev-only code from the bundle + 'process.env': { + NODE_ENV: JSON.stringify(webpackENV) + }, + }) + ] } From 58b5d67bdd619e150fdcac3971108916ebc5b8db Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 14 May 2018 10:29:42 +0100 Subject: [PATCH 03/34] Enable the v2 banner for everyone if showV2Banner is set --- services/web/app/coffee/infrastructure/Features.coffee | 2 ++ services/web/app/views/project/list/notifications.pug | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/infrastructure/Features.coffee b/services/web/app/coffee/infrastructure/Features.coffee index 519cfda6da..b1899e38bc 100644 --- a/services/web/app/coffee/infrastructure/Features.coffee +++ b/services/web/app/coffee/infrastructure/Features.coffee @@ -14,6 +14,8 @@ module.exports = Features = return Settings.enableGithubSync when 'v1-return-message' return Settings.accountMerge? and Settings.overleaf? + when 'v2-banner' + return Settings.showV2Banner when 'custom-togglers' return Settings.overleaf? else diff --git a/services/web/app/views/project/list/notifications.pug b/services/web/app/views/project/list/notifications.pug index f173555e1c..ee004039fc 100644 --- a/services/web/app/views/project/list/notifications.pug +++ b/services/web/app/views/project/list/notifications.pug @@ -1,4 +1,4 @@ -if (user.awareOfV2 && !settings.overleaf) +if hasFeature('v2-banner') .userNotifications ul.list-unstyled.notifications-list(ng-controller="OverleafV2NotificationController", ng-show="visible") li.notification_entry From 66846cc68c4847434157ea57e4acdcaca4b88bdc Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 15 May 2018 10:17:04 +0100 Subject: [PATCH 04/34] Put the compile timeout upgrade behind an enableSubscriptions flag --- services/web/app/views/project/editor/pdf.pug | 68 +++++++++---------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/services/web/app/views/project/editor/pdf.pug b/services/web/app/views/project/editor/pdf.pug index a874a0b88b..26de2f35b1 100644 --- a/services/web/app/views/project/editor/pdf.pug +++ b/services/web/app/views/project/editor/pdf.pug @@ -371,40 +371,40 @@ div.full-size.pdf(ng-controller="PdfController") a.text-info(href="https://www.sharelatex.com/learn/Debugging_Compilation_timeout_errors", target="_blank") | #{translate("learn_how_to_make_documents_compile_quickly")} - .alert.alert-success(ng-show="pdf.timedout && !hasPremiumCompile") - p(ng-if="project.owner._id == user.id") - strong #{translate("upgrade_for_faster_compiles")} - p(ng-if="project.owner._id != user.id") - strong #{translate("ask_proj_owner_to_upgrade_for_faster_compiles")} - p #{translate("free_accounts_have_timeout_upgrade_to_increase")} - p Plus: - div - ul.list-unstyled - li - i.fa.fa-check   - | #{translate("unlimited_projects")} - li - i.fa.fa-check   - | #{translate("collabs_per_proj", {collabcount:'Multiple'})} - li - i.fa.fa-check   - | #{translate("full_doc_history")} - li - i.fa.fa-check   - | #{translate("sync_to_dropbox")} - li - i.fa.fa-check   - | #{translate("sync_to_github")} - li - i.fa.fa-check   - |#{translate("compile_larger_projects")} - p(ng-controller="FreeTrialModalController", ng-if="project.owner._id == user.id") - a.btn.btn-success.row-spaced-small( - href - ng-class="buttonClass" - ng-click="startFreeTrial('compile-timeout')" - ) #{translate("start_free_trial")} - + if settings.enableSubscriptions + .alert.alert-success(ng-show="pdf.timedout && !hasPremiumCompile") + p(ng-if="project.owner._id == user.id") + strong #{translate("upgrade_for_faster_compiles")} + p(ng-if="project.owner._id != user.id") + strong #{translate("ask_proj_owner_to_upgrade_for_faster_compiles")} + p #{translate("free_accounts_have_timeout_upgrade_to_increase")} + p Plus: + div + ul.list-unstyled + li + i.fa.fa-check   + | #{translate("unlimited_projects")} + li + i.fa.fa-check   + | #{translate("collabs_per_proj", {collabcount:'Multiple'})} + li + i.fa.fa-check   + | #{translate("full_doc_history")} + li + i.fa.fa-check   + | #{translate("sync_to_dropbox")} + li + i.fa.fa-check   + | #{translate("sync_to_github")} + li + i.fa.fa-check   + |#{translate("compile_larger_projects")} + p(ng-controller="FreeTrialModalController", ng-if="project.owner._id == user.id") + a.btn.btn-success.row-spaced-small( + href + ng-class="buttonClass" + ng-click="startFreeTrial('compile-timeout')" + ) #{translate("start_free_trial")} .alert.alert-danger(ng-show="pdf.autoCompileDisabled") p From b9c479c24553b95e6002774c5dbbecc2aa90c499 Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Tue, 20 Mar 2018 11:00:37 +0000 Subject: [PATCH 05/34] Update README description of acceptance tests. --- services/web/README.md | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/services/web/README.md b/services/web/README.md index 811e7fbd33..7c8cbf0155 100644 --- a/services/web/README.md +++ b/services/web/README.md @@ -3,11 +3,11 @@ web-sharelatex web-sharelatex is the front-end web service of the open-source web-based collaborative LaTeX editor, [ShareLaTeX](https://www.sharelatex.com). -It serves all the HTML pages, CSS and javascript to the client. web-sharelatex also contains +It serves all the HTML pages, CSS and javascript to the client. web-sharelatex also contains a lot of logic around creating and editing projects, and account management. -The rest of the ShareLaTeX stack, along with information about contributing can be found in the +The rest of the ShareLaTeX stack, along with information about contributing can be found in the [sharelatex/sharelatex](https://github.com/sharelatex/sharelatex) repository. Build process @@ -20,7 +20,7 @@ Image processing tasks are commented out in the gruntfile and the needed package New Docker-based build process ------------------------------ -Note that the Grunt workflow from above should still work, but we are transitioning to a +Note that the Grunt workflow from above should still work, but we are transitioning to a Docker based testing workflow, which is documented below: ### Running the app @@ -59,19 +59,18 @@ Acceptance tests are run against a live service, which runs in the `acceptance_t To run the tests out-of-the-box, the makefile defines: ``` -make install # Only needs running once, or when npm packages are updated -make acceptance_test +make test_acceptance ``` However, during development it is often useful to leave the service running for rapid iteration on the acceptance tests. This can be done with: ``` -make acceptance_test_start_service -make acceptance_test_run # Run as many times as needed during development -make acceptance_test_stop_service +make test_acceptance_app_start_service +make test_acceptance_app_run # Run as many times as needed during development +make test_acceptance_app_stop_service ``` -`make acceptance_test` just runs these three commands in sequence. +`make test_acceptance` just runs these three commands in sequence and then runs `make test_acceptance_modules` which performs the tests for each module in the `modules` directory. (Note that there is not currently an equivalent to the `-start` / `-run` x _n_ / `-stop` series for modules.) During development it is often useful to only run a subset of tests, which can be configured with arguments to the mocha CLI: @@ -111,12 +110,3 @@ We gratefully acknowledge [IconShock](http://www.iconshock.com) for use of the i in the `public/img/iconshock` directory found via [findicons.com](http://findicons.com/icon/498089/height?id=526085#) - -## Acceptance Tests - -To run the Acceptance tests: - -- set `allowPublicAccess` to true, either in the configuration file, - or by setting the environment variable `SHARELATEX_ALLOW_PUBLIC_ACCESS` to `true` -- start the server (`grunt`) -- in a separate terminal, run `grunt test:acceptance` From 859858c02c6bb034c32e188a6e610f26e0c634cd Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Thu, 22 Mar 2018 09:41:22 +0000 Subject: [PATCH 06/34] Add V1 API to settings file --- services/web/config/settings.defaults.coffee | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/services/web/config/settings.defaults.coffee b/services/web/config/settings.defaults.coffee index d53b1a73ba..0892804778 100644 --- a/services/web/config/settings.defaults.coffee +++ b/services/web/config/settings.defaults.coffee @@ -156,6 +156,10 @@ module.exports = settings = url: process.env['LINKED_URL_PROXY'] thirdpartyreferences: url: "http://#{process.env['THIRD_PARTY_REFERENCES_HOST'] or 'localhost'}:3046" + v1: + url: "http://#{process.env['V1_HOST'] or 'localhost'}:5000" + user: 'overleaf' + pass: 'password' templates: user_id: process.env.TEMPLATES_USER_ID or "5395eb7aad1f29a88756c7f2" @@ -363,7 +367,7 @@ module.exports = settings = appName: "ShareLaTeX (Community Edition)" adminEmail: "placeholder@example.com" - + brandPrefix: "" # Set to 'ol-' for overleaf styles nav: From a661084485334553847aba8d54f31a137e2a9d0b Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Fri, 16 Mar 2018 10:25:40 +0000 Subject: [PATCH 07/34] Add ExportsHander for project exports performed via v1 --- .../Features/Exports/ExportsHandler.coffee | 93 ++++++++ .../coffee/Exports/ExportsHandlerTests.coffee | 202 ++++++++++++++++++ 2 files changed, 295 insertions(+) create mode 100644 services/web/app/coffee/Features/Exports/ExportsHandler.coffee create mode 100644 services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee diff --git a/services/web/app/coffee/Features/Exports/ExportsHandler.coffee b/services/web/app/coffee/Features/Exports/ExportsHandler.coffee new file mode 100644 index 0000000000..727b01a575 --- /dev/null +++ b/services/web/app/coffee/Features/Exports/ExportsHandler.coffee @@ -0,0 +1,93 @@ +ProjectGetter = require('../Project/ProjectGetter') +ProjectLocator = require('../Project/ProjectLocator') +UserGetter = require('../User/UserGetter') +logger = require('logger-sharelatex') +settings = require 'settings-sharelatex' +async = require 'async' +request = require 'request' +request = request.defaults() +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) -> + return callback(err) if err? + self._requestExport export_data, (err, export_v1_id) -> + return callback(err) if err? + export_data.v1_id = export_v1_id + # TODO: possibly store the export data in Mongo + callback null, export_data + + _buildExport: (project_id, user_id, brand_variation_id, callback=(err, export_data) ->) -> + jobs = + project: (cb) -> + ProjectGetter.getProject project_id, cb + # TODO: when we update async, signature will change from (cb, results) to (results, cb) + rootDoc: [ 'project', (cb, results) -> + ProjectLocator.findRootDoc {project: results.project, project_id: project_id}, cb + ] + user: (cb) -> + UserGetter.getUser user_id, {first_name: 1, last_name: 1, email: 1}, cb + historyVersion: (cb) -> + self._requestVersion project_id, cb + + async.auto jobs, (err, results) -> + if err? + logger.err err:err, project_id:project_id, user_id:user_id, brand_variation_id:brand_variation_id, "error building project export" + return callback(err) + + {project, rootDoc, user, historyVersion} = results + if !rootDoc[1]? + err = new Error("cannot export project without root doc") + logger.err err:err, project_id: project_id + return callback(err) + + export_data = + project: + id: project_id + rootDocPath: rootDoc[1]?.fileSystem + historyId: project.overleaf?.history?.id + historyVersion: historyVersion + user: + id: user_id + firstName: user.first_name + lastName: user.last_name + email: user.email + orcidId: null # until v2 gets ORCID + destination: + brandVariationId: brand_variation_id + options: + callbackUrl: null # for now, until we want v1 to call us back + callback null, export_data + + _requestExport: (export_data, callback=(err, export_v1_id) ->) -> + request.post { + url: "#{settings.apis.v1.url}/api/v1/sharelatex/exports" + auth: {user: settings.apis.v1.user, pass: settings.apis.v1.pass } + json: export_data + }, (err, res, body) -> + if err? + logger.err err:err, export:export_data, "error making request to v1 export" + callback err + else if 200 <= res.statusCode < 300 + callback null, body.exportId + else + err = new Error("v1 export returned a failure status code: #{res.statusCode}") + logger.err err:err, export:export_data, "v1 export returned failure status code: #{res.statusCode}" + callback err + + _requestVersion: (project_id, callback=(err, export_v1_id) ->) -> + request.get { + url: "#{settings.apis.project_history.url}/project/#{project_id}/version" + json: true + }, (err, res, body) -> + if err? + logger.err err:err, project_id:project_id, "error making request to project history" + callback err + else if res.statusCode >= 200 and res.statusCode < 300 + callback null, body.version + else + err = new Error("project history version returned a failure status code: #{res.statusCode}") + logger.err err:err, project_id:project_id, "project history version returned failure status code: #{res.statusCode}" + callback err diff --git a/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee b/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee new file mode 100644 index 0000000000..6333db8270 --- /dev/null +++ b/services/web/test/unit/coffee/Exports/ExportsHandlerTests.coffee @@ -0,0 +1,202 @@ +sinon = require('sinon') +chai = require('chai') +should = chai.should() +expect = chai.expect +modulePath = '../../../../app/js/Features/Exports/ExportsHandler.js' +SandboxedModule = require('sandboxed-module') + +describe 'ExportsHandler', -> + + beforeEach -> + @ProjectGetter = {} + @ProjectLocator = {} + @UserGetter = {} + @settings = {} + @stubRequest = {} + @request = defaults: => return @stubRequest + @ExportsHandler = SandboxedModule.require modulePath, requires: + 'logger-sharelatex': + log: -> + err: -> + '../Project/ProjectGetter': @ProjectGetter + '../Project/ProjectLocator': @ProjectLocator + '../User/UserGetter': @UserGetter + 'settings-sharelatex': @settings + 'request': @request + @project_id = "project-id-123" + @project_history_id = 987 + @user_id = "user-id-456" + @brand_variation_id = 789 + @callback = sinon.stub() + + describe 'exportProject', -> + beforeEach (done) -> + @export_data = {iAmAnExport: true} + @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) => + @callback(error, export_data) + done() + + it "should build the export", -> + @ExportsHandler._buildExport + .calledWith(@project_id, @user_id, @brand_variation_id) + .should.equal true + + it "should request the export", -> + @ExportsHandler._requestExport + .calledWith(@export_data) + .should.equal true + + it "should return the export", -> + @callback + .calledWith(null, @export_data) + .should.equal true + + describe '_buildExport', -> + beforeEach (done) -> + @project = + id: @project_id + overleaf: + history: + id: @project_history_id + @user = + id: @user_id + first_name: 'Arthur' + last_name: 'Author' + email: 'arthur.author@arthurauthoring.org' + @rootDocPath = 'main.tex' + @historyVersion = 777 + @ProjectGetter.getProject = sinon.stub().yields(null, @project) + @ProjectLocator.findRootDoc = sinon.stub().yields(null, [null, {fileSystem: 'main.tex'}]) + @UserGetter.getUser = sinon.stub().yields(null, @user) + @ExportsHandler._requestVersion = sinon.stub().yields(null, @historyVersion) + done() + + describe "when all goes well", -> + beforeEach (done) -> + @ExportsHandler._buildExport @project_id, @user_id, @brand_variation_id, (error, export_data) => + @callback(error, export_data) + done() + + it "should request the project history version", -> + @ExportsHandler._requestVersion.called + .should.equal true + + it "should return export data", -> + expected_export_data = + project: + id: @project_id + rootDocPath: @rootDocPath + historyId: @project_history_id + historyVersion: @historyVersion + user: + id: @user_id + firstName: @user.first_name + lastName: @user.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) -> + @ProjectGetter.getProject = sinon.stub().yields(new Error("project not found")) + @ExportsHandler._buildExport @project_id, @user_id, @brand_variation_id, (error, export_data) => + @callback(error, export_data) + done() + + it "should return an error", -> + (@callback.args[0][0] instanceof Error) + .should.equal true + + 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) => + @callback(error, export_data) + done() + + it "should return an error", -> + (@callback.args[0][0] instanceof Error) + .should.equal true + + 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) => + @callback(error, export_data) + done() + + it "should return an error", -> + (@callback.args[0][0] instanceof Error) + .should.equal true + + 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) => + @callback(error, export_data) + done() + + it "should return an error", -> + (@callback.args[0][0] instanceof Error) + .should.equal true + + describe '_requestExport', -> + beforeEach (done) -> + @settings.apis = + v1: + url: 'http://localhost:5000' + user: 'overleaf' + pass: 'pass' + @export_data = {iAmAnExport: true} + @export_id = 4096 + @stubPost = sinon.stub().yields(null, {statusCode: 200}, { exportId: @export_id }) + done() + + describe "when all goes well", -> + beforeEach (done) -> + @stubRequest.post = @stubPost + @ExportsHandler._requestExport @export_data, (error, export_v1_id) => + @callback(error, export_v1_id) + done() + + it 'should issue the request', -> + expect(@stubPost.getCall(0).args[0]).to.deep.equal + url: @settings.apis.v1.url + '/api/v1/sharelatex/exports' + auth: + user: @settings.apis.v1.user + pass: @settings.apis.v1.pass + json: @export_data + + it 'should return the v1 export id', -> + @callback.calledWith(null, @export_id) + .should.equal true + + describe "when the request fails", -> + beforeEach (done) -> + @stubRequest.post = sinon.stub().yields(new Error("export request failed")) + @ExportsHandler._requestExport @export_data, (error, export_v1_id) => + @callback(error, export_v1_id) + done() + + it "should return an error", -> + (@callback.args[0][0] instanceof Error) + .should.equal true + + describe "when the request returns an error code", -> + beforeEach (done) -> + @stubRequest.post = sinon.stub().yields(null, {statusCode: 401}, { }) + @ExportsHandler._requestExport @export_data, (error, export_v1_id) => + @callback(error, export_v1_id) + done() + + it "should return the error", -> + (@callback.args[0][0] instanceof Error) + .should.equal true From e34dd90a1f729a5150c53db068ea16a0e1c0ae46 Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Fri, 23 Mar 2018 16:30:48 +0000 Subject: [PATCH 08/34] Add project export route and controller --- .../Features/Exports/ExportsController.coffee | 18 +++++++++ services/web/app/coffee/router.coffee | 2 + .../Exports/ExportsControllerTests.coffee | 39 +++++++++++++++++++ 3 files changed, 59 insertions(+) create mode 100644 services/web/app/coffee/Features/Exports/ExportsController.coffee create mode 100644 services/web/test/unit/coffee/Exports/ExportsControllerTests.coffee diff --git a/services/web/app/coffee/Features/Exports/ExportsController.coffee b/services/web/app/coffee/Features/Exports/ExportsController.coffee new file mode 100644 index 0000000000..cda2296249 --- /dev/null +++ b/services/web/app/coffee/Features/Exports/ExportsController.coffee @@ -0,0 +1,18 @@ +ExportsHandler = require("./ExportsHandler") +AuthenticationController = require("../Authentication/AuthenticationController") +logger = require("logger-sharelatex") + +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) -> + logger.log + user_id:user_id + project_id: project_id + brand_variation_id:brand_variation_id + export_v1_id:export_data.v1_id + "exported project" + res.send export_v1_id: export_data.v1_id + diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 7ec4dafbf4..e6b2692f7c 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -26,6 +26,7 @@ HealthCheckController = require("./Features/HealthCheck/HealthCheckController") ProjectDownloadsController = require "./Features/Downloads/ProjectDownloadsController" FileStoreController = require("./Features/FileStore/FileStoreController") HistoryController = require("./Features/History/HistoryController") +ExportsController = require("./Features/Exports/ExportsController") PasswordResetRouter = require("./Features/PasswordReset/PasswordResetRouter") StaticPagesRouter = require("./Features/StaticPages/StaticPagesRouter") ChatController = require("./Features/Chat/ChatController") @@ -205,6 +206,7 @@ module.exports = class Router webRouter.post "/project/:project_id/restore_file", AuthorizationMiddlewear.ensureUserCanWriteProjectContent, HistoryController.restoreFileFromV2 privateApiRouter.post "/project/:Project_id/history/resync", AuthenticationController.httpAuth, HistoryController.resyncProjectHistory + webRouter.post '/project/:project_id/export/:brand_variation_id', AuthorizationMiddlewear.ensureUserCanAdminProject, ExportsController.exportProject webRouter.get '/Project/:Project_id/download/zip', AuthorizationMiddlewear.ensureUserCanReadProject, ProjectDownloadsController.downloadProject webRouter.get '/project/download/zip', AuthorizationMiddlewear.ensureUserCanReadMultipleProjects, ProjectDownloadsController.downloadMultipleProjects diff --git a/services/web/test/unit/coffee/Exports/ExportsControllerTests.coffee b/services/web/test/unit/coffee/Exports/ExportsControllerTests.coffee new file mode 100644 index 0000000000..821eb5b4e0 --- /dev/null +++ b/services/web/test/unit/coffee/Exports/ExportsControllerTests.coffee @@ -0,0 +1,39 @@ +SandboxedModule = require('sandboxed-module') +assert = require('assert') +chai = require('chai') +expect = chai.expect +sinon = require('sinon') +modulePath = require('path').join __dirname, '../../../../app/js/Features/Exports/ExportsController.js' + + +describe 'ExportsController', -> + project_id = "123njdskj9jlk" + user_id = "123nd3ijdks" + brand_variation_id = 22 + + beforeEach -> + @handler = + getUserNotifications: sinon.stub().callsArgWith(1) + @req = + params: + project_id: project_id + brand_variation_id: brand_variation_id + session: + user: + _id:user_id + i18n: + translate:-> + @AuthenticationController = + getLoggedInUserId: sinon.stub().returns(@req.session.user._id) + @controller = SandboxedModule.require modulePath, requires: + "./ExportsHandler":@handler + 'logger-sharelatex': + log:-> + err:-> + '../Authentication/AuthenticationController': @AuthenticationController + + it 'should ask the handler to perform the export', (done) -> + @handler.exportProject = sinon.stub().yields(null, {iAmAnExport: true, v1_id: 897}) + @controller.exportProject @req, send:(body) => + expect(body).to.deep.equal {export_v1_id: 897} + done() From 3922b8b9165579c5675a0df59fba3c2384dda461 Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Thu, 22 Mar 2018 09:49:12 +0000 Subject: [PATCH 09/34] Add project export acceptance tests - Add acceptance tests - Add `MockV1Api` helper - Add flush endpoint to `MockProjectHistoryApi` helper --- .../acceptance/coffee/ExportsTests.coffee | 56 +++++++++++++++++++ .../helpers/MockProjectHistoryApi.coffee | 12 ++++ .../coffee/helpers/MockV1Api.coffee | 34 +++++++++++ 3 files changed, 102 insertions(+) create mode 100644 services/web/test/acceptance/coffee/ExportsTests.coffee create mode 100644 services/web/test/acceptance/coffee/helpers/MockV1Api.coffee diff --git a/services/web/test/acceptance/coffee/ExportsTests.coffee b/services/web/test/acceptance/coffee/ExportsTests.coffee new file mode 100644 index 0000000000..7a6784008d --- /dev/null +++ b/services/web/test/acceptance/coffee/ExportsTests.coffee @@ -0,0 +1,56 @@ +expect = require('chai').expect +request = require './helpers/request' +_ = require 'underscore' + + +User = require './helpers/User' +ProjectGetter = require '../../../app/js/Features/Project/ProjectGetter.js' +ExportsHandler = require '../../../app/js/Features/Exports/ExportsHandler.js' + +MockProjectHistoryApi = require './helpers/MockProjectHistoryApi' +MockV1Api = require './helpers/MockV1Api' + +describe 'Exports', -> + before (done) -> + @brand_variation_id = '18' + @owner = new User() + @owner.login (error) => + throw error if error? + @owner.createProject 'example-project', {template: 'example'}, (error, @project_id) => + throw error if error? + done() + + describe 'exporting a project', -> + beforeEach (done) -> + @version = Math.floor(Math.random() * 10000) + MockProjectHistoryApi.setProjectVersion(@project_id, @version) + @export_id = Math.floor(Math.random() * 10000) + MockV1Api.setExportId(@export_id) + MockV1Api.clearExportParams() + @owner.request { + method: 'POST', + url: "/project/#{@project_id}/export/#{@brand_variation_id}", + json: {}, + }, (error, response, body) => + throw error if error? + expect(response.statusCode).to.equal 200 + @exportResponseBody = body + done() + + it 'should have sent correct data to v1', (done) -> + {project, user, destination, options} = MockV1Api.getLastExportParams() + # project details should match + expect(project.id).to.equal @project_id + expect(project.rootDocPath).to.equal '/main.tex' + # version should match what was retrieved from project-history + expect(project.historyVersion).to.equal @version + # user details should match + expect(user.id).to.equal @owner.id + expect(user.email).to.equal @owner.email + # brand-variation should match + expect(destination.brandVariationId).to.equal @brand_variation_id + done() + + it 'should have returned the export ID provided by v1', (done) -> + expect(@exportResponseBody.export_v1_id).to.equal @export_id + done() diff --git a/services/web/test/acceptance/coffee/helpers/MockProjectHistoryApi.coffee b/services/web/test/acceptance/coffee/helpers/MockProjectHistoryApi.coffee index 381d7ab272..b7df202d72 100644 --- a/services/web/test/acceptance/coffee/helpers/MockProjectHistoryApi.coffee +++ b/services/web/test/acceptance/coffee/helpers/MockProjectHistoryApi.coffee @@ -6,9 +6,14 @@ module.exports = MockProjectHistoryApi = oldFiles: {} + projectVersions: {} + addOldFile: (project_id, version, pathname, content) -> @oldFiles["#{project_id}:#{version}:#{pathname}"] = content + setProjectVersion: (project_id, version) -> + @projectVersions[project_id] = version + run: () -> app.post "/project", (req, res, next) => res.json project: id: 1 @@ -21,6 +26,13 @@ module.exports = MockProjectHistoryApi = else res.send 404 + app.get "/project/:project_id/version", (req, res, next) => + {project_id} = req.params + if @projectVersions[project_id]? + res.json version: @projectVersions[project_id] + else + res.send 404 + app.listen 3054, (error) -> throw error if error? .on "error", (error) -> diff --git a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee new file mode 100644 index 0000000000..f27a4e1662 --- /dev/null +++ b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee @@ -0,0 +1,34 @@ +express = require("express") +app = express() +bodyParser = require('body-parser') + +app.use(bodyParser.json()) + +module.exports = MockV1Api = + + exportId: null + + exportParams: null + + setExportId: (id) -> + @exportId = id + + getLastExportParams: () -> + @exportParams + + clearExportParams: () -> + @exportParams = null + + run: () -> + 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) -> + console.error "error starting MockOverleafAPI:", error.message + process.exit(1) + +MockV1Api.run() From 3724edfc10c43ef831e0efa01dc60f67a9226ec2 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 17 May 2018 11:25:14 +0100 Subject: [PATCH 10/34] Fix translation string interpolation --- services/web/app/views/contact-us-modal.pug | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/views/contact-us-modal.pug b/services/web/app/views/contact-us-modal.pug index aad68a53d3..eacfd10533 100644 --- a/services/web/app/views/contact-us-modal.pug +++ b/services/web/app/views/contact-us-modal.pug @@ -22,7 +22,7 @@ script(type='text/ng-template', id='supportModalTemplate') tabindex='1', onkeyup='') .contact-suggestions(ng-show="suggestions.length") - p.contact-suggestion-label !{translate("kb_suggestions_enquiry", { kbLink: "__kb__", kb: translate("knowledge_base") })} + p.contact-suggestion-label !{translate("kb_suggestions_enquiry", { kbLink: "" + translate("knowledge_base") + "" })} ul.contact-suggestion-list li(ng-repeat="suggestion in suggestions") a.contact-suggestion-list-item(ng-href="{{ suggestion.url }}", ng-click="clickSuggestionLink(suggestion.url);" target="_blank") From 0fd29d481928247689780c1734862fdff779f275 Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Thu, 17 May 2018 09:43:19 -0500 Subject: [PATCH 11/34] Remove missing wiki controller --- services/web/public/coffee/main/learn.coffee | 28 -------------------- 1 file changed, 28 deletions(-) diff --git a/services/web/public/coffee/main/learn.coffee b/services/web/public/coffee/main/learn.coffee index eeae4219b8..375c2f0365 100644 --- a/services/web/public/coffee/main/learn.coffee +++ b/services/web/public/coffee/main/learn.coffee @@ -55,32 +55,4 @@ define [ hits = _.map response.hits, buildHitViewModel updateHits hits - $scope.showMissingTemplateModal = () -> - modalInstance = $modal.open( - templateUrl: "missingWikiPageModal" - controller: "MissingWikiPageController" - ) - - App.controller 'MissingWikiPageController', ($scope, $modalInstance) -> - $scope.form = {} - $scope.sent = false - $scope.sending = false - $scope.contactUs = -> - if !$scope.form.message? - console.log "message not set" - return - $scope.sending = true - ticketNumber = Math.floor((1 + Math.random()) * 0x10000).toString(32) - params = - email: $scope.form.email or "support@sharelatex.com" - message: $scope.form.message or "" - subject: "new wiki page sujection - [#{ticketNumber}]" - labels: "support wiki" - - Groove.createTicket params, (err, json)-> - $scope.sent = true - $scope.$apply() - - $scope.close = () -> - $modalInstance.close() From 37ca7b54a69186c0ed0bffd75ebfc467f83eff04 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 24 Apr 2018 10:27:17 +0100 Subject: [PATCH 12/34] Re-implement spell check manager with adapter to abstract away editor --- .../spell-check/SpellCheckManager.coffee | 184 +++++------------- 1 file changed, 51 insertions(+), 133 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee index acbd636531..4dee1faf52 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee @@ -1,129 +1,38 @@ -define [ - "ide/editor/directives/aceEditor/spell-check/HighlightedWordManager" - "ace/ace" -], (HighlightedWordManager) -> - Range = ace.require("ace/range").Range - +define [], () -> class SpellCheckManager - constructor: (@$scope, @editor, @element, @cache, @$http, @$q) -> - $(document.body).append @element.find(".spell-check-menu") + constructor: (@$scope, @cache, @$http, @$q, @adapter) -> @inProgressRequest = null @updatedLines = [] - @highlightedWordManager = new HighlightedWordManager(@editor) - @$scope.$watch "spellCheckLanguage", (language, oldLanguage) => + @$scope.$watch 'spellCheckLanguage', (language, oldLanguage) => if language != oldLanguage and oldLanguage? @runFullCheck() - onChange = (e) => - @runCheckOnChange(e) - - onScroll = () => - @closeContextMenu() + init: () -> + @updatedLines = Array(@adapter.getLines().length).fill(true) + @runSpellCheckSoon(200) if @isSpellCheckEnabled() - @editor.on "changeSession", (e) => - @highlightedWordManager.reset() - if @inProgressRequest? - @inProgressRequest.abort() + isSpellCheckEnabled: () -> + return !!( + @$scope.spellCheck and + @$scope.spellCheckLanguage and + @$scope.spellCheckLanguage != '' + ) - if @$scope.spellCheckEnabled and @$scope.spellCheckLanguage and @$scope.spellCheckLanguage != "" - @runSpellCheckSoon(200) - - e.oldSession?.getDocument().off "change", onChange - e.session.getDocument().on "change", onChange - - e.oldSession?.off "changeScrollTop", onScroll - e.session.on "changeScrollTop", onScroll - - @$scope.spellingMenu = {left: '0px', top: '0px'} - - @editor.on "nativecontextmenu", (e) => - e.domEvent.stopPropagation(); - @closeContextMenu(e.domEvent) - @openContextMenu(e.domEvent) - - $(document).on "click", (e) => - if e.which != 3 # Ignore if this was a right click - @closeContextMenu(e) - return true - - @$scope.replaceWord = (highlight, suggestion) => - @replaceWord(highlight, suggestion) - - @$scope.learnWord = (highlight) => - @learnWord(highlight) - - runFullCheck: () -> - @highlightedWordManager.clearRows() - if @$scope.spellCheckLanguage and @$scope.spellCheckLanguage != "" - @runSpellCheck() - - runCheckOnChange: (e) -> - if @$scope.spellCheckLanguage and @$scope.spellCheckLanguage != "" - @highlightedWordManager.applyChange(e) - @markLinesAsUpdated(e) + onChange: (e) => + if @isSpellCheckEnabled() + @markLinesAsUpdated(@adapter.normalizeChangeEvent(e)) @runSpellCheckSoon() - openContextMenu: (e) -> - position = @editor.renderer.screenToTextCoordinates(e.clientX, e.clientY) - highlight = @highlightedWordManager.findHighlightWithinRange - start: position - end: position + onSessionChange: () => + @adapter.wordManager.reset() + @inProgressRequest.abort() if @inProgressRequest? - @$scope.$apply () => - @$scope.spellingMenu.highlight = highlight + @runSpellCheckSoon(200) if @isSpellCheckEnabled() - if highlight - e.stopPropagation() - e.preventDefault() - - @editor.getSession().getSelection().setSelectionRange( - new Range( - highlight.row, highlight.column - highlight.row, highlight.column + highlight.word.length - ) - ) - - @$scope.$apply () => - @$scope.spellingMenu.open = true - @$scope.spellingMenu.left = e.clientX + 'px' - @$scope.spellingMenu.top = e.clientY + 'px' - return false - - closeContextMenu: (e) -> - # this is triggered on scroll, so for performance only apply - # setting when it changes - if @$scope?.spellingMenu?.open != false - @$scope.$apply () => - @$scope.spellingMenu.open = false - - replaceWord: (highlight, text) -> - @editor.getSession().replace(new Range( - highlight.row, highlight.column, - highlight.row, highlight.column + highlight.word.length - ), text) - - learnWord: (highlight) -> - @apiRequest "/learn", word: highlight.word - @highlightedWordManager.removeWord highlight.word - language = @$scope.spellCheckLanguage - @cache?.put("#{language}:#{highlight.word}", true) - - getHighlightedWordAtCursor: () -> - cursor = @editor.getCursorPosition() - highlight = @highlightedWordManager.findHighlightWithinRange - start: cursor - end: cursor - return highlight - - runSpellCheckSoon: (delay = 1000) -> - run = () => - delete @timeoutId - @runSpellCheck(@updatedLines) - @updatedLines = [] - if @timeoutId? - clearTimeout @timeoutId - @timeoutId = setTimeout run, delay + runFullCheck: () -> + @adapter.wordManager.reset() + @runSpellCheck() if @isSpellCheckEnabled() markLinesAsUpdated: (change) -> start = change.start @@ -146,6 +55,15 @@ define [ @updatedLines[start.row] = true removeLines() + runSpellCheckSoon: (delay = 1000) -> + run = () => + delete @timeoutId + @runSpellCheck(@updatedLines) + @updatedLines = [] + if @timeoutId? + clearTimeout @timeoutId + @timeoutId = setTimeout run, delay + runSpellCheck: (linesToProcess) -> {words, positions} = @getWords(linesToProcess) language = @$scope.spellCheckLanguage @@ -178,11 +96,11 @@ define [ displayResult = (highlights) => if linesToProcess? for shouldProcess, row in linesToProcess - @highlightedWordManager.clearRows(row, row) if shouldProcess + @adapter.wordManager.clearRow(row) if shouldProcess else - @highlightedWordManager.clearRows() + @adapter.wordManager.reset() for highlight in highlights - @highlightedWordManager.addHighlight highlight + @adapter.wordManager.addHighlight highlight if not words.length displayResult highlights @@ -212,8 +130,24 @@ define [ seen[key] = true displayResult highlights + apiRequest: (endpoint, data, callback = (error, result) ->)-> + data.token = window.user.id + data._csrf = window.csrfToken + # use angular timeout option to cancel request if doc is changed + requestHandler = @$q.defer() + options = {timeout: requestHandler.promise} + httpRequest = @$http.post("/spelling" + endpoint, data, options) + .then (response) => + callback(null, response.data) + .catch (response) => + callback(new Error('api failure')) + # provide a method to cancel the request + abortRequest = () -> + requestHandler.resolve() + return { abort: abortRequest } + getWords: (linesToProcess) -> - lines = @editor.getValue().split("\n") + lines = @adapter.getLines() words = [] positions = [] for line, row in lines @@ -232,22 +166,6 @@ define [ words.push(word) return words: words, positions: positions - apiRequest: (endpoint, data, callback = (error, result) ->)-> - data.token = window.user.id - data._csrf = window.csrfToken - # use angular timeout option to cancel request if doc is changed - requestHandler = @$q.defer() - options = {timeout: requestHandler.promise} - httpRequest = @$http.post("/spelling" + endpoint, data, options) - .then (response) => - callback(null, response.data) - .catch (response) => - callback(new Error('api failure')) - # provide a method to cancel the request - abortRequest = () -> - requestHandler.resolve() - return { abort: abortRequest } - blacklistedCommandRegex: /// \\ # initial backslash (label # any of these commands From 22e41cdce75c02af3ce87ea29fe24629eef7bce5 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 24 Apr 2018 10:27:17 +0100 Subject: [PATCH 13/34] Simplify word manager to use Range + Anchor to automatically keep marker positions up-to-date Re-implement highlighted word manager to be simpler --- .../spell-check/HighlightedWordManager.coffee | 138 +++--------------- 1 file changed, 22 insertions(+), 116 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee index 5014559562..ba7561411d 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee @@ -13,18 +13,31 @@ define [ class HighlightedWordManager constructor: (@editor) -> @reset() - + reset: () -> + @highlights?.rows.forEach (highlight) => + @editor.getSession().removeMarker(highlight.markerId) @highlights = rows: [] addHighlight: (highlight) -> unless highlight instanceof Highlight highlight = new Highlight(highlight) - range = new Range( - highlight.row, highlight.column, - highlight.row, highlight.column + highlight.word.length - ) - highlight.markerId = @editor.getSession().addMarker range, "spelling-highlight", 'text', false + + session = @editor.getSession() + doc = session.getDocument() + # Set up Range that will automatically update it's positions when the + # document changes + range = new Range() + range.start = doc.createAnchor({ + row: highlight.row, + column: highlight.column + }) + range.end = doc.createAnchor({ + row: highlight.row, + column: highlight.column + highlight.word.length + }) + + highlight.markerId = session.addMarker range, "spelling-highlight", 'text', false @highlights.rows[highlight.row] ||= [] @highlights.rows[highlight.row].push highlight @@ -34,114 +47,7 @@ define [ if h == highlight @highlights.rows[highlight.row].splice(i, 1) - removeWord: (word) -> - toRemove = [] - for row in @highlights.rows - for highlight in (row || []) - if highlight.word == word - toRemove.push(highlight) - for highlight in toRemove + clearRow: (row) -> + row = @highlights.rows[row] + for highlight in (row || []).slice() @removeHighlight highlight - - moveHighlight: (highlight, position) -> - @removeHighlight highlight - highlight.row = position.row - highlight.column = position.column - @addHighlight highlight - - clearRows: (from, to) -> - from ||= 0 - to ||= @highlights.rows.length - 1 - for row in @highlights.rows.slice(from, to + 1) - for highlight in (row || []).slice(0) - @removeHighlight highlight - - insertRows: (offset, number) -> - # rows are inserted after offset. i.e. offset row is not modified - affectedHighlights = [] - for row in @highlights.rows.slice(offset) - affectedHighlights.push(highlight) for highlight in (row || []) - for highlight in affectedHighlights - @moveHighlight highlight, - row: highlight.row + number - column: highlight.column - - removeRows: (offset, number) -> - # offset is the first row to delete - affectedHighlights = [] - for row in @highlights.rows.slice(offset) - affectedHighlights.push(highlight) for highlight in (row || []) - for highlight in affectedHighlights - if highlight.row >= offset + number - @moveHighlight highlight, - row: highlight.row - number - column: highlight.column - else - @removeHighlight highlight - - findHighlightWithinRange: (range) -> - rows = @highlights.rows.slice(range.start.row, range.end.row + 1) - for row in rows - for highlight in (row || []) - if @_doesHighlightOverlapRange(highlight, range.start, range.end) - return highlight - return null - - applyChange: (change) -> - start = change.start - end = change.end - if change.action == "insert" - if start.row != end.row - rowsAdded = end.row - start.row - @insertRows start.row + 1, rowsAdded - # make a copy since we're going to modify in place - oldHighlights = (@highlights.rows[start.row] || []).slice(0) - for highlight in oldHighlights - if highlight.column > start.column - # insertion was fully before this highlight - @moveHighlight highlight, - row: end.row - column: highlight.column + (end.column - start.column) - else if highlight.column + highlight.word.length >= start.column - # insertion was inside this highlight - @removeHighlight highlight - - else if change.action == "remove" - if start.row == end.row - oldHighlights = (@highlights.rows[start.row] || []).slice(0) - else - rowsRemoved = end.row - start.row - oldHighlights = - (@highlights.rows[start.row] || []).concat( - (@highlights.rows[end.row] || []) - ) - @removeRows start.row + 1, rowsRemoved - - for highlight in oldHighlights - if @_doesHighlightOverlapRange highlight, start, end - @removeHighlight highlight - else if @_isHighlightAfterRange highlight, start, end - @moveHighlight highlight, - row: start.row - column: highlight.column - (end.column - start.column) - - _doesHighlightOverlapRange: (highlight, start, end) -> - highlightIsAllBeforeRange = - highlight.row < start.row or - (highlight.row == start.row and highlight.column + highlight.word.length <= start.column) - highlightIsAllAfterRange = - highlight.row > end.row or - (highlight.row == end.row and highlight.column >= end.column) - !(highlightIsAllBeforeRange or highlightIsAllAfterRange) - - _isHighlightAfterRange: (highlight, start, end) -> - return true if highlight.row > end.row - return false if highlight.row < end.row - highlight.column >= end.column - - - - - - - From 8de226782495666fd2d7f779e2d1f552349a9084 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Mon, 30 Apr 2018 16:03:35 +0100 Subject: [PATCH 14/34] Adapt aceEditor to use new spell check manager with adapter --- .../ide/editor/directives/aceEditor.coffee | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index ef3c5b20f8..77a0b94b46 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -7,6 +7,7 @@ define [ "ide/editor/directives/aceEditor/undo/UndoManager" "ide/editor/directives/aceEditor/auto-complete/AutoCompleteManager" "ide/editor/directives/aceEditor/spell-check/SpellCheckManager" + "ide/editor/directives/aceEditor/spell-check/HighlightedWordManager" "ide/editor/directives/aceEditor/highlights/HighlightsManager" "ide/editor/directives/aceEditor/cursor-position/CursorPositionManager" "ide/editor/directives/aceEditor/track-changes/TrackChangesManager" @@ -15,7 +16,7 @@ define [ "ide/graphics/services/graphics" "ide/preamble/services/preamble" "ide/files/services/files" -], (App, Ace, SearchBox, Vim, ModeList, UndoManager, AutoCompleteManager, SpellCheckManager, HighlightsManager, CursorPositionManager, TrackChangesManager, MetadataManager) -> +], (App, Ace, SearchBox, Vim, ModeList, UndoManager, AutoCompleteManager, SpellCheckManager, HighlightedWordManager, HighlightsManager, CursorPositionManager, TrackChangesManager, MetadataManager) -> EditSession = ace.require('ace/edit_session').EditSession ModeList = ace.require('ace/ext/modelist') Vim = ace.require('ace/keyboard/vim').Vim @@ -103,7 +104,8 @@ define [ if scope.spellCheck # only enable spellcheck when explicitly required spellCheckCache = $cacheFactory.get("spellCheck-#{scope.name}") || $cacheFactory("spellCheck-#{scope.name}", {capacity: 1000}) - spellCheckManager = new SpellCheckManager(scope, editor, element, spellCheckCache, $http, $q) + spellCheckManager = new SpellCheckManager(scope, spellCheckCache, $http, $q, new SpellCheckAdapter(editor)) + undoManager = new UndoManager(scope, editor, element) highlightsManager = new HighlightsManager(scope, editor, element) cursorPositionManager = new CursorPositionManager(scope, editor, element, localStorage) @@ -361,6 +363,19 @@ define [ session.setScrollTop(session.getScrollTop() + 1) session.setScrollTop(session.getScrollTop() - 1) + onSessionChange = (e) -> + spellCheckManager.onSessionChange() + e.oldSession?.getDocument().off "change", spellCheckManager.onChange + e.session.getDocument().on "change", spellCheckManager.onChange + + attachToSpellCheck = () -> + spellCheckManager.init() + editor.on 'changeSession', onSessionChange + onSessionChange({ session: editor.getSession() }) # Force initial setup + + detachFromSpellCheck = () -> + editor.off 'changeSession', onSessionChange + attachToAce = (sharejs_doc) -> lines = sharejs_doc.getSnapshot().split("\n") session = editor.getSession() @@ -406,6 +421,7 @@ define [ editor.initing = false # now ready to edit document editor.setReadOnly(scope.readOnly) # respect the readOnly setting, normally false + attachToSpellCheck() resetScrollMargins() @@ -467,6 +483,7 @@ define [ scope.$on '$destroy', () -> if scope.sharejsDoc? + detachFromSpellCheck() detachFromAce(scope.sharejsDoc) session = editor.getSession() session?.destroy() @@ -585,3 +602,9 @@ define [ SearchBox::$init = () -> @element = $compile(searchHtml)($rootScope.$new())[0]; $init.apply(@) + + class SpellCheckAdapter + constructor: (@editor) -> + @wordManager = new HighlightedWordManager(@editor) + getLines: () -> @editor.getValue().split('\n') + normalizeChangeEvent: (e) -> e From abcc2cc11bd27bd689100740807affdfabef9a0a Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 24 Apr 2018 10:27:17 +0100 Subject: [PATCH 15/34] Style codemirror spelling errors --- services/web/public/stylesheets/app/editor/rich-text.less | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/services/web/public/stylesheets/app/editor/rich-text.less b/services/web/public/stylesheets/app/editor/rich-text.less index 493540e705..37a896269d 100644 --- a/services/web/public/stylesheets/app/editor/rich-text.less +++ b/services/web/public/stylesheets/app/editor/rich-text.less @@ -219,4 +219,11 @@ font-style: italic; color: #999; } + + .spelling-error { + background-image: url(/img/spellcheck-underline.png); + background-repeat: repeat-x; + background-position: bottom; + } } + From e6ffaaa489a68a902a4c77363f7d16ae7339133b Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 3 May 2018 17:28:36 +0100 Subject: [PATCH 16/34] Handle contextmenu for spelling --- .../ide/editor/directives/aceEditor.coffee | 42 +++++++++++++++---- .../spell-check/HighlightedWordManager.coffee | 17 ++++++++ .../spell-check/SpellCheckManager.coffee | 40 ++++++++++++++++++ 3 files changed, 91 insertions(+), 8 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index 77a0b94b46..ad34d4d493 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -20,6 +20,7 @@ define [ EditSession = ace.require('ace/edit_session').EditSession ModeList = ace.require('ace/ext/modelist') Vim = ace.require('ace/keyboard/vim').Vim + Range = ace.require('ace/range').Range # set the path for ace workers if using a CDN (from editor.pug) if window.aceWorkerPath != "" @@ -363,18 +364,22 @@ define [ session.setScrollTop(session.getScrollTop() + 1) session.setScrollTop(session.getScrollTop() - 1) - onSessionChange = (e) -> + onSessionChangeForSpellCheck = (e) -> spellCheckManager.onSessionChange() e.oldSession?.getDocument().off "change", spellCheckManager.onChange e.session.getDocument().on "change", spellCheckManager.onChange + e.oldSession?.off "changeScrollTop", spellCheckManager.onScroll + e.session.on "changeScrollTop", spellCheckManager.onScroll - attachToSpellCheck = () -> + initSpellCheck = () -> spellCheckManager.init() - editor.on 'changeSession', onSessionChange - onSessionChange({ session: editor.getSession() }) # Force initial setup + editor.on 'changeSession', onSessionChangeForSpellCheck + onSessionChangeForSpellCheck({ session: editor.getSession() }) # Force initial setup + editor.on 'nativecontextmenu', spellCheckManager.onContextMenu - detachFromSpellCheck = () -> - editor.off 'changeSession', onSessionChange + tearDownSpellCheck = () -> + editor.off 'changeSession', onSessionChangeForSpellCheck + editor.off 'nativecontextmenu', spellCheckManager.onContextMenu attachToAce = (sharejs_doc) -> lines = sharejs_doc.getSnapshot().split("\n") @@ -421,7 +426,7 @@ define [ editor.initing = false # now ready to edit document editor.setReadOnly(scope.readOnly) # respect the readOnly setting, normally false - attachToSpellCheck() + initSpellCheck() resetScrollMargins() @@ -483,7 +488,7 @@ define [ scope.$on '$destroy', () -> if scope.sharejsDoc? - detachFromSpellCheck() + tearDownSpellCheck() detachFromAce(scope.sharejsDoc) session = editor.getSession() session?.destroy() @@ -608,3 +613,24 @@ define [ @wordManager = new HighlightedWordManager(@editor) getLines: () -> @editor.getValue().split('\n') normalizeChangeEvent: (e) -> e + getCoordsFromContextMenuEvent: (e) -> + e.domEvent.stopPropagation() + return { + x: e.domEvent.clientX, + y: e.domEvent.clientY + } + preventContextMenuEventDefault: (e) -> + e.domEvent.preventDefault() + getHighlightFromCoords: (coords) -> + position = @editor.renderer.screenToTextCoordinates(coords.x, coords.y) + @wordManager.findHighlightWithinRange({ + start: position + end: position + }) + selectHighlightedWord: (highlight) -> + @editor.getSession().getSelection().setSelectionRange( + new Range( + highlight.row, highlight.column, + highlight.row, highlight.column + highlight.word.length + ) + ) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee index ba7561411d..5477bdcafa 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee @@ -51,3 +51,20 @@ define [ row = @highlights.rows[row] for highlight in (row || []).slice() @removeHighlight highlight + + findHighlightWithinRange: (range) -> + rows = @highlights.rows.slice(range.start.row, range.end.row + 1) + for row in rows + for highlight in (row || []) + if @_doesHighlightOverlapRange(highlight, range.start, range.end) + return highlight + return null + + _doesHighlightOverlapRange: (highlight, start, end) -> + highlightIsAllBeforeRange = + highlight.row < start.row or + (highlight.row == start.row and highlight.column + highlight.word.length <= start.column) + highlightIsAllAfterRange = + highlight.row > end.row or + (highlight.row == end.row and highlight.column >= end.column) + !(highlightIsAllBeforeRange or highlightIsAllAfterRange) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee index 4dee1faf52..c6b40c7b77 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee @@ -1,6 +1,12 @@ define [], () -> class SpellCheckManager constructor: (@$scope, @cache, @$http, @$q, @adapter) -> + @$scope.spellMenu = { + open: false + top: '0px' + left: '0px' + suggestions: [] + } @inProgressRequest = null @updatedLines = [] @@ -30,6 +36,40 @@ define [], () -> @runSpellCheckSoon(200) if @isSpellCheckEnabled() + onContextMenu: (e) => + @closeContextMenu() + @openContextMenu(e) + + onScroll: () => @closeContextMenu() + + openContextMenu: (e) -> + coords = @adapter.getCoordsFromContextMenuEvent(e) + highlight = @adapter.getHighlightFromCoords(coords) + if highlight + @adapter.preventContextMenuEventDefault(e) + @adapter.selectHighlightedWord(highlight) + @$scope.$apply () => + @$scope.spellMenu = { + open: true + top: coords.y + 'px' + left: coords.x + 'px' + suggestions: highlight.suggestions + } + @setUpClickOffContextMenuListener() + return false + + setUpClickOffContextMenuListener: () -> + $(document).one 'click', (e) => + @closeContextMenu() if e.which != 3 # Ignore if right click + return true + + closeContextMenu: () -> + # This is triggered on scroll, so for performance only apply setting when + # it changes + if @$scope?.spellMenu and @$scope.spellMenu.open != false + @$scope.$apply () => + @$scope.spellMenu.open = false + runFullCheck: () -> @adapter.wordManager.reset() @runSpellCheck() if @isSpellCheckEnabled() From cf123ce85727f057906baa9339ec01e4c9f17050 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 3 May 2018 17:29:05 +0100 Subject: [PATCH 17/34] Extract spellMenu component and use when showing spell suggestions --- .../coffee/ide/editor/EditorManager.coffee | 1 + .../ide/editor/components/spellMenu.coffee | 29 +++++++++++++++++++ .../ide/editor/directives/aceEditor.coffee | 22 ++++---------- 3 files changed, 36 insertions(+), 16 deletions(-) create mode 100644 services/web/public/coffee/ide/editor/components/spellMenu.coffee diff --git a/services/web/public/coffee/ide/editor/EditorManager.coffee b/services/web/public/coffee/ide/editor/EditorManager.coffee index 72bbe8509a..e3cabf8e98 100644 --- a/services/web/public/coffee/ide/editor/EditorManager.coffee +++ b/services/web/public/coffee/ide/editor/EditorManager.coffee @@ -1,5 +1,6 @@ define [ "ide/editor/Document" + "ide/editor/components/spellMenu" "ide/editor/directives/aceEditor" "ide/editor/directives/toggleSwitch" "ide/editor/controllers/SavingNotificationController" diff --git a/services/web/public/coffee/ide/editor/components/spellMenu.coffee b/services/web/public/coffee/ide/editor/components/spellMenu.coffee new file mode 100644 index 0000000000..c3fd78efba --- /dev/null +++ b/services/web/public/coffee/ide/editor/components/spellMenu.coffee @@ -0,0 +1,29 @@ +define ["base"], (App) -> + App.component "spellMenu", { + bindings: { + open: "<" + top: "<" + left: "<" + suggestions: "<" + replaceWord: "&" + learnWord: "&" + } + template: """ +
+ +
+ """ + } \ No newline at end of file diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index ad34d4d493..a30b93aaa0 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -510,22 +510,12 @@ define [ >Dismiss
-
- -
+
Date: Fri, 4 May 2018 15:30:06 +0100 Subject: [PATCH 18/34] Replace word with suggestion and learn word --- .../coffee/ide/editor/components/spellMenu.coffee | 13 +++++++++---- .../coffee/ide/editor/directives/aceEditor.coffee | 9 ++++++++- .../aceEditor/spell-check/SpellCheckManager.coffee | 11 ++++++++++- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/services/web/public/coffee/ide/editor/components/spellMenu.coffee b/services/web/public/coffee/ide/editor/components/spellMenu.coffee index c3fd78efba..ff3462e03e 100644 --- a/services/web/public/coffee/ide/editor/components/spellMenu.coffee +++ b/services/web/public/coffee/ide/editor/components/spellMenu.coffee @@ -4,7 +4,7 @@ define ["base"], (App) -> open: "<" top: "<" left: "<" - suggestions: "<" + highlight: "<" replaceWord: "&" learnWord: "&" } @@ -16,12 +16,17 @@ define ["base"], (App) -> ng-class="{open: $ctrl.open}" >
diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index a30b93aaa0..34ffedcac8 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -514,7 +514,9 @@ define [ open="spellMenu.open" top="spellMenu.top" left="spellMenu.left" - suggestions="spellMenu.suggestions" + highlight="spellMenu.highlight" + replace-word="replaceWord(highlight, suggestion)" + learn-word="learnWord(highlight)" >
+ @editor.getSession().replace(new Range( + highlight.row, highlight.column, + highlight.row, highlight.column + highlight.word.length + ), newWord) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee index c6b40c7b77..3d274abea4 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee @@ -14,6 +14,9 @@ define [], () -> if language != oldLanguage and oldLanguage? @runFullCheck() + @$scope.replaceWord = @adapter.replaceWord + @$scope.learnWord = @learnWord + init: () -> @updatedLines = Array(@adapter.getLines().length).fill(true) @runSpellCheckSoon(200) if @isSpellCheckEnabled() @@ -53,7 +56,7 @@ define [], () -> open: true top: coords.y + 'px' left: coords.x + 'px' - suggestions: highlight.suggestions + highlight: highlight } @setUpClickOffContextMenuListener() return false @@ -70,6 +73,12 @@ define [], () -> @$scope.$apply () => @$scope.spellMenu.open = false + learnWord: (highlight) => + @apiRequest "/learn", word: highlight.word + @adapter.wordManager.removeHighlight highlight + language = @$scope.spellCheckLanguage + @cache?.put("#{language}:#{highlight.word}", true) + runFullCheck: () -> @adapter.wordManager.reset() @runSpellCheck() if @isSpellCheckEnabled() From 9c56f6c2fccb56c59810e00c548f644185bef01f Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Wed, 9 May 2018 16:35:59 +0100 Subject: [PATCH 19/34] Add init test for SpellCheckManager --- .../spell-check/SpellCheckManagerTests.coffee | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 services/web/test/unit_frontend/coffee/ide/editor/aceEditor/spell-check/SpellCheckManagerTests.coffee diff --git a/services/web/test/unit_frontend/coffee/ide/editor/aceEditor/spell-check/SpellCheckManagerTests.coffee b/services/web/test/unit_frontend/coffee/ide/editor/aceEditor/spell-check/SpellCheckManagerTests.coffee new file mode 100644 index 0000000000..68e47511d3 --- /dev/null +++ b/services/web/test/unit_frontend/coffee/ide/editor/aceEditor/spell-check/SpellCheckManagerTests.coffee @@ -0,0 +1,46 @@ +define [ + 'ide/editor/directives/aceEditor/spell-check/SpellCheckManager' +], (SpellCheckManager) -> + describe 'SpellCheckManager', -> + beforeEach (done) -> + @timelord = sinon.useFakeTimers() + + window.user = { id: 1 } + window.csrfToken = 'token' + @scope = { + $watch: sinon.stub() + spellCheck: true + spellCheckLanguage: 'en' + } + @wordManager = { + reset: sinon.stub() + clearRow: sinon.stub() + addHighlight: sinon.stub() + } + @adapter = { + getLines: sinon.stub() + wordManager: @wordManager + } + inject ($q, $http, $httpBackend, $cacheFactory) => + @$http = $http + @$q = $q + @$httpBackend = $httpBackend + cache = $cacheFactory('spellCheckTest', {capacity: 1000}) + @spellCheckManager = new SpellCheckManager(@scope, cache, $http, $q, @adapter) + done() + + afterEach -> + @timelord.restore() + + it 'runs a full check soon after init', () -> + @$httpBackend.when('POST', '/spelling/check').respond({ + misspellings: [{ + index: 0 + suggestions: ['opposition'] + }] + }) + @adapter.getLines.returns(['oppozition']) + @spellCheckManager.init() + @timelord.tick(200) + @$httpBackend.flush() + expect(@wordManager.addHighlight).to.have.been.called From d2bba0eb60c2361e337ebedfcf55492458ed10fd Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 11 May 2018 17:53:22 +0100 Subject: [PATCH 20/34] Fix firefox not closing contextmenu correctly --- .../aceEditor/spell-check/SpellCheckManager.coffee | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee index 3d274abea4..6cc95d58c4 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee @@ -17,6 +17,10 @@ define [], () -> @$scope.replaceWord = @adapter.replaceWord @$scope.learnWord = @learnWord + $(document).on 'click', (e) => + @closeContextMenu() if e.which != 3 # Ignore if right click + return true + init: () -> @updatedLines = Array(@adapter.getLines().length).fill(true) @runSpellCheckSoon(200) if @isSpellCheckEnabled() @@ -58,14 +62,8 @@ define [], () -> left: coords.x + 'px' highlight: highlight } - @setUpClickOffContextMenuListener() return false - setUpClickOffContextMenuListener: () -> - $(document).one 'click', (e) => - @closeContextMenu() if e.which != 3 # Ignore if right click - return true - closeContextMenu: () -> # This is triggered on scroll, so for performance only apply setting when # it changes From ebf1b7c84c6b3eb42e92cedf7a148c8c44fdd57c Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 11 May 2018 12:22:45 +0100 Subject: [PATCH 21/34] Extract SpellCheckAdapter to separate file --- .../ide/editor/directives/aceEditor.coffee | 37 +-------------- .../spell-check/SpellCheckAdapter.coffee | 45 +++++++++++++++++++ 2 files changed, 47 insertions(+), 35 deletions(-) create mode 100644 services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index 34ffedcac8..617b41b845 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -7,7 +7,7 @@ define [ "ide/editor/directives/aceEditor/undo/UndoManager" "ide/editor/directives/aceEditor/auto-complete/AutoCompleteManager" "ide/editor/directives/aceEditor/spell-check/SpellCheckManager" - "ide/editor/directives/aceEditor/spell-check/HighlightedWordManager" + "ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter" "ide/editor/directives/aceEditor/highlights/HighlightsManager" "ide/editor/directives/aceEditor/cursor-position/CursorPositionManager" "ide/editor/directives/aceEditor/track-changes/TrackChangesManager" @@ -16,11 +16,10 @@ define [ "ide/graphics/services/graphics" "ide/preamble/services/preamble" "ide/files/services/files" -], (App, Ace, SearchBox, Vim, ModeList, UndoManager, AutoCompleteManager, SpellCheckManager, HighlightedWordManager, HighlightsManager, CursorPositionManager, TrackChangesManager, MetadataManager) -> +], (App, Ace, SearchBox, Vim, ModeList, UndoManager, AutoCompleteManager, SpellCheckManager, SpellCheckAdapter, HighlightsManager, CursorPositionManager, TrackChangesManager, MetadataManager) -> EditSession = ace.require('ace/edit_session').EditSession ModeList = ace.require('ace/ext/modelist') Vim = ace.require('ace/keyboard/vim').Vim - Range = ace.require('ace/range').Range # set the path for ace workers if using a CDN (from editor.pug) if window.aceWorkerPath != "" @@ -599,35 +598,3 @@ define [ SearchBox::$init = () -> @element = $compile(searchHtml)($rootScope.$new())[0]; $init.apply(@) - - class SpellCheckAdapter - constructor: (@editor) -> - @wordManager = new HighlightedWordManager(@editor) - getLines: () -> @editor.getValue().split('\n') - normalizeChangeEvent: (e) -> e - getCoordsFromContextMenuEvent: (e) -> - e.domEvent.stopPropagation() - return { - x: e.domEvent.clientX, - y: e.domEvent.clientY - } - preventContextMenuEventDefault: (e) -> - e.domEvent.preventDefault() - getHighlightFromCoords: (coords) -> - position = @editor.renderer.screenToTextCoordinates(coords.x, coords.y) - @wordManager.findHighlightWithinRange({ - start: position - end: position - }) - selectHighlightedWord: (highlight) -> - @editor.getSession().getSelection().setSelectionRange( - new Range( - highlight.row, highlight.column, - highlight.row, highlight.column + highlight.word.length - ) - ) - replaceWord: (highlight, newWord) => - @editor.getSession().replace(new Range( - highlight.row, highlight.column, - highlight.row, highlight.column + highlight.word.length - ), newWord) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee new file mode 100644 index 0000000000..feed646c95 --- /dev/null +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee @@ -0,0 +1,45 @@ +define [ + "ace/ace" + "ide/editor/directives/aceEditor/spell-check/HighlightedWordManager" +], (Ace, HighlightedWordManager) -> + Range = ace.require('ace/range').Range + + class SpellCheckAdapter + constructor: (@editor) -> + @wordManager = new HighlightedWordManager(@editor) + + getLines: () -> + @editor.getValue().split('\n') + + normalizeChangeEvent: (e) -> e + + getCoordsFromContextMenuEvent: (e) -> + e.domEvent.stopPropagation() + return { + x: e.domEvent.clientX, + y: e.domEvent.clientY + } + + preventContextMenuEventDefault: (e) -> + e.domEvent.preventDefault() + + getHighlightFromCoords: (coords) -> + position = @editor.renderer.screenToTextCoordinates(coords.x, coords.y) + @wordManager.findHighlightWithinRange({ + start: position + end: position + }) + + selectHighlightedWord: (highlight) -> + @editor.getSession().getSelection().setSelectionRange( + new Range( + highlight.row, highlight.column, + highlight.row, highlight.column + highlight.word.length + ) + ) + + replaceWord: (highlight, newWord) => + @editor.getSession().replace(new Range( + highlight.row, highlight.column, + highlight.row, highlight.column + highlight.word.length + ), newWord) From 9fa85400b35b061d2a48373f901d32c26a26b4dd Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Mon, 14 May 2018 16:28:16 +0100 Subject: [PATCH 22/34] HighlightedWordManager is more naive about tracking position We are relying entirely on Ace's tracking of markers with the anchor trick. This means that we don't have to apply changes to ensure that the word manager data structure tracks which row the highlights are on. This is traded off against slightly less efficient searching/removing --- .../spell-check/HighlightedWordManager.coffee | 59 +++++++++---------- .../spell-check/SpellCheckAdapter.coffee | 16 +++-- 2 files changed, 39 insertions(+), 36 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee index 5477bdcafa..627fbb0f88 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee @@ -4,9 +4,7 @@ define [ Range = ace.require("ace/range").Range class Highlight - constructor: (options) -> - @row = options.row - @column = options.column + constructor: (@markerId, @range, options) -> @word = options.word @suggestions = options.suggestions @@ -15,56 +13,53 @@ define [ @reset() reset: () -> - @highlights?.rows.forEach (highlight) => + @highlights?.forEach (highlight) => @editor.getSession().removeMarker(highlight.markerId) - @highlights = rows: [] - - addHighlight: (highlight) -> - unless highlight instanceof Highlight - highlight = new Highlight(highlight) + @highlights = [] + addHighlight: (options) -> session = @editor.getSession() doc = session.getDocument() # Set up Range that will automatically update it's positions when the # document changes range = new Range() range.start = doc.createAnchor({ - row: highlight.row, - column: highlight.column + row: options.row, + column: options.column }) range.end = doc.createAnchor({ - row: highlight.row, - column: highlight.column + highlight.word.length + row: options.row, + column: options.column + options.word.length }) - highlight.markerId = session.addMarker range, "spelling-highlight", 'text', false - @highlights.rows[highlight.row] ||= [] - @highlights.rows[highlight.row].push highlight + markerId = session.addMarker range, "spelling-highlight", 'text', false + + @highlights.push new Highlight(markerId, range, options) removeHighlight: (highlight) -> @editor.getSession().removeMarker(highlight.markerId) - for h, i in @highlights.rows[highlight.row] - if h == highlight - @highlights.rows[highlight.row].splice(i, 1) + @highlights = @highlights.filter (hl) -> + hl != highlight clearRow: (row) -> - row = @highlights.rows[row] - for highlight in (row || []).slice() - @removeHighlight highlight + @highlights.filter (highlight) -> + highlight.range.start.row == row + .forEach (highlight) => + @removeHighlight(highlight) findHighlightWithinRange: (range) -> - rows = @highlights.rows.slice(range.start.row, range.end.row + 1) - for row in rows - for highlight in (row || []) - if @_doesHighlightOverlapRange(highlight, range.start, range.end) - return highlight - return null + @highlights.find (highlight) => + @_doesHighlightOverlapRange highlight, range.start, range.end _doesHighlightOverlapRange: (highlight, start, end) -> + highlightRow = highlight.range.start.row + highlightStartColumn = highlight.range.start.column + highlightEndColumn = highlight.range.end.column + highlightIsAllBeforeRange = - highlight.row < start.row or - (highlight.row == start.row and highlight.column + highlight.word.length <= start.column) + highlightRow < start.row or + (highlightRow == start.row and highlightEndColumn <= start.column) highlightIsAllAfterRange = - highlight.row > end.row or - (highlight.row == end.row and highlight.column >= end.column) + highlightRow > end.row or + (highlightRow == end.row and highlightStartColumn >= end.column) !(highlightIsAllBeforeRange or highlightIsAllAfterRange) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee index feed646c95..51443d9848 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee @@ -31,15 +31,23 @@ define [ }) selectHighlightedWord: (highlight) -> + row = highlight.range.start.row + startColumn = highlight.range.start.column + endColumn = highlight.range.end.column + @editor.getSession().getSelection().setSelectionRange( new Range( - highlight.row, highlight.column, - highlight.row, highlight.column + highlight.word.length + row, startColumn, + row, endColumn ) ) replaceWord: (highlight, newWord) => + row = highlight.range.start.row + startColumn = highlight.range.start.column + endColumn = highlight.range.end.column + @editor.getSession().replace(new Range( - highlight.row, highlight.column, - highlight.row, highlight.column + highlight.word.length + row, startColumn, + row, endColumn ), newWord) From 2be023c731343f192c58f85c873756dbc7b7fece Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Mon, 14 May 2018 16:31:45 +0100 Subject: [PATCH 23/34] Prevent spell error marker adding newly typed characters --- .../aceEditor/spell-check/HighlightedWordManager.coffee | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee index 627fbb0f88..46b7a218ab 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee @@ -31,6 +31,10 @@ define [ row: options.row, column: options.column + options.word.length }) + # Prevent range from adding newly typed characters to the end of the word. + # This makes it appear as if the spelling error continues to the next word + # even after a space + range.end.$insertRight = true markerId = session.addMarker range, "spelling-highlight", 'text', false From 846f27f0adfcfa3d05360af3a892646a9b97efb2 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 15 May 2018 15:08:36 +0100 Subject: [PATCH 24/34] Clear highlights that are "touching" the cursor on change This means that correcting a mistake won't wait until the request has resolved and that only the word at the end of the line will have it's spelling highlight removed instead of the entire row --- .../spell-check/HighlightedWordManager.coffee | 21 +++++++++++++++++++ .../spell-check/SpellCheckManager.coffee | 3 +++ 2 files changed, 24 insertions(+) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee index 46b7a218ab..8bb350914f 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee @@ -67,3 +67,24 @@ define [ highlightRow > end.row or (highlightRow == end.row and highlightStartColumn >= end.column) !(highlightIsAllBeforeRange or highlightIsAllAfterRange) + + clearHighlightTouchingRange: (range) -> + highlight = @highlights.find (hl) => + @_doesHighlightTouchRange hl, range.start, range.end + if highlight + @removeHighlight highlight + + _doesHighlightTouchRange: (highlight, start, end) -> + highlightRow = highlight.range.start.row + highlightStartColumn = highlight.range.start.column + highlightEndColumn = highlight.range.end.column + + rangeStartIsWithinHighlight = + highlightStartColumn <= start.column and + highlightEndColumn >= start.column + rangeEndIsWithinHighlight = + highlightStartColumn <= end.column and + highlightEndColumn >= end.column + + highlightRow == start.row and + (rangeStartIsWithinHighlight or rangeEndIsWithinHighlight) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee index 6cc95d58c4..2828f4b426 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee @@ -35,6 +35,9 @@ define [], () -> onChange: (e) => if @isSpellCheckEnabled() @markLinesAsUpdated(@adapter.normalizeChangeEvent(e)) + + @adapter.wordManager.clearHighlightTouchingRange(e) + @runSpellCheckSoon() onSessionChange: () => From 681e67ecea10a569946c66c7e8ba8157afcd532d Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 17 May 2018 15:59:13 +0100 Subject: [PATCH 25/34] Be more consistent with naming --- .../aceEditor/spell-check/SpellCheckAdapter.coffee | 4 ++-- .../aceEditor/spell-check/SpellCheckManager.coffee | 14 +++++++------- .../spell-check/SpellCheckManagerTests.coffee | 6 +++--- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee index 51443d9848..9b6d809a37 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee @@ -6,7 +6,7 @@ define [ class SpellCheckAdapter constructor: (@editor) -> - @wordManager = new HighlightedWordManager(@editor) + @highlightedWordManager = new HighlightedWordManager(@editor) getLines: () -> @editor.getValue().split('\n') @@ -25,7 +25,7 @@ define [ getHighlightFromCoords: (coords) -> position = @editor.renderer.screenToTextCoordinates(coords.x, coords.y) - @wordManager.findHighlightWithinRange({ + @highlightedWordManager.findHighlightWithinRange({ start: position end: position }) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee index 2828f4b426..89e3d1509d 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee @@ -36,12 +36,12 @@ define [], () -> if @isSpellCheckEnabled() @markLinesAsUpdated(@adapter.normalizeChangeEvent(e)) - @adapter.wordManager.clearHighlightTouchingRange(e) + @adapter.highlightedWordManager.clearHighlightTouchingRange(e) @runSpellCheckSoon() onSessionChange: () => - @adapter.wordManager.reset() + @adapter.highlightedWordManager.reset() @inProgressRequest.abort() if @inProgressRequest? @runSpellCheckSoon(200) if @isSpellCheckEnabled() @@ -76,12 +76,12 @@ define [], () -> learnWord: (highlight) => @apiRequest "/learn", word: highlight.word - @adapter.wordManager.removeHighlight highlight + @adapter.highlightedWordManager.removeHighlight highlight language = @$scope.spellCheckLanguage @cache?.put("#{language}:#{highlight.word}", true) runFullCheck: () -> - @adapter.wordManager.reset() + @adapter.highlightedWordManager.reset() @runSpellCheck() if @isSpellCheckEnabled() markLinesAsUpdated: (change) -> @@ -146,11 +146,11 @@ define [], () -> displayResult = (highlights) => if linesToProcess? for shouldProcess, row in linesToProcess - @adapter.wordManager.clearRow(row) if shouldProcess + @adapter.highlightedWordManager.clearRow(row) if shouldProcess else - @adapter.wordManager.reset() + @adapter.highlightedWordManager.reset() for highlight in highlights - @adapter.wordManager.addHighlight highlight + @adapter.highlightedWordManager.addHighlight highlight if not words.length displayResult highlights diff --git a/services/web/test/unit_frontend/coffee/ide/editor/aceEditor/spell-check/SpellCheckManagerTests.coffee b/services/web/test/unit_frontend/coffee/ide/editor/aceEditor/spell-check/SpellCheckManagerTests.coffee index 68e47511d3..1b0dddced6 100644 --- a/services/web/test/unit_frontend/coffee/ide/editor/aceEditor/spell-check/SpellCheckManagerTests.coffee +++ b/services/web/test/unit_frontend/coffee/ide/editor/aceEditor/spell-check/SpellCheckManagerTests.coffee @@ -12,14 +12,14 @@ define [ spellCheck: true spellCheckLanguage: 'en' } - @wordManager = { + @highlightedWordManager = { reset: sinon.stub() clearRow: sinon.stub() addHighlight: sinon.stub() } @adapter = { getLines: sinon.stub() - wordManager: @wordManager + highlightedWordManager: @highlightedWordManager } inject ($q, $http, $httpBackend, $cacheFactory) => @$http = $http @@ -43,4 +43,4 @@ define [ @spellCheckManager.init() @timelord.tick(200) @$httpBackend.flush() - expect(@wordManager.addHighlight).to.have.been.called + expect(@highlightedWordManager.addHighlight).to.have.been.called From a719ac6e6e61e114cd2a1197aa3a2afd8d4f0135 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 18 May 2018 13:33:06 +0100 Subject: [PATCH 26/34] IE11 doesn't support Array.find so use underscore instead --- .../aceEditor/spell-check/HighlightedWordManager.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee index 8bb350914f..c36edb309c 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee @@ -52,7 +52,7 @@ define [ @removeHighlight(highlight) findHighlightWithinRange: (range) -> - @highlights.find (highlight) => + _.find @highlights, (highlight) => @_doesHighlightOverlapRange highlight, range.start, range.end _doesHighlightOverlapRange: (highlight, start, end) -> @@ -69,7 +69,7 @@ define [ !(highlightIsAllBeforeRange or highlightIsAllAfterRange) clearHighlightTouchingRange: (range) -> - highlight = @highlights.find (hl) => + highlight = _.find @highlights, (hl) => @_doesHighlightTouchRange hl, range.start, range.end if highlight @removeHighlight highlight From c2d7809e055053b583ee958c9b570705dd7688d0 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 18 May 2018 14:23:36 +0100 Subject: [PATCH 27/34] Add removeWord so that learning word removes all highlights for given word --- .../aceEditor/spell-check/HighlightedWordManager.coffee | 6 ++++++ .../aceEditor/spell-check/SpellCheckManager.coffee | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee index c36edb309c..daeb4cc034 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/HighlightedWordManager.coffee @@ -45,6 +45,12 @@ define [ @highlights = @highlights.filter (hl) -> hl != highlight + removeWord: (word) -> + @highlights.filter (highlight) -> + highlight.word == word + .forEach (highlight) => + @removeHighlight(highlight) + clearRow: (row) -> @highlights.filter (highlight) -> highlight.range.start.row == row diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee index 89e3d1509d..cbe4fdbd64 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckManager.coffee @@ -76,7 +76,7 @@ define [], () -> learnWord: (highlight) => @apiRequest "/learn", word: highlight.word - @adapter.highlightedWordManager.removeHighlight highlight + @adapter.highlightedWordManager.removeWord highlight.word language = @$scope.spellCheckLanguage @cache?.put("#{language}:#{highlight.word}", true) From 309792401f8c6c1239fcb56c1981624367134b86 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Mon, 21 May 2018 10:35:43 +0100 Subject: [PATCH 28/34] Re-focus editor after clicking suggestion --- .../directives/aceEditor/spell-check/SpellCheckAdapter.coffee | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee index 9b6d809a37..2afc2cf0ff 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/spell-check/SpellCheckAdapter.coffee @@ -51,3 +51,6 @@ define [ row, startColumn, row, endColumn ), newWord) + + # Bring editor back into focus after clicking on suggestion + @editor.focus() From b1f378208de893e2f4ff155b740e08354d9ed5af Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 16 May 2018 16:31:28 +0100 Subject: [PATCH 29/34] Properly merge subscriptions from different places --- services/web/Makefile | 2 +- .../Features/Referal/ReferalAllocator.coffee | 6 +- .../SubscriptionController.coffee | 7 + .../Subscription/SubscriptionLocator.coffee | 4 +- .../Subscription/SubscriptionRouter.coffee | 2 + .../Subscription/SubscriptionUpdater.coffee | 87 ++++++---- .../Subscription/UserFeaturesUpdater.coffee | 9 +- .../Subscription/V1SubscriptionManager.coffee | 48 ++++++ services/web/docker-compose.yml | 1 + .../coffee/SubscriptionTests.coffee | 151 ++++++++++++++++++ .../coffee/helpers/MockV1Api.coffee | 13 +- .../acceptance/config/settings.test.coffee | 101 ++++++++++++ 12 files changed, 389 insertions(+), 42 deletions(-) create mode 100644 services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee create mode 100644 services/web/test/acceptance/coffee/SubscriptionTests.coffee create mode 100644 services/web/test/acceptance/config/settings.test.coffee diff --git a/services/web/Makefile b/services/web/Makefile index a0f8c1c503..720df034e8 100644 --- a/services/web/Makefile +++ b/services/web/Makefile @@ -9,7 +9,7 @@ COFFEE := node_modules/.bin/coffee $(COFFEE_OPTIONS) GRUNT := node_modules/.bin/grunt APP_COFFEE_FILES := $(shell find app/coffee -name '*.coffee') FRONT_END_COFFEE_FILES := $(shell find public/coffee -name '*.coffee') -TEST_COFFEE_FILES := $(shell find test -name '*.coffee') +TEST_COFFEE_FILES := $(shell find test/*/coffee -name '*.coffee') MODULE_MAIN_COFFEE_FILES := $(shell find modules -type f -wholename '*main/index.coffee') MODULE_IDE_COFFEE_FILES := $(shell find modules -type f -wholename '*ide/index.coffee') COFFEE_FILES := app.coffee $(APP_COFFEE_FILES) $(FRONT_END_COFFEE_FILES) $(TEST_COFFEE_FILES) diff --git a/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee b/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee index b28979aaf0..aef283d68d 100644 --- a/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee +++ b/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee @@ -31,7 +31,7 @@ module.exports = ReferalAllocator = - assignBonus: (user_id, callback = (error) ->) -> + getBonusFeatures: (user_id, callback = (error) ->) -> query = _id: user_id User.findOne query, (error, user) -> return callback(error) if error @@ -39,9 +39,7 @@ module.exports = ReferalAllocator = logger.log user_id: user_id, refered_user_count: user.refered_user_count, "assigning bonus" if user.refered_user_count? and user.refered_user_count > 0 newFeatures = ReferalAllocator._calculateFeatures(user) - if _.isEqual newFeatures, user.features - return callback() - User.update query, { $set: features: newFeatures }, callback + callback null, newFeatures else callback() diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index 9da6fc688d..1b7078d9cb 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -9,6 +9,7 @@ logger = require('logger-sharelatex') GeoIpLookup = require("../../infrastructure/GeoIpLookup") SubscriptionDomainHandler = require("./SubscriptionDomainHandler") UserGetter = require "../User/UserGetter" +SubscriptionUpdater = require './SubscriptionUpdater' module.exports = SubscriptionController = @@ -237,3 +238,9 @@ module.exports = SubscriptionController = return next(error) if error? req.body = body next() + + refreshUserSubscription: (req, res, next) -> + {user_id} = req.params + SubscriptionUpdater.refreshSubscription user_id, (error) -> + return next(error) if error? + res.sendStatus 200 \ No newline at end of file diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee index 1452f26d99..33376f504b 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionLocator.coffee @@ -28,8 +28,8 @@ module.exports = getSubscriptionByMemberIdAndId: (user_id, subscription_id, callback)-> Subscription.findOne {member_ids: user_id, _id:subscription_id}, {_id:1}, callback - getGroupSubscriptionMemberOf: (user_id, callback)-> - Subscription.findOne {member_ids: user_id}, {_id:1, planCode:1}, callback + getGroupSubscriptionsMemberOf: (user_id, callback)-> + Subscription.find {member_ids: user_id}, {_id:1, planCode:1}, callback getGroupsWithEmailInvite: (email, callback) -> Subscription.find { invited_emails: email }, callback \ No newline at end of file diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee index d6d049f5bf..692f6c92c8 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee @@ -46,4 +46,6 @@ module.exports = webRouter.get "/user/subscription/upgrade-annual", AuthenticationController.requireLogin(), SubscriptionController.renderUpgradeToAnnualPlanPage webRouter.post "/user/subscription/upgrade-annual", AuthenticationController.requireLogin(), SubscriptionController.processUpgradeToAnnualPlan + # Currently used in acceptance tests only, as a way to trigger the syncing logic + publicApiRouter.post "/user/:user_id/subscription/sync", AuthenticationController.httpAuth, SubscriptionController.refreshUserSubscription diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee index 649551b5b2..c76bffd453 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee @@ -8,6 +8,7 @@ Settings = require("settings-sharelatex") logger = require("logger-sharelatex") ObjectId = require('mongoose').Types.ObjectId ReferalAllocator = require("../Referal/ReferalAllocator") +V1SubscriptionManager = require("./V1SubscriptionManager") oneMonthInSeconds = 60 * 60 * 24 * 30 @@ -105,38 +106,66 @@ module.exports = SubscriptionUpdater = _setUsersMinimumFeatures: (user_id, callback)-> jobs = - subscription: (cb)-> - SubscriptionLocator.getUsersSubscription user_id, cb - groupSubscription: (cb)-> - SubscriptionLocator.getGroupSubscriptionMemberOf user_id, cb - v1PlanCode: (cb) -> - Modules = require '../../infrastructure/Modules' - Modules.hooks.fire 'getV1PlanCode', user_id, (err, results) -> - cb(err, results?[0] || null) + individualFeatures: (cb)-> + SubscriptionLocator.getUsersSubscription user_id, (err, sub)-> + cb err, SubscriptionUpdater._subscriptionToFeatures(sub) + groupFeatures: (cb) -> + SubscriptionLocator.getGroupSubscriptionsMemberOf user_id, (err, subs) -> + cb err, (subs or []).map SubscriptionUpdater._subscriptionToFeatures + v1Features: (cb) -> + V1SubscriptionManager.getPlanCodeFromV1 user_id, (err, planCode) -> + cb err, SubscriptionUpdater._planCodeToFeatures(planCode) + bonusFeatures: (cb) -> + ReferalAllocator.getBonusFeatures user_id, cb async.series jobs, (err, results)-> if err? logger.err err:err, user_id:user_id, "error getting subscription or group for _setUsersMinimumFeatures" return callback(err) - {subscription, groupSubscription, v1PlanCode} = results - # Group Subscription - if groupSubscription? and groupSubscription.planCode? - logger.log user_id:user_id, "using group which user is memor of for features" - UserFeaturesUpdater.updateFeatures user_id, groupSubscription.planCode, callback - # Personal Subscription - else if subscription? and subscription.planCode? and subscription.planCode != Settings.defaultPlanCode - logger.log user_id:user_id, "using users subscription plan code for features" - UserFeaturesUpdater.updateFeatures user_id, subscription.planCode, callback - # V1 Subscription - else if v1PlanCode? - logger.log user_id: user_id, "using the V1 plan for features" - UserFeaturesUpdater.updateFeatures user_id, v1PlanCode, callback - # Default - else - logger.log user_id:user_id, "using default features for user with no subscription or group" - UserFeaturesUpdater.updateFeatures user_id, Settings.defaultPlanCode, (err)-> - if err? - logger.err err:err, user_id:user_id, "Error setting minimum user feature" - return callback(err) - ReferalAllocator.assignBonus user_id, callback + {individualFeatures, groupFeatures, v1Features, bonusFeatures} = results + logger.log {user_id, individualFeatures, groupFeatures, v1Features, bonusFeatures}, 'merging user features' + featureSets = groupFeatures.concat [individualFeatures, v1Features, bonusFeatures] + features = _.reduce(featureSets, SubscriptionUpdater._mergeFeatures, Settings.defaultFeatures) + + logger.log {user_id, features}, 'updating user features' + UserFeaturesUpdater.updateFeatures user_id, features, callback + + _mergeFeatures: (featuresA, featuresB) -> + features = Object.assign({}, featuresA) + for key, value of featuresB + # Special merging logic for non-boolean features + if key == 'compileGroup' + if features['compileGroup'] == 'priority' or featuresB['compileGroup'] == 'priority' + features['compileGroup'] = 'priority' + else + features['compileGroup'] = 'standard' + else if key == 'collaborators' + if features['collaborators'] == -1 or featuresB['collaborators'] == -1 + features['collaborators'] = -1 + else + features['collaborators'] = Math.max( + features['collaborators'] or 0, + featuresB['collaborators'] or 0 + ) + else if key == 'compileTimeout' + features['compileTimeout'] = Math.max( + features['compileTimeout'] or 0, + featuresB['compileTimeout'] or 0 + ) + else + # Boolean keys, true is better + features[key] = features[key] or featuresB[key] + return features + + _subscriptionToFeatures: (subscription) -> + SubscriptionUpdater._planCodeToFeatures(subscription?.planCode) + + _planCodeToFeatures: (planCode) -> + if !planCode? + return {} + plan = PlansLocator.findLocalPlanInSettings planCode + if !plan? + return {} + else + return plan.features \ No newline at end of file diff --git a/services/web/app/coffee/Features/Subscription/UserFeaturesUpdater.coffee b/services/web/app/coffee/Features/Subscription/UserFeaturesUpdater.coffee index c0b691e677..520139e9be 100644 --- a/services/web/app/coffee/Features/Subscription/UserFeaturesUpdater.coffee +++ b/services/web/app/coffee/Features/Subscription/UserFeaturesUpdater.coffee @@ -4,12 +4,11 @@ PlansLocator = require("./PlansLocator") module.exports = - updateFeatures: (user_id, plan_code, callback = (err, features)->)-> + updateFeatures: (user_id, features, callback = (err, features)->)-> conditions = _id:user_id update = {} - plan = PlansLocator.findLocalPlanInSettings(plan_code) - logger.log user_id:user_id, features:plan.features, plan_code:plan_code, "updating users features" - update["features.#{key}"] = value for key, value of plan.features + logger.log user_id:user_id, features:features, "updating users features" + update["features.#{key}"] = value for key, value of features User.update conditions, update, (err)-> - callback err, plan.features + callback err, features diff --git a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee new file mode 100644 index 0000000000..37460cd773 --- /dev/null +++ b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee @@ -0,0 +1,48 @@ +UserGetter = require "../User/UserGetter" +request = require "request" +settings = require "settings-sharelatex" +logger = require "logger-sharelatex" + +module.exports = V1SubscriptionManager = + # Returned planCode = 'v1_pro' | 'v1_pro_plus' | 'v1_student' | 'v1_free' | null + # For this to work, we need plans in settings with plan-codes: + # - 'v1_pro' + # - 'v1_pro_plus' + # - 'v1_student' + # - 'v1_free' + getPlanCodeFromV1: (userId, callback=(err, planCode)->) -> + logger.log {userId}, "[V1SubscriptionManager] fetching v1 plan for user" + 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}" + else + # Throw away 'anonymous', etc as being equivalent to null + planName = null + return callback(null, planName) + + _v1PlanRequest: (v1Id, callback=(err, body)->) -> + 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/docker-compose.yml b/services/web/docker-compose.yml index 5a668bc4a3..a062c0df4e 100644 --- a/services/web/docker-compose.yml +++ b/services/web/docker-compose.yml @@ -17,6 +17,7 @@ services: PROJECT_HISTORY_ENABLED: 'true' ENABLED_LINKED_FILE_TYPES: 'url' LINKED_URL_PROXY: 'http://localhost:6543' + SHARELATEX_CONFIG: /app/test/acceptance/config/settings.test.coffee depends_on: - redis - mongo diff --git a/services/web/test/acceptance/coffee/SubscriptionTests.coffee b/services/web/test/acceptance/coffee/SubscriptionTests.coffee new file mode 100644 index 0000000000..55f3a0efbc --- /dev/null +++ b/services/web/test/acceptance/coffee/SubscriptionTests.coffee @@ -0,0 +1,151 @@ +expect = require("chai").expect +async = require("async") +UserClient = require "./helpers/User" +request = require "./helpers/request" +settings = require "settings-sharelatex" +{ObjectId} = require("../../../app/js/infrastructure/mongojs") +Subscription = require("../../../app/js/models/Subscription").Subscription +User = require("../../../app/js/models/User").User + +MockV1Api = require "./helpers/MockV1Api" + +syncUserAndGetFeatures = (user, callback = (error, features) ->) -> + request { + method: 'POST', + url: "/user/#{user._id}/subscription/sync", + auth: + user: 'sharelatex' + pass: 'password' + sendImmediately: true + }, (error, response, body) -> + throw error if error? + expect(response.statusCode).to.equal 200 + 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", -> + beforeEach (done) -> + @user = new UserClient() + @user.ensureUserExists (error) -> + throw error if error? + done() + + describe "when user has no subscriptions", -> + it "should set their features to the basic set", (done) -> + syncUserAndGetFeatures @user, (error, features) => + throw error if error? + expect(features).to.deep.equal(settings.defaultFeatures) + done() + + describe "when the user has an individual subscription", -> + beforeEach -> + Subscription.create { + admin_id: @user._id + planCode: 'collaborator' + customAccount: true + } # returns a promise + + it "should set their features to the upgraded set", (done) -> + syncUserAndGetFeatures @user, (error, features) => + throw error if error? + plan = settings.plans.find (plan) -> plan.planCode == 'collaborator' + expect(features).to.deep.equal(plan.features) + done() + + describe "when the user is in a group subscription", -> + beforeEach -> + Subscription.create { + admin_id: ObjectId() + member_ids: [@user._id] + groupAccount: true + planCode: 'collaborator' + customAccount: true + } # returns a promise + + it "should set their features to the upgraded set", (done) -> + syncUserAndGetFeatures @user, (error, features) => + throw error if error? + plan = settings.plans.find (plan) -> plan.planCode == 'collaborator' + expect(features).to.deep.equal(plan.features) + done() + + describe "when the user has bonus features", -> + beforeEach -> + User.update { + _id: @user._id + }, { + refered_user_count: 10 + } # returns a promise + + it "should set their features to the bonus set", (done) -> + syncUserAndGetFeatures @user, (error, features) => + throw error if error? + expect(features).to.deep.equal(Object.assign( + {}, settings.defaultFeatures, settings.bonus_features[9] + )) + done() + + describe "when the user has a v1 plan", -> + beforeEach -> + MockV1Api.setUser 42, plan_name: 'free' + User.update { + _id: @user._id + }, { + overleaf: + id: 42 + } # returns a promise + + it "should set their features to the v1 plan", (done) -> + syncUserAndGetFeatures @user, (error, features) => + throw error if error? + plan = settings.plans.find (plan) -> plan.planCode == 'v1_free' + expect(features).to.deep.equal(plan.features) + done() + + describe "when the user has a v1 plan and bonus features", -> + beforeEach -> + MockV1Api.setUser 42, plan_name: 'free' + User.update { + _id: @user._id + }, { + overleaf: + id: 42 + refered_user_count: 10 + } # returns a promise + + it "should set their features to the best of the v1 plan and bonus features", (done) -> + syncUserAndGetFeatures @user, (error, features) => + throw error if error? + v1plan = settings.plans.find (plan) -> plan.planCode == 'v1_free' + expectedFeatures = Object.assign( + {}, v1plan.features, settings.bonus_features[9] + ) + expect(features).to.deep.equal(expectedFeatures) + done() + + describe "when the user has a group and personal subscription", -> + beforeEach (done) -> + Subscription.create { + admin_id: @user._id + planCode: 'professional' + customAccount: true + }, (error) => + throw error if error? + Subscription.create { + admin_id: ObjectId() + member_ids: [@user._id] + groupAccount: true + planCode: 'collaborator' + customAccount: true + }, done + return + + it "should set their features to the best set", (done) -> + syncUserAndGetFeatures @user, (error, features) => + 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 diff --git a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee index f27a4e1662..5c2cf47ad9 100644 --- a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee +++ b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee @@ -5,6 +5,10 @@ bodyParser = require('body-parser') app.use(bodyParser.json()) module.exports = MockV1Api = + users: { } + + setUser: (id, user) -> + @users[id] = user exportId: null @@ -20,6 +24,13 @@ module.exports = MockV1Api = @exportParams = null run: () -> + app.get "/api/v1/sharelatex/users/:ol_user_id/plan_code", (req, res, next) => + user = @users[req.params.ol_user_id] + if user + res.json user + else + res.sendStatus 404 + app.post "/api/v1/sharelatex/exports", (req, res, next) => #{project, version, pathname} @exportParams = Object.assign({}, req.body) @@ -28,7 +39,7 @@ module.exports = MockV1Api = app.listen 5000, (error) -> throw error if error? .on "error", (error) -> - console.error "error starting MockOverleafAPI:", error.message + console.error "error starting MockV1Api:", error.message process.exit(1) MockV1Api.run() diff --git a/services/web/test/acceptance/config/settings.test.coffee b/services/web/test/acceptance/config/settings.test.coffee new file mode 100644 index 0000000000..1bcb7426ff --- /dev/null +++ b/services/web/test/acceptance/config/settings.test.coffee @@ -0,0 +1,101 @@ +module.exports = + apis: + v1: + url: "http://localhost:5000" + user: 'overleaf' + pass: 'password' + + enableSubscriptions: true + + features: features = + v1_free: + collaborators: 1 + dropbox: false + versioning: false + github: true + templates: false + references: false + referencesSearch: false + mendeley: true + compileTimeout: 60 + compileGroup: "standard" + trackChanges: false + personal: + collaborators: 1 + dropbox: false + versioning: false + github: false + templates: false + references: false + referencesSearch: false + mendeley: false + compileTimeout: 60 + compileGroup: "standard" + trackChanges: false + collaborator: + collaborators: 10 + dropbox: true + versioning: true + github: true + templates: true + references: true + referencesSearch: true + mendeley: true + compileTimeout: 180 + compileGroup: "priority" + trackChanges: true + professional: + collaborators: -1 + dropbox: true + versioning: true + github: true + templates: true + references: true + referencesSearch: true + mendeley: true + compileTimeout: 180 + compileGroup: "priority" + trackChanges: true + + defaultFeatures: features.personal + defaultPlanCode: 'personal' + + plans: plans = [{ + planCode: "v1_free" + name: "V1 Free" + price: 0 + features: features.v1_free + },{ + planCode: "personal" + name: "Personal" + price: 0 + features: features.personal + },{ + planCode: "collaborator" + name: "Collaborator" + price: 1500 + features: features.collaborator + },{ + planCode: "professional" + name: "Professional" + price: 3000 + features: features.professional + }] + + bonus_features: + 1: + collaborators: 2 + dropbox: false + versioning: false + 3: + collaborators: 4 + dropbox: false + versioning: false + 6: + collaborators: 4 + dropbox: true + versioning: true + 9: + collaborators: -1 + dropbox: true + versioning: true From 4deaf7865da6f939241c87a1b83d986775dbd30b Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 16 May 2018 16:33:26 +0100 Subject: [PATCH 30/34] Guard against no apis.v1 setting --- .../coffee/Features/Subscription/V1SubscriptionManager.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee index 37460cd773..05dc140be2 100644 --- a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee +++ b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee @@ -20,7 +20,7 @@ module.exports = V1SubscriptionManager = return callback(null, null) V1SubscriptionManager._v1PlanRequest v1Id, (err, body) -> return callback(err) if err? - planName = body.plan_name + 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}" @@ -30,6 +30,8 @@ module.exports = V1SubscriptionManager = return callback(null, planName) _v1PlanRequest: (v1Id, callback=(err, body)->) -> + if !settings?.apis?.v1 + return callback null, null request { method: 'GET', url: settings.apis.v1.url + From 0830c473ad94da146a1549db656d1424ed224c86 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 17 May 2018 16:58:58 +0100 Subject: [PATCH 31/34] Update unit tests and refactor to make more testable --- .../Features/Referal/ReferalAllocator.coffee | 46 +--- .../Features/Referal/ReferalFeatures.coffee | 44 ++++ .../SubscriptionController.coffee | 4 +- .../Subscription/SubscriptionRouter.coffee | 2 +- .../Subscription/SubscriptionUpdater.coffee | 48 ++-- .../Subscription/UserFeaturesUpdater.coffee | 2 - .../coffee/SubscriptionTests.coffee | 2 +- .../Referal/ReferalAllocatorTests.coffee | 125 +--------- .../Referal/ReferalFeaturesTests.coffee | 65 +++++ .../SubscriptionControllerTests.coffee | 1 + .../SubscriptionUpdaterTests.coffee | 236 ++++++++++++------ .../UserFeaturesUpdaterTests.coffee | 17 +- .../V1SusbcriptionManagerTests.coffee | 128 ++++++++++ 13 files changed, 438 insertions(+), 282 deletions(-) create mode 100644 services/web/app/coffee/Features/Referal/ReferalFeatures.coffee create mode 100644 services/web/test/unit/coffee/Referal/ReferalFeaturesTests.coffee create mode 100644 services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee diff --git a/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee b/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee index aef283d68d..3344747968 100644 --- a/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee +++ b/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee @@ -1,8 +1,8 @@ _ = require("underscore") logger = require('logger-sharelatex') User = require('../../models/User').User -SubscriptionLocator = require "../Subscription/SubscriptionLocator" Settings = require "settings-sharelatex" +SubscriptionUpdater = require "../Subscription/SubscriptionUpdater" module.exports = ReferalAllocator = allocate: (referal_id, new_user_id, referal_source, referal_medium, callback = ->)-> @@ -25,48 +25,6 @@ module.exports = ReferalAllocator = if err? logger.err err:err, referal_id:referal_id, new_user_id:new_user_id, "something went wrong allocating referal" return callback(err) - ReferalAllocator.assignBonus user._id, callback + SubscriptionUpdater.refreshFeatures user._id, callback else callback() - - - - getBonusFeatures: (user_id, callback = (error) ->) -> - query = _id: user_id - User.findOne query, (error, user) -> - return callback(error) if error - return callback(new Error("user not found #{user_id} for assignBonus")) if !user? - logger.log user_id: user_id, refered_user_count: user.refered_user_count, "assigning bonus" - if user.refered_user_count? and user.refered_user_count > 0 - newFeatures = ReferalAllocator._calculateFeatures(user) - callback null, newFeatures - else - callback() - - _calculateFeatures : (user)-> - bonusLevel = ReferalAllocator._getBonusLevel(user) - currentFeatures = _.clone(user.features) #need to clone because we exend with underscore later - betterBonusFeatures = {} - _.each Settings.bonus_features["#{bonusLevel}"], (bonusLevel, key)-> - currentLevel = user?.features?[key] - if _.isBoolean(currentLevel) and currentLevel == false - betterBonusFeatures[key] = bonusLevel - - if _.isNumber(currentLevel) - if currentLevel == -1 - return - bonusIsGreaterThanCurrent = currentLevel < bonusLevel - if bonusIsGreaterThanCurrent or bonusLevel == -1 - betterBonusFeatures[key] = bonusLevel - newFeatures = _.extend(currentFeatures, betterBonusFeatures) - return newFeatures - - - _getBonusLevel: (user)-> - highestBonusLevel = 0 - _.each _.keys(Settings.bonus_features), (level)-> - levelIsLessThanUser = level <= user.refered_user_count - levelIsMoreThanCurrentHighest = level >= highestBonusLevel - if levelIsLessThanUser and levelIsMoreThanCurrentHighest - highestBonusLevel = level - return highestBonusLevel diff --git a/services/web/app/coffee/Features/Referal/ReferalFeatures.coffee b/services/web/app/coffee/Features/Referal/ReferalFeatures.coffee new file mode 100644 index 0000000000..34651ef1f5 --- /dev/null +++ b/services/web/app/coffee/Features/Referal/ReferalFeatures.coffee @@ -0,0 +1,44 @@ +_ = require("underscore") +logger = require('logger-sharelatex') +User = require('../../models/User').User +Settings = require "settings-sharelatex" + +module.exports = ReferalFeatures = + getBonusFeatures: (user_id, callback = (error) ->) -> + query = _id: user_id + User.findOne query, (error, user) -> + return callback(error) if error + return callback(new Error("user not found #{user_id} for assignBonus")) if !user? + logger.log user_id: user_id, refered_user_count: user.refered_user_count, "assigning bonus" + if user.refered_user_count? and user.refered_user_count > 0 + newFeatures = ReferalFeatures._calculateFeatures(user) + callback null, newFeatures + else + callback null, {} + + _calculateFeatures : (user)-> + bonusLevel = ReferalFeatures._getBonusLevel(user) + currentFeatures = _.clone(user.features) #need to clone because we exend with underscore later + betterBonusFeatures = {} + _.each Settings.bonus_features["#{bonusLevel}"], (bonusLevel, key)-> + currentLevel = user?.features?[key] + if _.isBoolean(currentLevel) and currentLevel == false + betterBonusFeatures[key] = bonusLevel + + if _.isNumber(currentLevel) + if currentLevel == -1 + return + bonusIsGreaterThanCurrent = currentLevel < bonusLevel + if bonusIsGreaterThanCurrent or bonusLevel == -1 + betterBonusFeatures[key] = bonusLevel + newFeatures = _.extend(currentFeatures, betterBonusFeatures) + return newFeatures + + _getBonusLevel: (user)-> + highestBonusLevel = 0 + _.each _.keys(Settings.bonus_features), (level)-> + levelIsLessThanUser = level <= user.refered_user_count + levelIsMoreThanCurrentHighest = level >= highestBonusLevel + if levelIsLessThanUser and levelIsMoreThanCurrentHighest + highestBonusLevel = level + return highestBonusLevel diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index 1b7078d9cb..f127f7ed35 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -239,8 +239,8 @@ module.exports = SubscriptionController = req.body = body next() - refreshUserSubscription: (req, res, next) -> + refreshUserFeatures: (req, res, next) -> {user_id} = req.params - SubscriptionUpdater.refreshSubscription user_id, (error) -> + SubscriptionUpdater.refreshFeatures user_id, (error) -> return next(error) if error? res.sendStatus 200 \ No newline at end of file diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee index 692f6c92c8..8c5f631e12 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionRouter.coffee @@ -47,5 +47,5 @@ module.exports = webRouter.post "/user/subscription/upgrade-annual", AuthenticationController.requireLogin(), SubscriptionController.processUpgradeToAnnualPlan # Currently used in acceptance tests only, as a way to trigger the syncing logic - publicApiRouter.post "/user/:user_id/subscription/sync", AuthenticationController.httpAuth, SubscriptionController.refreshUserSubscription + publicApiRouter.post "/user/:user_id/features/sync", AuthenticationController.httpAuth, SubscriptionController.refreshUserFeatures diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee index c76bffd453..8e583bd07b 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee @@ -7,7 +7,7 @@ PlansLocator = require("./PlansLocator") Settings = require("settings-sharelatex") logger = require("logger-sharelatex") ObjectId = require('mongoose').Types.ObjectId -ReferalAllocator = require("../Referal/ReferalAllocator") +ReferalFeatures = require("../Referal/ReferalFeatures") V1SubscriptionManager = require("./V1SubscriptionManager") oneMonthInSeconds = 60 * 60 * 24 * 30 @@ -54,7 +54,7 @@ module.exports = SubscriptionUpdater = if err? logger.err err:err, searchOps:searchOps, removeOperation:removeOperation, "error removing user from group" return callback(err) - SubscriptionUpdater._setUsersMinimumFeatures user_id, callback + SubscriptionUpdater.refreshFeatures user_id, callback removeEmailInviteFromGroup: (adminUser_id, email, callback)-> Subscription.update { @@ -63,9 +63,6 @@ module.exports = SubscriptionUpdater = invited_emails: email }, callback - refreshSubscription: (user_id, callback=(err)->) -> - SubscriptionUpdater._setUsersMinimumFeatures user_id, callback - deleteSubscription: (subscription_id, callback = (error) ->) -> SubscriptionLocator.getSubscription subscription_id, (err, subscription) -> return callback(err) if err? @@ -73,7 +70,7 @@ module.exports = SubscriptionUpdater = logger.log {subscription_id, affected_user_ids}, "deleting subscription and downgrading users" Subscription.remove {_id: ObjectId(subscription_id)}, (err) -> return callback(err) if err? - async.mapSeries affected_user_ids, SubscriptionUpdater._setUsersMinimumFeatures, callback + async.mapSeries affected_user_ids, SubscriptionUpdater.refreshFeatures, callback _createNewSubscription: (adminUser_id, callback)-> logger.log adminUser_id:adminUser_id, "creating new subscription" @@ -101,36 +98,41 @@ module.exports = SubscriptionUpdater = allIds = _.union subscription.member_ids, [subscription.admin_id] jobs = allIds.map (user_id)-> return (cb)-> - SubscriptionUpdater._setUsersMinimumFeatures user_id, cb + SubscriptionUpdater.refreshFeatures user_id, cb async.series jobs, callback - _setUsersMinimumFeatures: (user_id, callback)-> + refreshFeatures: (user_id, callback)-> jobs = - individualFeatures: (cb)-> - SubscriptionLocator.getUsersSubscription user_id, (err, sub)-> - cb err, SubscriptionUpdater._subscriptionToFeatures(sub) - groupFeatures: (cb) -> - SubscriptionLocator.getGroupSubscriptionsMemberOf user_id, (err, subs) -> - cb err, (subs or []).map SubscriptionUpdater._subscriptionToFeatures - v1Features: (cb) -> - V1SubscriptionManager.getPlanCodeFromV1 user_id, (err, planCode) -> - cb err, SubscriptionUpdater._planCodeToFeatures(planCode) - bonusFeatures: (cb) -> - ReferalAllocator.getBonusFeatures user_id, cb + individualFeatures: (cb) -> SubscriptionUpdater._getIndividualFeatures user_id, cb + groupFeatureSets: (cb) -> SubscriptionUpdater._getGroupFeatureSets user_id, cb + v1Features: (cb) -> SubscriptionUpdater._getV1Features user_id, cb + bonusFeatures: (cb) -> ReferalFeatures.getBonusFeatures user_id, cb async.series jobs, (err, results)-> if err? logger.err err:err, user_id:user_id, - "error getting subscription or group for _setUsersMinimumFeatures" + "error getting subscription or group for refreshFeatures" return callback(err) - {individualFeatures, groupFeatures, v1Features, bonusFeatures} = results - logger.log {user_id, individualFeatures, groupFeatures, v1Features, bonusFeatures}, 'merging user features' - featureSets = groupFeatures.concat [individualFeatures, v1Features, bonusFeatures] + {individualFeatures, groupFeatureSets, v1Features, bonusFeatures} = results + logger.log {user_id, individualFeatures, groupFeatureSets, v1Features, bonusFeatures}, 'merging user features' + featureSets = groupFeatureSets.concat [individualFeatures, v1Features, bonusFeatures] features = _.reduce(featureSets, SubscriptionUpdater._mergeFeatures, Settings.defaultFeatures) logger.log {user_id, features}, 'updating user features' UserFeaturesUpdater.updateFeatures user_id, features, callback + _getIndividualFeatures: (user_id, callback = (error, features = {}) ->) -> + SubscriptionLocator.getUsersSubscription user_id, (err, sub)-> + callback err, SubscriptionUpdater._subscriptionToFeatures(sub) + + _getGroupFeatureSets: (user_id, callback = (error, featureSets = []) ->) -> + SubscriptionLocator.getGroupSubscriptionsMemberOf user_id, (err, subs) -> + callback err, (subs or []).map SubscriptionUpdater._subscriptionToFeatures + + _getV1Features: (user_id, callback = (error, features = {}) ->) -> + V1SubscriptionManager.getPlanCodeFromV1 user_id, (err, planCode) -> + callback err, SubscriptionUpdater._planCodeToFeatures(planCode) + _mergeFeatures: (featuresA, featuresB) -> features = Object.assign({}, featuresA) for key, value of featuresB diff --git a/services/web/app/coffee/Features/Subscription/UserFeaturesUpdater.coffee b/services/web/app/coffee/Features/Subscription/UserFeaturesUpdater.coffee index 520139e9be..28a49b2126 100644 --- a/services/web/app/coffee/Features/Subscription/UserFeaturesUpdater.coffee +++ b/services/web/app/coffee/Features/Subscription/UserFeaturesUpdater.coffee @@ -1,9 +1,7 @@ logger = require("logger-sharelatex") User = require('../../models/User').User -PlansLocator = require("./PlansLocator") module.exports = - updateFeatures: (user_id, features, callback = (err, features)->)-> conditions = _id:user_id update = {} diff --git a/services/web/test/acceptance/coffee/SubscriptionTests.coffee b/services/web/test/acceptance/coffee/SubscriptionTests.coffee index 55f3a0efbc..ce762ea0ee 100644 --- a/services/web/test/acceptance/coffee/SubscriptionTests.coffee +++ b/services/web/test/acceptance/coffee/SubscriptionTests.coffee @@ -12,7 +12,7 @@ MockV1Api = require "./helpers/MockV1Api" syncUserAndGetFeatures = (user, callback = (error, features) ->) -> request { method: 'POST', - url: "/user/#{user._id}/subscription/sync", + url: "/user/#{user._id}/features/sync", auth: user: 'sharelatex' pass: 'password' diff --git a/services/web/test/unit/coffee/Referal/ReferalAllocatorTests.coffee b/services/web/test/unit/coffee/Referal/ReferalAllocatorTests.coffee index 3fe925f91c..ef0f0243c8 100644 --- a/services/web/test/unit/coffee/Referal/ReferalAllocatorTests.coffee +++ b/services/web/test/unit/coffee/Referal/ReferalAllocatorTests.coffee @@ -4,12 +4,12 @@ require('chai').should() sinon = require('sinon') modulePath = require('path').join __dirname, '../../../../app/js/Features/Referal/ReferalAllocator.js' -describe 'Referalallocator', -> +describe 'ReferalAllocator', -> beforeEach -> @ReferalAllocator = SandboxedModule.require modulePath, requires: '../../models/User': User: @User = {} - "../Subscription/SubscriptionLocator": @SubscriptionLocator = {} + "../Subscription/SubscriptionUpdater": @SubscriptionUpdater = {} "settings-sharelatex": @Settings = {} 'logger-sharelatex': log:-> @@ -26,7 +26,7 @@ describe 'Referalallocator', -> @referal_source = "bonus" @User.update = sinon.stub().callsArgWith 3, null @User.findOne = sinon.stub().callsArgWith 1, null, { _id: @user_id } - @ReferalAllocator.assignBonus = sinon.stub().callsArg 1 + @SubscriptionUpdater.refreshFeatures = sinon.stub().yields() @ReferalAllocator.allocate @referal_id, @new_user_id, @referal_source, @referal_medium, @callback it 'should update the referring user with the refered users id', -> @@ -44,8 +44,8 @@ describe 'Referalallocator', -> .calledWith( referal_id: @referal_id ) .should.equal true - it "shoudl assign the user their bonus", -> - @ReferalAllocator.assignBonus + it "should refresh the user's subscription", -> + @SubscriptionUpdater.refreshFeatures .calledWith(@user_id) .should.equal true @@ -57,7 +57,7 @@ describe 'Referalallocator', -> @referal_source = "public_share" @User.update = sinon.stub().callsArgWith 3, null @User.findOne = sinon.stub().callsArgWith 1, null, { _id: @user_id } - @ReferalAllocator.assignBonus = sinon.stub().callsArg 1 + @SubscriptionUpdater.refreshFeatures = sinon.stub().yields() @ReferalAllocator.allocate @referal_id, @new_user_id, @referal_source, @referal_medium, @callback it 'should not update the referring user with the refered users id', -> @@ -69,118 +69,7 @@ describe 'Referalallocator', -> .should.equal true it "should not assign the user a bonus", -> - @ReferalAllocator.assignBonus.called.should.equal false + @SubscriptionUpdater.refreshFeatures.called.should.equal false it "should call the callback", -> @callback.called.should.equal true - - describe "assignBonus", -> - beforeEach -> - @refered_user_count = 3 - @Settings.bonus_features = - "3": - collaborators: 3 - dropbox: false - versioning: false - stubbedUser = { - refered_user_count: @refered_user_count, - features:{collaborators:1, dropbox:false, versioning:false} - } - - @User.findOne = sinon.stub().callsArgWith 1, null, stubbedUser - @User.update = sinon.stub().callsArgWith 2, null - @ReferalAllocator.assignBonus @user_id, @callback - - it "should get the users number of refered user", -> - @User.findOne - .calledWith(_id: @user_id) - .should.equal true - - it "should update the user to bonus features", -> - @User.update - .calledWith({ - _id: @user_id - }, { - $set: - features: - @Settings.bonus_features[@refered_user_count.toString()] - }) - .should.equal true - - it "should call the callback", -> - @callback.called.should.equal true - - describe "when there is nothing to assign", -> - - beforeEach -> - @ReferalAllocator._calculateBonuses = sinon.stub().returns({}) - @stubbedUser = - refered_user_count:4 - features:{collaborators:3, versioning:true, dropbox:false} - @Settings.bonus_features = - "4": - collaborators:3 - versioning:true - dropbox:false - @User.findOne = sinon.stub().callsArgWith 1, null, @stubbedUser - @User.update = sinon.stub().callsArgWith 2, null - - it "should not call update if there are no bonuses to apply", (done)-> - @ReferalAllocator.assignBonus @user_id, (err)=> - @User.update.called.should.equal false - done() - - describe "when the user has better features already", -> - - beforeEach -> - @refered_user_count = 3 - @stubbedUser = - refered_user_count:4 - features: - collaborators:3 - dropbox:false - versioning:false - @Settings.bonus_features = - "4": - collaborators: 10 - dropbox: true - versioning: false - - @User.findOne = sinon.stub().callsArgWith 1, null, @stubbedUser - @User.update = sinon.stub().callsArgWith 2, null - - it "should not set in in mongo when the feature is better", (done)-> - @ReferalAllocator.assignBonus @user_id, => - @User.update.calledWith({_id: @user_id }, {$set: features:{dropbox:true, versioning:false, collaborators:10} }).should.equal true - done() - - it "should not overright if the user has -1 users", (done)-> - @stubbedUser.features.collaborators = -1 - @ReferalAllocator.assignBonus @user_id, => - @User.update.calledWith({_id: @user_id }, {$set: features:{dropbox:true, versioning:false, collaborators:-1} }).should.equal true - done() - - describe "when the user is not at a bonus level", -> - beforeEach -> - @refered_user_count = 0 - @Settings.bonus_features = - "1": - collaborators: 3 - dropbox: false - versioning: false - @User.findOne = sinon.stub().callsArgWith 1, null, { refered_user_count: @refered_user_count } - @User.update = sinon.stub().callsArgWith 2, null - @ReferalAllocator.assignBonus @user_id, @callback - - it "should get the users number of refered user", -> - @User.findOne - .calledWith(_id: @user_id) - .should.equal true - - it "should not update the user to bonus features", -> - @User.update.called.should.equal false - - it "should call the callback", -> - @callback.called.should.equal true - - diff --git a/services/web/test/unit/coffee/Referal/ReferalFeaturesTests.coffee b/services/web/test/unit/coffee/Referal/ReferalFeaturesTests.coffee new file mode 100644 index 0000000000..dfa70f8ebe --- /dev/null +++ b/services/web/test/unit/coffee/Referal/ReferalFeaturesTests.coffee @@ -0,0 +1,65 @@ +SandboxedModule = require('sandboxed-module') +assert = require('assert') +require('chai').should() +sinon = require('sinon') +modulePath = require('path').join __dirname, '../../../../app/js/Features/Referal/ReferalFeatures.js' + +describe 'ReferalFeatures', -> + + beforeEach -> + @ReferalFeatures = SandboxedModule.require modulePath, requires: + '../../models/User': User: @User = {} + "settings-sharelatex": @Settings = {} + 'logger-sharelatex': + log:-> + err:-> + @callback = sinon.stub() + @referal_id = "referal-id-123" + @referal_medium = "twitter" + @user_id = "user-id-123" + @new_user_id = "new-user-id-123" + + describe "getBonusFeatures", -> + beforeEach -> + @refered_user_count = 3 + @Settings.bonus_features = + "3": + collaborators: 3 + dropbox: false + versioning: false + stubbedUser = { + refered_user_count: @refered_user_count, + features:{collaborators:1, dropbox:false, versioning:false} + } + + @User.findOne = sinon.stub().callsArgWith 1, null, stubbedUser + @ReferalFeatures.getBonusFeatures @user_id, @callback + + it "should get the users number of refered user", -> + @User.findOne + .calledWith(_id: @user_id) + .should.equal true + + it "should call the callback with the features", -> + @callback.calledWith(null, @Settings.bonus_features[3]).should.equal true + + describe "when the user is not at a bonus level", -> + beforeEach -> + @refered_user_count = 0 + @Settings.bonus_features = + "1": + collaborators: 3 + dropbox: false + versioning: false + @User.findOne = sinon.stub().callsArgWith 1, null, { refered_user_count: @refered_user_count } + @ReferalFeatures.getBonusFeatures @user_id, @callback + + it "should get the users number of refered user", -> + @User.findOne + .calledWith(_id: @user_id) + .should.equal true + + it "should call the callback with no features", -> + @callback.calledWith(null, {}).should.equal true + + diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee index 22f571ffa8..47301ecf7c 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee @@ -75,6 +75,7 @@ describe "SubscriptionController", -> "./SubscriptionDomainHandler":@SubscriptionDomainHandler "../User/UserGetter": @UserGetter "./RecurlyWrapper": @RecurlyWrapper = {} + "./SubscriptionUpdater": @SubscriptionUpdater = {} @res = new MockResponse() diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee index 93479a03a1..71ae6346ce 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee @@ -1,5 +1,6 @@ SandboxedModule = require('sandboxed-module') should = require('chai').should() +expect = require('chai').expect sinon = require 'sinon' modulePath = "../../../../app/js/Features/Subscription/SubscriptionUpdater" assert = require("chai").assert @@ -22,6 +23,7 @@ describe "SubscriptionUpdater", -> save: sinon.stub().callsArgWith(0) freeTrial:{} planCode:"student_or_something" + @user_id = @adminuser_id @groupSubscription = admin_id: @adminUser._id @@ -48,15 +50,15 @@ describe "SubscriptionUpdater", -> @Settings = freeTrialPlanCode: "collaborator" defaultPlanCode: "personal" + defaultFeatures: { "default": "features" } @UserFeaturesUpdater = - updateFeatures : sinon.stub().callsArgWith(2) + updateFeatures : sinon.stub().yields() @PlansLocator = findLocalPlanInSettings: sinon.stub().returns({}) - @ReferalAllocator = assignBonus:sinon.stub().callsArgWith(1) - @ReferalAllocator.cock = true + @ReferalFeatures = getBonusFeatures: sinon.stub().callsArgWith(1) @Modules = {hooks: {fire: sinon.stub().callsArgWith(2, null, null)}} @SubscriptionUpdater = SandboxedModule.require modulePath, requires: '../../models/Subscription': Subscription:@SubscriptionModel @@ -65,8 +67,9 @@ describe "SubscriptionUpdater", -> './PlansLocator': @PlansLocator "logger-sharelatex": log:-> 'settings-sharelatex': @Settings - "../Referal/ReferalAllocator" : @ReferalAllocator + "../Referal/ReferalFeatures" : @ReferalFeatures '../../infrastructure/Modules': @Modules + "./V1SubscriptionManager": @V1SubscriptionManager = {} describe "syncSubscription", -> @@ -97,7 +100,7 @@ describe "SubscriptionUpdater", -> describe "_updateSubscriptionFromRecurly", -> beforeEach -> - @SubscriptionUpdater._setUsersMinimumFeatures = sinon.stub().callsArgWith(1) + @SubscriptionUpdater.refreshFeatures = sinon.stub().callsArgWith(1) it "should update the subscription with token etc when not expired", (done)-> @SubscriptionUpdater._updateSubscriptionFromRecurly @recurlySubscription, @subscription, (err)=> @@ -108,7 +111,7 @@ describe "SubscriptionUpdater", -> assert.equal(@subscription.freeTrial.expiresAt, undefined) assert.equal(@subscription.freeTrial.planCode, undefined) @subscription.save.called.should.equal true - @SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@adminUser._id).should.equal true + @SubscriptionUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true done() it "should remove the recurlySubscription_id when expired", (done)-> @@ -117,15 +120,15 @@ describe "SubscriptionUpdater", -> @SubscriptionUpdater._updateSubscriptionFromRecurly @recurlySubscription, @subscription, (err)=> assert.equal(@subscription.recurlySubscription_id, undefined) @subscription.save.called.should.equal true - @SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@adminUser._id).should.equal true + @SubscriptionUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true done() it "should update all the users features", (done)-> @SubscriptionUpdater._updateSubscriptionFromRecurly @recurlySubscription, @subscription, (err)=> - @SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@adminUser._id).should.equal true - @SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@allUserIds[0]).should.equal true - @SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@allUserIds[1]).should.equal true - @SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@allUserIds[2]).should.equal true + @SubscriptionUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true + @SubscriptionUpdater.refreshFeatures.calledWith(@allUserIds[0]).should.equal true + @SubscriptionUpdater.refreshFeatures.calledWith(@allUserIds[1]).should.equal true + @SubscriptionUpdater.refreshFeatures.calledWith(@allUserIds[2]).should.equal true done() it "should set group to true and save how many members can be added to group", (done)-> @@ -168,7 +171,7 @@ describe "SubscriptionUpdater", -> describe "removeUserFromGroup", -> beforeEach -> - @SubscriptionUpdater._setUsersMinimumFeatures = sinon.stub().callsArgWith(1) + @SubscriptionUpdater.refreshFeatures = sinon.stub().callsArgWith(1) it "should pull the users id from the group", (done)-> @SubscriptionUpdater.removeUserFromGroup @adminUser._id, @otherUserId, => @@ -181,70 +184,159 @@ describe "SubscriptionUpdater", -> it "should update the users features", (done)-> @SubscriptionUpdater.removeUserFromGroup @adminUser._id, @otherUserId, => - @SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@otherUserId).should.equal true + @SubscriptionUpdater.refreshFeatures.calledWith(@otherUserId).should.equal true done() - describe "_setUsersMinimumFeatures", -> + describe "refreshFeatures", -> + beforeEach -> + @SubscriptionUpdater._getIndividualFeatures = sinon.stub().yields(null, { 'individual': 'features' }) + @SubscriptionUpdater._getGroupFeatureSets = sinon.stub().yields(null, [{ 'group': 'features' }, { 'group': 'features2' }]) + @SubscriptionUpdater._getV1Features = sinon.stub().yields(null, { 'v1': 'features' }) + @ReferalFeatures.getBonusFeatures = sinon.stub().yields(null, { 'bonus': 'features' }) + @SubscriptionUpdater._mergeFeatures = sinon.stub().returns({'merged': 'features'}) + @callback = sinon.stub() + @SubscriptionUpdater.refreshFeatures @user_id, @callback - it "should call updateFeatures with the subscription if set", (done)-> - @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, @subscription) - @SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null) + it "should get the individual features", -> + @SubscriptionUpdater._getIndividualFeatures + .calledWith(@user_id) + .should.equal true - @SubscriptionUpdater._setUsersMinimumFeatures @adminUser._id, (err)=> - args = @UserFeaturesUpdater.updateFeatures.args[0] - assert.equal args[0], @adminUser._id - assert.equal args[1], @subscription.planCode - done() + it "should get the group features", -> + @SubscriptionUpdater._getGroupFeatureSets + .calledWith(@user_id) + .should.equal true - it "should call updateFeatures with the group subscription if set", (done)-> - @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null) - @SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null, @groupSubscription) + it "should get the v1 features", -> + @SubscriptionUpdater._getV1Features + .calledWith(@user_id) + .should.equal true - @SubscriptionUpdater._setUsersMinimumFeatures @adminUser._id, (err)=> - args = @UserFeaturesUpdater.updateFeatures.args[0] - assert.equal args[0], @adminUser._id - assert.equal args[1], @groupSubscription.planCode - done() + it "should get the bonus features", -> + @ReferalFeatures.getBonusFeatures + .calledWith(@user_id) + .should.equal true - it "should call updateFeatures with the overleaf subscription if set", (done)-> - @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null) - @SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null, null) - @Modules.hooks.fire = sinon.stub().callsArgWith(2, null, ['ol_pro']) + it "should merge from the default features", -> + @SubscriptionUpdater._mergeFeatures.calledWith(@Settings.defaultFeatures).should.equal true - @SubscriptionUpdater._setUsersMinimumFeatures @adminUser._id, (err)=> - args = @UserFeaturesUpdater.updateFeatures.args[0] - assert.equal args[0], @adminUser._id - assert.equal args[1], 'ol_pro' - done() + it "should merge the individual features", -> + @SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'individual': 'features' }).should.equal true - it "should call not call updateFeatures with users subscription if the subscription plan code is the default one (downgraded)", (done)-> - @subscription.planCode = @Settings.defaultPlanCode - @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null, @subscription) - @SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null, @groupSubscription) - @Modules.hooks.fire = sinon.stub().callsArgWith(2, null, null) - @SubscriptionUpdater._setUsersMinimumFeatures @adminuser_id, (err)=> - args = @UserFeaturesUpdater.updateFeatures.args[0] - assert.equal args[0], @adminUser._id - assert.equal args[1], @groupSubscription.planCode - done() + it "should merge the group features", -> + @SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'group': 'features' }).should.equal true + @SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'group': 'features2' }).should.equal true + it "should merge the v1 features", -> + @SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'v1': 'features' }).should.equal true - it "should call updateFeatures with default if there are no subscriptions for user", (done)-> - @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null) - @SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null) - @Modules.hooks.fire = sinon.stub().callsArgWith(2, null, null) - @SubscriptionUpdater._setUsersMinimumFeatures @adminuser_id, (err)=> - args = @UserFeaturesUpdater.updateFeatures.args[0] - assert.equal args[0], @adminUser._id - assert.equal args[1], @Settings.defaultPlanCode - done() + it "should merge the bonus features", -> + @SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'bonus': 'features' }).should.equal true - it "should call assignBonus", (done)-> - @SubscriptionLocator.getUsersSubscription.callsArgWith(1, null) - @SubscriptionLocator.getGroupSubscriptionMemberOf.callsArgWith(1, null) - @SubscriptionUpdater._setUsersMinimumFeatures @adminuser_id, (err)=> - @ReferalAllocator.assignBonus.calledWith(@adminuser_id).should.equal true - done() + it "should update the user with the merged features", -> + @UserFeaturesUpdater.updateFeatures + .calledWith(@user_id, {'merged': 'features'}) + .should.equal true + + describe "_mergeFeatures", -> + it "should prefer priority over standard for compileGroup", -> + expect(@SubscriptionUpdater._mergeFeatures({ + compileGroup: 'priority' + }, { + compileGroup: 'standard' + })).to.deep.equal({ + compileGroup: 'priority' + }) + expect(@SubscriptionUpdater._mergeFeatures({ + compileGroup: 'standard' + }, { + compileGroup: 'priority' + })).to.deep.equal({ + compileGroup: 'priority' + }) + expect(@SubscriptionUpdater._mergeFeatures({ + compileGroup: 'priority' + }, { + compileGroup: 'priority' + })).to.deep.equal({ + compileGroup: 'priority' + }) + expect(@SubscriptionUpdater._mergeFeatures({ + compileGroup: 'standard' + }, { + compileGroup: 'standard' + })).to.deep.equal({ + compileGroup: 'standard' + }) + + it "should prefer -1 over any other for collaborators", -> + expect(@SubscriptionUpdater._mergeFeatures({ + collaborators: -1 + }, { + collaborators: 10 + })).to.deep.equal({ + collaborators: -1 + }) + expect(@SubscriptionUpdater._mergeFeatures({ + collaborators: 10 + }, { + collaborators: -1 + })).to.deep.equal({ + collaborators: -1 + }) + expect(@SubscriptionUpdater._mergeFeatures({ + collaborators: 4 + }, { + collaborators: 10 + })).to.deep.equal({ + collaborators: 10 + }) + + it "should prefer the higher of compileTimeout", -> + expect(@SubscriptionUpdater._mergeFeatures({ + compileTimeout: 20 + }, { + compileTimeout: 10 + })).to.deep.equal({ + compileTimeout: 20 + }) + expect(@SubscriptionUpdater._mergeFeatures({ + compileTimeout: 10 + }, { + compileTimeout: 20 + })).to.deep.equal({ + compileTimeout: 20 + }) + + it "should prefer the true over false for other keys", -> + expect(@SubscriptionUpdater._mergeFeatures({ + github: true + }, { + github: false + })).to.deep.equal({ + github: true + }) + expect(@SubscriptionUpdater._mergeFeatures({ + github: false + }, { + github: true + })).to.deep.equal({ + github: true + }) + expect(@SubscriptionUpdater._mergeFeatures({ + github: true + }, { + github: true + })).to.deep.equal({ + github: true + }) + expect(@SubscriptionUpdater._mergeFeatures({ + github: false + }, { + github: false + })).to.deep.equal({ + github: false + }) describe "deleteSubscription", -> beforeEach (done) -> @@ -255,7 +347,7 @@ describe "SubscriptionUpdater", -> member_ids: [ ObjectId(), ObjectId(), ObjectId() ] } @SubscriptionLocator.getSubscription = sinon.stub().yields(null, @subscription) - @SubscriptionUpdater._setUsersMinimumFeatures = sinon.stub().yields() + @SubscriptionUpdater.refreshFeatures = sinon.stub().yields() @SubscriptionUpdater.deleteSubscription @subscription_id, done it "should look up the subscription", -> @@ -269,22 +361,12 @@ describe "SubscriptionUpdater", -> .should.equal true it "should downgrade the admin_id", -> - @SubscriptionUpdater._setUsersMinimumFeatures + @SubscriptionUpdater.refreshFeatures .calledWith(@subscription.admin_id) .should.equal true it "should downgrade all of the members", -> for user_id in @subscription.member_ids - @SubscriptionUpdater._setUsersMinimumFeatures + @SubscriptionUpdater.refreshFeatures .calledWith(user_id) .should.equal true - - describe 'refreshSubscription', -> - beforeEach -> - @SubscriptionUpdater._setUsersMinimumFeatures = sinon.stub() - .callsArgWith(1, null) - - it 'should call to _setUsersMinimumFeatures', -> - @SubscriptionUpdater.refreshSubscription(@adminUser._id, ()->) - @SubscriptionUpdater._setUsersMinimumFeatures.callCount.should.equal 1 - @SubscriptionUpdater._setUsersMinimumFeatures.calledWith(@adminUser._id).should.equal true diff --git a/services/web/test/unit/coffee/Subscription/UserFeaturesUpdaterTests.coffee b/services/web/test/unit/coffee/Subscription/UserFeaturesUpdaterTests.coffee index d388d67c3b..1ba4e0b32f 100644 --- a/services/web/test/unit/coffee/Subscription/UserFeaturesUpdaterTests.coffee +++ b/services/web/test/unit/coffee/Subscription/UserFeaturesUpdaterTests.coffee @@ -4,31 +4,20 @@ sinon = require 'sinon' modulePath = "../../../../app/js/Features/Subscription/UserFeaturesUpdater" assert = require("chai").assert - describe "UserFeaturesUpdater", -> - beforeEach -> - @User = update: sinon.stub().callsArgWith(2) - - @PlansLocator = - findLocalPlanInSettings : sinon.stub() - @UserFeaturesUpdater = SandboxedModule.require modulePath, requires: '../../models/User': User:@User "logger-sharelatex": log:-> - './PlansLocator': @PlansLocator describe "updateFeatures", -> - it "should send the users features", (done)-> user_id = "5208dd34438842e2db000005" - plan_code = "student" - @features = features:{versioning:true, collaborators:10} - @PlansLocator.findLocalPlanInSettings = sinon.stub().returns(@features) - @UserFeaturesUpdater.updateFeatures user_id, plan_code, (err, features)=> + @features = {versioning:true, collaborators:10} + @UserFeaturesUpdater.updateFeatures user_id, @features, (err, features)=> update = {"features.versioning":true, "features.collaborators":10} @User.update.calledWith({"_id":user_id}, update).should.equal true - features.should.deep.equal @features.features + features.should.deep.equal @features done() \ No newline at end of file diff --git a/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee b/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee new file mode 100644 index 0000000000..ae6237e627 --- /dev/null +++ b/services/web/test/unit/coffee/Subscription/V1SusbcriptionManagerTests.coffee @@ -0,0 +1,128 @@ +should = require('chai').should() +SandboxedModule = require('sandboxed-module') +assert = require('assert') +path = require('path') +modulePath = path.join __dirname, '../../../../app/js/Features/Subscription/V1SubscriptionManager' +sinon = require("sinon") +expect = require("chai").expect + + +describe 'V1SubscriptionManager', -> + beforeEach -> + @V1SubscriptionManager = SandboxedModule.require modulePath, requires: + "../User/UserGetter": @UserGetter = {} + "logger-sharelatex": + log: sinon.stub() + err: sinon.stub() + warn: sinon.stub() + "settings-sharelatex": + overleaf: + host: @host = "http://overleaf.example.com" + "request": @request = sinon.stub() + @V1SubscriptionManager._v1PlanRequest = sinon.stub() + @userId = 'abcd' + @v1UserId = 42 + @user = + _id: @userId + email: 'user@example.com' + overleaf: + id: @v1UserId + + describe 'getPlanCodeFromV1', -> + beforeEach -> + @responseBody = + id: 32, + plan_name: 'pro' + @UserGetter.getUser = sinon.stub() + .yields(null, @user) + @V1SubscriptionManager._v1PlanRequest = sinon.stub() + .yields(null, @responseBody) + @call = (cb) => + @V1SubscriptionManager.getPlanCodeFromV1 @userId, cb + + describe 'when all goes well', -> + + it 'should call getUser', (done) -> + @call (err, planCode) => + expect( + @UserGetter.getUser.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 + ) + ).to.equal true + done() + + it 'should produce a plan-code without error', (done) -> + @call (err, planCode) => + expect(err).to.not.exist + expect(planCode).to.equal 'v1_pro' + done() + + describe 'when the plan_name from v1 is null', -> + beforeEach -> + @responseBody.plan_name = null + + it 'should produce a null plan-code without error', (done) -> + @call (err, planCode) => + expect(err).to.not.exist + expect(planCode).to.equal null + done() + + describe 'when getUser produces an error', -> + beforeEach -> + @UserGetter.getUser = sinon.stub() + .yields(new Error('woops')) + + it 'should not call _v1PlanRequest', (done) -> + @call (err, planCode) => + expect( + @V1SubscriptionManager._v1PlanRequest.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) + + it 'should not call _v1PlanRequest', (done) -> + @call (err, planCode) => + expect( + @V1SubscriptionManager._v1PlanRequest.callCount + ).to.equal 0 + done() + + it 'should produce a null plan-code, without error', (done) -> + @call (err, planCode) => + 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')) + + it 'should produce an error', (done) -> + @call (err, planCode) => + expect(err).to.exist + expect(planCode).to.not.exist + done() From 50bd60dd51fb483330d1ca52f0056e9e2b783166 Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 21 May 2018 15:06:01 +0100 Subject: [PATCH 32/34] Split FeaturesUpdater out of SubscriptionUpdater --- .../Subscription/FeaturesUpdater.coffee | 83 ++++++++ .../SubscriptionController.coffee | 4 +- .../Subscription/SubscriptionUpdater.coffee | 88 +-------- .../acceptance/config/settings.test.coffee | 6 - .../Subscription/FeaturesUpdaterTests.coffee | 173 ++++++++++++++++ .../SubscriptionControllerTests.coffee | 2 +- .../SubscriptionUpdaterTests.coffee | 184 ++---------------- 7 files changed, 284 insertions(+), 256 deletions(-) create mode 100644 services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee create mode 100644 services/web/test/unit/coffee/Subscription/FeaturesUpdaterTests.coffee diff --git a/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee new file mode 100644 index 0000000000..5c176c611f --- /dev/null +++ b/services/web/app/coffee/Features/Subscription/FeaturesUpdater.coffee @@ -0,0 +1,83 @@ +async = require("async") +PlansLocator = require("./PlansLocator") +_ = require("underscore") +SubscriptionLocator = require("./SubscriptionLocator") +UserFeaturesUpdater = require("./UserFeaturesUpdater") +Settings = require("settings-sharelatex") +logger = require("logger-sharelatex") +ReferalFeatures = require("../Referal/ReferalFeatures") +V1SubscriptionManager = require("./V1SubscriptionManager") + +oneMonthInSeconds = 60 * 60 * 24 * 30 + +module.exports = FeaturesUpdater = + refreshFeatures: (user_id, callback)-> + jobs = + individualFeatures: (cb) -> FeaturesUpdater._getIndividualFeatures user_id, cb + groupFeatureSets: (cb) -> FeaturesUpdater._getGroupFeatureSets user_id, cb + v1Features: (cb) -> FeaturesUpdater._getV1Features user_id, cb + bonusFeatures: (cb) -> ReferalFeatures.getBonusFeatures user_id, cb + async.series jobs, (err, results)-> + if err? + logger.err err:err, user_id:user_id, + "error getting subscription or group for refreshFeatures" + return callback(err) + + {individualFeatures, groupFeatureSets, v1Features, bonusFeatures} = results + logger.log {user_id, individualFeatures, groupFeatureSets, v1Features, bonusFeatures}, 'merging user features' + featureSets = groupFeatureSets.concat [individualFeatures, v1Features, bonusFeatures] + features = _.reduce(featureSets, FeaturesUpdater._mergeFeatures, Settings.defaultFeatures) + + logger.log {user_id, features}, 'updating user features' + UserFeaturesUpdater.updateFeatures user_id, features, callback + + _getIndividualFeatures: (user_id, callback = (error, features = {}) ->) -> + SubscriptionLocator.getUsersSubscription user_id, (err, sub)-> + callback err, FeaturesUpdater._subscriptionToFeatures(sub) + + _getGroupFeatureSets: (user_id, callback = (error, featureSets = []) ->) -> + SubscriptionLocator.getGroupSubscriptionsMemberOf user_id, (err, subs) -> + callback err, (subs or []).map FeaturesUpdater._subscriptionToFeatures + + _getV1Features: (user_id, callback = (error, features = {}) ->) -> + V1SubscriptionManager.getPlanCodeFromV1 user_id, (err, planCode) -> + callback err, FeaturesUpdater._planCodeToFeatures(planCode) + + _mergeFeatures: (featuresA, featuresB) -> + features = Object.assign({}, featuresA) + for key, value of featuresB + # Special merging logic for non-boolean features + if key == 'compileGroup' + if features['compileGroup'] == 'priority' or featuresB['compileGroup'] == 'priority' + features['compileGroup'] = 'priority' + else + features['compileGroup'] = 'standard' + else if key == 'collaborators' + if features['collaborators'] == -1 or featuresB['collaborators'] == -1 + features['collaborators'] = -1 + else + features['collaborators'] = Math.max( + features['collaborators'] or 0, + featuresB['collaborators'] or 0 + ) + else if key == 'compileTimeout' + features['compileTimeout'] = Math.max( + features['compileTimeout'] or 0, + featuresB['compileTimeout'] or 0 + ) + else + # Boolean keys, true is better + features[key] = features[key] or featuresB[key] + return features + + _subscriptionToFeatures: (subscription) -> + FeaturesUpdater._planCodeToFeatures(subscription?.planCode) + + _planCodeToFeatures: (planCode) -> + if !planCode? + return {} + plan = PlansLocator.findLocalPlanInSettings planCode + if !plan? + return {} + else + return plan.features \ No newline at end of file diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index f127f7ed35..32d2abf594 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -9,7 +9,7 @@ logger = require('logger-sharelatex') GeoIpLookup = require("../../infrastructure/GeoIpLookup") SubscriptionDomainHandler = require("./SubscriptionDomainHandler") UserGetter = require "../User/UserGetter" -SubscriptionUpdater = require './SubscriptionUpdater' +FeaturesUpdater = require './FeaturesUpdater' module.exports = SubscriptionController = @@ -241,6 +241,6 @@ module.exports = SubscriptionController = refreshUserFeatures: (req, res, next) -> {user_id} = req.params - SubscriptionUpdater.refreshFeatures user_id, (error) -> + FeaturesUpdater.refreshFeatures user_id, (error) -> return next(error) if error? res.sendStatus 200 \ No newline at end of file diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee index 8e583bd07b..cb5c39d122 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionUpdater.coffee @@ -2,27 +2,26 @@ async = require("async") _ = require("underscore") Subscription = require('../../models/Subscription').Subscription SubscriptionLocator = require("./SubscriptionLocator") -UserFeaturesUpdater = require("./UserFeaturesUpdater") PlansLocator = require("./PlansLocator") Settings = require("settings-sharelatex") logger = require("logger-sharelatex") -ObjectId = require('mongoose').Types.ObjectId -ReferalFeatures = require("../Referal/ReferalFeatures") -V1SubscriptionManager = require("./V1SubscriptionManager") +ObjectId = require('mongoose').Types.ObjectId +FeaturesUpdater = require('./FeaturesUpdater') oneMonthInSeconds = 60 * 60 * 24 * 30 module.exports = SubscriptionUpdater = - syncSubscription: (recurlySubscription, adminUser_id, callback) -> logger.log adminUser_id:adminUser_id, recurlySubscription:recurlySubscription, "syncSubscription, creating new if subscription does not exist" SubscriptionLocator.getUsersSubscription adminUser_id, (err, subscription)-> + return callback(err) if err? if subscription? logger.log adminUser_id:adminUser_id, recurlySubscription:recurlySubscription, "subscription does exist" SubscriptionUpdater._updateSubscriptionFromRecurly recurlySubscription, subscription, callback else logger.log adminUser_id:adminUser_id, recurlySubscription:recurlySubscription, "subscription does not exist, creating a new one" SubscriptionUpdater._createNewSubscription adminUser_id, (err, subscription)-> + return callback(err) if err? SubscriptionUpdater._updateSubscriptionFromRecurly recurlySubscription, subscription, callback addUserToGroup: (adminUser_id, user_id, callback)-> @@ -35,7 +34,7 @@ module.exports = SubscriptionUpdater = if err? logger.err err:err, searchOps:searchOps, insertOperation:insertOperation, "error findy and modify add user to group" return callback(err) - UserFeaturesUpdater.updateFeatures user_id, subscription.planCode, callback + FeaturesUpdater.refreshFeatures user_id, callback addEmailInviteToGroup: (adminUser_id, email, callback) -> logger.log {adminUser_id, email}, "adding email into mongo subscription" @@ -54,7 +53,7 @@ module.exports = SubscriptionUpdater = if err? logger.err err:err, searchOps:searchOps, removeOperation:removeOperation, "error removing user from group" return callback(err) - SubscriptionUpdater.refreshFeatures user_id, callback + FeaturesUpdater.refreshFeatures user_id, callback removeEmailInviteFromGroup: (adminUser_id, email, callback)-> Subscription.update { @@ -70,7 +69,7 @@ module.exports = SubscriptionUpdater = logger.log {subscription_id, affected_user_ids}, "deleting subscription and downgrading users" Subscription.remove {_id: ObjectId(subscription_id)}, (err) -> return callback(err) if err? - async.mapSeries affected_user_ids, SubscriptionUpdater.refreshFeatures, callback + async.mapSeries affected_user_ids, FeaturesUpdater.refreshFeatures, callback _createNewSubscription: (adminUser_id, callback)-> logger.log adminUser_id:adminUser_id, "creating new subscription" @@ -98,76 +97,5 @@ module.exports = SubscriptionUpdater = allIds = _.union subscription.member_ids, [subscription.admin_id] jobs = allIds.map (user_id)-> return (cb)-> - SubscriptionUpdater.refreshFeatures user_id, cb + FeaturesUpdater.refreshFeatures user_id, cb async.series jobs, callback - - refreshFeatures: (user_id, callback)-> - jobs = - individualFeatures: (cb) -> SubscriptionUpdater._getIndividualFeatures user_id, cb - groupFeatureSets: (cb) -> SubscriptionUpdater._getGroupFeatureSets user_id, cb - v1Features: (cb) -> SubscriptionUpdater._getV1Features user_id, cb - bonusFeatures: (cb) -> ReferalFeatures.getBonusFeatures user_id, cb - async.series jobs, (err, results)-> - if err? - logger.err err:err, user_id:user_id, - "error getting subscription or group for refreshFeatures" - return callback(err) - - {individualFeatures, groupFeatureSets, v1Features, bonusFeatures} = results - logger.log {user_id, individualFeatures, groupFeatureSets, v1Features, bonusFeatures}, 'merging user features' - featureSets = groupFeatureSets.concat [individualFeatures, v1Features, bonusFeatures] - features = _.reduce(featureSets, SubscriptionUpdater._mergeFeatures, Settings.defaultFeatures) - - logger.log {user_id, features}, 'updating user features' - UserFeaturesUpdater.updateFeatures user_id, features, callback - - _getIndividualFeatures: (user_id, callback = (error, features = {}) ->) -> - SubscriptionLocator.getUsersSubscription user_id, (err, sub)-> - callback err, SubscriptionUpdater._subscriptionToFeatures(sub) - - _getGroupFeatureSets: (user_id, callback = (error, featureSets = []) ->) -> - SubscriptionLocator.getGroupSubscriptionsMemberOf user_id, (err, subs) -> - callback err, (subs or []).map SubscriptionUpdater._subscriptionToFeatures - - _getV1Features: (user_id, callback = (error, features = {}) ->) -> - V1SubscriptionManager.getPlanCodeFromV1 user_id, (err, planCode) -> - callback err, SubscriptionUpdater._planCodeToFeatures(planCode) - - _mergeFeatures: (featuresA, featuresB) -> - features = Object.assign({}, featuresA) - for key, value of featuresB - # Special merging logic for non-boolean features - if key == 'compileGroup' - if features['compileGroup'] == 'priority' or featuresB['compileGroup'] == 'priority' - features['compileGroup'] = 'priority' - else - features['compileGroup'] = 'standard' - else if key == 'collaborators' - if features['collaborators'] == -1 or featuresB['collaborators'] == -1 - features['collaborators'] = -1 - else - features['collaborators'] = Math.max( - features['collaborators'] or 0, - featuresB['collaborators'] or 0 - ) - else if key == 'compileTimeout' - features['compileTimeout'] = Math.max( - features['compileTimeout'] or 0, - featuresB['compileTimeout'] or 0 - ) - else - # Boolean keys, true is better - features[key] = features[key] or featuresB[key] - return features - - _subscriptionToFeatures: (subscription) -> - SubscriptionUpdater._planCodeToFeatures(subscription?.planCode) - - _planCodeToFeatures: (planCode) -> - if !planCode? - return {} - plan = PlansLocator.findLocalPlanInSettings planCode - if !plan? - return {} - else - return plan.features \ No newline at end of file diff --git a/services/web/test/acceptance/config/settings.test.coffee b/services/web/test/acceptance/config/settings.test.coffee index 1bcb7426ff..3d665f8a54 100644 --- a/services/web/test/acceptance/config/settings.test.coffee +++ b/services/web/test/acceptance/config/settings.test.coffee @@ -1,10 +1,4 @@ module.exports = - apis: - v1: - url: "http://localhost:5000" - user: 'overleaf' - pass: 'password' - enableSubscriptions: true features: features = diff --git a/services/web/test/unit/coffee/Subscription/FeaturesUpdaterTests.coffee b/services/web/test/unit/coffee/Subscription/FeaturesUpdaterTests.coffee new file mode 100644 index 0000000000..c8f77bfd40 --- /dev/null +++ b/services/web/test/unit/coffee/Subscription/FeaturesUpdaterTests.coffee @@ -0,0 +1,173 @@ +SandboxedModule = require('sandboxed-module') +should = require('chai').should() +expect = require('chai').expect +sinon = require 'sinon' +modulePath = "../../../../app/js/Features/Subscription/FeaturesUpdater" +assert = require("chai").assert +ObjectId = require('mongoose').Types.ObjectId + +describe "FeaturesUpdater", -> + + beforeEach -> + @user_id = ObjectId().toString() + + @FeaturesUpdater = SandboxedModule.require modulePath, requires: + './UserFeaturesUpdater': @UserFeaturesUpdater = {} + './SubscriptionLocator': @SubscriptionLocator = {} + './PlansLocator': @PlansLocator = {} + "logger-sharelatex": log:-> + 'settings-sharelatex': @Settings = {} + "../Referal/ReferalFeatures" : @ReferalFeatures = {} + "./V1SubscriptionManager": @V1SubscriptionManager = {} + + describe "refreshFeatures", -> + beforeEach -> + @UserFeaturesUpdater.updateFeatures = sinon.stub().yields() + @FeaturesUpdater._getIndividualFeatures = sinon.stub().yields(null, { 'individual': 'features' }) + @FeaturesUpdater._getGroupFeatureSets = sinon.stub().yields(null, [{ 'group': 'features' }, { 'group': 'features2' }]) + @FeaturesUpdater._getV1Features = sinon.stub().yields(null, { 'v1': 'features' }) + @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 + + it "should get the group features", -> + @FeaturesUpdater._getGroupFeatureSets + .calledWith(@user_id) + .should.equal true + + it "should get the v1 features", -> + @FeaturesUpdater._getV1Features + .calledWith(@user_id) + .should.equal true + + it "should get the bonus features", -> + @ReferalFeatures.getBonusFeatures + .calledWith(@user_id) + .should.equal true + + it "should merge from the default features", -> + @FeaturesUpdater._mergeFeatures.calledWith(@Settings.defaultFeatures).should.equal true + + it "should merge the individual features", -> + @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'individual': '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 v1 features", -> + @FeaturesUpdater._mergeFeatures.calledWith(sinon.match.any, { 'v1': '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 + + describe "_mergeFeatures", -> + it "should prefer priority over standard for compileGroup", -> + expect(@FeaturesUpdater._mergeFeatures({ + compileGroup: 'priority' + }, { + compileGroup: 'standard' + })).to.deep.equal({ + compileGroup: 'priority' + }) + expect(@FeaturesUpdater._mergeFeatures({ + compileGroup: 'standard' + }, { + compileGroup: 'priority' + })).to.deep.equal({ + compileGroup: 'priority' + }) + expect(@FeaturesUpdater._mergeFeatures({ + compileGroup: 'priority' + }, { + compileGroup: 'priority' + })).to.deep.equal({ + compileGroup: 'priority' + }) + expect(@FeaturesUpdater._mergeFeatures({ + compileGroup: 'standard' + }, { + compileGroup: 'standard' + })).to.deep.equal({ + compileGroup: 'standard' + }) + + it "should prefer -1 over any other for collaborators", -> + expect(@FeaturesUpdater._mergeFeatures({ + collaborators: -1 + }, { + collaborators: 10 + })).to.deep.equal({ + collaborators: -1 + }) + expect(@FeaturesUpdater._mergeFeatures({ + collaborators: 10 + }, { + collaborators: -1 + })).to.deep.equal({ + collaborators: -1 + }) + expect(@FeaturesUpdater._mergeFeatures({ + collaborators: 4 + }, { + collaborators: 10 + })).to.deep.equal({ + collaborators: 10 + }) + + it "should prefer the higher of compileTimeout", -> + expect(@FeaturesUpdater._mergeFeatures({ + compileTimeout: 20 + }, { + compileTimeout: 10 + })).to.deep.equal({ + compileTimeout: 20 + }) + expect(@FeaturesUpdater._mergeFeatures({ + compileTimeout: 10 + }, { + compileTimeout: 20 + })).to.deep.equal({ + compileTimeout: 20 + }) + + it "should prefer the true over false for other keys", -> + expect(@FeaturesUpdater._mergeFeatures({ + github: true + }, { + github: false + })).to.deep.equal({ + github: true + }) + expect(@FeaturesUpdater._mergeFeatures({ + github: false + }, { + github: true + })).to.deep.equal({ + github: true + }) + expect(@FeaturesUpdater._mergeFeatures({ + github: true + }, { + github: true + })).to.deep.equal({ + github: true + }) + expect(@FeaturesUpdater._mergeFeatures({ + github: false + }, { + github: false + })).to.deep.equal({ + github: false + }) diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee index 47301ecf7c..04888e2dd7 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionControllerTests.coffee @@ -75,7 +75,7 @@ describe "SubscriptionController", -> "./SubscriptionDomainHandler":@SubscriptionDomainHandler "../User/UserGetter": @UserGetter "./RecurlyWrapper": @RecurlyWrapper = {} - "./SubscriptionUpdater": @SubscriptionUpdater = {} + "./FeaturesUpdater": @FeaturesUpdater = {} @res = new MockResponse() diff --git a/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee b/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee index 71ae6346ce..dd2afad56b 100644 --- a/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee +++ b/services/web/test/unit/coffee/Subscription/SubscriptionUpdaterTests.coffee @@ -67,9 +67,7 @@ describe "SubscriptionUpdater", -> './PlansLocator': @PlansLocator "logger-sharelatex": log:-> 'settings-sharelatex': @Settings - "../Referal/ReferalFeatures" : @ReferalFeatures - '../../infrastructure/Modules': @Modules - "./V1SubscriptionManager": @V1SubscriptionManager = {} + "./FeaturesUpdater": @FeaturesUpdater = {} describe "syncSubscription", -> @@ -100,7 +98,7 @@ describe "SubscriptionUpdater", -> describe "_updateSubscriptionFromRecurly", -> beforeEach -> - @SubscriptionUpdater.refreshFeatures = sinon.stub().callsArgWith(1) + @FeaturesUpdater.refreshFeatures = sinon.stub().callsArgWith(1) it "should update the subscription with token etc when not expired", (done)-> @SubscriptionUpdater._updateSubscriptionFromRecurly @recurlySubscription, @subscription, (err)=> @@ -111,7 +109,7 @@ describe "SubscriptionUpdater", -> assert.equal(@subscription.freeTrial.expiresAt, undefined) assert.equal(@subscription.freeTrial.planCode, undefined) @subscription.save.called.should.equal true - @SubscriptionUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true + @FeaturesUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true done() it "should remove the recurlySubscription_id when expired", (done)-> @@ -120,15 +118,15 @@ describe "SubscriptionUpdater", -> @SubscriptionUpdater._updateSubscriptionFromRecurly @recurlySubscription, @subscription, (err)=> assert.equal(@subscription.recurlySubscription_id, undefined) @subscription.save.called.should.equal true - @SubscriptionUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true + @FeaturesUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true done() it "should update all the users features", (done)-> @SubscriptionUpdater._updateSubscriptionFromRecurly @recurlySubscription, @subscription, (err)=> - @SubscriptionUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true - @SubscriptionUpdater.refreshFeatures.calledWith(@allUserIds[0]).should.equal true - @SubscriptionUpdater.refreshFeatures.calledWith(@allUserIds[1]).should.equal true - @SubscriptionUpdater.refreshFeatures.calledWith(@allUserIds[2]).should.equal true + @FeaturesUpdater.refreshFeatures.calledWith(@adminUser._id).should.equal true + @FeaturesUpdater.refreshFeatures.calledWith(@allUserIds[0]).should.equal true + @FeaturesUpdater.refreshFeatures.calledWith(@allUserIds[1]).should.equal true + @FeaturesUpdater.refreshFeatures.calledWith(@allUserIds[2]).should.equal true done() it "should set group to true and save how many members can be added to group", (done)-> @@ -155,6 +153,9 @@ describe "SubscriptionUpdater", -> done() describe "addUserToGroup", -> + beforeEach -> + @FeaturesUpdater.refreshFeatures = sinon.stub().callsArgWith(1) + it "should add the users id to the group as a set", (done)-> @SubscriptionUpdater.addUserToGroup @adminUser._id, @otherUserId, => searchOps = @@ -166,12 +167,12 @@ describe "SubscriptionUpdater", -> it "should update the users features", (done)-> @SubscriptionUpdater.addUserToGroup @adminUser._id, @otherUserId, => - @UserFeaturesUpdater.updateFeatures.calledWith(@otherUserId, @subscription.planCode).should.equal true + @FeaturesUpdater.refreshFeatures.calledWith(@otherUserId).should.equal true done() describe "removeUserFromGroup", -> beforeEach -> - @SubscriptionUpdater.refreshFeatures = sinon.stub().callsArgWith(1) + @FeaturesUpdater.refreshFeatures = sinon.stub().callsArgWith(1) it "should pull the users id from the group", (done)-> @SubscriptionUpdater.removeUserFromGroup @adminUser._id, @otherUserId, => @@ -184,160 +185,9 @@ describe "SubscriptionUpdater", -> it "should update the users features", (done)-> @SubscriptionUpdater.removeUserFromGroup @adminUser._id, @otherUserId, => - @SubscriptionUpdater.refreshFeatures.calledWith(@otherUserId).should.equal true + @FeaturesUpdater.refreshFeatures.calledWith(@otherUserId).should.equal true done() - describe "refreshFeatures", -> - beforeEach -> - @SubscriptionUpdater._getIndividualFeatures = sinon.stub().yields(null, { 'individual': 'features' }) - @SubscriptionUpdater._getGroupFeatureSets = sinon.stub().yields(null, [{ 'group': 'features' }, { 'group': 'features2' }]) - @SubscriptionUpdater._getV1Features = sinon.stub().yields(null, { 'v1': 'features' }) - @ReferalFeatures.getBonusFeatures = sinon.stub().yields(null, { 'bonus': 'features' }) - @SubscriptionUpdater._mergeFeatures = sinon.stub().returns({'merged': 'features'}) - @callback = sinon.stub() - @SubscriptionUpdater.refreshFeatures @user_id, @callback - - it "should get the individual features", -> - @SubscriptionUpdater._getIndividualFeatures - .calledWith(@user_id) - .should.equal true - - it "should get the group features", -> - @SubscriptionUpdater._getGroupFeatureSets - .calledWith(@user_id) - .should.equal true - - it "should get the v1 features", -> - @SubscriptionUpdater._getV1Features - .calledWith(@user_id) - .should.equal true - - it "should get the bonus features", -> - @ReferalFeatures.getBonusFeatures - .calledWith(@user_id) - .should.equal true - - it "should merge from the default features", -> - @SubscriptionUpdater._mergeFeatures.calledWith(@Settings.defaultFeatures).should.equal true - - it "should merge the individual features", -> - @SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'individual': 'features' }).should.equal true - - it "should merge the group features", -> - @SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'group': 'features' }).should.equal true - @SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'group': 'features2' }).should.equal true - - it "should merge the v1 features", -> - @SubscriptionUpdater._mergeFeatures.calledWith(sinon.match.any, { 'v1': 'features' }).should.equal true - - it "should merge the bonus features", -> - @SubscriptionUpdater._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 - - describe "_mergeFeatures", -> - it "should prefer priority over standard for compileGroup", -> - expect(@SubscriptionUpdater._mergeFeatures({ - compileGroup: 'priority' - }, { - compileGroup: 'standard' - })).to.deep.equal({ - compileGroup: 'priority' - }) - expect(@SubscriptionUpdater._mergeFeatures({ - compileGroup: 'standard' - }, { - compileGroup: 'priority' - })).to.deep.equal({ - compileGroup: 'priority' - }) - expect(@SubscriptionUpdater._mergeFeatures({ - compileGroup: 'priority' - }, { - compileGroup: 'priority' - })).to.deep.equal({ - compileGroup: 'priority' - }) - expect(@SubscriptionUpdater._mergeFeatures({ - compileGroup: 'standard' - }, { - compileGroup: 'standard' - })).to.deep.equal({ - compileGroup: 'standard' - }) - - it "should prefer -1 over any other for collaborators", -> - expect(@SubscriptionUpdater._mergeFeatures({ - collaborators: -1 - }, { - collaborators: 10 - })).to.deep.equal({ - collaborators: -1 - }) - expect(@SubscriptionUpdater._mergeFeatures({ - collaborators: 10 - }, { - collaborators: -1 - })).to.deep.equal({ - collaborators: -1 - }) - expect(@SubscriptionUpdater._mergeFeatures({ - collaborators: 4 - }, { - collaborators: 10 - })).to.deep.equal({ - collaborators: 10 - }) - - it "should prefer the higher of compileTimeout", -> - expect(@SubscriptionUpdater._mergeFeatures({ - compileTimeout: 20 - }, { - compileTimeout: 10 - })).to.deep.equal({ - compileTimeout: 20 - }) - expect(@SubscriptionUpdater._mergeFeatures({ - compileTimeout: 10 - }, { - compileTimeout: 20 - })).to.deep.equal({ - compileTimeout: 20 - }) - - it "should prefer the true over false for other keys", -> - expect(@SubscriptionUpdater._mergeFeatures({ - github: true - }, { - github: false - })).to.deep.equal({ - github: true - }) - expect(@SubscriptionUpdater._mergeFeatures({ - github: false - }, { - github: true - })).to.deep.equal({ - github: true - }) - expect(@SubscriptionUpdater._mergeFeatures({ - github: true - }, { - github: true - })).to.deep.equal({ - github: true - }) - expect(@SubscriptionUpdater._mergeFeatures({ - github: false - }, { - github: false - })).to.deep.equal({ - github: false - }) - describe "deleteSubscription", -> beforeEach (done) -> @subscription_id = ObjectId().toString() @@ -347,7 +197,7 @@ describe "SubscriptionUpdater", -> member_ids: [ ObjectId(), ObjectId(), ObjectId() ] } @SubscriptionLocator.getSubscription = sinon.stub().yields(null, @subscription) - @SubscriptionUpdater.refreshFeatures = sinon.stub().yields() + @FeaturesUpdater.refreshFeatures = sinon.stub().yields() @SubscriptionUpdater.deleteSubscription @subscription_id, done it "should look up the subscription", -> @@ -361,12 +211,12 @@ describe "SubscriptionUpdater", -> .should.equal true it "should downgrade the admin_id", -> - @SubscriptionUpdater.refreshFeatures + @FeaturesUpdater.refreshFeatures .calledWith(@subscription.admin_id) .should.equal true it "should downgrade all of the members", -> for user_id in @subscription.member_ids - @SubscriptionUpdater.refreshFeatures + @FeaturesUpdater.refreshFeatures .calledWith(user_id) .should.equal true From 1d0be569a6cd8db44a38047433ad8a60fd80686d Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 21 May 2018 17:50:49 +0100 Subject: [PATCH 33/34] Fix SubscriptionUpdater -> FeaturesUpdater miscall --- .../coffee/Features/Referal/ReferalAllocator.coffee | 4 ++-- .../unit/coffee/Referal/ReferalAllocatorTests.coffee | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee b/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee index 3344747968..53d3696807 100644 --- a/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee +++ b/services/web/app/coffee/Features/Referal/ReferalAllocator.coffee @@ -2,7 +2,7 @@ _ = require("underscore") logger = require('logger-sharelatex') User = require('../../models/User').User Settings = require "settings-sharelatex" -SubscriptionUpdater = require "../Subscription/SubscriptionUpdater" +FeaturesUpdater = require "../Subscription/FeaturesUpdater" module.exports = ReferalAllocator = allocate: (referal_id, new_user_id, referal_source, referal_medium, callback = ->)-> @@ -25,6 +25,6 @@ module.exports = ReferalAllocator = if err? logger.err err:err, referal_id:referal_id, new_user_id:new_user_id, "something went wrong allocating referal" return callback(err) - SubscriptionUpdater.refreshFeatures user._id, callback + FeaturesUpdater.refreshFeatures user._id, callback else callback() diff --git a/services/web/test/unit/coffee/Referal/ReferalAllocatorTests.coffee b/services/web/test/unit/coffee/Referal/ReferalAllocatorTests.coffee index ef0f0243c8..308c2ce80e 100644 --- a/services/web/test/unit/coffee/Referal/ReferalAllocatorTests.coffee +++ b/services/web/test/unit/coffee/Referal/ReferalAllocatorTests.coffee @@ -9,7 +9,7 @@ describe 'ReferalAllocator', -> beforeEach -> @ReferalAllocator = SandboxedModule.require modulePath, requires: '../../models/User': User: @User = {} - "../Subscription/SubscriptionUpdater": @SubscriptionUpdater = {} + "../Subscription/FeaturesUpdater": @FeaturesUpdater = {} "settings-sharelatex": @Settings = {} 'logger-sharelatex': log:-> @@ -26,7 +26,7 @@ describe 'ReferalAllocator', -> @referal_source = "bonus" @User.update = sinon.stub().callsArgWith 3, null @User.findOne = sinon.stub().callsArgWith 1, null, { _id: @user_id } - @SubscriptionUpdater.refreshFeatures = sinon.stub().yields() + @FeaturesUpdater.refreshFeatures = sinon.stub().yields() @ReferalAllocator.allocate @referal_id, @new_user_id, @referal_source, @referal_medium, @callback it 'should update the referring user with the refered users id', -> @@ -45,7 +45,7 @@ describe 'ReferalAllocator', -> .should.equal true it "should refresh the user's subscription", -> - @SubscriptionUpdater.refreshFeatures + @FeaturesUpdater.refreshFeatures .calledWith(@user_id) .should.equal true @@ -57,7 +57,7 @@ describe 'ReferalAllocator', -> @referal_source = "public_share" @User.update = sinon.stub().callsArgWith 3, null @User.findOne = sinon.stub().callsArgWith 1, null, { _id: @user_id } - @SubscriptionUpdater.refreshFeatures = sinon.stub().yields() + @FeaturesUpdater.refreshFeatures = sinon.stub().yields() @ReferalAllocator.allocate @referal_id, @new_user_id, @referal_source, @referal_medium, @callback it 'should not update the referring user with the refered users id', -> @@ -69,7 +69,7 @@ describe 'ReferalAllocator', -> .should.equal true it "should not assign the user a bonus", -> - @SubscriptionUpdater.refreshFeatures.called.should.equal false + @FeaturesUpdater.refreshFeatures.called.should.equal false it "should call the callback", -> @callback.called.should.equal true From 701a803da73eb4c72382f72780ae1f603e2b267d Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 22 May 2018 10:19:47 +0100 Subject: [PATCH 34/34] Fix ProjectInvite tests since the user no longer had the features --- .../web/test/acceptance/coffee/ProjectInviteTests.coffee | 7 ++++--- services/web/test/acceptance/coffee/helpers/User.coffee | 6 ++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/services/web/test/acceptance/coffee/ProjectInviteTests.coffee b/services/web/test/acceptance/coffee/ProjectInviteTests.coffee index b599578738..02598b320f 100644 --- a/services/web/test/acceptance/coffee/ProjectInviteTests.coffee +++ b/services/web/test/acceptance/coffee/ProjectInviteTests.coffee @@ -16,6 +16,7 @@ createInvite = (sendingUser, projectId, email, callback=(err, invite)->) -> privileges: 'readAndWrite' }, (err, response, body) -> return callback(err) if err + expect(response.statusCode).to.equal 200 callback(null, body.invite) createProject = (owner, projectName, callback=(err, projectId, project)->) -> @@ -207,9 +208,9 @@ describe "ProjectInviteTests", -> @email = 'smoketestuser@example.com' @projectName = 'sharing test' Async.series [ - (cb) => @user.login cb - (cb) => @user.logout cb + (cb) => @user.ensureUserExists cb (cb) => @sendingUser.login cb + (cb) => @sendingUser.setFeatures { collaborators: 10 }, cb ], done describe 'creating invites', -> @@ -266,7 +267,7 @@ describe "ProjectInviteTests", -> (cb) => expectInvitesInJoinProjectCount @sendingUser, @projectId, 0, cb ], done - it 'should allow the project owner to many invites at once', (done) -> + it 'should allow the project owner to create many invites at once', (done) -> @inviteOne = null @inviteTwo = null Async.series [ diff --git a/services/web/test/acceptance/coffee/helpers/User.coffee b/services/web/test/acceptance/coffee/helpers/User.coffee index 7d8e9086d4..a02ab5c42c 100644 --- a/services/web/test/acceptance/coffee/helpers/User.coffee +++ b/services/web/test/acceptance/coffee/helpers/User.coffee @@ -40,6 +40,12 @@ class User @referal_id = user?.referal_id callback(null, @password) + setFeatures: (features, callback = (error) ->) -> + update = {} + for key, value of features + update["features.#{key}"] = value + UserModel.update { _id: @id }, update, callback + logout: (callback = (error) ->) -> @getCsrfToken (error) => return callback(error) if error?