Address feedback

app/controllers/projects/blob_controller.rb

@@ -30,9 +30,8 @@ class Projects::BlobController < Projects::ApplicationController
   end
   def show
-    environment_args = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit }
-    @environment = @project.environments_for(**environment_args).last
-    @environment = nil unless can?(current_user, :read_environment, @environment)
+    environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit }
+    @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
   end
   def edit

app/controllers/projects/commit_controller.rb

@@ -95,9 +95,8 @@ class Projects::CommitController < Projects::ApplicationController
     @diffs = commit.diffs(opts)
     @notes_count = commit.notes.count
-
-    @environment = @project.environments_for(commit: @commit).last
-    @environment = nil unless can?(current_user, :read_environment, @environment)
+    
+    @environment = EnvironmentsFinder.new(@project, current_user, commit: @commit).execute.last
   end
   def define_note_vars

app/controllers/projects/compare_controller.rb

@@ -57,9 +57,8 @@ class Projects::CompareController < Projects::ApplicationController
       @diffs = @compare.diffs(diff_options)
-      environment_args = @repository.branch_exists?(@head_ref) ? { ref: @head_ref } : { commit: @commit }
-      @environment = @project.environments_for(**environment_args).last
-      @environment = nil unless can?(current_user, :read_environment, @environment)
+      environment_params = @repository.branch_exists?(@head_ref) ? { ref: @head_ref } : { commit: @commit }
+      @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
       @diff_notes_disabled = true
       @grouped_diff_discussions = {}

app/controllers/projects/merge_requests_controller.rb

@@ -103,8 +103,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       end
     end
-    @environment = @merge_request.environments.last
-    @environment = nil unless can?(current_user, :read_environment, @environment)
+    @environment = @merge_request.environments_for(current_user).last
     respond_to do |format|
       format.html { define_discussion_vars }
@@ -248,8 +247,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
                  end
         @diff_notes_disabled = true
-        @environment = @merge_request.environments.last
-        @environment = nil unless can?(current_user, :read_environment, @environment)
+        @environment = @merge_request.environments_for(current_user).last
         render json: { html: view_to_html_string('projects/merge_requests/_new_diffs', diffs: @diffs, environment: @environment) }
       end
@@ -450,9 +448,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def ci_environments_status
     environments =
       begin
-        @merge_request.environments.map do |environment|
-          next unless can?(current_user, :read_environment, environment)
-
+        @merge_request.environments_for(current_user).map do |environment|
           project = environment.project
           deployment = environment.first_deployment_for(@merge_request.diff_head_commit)

app/finders/environments_finder.rb

+class EnvironmentsFinder
+  attr_reader :project, :current_user, :params
+
+  def initialize(project, current_user, params = {})
+    @project, @current_user, @params = project, current_user, params
+  end
+
+  def execute(skip_authorization: false)
+    deployments = project.deployments
+    deployments =
+      if ref
+        deployments_query = params[:with_tags] ? 'ref = :ref OR tag IS TRUE' : 'ref = :ref'
+        deployments.where(deployments_query, ref: ref.to_s)
+      elsif commit
+        deployments.where(sha: commit.sha)
+      else
+        deployments.none
+      end
+
+    environment_ids = deployments
+      .group(:environment_id)
+      .select(:environment_id)
+
+    environments = project.environments.available
+      .where(id: environment_ids).order_by_last_deployed_at.to_a
+
+    if ref && commit
+      environments.select! do |environment|
+        environment.includes_commit?(commit)
+      end
+    end
+
+    if ref && params[:recently_updated]
+      environments.select! do |environment|
+        environment.recently_updated_on_branch?(ref)
+      end
+    end
+
+    unless skip_authorization
+      environments.select! do |environment|
+        Ability.allowed?(current_user, :read_environment, environment)
+      end
+    end
+
+    environments
+  end
+
+  private
+
+  def ref
+    params[:ref].try(:to_s)
+  end
+
+  def commit
+    params[:commit]
+  end
+end

