From 73e873cb22668f99cb4ca4a97b149b1766ec4abb Mon Sep 17 00:00:00 2001
From: Phil Hughes <me@iamphill.com>
Date: Thu, 14 Sep 2023 10:15:18 +0000
Subject: [PATCH] Change target branch in compare form automatically
Created an internal API that will get called whenever the source branch
dropdown value. This fetches and returns the target branch
for the specified source branch.
https://gitlab.com/gitlab-org/gitlab/-/issues/17909
---
.../merge_requests/components/compare_app.vue | 18 +++++--
.../components/compare_dropdown.vue | 6 +++
.../creations/new/branch_finder.js | 1 +
.../merge_requests/creations/new/index.js | 36 +++++++++++--
app/helpers/merge_requests_helper.rb | 7 +++
.../creations/_new_compare.html.haml | 2 +-
.../creations/new/branch_finder.js | 12 +++++
.../target_branch_rules_controller.rb | 6 +++
ee/app/helpers/ee/merge_requests_helper.rb | 9 ++++
ee/config/routes/project.rb | 2 +-
.../creations/new/branch_finder_spec.js | 36 +++++++++++++
ee/spec/helpers/merge_requests_helper_spec.rb | 38 +++++++++++++
.../target_branch_rules_controller_spec.rb | 47 ++++++++++++++++
ee/spec/routing/project_routing_spec.rb | 4 ++
.../components/compare_app_spec.js | 54 +++++++++++++++++--
15 files changed, 266 insertions(+), 12 deletions(-)
create mode 100644 app/assets/javascripts/pages/projects/merge_requests/creations/new/branch_finder.js
create mode 100644 ee/app/assets/javascripts/pages/projects/merge_requests/creations/new/branch_finder.js
create mode 100644 ee/spec/frontend/pages/projects/merge_requests/creations/new/branch_finder_spec.js
diff --git a/app/assets/javascripts/merge_requests/components/compare_app.vue b/app/assets/javascripts/merge_requests/components/compare_app.vue
index 8e02048f4948e..20c95174989d3 100644
--- a/app/assets/javascripts/merge_requests/components/compare_app.vue
+++ b/app/assets/javascripts/merge_requests/components/compare_app.vue
@@ -23,9 +23,6 @@ export default {
currentProject: {
default: () => ({}),
},
- currentBranch: {
- default: () => ({}),
- },
inputs: {
default: () => ({}),
},
@@ -39,6 +36,13 @@ export default {
default: '',
},
},
+ props: {
+ currentBranch: {
+ type: Object,
+ required: false,
+ default: () => ({}),
+ },
+ },
data() {
return {
selectedProject: this.currentProject,
@@ -57,6 +61,12 @@ export default {
return this.commitHtml || this.loading || !this.selectedBranch.value;
},
},
+ watch: {
+ currentBranch(newVal) {
+ this.selectedBranch = newVal;
+ this.fetchCommit();
+ },
+ },
mounted() {
this.fetchCommit();
},
@@ -67,6 +77,7 @@ export default {
selectBranch(branch) {
this.selectedBranch = branch;
this.fetchCommit();
+ this.$emit('select-branch', branch.value);
},
async fetchCommit() {
if (!this.selectedBranch.value) return;
@@ -109,6 +120,7 @@ export default {
:default="currentBranch"
:toggle-class="toggleClass.branch"
:qa-selector="branchQaSelector"
+ data-testid="compare-dropdown"
@selected="selectBranch"
/>
</div>
diff --git a/app/assets/javascripts/merge_requests/components/compare_dropdown.vue b/app/assets/javascripts/merge_requests/components/compare_dropdown.vue
index a5a4e683214e4..6f46b41ee5b27 100644
--- a/app/assets/javascripts/merge_requests/components/compare_dropdown.vue
+++ b/app/assets/javascripts/merge_requests/components/compare_dropdown.vue
@@ -70,6 +70,12 @@ export default {
);
},
},
+ watch: {
+ default(newVal) {
+ this.current = newVal;
+ this.selected = newVal.value;
+ },
+ },
methods: {
async fetchData() {
if (!this.endpoint) return;
diff --git a/app/assets/javascripts/pages/projects/merge_requests/creations/new/branch_finder.js b/app/assets/javascripts/pages/projects/merge_requests/creations/new/branch_finder.js
new file mode 100644
index 0000000000000..ee84f54978acc
--- /dev/null
+++ b/app/assets/javascripts/pages/projects/merge_requests/creations/new/branch_finder.js
@@ -0,0 +1 @@
+export const findTargetBranch = async () => {};
diff --git a/app/assets/javascripts/pages/projects/merge_requests/creations/new/index.js b/app/assets/javascripts/pages/projects/merge_requests/creations/new/index.js
index f71a1041068e8..9db400b3b0a53 100644
--- a/app/assets/javascripts/pages/projects/merge_requests/creations/new/index.js
+++ b/app/assets/javascripts/pages/projects/merge_requests/creations/new/index.js
@@ -2,6 +2,8 @@ import Vue from 'vue';
import { mountMarkdownEditor } from 'ee_else_ce/vue_shared/components/markdown/mount_markdown_editor';
+import { findTargetBranch } from 'ee_else_ce/pages/projects/merge_requests/creations/new/branch_finder';
+
import initPipelines from '~/commit/pipelines/pipelines_bundle';
import MergeRequest from '~/merge_request';
import CompareApp from '~/merge_requests/components/compare_app.vue';
@@ -13,14 +15,15 @@ if (mrNewCompareNode) {
const targetCompareEl = document.getElementById('js-target-project-dropdown');
const sourceCompareEl = document.getElementById('js-source-project-dropdown');
const compareEl = document.querySelector('.js-merge-request-new-compare');
+ const targetBranch = Vue.observable({ name: '' });
+ const currentSourceBranch = JSON.parse(sourceCompareEl.dataset.currentBranch);
// eslint-disable-next-line no-new
new Vue({
el: sourceCompareEl,
name: 'SourceCompareApp',
provide: {
currentProject: JSON.parse(sourceCompareEl.dataset.currentProject),
- currentBranch: JSON.parse(sourceCompareEl.dataset.currentBranch),
branchCommitPath: compareEl.dataset.sourceBranchUrl,
inputs: {
project: {
@@ -42,18 +45,34 @@ if (mrNewCompareNode) {
},
branchQaSelector: 'source_branch_dropdown',
},
+ methods: {
+ async selectedBranch(branchName) {
+ const targetBranchName = await findTargetBranch(branchName);
+
+ if (targetBranchName) {
+ targetBranch.name = targetBranchName;
+ }
+ },
+ },
render(h) {
- return h(CompareApp);
+ return h(CompareApp, {
+ props: {
+ currentBranch: currentSourceBranch,
+ },
+ on: {
+ 'select-branch': this.selectedBranch,
+ },
+ });
},
});
+ const currentTargetBranch = JSON.parse(targetCompareEl.dataset.currentBranch);
// eslint-disable-next-line no-new
new Vue({
el: targetCompareEl,
name: 'TargetCompareApp',
provide: {
currentProject: JSON.parse(targetCompareEl.dataset.currentProject),
- currentBranch: JSON.parse(targetCompareEl.dataset.currentBranch),
projectsPath: targetCompareEl.dataset.targetProjectsPath,
branchCommitPath: compareEl.dataset.targetBranchUrl,
inputs: {
@@ -75,8 +94,17 @@ if (mrNewCompareNode) {
branch: 'js-target-branch gl-font-monospace',
},
},
+ computed: {
+ currentBranch() {
+ if (targetBranch.name) {
+ return { text: targetBranch.name, value: targetBranch.name };
+ }
+
+ return currentTargetBranch;
+ },
+ },
render(h) {
- return h(CompareApp);
+ return h(CompareApp, { props: { currentBranch: this.currentBranch } });
},
});
} else {
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index b018c03eefd19..a90a16e120cf2 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -226,6 +226,13 @@ def how_merge_modal_data(merge_request)
}
end
+ def mr_compare_form_data(_, merge_request)
+ {
+ source_branch_url: project_new_merge_request_branch_from_path(merge_request.source_project),
+ target_branch_url: project_new_merge_request_branch_to_path(merge_request.source_project)
+ }
+ end
+
private
def review_requested_merge_requests_count
diff --git a/app/views/projects/merge_requests/creations/_new_compare.html.haml b/app/views/projects/merge_requests/creations/_new_compare.html.haml
index 015f6423e7c5f..e6bd0b05f003e 100644
--- a/app/views/projects/merge_requests/creations/_new_compare.html.haml
+++ b/app/views/projects/merge_requests/creations/_new_compare.html.haml
@@ -14,7 +14,7 @@
= gitlab_ui_form_for [@project, @merge_request], url: project_new_merge_request_path(@project), method: :get, html: { class: "merge-request-form js-requires-input" } do |f|
- if params[:nav_source].present?
= hidden_field_tag(:nav_source, params[:nav_source])
- .js-merge-request-new-compare.row{ 'data-source-branch-url': project_new_merge_request_branch_from_path(@source_project), 'data-target-branch-url': project_new_merge_request_branch_to_path(@source_project) }
+ .js-merge-request-new-compare.row{ data: mr_compare_form_data(current_user, @merge_request) }
.col-lg-6
.card-new-merge-request
%h2.gl-font-size-h2
diff --git a/ee/app/assets/javascripts/pages/projects/merge_requests/creations/new/branch_finder.js b/ee/app/assets/javascripts/pages/projects/merge_requests/creations/new/branch_finder.js
new file mode 100644
index 0000000000000..3f1ed4065b9a5
--- /dev/null
+++ b/ee/app/assets/javascripts/pages/projects/merge_requests/creations/new/branch_finder.js
@@ -0,0 +1,12 @@
+import axios from '~/lib/utils/axios_utils';
+
+export const findTargetBranch = async (branch) => {
+ const path = document.querySelector('.js-merge-request-new-compare')?.dataset
+ .targetBranchFinderPath;
+
+ if (!path) return null;
+
+ const { data } = await axios.get(path, { params: { branch_name: branch } });
+
+ return data.target_branch;
+};
diff --git a/ee/app/controllers/projects/target_branch_rules_controller.rb b/ee/app/controllers/projects/target_branch_rules_controller.rb
index 5bbb4486bac34..5f5c0e7d63ccc 100644
--- a/ee/app/controllers/projects/target_branch_rules_controller.rb
+++ b/ee/app/controllers/projects/target_branch_rules_controller.rb
@@ -9,6 +9,12 @@ class TargetBranchRulesController < Projects::ApplicationController
render_404 unless can?(current_user, :read_target_branch_rule, @project)
end
+ def index
+ service = ::TargetBranchRules::FindService.new(@project, current_user)
+
+ render json: { target_branch: service.execute(params[:branch_name]) }.to_json
+ end
+
def create
result = TargetBranchRules::CreateService.new(@project, current_user, create_params).execute
diff --git a/ee/app/helpers/ee/merge_requests_helper.rb b/ee/app/helpers/ee/merge_requests_helper.rb
index 9077b1491fd04..ec867993beaf5 100644
--- a/ee/app/helpers/ee/merge_requests_helper.rb
+++ b/ee/app/helpers/ee/merge_requests_helper.rb
@@ -30,6 +30,15 @@ def diffs_tab_pane_data(project, merge_request, params)
super.merge(data)
end
+ override :mr_compare_form_data
+ def mr_compare_form_data(user, merge_request)
+ target_branch_finder_path = if can?(user, :read_target_branch_rule, merge_request.project)
+ project_target_branch_rules_path(merge_request.project)
+ end
+
+ super.merge({ target_branch_finder_path: target_branch_finder_path })
+ end
+
def summarize_llm_enabled?(project, user)
::Llm::MergeRequests::SummarizeDiffService.enabled?(group: project.root_ancestor, user: user)
end
diff --git a/ee/config/routes/project.rb b/ee/config/routes/project.rb
index 3cf2397b3937b..ecd8a69844902 100644
--- a/ee/config/routes/project.rb
+++ b/ee/config/routes/project.rb
@@ -42,7 +42,7 @@
end
end
- resources :target_branch_rules, only: [:create]
+ resources :target_branch_rules, only: [:index, :create]
resources :automations, only: [:index]
diff --git a/ee/spec/frontend/pages/projects/merge_requests/creations/new/branch_finder_spec.js b/ee/spec/frontend/pages/projects/merge_requests/creations/new/branch_finder_spec.js
new file mode 100644
index 0000000000000..f564116b58c64
--- /dev/null
+++ b/ee/spec/frontend/pages/projects/merge_requests/creations/new/branch_finder_spec.js
@@ -0,0 +1,36 @@
+import MockAdapter from 'axios-mock-adapter';
+import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures';
+import axios from '~/lib/utils/axios_utils';
+import { findTargetBranch } from 'ee/pages/projects/merge_requests/creations/new/branch_finder';
+
+let mock;
+
+describe('Merge request find target branch', () => {
+ beforeEach(() => {
+ mock = new MockAdapter(axios);
+ mock.onGet('/target_branch').reply(200, { target_branch: 'main' });
+ });
+
+ afterEach(() => {
+ resetHTMLFixture();
+ mock.restore();
+ });
+
+ describe('element does not exist', () => {
+ it('returns null', async () => {
+ expect(await findTargetBranch()).toBe(null);
+ });
+ });
+
+ describe('element exists', () => {
+ beforeEach(() => {
+ setHTMLFixture(
+ '<div class="js-merge-request-new-compare" data-target-branch-finder-path="/target_branch"></div>',
+ );
+ });
+
+ it('returns target branch', async () => {
+ expect(await findTargetBranch()).toBe('main');
+ });
+ });
+});
diff --git a/ee/spec/helpers/merge_requests_helper_spec.rb b/ee/spec/helpers/merge_requests_helper_spec.rb
index 195500de99570..6ff455857d6ee 100644
--- a/ee/spec/helpers/merge_requests_helper_spec.rb
+++ b/ee/spec/helpers/merge_requests_helper_spec.rb
@@ -153,4 +153,42 @@
end
end
end
+
+ describe '#mr_compare_form_data' do
+ let_it_be(:project) { build_stubbed(:project) }
+ let_it_be(:merge_request) { build_stubbed(:merge_request, source_project: project) }
+ let_it_be(:user) { build_stubbed(:user) }
+
+ subject(:mr_compare_form_data) { helper.mr_compare_form_data(user, merge_request) }
+
+ describe 'when the target_branch_rules_flag flag is disabled' do
+ before do
+ stub_feature_flags(target_branch_rules_flag: false)
+ end
+
+ it 'returns target_branch_finder_path as nil' do
+ expect(subject[:target_branch_finder_path]).to eq(nil)
+ end
+ end
+
+ describe 'when the project does not have the correct license' do
+ before do
+ stub_licensed_features(target_branch_rules: false)
+ end
+
+ it 'returns target_branch_finder_path as nil' do
+ expect(subject[:target_branch_finder_path]).to eq(nil)
+ end
+ end
+
+ describe 'when the project has the correct license' do
+ before do
+ stub_licensed_features(target_branch_rules: true)
+ end
+
+ it 'returns target_branch_finder_path' do
+ expect(subject[:target_branch_finder_path]).to eq(project_target_branch_rules_path(project))
+ end
+ end
+ end
end
diff --git a/ee/spec/requests/projects/target_branch_rules_controller_spec.rb b/ee/spec/requests/projects/target_branch_rules_controller_spec.rb
index f7fe42b0685a9..4a2be8043bb72 100644
--- a/ee/spec/requests/projects/target_branch_rules_controller_spec.rb
+++ b/ee/spec/requests/projects/target_branch_rules_controller_spec.rb
@@ -10,6 +10,53 @@
login_as(user)
end
+ describe 'GET #index' do
+ describe 'when the target_branch_rules_flag flag is disabled' do
+ before do
+ stub_feature_flags(target_branch_rules_flag: false)
+ end
+
+ it 'returns 404' do
+ get project_target_branch_rules_path(project)
+
+ expect(response).to have_gitlab_http_status(:not_found)
+ end
+ end
+
+ describe 'when the project does not have the correct license' do
+ before do
+ stub_licensed_features(target_branch_rules: false)
+ end
+
+ it 'returns 404' do
+ get project_target_branch_rules_path(project)
+
+ expect(response).to have_gitlab_http_status(:not_found)
+ end
+ end
+
+ describe 'when target_branch_rules_flag is enabled and project has the correct license' do
+ before do
+ stub_licensed_features(target_branch_rules: true)
+ create(:target_branch_rule, project: project, name: 'feature', target_branch: 'other-branch')
+ end
+
+ it 'calls TargetBranchRules::FindService' do
+ expect_next_instance_of(TargetBranchRules::FindService) do |service|
+ expect(service).to receive(:execute).with('feature')
+ end
+
+ get project_target_branch_rules_path(project), params: { branch_name: 'feature' }
+ end
+
+ it 'renders JSON with target_branch property' do
+ get project_target_branch_rules_path(project), params: { branch_name: 'feature' }
+
+ expect(json_response).to include("target_branch" => 'other-branch')
+ end
+ end
+ end
+
describe 'POST #create' do
let(:params) { { name: 'dev/*', target_branch: 'develop' } }
diff --git a/ee/spec/routing/project_routing_spec.rb b/ee/spec/routing/project_routing_spec.rb
index 96d90db3141d9..685b980ea11e3 100644
--- a/ee/spec/routing/project_routing_spec.rb
+++ b/ee/spec/routing/project_routing_spec.rb
@@ -86,6 +86,10 @@
end
describe Projects::TargetBranchRulesController, 'routing' do
+ it "to #index" do
+ expect(get("/gitlab/gitlabhq/-/target_branch_rules")).to route_to('projects/target_branch_rules#index', namespace_id: 'gitlab', project_id: 'gitlabhq')
+ end
+
it "to #create" do
expect(post("/gitlab/gitlabhq/-/target_branch_rules")).to route_to('projects/target_branch_rules#create', namespace_id: 'gitlab', project_id: 'gitlabhq')
end
diff --git a/spec/frontend/merge_requests/components/compare_app_spec.js b/spec/frontend/merge_requests/components/compare_app_spec.js
index ba129363ffd47..887f79f9fadc0 100644
--- a/spec/frontend/merge_requests/components/compare_app_spec.js
+++ b/spec/frontend/merge_requests/components/compare_app_spec.js
@@ -1,10 +1,14 @@
-import { shallowMount } from '@vue/test-utils';
+import MockAdapter from 'axios-mock-adapter';
+import waitForPromises from 'helpers/wait_for_promises';
+import { shallowMountExtended } from 'helpers/vue_test_utils_helper';
+import axios from '~/lib/utils/axios_utils';
import CompareApp from '~/merge_requests/components/compare_app.vue';
let wrapper;
+let mock;
function factory(provideData = {}) {
- wrapper = shallowMount(CompareApp, {
+ wrapper = shallowMountExtended(CompareApp, {
provide: {
inputs: {
project: {
@@ -16,6 +20,7 @@ function factory(provideData = {}) {
name: 'branch',
},
},
+ branchCommitPath: '/commit',
toggleClass: {
project: 'project',
branch: 'branch',
@@ -29,7 +34,18 @@ function factory(provideData = {}) {
});
}
+const findCommitBox = () => wrapper.findByTestId('commit-box');
+
describe('Merge requests compare app component', () => {
+ beforeEach(() => {
+ mock = new MockAdapter(axios);
+ mock.onGet('/commit').reply(200, 'commit content');
+ });
+
+ afterEach(() => {
+ mock.restore();
+ });
+
it('shows commit box when selected branch is empty', () => {
factory({
currentBranch: {
@@ -38,9 +54,41 @@ describe('Merge requests compare app component', () => {
},
});
- const commitBox = wrapper.find('[data-testid="commit-box"]');
+ const commitBox = findCommitBox();
expect(commitBox.exists()).toBe(true);
expect(commitBox.text()).toBe('Select a branch to compare');
});
+
+ it('emits select-branch on selected event', () => {
+ factory({
+ currentBranch: {
+ text: '',
+ value: '',
+ },
+ });
+
+ wrapper.findByTestId('compare-dropdown').vm.$emit('selected', { value: 'main' });
+
+ expect(wrapper.emitted('select-branch')).toEqual([['main']]);
+ });
+
+ describe('currentBranch watcher', () => {
+ it('changes selected value', async () => {
+ factory({
+ currentBranch: {
+ text: '',
+ value: '',
+ },
+ });
+
+ expect(findCommitBox().text()).toBe('Select a branch to compare');
+
+ wrapper.setProps({ currentBranch: { text: 'main', value: 'main ' } });
+
+ await waitForPromises();
+
+ expect(findCommitBox().text()).toBe('commit content');
+ });
+ });
});
--
GitLab