Use Slack display names instead of the "name" field (#29)

* Use Slack display names instead of the "name" field

The "name" field is the left-hand side of the email address in many
cases.  This information is not otherwise available so we shouldn't
expose it in forum posts.

* Fall back to real_name, add comments

* Store users as a hash

This should avoid a lot of O(n) lookups

* Format user mentions with the correct name also

* Fix the tests (first try)

* Fix the tests (second try); add a test for user mentions

* Fix the tests (third try)

* Empty commit to trigger Travis

* Fix the tests (fourth try)

* Fix the tests (fifth try)

* Change spaces to underscores

* Updates per feedback

lib/discourse_chat/provider/slack/slack_message.rb

@@ -9,9 +9,10 @@ module DiscourseChat::Provider::SlackProvider
 
     def username
       if user
-        user['name']
+        user["_transcript_username"]
       elsif @raw.key?("username")
-        @raw["username"]
+        # This is for bot messages
+        @raw["username"].gsub(' ', '_')
       end
     end
 
@@ -36,8 +37,13 @@ module DiscourseChat::Provider::SlackProvider
         text = parts.length > 1 ? parts[1] : parts[0]
 
         if parts[0].start_with?('@')
-          user = @transcript.users.find { |u| u["id"] == parts[0].gsub('@', '') }
+          user_id = parts[0][1..-1]
-          next "@#{user['name']}"
+          if user = @transcript.users[user_id]
+            user_name = user['_transcript_username']
+          else
+            user_name = user_id
+          end
+          next "@#{user_name}"
         end
 
         "[#{text}](#{link})"
@@ -91,7 +97,7 @@ module DiscourseChat::Provider::SlackProvider
 
     def user
       return nil unless user_id = @raw["user"]
-      @transcript.users.find { |u| u["id"] == user_id }
+      @transcript.users[user_id]
     end
   end
 end

lib/discourse_chat/provider/slack/slack_transcript.rb

@@ -197,7 +197,7 @@ module DiscourseChat::Provider::SlackProvider
       cursor = nil
       req = Net::HTTP::Post.new(URI('https://slack.com/api/users.list'))
 
-      @users = []
+      @users = {}
       loop do
         break if cursor == ""
         req.set_form_data(token: SiteSetting.chat_integration_slack_access_token, limit: 200, cursor: cursor)
@@ -206,9 +206,18 @@ module DiscourseChat::Provider::SlackProvider
         json = JSON.parse(response.body)
         return false unless json['ok']
         cursor = json['response_metadata']['next_cursor']
-        @users << json['members']
+        json['members'].each do |user|
+          # Slack uses display_name and falls back to real_name if it is not set
+          if user['profile']['display_name'].blank?
+            user['_transcript_username'] = user['profile']['real_name']
+          else
+            user['_transcript_username'] = user['profile']['display_name']
+          end
+          user['_transcript_username'] = user['_transcript_username'].gsub(' ', '_')
+          @users[user['id']] = user
+        end
       end
-      @users&.flatten!
+      true
     end
 
     def load_chat_history(count: 500)

spec/lib/discourse_chat/provider/slack/slack_command_controller_spec.rb

@@ -197,7 +197,7 @@ describe 'Slack Command Controller', type: :request do
 
         context "with valid slack responses" do
           before do
-            stub_request(:post, "https://slack.com/api/users.list").to_return(body: '{"ok":true,"members":[{"id":"U5Z773QLS","name":"david","profile":{"icon_24":"https://example.com/avatar"}}],"response_metadata":{"next_cursor":""}}')
+            stub_request(:post, "https://slack.com/api/users.list").to_return(body: '{"ok":true,"members":[{"id":"U5Z773QLS","profile":{"display_name":"david","real_name":"david","icon_24":"https://example.com/avatar"}}],"response_metadata":{"next_cursor":""}}')
             stub_request(:post, "https://slack.com/api/conversations.history").to_return(body: { ok: true, messages: messages_fixture }.to_json)
           end
 

spec/lib/discourse_chat/provider/slack/slack_transcript_spec.rb

@@ -14,8 +14,8 @@ RSpec.describe DiscourseChat::Provider::SlackProvider::SlackTranscript do
       },
       {
           "type": "message",
-          "user": "U5Z773QLS",
+          "user": "U5Z773QLZ",
-          "text": "Oooh a new discourse plugin???",
+          "text": "Oooh a new discourse plugin <@U5Z773QLS> ???",
           "ts": "1501801643.056375"
       },
       {
@@ -62,10 +62,32 @@ RSpec.describe DiscourseChat::Provider::SlackProvider::SlackTranscript do
         {
             "type": "message",
             "user": "U5Z773QLS",
-            "text": "Let’s try some *bold text*",
+            "text": "Let’s try some *bold text* <@U5Z773QLZ> <@someotheruser>",
             "ts": "1501093331.439776"
         },
+    ]
+  }
 