app/helpers/commits_helper.rb

@@ -194,7 +194,7 @@ module CommitsHelper
     end
   end
-  def view_file_btn(commit_sha, diff_new_path, project)
+  def view_file_button(commit_sha, diff_new_path, project)
     link_to(
       namespace_project_blob_path(project.namespace, project,
                                   tree_join(commit_sha, diff_new_path)),
@@ -205,7 +205,7 @@ module CommitsHelper
     end
   end
-  def view_on_environment_btn(commit_sha, diff_new_path, environment)
+  def view_on_environment_button(commit_sha, diff_new_path, environment)
     return unless environment && commit_sha
     external_url = environment.external_url_for(diff_new_path, commit_sha)

app/models/environment.rb

@@ -38,7 +38,13 @@ class Environment < ActiveRecord::Base
   scope :available, -> { with_state(:available) }
   scope :stopped, -> { with_state(:stopped) }
-  scope :order_by_last_deployed_at, -> { order(Gitlab::Database.nulls_first_order('(SELECT MAX(id) FROM deployments WHERE deployments.environment_id = environments.id)', 'ASC')) }
+  scope :order_by_last_deployed_at, -> do
+    max_deployment_id_sql =
+      Deployment.select(Deployment.arel_table[:id].maximum).
+      where(Deployment.arel_table[:environment_id].eq(arel_table[:id])).
+      to_sql
+    order(Gitlab::Database.nulls_first_order("(#{max_deployment_id_sql})", 'ASC'))
+  end
   state_machine :state, initial: :available do
     event :start do

app/models/merge_request.rb

@@ -715,18 +715,22 @@ class MergeRequest < ActiveRecord::Base
     !head_pipeline || head_pipeline.success? || head_pipeline.skipped?
   end
-  def environments
+  def environments_for(current_user)
     return [] unless diff_head_commit
-    @environments ||= begin
-      target_envs = target_project.environments_for(
-        ref: target_branch, commit: diff_head_commit, with_tags: true)
-      source_envs = source_project.environments_for(
-        ref: source_branch, commit: diff_head_commit) if source_project
-      (target_envs.to_a + source_envs.to_a).uniq
+    @environments ||= Hash.new do |h, current_user|
+      envs = EnvironmentsFinder.new(target_project, current_user,
+        ref: target_branch, commit: diff_head_commit, with_tags: true).execute
+      if source_project
+        envs.concat EnvironmentsFinder.new(source_project, current_user,
+          ref: source_branch, commit: diff_head_commit).execute
+      end
+      h[current_user] = envs.uniq
     end
+
+    @environments[current_user]
   end
   def state_human_name

app/models/project.rb

@@ -1306,37 +1306,6 @@ class Project < ActiveRecord::Base
     Gitlab::Redis.with { |redis| redis.del(pushes_since_gc_redis_key) }
   end
-  def environments_for(ref: nil, commit: nil, with_tags: false)
-    deps =
-      if ref
-        deployments_query = with_tags ? 'ref = ? OR tag IS TRUE' : 'ref = ?'
-        deployments.where(deployments_query, ref.to_s)
-      elsif commit
-        deployments.where(sha: commit.sha)
-      else
-        Deployment.none
-      end
-
-    environment_ids = deps
-      .group(:environment_id)
-      .select(:environment_id)
-
-    environments_found = environments.available
-      .where(id: environment_ids).order_by_last_deployed_at.to_a
-
-    return environments_found unless ref && commit
-
-    environments_found.select do |environment|
-      environment.includes_commit?(commit)
-    end
-  end
-
-  def environments_recently_updated_on_branch(branch)
-    environments_for(ref: branch).select do |environment|
-      environment.recently_updated_on_branch?(branch)
-    end
-  end
-
   def route_map_for(commit_sha)
     @route_maps_by_commit ||= Hash.new do |h, sha|
       h[sha] = begin

app/models/repository.rb

@@ -35,9 +35,6 @@ class Repository
     avatar: :avatar
   }
