Add Project#get_issue

app/models/project.rb

@@ -329,14 +329,18 @@ class Project < ActiveRecord::Base
     self.id
   end
-  def issue_exists?(issue_id)
+  def get_issue(issue_id)
     if default_issues_tracker?
-      self.issues.where(iid: issue_id).first.present?
+      issues.find_by(iid: issue_id)
     else
-      true
+      ExternalIssue.new(issue_id, self)
     end
   end
+  def issue_exists?(issue_id)
+    get_issue(issue_id).present?
+  end
+
   def default_issue_tracker
     gitlab_issue_tracker_service || create_gitlab_issue_tracker_service
   end
@@ -350,11 +354,7 @@ class Project < ActiveRecord::Base
   end
   def default_issues_tracker?
-    if external_issue_tracker
-      false
-    else
-      true
-    end
+    external_issue_tracker.blank?
   end
   def external_issues_trackers

lib/gitlab/markdown/issue_reference_filter.rb

@@ -44,21 +44,20 @@ module Gitlab
       # Returns a String with `#123` references replaced with links. All links
       # have `gfm` and `gfm-issue` class names attached for styling.
       def issue_link_filter(text)
-        self.class.references_in(text) do |match, issue, project_ref|
+        self.class.references_in(text) do |match, id, project_ref|
           project = self.project_from_ref(project_ref)
-          if project && project.issue_exists?(issue)
-            # FIXME (rspeicher): Law of Demeter
-            push_result(:issue, project.issues.where(iid: issue).first)
-            url = url_for_issue(issue, project, only_path: context[:only_path])
-            title = escape_once("Issue: #{title_for_issue(issue, project)}")
+          if project && issue = project.get_issue(id)
+            push_result(:issue, issue)
+            url = url_for_issue(id, project, only_path: context[:only_path])
+            title = escape_once("Issue: #{title_for_issue(id, project)}")
             klass = reference_class(:issue)
             %(<a href="#{url}"
                  title="#{title}"
-                 class="#{klass}">#{project_ref}##{issue}</a>)
+                 class="#{klass}">#{project_ref}##{id}</a>)
           else
             match
           end

spec/lib/gitlab/markdown/issue_reference_filter_spec.rb

@@ -27,7 +27,7 @@ module Gitlab::Markdown
       let(:reference) { "##{issue.iid}" }
       it 'ignores valid references when using non-default tracker' do
-        expect(project).to receive(:issue_exists?).with(issue.iid).and_return(false)
+        expect(project).to receive(:get_issue).with(issue.iid).and_return(nil)
         exp = act = "Issue ##{issue.iid}"
         expect(filter(act).to_html).to eq exp
@@ -48,7 +48,7 @@ module Gitlab::Markdown
       it 'ignores invalid issue IDs' do
         exp = act = "Fixed ##{issue.iid + 1}"
-        expect(project).to receive(:issue_exists?).with(issue.iid + 1)
+        expect(project).to receive(:get_issue).with(issue.iid + 1).and_return(nil)
         expect(filter(act).to_html).to eq exp
       end
@@ -98,8 +98,8 @@ module Gitlab::Markdown
         before { allow_cross_reference! }
         it 'ignores valid references when cross-reference project uses external tracker' do
-          expect_any_instance_of(Project).to receive(:issue_exists?).
-            with(issue.iid).and_return(false)
+          expect_any_instance_of(Project).to receive(:get_issue).
+            with(issue.iid).and_return(nil)
           exp = act = "Issue ##{issue.iid}"
           expect(filter(act).to_html).to eq exp

[Douwe Maan]

Are there other places that could use this new get_issue method?

[Douwe Maan]

I dislike using present? and blank? with anything but strings. With strings it's useful to check if the string is, well, blank, but with anything else it's behaviorally identical to not having present? there at all, or prepending ! rather than using blank?.

Per Ruby semantics, there's no good reason to prefer actual true or false values over truthy or falsy values. A predicate method (one ending in ?) is completely fine returning object/nil, or true/false. See the answers here: http://stackoverflow.com/questions/10527997/should-a-method-ending-in-question-mark-return-only-a-boolean and the discussion here: https://github.com/rails/rails/pull/5582

[Douwe Maan]

@rspeicher If you want to make me really happy, remove the .present? on line 341 as well ;)

[Robert Speicher]

@DouweM Done.

[Douwe Maan]

Using title_for_issue does a pointless extra lookup. Just use issue.title and implement title on ExternalIssue to return External issue #{id} or whatever.