From d722f47b0f62e725596f7a28df31dddcb66af32f Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Wed, 8 Mar 2017 17:51:35 +0000 Subject: [PATCH 01/43] add indentify option and uuid for users not logged in --- .../coffee/Features/Analytics/AnalyticsController.coffee | 4 +++- .../app/coffee/Features/Analytics/AnalyticsManager.coffee | 8 ++++++++ .../coffee/Features/StaticPages/StaticPagesRouter.coffee | 2 ++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee b/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee index f9431a0f5c..a4267e28df 100644 --- a/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee +++ b/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee @@ -1,7 +1,9 @@ AnalyticsManager = require "./AnalyticsManager" +AuthenticationController = require("../Authentication/AuthenticationController") module.exports = AnalyticsController = recordEvent: (req, res, next) -> - AnalyticsManager.recordEvent req.session?.user?._id, req.params.event, req.body, (error) -> + user_id = AuthenticationController.getLoggedInUserId(req) or req.sessionID + AnalyticsManager.recordEvent user_id, req.params.event, req.body, (error) -> return next(error) if error? res.send 204 diff --git a/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee b/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee index 9f34d6d9d1..9165678070 100644 --- a/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee +++ b/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee @@ -16,6 +16,14 @@ makeRequest = (opts, callback)-> module.exports = + idendifyUser: (user_id, old_user_id, callback)-> + opts = + body: + old_user_id:old_user_id + json:true + method:"POST" + timeout:1000 + url: "/user/#{user_id}/idendify" recordEvent: (user_id, event, segmentation = {}, callback = (error) ->) -> if user_id+"" == settings.smokeTest?.userId+"" diff --git a/services/web/app/coffee/Features/StaticPages/StaticPagesRouter.coffee b/services/web/app/coffee/Features/StaticPages/StaticPagesRouter.coffee index f1f55814c7..66c210491b 100644 --- a/services/web/app/coffee/Features/StaticPages/StaticPagesRouter.coffee +++ b/services/web/app/coffee/Features/StaticPages/StaticPagesRouter.coffee @@ -9,6 +9,8 @@ module.exports = webRouter.get '/tos', HomeController.externalPage("tos", "Terms of Service") webRouter.get '/about', HomeController.externalPage("about", "About Us") + webRouter.get '/review', HomeController.externalPage("review", "About Us") + webRouter.get '/security', HomeController.externalPage("security", "Security") webRouter.get '/privacy_policy', HomeController.externalPage("privacy", "Privacy Policy") webRouter.get '/planned_maintenance', HomeController.externalPage("planned_maintenance", "Planned Maintenance") From 6c1a15a698dd6202eab4cdddf0120380bb7cb652 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 16 Mar 2017 15:48:57 +0000 Subject: [PATCH 02/43] Move comments when cutting and pasting --- .../track-changes/TrackChangesManager.coffee | 52 +++++++++++++++++++ .../ide/review-panel/RangesTracker.coffee | 48 +++++++++++++---- 2 files changed, 89 insertions(+), 11 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee index f51d1ed41e..b2257099b4 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee @@ -73,16 +73,24 @@ define [ _scrollTimeout = null , 200 + @_resetCutState() + onCut = () => @onCut() + onPaste = () => @onPaste() + bindToAce = () => @editor.on "changeSelection", onChangeSelection @editor.on "change", onChangeSelection # Selection also moves with updates elsewhere in the document @editor.on "changeSession", onChangeSession + @editor.on "cut", onCut + @editor.on "paste", onPaste @editor.renderer.on "resize", onResize unbindFromAce = () => @editor.off "changeSelection", onChangeSelection @editor.off "change", onChangeSelection @editor.off "changeSession", onChangeSession + @editor.off "cut", onCut + @editor.off "paste", onPaste @editor.renderer.off "resize", onResize @$scope.$watch "trackChangesEnabled", (enabled) => @@ -244,6 +252,50 @@ define [ @_onCommentAdded(comment) @broadcastChange() + _resetCutState: () -> + @_cutState = { + text: null + comments: [] + docId: null + } + + onCut: () -> + @_resetCutState() + selection = @editor.getSelectionRange() + selection_start = @_aceRangeToShareJs(selection.start) + selection_end = @_aceRangeToShareJs(selection.end) + @_cutState.text = @editor.getSelectedText() + @_cutState.docId = @$scope.docId + for comment in @rangesTracker.comments + comment_start = comment.op.p + comment_end = comment_start + comment.op.c.length + if selection_start <= comment_start and comment_end <= selection_end + @_cutState.comments.push { + offset: comment.op.p - selection_start + text: comment.op.c + comment: comment + } + + onPaste: () => + @editor.once "change", (change) => + return if change.action != "insert" + pasted_text = change.lines.join("\n") + paste_offset = @_aceRangeToShareJs(change.start) + console.log "PASTE", pasted_text, paste_offset + # We have to wait until the change has been processed by the range tracker, + # since if we move the ops into place beforehand, they will be moved again + # when the changes are processed by the range tracker. This ranges:dirty + # event is fired after the doc has applied the changes to the range tracker. + @$scope.sharejsDoc.on "ranges:dirty.paste", () => + @$scope.sharejsDoc.off "ranges:dirty.paste" # Doc event emitter uses namespaced events + if pasted_text == @_cutState.text and @$scope.docId == @_cutState.docId + for {comment, offset, text} in @_cutState.comments + op = { c: text, p: paste_offset + offset, t: comment.id } + @$scope.sharejsDoc.submitOp op # Resubmitting an existing comment op (by thread id) will move it + @_resetCutState() + # Check that comments still match text. Will throw error if not. + @rangesTracker.validate(@editor.getValue()) + checkMapping: () -> # TODO: reintroduce this check session = @editor.getSession() diff --git a/services/web/public/coffee/ide/review-panel/RangesTracker.coffee b/services/web/public/coffee/ide/review-panel/RangesTracker.coffee index a9c43e9816..14193f628d 100644 --- a/services/web/public/coffee/ide/review-panel/RangesTracker.coffee +++ b/services/web/public/coffee/ide/review-panel/RangesTracker.coffee @@ -1,3 +1,6 @@ +# This file is shared between document-updater and web, so that the server and client share +# an identical track changes implementation. Do not edit it directly in web or document-updater, +# instead edit it at https://github.com/sharelatex/ranges-tracker, where it has a suite of tests load = () -> class RangesTracker # The purpose of this class is to track a set of inserts and deletes to a document, like @@ -78,6 +81,13 @@ load = () -> @comments = @comments.filter (c) -> c.id != comment_id @_markAsDirty comment, "comment", "removed" + moveCommentId: (comment_id, position, text) -> + for comment in @comments + if comment.id == comment_id + comment.op.p = position + comment.op.c = text + @_markAsDirty comment, "comment", "moved" + getChange: (change_id) -> change = null for c in @changes @@ -90,6 +100,18 @@ load = () -> change = @getChange(change_id) return if !change? @_removeChange(change) + + validate: (text) -> + for change in @changes + if change.op.i? + content = text.slice(change.op.p, change.op.p + change.op.i.length) + if content != change.op.i + throw new Error("Change (#{JSON.stringify(change)}) doesn't match text (#{JSON.stringify(content)})") + for comment in @comments + content = text.slice(comment.op.p, comment.op.p + comment.op.c.length) + if content != comment.op.c + throw new Error("Comment (#{JSON.stringify(comment)}) doesn't match text (#{JSON.stringify(content)})") + return true applyOp: (op, metadata = {}) -> metadata.ts ?= new Date() @@ -110,17 +132,21 @@ load = () -> @applyOp(op, metadata) addComment: (op, metadata) -> - # TODO: Don't allow overlapping comments? - @comments.push comment = { - id: op.t or @newId() - op: # Copy because we'll modify in place - c: op.c - p: op.p - t: op.t - metadata - } - @_markAsDirty comment, "comment", "added" - return comment + existing = @getComment(op.t) + if existing? + @moveCommentId(op.t, op.p, op.c) + return existing + else + @comments.push comment = { + id: op.t or @newId() + op: # Copy because we'll modify in place + c: op.c + p: op.p + t: op.t + metadata + } + @_markAsDirty comment, "comment", "added" + return comment applyInsertToComments: (op) -> for comment in @comments From aa36768d8ad014869fddf84e569694c7623b866c Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 16 Mar 2017 16:01:03 +0000 Subject: [PATCH 03/43] Remove debugging line --- .../aceEditor/track-changes/TrackChangesManager.coffee | 1 - 1 file changed, 1 deletion(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee index b2257099b4..0dd6a9f06b 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee @@ -281,7 +281,6 @@ define [ return if change.action != "insert" pasted_text = change.lines.join("\n") paste_offset = @_aceRangeToShareJs(change.start) - console.log "PASTE", pasted_text, paste_offset # We have to wait until the change has been processed by the range tracker, # since if we move the ops into place beforehand, they will be moved again # when the changes are processed by the range tracker. This ranges:dirty From a4d6d5c53f3a3eaebc709e64ba600f3141efe380 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 16 Mar 2017 17:33:01 +0000 Subject: [PATCH 04/43] Add opacity to comment and insert ranges so they can be seen overlapping --- .../public/stylesheets/app/editor/review-panel.less | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index 7d2dbb1c9f..5cf9117587 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -9,13 +9,10 @@ @rp-border-grey : #d9d9d9; @rp-green : #2c8e30; -@rp-dim-green : #cae3cb; @rp-green-on-dark : rgba(37, 107, 41, 0.5); @rp-red : #c5060b; -@rp-dim-red : #f3cdce; @rp-yellow : #f3b111; @rp-yellow-on-dark : rgba(194, 93, 11, 0.5); -@rp-dim-yellow : #ffe9b2; @rp-grey : #aaaaaa; @rp-type-blue : #6b7797; @@ -748,13 +745,16 @@ .rp-loading-threads & { display: none; } + z-index: 6 // Appear above text selection } .track-changes-comment-marker { - background-color: @rp-dim-yellow; + background-color: @rp-yellow; + opacity: 0.3; } .track-changes-added-marker { - background-color: @rp-dim-green; + background-color: @rp-green; + opacity: 0.3; } .track-changes-deleted-marker { border-left: 2px dotted @rp-red; From 9a8ee112a5882cbf55077fe58383e3f9704ad3b3 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Fri, 17 Mar 2017 10:35:07 +0000 Subject: [PATCH 05/43] null check path.split https://sentry.io/sharelatex-1/sl-web-client-prod/issues/202702369/ --- services/web/public/coffee/ide/file-tree/FileTreeManager.coffee | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/web/public/coffee/ide/file-tree/FileTreeManager.coffee b/services/web/public/coffee/ide/file-tree/FileTreeManager.coffee index c4ad4b30a4..dd6f813430 100644 --- a/services/web/public/coffee/ide/file-tree/FileTreeManager.coffee +++ b/services/web/public/coffee/ide/file-tree/FileTreeManager.coffee @@ -173,6 +173,8 @@ define [ @_findEntityByPathInFolder @$scope.rootFolder, path _findEntityByPathInFolder: (folder, path) -> + if !path? or !folder? + return null parts = path.split("/") name = parts.shift() rest = parts.join("/") From d453a4d5c7ad814bc08c19e42869feae1b83d070 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Fri, 17 Mar 2017 13:03:16 +0000 Subject: [PATCH 06/43] null check stat when we check file on disk https://sentry.io/sharelatex-1/sl-web-server-prod/issues/125814174/ --- .../app/coffee/Features/FileStore/FileStoreHandler.coffee | 3 +++ .../coffee/FileStore/FileStoreHandlerTests.coffee | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee b/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee index eb5c2cd03f..10545cca76 100644 --- a/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee +++ b/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee @@ -13,6 +13,9 @@ module.exports = FileStoreHandler = if err? logger.err err:err, project_id:project_id, file_id:file_id, fsPath:fsPath, "error stating file" callback(err) + if !stat? + logger.err project_id:project_id, file_id:file_id, fsPath:fsPath, "stat is not available, can not check file from disk" + return callback(new Error("error getting stat, not available")) if !stat.isFile() logger.log project_id:project_id, file_id:file_id, fsPath:fsPath, "tried to upload symlink, not contining" return callback(new Error("can not upload symlink")) diff --git a/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee b/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee index 7452c6fb79..01990787e1 100644 --- a/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee @@ -96,6 +96,13 @@ describe "FileStoreHandler", -> @fs.createReadStream.called.should.equal false done() + describe "symlink", -> + it "should not read file stat returns nothing", (done)-> + @fs.lstat = sinon.stub().callsArgWith(1, null, null) + @handler.uploadFileFromDisk @project_id, @file_id, @fsPath, => + @fs.createReadStream.called.should.equal false + done() + describe "when upload fails", -> beforeEach -> @writeStream.on = (type, cb) -> From e5468983cebfee8bc8211821a11d217f5f63ea8d Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Fri, 17 Mar 2017 13:21:30 +0000 Subject: [PATCH 07/43] clone project plow though null doc/file/folders https://sentry.io/sharelatex-1/sl-web-server-prod/issues/227107799/ --- .../Features/Project/ProjectDuplicator.coffee | 17 ++++++++++++----- .../Project/ProjectDuplicatorTests.coffee | 8 ++++---- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectDuplicator.coffee b/services/web/app/coffee/Features/Project/ProjectDuplicator.coffee index 6cae962b04..d945326395 100644 --- a/services/web/app/coffee/Features/Project/ProjectDuplicator.coffee +++ b/services/web/app/coffee/Features/Project/ProjectDuplicator.coffee @@ -15,9 +15,11 @@ module.exports = ProjectDuplicator = _copyDocs: (newProject, originalRootDoc, originalFolder, desFolder, docContents, callback)-> setRootDoc = _.once (doc_id)-> projectEntityHandler.setRootDoc newProject._id, doc_id - - jobs = originalFolder.docs.map (doc)-> + docs = originalFolder.docs or [] + jobs = docs.map (doc)-> return (cb)-> + if !doc?._id? + return callback() content = docContents[doc._id.toString()] projectEntityHandler.addDocWithProject newProject, desFolder._id, doc.name, content.lines, (err, newDoc)-> if err? @@ -30,7 +32,8 @@ module.exports = ProjectDuplicator = async.series jobs, callback _copyFiles: (newProject, originalProject_id, originalFolder, desFolder, callback)-> - jobs = originalFolder.fileRefs.map (file)-> + fileRefs = originalFolder.fileRefs or [] + jobs = fileRefs.map (file)-> return (cb)-> projectEntityHandler.copyFileFromExistingProjectWithProject newProject, desFolder._id, originalProject_id, file, cb async.parallelLimit jobs, 5, callback @@ -40,10 +43,14 @@ module.exports = ProjectDuplicator = ProjectGetter.getProject newProject_id, {rootFolder:true, name:true}, (err, newProject)-> if err? logger.err project_id:newProject_id, "could not get project" - return cb(err) + return callback(err) - jobs = originalFolder.folders.map (childFolder)-> + folders = originalFolder.folders or [] + + jobs = folders.map (childFolder)-> return (cb)-> + if !childFolder?._id? + return cb() projectEntityHandler.addFolderWithProject newProject, desFolder?._id, childFolder.name, (err, newFolder)-> return cb(err) if err? ProjectDuplicator._copyFolderRecursivly newProject_id, originalProject_id, originalRootDoc, childFolder, newFolder, docContents, cb diff --git a/services/web/test/UnitTests/coffee/Project/ProjectDuplicatorTests.coffee b/services/web/test/UnitTests/coffee/Project/ProjectDuplicatorTests.coffee index 7897ae47c8..a1101cfbe0 100644 --- a/services/web/test/UnitTests/coffee/Project/ProjectDuplicatorTests.coffee +++ b/services/web/test/UnitTests/coffee/Project/ProjectDuplicatorTests.coffee @@ -9,7 +9,7 @@ describe 'ProjectDuplicator', -> @level2folder = name: "level2folderName" _id:"level2folderId" - docs:[@doc2 = {_id: "doc2_id", name:"level2folderDocName"}] + docs:[@doc2 = {_id: "doc2_id", name:"level2folderDocName"}, undefined] folders:[] fileRefs:[{name:"file2", _id:"file2"}] @level1folder = @@ -17,12 +17,12 @@ describe 'ProjectDuplicator', -> _id:"level1folderId" docs:[@doc1 = {_id: "doc1_id", name:"level1folderDocName"}] folders:[@level2folder] - fileRefs:[{name:"file1", _id:"file1"}] + fileRefs:[{name:"file1", _id:"file1"}, null] @rootFolder = name:"rootFolder" _id:"rootFolderId" docs:[@doc0 = {_id: "doc0_id", name:"rootDocHere"}] - folders:[@level1folder] + folders:[@level1folder, {}] fileRefs:[{name:"file0", _id:"file0"}] @project = _id: @old_project_id = "this_is_the_old_project_id" @@ -117,7 +117,7 @@ describe 'ProjectDuplicator', -> @projectOptionsHandler.setCompiler.calledWith(@stubbedNewProject._id, @project.compiler).should.equal true done() - it 'should use the same root docccccccc', (done)-> + it 'should use the same root doc', (done)-> @entityHandler.addDocWithProject.callsArgWith(4, null, @rootFolder.docs[0]) @duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=> @entityHandler.setRootDoc.calledWith(@stubbedNewProject._id, @rootFolder.docs[0]._id).should.equal true From 31b1c53faab029e14aa9f411b0fec6c37de3b58e Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Fri, 17 Mar 2017 13:24:50 +0000 Subject: [PATCH 08/43] proxy jpg to blog backend https://sentry.io/sharelatex-1/sl-web-server-prod/issues/212236471/ --- services/web/app/coffee/Features/Blog/BlogController.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Blog/BlogController.coffee b/services/web/app/coffee/Features/Blog/BlogController.coffee index 0f0d9383c7..966116899f 100644 --- a/services/web/app/coffee/Features/Blog/BlogController.coffee +++ b/services/web/app/coffee/Features/Blog/BlogController.coffee @@ -10,7 +10,7 @@ module.exports = BlogController = url = req.url?.toLowerCase() blogUrl = "#{settings.apis.blog.url}#{url}" - extensionsToProxy = [".png", ".xml", ".jpeg", ".json", ".zip", ".eps", ".gif"] + extensionsToProxy = [".png", ".xml", ".jpeg", ".jpeg", ".jpg", ".zip", ".eps", ".gif"] shouldProxy = _.find extensionsToProxy, (extension)-> url.indexOf(extension) != -1 From 8ee2e5ba638334bd4570fe333531a85a52954293 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Fri, 17 Mar 2017 13:29:09 +0000 Subject: [PATCH 09/43] null check folder in findElement https://sentry.io/sharelatex-1/sl-web-server-prod/issues/236000085/ --- services/web/app/coffee/Features/Project/ProjectLocator.coffee | 2 ++ .../test/UnitTests/coffee/Project/ProjectLocatorTests.coffee | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Project/ProjectLocator.coffee b/services/web/app/coffee/Features/Project/ProjectLocator.coffee index 44f68123d6..62b495e5d2 100644 --- a/services/web/app/coffee/Features/Project/ProjectLocator.coffee +++ b/services/web/app/coffee/Features/Project/ProjectLocator.coffee @@ -26,6 +26,8 @@ module.exports = ProjectLocator = element = _.find searchFolder[elementType], (el)-> el?._id+'' == element_id+'' #need to ToString both id's for robustness if !element? && searchFolder.folders? && searchFolder.folders.length != 0 _.each searchFolder.folders, (folder, index)-> + if !folder? + return newPath = {} newPath[key] = value for own key,value of path #make a value copy of the string newPath.fileSystem += "/#{folder.name}" diff --git a/services/web/test/UnitTests/coffee/Project/ProjectLocatorTests.coffee b/services/web/test/UnitTests/coffee/Project/ProjectLocatorTests.coffee index 1d982d90af..9257c2e83e 100644 --- a/services/web/test/UnitTests/coffee/Project/ProjectLocatorTests.coffee +++ b/services/web/test/UnitTests/coffee/Project/ProjectLocatorTests.coffee @@ -17,7 +17,7 @@ file1 = name:"file1", _id:"dsa9lkdsad" subSubFile = name:"subSubFile", _id:"d1d2dk" subSubDoc = name:"subdoc.txt", _id:"321dmdwi" secondSubFolder = name:"secondSubFolder", _id:"dsa3e23", docs:[subSubDoc], fileRefs:[subSubFile], folders:[] -subFolder = name:"subFolder", _id:"dsadsa93", folders:[secondSubFolder], docs:[], fileRefs:[] +subFolder = name:"subFolder", _id:"dsadsa93", folders:[secondSubFolder, null], docs:[], fileRefs:[] subFolder1 = name:"subFolder1", _id:"123asdjoij" rootFolder = From 625fa810c25a28fdf9aee9068ae1307bb012177e Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Fri, 17 Mar 2017 14:42:07 +0000 Subject: [PATCH 10/43] validate mongo id in getPrivilegeLevelForProject https://sentry.io/sharelatex-1/sl-web-server-prod/issues/204397665/ --- .../Authorization/AuthorizationManager.coffee | 4 ++++ .../AuthorizationManagerTests.coffee | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Authorization/AuthorizationManager.coffee b/services/web/app/coffee/Features/Authorization/AuthorizationManager.coffee index ded0b6f979..90c8cdb485 100644 --- a/services/web/app/coffee/Features/Authorization/AuthorizationManager.coffee +++ b/services/web/app/coffee/Features/Authorization/AuthorizationManager.coffee @@ -4,6 +4,8 @@ User = require("../../models/User").User PrivilegeLevels = require("./PrivilegeLevels") PublicAccessLevels = require("./PublicAccessLevels") Errors = require("../Errors/Errors") +ObjectId = require("mongojs").ObjectId + module.exports = AuthorizationManager = # Get the privilege level that the user has for the project @@ -13,6 +15,8 @@ module.exports = AuthorizationManager = # * becausePublic: true if the access level is only because the project is public. getPrivilegeLevelForProject: (user_id, project_id, callback = (error, privilegeLevel, becausePublic) ->) -> getPublicAccessLevel = () -> + if !ObjectId.isValid(project_id) + return callback(new Error("invalid project id")) Project.findOne { _id: project_id }, { publicAccesLevel: 1 }, (error, project) -> return callback(error) if error? if !project? diff --git a/services/web/test/UnitTests/coffee/Authorization/AuthorizationManagerTests.coffee b/services/web/test/UnitTests/coffee/Authorization/AuthorizationManagerTests.coffee index fcacce5164..b85449d7fd 100644 --- a/services/web/test/UnitTests/coffee/Authorization/AuthorizationManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authorization/AuthorizationManagerTests.coffee @@ -136,7 +136,20 @@ describe "AuthorizationManager", -> it "should return a NotFoundError", -> @AuthorizationManager.getPrivilegeLevelForProject @user_id, @project_id, (error) -> error.should.be.instanceof Errors.NotFoundError - + + describe "when the project id is not validssssssss", -> + beforeEach -> + @AuthorizationManager.isUserSiteAdmin.withArgs(@user_id).yields(null, false) + @CollaboratorsHandler.getMemberIdPrivilegeLevel + .withArgs(@user_id, @project_id) + .yields(null, "readOnly") + + it "should return a error", (done)-> + @AuthorizationManager.getPrivilegeLevelForProject undefined, "not project id", (err) => + @Project.findOne.called.should.equal false + expect(err).to.exist + done() + describe "canUserReadProject", -> beforeEach -> @AuthorizationManager.getPrivilegeLevelForProject = sinon.stub() From 3c2f5525a14f213829adba41c6d7f1a80f1fbbe5 Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 17 Mar 2017 14:49:32 +0000 Subject: [PATCH 11/43] Show an explanation error message on too many track changes error --- .../public/coffee/ide/editor/EditorManager.coffee | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/services/web/public/coffee/ide/editor/EditorManager.coffee b/services/web/public/coffee/ide/editor/EditorManager.coffee index e01a12cb8b..a1d00d3180 100644 --- a/services/web/public/coffee/ide/editor/EditorManager.coffee +++ b/services/web/public/coffee/ide/editor/EditorManager.coffee @@ -121,11 +121,22 @@ define [ _bindToDocumentEvents: (doc, sharejs_doc) -> sharejs_doc.on "error", (error, meta) => - if error?.message?.match "maxDocLength" + if error?.message? + message = error.message + else if typeof error == "string" + message = error + else + message = "" + if message.match "maxDocLength" @ide.showGenericMessageModal( "Document Too Long" "Sorry, this file is too long to be edited manually. Please upload it directly." ) + else if message.match "too many comments or tracked changes" + @ide.showGenericMessageModal( + "Too many comments or tracked changes" + "Sorry, this file has too many comments or tracked changes. Please try accepting or rejecting some existing changes, or resolving and deleting some comments." + ) else @ide.socket.disconnect() @ide.reportError(error, meta) From c74449b2e3cce3b4ae6814191213ea04b2bb99ca Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Fri, 17 Mar 2017 16:28:21 +0000 Subject: [PATCH 12/43] Don't show the mini review panel when the only visible entry is the add comment one. --- .../review-panel/controllers/ReviewPanelController.coffee | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 5067799571..6c6354a773 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -163,7 +163,10 @@ define [ $scope.$watch (() -> entries = $scope.reviewPanel.entries[$scope.editor.open_doc_id] or {} - Object.keys(entries).length + permEntries = {} + for entry, entryData of entries + permEntries[entry] = entryData if entry != "add-comment" + Object.keys(permEntries).length ), (nEntries) -> $scope.reviewPanel.hasEntries = nEntries > 0 and $scope.project.features.trackChangesVisible From beac43741cf3bead3bfed015f458ceab8b58d936 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Fri, 17 Mar 2017 17:05:38 +0000 Subject: [PATCH 13/43] New add comment button. --- .../app/views/project/editor/review-panel.pug | 10 ++- .../controllers/ReviewPanelController.coffee | 1 + .../stylesheets/app/editor/review-panel.less | 73 +++++++++++-------- 3 files changed, 52 insertions(+), 32 deletions(-) diff --git a/services/web/app/views/project/editor/review-panel.pug b/services/web/app/views/project/editor/review-panel.pug index b9dea40207..f4606be9a3 100644 --- a/services/web/app/views/project/editor/review-panel.pug +++ b/services/web/app/views/project/editor/review-panel.pug @@ -1,10 +1,18 @@ -#review-panel +.rp-in-editor-widgets a.rp-track-changes-indicator( href ng-if="editor.wantTrackChanges" ng-click="toggleReviewPanel();" ng-class="{ 'rp-track-changes-indicator-on-dark' : darkTheme }" ) !{translate("track_changes_is_on")} + a.rp-add-comment-btn( + href + ng-if="reviewPanel.entries[editor.open_doc_id]['add-comment'] != null" + ng-click="startNewComment();" + ) + i.fa.fa-comment + |  #{translate("add_comment")} +#review-panel .review-panel-toolbar resolved-comments-dropdown( class="rp-flex-block" diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 6c6354a773..04a32b2626 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -15,6 +15,7 @@ define [ entries: {} resolvedComments: {} hasEntries: false + showAddComment: false subView: $scope.SubViews.CUR_FILE openSubView: $scope.SubViews.CUR_FILE overview: diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index 7d2dbb1c9f..5bfd6275f9 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -448,6 +448,11 @@ display: block; padding: 5px 10px; border-radius: 3px; + + .rp-in-editor-widgets & { + border-radius: 0; + border-bottom-left-radius: 3px; + } } .rp-new-comment { @@ -871,43 +876,49 @@ } } -.rp-track-changes-indicator { - display: none; +.rp-in-editor-widgets { position: absolute; top: 0; - right: @review-off-width; - padding: 5px 10px; - background-color: rgba(240, 240, 240, 0.9); - color: @rp-type-blue; - text-align: center; - border-bottom-left-radius: 3px; + right: 0; font-size: 10px; - z-index: 2; - white-space: nowrap; - - &.rp-track-changes-indicator-on-dark { - background-color: rgba(88, 88, 88, .8); - color: #FFF; - - &:hover, - &:focus { - background-color: rgba(88, 88, 88, 1); - color: #FFF; - } - } - - &:hover, - &:focus { - outline: 0; - text-decoration: none; - background-color: rgba(240, 240, 240, 1); - color: @rp-type-blue; - } - + .rp-size-mini & { - display: block; + right: @review-off-width; } } + .rp-track-changes-indicator { + display: block; + padding: 5px 10px; + background-color: rgba(240, 240, 240, 0.9); + color: @rp-type-blue; + text-align: center; + border-bottom-left-radius: 3px; + z-index: 2; + white-space: nowrap; + + &.rp-track-changes-indicator-on-dark { + background-color: rgba(88, 88, 88, .8); + color: #FFF; + + &:hover, + &:focus { + background-color: rgba(88, 88, 88, 1); + color: #FFF; + } + } + + &:hover, + &:focus { + outline: 0; + text-decoration: none; + background-color: rgba(240, 240, 240, 1); + color: @rp-type-blue; + } + + .rp-size-expanded & { + display: none; + } + } // Helper class for elements which aren't treated as flex-items by IE10, e.g: // * inline items; From ca3849c0c24bb88fc413bec8c9e3731e57bb9efa Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Mon, 20 Mar 2017 11:08:42 +0000 Subject: [PATCH 14/43] jpeg -> json --- services/web/app/coffee/Features/Blog/BlogController.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/Blog/BlogController.coffee b/services/web/app/coffee/Features/Blog/BlogController.coffee index 966116899f..eb2b3fad94 100644 --- a/services/web/app/coffee/Features/Blog/BlogController.coffee +++ b/services/web/app/coffee/Features/Blog/BlogController.coffee @@ -10,7 +10,7 @@ module.exports = BlogController = url = req.url?.toLowerCase() blogUrl = "#{settings.apis.blog.url}#{url}" - extensionsToProxy = [".png", ".xml", ".jpeg", ".jpeg", ".jpg", ".zip", ".eps", ".gif"] + extensionsToProxy = [".png", ".xml", ".jpeg", ".jpg", ".json", ".zip", ".eps", ".gif"] shouldProxy = _.find extensionsToProxy, (extension)-> url.indexOf(extension) != -1 @@ -42,4 +42,4 @@ module.exports = BlogController = upstream = request.get(originUrl) upstream.on "error", (error) -> logger.error err: error, "blog proxy error" - upstream.pipe res \ No newline at end of file + upstream.pipe res From 2c2abc3cae17abc13cb1032d56c1be7d26fb6e51 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 20 Mar 2017 11:18:29 +0000 Subject: [PATCH 15/43] Wire up new button with existing UI for adding comments. --- services/web/app/views/project/editor/review-panel.pug | 2 +- .../ide/review-panel/controllers/ReviewPanelController.coffee | 4 ++++ .../coffee/ide/review-panel/directives/addCommentEntry.coffee | 3 +++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/services/web/app/views/project/editor/review-panel.pug b/services/web/app/views/project/editor/review-panel.pug index f4606be9a3..026663e35f 100644 --- a/services/web/app/views/project/editor/review-panel.pug +++ b/services/web/app/views/project/editor/review-panel.pug @@ -8,7 +8,7 @@ a.rp-add-comment-btn( href ng-if="reviewPanel.entries[editor.open_doc_id]['add-comment'] != null" - ng-click="startNewComment();" + ng-click="addNewComment();" ) i.fa.fa-comment |  #{translate("add_comment")} diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 04a32b2626..2f903879ec 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -327,6 +327,10 @@ define [ $scope.$broadcast "change:reject", entry_id event_tracking.sendMB "rp-change-rejected", { view: if $scope.ui.reviewPanelOpen then $scope.reviewPanel.subView else 'mini' } + $scope.addNewComment = () -> + $scope.$broadcast "comment:start_adding" + $scope.toggleReviewPanel() + $scope.startNewComment = () -> $scope.$broadcast "comment:select_line" $timeout () -> diff --git a/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee b/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee index 2ec79efcd5..40282562d4 100644 --- a/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee +++ b/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee @@ -15,6 +15,9 @@ define [ isAdding: false content: "" + scope.$on "comment:start_adding", () -> + scope.startNewComment() + scope.startNewComment = () -> scope.state.isAdding = true scope.onStartNew() From 8951e91e315e5d5161c54cdaf98b681f5163ab3c Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 20 Mar 2017 11:35:35 +0000 Subject: [PATCH 16/43] Remove add comment button from the mini review panel. --- services/web/app/views/project/editor/review-panel.pug | 9 --------- .../ide/review-panel/directives/addCommentEntry.coffee | 3 +-- .../web/public/stylesheets/app/editor/review-panel.less | 4 ++++ 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/services/web/app/views/project/editor/review-panel.pug b/services/web/app/views/project/editor/review-panel.pug index 026663e35f..83a985ebf0 100644 --- a/services/web/app/views/project/editor/review-panel.pug +++ b/services/web/app/views/project/editor/review-panel.pug @@ -81,7 +81,6 @@ on-start-new="startNewComment();" on-submit="submitNewComment(content);" on-cancel="cancelNewComment();" - on-indicator-click="toggleReviewPanel();" layout-to-left="reviewPanel.layoutToLeft" ) @@ -322,14 +321,6 @@ script(type='text/ng-template', id='resolvedCommentEntryTemplate') script(type='text/ng-template', id='addCommentEntryTemplate') div .rp-entry-callout.rp-entry-callout-add-comment - .rp-entry-indicator( - ng-if="!commentState.adding" - ng-click="startNewComment(); onIndicatorClick();" - tooltip=translate("add_comment") - tooltip-placement="{{ layoutToLeft ? 'left' : 'right' }}" - tooltip-append-to-body="true" - ) - i.fa.fa-commenting .rp-entry.rp-entry-add-comment( ng-class="[ (state.isAdding ? 'rp-entry-adding-comment' : ''), (entry.focused ? 'rp-entry-focused' : '')]" ) diff --git a/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee b/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee index 40282562d4..abbce86e9b 100644 --- a/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee +++ b/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee @@ -7,8 +7,7 @@ define [ scope: onStartNew: "&" onSubmit: "&" - onCancel: "&" - onIndicatorClick: "&" + onCancel: "&" layoutToLeft: "=" link: (scope, element, attrs) -> scope.state = diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index 5bfd6275f9..24f8759b29 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -568,6 +568,10 @@ border-color: @rp-yellow; } } + + .rp-state-current-file &-add-comment { + display: none; + } } .rp-overview-file-header { From 0e24d7118dda2a5d94af589cb006758629c5b86b Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 20 Mar 2017 13:56:36 +0000 Subject: [PATCH 17/43] Hide new UI behind query string parameter. --- services/web/app/views/project/editor/editor.pug | 3 ++- .../web/app/views/project/editor/review-panel.pug | 11 ++++++++++- .../controllers/ReviewPanelController.coffee | 7 ++++++- .../review-panel/directives/addCommentEntry.coffee | 3 ++- .../public/stylesheets/app/editor/review-panel.less | 8 ++++++-- 5 files changed, 26 insertions(+), 6 deletions(-) diff --git a/services/web/app/views/project/editor/editor.pug b/services/web/app/views/project/editor/editor.pug index 9924fe1221..ce24108f77 100644 --- a/services/web/app/views/project/editor/editor.pug +++ b/services/web/app/views/project/editor/editor.pug @@ -19,7 +19,8 @@ div.full-size( 'rp-size-mini': (!ui.reviewPanelOpen && reviewPanel.hasEntries),\ 'rp-size-expanded': ui.reviewPanelOpen,\ 'rp-layout-left': reviewPanel.layoutToLeft,\ - 'rp-loading-threads': reviewPanel.loadingThreads\ + 'rp-loading-threads': reviewPanel.loadingThreads,\ + 'rp-new-comment-ui': reviewPanel.newAddCommentUI\ }" ) .loading-panel(ng-show="!editor.sharejs_doc || editor.opening") diff --git a/services/web/app/views/project/editor/review-panel.pug b/services/web/app/views/project/editor/review-panel.pug index 83a985ebf0..89b0e05305 100644 --- a/services/web/app/views/project/editor/review-panel.pug +++ b/services/web/app/views/project/editor/review-panel.pug @@ -7,7 +7,7 @@ ) !{translate("track_changes_is_on")} a.rp-add-comment-btn( href - ng-if="reviewPanel.entries[editor.open_doc_id]['add-comment'] != null" + ng-if="reviewPanel.newAddCommentUI && reviewPanel.entries[editor.open_doc_id]['add-comment'] != null" ng-click="addNewComment();" ) i.fa.fa-comment @@ -81,6 +81,7 @@ on-start-new="startNewComment();" on-submit="submitNewComment(content);" on-cancel="cancelNewComment();" + on-indicator-click="toggleReviewPanel();" layout-to-left="reviewPanel.layoutToLeft" ) @@ -321,6 +322,14 @@ script(type='text/ng-template', id='resolvedCommentEntryTemplate') script(type='text/ng-template', id='addCommentEntryTemplate') div .rp-entry-callout.rp-entry-callout-add-comment + .rp-entry-indicator.rp-entry-indicator-add-comment( + ng-if="!commentState.adding" + ng-click="startNewComment(); onIndicatorClick();" + tooltip=translate("add_comment") + tooltip-placement="{{ layoutToLeft ? 'left' : 'right' }}" + tooltip-append-to-body="true" + ) + i.fa.fa-commenting .rp-entry.rp-entry-add-comment( ng-class="[ (state.isAdding ? 'rp-entry-adding-comment' : ''), (entry.focused ? 'rp-entry-focused' : '')]" ) diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 2f903879ec..ba5c70daab 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -28,6 +28,10 @@ define [ layoutToLeft: false rendererData: {} loadingThreads: false + newAddCommentUI: false # Test new UI for adding comments; remove afterwards. + + if window.location.search.match /new-comments=true/ + $scope.reviewPanel.newAddCommentUI = true window.addEventListener "beforeunload", () -> collapsedStates = {} @@ -166,7 +170,8 @@ define [ entries = $scope.reviewPanel.entries[$scope.editor.open_doc_id] or {} permEntries = {} for entry, entryData of entries - permEntries[entry] = entryData if entry != "add-comment" + if entry != "add-comment" or !$scope.reviewPanel.newAddCommentUI + permEntries[entry] = entryData Object.keys(permEntries).length ), (nEntries) -> $scope.reviewPanel.hasEntries = nEntries > 0 and $scope.project.features.trackChangesVisible diff --git a/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee b/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee index abbce86e9b..9d33bcb35a 100644 --- a/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee +++ b/services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee @@ -7,7 +7,8 @@ define [ scope: onStartNew: "&" onSubmit: "&" - onCancel: "&" + onCancel: "&" + onIndicatorClick: "&" layoutToLeft: "=" link: (scope, element, attrs) -> scope.state = diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index 24f8759b29..20b1b953fb 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -197,6 +197,10 @@ right: 4px; z-index: 1; } + + .rp-new-comment-ui &-add-comment { + display: none; + } } .rp-entry-wrapper { @@ -569,7 +573,7 @@ } } - .rp-state-current-file &-add-comment { + .rp-size-mini.rp-new-comment-ui &-add-comment { display: none; } } @@ -884,7 +888,7 @@ position: absolute; top: 0; right: 0; - font-size: 10px; + font-size: 11px; .rp-size-mini & { right: @review-off-width; From a35f2585c7ce21f7e08334ea90a89363dddb7421 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 20 Mar 2017 14:02:25 +0000 Subject: [PATCH 18/43] Remove unused flag. --- .../ide/review-panel/controllers/ReviewPanelController.coffee | 1 - 1 file changed, 1 deletion(-) diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index ba5c70daab..406d2a9c5b 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -15,7 +15,6 @@ define [ entries: {} resolvedComments: {} hasEntries: false - showAddComment: false subView: $scope.SubViews.CUR_FILE openSubView: $scope.SubViews.CUR_FILE overview: From 8a8a5a7079f8dbd5ae3786ef1cb746c75fb2a456 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 21 Mar 2017 10:57:09 +0000 Subject: [PATCH 19/43] Add a 'ServiceNotConfiguredError' to Errors module. --- services/web/app/coffee/Features/Errors/Errors.coffee | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Errors/Errors.coffee b/services/web/app/coffee/Features/Errors/Errors.coffee index 0bbff1f19b..92228f7b0b 100644 --- a/services/web/app/coffee/Features/Errors/Errors.coffee +++ b/services/web/app/coffee/Features/Errors/Errors.coffee @@ -5,5 +5,14 @@ NotFoundError = (message) -> return error NotFoundError.prototype.__proto__ = Error.prototype + +ServiceNotConfiguredError = (message) -> + error = new Error(message) + error.name = "ServiceNotConfiguredError" + error.__proto__ = ServiceNotConfiguredError.prototype + return error + + module.exports = Errors = - NotFoundError: NotFoundError \ No newline at end of file + NotFoundError: NotFoundError + ServiceNotConfiguredError: ServiceNotConfiguredError From 1ed1eaaa052a04a727771a51429ad9313e1d2385 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 21 Mar 2017 10:57:39 +0000 Subject: [PATCH 20/43] If analytics is not configured, produce a ServiceNotConfiguredError --- .../coffee/Features/Analytics/AnalyticsController.coffee | 9 ++++++++- .../coffee/Features/Analytics/AnalyticsManager.coffee | 4 ++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee b/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee index f9431a0f5c..75a8bf092a 100644 --- a/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee +++ b/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee @@ -1,7 +1,14 @@ AnalyticsManager = require "./AnalyticsManager" +Errors = require "../Errors/Errors" + module.exports = AnalyticsController = recordEvent: (req, res, next) -> AnalyticsManager.recordEvent req.session?.user?._id, req.params.event, req.body, (error) -> - return next(error) if error? + if error? + if error instanceof Errors.ServiceNotConfiguredError + # ignore, no-op + return next(204) + else + return next(error) res.send 204 diff --git a/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee b/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee index 9f34d6d9d1..66d2818032 100644 --- a/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee +++ b/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee @@ -2,6 +2,7 @@ settings = require "settings-sharelatex" logger = require "logger-sharelatex" _ = require "underscore" request = require "request" +Errors = require '../Errors/Errors' makeRequest = (opts, callback)-> @@ -10,8 +11,7 @@ makeRequest = (opts, callback)-> opts.url = "#{settings.apis.analytics.url}#{urlPath}" request opts, callback else - callback() - + callback(new Errors.ServiceNotConfiguredError('Analytics service not configured')) module.exports = From 1663f2a8eb6e19fa490bb8f24f15253b6bd56d95 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 21 Mar 2017 11:09:39 +0000 Subject: [PATCH 21/43] Use res.send, not next --- .../app/coffee/Features/Analytics/AnalyticsController.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee b/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee index 75a8bf092a..1f2466674c 100644 --- a/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee +++ b/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee @@ -8,7 +8,7 @@ module.exports = AnalyticsController = if error? if error instanceof Errors.ServiceNotConfiguredError # ignore, no-op - return next(204) + return res.send(204) else return next(error) res.send 204 From d611ccd2cb38d0095b896eab4d8cab35e2f4ec82 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Tue, 21 Mar 2017 14:28:58 +0000 Subject: [PATCH 22/43] Apply z-index rule to container, not to the button. --- services/web/public/stylesheets/app/editor/review-panel.less | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index c2bef96e72..c97c3bf9a3 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -889,6 +889,7 @@ top: 0; right: 0; font-size: 11px; + z-index: 2; .rp-size-mini & { right: @review-off-width; @@ -901,7 +902,6 @@ color: @rp-type-blue; text-align: center; border-bottom-left-radius: 3px; - z-index: 2; white-space: nowrap; &.rp-track-changes-indicator-on-dark { From f910bb58de378609aea158fb0806a38cc7c44064 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Wed, 22 Mar 2017 13:11:45 +0000 Subject: [PATCH 23/43] add tests for AnalyticsController --- .../Analytics/AnalyticsControllerTests.coffee | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 services/web/test/UnitTests/coffee/Analytics/AnalyticsControllerTests.coffee diff --git a/services/web/test/UnitTests/coffee/Analytics/AnalyticsControllerTests.coffee b/services/web/test/UnitTests/coffee/Analytics/AnalyticsControllerTests.coffee new file mode 100644 index 0000000000..2c52b8b5f0 --- /dev/null +++ b/services/web/test/UnitTests/coffee/Analytics/AnalyticsControllerTests.coffee @@ -0,0 +1,44 @@ +should = require('chai').should() +SandboxedModule = require('sandboxed-module') +assert = require('assert') +path = require('path') +modulePath = path.join __dirname, '../../../../app/js/Features/Analytics/AnalyticsController' +sinon = require("sinon") +expect = require("chai").expect + + +describe 'AnalyticsController', -> + + beforeEach -> + @AuthenticationController = + getLoggedInUserId: sinon.stub() + + @AnalyticsManager = + recordEvent: sinon.stub().callsArgWith(3) + + @req = + params: + event:"i_did_something" + body:"stuff" + sessionID: "sessionIDHere" + + @res = + send:-> + @controller = SandboxedModule.require modulePath, requires: + "./AnalyticsManager":@AnalyticsManager + "../Authentication/AuthenticationController":@AuthenticationController + "logger-sharelatex": + log:-> + + describe "recordEvent", -> + + it "should use the user_id", (done)-> + @AuthenticationController.getLoggedInUserId.returns("1234") + @controller.recordEvent @req, @res + @AnalyticsManager.recordEvent.calledWith("1234", @req.params["event"], @req.body).should.equal true + done() + + it "should use the session id", (done)-> + @controller.recordEvent @req, @res + @AnalyticsManager.recordEvent.calledWith(@req.sessionID, @req.params["event"], @req.body).should.equal true + done() From ebdce6169efae75ed5b94e7a0d160dceb3305526 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Wed, 22 Mar 2017 15:50:49 +0000 Subject: [PATCH 24/43] idendifyUser on login --- .../app/coffee/Features/Analytics/AnalyticsManager.coffee | 5 +++-- .../Features/Authentication/AuthenticationController.coffee | 1 + .../Authentication/AuthenticationControllerTests.coffee | 5 +++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee b/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee index 9165678070..003a59de2d 100644 --- a/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee +++ b/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee @@ -16,14 +16,15 @@ makeRequest = (opts, callback)-> module.exports = - idendifyUser: (user_id, old_user_id, callback)-> + idendifyUser: (user_id, old_user_id, callback = (error)->)-> opts = body: old_user_id:old_user_id json:true method:"POST" timeout:1000 - url: "/user/#{user_id}/idendify" + url: "/user/#{user_id}/identify" + makeRequest opts, callback recordEvent: (user_id, event, segmentation = {}, callback = (error) ->) -> if user_id+"" == settings.smokeTest?.userId+"" diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 485b046a85..b78afef5fb 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -87,6 +87,7 @@ module.exports = AuthenticationController = LoginRateLimiter.recordSuccessfulLogin(email) AuthenticationController._recordSuccessfulLogin(user._id) Analytics.recordEvent(user._id, "user-logged-in", {ip:req.ip}) + Analytics.idendifyUser(user._id, req.sessionID) logger.log email: email, user_id: user._id.toString(), "successful log in" req.session.justLoggedIn = true # capture the request ip for use when creating the session diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee index 94e930c7b1..6230696d99 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee @@ -254,6 +254,8 @@ describe "AuthenticationController", -> @cb = sinon.stub() @LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) @AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, null, @user) + @req.sessionID = Math.random() + @AnalyticsManager.idendifyUser = sinon.stub() @AuthenticationController.doPassportLogin(@req, @req.body.email, @req.body.password, @cb) it "should attempt to authorise the user", -> @@ -261,6 +263,9 @@ describe "AuthenticationController", -> .calledWith(email: @email.toLowerCase(), @password) .should.equal true + it "should call idendifyUser", -> + @AnalyticsManager.idendifyUser.calledWith(@user._id, @req.sessionID).should.equal true + it "should setup the user data in the background", -> @UserHandler.setupLoginData.calledWith(@user).should.equal true From cff922a0f548040ba012dbc28d0872cf596fcf72 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Wed, 22 Mar 2017 16:01:26 +0000 Subject: [PATCH 25/43] idendify -> identify --- .../app/coffee/Features/Analytics/AnalyticsManager.coffee | 2 +- .../Features/Authentication/AuthenticationController.coffee | 2 +- .../Authentication/AuthenticationControllerTests.coffee | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee b/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee index 003a59de2d..be6e011288 100644 --- a/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee +++ b/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee @@ -16,7 +16,7 @@ makeRequest = (opts, callback)-> module.exports = - idendifyUser: (user_id, old_user_id, callback = (error)->)-> + identifyUser: (user_id, old_user_id, callback = (error)->)-> opts = body: old_user_id:old_user_id diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index b78afef5fb..3c43323887 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -87,7 +87,7 @@ module.exports = AuthenticationController = LoginRateLimiter.recordSuccessfulLogin(email) AuthenticationController._recordSuccessfulLogin(user._id) Analytics.recordEvent(user._id, "user-logged-in", {ip:req.ip}) - Analytics.idendifyUser(user._id, req.sessionID) + Analytics.identifyUser(user._id, req.sessionID) logger.log email: email, user_id: user._id.toString(), "successful log in" req.session.justLoggedIn = true # capture the request ip for use when creating the session diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee index 6230696d99..f44968a0e5 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee @@ -255,7 +255,7 @@ describe "AuthenticationController", -> @LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) @AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, null, @user) @req.sessionID = Math.random() - @AnalyticsManager.idendifyUser = sinon.stub() + @AnalyticsManager.identifyUser = sinon.stub() @AuthenticationController.doPassportLogin(@req, @req.body.email, @req.body.password, @cb) it "should attempt to authorise the user", -> @@ -263,8 +263,8 @@ describe "AuthenticationController", -> .calledWith(email: @email.toLowerCase(), @password) .should.equal true - it "should call idendifyUser", -> - @AnalyticsManager.idendifyUser.calledWith(@user._id, @req.sessionID).should.equal true + it "should call identifyUser", -> + @AnalyticsManager.identifyUser.calledWith(@user._id, @req.sessionID).should.equal true it "should setup the user data in the background", -> @UserHandler.setupLoginData.calledWith(@user).should.equal true From 254e4953cb9d5fdbb1f2b10539a39d237620af38 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Thu, 23 Mar 2017 12:26:21 +0000 Subject: [PATCH 26/43] Wire-up AB test for adding comments. --- services/web/public/coffee/ide.coffee | 6 ++++++ .../review-panel/controllers/ReviewPanelController.coffee | 7 ++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/services/web/public/coffee/ide.coffee b/services/web/public/coffee/ide.coffee index 9fbbfe9937..2abb5a19b5 100644 --- a/services/web/public/coffee/ide.coffee +++ b/services/web/public/coffee/ide.coffee @@ -81,6 +81,12 @@ define [ if $scope.user.signUpDate >= '2016-10-27' $scope.shouldABTestPlans = true + $scope.shouldABAddCommentBtn = false + if $scope.user.signUpDate >= '2016-03-22' + $scope.shouldABAddCommentBtn = true + sixpack.participate "add-comment-btn", [ "default", "editor-corner" ], (variation) -> + $scope.variationABAddCommentBtn = variation + $scope.settings = window.userSettings $scope.anonymous = window.anonymous diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 406d2a9c5b..46e7617a9e 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -29,8 +29,13 @@ define [ loadingThreads: false newAddCommentUI: false # Test new UI for adding comments; remove afterwards. - if window.location.search.match /new-comments=true/ + if $scope.shouldABAddCommentBtn and $scope.variationABAddCommentBtn? == "editor-corner" $scope.reviewPanel.newAddCommentUI = true + console.log "editor corner" + else + console.log "default" + + console.log $scope.shouldABAddCommentBtn window.addEventListener "beforeunload", () -> collapsedStates = {} From c72ee95177e048275ac837d2ee1d9442a68612c2 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Thu, 23 Mar 2017 15:00:43 +0000 Subject: [PATCH 27/43] Refactor AB test participation code; add conversion code. --- services/web/public/coffee/ide.coffee | 6 ------ .../controllers/ReviewPanelController.coffee | 19 ++++++++++--------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/services/web/public/coffee/ide.coffee b/services/web/public/coffee/ide.coffee index 2abb5a19b5..9fbbfe9937 100644 --- a/services/web/public/coffee/ide.coffee +++ b/services/web/public/coffee/ide.coffee @@ -81,12 +81,6 @@ define [ if $scope.user.signUpDate >= '2016-10-27' $scope.shouldABTestPlans = true - $scope.shouldABAddCommentBtn = false - if $scope.user.signUpDate >= '2016-03-22' - $scope.shouldABAddCommentBtn = true - sixpack.participate "add-comment-btn", [ "default", "editor-corner" ], (variation) -> - $scope.variationABAddCommentBtn = variation - $scope.settings = window.userSettings $scope.anonymous = window.anonymous diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 46e7617a9e..4ce7741dcc 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -4,7 +4,7 @@ define [ "ide/colors/ColorManager" "ide/review-panel/RangesTracker" ], (App, EventEmitter, ColorManager, RangesTracker) -> - App.controller "ReviewPanelController", ($scope, $element, ide, $timeout, $http, $modal, event_tracking, localStorage) -> + App.controller "ReviewPanelController", ($scope, $element, ide, $timeout, $http, $modal, event_tracking, sixpack, localStorage) -> $reviewPanelEl = $element.find "#review-panel" $scope.SubViews = @@ -29,13 +29,12 @@ define [ loadingThreads: false newAddCommentUI: false # Test new UI for adding comments; remove afterwards. - if $scope.shouldABAddCommentBtn and $scope.variationABAddCommentBtn? == "editor-corner" - $scope.reviewPanel.newAddCommentUI = true - console.log "editor corner" - else - console.log "default" - - console.log $scope.shouldABAddCommentBtn + $scope.shouldABAddCommentBtn = false + if $scope.user.signUpDate >= '2016-03-22' + sixpack.participate "add-comment-btn", [ "default", "editor-corner" ], (variation) -> + $scope.shouldABAddCommentBtn = true + $scope.variationABAddCommentBtn = variation + $scope.reviewPanel.newAddCommentUI = (variation == "editor-corner") window.addEventListener "beforeunload", () -> collapsedStates = {} @@ -344,7 +343,9 @@ define [ $scope.$broadcast "comment:select_line" $timeout () -> $scope.$broadcast "review-panel:layout" - + if $scope.shouldABAddCommentBtn and !$scope.ui.reviewPanelOpen + sixpack.convert "add-comment-btn" + $scope.submitNewComment = (content) -> return if !content? or content == "" doc_id = $scope.editor.open_doc_id From ed4a3219068e7597cf642bcf4ca230b5dd8c9f0d Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Thu, 23 Mar 2017 15:39:12 +0000 Subject: [PATCH 28/43] remove extra debug route --- .../app/coffee/Features/StaticPages/StaticPagesRouter.coffee | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/StaticPages/StaticPagesRouter.coffee b/services/web/app/coffee/Features/StaticPages/StaticPagesRouter.coffee index 66c210491b..47360f1728 100644 --- a/services/web/app/coffee/Features/StaticPages/StaticPagesRouter.coffee +++ b/services/web/app/coffee/Features/StaticPages/StaticPagesRouter.coffee @@ -9,7 +9,6 @@ module.exports = webRouter.get '/tos', HomeController.externalPage("tos", "Terms of Service") webRouter.get '/about', HomeController.externalPage("about", "About Us") - webRouter.get '/review', HomeController.externalPage("review", "About Us") webRouter.get '/security', HomeController.externalPage("security", "Security") webRouter.get '/privacy_policy', HomeController.externalPage("privacy", "Privacy Policy") @@ -20,4 +19,4 @@ module.exports = webRouter.get '/dropbox', HomeController.externalPage("dropbox", "Dropbox and ShareLaTeX") webRouter.get '/university', UniversityController.getIndexPage - webRouter.get '/university/*', UniversityController.getPage \ No newline at end of file + webRouter.get '/university/*', UniversityController.getPage From 78e8a8319d25ebb1446469a0f4026c8be2d638ee Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Fri, 24 Mar 2017 14:04:37 +0000 Subject: [PATCH 29/43] Put review panel widgets container inside the review panel element; rearrange visibility logic. --- .../app/views/project/editor/review-panel.pug | 28 +++++++++---------- .../stylesheets/app/editor/review-panel.less | 16 +++++++---- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/services/web/app/views/project/editor/review-panel.pug b/services/web/app/views/project/editor/review-panel.pug index 89b0e05305..da14b69a99 100644 --- a/services/web/app/views/project/editor/review-panel.pug +++ b/services/web/app/views/project/editor/review-panel.pug @@ -1,18 +1,18 @@ -.rp-in-editor-widgets - a.rp-track-changes-indicator( - href - ng-if="editor.wantTrackChanges" - ng-click="toggleReviewPanel();" - ng-class="{ 'rp-track-changes-indicator-on-dark' : darkTheme }" - ) !{translate("track_changes_is_on")} - a.rp-add-comment-btn( - href - ng-if="reviewPanel.newAddCommentUI && reviewPanel.entries[editor.open_doc_id]['add-comment'] != null" - ng-click="addNewComment();" - ) - i.fa.fa-comment - |  #{translate("add_comment")} #review-panel + .rp-in-editor-widgets + a.rp-track-changes-indicator( + href + ng-if="editor.wantTrackChanges" + ng-click="toggleReviewPanel();" + ng-class="{ 'rp-track-changes-indicator-on-dark' : darkTheme }" + ) !{translate("track_changes_is_on")} + a.rp-add-comment-btn( + href + ng-if="reviewPanel.newAddCommentUI && reviewPanel.entries[editor.open_doc_id]['add-comment'] != null" + ng-click="addNewComment();" + ) + i.fa.fa-comment + |  #{translate("add_comment")} .review-panel-toolbar resolved-comments-dropdown( class="rp-flex-block" diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index c97c3bf9a3..3030e7dbb9 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -90,7 +90,7 @@ } #review-panel { - display: none; + display: block; .rp-size-expanded & { display: flex; flex-direction: column; @@ -98,7 +98,6 @@ overflow: visible; } .rp-size-mini & { - display: block; width: @review-off-width; z-index: 6; } @@ -152,7 +151,13 @@ } .rp-entry-list { + display: none; width: 100%; + + .rp-size-expanded &, + .rp-size-mini & { + display: block; + } .rp-state-current-file & { position: absolute; @@ -620,11 +625,12 @@ } .rp-nav { - display: flex; + display: none; flex-shrink: 0; - .rp-size-mini & { - display: none; + .rp-size-expanded & { + display: flex; } + .rp-state-current-file & { position: absolute; bottom: 0; From 93934b9c8f091ea13485c994c76a96e7d2e20f55 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 27 Mar 2017 09:32:02 +0100 Subject: [PATCH 30/43] Use deploy date. --- .../ide/review-panel/controllers/ReviewPanelController.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 4ce7741dcc..d4c40084bd 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -30,7 +30,7 @@ define [ newAddCommentUI: false # Test new UI for adding comments; remove afterwards. $scope.shouldABAddCommentBtn = false - if $scope.user.signUpDate >= '2016-03-22' + if $scope.user.signUpDate >= '2017-03-27' sixpack.participate "add-comment-btn", [ "default", "editor-corner" ], (variation) -> $scope.shouldABAddCommentBtn = true $scope.variationABAddCommentBtn = variation From f27dfa54f1728e61cac25df67e7adc1bc0eeb2b5 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Mon, 27 Mar 2017 09:42:38 +0100 Subject: [PATCH 31/43] fixup AuthenticationController from missing module after merge --- .../Analytics/AnalyticsController.coffee | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee b/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee index b96084c288..18a7de5e24 100644 --- a/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee +++ b/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee @@ -1,15 +1,15 @@ AnalyticsManager = require "./AnalyticsManager" Errors = require "../Errors/Errors" - +AuthenticationController = require("../Authentication/AuthenticationController") module.exports = AnalyticsController = recordEvent: (req, res, next) -> user_id = AuthenticationController.getLoggedInUserId(req) or req.sessionID AnalyticsManager.recordEvent user_id, req.params.event, req.body, (error) -> - if error? - if error instanceof Errors.ServiceNotConfiguredError - # ignore, no-op - return res.send(204) - else - return next(error) - res.send 204 + if error instanceof Errors.ServiceNotConfiguredError + # ignore, no-op + return res.send(204) + else if error? + return next(error) + else + return res.send 204 From e22da8e530ec68cd23e7c43cbe967ad70be26b85 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Mon, 27 Mar 2017 09:57:15 +0100 Subject: [PATCH 32/43] rename unit test --- .../coffee/Authorization/AuthorizationManagerTests.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/test/UnitTests/coffee/Authorization/AuthorizationManagerTests.coffee b/services/web/test/UnitTests/coffee/Authorization/AuthorizationManagerTests.coffee index b85449d7fd..d779934055 100644 --- a/services/web/test/UnitTests/coffee/Authorization/AuthorizationManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authorization/AuthorizationManagerTests.coffee @@ -137,7 +137,7 @@ describe "AuthorizationManager", -> @AuthorizationManager.getPrivilegeLevelForProject @user_id, @project_id, (error) -> error.should.be.instanceof Errors.NotFoundError - describe "when the project id is not validssssssss", -> + describe "when the project id is not valid", -> beforeEach -> @AuthorizationManager.isUserSiteAdmin.withArgs(@user_id).yields(null, false) @CollaboratorsHandler.getMemberIdPrivilegeLevel From e473b5e2708d894d7e8fb3275c2d90dee70b06ac Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 27 Mar 2017 10:02:09 +0100 Subject: [PATCH 33/43] Avoid line breaks in the add comment button. --- services/web/public/stylesheets/app/editor/review-panel.less | 1 + 1 file changed, 1 insertion(+) diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index 3030e7dbb9..c069c536e2 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -456,6 +456,7 @@ border-radius: 3px; .rp-in-editor-widgets & { + white-space: nowrap; border-radius: 0; border-bottom-left-radius: 3px; } From 7aa4c0c030d994272f17bbd09d82c0583d64dab3 Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 27 Mar 2017 12:07:43 +0100 Subject: [PATCH 34/43] Check Recurly for subscription as well before creating subscription --- .../Subscription/RecurlyWrapper.coffee | 38 +++++----- .../SubscriptionController.coffee | 58 ++++++++------- .../Subscription/SubscriptionHandler.coffee | 19 ++++- .../SubscriptionControllerTests.coffee | 14 +++- .../SubscriptionHandlerTests.coffee | 70 +++++++++++++++---- 5 files changed, 137 insertions(+), 62 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/RecurlyWrapper.coffee b/services/web/app/coffee/Features/Subscription/RecurlyWrapper.coffee index 60387c0dc0..ce40847a27 100644 --- a/services/web/app/coffee/Features/Subscription/RecurlyWrapper.coffee +++ b/services/web/app/coffee/Features/Subscription/RecurlyWrapper.coffee @@ -469,33 +469,35 @@ module.exports = RecurlyWrapper = logger.err err:error, subscriptionId:subscriptionId, daysUntilExpire:daysUntilExpire, "error exending trial" callback(error) ) + + listAccountActiveSubscriptions: (account_id, callback = (error, subscriptions) ->) -> + RecurlyWrapper.apiRequest { + url: "accounts/#{account_id}/subscriptions" + qs: + state: "active" + }, (error, response, body) -> + return callback(error) if error? + RecurlyWrapper._parseSubscriptionsXml body, callback + + _parseSubscriptionsXml: (xml, callback) -> + RecurlyWrapper._parseXmlAndGetAttribute xml, "subscriptions", callback _parseSubscriptionXml: (xml, callback) -> - RecurlyWrapper._parseXml xml, (error, data) -> - return callback(error) if error? - if data? and data.subscription? - recurlySubscription = data.subscription - else - return callback "I don't understand the response from Recurly" - callback null, recurlySubscription + RecurlyWrapper._parseXmlAndGetAttribute xml, "subscription", callback _parseAccountXml: (xml, callback) -> - RecurlyWrapper._parseXml xml, (error, data) -> - return callback(error) if error? - if data? and data.account? - account = data.account - else - return callback "I don't understand the response from Recurly" - callback null, account + RecurlyWrapper._parseXmlAndGetAttribute xml, "account", callback _parseBillingInfoXml: (xml, callback) -> + RecurlyWrapper._parseXmlAndGetAttribute xml, "billing_info", callback + + _parseXmlAndGetAttribute: (xml, attribute, callback) -> RecurlyWrapper._parseXml xml, (error, data) -> return callback(error) if error? - if data? and data.billing_info? - billingInfo = data.billing_info + if data? and data[attribute]? + return callback null, data[attribute] else - return callback "I don't understand the response from Recurly" - callback null, billingInfo + return callback(new Error("I don't understand the response from Recurly")) _parseXml: (xml, callback) -> convertDataTypes = (data) -> diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index 84de417bb6..cc3013a42d 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -46,31 +46,39 @@ module.exports = SubscriptionController = if hasSubscription or !plan? res.redirect "/user/subscription" else - currency = req.query.currency?.toUpperCase() - GeoIpLookup.getCurrencyCode req.query?.ip || req.ip, (err, recomendedCurrency, countryCode)-> - return next(err) if err? - if recomendedCurrency? and !currency? - currency = recomendedCurrency - RecurlyWrapper.sign { - subscription: - plan_code : req.query.planCode - currency: currency - account_code: user._id - }, (error, signature) -> - return next(error) if error? - res.render "subscriptions/new", - title : "subscribe" - plan_code: req.query.planCode - currency: currency - countryCode:countryCode - plan:plan - showStudentPlan: req.query.ssp - recurlyConfig: JSON.stringify - currency: currency - subdomain: Settings.apis.recurly.subdomain - showCouponField: req.query.scf - showVatField: req.query.svf - couponCode: req.query.cc or "" + # LimitationsManager.userHasSubscription only checks Mongo. Double check with + # Recurly as well at this point (we don't do this most places for speed). + SubscriptionHandler.validateNoSubscriptionInRecurly user._id, (error, valid) -> + return next(error) if error? + if !valid + res.redirect "/user/subscription" + return + else + currency = req.query.currency?.toUpperCase() + GeoIpLookup.getCurrencyCode req.query?.ip || req.ip, (err, recomendedCurrency, countryCode)-> + return next(err) if err? + if recomendedCurrency? and !currency? + currency = recomendedCurrency + RecurlyWrapper.sign { + subscription: + plan_code : req.query.planCode + currency: currency + account_code: user._id + }, (error, signature) -> + return next(error) if error? + res.render "subscriptions/new", + title : "subscribe" + plan_code: req.query.planCode + currency: currency + countryCode:countryCode + plan:plan + showStudentPlan: req.query.ssp + recurlyConfig: JSON.stringify + currency: currency + subdomain: Settings.apis.recurly.subdomain + showCouponField: req.query.scf + showVatField: req.query.svf + couponCode: req.query.cc or "" diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionHandler.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionHandler.coffee index ba95895dcd..99da3270a8 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionHandler.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionHandler.coffee @@ -11,15 +11,28 @@ Analytics = require("../Analytics/AnalyticsManager") module.exports = + validateNoSubscriptionInRecurly: (user_id, callback = (error, valid) ->) -> + RecurlyWrapper.listAccountActiveSubscriptions user_id, (error, subscriptions = []) -> + return callback(error) if error? + if subscriptions.length > 0 + SubscriptionUpdater.syncSubscription subscriptions[0], user_id, (error) -> + return callback(error) if error? + return callback(null, false) + else + return callback(null, true) createSubscription: (user, subscriptionDetails, recurly_token_id, callback)-> self = @ clientTokenId = "" - RecurlyWrapper.createSubscription user, subscriptionDetails, recurly_token_id, (error, recurlySubscription)-> + @validateNoSubscriptionInRecurly user._id, (error, valid) -> return callback(error) if error? - SubscriptionUpdater.syncSubscription recurlySubscription, user._id, (error) -> + if !valid + return callback(new Error("user already has subscription in recurly")) + RecurlyWrapper.createSubscription user, subscriptionDetails, recurly_token_id, (error, recurlySubscription)-> return callback(error) if error? - callback() + SubscriptionUpdater.syncSubscription recurlySubscription, user._id, (error) -> + return callback(error) if error? + callback() updateSubscription: (user, plan_code, coupon_code, callback)-> logger.log user:user, plan_code:plan_code, coupon_code:coupon_code, "updating subscription" diff --git a/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee b/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee index 3a43c8988e..b95fba1cdd 100644 --- a/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee @@ -17,8 +17,7 @@ mockSubscriptions = account: account_code: "user-123" -describe "SubscriptionController sanboxed", -> - +describe "SubscriptionController", -> beforeEach -> @user = {email:"tom@yahoo.com", _id: 'one', signUpDate: new Date('2000-10-01')} @activeRecurlySubscription = mockSubscriptions["subscription-123-active"] @@ -150,6 +149,7 @@ describe "SubscriptionController sanboxed", -> describe "paymentPage", -> beforeEach -> @req.headers = {} + @SubscriptionHandler.validateNoSubscriptionInRecurly = sinon.stub().yields(null, true) @GeoIpLookup.getCurrencyCode.callsArgWith(1, null, @stubbedCurrencyCode) describe "with a user without a subscription", -> @@ -209,6 +209,16 @@ describe "SubscriptionController sanboxed", -> opts.currency.should.equal @stubbedCurrencyCode done() @SubscriptionController.paymentPage @req, @res + + describe "with a recurly subscription already", -> + it "should redirect to the subscription dashboard", (done)-> + @LimitationsManager.userHasSubscription.callsArgWith(1, null, false) + @SubscriptionHandler.validateNoSubscriptionInRecurly = sinon.stub().yields(null, false) + @res.redirect = (url)=> + url.should.equal "/user/subscription" + done() + @SubscriptionController.paymentPage(@req, @res) + describe "successful_subscription", -> beforeEach (done) -> diff --git a/services/web/test/UnitTests/coffee/Subscription/SubscriptionHandlerTests.coffee b/services/web/test/UnitTests/coffee/Subscription/SubscriptionHandlerTests.coffee index 935ed6d025..1e2cec4bfa 100644 --- a/services/web/test/UnitTests/coffee/Subscription/SubscriptionHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Subscription/SubscriptionHandlerTests.coffee @@ -16,7 +16,7 @@ mockRecurlySubscriptions = account: account_code: "user-123" -describe "Subscription Handler sanboxed", -> +describe "SubscriptionHandler", -> beforeEach -> @Settings = @@ -33,7 +33,7 @@ describe "Subscription Handler sanboxed", -> @activeRecurlySubscription = mockRecurlySubscriptions["subscription-123-active"] @User = {} @user = - _id: "user_id_here_" + _id: @user_id = "user_id_here_" @subscription = recurlySubscription_id: @activeRecurlySubscription.uuid @RecurlyWrapper = @@ -77,23 +77,33 @@ describe "Subscription Handler sanboxed", -> describe "createSubscription", -> - beforeEach (done) -> + beforeEach -> + @callback = sinon.stub() @subscriptionDetails = cvv:"123" number:"12345" @recurly_token_id = "45555666" - @SubscriptionHandler.createSubscription(@user, @subscriptionDetails, @recurly_token_id, done) + @SubscriptionHandler.validateNoSubscriptionInRecurly = sinon.stub().yields(null, true) - it "should create the subscription with the wrapper", (done)-> - @RecurlyWrapper.createSubscription.calledWith(@user, @subscriptionDetails, @recurly_token_id).should.equal true - done() + describe "successfully", -> + beforeEach -> + @SubscriptionHandler.createSubscription(@user, @subscriptionDetails, @recurly_token_id, @callback) - it "should sync the subscription to the user", (done)-> - @SubscriptionUpdater.syncSubscription.calledOnce.should.equal true - @SubscriptionUpdater.syncSubscription.args[0][0].should.deep.equal @activeRecurlySubscription - @SubscriptionUpdater.syncSubscription.args[0][1].should.deep.equal @user._id - done() + it "should create the subscription with the wrapper", -> + @RecurlyWrapper.createSubscription.calledWith(@user, @subscriptionDetails, @recurly_token_id).should.equal true + it "should sync the subscription to the user", -> + @SubscriptionUpdater.syncSubscription.calledOnce.should.equal true + @SubscriptionUpdater.syncSubscription.args[0][0].should.deep.equal @activeRecurlySubscription + @SubscriptionUpdater.syncSubscription.args[0][1].should.deep.equal @user._id + + describe "when there is already a subscription in Recurly", -> + beforeEach -> + @SubscriptionHandler.validateNoSubscriptionInRecurly = sinon.stub().yields(null, false) + @SubscriptionHandler.createSubscription(@user, @subscriptionDetails, @recurly_token_id, @callback) + + it "should return an error", -> + @callback.calledWith(new Error("user already has subscription in recurly")) describe "updateSubscription", -> describe "with a user with a subscription", -> @@ -145,8 +155,6 @@ describe "Subscription Handler sanboxed", -> updateOptions = @RecurlyWrapper.updateSubscription.args[0][1] updateOptions.plan_code.should.equal @plan_code - - describe "cancelSubscription", -> describe "with a user without a subscription", -> beforeEach (done) -> @@ -210,5 +218,39 @@ describe "Subscription Handler sanboxed", -> @SubscriptionUpdater.syncSubscription.args[0][0].should.deep.equal @activeRecurlySubscription @SubscriptionUpdater.syncSubscription.args[0][1].should.deep.equal @user._id + describe "validateNoSubscriptionInRecurly", -> + beforeEach -> + @subscriptions = [] + @RecurlyWrapper.listAccountActiveSubscriptions = sinon.stub().yields(null, @subscriptions) + @SubscriptionUpdater.syncSubscription = sinon.stub().yields() + @callback = sinon.stub() + describe "with no subscription in recurly", -> + beforeEach -> + @subscriptions.push @subscription = { "mock": "subscription" } + @SubscriptionHandler.validateNoSubscriptionInRecurly @user_id, @callback + it "should call RecurlyWrapper.listAccountActiveSubscriptions with the user id", -> + @RecurlyWrapper.listAccountActiveSubscriptions + .calledWith(@user_id) + .should.equal true + + it "should sync the subscription", -> + @SubscriptionUpdater.syncSubscription + .calledWith(@subscription, @user_id) + .should.equal true + + it "should call the callback with valid == false", -> + @callback.calledWith(null, false).should.equal true + + describe "with a subscription in recurly", -> + beforeEach -> + @SubscriptionHandler.validateNoSubscriptionInRecurly @user_id, @callback + + it "should not sync the subscription", -> + @SubscriptionUpdater.syncSubscription + .called + .should.equal false + + it "should call the callback with valid == true", -> + @callback.calledWith(null, true).should.equal true From 30c5bbfdfc832951607f2d3e0b818801809c083c Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Mon, 27 Mar 2017 14:45:34 +0100 Subject: [PATCH 35/43] Add a .nvmrc file --- services/web/.nvmrc | 1 + 1 file changed, 1 insertion(+) create mode 100644 services/web/.nvmrc diff --git a/services/web/.nvmrc b/services/web/.nvmrc new file mode 100644 index 0000000000..994fe99096 --- /dev/null +++ b/services/web/.nvmrc @@ -0,0 +1 @@ +0.10.22 \ No newline at end of file From 69b9b308d4c929cfa133e12d0d827dd1b800c0ce Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Mon, 27 Mar 2017 16:17:38 +0100 Subject: [PATCH 36/43] If sending email fails, return a generic error. This prevents us from leaking juicy details of our aws/ses setup via the password-reset form. --- services/web/app/coffee/Features/Email/EmailSender.coffee | 2 +- .../web/test/UnitTests/coffee/Email/EmailSenderTests.coffee | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/services/web/app/coffee/Features/Email/EmailSender.coffee b/services/web/app/coffee/Features/Email/EmailSender.coffee index 69574c8276..009d24e182 100644 --- a/services/web/app/coffee/Features/Email/EmailSender.coffee +++ b/services/web/app/coffee/Features/Email/EmailSender.coffee @@ -74,4 +74,4 @@ module.exports = logger.err err:err, "error sending message" else logger.log "Message sent to #{options.to}" - callback(err) + callback(new Error('Cannot send email')) diff --git a/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee b/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee index beba91fcb3..bdc2a44811 100644 --- a/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee +++ b/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee @@ -58,10 +58,11 @@ describe "EmailSender", -> args.subject.should.equal @opts.subject done() - it "should return the error", (done)-> + it "should return a non-specific error", (done)-> @sesClient.sendMail.callsArgWith(1, "error") @sender.sendEmail {}, (err)=> - err.should.equal "error" + err.should.exist + err.toString().should.equal 'Error: Cannot send email' done() From d2e1efe4a9098bc6713301061d5f923f255034f5 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Mon, 27 Mar 2017 17:45:19 +0100 Subject: [PATCH 37/43] fix a daft mistake --- services/web/app/coffee/Features/Email/EmailSender.coffee | 3 ++- .../web/test/UnitTests/coffee/Email/EmailSenderTests.coffee | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/Email/EmailSender.coffee b/services/web/app/coffee/Features/Email/EmailSender.coffee index 009d24e182..04b4d39132 100644 --- a/services/web/app/coffee/Features/Email/EmailSender.coffee +++ b/services/web/app/coffee/Features/Email/EmailSender.coffee @@ -72,6 +72,7 @@ module.exports = client.sendMail options, (err, res)-> if err? logger.err err:err, "error sending message" + err = new Error('Cannot send email') else logger.log "Message sent to #{options.to}" - callback(new Error('Cannot send email')) + callback(err) diff --git a/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee b/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee index bdc2a44811..dc504907bf 100644 --- a/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee +++ b/services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee @@ -51,7 +51,8 @@ describe "EmailSender", -> it "should set the properties on the email to send", (done)-> @sesClient.sendMail.callsArgWith(1) - @sender.sendEmail @opts, => + @sender.sendEmail @opts, (err) => + expect(err).to.not.exist args = @sesClient.sendMail.args[0][0] args.html.should.equal @opts.html args.to.should.equal @opts.to From 4e66b045e38cb1b8bf354e3936994b48588e1c6e Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 28 Mar 2017 09:44:50 +0100 Subject: [PATCH 38/43] fix unhandled exception in ProjectDetailsHandler --- .../coffee/Features/Project/ProjectDetailsHandler.coffee | 2 +- .../coffee/Project/ProjectDetailsHandlerTests.coffee | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee b/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee index 7bd1d33561..a067a600d1 100644 --- a/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee @@ -10,7 +10,7 @@ module.exports = getDetails: (project_id, callback)-> ProjectGetter.getProject project_id, {name:true, description:true, compiler:true, features:true, owner_ref:true}, (err, project)-> - if err? + if err? or !project? logger.err err:err, project_id:project_id, "error getting project" return callback(err) UserGetter.getUser project.owner_ref, (err, user) -> diff --git a/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee b/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee index b9e8ca27c6..228319abf2 100644 --- a/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee @@ -48,6 +48,13 @@ describe 'ProjectDetailsHandler', -> assert.equal(details.something, undefined) done() + it "should return an error for a non-existent project", (done)-> + @ProjectGetter.getProject.callsArg(2, null, null) + @handler.getDetails "0123456789012345678901234", (err, details)=> + assert.equal(err, undefined) + assert.equal(details, undefined) + done() + it "should return the error", (done)-> error = "some error" @ProjectGetter.getProject.callsArgWith(2, error) From f433510e619d116190d8f7b30fea3343fdf8cf5f Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 28 Mar 2017 10:12:52 +0100 Subject: [PATCH 39/43] return NotFound error in ProjectDetailsHandler --- .../coffee/Features/Project/ProjectDetailsHandler.coffee | 4 +++- .../coffee/Project/ProjectDetailsHandlerTests.coffee | 7 ++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee b/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee index a067a600d1..9c823c9f17 100644 --- a/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee @@ -5,14 +5,16 @@ logger = require("logger-sharelatex") tpdsUpdateSender = require '../ThirdPartyDataStore/TpdsUpdateSender' _ = require("underscore") PublicAccessLevels = require("../Authorization/PublicAccessLevels") +Errors = require("../Errors/Errors") module.exports = getDetails: (project_id, callback)-> ProjectGetter.getProject project_id, {name:true, description:true, compiler:true, features:true, owner_ref:true}, (err, project)-> - if err? or !project? + if err? logger.err err:err, project_id:project_id, "error getting project" return callback(err) + return callback(new Errors.NotFoundError("project not found")) if !project? UserGetter.getUser project.owner_ref, (err, user) -> return callback(err) if err? details = diff --git a/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee b/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee index 228319abf2..501e48f1a5 100644 --- a/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee @@ -1,5 +1,6 @@ should = require('chai').should() modulePath = "../../../../app/js/Features/Project/ProjectDetailsHandler" +Errors = require "../../../../app/js/Features/Errors/Errors" SandboxedModule = require('sandboxed-module') sinon = require('sinon') assert = require("chai").assert @@ -50,9 +51,9 @@ describe 'ProjectDetailsHandler', -> it "should return an error for a non-existent project", (done)-> @ProjectGetter.getProject.callsArg(2, null, null) - @handler.getDetails "0123456789012345678901234", (err, details)=> - assert.equal(err, undefined) - assert.equal(details, undefined) + err = new Errors.NotFoundError("project not found") + @handler.getDetails "0123456789012345678901234", (error, details) => + err.should.eql error done() it "should return the error", (done)-> From 6002fdbad60b2dd3d42d8e6ce0537d6f20079a40 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 28 Mar 2017 10:30:53 +0100 Subject: [PATCH 40/43] return 404 on project details not found --- .../coffee/Features/Project/ProjectApiController.coffee | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectApiController.coffee b/services/web/app/coffee/Features/Project/ProjectApiController.coffee index b16991ac62..715ccf19ae 100644 --- a/services/web/app/coffee/Features/Project/ProjectApiController.coffee +++ b/services/web/app/coffee/Features/Project/ProjectApiController.coffee @@ -1,4 +1,5 @@ ProjectDetailsHandler = require("./ProjectDetailsHandler") +Errors = require("../Errors/Errors") logger = require("logger-sharelatex") @@ -7,8 +8,11 @@ module.exports = getProjectDetails : (req, res)-> {project_id} = req.params ProjectDetailsHandler.getDetails project_id, (err, projDetails)-> - if err? + if err? and err instanceof Errors.NotFoundError + return res.sendStatus 404 + else if err? logger.log err:err, project_id:project_id, "something went wrong getting project details" return res.sendStatus 500 - res.json(projDetails) + else + res.json(projDetails) From 835d8d618d21f0bd9098953c0570f589654915f9 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 28 Mar 2017 11:33:37 +0100 Subject: [PATCH 41/43] use error handler --- .../Features/Project/ProjectApiController.coffee | 12 +++--------- .../coffee/Project/ProjectApiControllerTests.coffee | 9 ++++----- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectApiController.coffee b/services/web/app/coffee/Features/Project/ProjectApiController.coffee index 715ccf19ae..f832a75b54 100644 --- a/services/web/app/coffee/Features/Project/ProjectApiController.coffee +++ b/services/web/app/coffee/Features/Project/ProjectApiController.coffee @@ -1,18 +1,12 @@ ProjectDetailsHandler = require("./ProjectDetailsHandler") -Errors = require("../Errors/Errors") logger = require("logger-sharelatex") module.exports = - getProjectDetails : (req, res)-> + getProjectDetails : (req, res, next)-> {project_id} = req.params ProjectDetailsHandler.getDetails project_id, (err, projDetails)-> - if err? and err instanceof Errors.NotFoundError - return res.sendStatus 404 - else if err? - logger.log err:err, project_id:project_id, "something went wrong getting project details" - return res.sendStatus 500 - else - res.json(projDetails) + return next(err) if err? + res.json(projDetails) diff --git a/services/web/test/UnitTests/coffee/Project/ProjectApiControllerTests.coffee b/services/web/test/UnitTests/coffee/Project/ProjectApiControllerTests.coffee index 9476a80019..ddf23c0bac 100644 --- a/services/web/test/UnitTests/coffee/Project/ProjectApiControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Project/ProjectApiControllerTests.coffee @@ -20,6 +20,7 @@ describe 'Project api controller', -> session: destroy:sinon.stub() @res = {} + @next = sinon.stub() @projDetails = {name:"something"} @@ -34,9 +35,7 @@ describe 'Project api controller', -> @controller.getProjectDetails @req, @res - it "should send a 500 if there is an error", (done)-> + it "should send a 500 if there is an error", ()-> @ProjectDetailsHandler.getDetails.callsArgWith(1, "error") - @res.sendStatus = (resCode)=> - resCode.should.equal 500 - done() - @controller.getProjectDetails @req, @res + @controller.getProjectDetails @req, @res, @next + @next.calledWith("error").should.equal true From 08699d7aa265ad0622a0ef82cb00ec0ac892aeba Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 28 Mar 2017 15:46:58 +0100 Subject: [PATCH 42/43] Handle a 404 from Recurly if account doesn't exist --- .../Subscription/RecurlyWrapper.coffee | 6 +++- .../Subscription/RecurlyWrapperTests.coffee | 32 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Subscription/RecurlyWrapper.coffee b/services/web/app/coffee/Features/Subscription/RecurlyWrapper.coffee index ce40847a27..fddd4f3567 100644 --- a/services/web/app/coffee/Features/Subscription/RecurlyWrapper.coffee +++ b/services/web/app/coffee/Features/Subscription/RecurlyWrapper.coffee @@ -475,9 +475,13 @@ module.exports = RecurlyWrapper = url: "accounts/#{account_id}/subscriptions" qs: state: "active" + expect404: true }, (error, response, body) -> return callback(error) if error? - RecurlyWrapper._parseSubscriptionsXml body, callback + if response.statusCode == 404 + return callback null, [] + else + RecurlyWrapper._parseSubscriptionsXml body, callback _parseSubscriptionsXml: (xml, callback) -> RecurlyWrapper._parseXmlAndGetAttribute xml, "subscriptions", callback diff --git a/services/web/test/UnitTests/coffee/Subscription/RecurlyWrapperTests.coffee b/services/web/test/UnitTests/coffee/Subscription/RecurlyWrapperTests.coffee index c55efdb3c5..d951653310 100644 --- a/services/web/test/UnitTests/coffee/Subscription/RecurlyWrapperTests.coffee +++ b/services/web/test/UnitTests/coffee/Subscription/RecurlyWrapperTests.coffee @@ -1030,3 +1030,35 @@ describe "RecurlyWrapper", -> @call (err, result) => expect(err).to.be.instanceof Error done() + + describe "listAccountActiveSubscriptions", -> + beforeEach -> + @user_id = "mock-user-id" + @callback = sinon.stub() + @RecurlyWrapper.apiRequest = sinon.stub().yields(null, @response = {"mock": "response"}, @body = "") + @RecurlyWrapper._parseSubscriptionsXml = sinon.stub().yields(null, @subscriptions = ["mock", "subscriptions"]) + + describe "with an account", -> + beforeEach -> + @RecurlyWrapper.listAccountActiveSubscriptions @user_id, @callback + + it "should send a request to Recurly", -> + @RecurlyWrapper.apiRequest + .calledWith({ + url: "accounts/#{@user_id}/subscriptions" + qs: + state: "active" + expect404: true + }) + .should.equal true + + it "should return the subscriptions", -> + @callback.calledWith(null, @subscriptions).should.equal true + + describe "without an account", -> + beforeEach -> + @response.statusCode = 404 + @RecurlyWrapper.listAccountActiveSubscriptions @user_id, @callback + + it "should return an empty array of subscriptions", -> + @callback.calledWith(null, []).should.equal true \ No newline at end of file From 13492c7fc4e99af7790feb21c116c9c502a40c19 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 29 Mar 2017 15:27:10 +0100 Subject: [PATCH 43/43] handle the \feynmandiagram command in code check pulled from our ace repository https://github.com/sharelatex/ace commit baeb9aff561d048b8a839683261ffdf149ecd4ef --- services/web/public/js/ace-1.2.5/worker-latex.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/web/public/js/ace-1.2.5/worker-latex.js b/services/web/public/js/ace-1.2.5/worker-latex.js index 3fa9bceb07..ccd7e2da9b 100644 --- a/services/web/public/js/ace-1.2.5/worker-latex.js +++ b/services/web/public/js/ace-1.2.5/worker-latex.js @@ -1931,7 +1931,7 @@ var InterpretTokens = function (TokeniseResult, ErrorReporter) { if (newPos === null) { continue; } else {i = newPos;}; } else if (seq === "hbox" || seq === "text" || seq === "mbox" || seq === "footnote" || seq === "intertext" || seq === "shortintertext" || seq === "textnormal" || seq === "tag" || seq === "reflectbox" || seq === "textrm") { nextGroupMathMode = false; - } else if (seq === "rotatebox" || seq === "scalebox") { + } else if (seq === "rotatebox" || seq === "scalebox" || seq == "feynmandiagram") { newPos = readOptionalGeneric(TokeniseResult, i); if (newPos === null) { /* do nothing */ } else {i = newPos;}; newPos = readDefinition(TokeniseResult, i); @@ -2179,7 +2179,7 @@ var EnvHandler = function (ErrorReporter) { this._beginMathMode = function (thisEnv) { var currentMathMode = this.getMathMode(); // undefined, null, $, $$, name of mathmode env if (currentMathMode) { - ErrorFrom(thisEnv, thisEnv.name + " used inside existing math mode " + getName(currentMathMode), + ErrorFrom(thisEnv, getName(thisEnv) + " used inside existing math mode " + getName(currentMathMode), {suppressIfEditing:true, errorAtStart: true, mathMode:true}); }; thisEnv.mathMode = thisEnv;