Improve handling with new mention syntax without name

* fallback to diaspora ID when person is not found
* fix filter_people when the person is not found
* write some more tests

lib/diaspora/mentionable.rb

@@ -14,7 +14,7 @@ module Diaspora::Mentionable
   def self.mention_attrs(mention_str)
     name, diaspora_id = mention_str.match(REGEX).captures
 
-    [name.try(:strip), diaspora_id.strip]
+    [name.try(:strip).presence, diaspora_id.strip]
   end
 
   # takes a message text and returns the text with mentions in (html escaped)
@@ -32,7 +32,7 @@ module Diaspora::Mentionable
       name, diaspora_id = mention_attrs(match_str)
       person = people.find {|p| p.diaspora_handle == diaspora_id }
 
-      ERB::Util.h(MentionsInternal.mention_link(person, name, opts))
+      ERB::Util.h(MentionsInternal.mention_link(person, name, diaspora_id, opts))
     }
   end
 
@@ -60,9 +60,12 @@ module Diaspora::Mentionable
     msg_text.to_s.gsub(REGEX) {|match_str|
       name, diaspora_id = mention_attrs(match_str)
       person = mentioned_ppl.find {|p| p.diaspora_handle == diaspora_id }
-      mention = MentionsInternal.profile_link(person, name) unless allowed_people.include?(person.id)
 
-      mention || match_str
+      if person && allowed_people.include?(person.id)
+        match_str
+      else
+        MentionsInternal.profile_link(person, name, diaspora_id)
+      end
     }
   end
 
@@ -97,11 +100,11 @@ module Diaspora::Mentionable
     # @param [Person] AR Person
     # @param [String] display name
     # @param [Hash] formatting options
-    def self.mention_link(person, display_name, opts)
+    def self.mention_link(person, display_name, diaspora_id, opts)
-      return display_name unless person.present?
+      return display_name || diaspora_id unless person.present?
 
       if opts[:plain_text]
-        display_name.presence || person.name
+        display_name || person.name
       else
         person_link(person, class: PERSON_HREF_CLASS, display_name: display_name)
       end
@@ -113,10 +116,10 @@ module Diaspora::Mentionable
     # @param [Person] AR Person
     # @param [String] display name
     # @return [String] markdown person link
-    def self.profile_link(person, display_name)
+    def self.profile_link(person, display_name, diaspora_id)
-      return display_name unless person.present?
+      return display_name || diaspora_id unless person.present?
 
-      "[#{display_name.presence || person.name}](#{local_or_remote_person_path(person)})"
+      "[#{display_name || person.name}](#{local_or_remote_person_path(person)})"
     end
   end
 end

spec/lib/diaspora/mentionable_spec.rb

@@ -1,66 +1,74 @@
 describe Diaspora::Mentionable do
   include PeopleHelper
 