+  let(:users_fixture) {
+    [
+      {
+        id: "U5Z773QLS",
+        name: "awesomeguyemail",
+        profile: {
+          image_24: "https://example.com/avatar",
+          display_name: "awesomeguy",
+          real_name: "actually just a guy"
+        }
+      },
+      {
+        id: "U5Z773QLZ",
+        name: "otherguyemail",
+        profile: {
+          image_24: "https://example.com/avatar",
+          display_name: "",
+          real_name: "another guy"
+        }
+      }
     ]
   }
 
@@ -78,7 +100,7 @@ RSpec.describe DiscourseChat::Provider::SlackProvider::SlackTranscript do
     it 'loads users correctly' do
       stub_request(:post, "https://slack.com/api/users.list")
         .with(body: { token: "abcde", "cursor": nil, "limit": "200" })
-        .to_return(status: 200, body: { ok: true, members: [{ id: "U5Z773QLS", name: "awesomeguy", profile: { image_24: "https://example.com/avatar" } }], response_metadata: { next_cursor: "" } }.to_json)
+        .to_return(status: 200, body: { ok: true, members: users_fixture, response_metadata: { next_cursor: "" } }.to_json)
 
       expect(transcript.load_user_data).to be_truthy
     end
@@ -101,7 +123,7 @@ RSpec.describe DiscourseChat::Provider::SlackProvider::SlackTranscript do
   context 'with loaded users' do
     before do
       stub_request(:post, "https://slack.com/api/users.list")
-        .to_return(status: 200, body: { ok: true, members: [{ id: "U5Z773QLS", name: "awesomeguy", profile: { image_24: "https://example.com/avatar" } }], response_metadata: { next_cursor: "" } }.to_json)
+        .to_return(status: 200, body: { ok: true, members: users_fixture, response_metadata: { next_cursor: "" } }.to_json)
       transcript.load_user_data
     end
 
@@ -166,7 +188,7 @@ RSpec.describe DiscourseChat::Provider::SlackProvider::SlackTranscript do
       end
 
       it 'handles bold text' do
-        expect(transcript.messages.first.text).to eq("Let’s try some **bold text**")
+        expect(transcript.messages.first.text).to start_with("Let’s try some **bold text** ")
       end
 
       it 'handles links' do
@@ -221,8 +243,19 @@ RSpec.describe DiscourseChat::Provider::SlackProvider::SlackTranscript do
 
       it 'handles usernames correctly' do
         expect(transcript.first_message.username).to eq('awesomeguy') # Normal user
+        expect(transcript.messages[1].username).to eq('Test_Community') # Bot user
         expect(transcript.messages[2].username).to eq(nil) # Unknown normal user
-        expect(transcript.messages[1].username).to eq('Test Community') # Bot user
+        # Normal user, display_name not set (fall back to real_name)
+        expect(transcript.messages[4].username).to eq('another_guy')
+      end
+
+      it 'handles user mentions correctly' do
+        # User with display_name not set, unrecognized user
+        expect(transcript.first_message.text).to \
+          eq('Let’s try some **bold text** @another_guy @someotheruser')
+        # Normal user
+        expect(transcript.messages[4].text).to \
+          eq('Oooh a new discourse plugin @awesomeguy ???')
       end
 
       it 'handles avatars correctly' do
@@ -241,9 +274,9 @@ RSpec.describe DiscourseChat::Provider::SlackProvider::SlackTranscript do
         [quote]
         [**View in #general on Slack**](https://slack.com/archives/G1234/p1501093331439776)
 
-        ![awesomeguy] **@awesomeguy:** Let’s try some **bold text**
+        ![awesomeguy] **@awesomeguy:** Let’s try some **bold text** @another_guy @someotheruser
 
-        **@Test Community:** 
+        **@Test_Community:** 
         > Discourse can now be integrated with Mattermost! - @david
 
         [/quote]