-  ROUTE_MAP_PATH = '.gitlab/route-map.yml'
-  GITLAB_CI_YML_PATH = '.gitlab-ci.yml'
-
   # Wraps around the given method and caches its output in Redis and an instance
   # variable.
   #
@@ -1165,6 +1162,14 @@ class Repository
     end
   end
+  def route_map_for(sha)
+    blob_data_at(sha, '.gitlab/route-map.yml')
+  end
+
+  def gitlab_ci_yml_for(sha)
+    blob_data_at(sha, '.gitlab-ci.yml')
+  end
+
   protected
   def tree_entry_at(branch_name, path)
@@ -1189,24 +1194,16 @@ class Repository
     end
   end
-  def route_map_for(sha)
-    blob = blob_at(sha, ROUTE_MAP_PATH)
-    return unless blob
-
-    blob.load_all_data!(self)
-    blob.data
-  end
-  def gitlab_ci_yml_for(sha)
-    blob = blob_at(sha, GITLAB_CI_YML_PATH)
+  private
+  def blob_data_at(sha, path)
+    blob = blob_at(sha, path)
     return unless blob
     blob.load_all_data!(self)
     blob.data
   end
-  private
-
   def git_action(index, action)
     path = normalize_path(action[:file_path])

app/services/ci/stop_environments_service.rb

@@ -22,8 +22,8 @@ module Ci
     end
     def environments
-      @environments ||= project
-        .environments_recently_updated_on_branch(@ref)
+      @environments ||=
+        EnvironmentsFinder.new(project, nil, ref: @ref, recently_updated: true).execute(skip_authorization: true)
     end
   end
 end

app/views/projects/blob/_actions.html.haml

 .btn-group
-  = view_on_environment_btn(@commit.sha, @path, @environment) if @environment
+  = view_on_environment_button(@commit.sha, @path, @environment) if @environment
 .btn-group.tree-btn-group
   = link_to 'Raw', namespace_project_raw_path(@project.namespace, @project, @id),

app/views/projects/diffs/_file.html.haml

@@ -14,7 +14,7 @@
             = edit_blob_link(@merge_request.source_project, @merge_request.source_branch, diff_file.new_path,
                              blob: blob, link_opts: link_opts)
-        = view_file_btn(diff_commit.id, diff_file.new_path, project)
-        = view_on_environment_btn(diff_commit.id, diff_file.new_path, environment) if environment
+        = view_file_button(diff_commit.id, diff_file.new_path, project)
+        = view_on_environment_button(diff_commit.id, diff_file.new_path, environment) if environment
   = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, blob: blob, project: project

doc/ci/environments.md

@@ -444,15 +444,17 @@ and/or `production`) you can see this information in the merge request itself.
 ### Go directly from source files to public pages on the environment
+> Introduced in GitLab 8.17.
+
 To go one step further, we can specify a Route Map to get GitLab to show us "View on [environment URL]" buttons to go directly from a file to that file's representation on the deployed website. It will be exposed in a few places:
 | In the diff for a merge request, comparison or commit | In the file view |
 | ------ | ------ |
 | !["View on env" button in merge request diff](img/view_on_env_mr.png) | !["View on env" button in file view](img/view_on_env_blob.png) |
-To get this to work, you need to tell GitLab how the paths of files in your repository map to paths of pages on your website, using a so-called Route Map.
-The Route Map is a file inside the repository at `.gitlab/route-map.yml`, that contains a YAML array that maps `source` paths (in the repository) to `public` paths (on the website).
+To get this to work, you need to tell GitLab how the paths of files in your repository map to paths of pages on your website, using a Route Map.
+A Route Map is a file inside the repository at `.gitlab/route-map.yml`, which contains a YAML array that maps `source` paths (in the repository) to `public` paths (on the website).
 This is an example of a route map for [Middleman](https://middlemanapp.com) static websites like [http://about.gitlab.com](https://gitlab.com/gitlab-com/www-gitlab-com):

lib/gitlab/route_map.rb

@@ -2,8 +2,6 @@ module Gitlab
   class RouteMap
     class FormatError < StandardError; end
-    attr_reader :map
-
     def initialize(data)
       begin
         entries = YAML.safe_load(data)

spec/features/projects/view_on_env_spec.rb

@@ -5,11 +5,11 @@ describe 'View on environment', js: true do
   let(:branch_name) { 'feature' }
   let(:file_path) { 'files/ruby/feature.rb' }
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :repository) }
   let(:user) { project.creator }
   before do
