From da10f2ad450c46075f03a0ab1b2def9225e382c6 Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Fri, 21 May 2021 15:45:50 -0700 Subject: [PATCH 1/3] Support restrictedVisibilityLevels in fork form Solve issue: https://gitlab.com/gitlab-org/gitlab/-/issues/329575 --- .../projects/forks/new/components/app.vue | 5 + .../forks/new/components/fork_form.vue | 36 +++-- .../pages/projects/forks/new/index.js | 2 + app/views/projects/forks/new.html.haml | 3 +- .../projects/forks/new/components/app_spec.js | 1 + .../forks/new/components/fork_form_spec.js | 123 ++++++++++++++---- 6 files changed, 136 insertions(+), 34 deletions(-) diff --git a/app/assets/javascripts/pages/projects/forks/new/components/app.vue b/app/assets/javascripts/pages/projects/forks/new/components/app.vue index 02b357d389b80c..7fb41c6e7b7494 100644 --- a/app/assets/javascripts/pages/projects/forks/new/components/app.vue +++ b/app/assets/javascripts/pages/projects/forks/new/components/app.vue @@ -38,6 +38,10 @@ export default { type: String, required: true, }, + restrictedVisibilityLevels: { + type: Array, + required: true, + }, }, }; @@ -66,6 +70,7 @@ export default { :project-path="projectPath" :project-description="projectDescription" :project-visibility="projectVisibility" + :restricted-visibility-levels="restrictedVisibilityLevels" /> diff --git a/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue b/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue index 96bd38ea9e2641..d89086f188da9b 100644 --- a/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue +++ b/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue @@ -95,6 +95,10 @@ export default { type: String, required: true, }, + restrictedVisibilityLevels: { + type: Array, + required: true, + }, }, data() { const form = { @@ -111,7 +115,7 @@ export default { required: false, skipValidation: true, }), - visibility: initFormField({ value: this.projectVisibility }), + visibility: initFormField({ value: this.getInitialVisibilityValue() }), }, }; return { @@ -135,12 +139,24 @@ export default { return Math.min(this.projectVisibilityLevel, this.namespaceVisibilityLevel); }, allowedVisibilityLevels() { - return Object.entries(VISIBILITY_LEVEL).reduce((levels, [levelName, levelValue]) => { - if (levelValue <= this.visibilityLevelCap) { - levels.push(levelName); - } - return levels; - }, []); + const allowedLevels = Object.entries(VISIBILITY_LEVEL).reduce( + (levels, [levelName, levelValue]) => { + if ( + !this.restrictedVisibilityLevels.includes(levelValue) && + levelValue <= this.visibilityLevelCap + ) { + levels.push(levelName); + } + return levels; + }, + [], + ); + + if (!allowedLevels.length) { + return [PRIVATE_VISIBILITY]; + } + + return allowedLevels; }, visibilityLevels() { return [ @@ -173,7 +189,8 @@ export default { watch: { // eslint-disable-next-line func-names 'form.fields.namespace.value': function () { - this.form.fields.visibility.value = PRIVATE_VISIBILITY; + this.form.fields.visibility.value = + this.restrictedVisibilityLevels.length !== 0 ? null : PRIVATE_VISIBILITY; }, // eslint-disable-next-line func-names 'form.fields.name.value': function (newVal) { @@ -191,6 +208,9 @@ export default { isVisibilityLevelDisabled(visibility) { return !this.allowedVisibilityLevels.includes(visibility); }, + getInitialVisibilityValue() { + return this.restrictedVisibilityLevels.length !== 0 ? null : this.projectVisibility; + }, async onSubmit() { this.form.showValidation = true; diff --git a/app/assets/javascripts/pages/projects/forks/new/index.js b/app/assets/javascripts/pages/projects/forks/new/index.js index 372967c8a1e48b..1a1712520481e7 100644 --- a/app/assets/javascripts/pages/projects/forks/new/index.js +++ b/app/assets/javascripts/pages/projects/forks/new/index.js @@ -16,6 +16,7 @@ if (gon.features.forkProjectForm) { projectPath, projectDescription, projectVisibility, + restrictedVisibilityLevels, } = mountElement.dataset; // eslint-disable-next-line no-new @@ -38,6 +39,7 @@ if (gon.features.forkProjectForm) { projectPath, projectDescription, projectVisibility, + restrictedVisibilityLevels: JSON.parse(restrictedVisibilityLevels), }, }); }, diff --git a/app/views/projects/forks/new.html.haml b/app/views/projects/forks/new.html.haml index 267fc3ae986bd8..0716eda79a848c 100644 --- a/app/views/projects/forks/new.html.haml +++ b/app/views/projects/forks/new.html.haml @@ -10,7 +10,8 @@ project_name: @project.name, project_path: @project.path, project_description: @project.description, - project_visibility: @project.visibility } } + project_visibility: @project.visibility, + restricted_visibility_levels: Gitlab::CurrentSettings.restricted_visibility_levels.to_json } } - else .row.gl-mt-3 .col-lg-3 diff --git a/spec/frontend/pages/projects/forks/new/components/app_spec.js b/spec/frontend/pages/projects/forks/new/components/app_spec.js index e182060670412c..a7b4b9c42bd5cb 100644 --- a/spec/frontend/pages/projects/forks/new/components/app_spec.js +++ b/spec/frontend/pages/projects/forks/new/components/app_spec.js @@ -13,6 +13,7 @@ describe('App component', () => { projectPath: 'project-name', projectDescription: 'some project description', projectVisibility: 'private', + restrictedVisibilityLevels: [], }; const createComponent = (props = {}) => { diff --git a/spec/frontend/pages/projects/forks/new/components/fork_form_spec.js b/spec/frontend/pages/projects/forks/new/components/fork_form_spec.js index 03338b1930c905..a522ddd22bc239 100644 --- a/spec/frontend/pages/projects/forks/new/components/fork_form_spec.js +++ b/spec/frontend/pages/projects/forks/new/components/fork_form_spec.js @@ -44,6 +44,7 @@ describe('ForkForm component', () => { projectPath: 'project-name', projectDescription: 'some project description', projectVisibility: 'private', + restrictedVisibilityLevels: [], }; const mockGetRequest = (data = {}, statusCode = httpStatus.OK) => { @@ -230,7 +231,7 @@ describe('ForkForm component', () => { expect(wrapper.findAll(GlFormRadio)).toHaveLength(3); }); - it('resets the visibility to default "private" when the namespace is changed', async () => { + describe('when the namespace is changed', () => { const namespaces = [ { visibility: 'private', @@ -243,42 +244,114 @@ describe('ForkForm component', () => { }, ]; - mockGetRequest(); - createComponent( - { - projectVisibility: 'public', - }, - { - namespaces, - }, - ); + beforeEach(() => { + mockGetRequest(); + }); - expect(wrapper.vm.form.fields.visibility.value).toBe('public'); - findFormSelect().vm.$emit('input', namespaces[1]); + it('resets the visibility to default "private"', async () => { + createFullComponent({ projectVisibility: 'public' }, { namespaces }); - await wrapper.vm.$nextTick(); + expect(wrapper.vm.form.fields.visibility.value).toBe('public'); + findFormSelect().vm.$emit('input', namespaces[1]); + + await wrapper.vm.$nextTick(); + + expect(wrapper.vm.form.fields.visibility.value).toBe('private'); + expect(wrapper.find('[name="visibility"]:checked').exists()).toBe(true); + }); + + it('sets the visibility to be null when restrictedVisibilityLevels is set', async () => { + createFullComponent({ restrictedVisibilityLevels: [10] }, { namespaces }); + + findFormSelect().vm.$emit('input', namespaces[1]); + + await wrapper.vm.$nextTick(); + + expect(wrapper.vm.form.fields.visibility.value).toBe(null); + expect(wrapper.find('[name="visibility"]:checked').exists()).toBe(false); + }); + }); + + it.each` + project | restrictedVisibilityLevels + ${'private'} | ${[]} + ${'internal'} | ${[]} + ${'public'} | ${[]} + ${'private'} | ${[0]} + ${'private'} | ${[10]} + ${'private'} | ${[20]} + ${'private'} | ${[0, 10]} + ${'private'} | ${[0, 20]} + ${'private'} | ${[10, 20]} + ${'private'} | ${[0, 10, 20]} + ${'internal'} | ${[0]} + ${'internal'} | ${[10]} + ${'internal'} | ${[20]} + ${'internal'} | ${[0, 10]} + ${'internal'} | ${[0, 20]} + ${'internal'} | ${[10, 20]} + ${'internal'} | ${[0, 10, 20]} + ${'public'} | ${[0]} + ${'public'} | ${[10]} + ${'public'} | ${[0, 10]} + ${'public'} | ${[0, 20]} + ${'public'} | ${[10, 20]} + ${'public'} | ${[0, 10, 20]} + `('checks the correct radio button', async ({ project, restrictedVisibilityLevels }) => { + mockGetRequest(); + createFullComponent({ + projectVisibility: project, + restrictedVisibilityLevels, + }); - expect(wrapper.vm.form.fields.visibility.value).toBe('private'); + if (restrictedVisibilityLevels.length === 0) { + expect(wrapper.find('[name="visibility"]:checked').attributes('value')).toBe(project); + } else { + expect(wrapper.find('[name="visibility"]:checked').exists()).toBe(false); + } }); it.each` - project | namespace | privateIsDisabled | internalIsDisabled | publicIsDisabled - ${'private'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} - ${'private'} | ${'internal'} | ${undefined} | ${'true'} | ${'true'} - ${'private'} | ${'public'} | ${undefined} | ${'true'} | ${'true'} - ${'internal'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} - ${'internal'} | ${'internal'} | ${undefined} | ${undefined} | ${'true'} - ${'internal'} | ${'public'} | ${undefined} | ${undefined} | ${'true'} - ${'public'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} - ${'public'} | ${'internal'} | ${undefined} | ${undefined} | ${'true'} - ${'public'} | ${'public'} | ${undefined} | ${undefined} | ${undefined} + project | namespace | privateIsDisabled | internalIsDisabled | publicIsDisabled | restrictedVisibilityLevels + ${'private'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} | ${[]} + ${'private'} | ${'internal'} | ${undefined} | ${'true'} | ${'true'} | ${[]} + ${'private'} | ${'public'} | ${undefined} | ${'true'} | ${'true'} | ${[]} + ${'internal'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} | ${[]} + ${'internal'} | ${'internal'} | ${undefined} | ${undefined} | ${'true'} | ${[]} + ${'internal'} | ${'public'} | ${undefined} | ${undefined} | ${'true'} | ${[]} + ${'public'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} | ${[]} + ${'public'} | ${'internal'} | ${undefined} | ${undefined} | ${'true'} | ${[]} + ${'public'} | ${'public'} | ${undefined} | ${undefined} | ${undefined} | ${[]} + ${'private'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} | ${[0]} + ${'internal'} | ${'internal'} | ${'true'} | ${undefined} | ${'true'} | ${[0]} + ${'public'} | ${'public'} | ${'true'} | ${undefined} | ${undefined} | ${[0]} + ${'private'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} | ${[10]} + ${'internal'} | ${'internal'} | ${undefined} | ${'true'} | ${'true'} | ${[10]} + ${'public'} | ${'public'} | ${undefined} | ${'true'} | ${undefined} | ${[10]} + ${'private'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} | ${[20]} + ${'internal'} | ${'internal'} | ${undefined} | ${undefined} | ${'true'} | ${[20]} + ${'public'} | ${'public'} | ${undefined} | ${undefined} | ${'true'} | ${[20]} + ${'private'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} | ${[10, 20]} + ${'internal'} | ${'internal'} | ${undefined} | ${'true'} | ${'true'} | ${[10, 20]} + ${'public'} | ${'public'} | ${undefined} | ${'true'} | ${'true'} | ${[10, 20]} + ${'private'} | ${'private'} | ${undefined} | ${'true'} | ${'true'} | ${[0, 10, 20]} + ${'internal'} | ${'internal'} | ${undefined} | ${'true'} | ${'true'} | ${[0, 10, 20]} + ${'public'} | ${'public'} | ${undefined} | ${'true'} | ${'true'} | ${[0, 10, 20]} `( 'sets appropriate radio button disabled state', - async ({ project, namespace, privateIsDisabled, internalIsDisabled, publicIsDisabled }) => { + async ({ + project, + namespace, + privateIsDisabled, + internalIsDisabled, + publicIsDisabled, + restrictedVisibilityLevels, + }) => { mockGetRequest(); createComponent( { projectVisibility: project, + restrictedVisibilityLevels, }, { form: { fields: { namespace: { value: { visibility: namespace } } } }, -- GitLab From ff4529e1189bfc4fa646dfc733ef9586d940f4aa Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Wed, 9 Jun 2021 01:26:50 -0700 Subject: [PATCH 2/3] Address reviewer's comment - Change to use Set - Check radio checked value instead of data --- .../projects/forks/new/components/fork_form.vue | 5 ++++- .../forks/new/components/fork_form_spec.js | 15 ++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue b/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue index d89086f188da9b..1bf40da1f41601 100644 --- a/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue +++ b/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue @@ -138,11 +138,14 @@ export default { visibilityLevelCap() { return Math.min(this.projectVisibilityLevel, this.namespaceVisibilityLevel); }, + restrictedVisibilityLevelsSet() { + return new Set(this.restrictedVisibilityLevels); + }, allowedVisibilityLevels() { const allowedLevels = Object.entries(VISIBILITY_LEVEL).reduce( (levels, [levelName, levelValue]) => { if ( - !this.restrictedVisibilityLevels.includes(levelValue) && + !this.restrictedVisibilityLevelsSet.has(levelValue) && levelValue <= this.visibilityLevelCap ) { levels.push(levelName); diff --git a/spec/frontend/pages/projects/forks/new/components/fork_form_spec.js b/spec/frontend/pages/projects/forks/new/components/fork_form_spec.js index a522ddd22bc239..980ac86a286f57 100644 --- a/spec/frontend/pages/projects/forks/new/components/fork_form_spec.js +++ b/spec/frontend/pages/projects/forks/new/components/fork_form_spec.js @@ -1,4 +1,11 @@ -import { GlFormInputGroup, GlFormInput, GlForm, GlFormRadio, GlFormSelect } from '@gitlab/ui'; +import { + GlFormInputGroup, + GlFormInput, + GlForm, + GlFormRadioGroup, + GlFormRadio, + GlFormSelect, +} from '@gitlab/ui'; import { mount, shallowMount } from '@vue/test-utils'; import axios from 'axios'; import AxiosMockAdapter from 'axios-mock-adapter'; @@ -69,6 +76,7 @@ describe('ForkForm component', () => { stubs: { GlFormInputGroup, GlFormInput, + GlFormRadioGroup, GlFormRadio, }, }); @@ -91,6 +99,7 @@ describe('ForkForm component', () => { }); const findFormSelect = () => wrapper.find(GlFormSelect); + const findRadioGroup = () => wrapper.find(GlFormRadioGroup); const findPrivateRadio = () => wrapper.find('[data-testid="radio-private"]'); const findInternalRadio = () => wrapper.find('[data-testid="radio-internal"]'); const findPublicRadio = () => wrapper.find('[data-testid="radio-public"]'); @@ -256,7 +265,7 @@ describe('ForkForm component', () => { await wrapper.vm.$nextTick(); - expect(wrapper.vm.form.fields.visibility.value).toBe('private'); + expect(findRadioGroup().vm.$attrs.checked).toBe('private'); expect(wrapper.find('[name="visibility"]:checked').exists()).toBe(true); }); @@ -267,7 +276,7 @@ describe('ForkForm component', () => { await wrapper.vm.$nextTick(); - expect(wrapper.vm.form.fields.visibility.value).toBe(null); + expect(findRadioGroup().vm.$attrs.checked).toBe(null); expect(wrapper.find('[name="visibility"]:checked').exists()).toBe(false); }); }); -- GitLab From f7644cccf543421d8bd883b0c34bf6734c6ef804 Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Thu, 10 Jun 2021 17:04:20 -0700 Subject: [PATCH 3/3] Apply patch from Justin - Switch to use selected - Reference by DOM instead of GL components --- .../forks/new/components/fork_form.vue | 1 + locale/gitlab.pot | 3 +++ .../forks/new/components/fork_form_spec.js | 25 +++++++------------ 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue b/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue index 1bf40da1f41601..75c3b6d564ccd7 100644 --- a/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue +++ b/app/assets/javascripts/pages/projects/forks/new/components/fork_form.vue @@ -363,6 +363,7 @@ export default { v-model="form.fields.visibility.value" data-testid="fork-visibility-radio-group" name="visibility" + :aria-label="__('visibility')" required > { axiosMock.restore(); }); - const findFormSelect = () => wrapper.find(GlFormSelect); - const findRadioGroup = () => wrapper.find(GlFormRadioGroup); + const findFormSelectOptions = () => wrapper.find('select[name="namespace"]').findAll('option'); const findPrivateRadio = () => wrapper.find('[data-testid="radio-private"]'); const findInternalRadio = () => wrapper.find('[data-testid="radio-internal"]'); const findPublicRadio = () => wrapper.find('[data-testid="radio-public"]'); @@ -261,23 +254,23 @@ describe('ForkForm component', () => { createFullComponent({ projectVisibility: 'public' }, { namespaces }); expect(wrapper.vm.form.fields.visibility.value).toBe('public'); - findFormSelect().vm.$emit('input', namespaces[1]); + await findFormSelectOptions().at(1).setSelected(); await wrapper.vm.$nextTick(); - expect(findRadioGroup().vm.$attrs.checked).toBe('private'); - expect(wrapper.find('[name="visibility"]:checked').exists()).toBe(true); + expect(getByRole(wrapper.element, 'radio', { name: /private/i }).checked).toBe(true); }); it('sets the visibility to be null when restrictedVisibilityLevels is set', async () => { createFullComponent({ restrictedVisibilityLevels: [10] }, { namespaces }); - findFormSelect().vm.$emit('input', namespaces[1]); + await findFormSelectOptions().at(1).setSelected(); await wrapper.vm.$nextTick(); - expect(findRadioGroup().vm.$attrs.checked).toBe(null); - expect(wrapper.find('[name="visibility"]:checked').exists()).toBe(false); + const container = getByRole(wrapper.element, 'radiogroup', { name: /visibility/i }); + const visibilityRadios = getAllByRole(container, 'radio'); + expect(visibilityRadios.filter((e) => e.checked)).toHaveLength(0); }); }); -- GitLab