From 0e928a9aeb50a66bf5abf2f46c0c3df9c2624a47 Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 6 Jan 2017 14:14:47 +0100 Subject: [PATCH] Don't fill user details --- .../Features/Messages/MessageFormatter.coffee | 6 +- .../Messages/MessageHttpController.coffee | 23 ++---- .../Features/Messages/MessageManager.coffee | 36 --------- .../Features/Users/UserFormatter.coffee | 19 ----- .../Features/WebApi/WebApiManager.coffee | 43 ---------- .../coffee/GettingMessagesTests.coffee | 78 ++----------------- .../coffee/ResolvingAThreadTests.coffee | 16 +--- .../coffee/SendingAMessageTests.coffee | 15 +--- .../coffee/helpers/MockWebApi.coffee | 27 ------- 9 files changed, 21 insertions(+), 242 deletions(-) delete mode 100644 services/chat/app/coffee/Features/Users/UserFormatter.coffee delete mode 100644 services/chat/app/coffee/Features/WebApi/WebApiManager.coffee delete mode 100644 services/chat/test/acceptance/coffee/helpers/MockWebApi.coffee diff --git a/services/chat/app/coffee/Features/Messages/MessageFormatter.coffee b/services/chat/app/coffee/Features/Messages/MessageFormatter.coffee index af2472973d..012f1f8882 100644 --- a/services/chat/app/coffee/Features/Messages/MessageFormatter.coffee +++ b/services/chat/app/coffee/Features/Messages/MessageFormatter.coffee @@ -1,5 +1,3 @@ -UserFormatter = require "../Users/UserFormatter" - module.exports = MessageFormatter = formatMessageForClientSide: (message) -> if message._id? @@ -9,7 +7,7 @@ module.exports = MessageFormatter = id: message.id content: message.content timestamp: message.timestamp - user: UserFormatter.formatUserForClientSide(message.user) + user_id: message.user_id return formattedMessage formatMessagesForClientSide: (messages) -> @@ -30,7 +28,7 @@ module.exports = MessageFormatter = if room.resolved? thread.resolved = true thread.resolved_at = room.resolved.ts - thread.resolved_by_user = UserFormatter.formatUserForClientSide(room.resolved.user) + thread.resolved_by_user_id = room.resolved.user_id threads[thread_id] = thread return thread diff --git a/services/chat/app/coffee/Features/Messages/MessageHttpController.coffee b/services/chat/app/coffee/Features/Messages/MessageHttpController.coffee index db582f3ba8..d36206afe9 100644 --- a/services/chat/app/coffee/Features/Messages/MessageHttpController.coffee +++ b/services/chat/app/coffee/Features/Messages/MessageHttpController.coffee @@ -25,10 +25,8 @@ module.exports = MessageHttpController = room_ids = rooms.map (r) -> r._id MessageManager.findAllMessagesInRooms room_ids, (error, messages) -> return next(error) if error? - MessageManager.populateMessagesAndRoomsWithUsers messages, rooms, (error) -> - return next(error) if error? - threads = MessageFormatter.groupMessagesByThreads rooms, messages - res.json threads + threads = MessageFormatter.groupMessagesByThreads rooms, messages + res.json threads resolveThread: (req, res, next) -> {project_id, thread_id} = req.params @@ -55,12 +53,9 @@ module.exports = MessageHttpController = return next(error) if error? MessageManager.createMessage thread._id, user_id, content, Date.now(), (error, message) -> return next(error) if error? - MessageManager.populateMessagesAndRoomsWithUsers [message], [], (error) -> - return next(error) if error? - message = MessageFormatter.formatMessageForClientSide(message) - message.room = - id: project_id - res.send(201, message) + message = MessageFormatter.formatMessageForClientSide(message) + message.room_id = project_id + res.send(201, message) _getMessages: (client_thread_id, req, res, next) -> {project_id} = req.params @@ -79,8 +74,6 @@ module.exports = MessageHttpController = logger.log {limit, before, project_id, client_thread_id, thread_object_id}, "found or created thread" MessageManager.getMessages thread_object_id, limit, before, (error, messages) -> return next(error) if error? - MessageManager.populateMessagesAndRoomsWithUsers messages, [], (error) -> - return next(error) if error? - messages = MessageFormatter.formatMessagesForClientSide messages - logger.log {project_id, messages}, "got messages" - res.send 200, messages + messages = MessageFormatter.formatMessagesForClientSide messages + logger.log {project_id, messages}, "got messages" + res.send 200, messages diff --git a/services/chat/app/coffee/Features/Messages/MessageManager.coffee b/services/chat/app/coffee/Features/Messages/MessageManager.coffee index 5e8f0e3974..317721ea28 100644 --- a/services/chat/app/coffee/Features/Messages/MessageManager.coffee +++ b/services/chat/app/coffee/Features/Messages/MessageManager.coffee @@ -1,7 +1,6 @@ mongojs = require "../../mongojs" db = mongojs.db ObjectId = mongojs.ObjectId -WebApiManager = require "../WebApi/WebApiManager" async = require "async" module.exports = MessageManager = @@ -28,41 +27,6 @@ module.exports = MessageManager = room_id: { $in: room_ids } }, callback - populateMessagesAndRoomsWithUsers: (messages, rooms, callback = (error) ->) -> - jobs = new Array() - - userCache = {} - getUserDetails = (user_id, callback = (error, user) ->) -> - return callback(null, userCache[user_id]) if userCache[user_id]? - WebApiManager.getUserDetails user_id, (error, user) -> - return callback(error) if error? - userCache[user_id] = user - callback null, user - - for message in messages - do (message) -> - if !message? - return - jobs.push (callback) -> - getUserDetails message.user_id.toString(), (error, user) -> - return callback(error) if error? - delete message.user_id - message.user = user - callback() - - for room in rooms - do (room) -> - if !room?.resolved?.user_id? - return - jobs.push (callback) -> - getUserDetails room.resolved.user_id.toString(), (error, user) -> - return callback(error) if error? - delete room.resolved.user_id - room.resolved.user = user - callback() - - async.series jobs, callback - _ensureIdsAreObjectIds: (query) -> if query.user_id? and query.user_id not instanceof ObjectId query.user_id = ObjectId(query.user_id) diff --git a/services/chat/app/coffee/Features/Users/UserFormatter.coffee b/services/chat/app/coffee/Features/Users/UserFormatter.coffee deleted file mode 100644 index 87af9b8d5d..0000000000 --- a/services/chat/app/coffee/Features/Users/UserFormatter.coffee +++ /dev/null @@ -1,19 +0,0 @@ -crypto = require "crypto" - -module.exports = UserFormatter = - formatUserForClientSide: (user) -> - return null if !user? - if user._id? - user.id = user._id.toString() - delete user._id - return { - id: user.id - first_name: user.first_name - last_name: user.last_name - email: user.email - gravatar_url: @_getGravatarUrlForEmail(user.email) - } - - _getGravatarUrlForEmail: (email) -> - hash = crypto.createHash("md5").update(email.toLowerCase()).digest("hex") - return "//www.gravatar.com/avatar/#{hash}" diff --git a/services/chat/app/coffee/Features/WebApi/WebApiManager.coffee b/services/chat/app/coffee/Features/WebApi/WebApiManager.coffee deleted file mode 100644 index 9f3960d612..0000000000 --- a/services/chat/app/coffee/Features/WebApi/WebApiManager.coffee +++ /dev/null @@ -1,43 +0,0 @@ -request = require('request').defaults(jar: false) -Settings = require("settings-sharelatex") - -# DEPRECATED! This method of getting user details via chat is deprecated -# in the way we lay out our services. -# Instead, web should be responsible for collecting the raw data (user_ids) and -# filling it out with calls to other services. All API calls should create a -# tree-like structure as much as possible, with web as the root. -module.exports = WebApiManager = - apiRequest: (url, method, options = {}, callback = (error, result) ->) -> - if typeof options == "function" - callback = options - options = {} - url = "#{Settings.apis.web.url}#{url}" - options.url = url - options.method = method - request options, (error, response, body) -> - return callback(error) if error? - if 200 <= response.statusCode < 300 - try - result = JSON.parse(body) - catch e - return callback(e) - return callback null, result - else - error = new Error("web api returned non-success code: #{response.statusCode}") - error.statusCode = response.statusCode - return callback error - - getUserDetails: (user_id, callback = (error, details) ->) -> - @apiRequest "/user/#{user_id}/personal_info", "get", { - auth: - user: Settings.apis.web.user - pass: Settings.apis.web.pass - sendImmediately: true - }, (error, data) -> - if error? - if error.statusCode == 404 - return callback null, null - else - return callback error - else - return callback null, data diff --git a/services/chat/test/acceptance/coffee/GettingMessagesTests.coffee b/services/chat/test/acceptance/coffee/GettingMessagesTests.coffee index 06489ee04c..6d622b409a 100644 --- a/services/chat/test/acceptance/coffee/GettingMessagesTests.coffee +++ b/services/chat/test/acceptance/coffee/GettingMessagesTests.coffee @@ -3,7 +3,6 @@ expect = require("chai").expect async = require "async" crypto = require "crypto" -MockWebApi = require "./helpers/MockWebApi" ChatClient = require "./helpers/ChatClient" describe "Getting messages", -> @@ -12,18 +11,6 @@ describe "Getting messages", -> @user_id2 = ObjectId().toString() @content1 = "foo bar" @content2 = "hello world" - MockWebApi.addUser @user_id1, @user1 = { - id: @user_id1 - first_name: "Jane" - last_name: "Smith" - email: "jane@example.com" - } - MockWebApi.addUser @user_id2, @user2 = { - id: @user_id2 - first_name: "John" - last_name: "Doe" - email: "john@example.com" - } describe "globally", -> before (done) -> @@ -38,21 +25,9 @@ describe "Getting messages", -> expect(messages.length).to.equal 2 messages.reverse() expect(messages[0].content).to.equal @content1 - expect(messages[0].user).to.deep.equal { - id: @user_id1 - first_name: "Jane" - last_name: "Smith" - email: "jane@example.com" - gravatar_url: "//www.gravatar.com/avatar/#{crypto.createHash("md5").update("jane@example.com").digest("hex")}" - } + expect(messages[0].user_id).to.equal @user_id1 expect(messages[1].content).to.equal @content2 - expect(messages[1].user).to.deep.equal { - id: @user_id2 - first_name: "John" - last_name: "Doe" - email: "john@example.com" - gravatar_url: "//www.gravatar.com/avatar/#{crypto.createHash("md5").update("john@example.com").digest("hex")}" - } + expect(messages[1].user_id).to.equal @user_id2 done() describe "from all the threads", -> @@ -76,53 +51,12 @@ describe "Getting messages", -> expect(thread2.messages.length).to.equal 2 expect(thread1.messages[0].content).to.equal "one" - expect(thread1.messages[0].user).to.deep.equal { - id: @user_id1 - first_name: "Jane" - last_name: "Smith" - email: "jane@example.com" - gravatar_url: "//www.gravatar.com/avatar/#{crypto.createHash("md5").update("jane@example.com").digest("hex")}" - } + expect(thread1.messages[0].user_id).to.equal @user_id1 expect(thread1.messages[1].content).to.equal "three" - expect(thread1.messages[1].user).to.deep.equal { - id: @user_id1 - first_name: "Jane" - last_name: "Smith" - email: "jane@example.com" - gravatar_url: "//www.gravatar.com/avatar/#{crypto.createHash("md5").update("jane@example.com").digest("hex")}" - } + expect(thread1.messages[1].user_id).to.equal @user_id1 expect(thread2.messages[0].content).to.equal "two" - expect(thread2.messages[0].user).to.deep.equal { - id: @user_id2 - first_name: "John" - last_name: "Doe" - email: "john@example.com" - gravatar_url: "//www.gravatar.com/avatar/#{crypto.createHash("md5").update("john@example.com").digest("hex")}" - } + expect(thread2.messages[0].user_id).to.equal @user_id2 expect(thread2.messages[1].content).to.equal "four" - expect(thread2.messages[1].user).to.deep.equal { - id: @user_id2 - first_name: "John" - last_name: "Doe" - email: "john@example.com" - gravatar_url: "//www.gravatar.com/avatar/#{crypto.createHash("md5").update("john@example.com").digest("hex")}" - } + expect(thread2.messages[1].user_id).to.equal @user_id2 done() - - describe "when a user doesn't exit", -> - before (done) -> - @project_id = ObjectId().toString() - @user_id3 = ObjectId().toString() - async.series [ - (cb) => ChatClient.sendGlobalMessage @project_id, @user_id3, @content1, cb - (cb) => ChatClient.sendGlobalMessage @project_id, @user_id2, @content2, cb - ], done - - it "should just return null for the user", (done) -> - ChatClient.getGlobalMessages @project_id, (error, response, messages) => - expect(messages.length).to.equal 2 - messages.reverse() - expect(messages[0].content).to.equal @content1 - expect(messages[0].user).to.equal null - done() \ No newline at end of file diff --git a/services/chat/test/acceptance/coffee/ResolvingAThreadTests.coffee b/services/chat/test/acceptance/coffee/ResolvingAThreadTests.coffee index 1b83b839a4..4f666666ac 100644 --- a/services/chat/test/acceptance/coffee/ResolvingAThreadTests.coffee +++ b/services/chat/test/acceptance/coffee/ResolvingAThreadTests.coffee @@ -2,20 +2,12 @@ expect = require("chai").expect crypto = require "crypto" -MockWebApi = require "./helpers/MockWebApi" ChatClient = require "./helpers/ChatClient" describe "Resolving a thread", -> before -> @project_id = ObjectId().toString() @user_id = ObjectId().toString() - MockWebApi.addUser @user_id, @user = { - id: @user_id - first_name: "Jane" - last_name: "Smith" - email: "jane@example.com" - } - describe "with a resolved thread", -> before (done) -> @thread_id = ObjectId().toString() @@ -33,13 +25,7 @@ describe "Resolving a thread", -> expect(error).to.be.null expect(response.statusCode).to.equal 200 expect(threads[@thread_id].resolved).to.equal true - expect(threads[@thread_id].resolved_by_user).to.deep.equal { - id: @user_id - first_name: "Jane" - last_name: "Smith" - email: "jane@example.com" - gravatar_url: "//www.gravatar.com/avatar/#{crypto.createHash("md5").update("jane@example.com").digest("hex")}" - } + expect(threads[@thread_id].resolved_by_user_id).to.equal @user_id resolved_at = new Date(threads[@thread_id].resolved_at) expect(new Date() - resolved_at).to.be.below 1000 done() diff --git a/services/chat/test/acceptance/coffee/SendingAMessageTests.coffee b/services/chat/test/acceptance/coffee/SendingAMessageTests.coffee index fb1ea63d9a..6181d353f4 100644 --- a/services/chat/test/acceptance/coffee/SendingAMessageTests.coffee +++ b/services/chat/test/acceptance/coffee/SendingAMessageTests.coffee @@ -1,7 +1,6 @@ {ObjectId} = require "../../../app/js/mongojs" expect = require("chai").expect -MockWebApi = require "./helpers/MockWebApi" ChatClient = require "./helpers/ChatClient" describe "Sending a message", -> @@ -9,12 +8,6 @@ describe "Sending a message", -> @project_id = ObjectId().toString() @user_id = ObjectId().toString() @thread_id = ObjectId().toString() - MockWebApi.addUser @user_id, @user = { - id: @user_id - first_name: "Jane" - last_name: "Smith" - email: "jane@example.com" - } describe "globally", -> before (done) -> @@ -23,8 +16,8 @@ describe "Sending a message", -> expect(error).to.be.null expect(response.statusCode).to.equal 201 expect(body.content).to.equal @content - expect(body.user.id).to.equal @user_id - expect(body.room.id).to.equal @project_id + expect(body.user_id).to.equal @user_id + expect(body.room_id).to.equal @project_id done() it "should then list the message in the project messages", (done) -> @@ -42,8 +35,8 @@ describe "Sending a message", -> expect(error).to.be.null expect(response.statusCode).to.equal 201 expect(body.content).to.equal @content - expect(body.user.id).to.equal @user_id - expect(body.room.id).to.equal @project_id + expect(body.user_id).to.equal @user_id + expect(body.room_id).to.equal @project_id done() it "should then list the message in the threads", (done) -> diff --git a/services/chat/test/acceptance/coffee/helpers/MockWebApi.coffee b/services/chat/test/acceptance/coffee/helpers/MockWebApi.coffee deleted file mode 100644 index 12d9c4f243..0000000000 --- a/services/chat/test/acceptance/coffee/helpers/MockWebApi.coffee +++ /dev/null @@ -1,27 +0,0 @@ -express = require("express") -app = express() - -module.exports = MockWebApi = - users: {} - - addUser: (user_id, user) -> - @users[user_id] = user - - getUser: (user_id, callback = (error, user) ->) -> - return callback null, @users[user_id] - - run: () -> - app.get "/user/:user_id/personal_info", (req, res, next) => - @getUser req.params.user_id, (error, user) -> - if error? - res.send 500 - else if user? - res.send JSON.stringify user - else - res.send 404 - - app.listen 3000, (error) -> - throw error if error? - -MockWebApi.run() -