From aeb3787829301dafec54ff67ce33d045e1e99412 Mon Sep 17 00:00:00 2001 From: Artur Fedorov Date: Fri, 9 Dec 2022 23:58:43 +0100 Subject: [PATCH 1/2] This MR migrates Dropdown to Listbox Listbox component is more suitable for namespaces case. Changelog: changed --- .../pipeline_new/components/refs_dropdown.vue | 72 +++++++------------ .../pipeline_new/utils/format_refs.js | 31 +++++++- .../pipelines/legacy_pipelines_spec.rb | 0 .../projects/pipelines/pipelines_spec.rb | 4 +- .../components/refs_dropdown_spec.js | 41 ++++++----- .../pipeline_new/utils/format_refs_spec.js | 41 ++++++++++- 6 files changed, 120 insertions(+), 69 deletions(-) create mode 100644 spec/features/projects/pipelines/legacy_pipelines_spec.rb diff --git a/app/assets/javascripts/pipeline_new/components/refs_dropdown.vue b/app/assets/javascripts/pipeline_new/components/refs_dropdown.vue index d35d2010150944..a8ed332603cfa8 100644 --- a/app/assets/javascripts/pipeline_new/components/refs_dropdown.vue +++ b/app/assets/javascripts/pipeline_new/components/refs_dropdown.vue @@ -1,16 +1,13 @@ diff --git a/app/assets/javascripts/pipeline_new/utils/format_refs.js b/app/assets/javascripts/pipeline_new/utils/format_refs.js index f0fbc5ed7b6e57..9f2b71279e80f9 100644 --- a/app/assets/javascripts/pipeline_new/utils/format_refs.js +++ b/app/assets/javascripts/pipeline_new/utils/format_refs.js @@ -1,6 +1,11 @@ +import { __ } from '~/locale'; import { BRANCH_REF_TYPE, TAG_REF_TYPE } from '../constants'; -export default (refs, type) => { +function convertToListBoxItems(items) { + return items.map(({ shortName, fullName }) => ({ text: shortName, value: fullName })); +} + +export function formatRefs(refs, type) { let fullName; return refs.map((ref) => { @@ -15,4 +20,28 @@ export default (refs, type) => { fullName, }; }); +} + +export const formatListBoxItems = (branches, tags) => [ + { + text: __('Branches'), + options: convertToListBoxItems(formatRefs(branches, BRANCH_REF_TYPE)), + }, + { + text: __('Tags'), + options: convertToListBoxItems(formatRefs(tags, TAG_REF_TYPE)), + }, +]; + +const isBranchType = (fullName) => fullName.includes('heads'); + +export const searchByFullNameInListboxOptions = (fullName, listBox) => { + const optionsToSearch = isBranchType(fullName) ? listBox[0].options : listBox[1].options; + + const foundOption = optionsToSearch.find(({ value }) => value === fullName); + + return { + shortName: foundOption?.text, + fullName: foundOption.value, + }; }; diff --git a/spec/features/projects/pipelines/legacy_pipelines_spec.rb b/spec/features/projects/pipelines/legacy_pipelines_spec.rb new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index 26ef0cd52f7374..1241fe9c87c663 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -674,7 +674,7 @@ def create_build(stage, stage_idx, name, status) click_button project.default_branch wait_for_requests - find('p', text: 'master').click + find('.gl-dropdown-item', text: 'master').click wait_for_requests end @@ -789,7 +789,7 @@ def create_build(stage, stage_idx, name, status) click_button project.default_branch page.within '[data-testid="ref-select"]' do - find('[data-testid="search-refs"]').native.send_keys('fix') + find('[data-testid="listbox-search-input"] input').native.send_keys('fix') page.within '.gl-dropdown-contents' do expect(page).to have_content('fix') diff --git a/spec/frontend/pipeline_new/components/refs_dropdown_spec.js b/spec/frontend/pipeline_new/components/refs_dropdown_spec.js index 8cba876c688d39..1f0cc3e6b742c7 100644 --- a/spec/frontend/pipeline_new/components/refs_dropdown_spec.js +++ b/spec/frontend/pipeline_new/components/refs_dropdown_spec.js @@ -1,6 +1,6 @@ -import { GlDropdown, GlDropdownItem, GlSearchBoxByType } from '@gitlab/ui'; -import { shallowMount } from '@vue/test-utils'; +import { GlListbox, GlListboxItem } from '@gitlab/ui'; import MockAdapter from 'axios-mock-adapter'; +import { mountExtended, shallowMountExtended } from 'helpers/vue_test_utils_helper'; import waitForPromises from 'helpers/wait_for_promises'; import axios from '~/lib/utils/axios_utils'; import httpStatusCodes from '~/lib/utils/http_status'; @@ -19,11 +19,11 @@ describe('Pipeline New Form', () => { let wrapper; let mock; - const findDropdown = () => wrapper.findComponent(GlDropdown); - const findRefsDropdownItems = () => wrapper.findAllComponents(GlDropdownItem); - const findSearchBox = () => wrapper.findComponent(GlSearchBoxByType); + const findDropdown = () => wrapper.findComponent(GlListbox); + const findRefsDropdownItems = () => wrapper.findAllComponents(GlListboxItem); + const findSearchBox = () => wrapper.findByTestId('listbox-search-input'); - const createComponent = (props = {}, mountFn = shallowMount) => { + const createComponent = (props = {}, mountFn = shallowMountExtended) => { wrapper = mountFn(RefsDropdown, { provide: { projectRefsEndpoint, @@ -35,6 +35,7 @@ describe('Pipeline New Form', () => { }, ...props, }, + stubs: { GlListboxItem }, }); }; @@ -55,7 +56,7 @@ describe('Pipeline New Form', () => { }); it('displays empty dropdown initially', async () => { - await findDropdown().vm.$emit('show'); + await findDropdown().vm.$emit('shown'); expect(findRefsDropdownItems()).toHaveLength(0); }); @@ -66,7 +67,8 @@ describe('Pipeline New Form', () => { describe('when user opens dropdown', () => { beforeEach(async () => { - await findDropdown().vm.$emit('show'); + createComponent({}, mountExtended); + await findDropdown().vm.$emit('shown'); await waitForPromises(); }); @@ -78,7 +80,6 @@ describe('Pipeline New Form', () => { it('displays dropdown with branches and tags', async () => { const refLength = mockRefs.Tags.length + mockRefs.Branches.length; - expect(findRefsDropdownItems()).toHaveLength(refLength); }); @@ -99,7 +100,8 @@ describe('Pipeline New Form', () => { const selectedIndex = 1; beforeEach(async () => { - await findRefsDropdownItems().at(selectedIndex).vm.$emit('click'); + await findRefsDropdownItems().at(selectedIndex).vm.$emit('select', 'refs/heads/branch-1'); + await waitForPromises(); }); it('component emits @input', () => { @@ -149,18 +151,21 @@ describe('Pipeline New Form', () => { }) .reply(httpStatusCodes.OK, mockRefs); - createComponent({ - value: { - shortName: mockShortName, - fullName: mockFullName, + createComponent( + { + value: { + shortName: mockShortName, + fullName: mockFullName, + }, }, - }); - await findDropdown().vm.$emit('show'); + mountExtended, + ); + await findDropdown().vm.$emit('shown'); await waitForPromises(); }); it('branch is checked', () => { - expect(findRefsDropdownItems().at(selectedIndex).props('isChecked')).toBe(true); + expect(findRefsDropdownItems().at(selectedIndex).props('isSelected')).toBe(true); }); }); @@ -170,7 +175,7 @@ describe('Pipeline New Form', () => { .onGet(projectRefsEndpoint, { params: { search: '' } }) .reply(httpStatusCodes.INTERNAL_SERVER_ERROR); - await findDropdown().vm.$emit('show'); + await findDropdown().vm.$emit('shown'); await waitForPromises(); }); diff --git a/spec/frontend/pipeline_new/utils/format_refs_spec.js b/spec/frontend/pipeline_new/utils/format_refs_spec.js index 71190f55c166e9..c9dbaf76356f30 100644 --- a/spec/frontend/pipeline_new/utils/format_refs_spec.js +++ b/spec/frontend/pipeline_new/utils/format_refs_spec.js @@ -1,5 +1,9 @@ import { BRANCH_REF_TYPE, TAG_REF_TYPE } from '~/pipeline_new/constants'; -import formatRefs from '~/pipeline_new/utils/format_refs'; +import { + formatRefs, + formatListBoxItems, + searchByFullNameInListboxOptions, +} from '~/pipeline_new/utils/format_refs'; import { mockBranchRefs, mockTagRefs } from '../mock_data'; describe('Format refs util', () => { @@ -19,3 +23,38 @@ describe('Format refs util', () => { ]); }); }); + +describe('formatListBoxItems', () => { + it('formats branches and tags to listbox items correctly', () => { + expect(formatListBoxItems(mockBranchRefs, mockTagRefs)).toEqual([ + { + text: 'Branches', + options: [ + { value: 'refs/heads/main', text: 'main' }, + { value: 'refs/heads/dev', text: 'dev' }, + { value: 'refs/heads/release', text: 'release' }, + ], + }, + { + text: 'Tags', + options: [ + { value: 'refs/tags/1.0.0', text: '1.0.0' }, + { value: 'refs/tags/1.1.0', text: '1.1.0' }, + { value: 'refs/tags/1.2.0', text: '1.2.0' }, + ], + }, + ]); + }); +}); + +describe('searchByFullNameInListboxOptions', () => { + const listbox = formatListBoxItems(mockBranchRefs, mockTagRefs); + + it.each` + fullName | expectedResult + ${'refs/heads/main'} | ${{ fullName: 'refs/heads/main', shortName: 'main' }} + ${'refs/tags/1.0.0'} | ${{ fullName: 'refs/tags/1.0.0', shortName: '1.0.0' }} + `('should search item in listbox correctly', ({ fullName, expectedResult }) => { + expect(searchByFullNameInListboxOptions(fullName, listbox)).toEqual(expectedResult); + }); +}); -- GitLab From 60d426c0c52bc9048c1405cb9d40d871e918584e Mon Sep 17 00:00:00 2001 From: Artur Fedorov Date: Thu, 29 Dec 2022 14:12:36 +0100 Subject: [PATCH 2/2] Applied review changes --- .../pipeline_new/components/refs_dropdown.vue | 7 +-- .../pipeline_new/utils/format_refs.js | 36 +++++++------ .../components/refs_dropdown_spec.js | 50 ++++++++++++------- spec/frontend/pipeline_new/mock_data.js | 10 +++- .../pipeline_new/utils/format_refs_spec.js | 22 ++++++++ 5 files changed, 87 insertions(+), 38 deletions(-) diff --git a/app/assets/javascripts/pipeline_new/components/refs_dropdown.vue b/app/assets/javascripts/pipeline_new/components/refs_dropdown.vue index a8ed332603cfa8..d0a955a1db3f11 100644 --- a/app/assets/javascripts/pipeline_new/components/refs_dropdown.vue +++ b/app/assets/javascripts/pipeline_new/components/refs_dropdown.vue @@ -32,11 +32,6 @@ export default { return this.value.shortName; }, }, - watch: { - searchTerm() { - this.debouncedLoadRefs(); - }, - }, methods: { loadRefs() { this.isLoading = true; @@ -69,6 +64,7 @@ export default { }, setSearchTerm(searchQuery) { this.searchTerm = searchQuery?.trim(); + this.debouncedLoadRefs(); }, }, }; @@ -78,7 +74,6 @@ export default { class="gl-w-full gl-font-monospace" data-testid="ref-select" :items="listBoxItems" - :loading="isLoading" :searchable="true" :searching="isLoading" :search-placeholder="__('Search refs')" diff --git a/app/assets/javascripts/pipeline_new/utils/format_refs.js b/app/assets/javascripts/pipeline_new/utils/format_refs.js index 9f2b71279e80f9..e6d26b32d4721c 100644 --- a/app/assets/javascripts/pipeline_new/utils/format_refs.js +++ b/app/assets/javascripts/pipeline_new/utils/format_refs.js @@ -22,26 +22,34 @@ export function formatRefs(refs, type) { }); } -export const formatListBoxItems = (branches, tags) => [ - { - text: __('Branches'), - options: convertToListBoxItems(formatRefs(branches, BRANCH_REF_TYPE)), - }, - { - text: __('Tags'), - options: convertToListBoxItems(formatRefs(tags, TAG_REF_TYPE)), - }, -]; - -const isBranchType = (fullName) => fullName.includes('heads'); +export const formatListBoxItems = (branches, tags) => { + const finalResults = []; + + if (branches.length > 0) { + finalResults.push({ + text: __('Branches'), + options: convertToListBoxItems(formatRefs(branches, BRANCH_REF_TYPE)), + }); + } + + if (tags.length > 0) { + finalResults.push({ + text: __('Tags'), + options: convertToListBoxItems(formatRefs(tags, TAG_REF_TYPE)), + }); + } + + return finalResults; +}; export const searchByFullNameInListboxOptions = (fullName, listBox) => { - const optionsToSearch = isBranchType(fullName) ? listBox[0].options : listBox[1].options; + const optionsToSearch = + listBox.length > 1 ? listBox[0].options.concat(listBox[1].options) : listBox[0]?.options; const foundOption = optionsToSearch.find(({ value }) => value === fullName); return { - shortName: foundOption?.text, + shortName: foundOption.text, fullName: foundOption.value, }; }; diff --git a/spec/frontend/pipeline_new/components/refs_dropdown_spec.js b/spec/frontend/pipeline_new/components/refs_dropdown_spec.js index 1f0cc3e6b742c7..6fae970aaf0f60 100644 --- a/spec/frontend/pipeline_new/components/refs_dropdown_spec.js +++ b/spec/frontend/pipeline_new/components/refs_dropdown_spec.js @@ -7,7 +7,7 @@ import httpStatusCodes from '~/lib/utils/http_status'; import RefsDropdown from '~/pipeline_new/components/refs_dropdown.vue'; -import { mockRefs, mockFilteredRefs } from '../mock_data'; +import { mockBranches, mockRefs, mockFilteredRefs, mockTags } from '../mock_data'; const projectRefsEndpoint = '/root/project/refs'; const refShortName = 'main'; @@ -22,6 +22,7 @@ describe('Pipeline New Form', () => { const findDropdown = () => wrapper.findComponent(GlListbox); const findRefsDropdownItems = () => wrapper.findAllComponents(GlListboxItem); const findSearchBox = () => wrapper.findByTestId('listbox-search-input'); + const findListboxGroups = () => wrapper.findAll('ul[role="group"]'); const createComponent = (props = {}, mountFn = shallowMountExtended) => { wrapper = mountFn(RefsDropdown, { @@ -35,7 +36,6 @@ describe('Pipeline New Form', () => { }, ...props, }, - stubs: { GlListboxItem }, }); }; @@ -44,19 +44,12 @@ describe('Pipeline New Form', () => { mock.onGet(projectRefsEndpoint, { params: { search: '' } }).reply(httpStatusCodes.OK, mockRefs); }); - afterEach(() => { - wrapper.destroy(); - wrapper = null; - - mock.restore(); - }); - beforeEach(() => { createComponent(); }); - it('displays empty dropdown initially', async () => { - await findDropdown().vm.$emit('shown'); + it('displays empty dropdown initially', () => { + findDropdown().vm.$emit('shown'); expect(findRefsDropdownItems()).toHaveLength(0); }); @@ -68,17 +61,17 @@ describe('Pipeline New Form', () => { describe('when user opens dropdown', () => { beforeEach(async () => { createComponent({}, mountExtended); - await findDropdown().vm.$emit('shown'); + findDropdown().vm.$emit('shown'); await waitForPromises(); }); - it('requests unfiltered tags and branches', async () => { + it('requests unfiltered tags and branches', () => { expect(mock.history.get).toHaveLength(1); expect(mock.history.get[0].url).toBe(projectRefsEndpoint); expect(mock.history.get[0].params).toEqual({ search: '' }); }); - it('displays dropdown with branches and tags', async () => { + it('displays dropdown with branches and tags', () => { const refLength = mockRefs.Tags.length + mockRefs.Branches.length; expect(findRefsDropdownItems()).toHaveLength(refLength); }); @@ -100,7 +93,7 @@ describe('Pipeline New Form', () => { const selectedIndex = 1; beforeEach(async () => { - await findRefsDropdownItems().at(selectedIndex).vm.$emit('select', 'refs/heads/branch-1'); + findRefsDropdownItems().at(selectedIndex).vm.$emit('select', 'refs/heads/branch-1'); await waitForPromises(); }); @@ -160,7 +153,7 @@ describe('Pipeline New Form', () => { }, mountExtended, ); - await findDropdown().vm.$emit('shown'); + findDropdown().vm.$emit('shown'); await waitForPromises(); }); @@ -175,7 +168,7 @@ describe('Pipeline New Form', () => { .onGet(projectRefsEndpoint, { params: { search: '' } }) .reply(httpStatusCodes.INTERNAL_SERVER_ERROR); - await findDropdown().vm.$emit('shown'); + findDropdown().vm.$emit('shown'); await waitForPromises(); }); @@ -184,4 +177,27 @@ describe('Pipeline New Form', () => { expect(wrapper.emitted('loadingError')[0]).toEqual([expect.any(Error)]); }); }); + + describe('should display branches and tags based on its length', () => { + it.each` + mockData | expectedGroupLength | expectedListboxItemsLength + ${{ ...mockBranches, Tags: [] }} | ${1} | ${mockBranches.Branches.length} + ${{ Branches: [], ...mockTags }} | ${1} | ${mockTags.Tags.length} + ${{ ...mockRefs }} | ${2} | ${mockBranches.Branches.length + mockTags.Tags.length} + ${{ Branches: undefined, Tags: undefined }} | ${0} | ${0} + `( + 'should render branches and tags based on presence', + async ({ mockData, expectedGroupLength, expectedListboxItemsLength }) => { + mock + .onGet(projectRefsEndpoint, { params: { search: '' } }) + .reply(httpStatusCodes.OK, mockData); + createComponent({}, mountExtended); + findDropdown().vm.$emit('shown'); + await waitForPromises(); + + expect(findListboxGroups()).toHaveLength(expectedGroupLength); + expect(findRefsDropdownItems()).toHaveLength(expectedListboxItemsLength); + }, + ); + }); }); diff --git a/spec/frontend/pipeline_new/mock_data.js b/spec/frontend/pipeline_new/mock_data.js index 2af0ef4d7c46aa..dfb643a0ba4258 100644 --- a/spec/frontend/pipeline_new/mock_data.js +++ b/spec/frontend/pipeline_new/mock_data.js @@ -1,8 +1,16 @@ -export const mockRefs = { +export const mockBranches = { Branches: ['main', 'branch-1', 'branch-2'], +}; + +export const mockTags = { Tags: ['1.0.0', '1.1.0', '1.2.0'], }; +export const mockRefs = { + ...mockBranches, + ...mockTags, +}; + export const mockFilteredRefs = { Branches: ['branch-1'], Tags: ['1.0.0', '1.1.0'], diff --git a/spec/frontend/pipeline_new/utils/format_refs_spec.js b/spec/frontend/pipeline_new/utils/format_refs_spec.js index c9dbaf76356f30..26dfa002a91be9 100644 --- a/spec/frontend/pipeline_new/utils/format_refs_spec.js +++ b/spec/frontend/pipeline_new/utils/format_refs_spec.js @@ -44,6 +44,28 @@ describe('formatListBoxItems', () => { ], }, ]); + + expect(formatListBoxItems(mockBranchRefs, [])).toEqual([ + { + text: 'Branches', + options: [ + { value: 'refs/heads/main', text: 'main' }, + { value: 'refs/heads/dev', text: 'dev' }, + { value: 'refs/heads/release', text: 'release' }, + ], + }, + ]); + + expect(formatListBoxItems([], mockTagRefs)).toEqual([ + { + text: 'Tags', + options: [ + { value: 'refs/tags/1.0.0', text: '1.0.0' }, + { value: 'refs/tags/1.1.0', text: '1.1.0' }, + { value: 'refs/tags/1.2.0', text: '1.2.0' }, + ], + }, + ]); }); }); -- GitLab