-  before do
+  let(:people) { [alice, bob, eve].map(&:person) }
-    @people = [alice, bob, eve].map(&:person)
+  let(:names) { %w(Alice\ A Bob\ B "Eve>\ E) }
-    @names = %w(Alice\ A Bob\ B "Eve>\ E)
+
-    @test_txt = <<-STR
+  let(:test_text_with_names) { <<-STR }
 This post contains a lot of mentions
-one @{#{@names[0]}; #{@people[0].diaspora_handle}},
+one @{#{names[0]}; #{people[0].diaspora_handle}},
-two @{#{@names[1]}; #{@people[1].diaspora_handle}} and finally
+two @{#{names[1]}; #{people[1].diaspora_handle}} and finally
-three @{#{@names[2]}; #{@people[2].diaspora_handle}}.
+three @{#{names[2]}; #{people[2].diaspora_handle}}.
 STR
-    @test_txt_plain = <<-STR
+
+  let(:test_text_without_names) { <<-STR }
 This post contains a lot of mentions
-one Alice A,
+one @{#{people[0].diaspora_handle}},
-two Bob B and finally
+two @{#{people[1].diaspora_handle}} and finally
-three &quot;Eve&gt; E.
+three @{#{people[2].diaspora_handle}}.
 STR
-  end
 
   describe ".mention_attrs" do
     it "returns name and diaspora ID" do
-      name, diaspora_id = Diaspora::Mentionable.mention_attrs("@{#{@names[0]}; #{@people[0].diaspora_handle}}")
+      name, diaspora_id = Diaspora::Mentionable.mention_attrs("@{#{names[0]}; #{people[0].diaspora_handle}}")
-      expect(name).to eq(@names[0])
+      expect(name).to eq(names[0])
-      expect(diaspora_id).to eq(@people[0].diaspora_handle)
+      expect(diaspora_id).to eq(people[0].diaspora_handle)
     end
 
     it "returns only diaspora-ID when no name is included" do
-      name, diaspora_id = Diaspora::Mentionable.mention_attrs("@{#{@people[0].diaspora_handle}}")
+      name, diaspora_id = Diaspora::Mentionable.mention_attrs("@{#{people[0].diaspora_handle}}")
-      expect(diaspora_id).to eq(@people[0].diaspora_handle)
+      expect(diaspora_id).to eq(people[0].diaspora_handle)
       expect(name).to be_nil
     end
 
     it "trims the name if available" do
-      name, diaspora_id = Diaspora::Mentionable.mention_attrs("@{#{@names[0]} ; #{@people[0].diaspora_handle}}")
+      name, diaspora_id = Diaspora::Mentionable.mention_attrs("@{#{names[0]} ; #{people[0].diaspora_handle}}")
-      expect(name).to eq(@names[0])
+      expect(name).to eq(names[0])
-      expect(diaspora_id).to eq(@people[0].diaspora_handle)
+      expect(diaspora_id).to eq(people[0].diaspora_handle)
     end
   end
 
   describe ".format" do
     context "html output" do
       it "adds the links to the formatted message" do
-        fmt_msg = Diaspora::Mentionable.format(@test_txt, @people)
+        fmt_msg = Diaspora::Mentionable.format(test_text_with_names, people)
 
-        [@people, @names].transpose.each do |person, name|
+        [people, names].transpose.each do |person, name|
           expect(fmt_msg).to include person_link(person, class: "mention hovercardable", display_name: name)
         end
       end
 
-      it "should work correct when message is escaped html" do
+      it "adds the links to the formatted message and uses the names from the people" do
-        fmt_msg = Diaspora::Mentionable.format(CGI.escapeHTML(@test_txt), @people)
+        fmt_msg = Diaspora::Mentionable.format(test_text_without_names, people)
 
-        [@people, @names].transpose.each do |person, name|
+        people.each do |person|
+          expect(fmt_msg).to include person_link(person, class: "mention hovercardable", display_name: person.name)
+        end
+      end
+
+      it "should work correct when message is escaped html" do
+        fmt_msg = Diaspora::Mentionable.format(CGI.escapeHTML(test_text_with_names), people)
+
+        [people, names].transpose.each do |person, name|
           expect(fmt_msg).to include person_link(person, class: "mention hovercardable", display_name: name)
         end
       end
 
       it "escapes the link title (name)" do
         name = "</a><script>alert('h')</script>"
-        test_txt = "two @{#{name}; #{@people[0].diaspora_handle}} and finally"
+        test_txt = "two @{#{name}; #{people[0].diaspora_handle}} and finally"
 
-        fmt_msg = Diaspora::Mentionable.format(test_txt, @people)
+        fmt_msg = Diaspora::Mentionable.format(test_txt, people)
 
         expect(fmt_msg).not_to include(name)
         expect(fmt_msg).to include("&gt;", "&lt;", "&#39;") # ">", "<", "'"
@@ -69,9 +77,9 @@ STR
 
     context "plain text output" do
       it "removes mention markup and displays unformatted name" do
-        fmt_msg = Diaspora::Mentionable.format(@test_txt, @people, plain_text: true)
+        fmt_msg = Diaspora::Mentionable.format(test_text_with_names, people, plain_text: true)
 
-        @names.each do |name|
+        names.each do |name|
           expect(fmt_msg).to include CGI.escapeHTML(name)
         end
         expect(fmt_msg).not_to include "<a", "</a>", "hovercardable"
@@ -79,27 +87,46 @@ STR
     end
 
     it "leaves the names of people that cannot be found" do
-      fmt_msg = Diaspora::Mentionable.format(@test_txt, [])
+      test_txt_plain = <<-STR
-      expect(fmt_msg).to eql @test_txt_plain
+This post contains a lot of mentions
+one Alice A,
+two Bob B and finally
+three &quot;Eve&gt; E.
+STR
+
+      fmt_msg = Diaspora::Mentionable.format(test_text_with_names, [])
+      expect(fmt_msg).to eql test_txt_plain
+    end
+
+    it "uses the diaspora ID when the person cannot be found" do
+      test_txt_plain = <<-STR
+This post contains a lot of mentions
+one #{people[0].diaspora_handle},
+two #{people[1].diaspora_handle} and finally
+three #{people[2].diaspora_handle}.
+STR
+
+      fmt_msg = Diaspora::Mentionable.format(test_text_without_names, [])
+      expect(fmt_msg).to eql test_txt_plain
     end
   end
 
   describe ".people_from_string" do
     it "extracts the mentioned people from the text" do
-      ppl = Diaspora::Mentionable.people_from_string(@test_txt)
+      ppl = Diaspora::Mentionable.people_from_string(test_text_with_names)
-      expect(ppl).to match_array(@people)
+      expect(ppl).to match_array(people)
     end
 
     it "extracts the mentioned people from the text without name" do
-      text = "test @{#{@people[0].diaspora_handle}} test"
+      text = "test @{#{people[0].diaspora_handle}} test"
       ppl = Diaspora::Mentionable.people_from_string(text)
-      expect(ppl).to match_array([@people[0]])
+      expect(ppl).to match_array([people[0]])
     end
 
     it "extracts the mentioned people from the text mixed mentions (with and without name)" do
-      text = "@{#{@people[0].diaspora_handle}} and @{#{@names[1]}; #{@people[1].diaspora_handle}}"
+      text = "@{#{people[0].diaspora_handle}} and @{#{names[1]}; #{people[1].diaspora_handle}}"
       ppl = Diaspora::Mentionable.people_from_string(text)
-      expect(ppl).to match_array([@people[0], @people[1]])
+      expect(ppl).to match_array([people[0], people[1]])
     end
 
     describe "returns an empty array if nobody was found" do
@@ -135,51 +162,55 @@ STR
   end
 
   describe ".filter_people" do
+    let(:user_a) { FactoryGirl.create(:user_with_aspect, username: "user_a") }
+    let(:user_b) { FactoryGirl.create(:user, username: "user_b") }
+    let(:user_c) { FactoryGirl.create(:user, username: "user_c") }
+
     before do
-      @user_a = FactoryGirl.create(:user_with_aspect, username: "user_a")
+      user_a.aspects.create!(name: "second")
-      @user_b = FactoryGirl.create(:user, username: "user_b")
-      @user_c = FactoryGirl.create(:user, username: "user_c")
 
-      @user_a.aspects.create!(name: "second")
+      user_a.share_with(user_b.person, user_a.aspects.where(name: "generic"))
-
+      user_a.share_with(user_c.person, user_a.aspects.where(name: "second"))
-      @mention_b = "@{user B; #{@user_b.diaspora_handle}}"
-      @mention_c = "@{user C; #{@user_c.diaspora_handle}}"
-
-      @user_a.share_with(@user_b.person, @user_a.aspects.where(name: "generic"))
-      @user_a.share_with(@user_c.person, @user_a.aspects.where(name: "second"))
-
-      @test_txt_b = "mentioning #{@mention_b}"
-      @test_txt_c = "mentioning #{@mention_c}"
-      @test_txt_bc = "mentioning #{@mention_b}} and #{@mention_c}"
     end
 
     it "filters mention, if contact is not in a given aspect" do
+      mention = "@{user C; #{user_c.diaspora_handle}}"
       txt = Diaspora::Mentionable.filter_people(
-        @test_txt_c,
+        "mentioning #{mention}",
-        @user_a.aspects.where(name: "generic").first.contacts.map(&:person_id)
+        user_a.aspects.where(name: "generic").first.contacts.map(&:person_id)
       )
 
       expect(txt).to include("user C")
-      expect(txt).to include(local_or_remote_person_path(@user_c.person))
+      expect(txt).to include(local_or_remote_person_path(user_c.person))
       expect(txt).not_to include("href")
-      expect(txt).not_to include(@mention_c)
+      expect(txt).not_to include(mention)
     end
 
     it "leaves mention, if contact is in a given aspect" do
+      mention = "@{user B; #{user_b.diaspora_handle}}"
       txt = Diaspora::Mentionable.filter_people(
-        @test_txt_b,
+        "mentioning #{mention}",
-        @user_a.aspects.where(name: "generic").first.contacts.map(&:person_id)
+        user_a.aspects.where(name: "generic").first.contacts.map(&:person_id)
       )
 
       expect(txt).to include("user B")
-      expect(txt).to include(@mention_b)
+      expect(txt).to include(mention)
+    end
+
+    it "works if the person cannot be found" do
+      expect(Person).to receive(:find_or_fetch_by_identifier).with("non_existing_user@example.org").and_return(nil)
+
+      mention = "@{non_existing_user@example.org}"
+      txt = Diaspora::Mentionable.filter_people("mentioning #{mention}", [])
+
+      expect(txt).to eq "mentioning non_existing_user@example.org"
     end
   end
 
   describe ".backport_mention_syntax" do
     it "replaces the new syntax with the old syntax" do
-      text = "mention @{#{@people[0].diaspora_handle}} text"
+      text = "mention @{#{people[0].diaspora_handle}} text"
-      expected_text = "mention @{#{@people[0].name}; #{@people[0].diaspora_handle}} text"
+      expected_text = "mention @{#{people[0].name}; #{people[0].diaspora_handle}} text"
       expect(Diaspora::Mentionable.backport_mention_syntax(text)).to eq(expected_text)
     end
 
@@ -192,7 +223,7 @@ STR
     end
 
     it "does not change the text, when the mention includes a name" do
-      text = "mention @{#{@names[0]}; #{@people[0].diaspora_handle}} text"
+      text = "mention @{#{names[0]}; #{people[0].diaspora_handle}} text"
       expect(Diaspora::Mentionable.backport_mention_syntax(text)).to eq(text)
     end
 

spec/lib/diaspora/message_renderer_spec.rb

@@ -202,6 +202,11 @@ describe Diaspora::MessageRenderer do
         msg = message("@{Alice; #{alice.diaspora_handle}} is cool", mentioned_people: alice.person)
         expect(msg.plain_text_without_markdown).to eq "Alice is cool"
       end
+
+      it "uses the diaspora ID when the person cannot be found" do
+        msg = message("@{#{alice.diaspora_handle}} is cool", mentioned_people: [])
+        expect(msg.plain_text_without_markdown).to eq "#{alice.diaspora_handle} is cool"
+      end
     end
   end