Merge branch '18027-cache-project-external_issue_tracker' into 'master'

CHANGELOG

@@ -62,6 +62,7 @@ v 8.9.0 (unreleased)
   - Markdown editor now correctly resets the input value on edit cancellation !4175
   - Toggling a task list item in a issue/mr description does not creates a Todo for mentions
   - Improved UX of date pickers on issue & milestone forms
+  - Cache on the database if a project has an active external issue tracker.
 
 v 8.8.5 (unreleased)
   - Ensure branch cleanup regardless of whether the GitHub import process succeeds

app/models/project.rb

@@ -523,9 +523,21 @@ class Project < ActiveRecord::Base
   end
 
   def external_issue_tracker
+    if has_external_issue_tracker.nil? # To populate existing projects
+      cache_has_external_issue_tracker
+    end
+
+    if has_external_issue_tracker?
       return @external_issue_tracker if defined?(@external_issue_tracker)
-    @external_issue_tracker ||=
-      services.issue_trackers.active.without_defaults.first
+
+      @external_issue_tracker = services.external_issue_trackers.first
+    else
+      nil
+    end
+  end
+
+  def cache_has_external_issue_tracker
+    update_column(:has_external_issue_tracker, services.external_issue_trackers.any?)
   end
 
   def can_have_issues_tracker_id?

app/models/service.rb

@@ -16,6 +16,7 @@ class Service < ActiveRecord::Base
   after_initialize :initialize_properties
 
   after_commit :reset_updated_properties
+  after_commit :cache_project_has_external_issue_tracker
 
   belongs_to :project
   has_one :service_hook
@@ -34,6 +35,7 @@ class Service < ActiveRecord::Base
   scope :note_hooks, -> { where(note_events: true, active: true) }
   scope :build_hooks, -> { where(build_events: true, active: true) }
   scope :wiki_page_hooks, -> { where(wiki_page_events: true, active: true) }
+  scope :external_issue_trackers, -> { issue_trackers.active.without_defaults }
 
   default_value_for :category, 'common'
 
@@ -192,4 +194,12 @@ class Service < ActiveRecord::Base
     service.project_id = project_id
     service if service.save
   end
+
+  private
+
+  def cache_project_has_external_issue_tracker
+    if project && !project.destroyed?
+      project.cache_has_external_issue_tracker
+    end
+  end
 end

db/migrate/20160603075128_add_has_external_issue_tracker_to_projects.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddHasExternalIssueTrackerToProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  def change
+    add_column(:projects, :has_external_issue_tracker, :boolean)
+  end
+end

db/schema.rb

@@ -780,6 +780,7 @@ ActiveRecord::Schema.define(version: 20160608155312) do
     t.datetime "last_repository_check_at"
     t.boolean  "container_registry_enabled"
     t.boolean  "only_allow_merge_if_build_succeeds", default: false,    null: false
+    t.boolean  "has_external_issue_tracker"
   end
 
   add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree

spec/lib/banzai/pipeline/wiki_pipeline_spec.rb

@@ -52,8 +52,8 @@ describe Banzai::Pipeline::WikiPipeline do
   end
 
   describe "Links" do
-    let(:namespace) { build_stubbed(:namespace, name: "wiki_link_ns") }
-    let(:project)   { build_stubbed(:empty_project, :public, name: "wiki_link_project", namespace: namespace) }
+    let(:namespace) { create(:namespace, name: "wiki_link_ns") }
+    let(:project)   { create(:empty_project, :public, name: "wiki_link_project", namespace: namespace) }
     let(:project_wiki) { ProjectWiki.new(project, double(:user)) }
     let(:page) { build(:wiki_page, wiki: project_wiki, page: OpenStruct.new(url_path: 'nested/twice/start-page')) }
 

spec/models/project_services/bamboo_service_spec.rb

