Remove problematic query from obselete open environment in diff feature

Changelog: removed

app/controllers/projects/merge_requests/creations_controller.rb

@@ -56,9 +56,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
 
     @diff_notes_disabled = true
 
-    @environment = @merge_request.environments_for(current_user, latest: true).last
-
-    render json: { html: view_to_html_string('projects/merge_requests/creations/_diffs', diffs: @diffs, environment: @environment) }
+    render json: { html: view_to_html_string('projects/merge_requests/creations/_diffs', diffs: @diffs) }
   end
 
   def diff_for_path

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -35,13 +35,11 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
 
     diffs = @compare.diffs_in_batch(params[:page], params[:per_page], diff_options: diff_options_hash)
     unfoldable_positions = @merge_request.note_positions_for_paths(diffs.diff_file_paths, current_user).unfoldable
-    environment = @merge_request.environments_for(current_user, latest: true).last
 
     diffs.unfold_diff_files(unfoldable_positions)
     diffs.write_cache
 
     options = {
-      environment: environment,
       merge_request: @merge_request,
       commit: commit,
       diff_view: diff_view,
@@ -54,7 +52,6 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
       # NOTE: Any variables that would affect the resulting json needs to be added to the cache_context to avoid stale cache issues.
       cache_context = [
         current_user&.cache_key,
-        environment&.cache_key,
         unfoldable_positions.map(&:to_h),
         diff_view,
         params[:w],
@@ -98,7 +95,6 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   # Deprecated: https://gitlab.com/gitlab-org/gitlab/issues/37735
   def render_diffs
     diffs = @compare.diffs(diff_options)
-    @environment = @merge_request.environments_for(current_user, latest: true).last
 
     diffs.unfold_diff_files(note_positions.unfoldable)
     diffs.write_cache
@@ -175,7 +171,6 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
 
   def additional_attributes
     {
-      environment: @environment,
       merge_request: @merge_request,
       merge_request_diff: @merge_request_diff,
       merge_request_diffs: @merge_request_diffs,

app/finders/environments/environments_by_deployments_finder.rb

@@ -14,8 +14,7 @@ module Environments
     def execute
       deployments =
         if ref
-          deployments_query = params[:with_tags] ? 'ref = :ref OR tag IS TRUE' : 'ref = :ref'
-          Deployment.where(deployments_query, ref: ref.to_s)
+          Deployment.where(ref: ref.to_s)
         elsif commit
           Deployment.where(sha: commit.sha)
         else

app/models/merge_request.rb

@@ -1395,20 +1395,6 @@ class MergeRequest < ApplicationRecord
     actual_head_pipeline.success?
   end
 
-  def environments_for(current_user, latest: false)
-    return [] unless diff_head_commit
-
-    envs = Environments::EnvironmentsByDeploymentsFinder.new(target_project, current_user,
-      ref: target_branch, commit: diff_head_commit, with_tags: true, find_latest: latest).execute
-
-    if source_project
-      envs.concat Environments::EnvironmentsByDeploymentsFinder.new(source_project, current_user,
-        ref: source_branch, commit: diff_head_commit, find_latest: latest).execute
-    end
-
-    envs.uniq
-  end
-
   ##
   # This method is for looking for active environments which created via pipelines for merge requests.
   # Since deployments run on a merge request ref (e.g. `refs/merge-requests/:iid/head`),

doc/ci/review_apps/index.md

@@ -181,7 +181,7 @@ After you have the route mapping set up, it takes effect in the following locati
 
     ![View app file list in merge request widget](img/view_on_mr_widget.png)
 
-- In the diff for a merge request, comparison, or commit.
+- In the diff for a comparison or commit.
 
   ![View on environment button in merge request diff](img/view_on_env_mr.png)
 

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -205,7 +205,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do
         let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiff }
         let(:expected_options) do
           {
-            environment: nil,
             merge_request: merge_request,
             merge_request_diff: merge_request.merge_request_diff,
             merge_request_diffs: merge_request.merge_request_diffs,
@@ -280,7 +279,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do
         let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiff }
         let(:expected_options) do
           {
-            environment: nil,
             merge_request: merge_request,
             merge_request_diff: merge_request.merge_request_diff,
             merge_request_diffs: merge_request.merge_request_diffs,
@@ -303,7 +301,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do
         let(:collection) { Gitlab::Diff::FileCollection::Commit }
         let(:expected_options) do
           {
-            environment: nil,
             merge_request: merge_request,
             merge_request_diff: nil,
             merge_request_diffs: merge_request.merge_request_diffs,
@@ -330,7 +327,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do
         let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiff }
         let(:expected_options) do
           {
-            environment: nil,
             merge_request: merge_request,
             merge_request_diff: merge_request.merge_request_diff,
             merge_request_diffs: merge_request.merge_request_diffs,
@@ -494,7 +490,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do
 
     def collection_arguments(pagination_data = {})
       {
-        environment: nil,
         merge_request: merge_request,
         commit: nil,
         diff_view: :inline,

spec/features/projects/view_on_env_spec.rb

@@ -48,26 +48,6 @@ RSpec.describe 'View on environment', :js do
       let(:environment) { create(:environment, project: project, name: 'review/feature', external_url: 'http://feature.review.example.com') }
       let!(:deployment) { create(:deployment, :success, environment: environment, ref: branch_name, sha: sha) }
 
-      context 'when visiting the diff of a merge request for the branch' do
-        let(:merge_request) { create(:merge_request, :simple, source_project: project, source_branch: branch_name) }
-
-        before do
-          sign_in(user)
-
-          visit diffs_project_merge_request_path(project, merge_request)
-
-          wait_for_requests
-        end
-
-        it 'has a "View on env" button' do
-          within '.diffs' do
-            text = 'View on feature.review.example.com'
-            url = 'http://feature.review.example.com/ruby/feature'
-            expect(page).to have_selector("a[title='#{text}'][href='#{url}']")
-          end
-        end
-      end
-
       context 'when visiting a comparison for the branch' do
         before do
           sign_in(user)

spec/finders/environments/environments_by_deployments_finder_spec.rb

@@ -22,16 +22,6 @@ RSpec.describe Environments::EnvironmentsByDeploymentsFinder do
         create(:deployment, :success, environment: environment_two, ref: 'v1.1.0', tag: true, sha: project.commit('HEAD~1').id)
       end
 
-      it 'returns environment when with_tags is set' do
-        expect(described_class.new(project, user, ref: 'master', commit: commit, with_tags: true).execute)
-          .to contain_exactly(environment, environment_two)
-      end
-
-      it 'does not return environment when no with_tags is set' do
-        expect(described_class.new(project, user, ref: 'master', commit: commit).execute)
-          .to be_empty
-      end
-
       it 'does not return environment when commit is not part of deployment' do
         expect(described_class.new(project, user, ref: 'master', commit: project.commit('feature')).execute)
           .to be_empty
@@ -41,7 +31,7 @@ RSpec.describe Environments::EnvironmentsByDeploymentsFinder do
       # This tests to ensure we don't call one CommitIsAncestor per environment
       it 'only calls Gitaly twice when multiple environments are present', :request_store do
         expect do
-          result = described_class.new(project, user, ref: 'master', commit: commit, with_tags: true, find_latest: true).execute
+          result = described_class.new(project, user, ref: 'v1.1.0', commit: commit, find_latest: true).execute
 
           expect(result).to contain_exactly(environment_two)
         end.to change { Gitlab::GitalyClient.get_request_count }.by(2)

spec/models/merge_request_spec.rb

@@ -3492,84 +3492,6 @@ RSpec.describe MergeRequest, factory_default: :keep do
     end
   end
 
-  describe "#environments_for" do
-    let(:project)       { create(:project, :repository) }
-    let(:user)          { project.creator }
-    let(:merge_request) { create(:merge_request, source_project: project) }
-    let(:source_branch) { merge_request.source_branch }
-    let(:target_branch) { merge_request.target_branch }
-    let(:source_oid) { project.commit(source_branch).id }
-    let(:target_oid) { project.commit(target_branch).id }
-
-    before do
-      merge_request.source_project.add_maintainer(user)
-      merge_request.target_project.add_maintainer(user)
-    end
-
-    context 'with multiple environments' do
-      let(:environments) { create_list(:environment, 3, project: project) }
-
-      before do
-        create(:deployment, :success, environment: environments.first, ref: source_branch, sha: source_oid)
-        create(:deployment, :success, environment: environments.second, ref: target_branch, sha: target_oid)
-      end
-
-      it 'selects deployed environments' do
-        expect(merge_request.environments_for(user)).to contain_exactly(environments.first)
-      end
-
-      it 'selects latest deployed environment' do
-        latest_environment = create(:environment, project: project)
-        create(:deployment, :success, environment: latest_environment, ref: source_branch, sha: source_oid)
-
-        expect(merge_request.environments_for(user)).to eq([environments.first, latest_environment])
-        expect(merge_request.environments_for(user, latest: true)).to contain_exactly(latest_environment)
-      end
-    end
-
-    context 'with environments on source project' do
-      let(:source_project) { fork_project(project, nil, repository: true) }
-
-      let(:merge_request) do
-        create(:merge_request,
-               source_project: source_project, source_branch: 'feature',
-               target_project: project)
-      end
-
-      let(:source_environment) { create(:environment, project: source_project) }
-
-      before do
-        create(:deployment, :success, environment: source_environment, ref: 'feature', sha: merge_request.diff_head_sha)
-      end
-
-      it 'selects deployed environments', :sidekiq_might_not_need_inline do
-        expect(merge_request.environments_for(user)).to contain_exactly(source_environment)
-      end
-
-      context 'with environments on target project' do
-        let(:target_environment) { create(:environment, project: project) }
-
-        before do
-          create(:deployment, :success, environment: target_environment, tag: true, sha: merge_request.diff_head_sha)
-        end
-
-        it 'selects deployed environments', :sidekiq_might_not_need_inline do
-          expect(merge_request.environments_for(user)).to contain_exactly(source_environment, target_environment)
-        end
-      end
-    end
-
-    context 'without a diff_head_commit' do
-      before do
-        expect(merge_request).to receive(:diff_head_commit).and_return(nil)
-      end
-
-      it 'returns an empty array' do
-        expect(merge_request.environments_for(user)).to be_empty
-      end
-    end
-  end
-
   describe "#environments" do
     subject { merge_request.environments }
 

spec/requests/projects/merge_requests/context_commit_diffs_spec.rb

@@ -31,7 +31,6 @@ RSpec.describe 'Merge Requests Context Commit Diffs' do
 
     def collection_arguments(pagination_data = {})
       {
-        environment: nil,
         merge_request: merge_request,
         commit: nil,
         diff_view: :inline,

spec/requests/projects/merge_requests/diffs_spec.rb

@@ -29,7 +29,6 @@ RSpec.describe 'Merge Requests Diffs' do
 
     def collection_arguments(pagination_data = {})
       {
-        environment: nil,
         merge_request: merge_request,
         commit: nil,
         diff_view: :inline,
@@ -110,21 +109,6 @@ RSpec.describe 'Merge Requests Diffs' do
           end
         end
 
-        context 'with a new environment' do
-          let(:environment) do
-            create(:environment, :available, project: project)
-          end
-
-          let!(:deployment) do
-            create(:deployment, :success, environment: environment, ref: merge_request.source_branch)
-          end
-
-          it_behaves_like 'serializes diffs with expected arguments' do
-            let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
-            let(:expected_options) { collection_arguments(total_pages: 20).merge(environment: environment) }
-          end
-        end
-
         context 'with disabled display_merge_conflicts_in_diff feature' do
           before do
             stub_feature_flags(display_merge_conflicts_in_diff: false)