-    project.team << [user, :master]
+    project.add_master(user)
   end
   context 'when the branch has a route map' do
@@ -24,7 +24,7 @@ describe 'View on environment', js: true do
       Files::CreateService.new(
         project,
         user,
-        source_branch: branch_name,
+        start_branch: branch_name,
         target_branch: branch_name,
         commit_message: "Add .gitlab/route-map.yml",
         file_path: '.gitlab/route-map.yml',
@@ -35,7 +35,7 @@ describe 'View on environment', js: true do
       Files::UpdateService.new(
         project,
         user,
-        source_branch: branch_name,
+        start_branch: branch_name,
         target_branch: branch_name,
         commit_message: "Update feature",
         file_path: file_path,

spec/finders/environments_finder_spec.rb

+require 'spec_helper'
+
+describe EnvironmentsFinder do
+  describe '#execute' do
+    let(:project) { create(:project, :repository) }
+    let(:user) { project.creator }
+    let(:environment) { create(:environment, project: project) }
+
+    before do
+      project.add_master(user)
+    end
+
+    context 'tagged deployment' do
+      before do
+        create(:deployment, environment: environment, ref: '1.0', tag: true, sha: project.commit.id)
+      end
+
+      it 'returns environment when with_tags is set' do
+        expect(EnvironmentsFinder.new(project, user, ref: 'master', commit: project.commit, with_tags: true).execute)
+          .to contain_exactly(environment)
+      end
+
+      it 'does not return environment when no with_tags is set' do
+        expect(EnvironmentsFinder.new(project, user, ref: 'master', commit: project.commit).execute)
+          .to be_empty
+      end
+
+      it 'does not return environment when commit is not part of deployment' do
+        expect(EnvironmentsFinder.new(project, user, ref: 'master', commit: project.commit('feature')).execute)
+          .to be_empty
+      end
+    end
+
+    context 'branch deployment' do
+      before do
+        create(:deployment, environment: environment, ref: 'master', sha: project.commit.id)
+      end
+
+      it 'returns environment when ref is set' do
+        expect(EnvironmentsFinder.new(project, user, ref: 'master', commit: project.commit).execute)
+          .to contain_exactly(environment)
+      end
+
+      it 'does not environment when ref is different' do
+        expect(EnvironmentsFinder.new(project, user, ref: 'feature', commit: project.commit).execute)
+          .to be_empty
+      end
+
+      it 'does not return environment when commit is not part of deployment' do
+        expect(EnvironmentsFinder.new(project, user, ref: 'master', commit: project.commit('feature')).execute)
+          .to be_empty
+      end
+
+      it 'returns environment when commit constraint is not set' do
+        expect(EnvironmentsFinder.new(project, user, ref: 'master').execute)
+          .to contain_exactly(environment)
+      end
+    end
+
+    context 'commit deployment' do
+      before do
+        create(:deployment, environment: environment, ref: 'master', sha: project.commit.id)
+      end
+
+      it 'returns environment' do
+        expect(EnvironmentsFinder.new(project, user, commit: project.commit).execute)
+          .to contain_exactly(environment)
+      end
+    end
+
+    context 'recently updated' do
+      context 'when last deployment to environment is the most recent one' do
+        before do
+          create(:deployment, environment: environment, ref: 'feature')
+        end
+
+        it 'finds recently updated environment' do
+          expect(EnvironmentsFinder.new(project, user, ref: 'feature', recently_updated: true).execute)
+            .to contain_exactly(environment)
+        end
+      end
+
+      context 'when last deployment to environment is not the most recent' do
+        before do
+          create(:deployment, environment: environment, ref: 'feature')
+          create(:deployment, environment: environment, ref: 'master')
+        end
+
+        it 'does not find environment' do
+          expect(EnvironmentsFinder.new(project, user, ref: 'feature', recently_updated: true).execute)
+            .to be_empty
+        end
+      end
+
+      context 'when there are two environments that deploy to the same branch' do
+        let(:second_environment) { create(:environment, project: project) }
+
+        before do
+          create(:deployment, environment: environment, ref: 'feature')
+          create(:deployment, environment: second_environment, ref: 'feature')
+        end
+
+        it 'finds both environments' do
+          expect(EnvironmentsFinder.new(project, user, ref: 'feature', recently_updated: true).execute)
+            .to contain_exactly(environment, second_environment)
+        end
+      end
+    end
+  end
+end

spec/helpers/commits_helper_spec.rb

@@ -27,7 +27,7 @@ describe CommitsHelper do
     end
   end
-  describe '#view_on_environment_btn' do
+  describe '#view_on_environment_button' do
     let(:project) { create(:empty_project) }
     let(:environment) { create(:environment, external_url: 'http://example.com') }
     let(:path) { 'source/file.html' }
@@ -38,7 +38,7 @@ describe CommitsHelper do
     end
     it 'returns a link tag linking to the file in the environment' do
-      html = helper.view_on_environment_btn(sha, path, environment)
+      html = helper.view_on_environment_button(sha, path, environment)
       node = Nokogiri::HTML.parse(html).at_css('a')
       expect(node[:title]).to eq('View on example.com')

spec/lib/gitlab/route_map_spec.rb

@@ -54,7 +54,8 @@ describe Gitlab::RouteMap, lib: true do
     context 'when all is good' do
       it 'returns a route map' do
         route_map = described_class.new(YAML.dump([{ 'source' => '/index\.html/', 'public' => 'index.html' }]))
-        expect(route_map.map).to eq([{ source: /^index\.html$/, public: 'index.html' }])
+        
+        expect(route_map.public_path_for_source_path('index.html')).to eq('index.html')
       end
     end
   end

spec/models/merge_request_spec.rb

@@ -1005,10 +1005,16 @@ describe MergeRequest, models: true do
     end
   end
-  describe "#environments" do
+  describe "#environments_for" do
     let(:project)       { create(:project, :repository) }
+    let(:user)          { project.creator }
     let(:merge_request) { create(:merge_request, source_project: project) }
+    before do
+      merge_request.source_project.add_master(user)
+      merge_request.target_project.add_master(user)
+    end
+
     context 'with multiple environments' do
       let(:environments) { create_list(:environment, 3, project: project) }
@@ -1018,7 +1024,7 @@ describe MergeRequest, models: true do
       end
       it 'selects deployed environments' do
-        expect(merge_request.environments).to contain_exactly(environments.first)
+        expect(merge_request.environments_for(user)).to contain_exactly(environments.first)
       end
     end
@@ -1042,7 +1048,7 @@ describe MergeRequest, models: true do
       end
       it 'selects deployed environments' do
-        expect(merge_request.environments).to contain_exactly(source_environment)
+        expect(merge_request.environments_for(user)).to contain_exactly(source_environment)
       end
       context 'with environments on target project' do
@@ -1053,7 +1059,7 @@ describe MergeRequest, models: true do
         end
         it 'selects deployed environments' do
-          expect(merge_request.environments).to contain_exactly(source_environment, target_environment)
+          expect(merge_request.environments_for(user)).to contain_exactly(source_environment, target_environment)
         end
       end
     end
@@ -1064,7 +1070,7 @@ describe MergeRequest, models: true do
       end
       it 'returns an empty array' do
-        expect(merge_request.environments).to be_empty
+        expect(merge_request.environments_for(user)).to be_empty
       end
     end
   end