@@ -194,7 +194,7 @@ describe BambooService, models: true do
 
   def service(bamboo_url: 'http://gitlab.com')
     described_class.create(
-      project: build_stubbed(:empty_project),
+      project: create(:empty_project),
       properties: {
         bamboo_url: bamboo_url,
         username: 'mic',

spec/models/project_services/teamcity_service_spec.rb

@@ -182,7 +182,7 @@ describe TeamcityService, models: true do
 
   def service(teamcity_url: 'http://gitlab.com')
     described_class.create(
-      project: build_stubbed(:empty_project),
+      project: create(:empty_project),
       properties: {
         teamcity_url: teamcity_url,
         username: 'mic',

spec/models/project_spec.rb

@@ -258,6 +258,69 @@ describe Project, models: true do
     end
   end
 
+  describe :external_issue_tracker do
+    let(:project) { create(:project) }
+    let(:ext_project) { create(:redmine_project) }
+
+    context 'on existing projects with no value for has_external_issue_tracker' do
+      before(:each) do
+        project.update_column(:has_external_issue_tracker, nil)
+        ext_project.update_column(:has_external_issue_tracker, nil)
+      end
+
+      it 'updates the has_external_issue_tracker boolean' do
+        expect do
+          project.external_issue_tracker
+        end.to change { project.reload.has_external_issue_tracker }.to(false)
+
+        expect do
+          ext_project.external_issue_tracker
+        end.to change { ext_project.reload.has_external_issue_tracker }.to(true)
+      end
+    end
+
+    it 'returns nil and does not query services when there is no external issue tracker' do
+      project.build_missing_services
+      project.reload
+
+      expect(project).not_to receive(:services)
+
+      expect(project.external_issue_tracker).to eq(nil)
+    end
+
+    it 'retrieves external_issue_tracker querying services and cache it when there is external issue tracker' do
+      ext_project.reload # Factory returns a project with changed attributes
+      ext_project.build_missing_services
+      ext_project.reload
+
+      expect(ext_project).to receive(:services).once.and_call_original
+
+      2.times { expect(ext_project.external_issue_tracker).to be_a_kind_of(RedmineService) }
+    end
+  end
+
+  describe :cache_has_external_issue_tracker do
+    let(:project) { create(:project) }
+
+    it 'stores true if there is any external_issue_tracker' do
+      services = double(:service, external_issue_trackers: [RedmineService.new])
+      expect(project).to receive(:services).and_return(services)
+
+      expect do
+        project.cache_has_external_issue_tracker
+      end.to change { project.has_external_issue_tracker}.to(true)
+    end
+
+    it 'stores false if there is no external_issue_tracker' do
+      services = double(:service, external_issue_trackers: [])
+      expect(project).to receive(:services).and_return(services)
+
+      expect do
+        project.cache_has_external_issue_tracker
+      end.to change { project.has_external_issue_tracker}.to(false)
+    end
+  end
+
   describe :can_have_issues_tracker_id? do
     let(:project) { create(:project) }
     let(:ext_project) { create(:redmine_project) }

spec/models/service_spec.rb

@@ -204,4 +204,37 @@ describe Service, models: true do
       expect(service.bamboo_url_was).to be_nil
     end
   end
+
+  describe "callbacks" do
+    let(:project) { create(:project) }
+    let!(:service) do
+      RedmineService.new(
+        project: project,
+        active: true,
+        properties: {
+          project_url: 'http://redmine/projects/project_name_in_redmine',
+          issues_url: "http://redmine/#{project.id}/project_name_in_redmine/:id",
+          new_issue_url: 'http://redmine/projects/project_name_in_redmine/issues/new'
+        }
+      )
+    end
+
+    describe "on create" do
+      it "updates the has_external_issue_tracker boolean" do
+        expect do
+          service.save!
+        end.to change { service.project.has_external_issue_tracker }.from(nil).to(true)
+      end
+    end
+
+    describe "on update" do
+      it "updates the has_external_issue_tracker boolean" do
+        service.save!
+
+        expect do
+          service.update_attributes(active: false)
+        end.to change { service.project.has_external_issue_tracker }.from(true).to(false)
+      end
+    end
+  end
 end