From 92fee4470e70096b0c695dd822d2a64add147124 Mon Sep 17 00:00:00 2001 From: Patrick Cyiza Date: Mon, 24 Oct 2022 14:11:05 +0200 Subject: [PATCH 1/5] Warning when branch name from issue contains a white space Changelog: added --- .../issues/create_merge_request_dropdown.js | 135 ++++++++++++------ locale/gitlab.pot | 9 ++ 2 files changed, 104 insertions(+), 40 deletions(-) diff --git a/app/assets/javascripts/issues/create_merge_request_dropdown.js b/app/assets/javascripts/issues/create_merge_request_dropdown.js index 27ba94c83813f0..4808cc4bf99763 100644 --- a/app/assets/javascripts/issues/create_merge_request_dropdown.js +++ b/app/assets/javascripts/issues/create_merge_request_dropdown.js @@ -19,6 +19,14 @@ const InputSetter = { ...ISetter }; const CREATE_MERGE_REQUEST = 'create-mr'; const CREATE_BRANCH = 'create-branch'; +const VALIDATION_TYPE_BRANCH_UNAVAILABLE = 'branch_unavailable'; +const VALIDATION_TYPE_INVALID_CHARS = 'invalid_chars'; + +const INPUT_TARGET_BRANCH = 'branch'; +const INPUT_TARGET_REF = 'ref'; + +const INVALID_CHARS_PATTERN = /(\s|~|\^|:|\?|\*|\[|\\|\.\.|@\{|\/{2,})/g; + function createEndpoint(projectPath, endpoint) { if (canCreateConfidentialMergeRequest()) { return endpoint.replace( @@ -30,6 +38,38 @@ function createEndpoint(projectPath, endpoint) { return endpoint; } +function getValidationError(target, inputValue, validationType) { + const invalidChars = inputValue.value.match(INVALID_CHARS_PATTERN); + let text; + if (invalidChars && validationType === VALIDATION_TYPE_INVALID_CHARS) { + if (invalidChars.includes(' ')) { + const excludeSpaces = invalidChars.filter((v) => v !== ' '); + if (excludeSpaces.length) { + text = sprintf(__("Can't contain spaces, %{chars}"), { + chars: excludeSpaces.join(', '), + }); + } else { + text = sprintf(__("Can't contain spaces")); + } + } else { + text = sprintf(__("Can't contain %{chars}"), { chars: invalidChars.join(',') }); + } + } + + if (validationType === VALIDATION_TYPE_BRANCH_UNAVAILABLE) { + text = + target === INPUT_TARGET_BRANCH + ? __('Branch is already taken') + : __('Source is not available'); + } + + return text; +} + +function containsInvalidCharacters(branchName) { + return INVALID_CHARS_PATTERN.test(branchName); +} + export default class CreateMergeRequestDropdown { constructor(wrapperEl) { this.wrapperEl = wrapperEl; @@ -124,18 +164,19 @@ export default class CreateMergeRequestDropdown { .then(({ data }) => { this.setUnavailableButtonState(false); - if (data.can_create_branch) { - this.available(); - this.enable(); - this.updateBranchName(data.suggested_branch_name); - - if (!this.droplabInitialized) { - this.droplabInitialized = true; - this.initDroplab(); - this.bindEvents(); - } - } else { + if (!data.can_create_branch) { this.hide(); + return; + } + + this.available(); + this.enable(); + this.updateBranchName(data.suggested_branch_name); + + if (!this.droplabInitialized) { + this.droplabInitialized = true; + this.initDroplab(); + this.bindEvents(); } }) .catch(() => { @@ -274,7 +315,7 @@ export default class CreateMergeRequestDropdown { const tags = data[Object.keys(data)[1]]; let result; - if (target === 'branch') { + if (target === INPUT_TARGET_BRANCH) { result = CreateMergeRequestDropdown.findByValue(branches, ref); } else { result = @@ -354,10 +395,10 @@ export default class CreateMergeRequestDropdown { } if (event.target === this.branchInput) { - target = 'branch'; + target = INPUT_TARGET_BRANCH; ({ value } = this.branchInput); } else if (event.target === this.refInput) { - target = 'ref'; + target = INPUT_TARGET_REF; if (event.target === document.activeElement) { value = event.target.value.slice(0, event.target.selectionStart) + @@ -382,7 +423,7 @@ export default class CreateMergeRequestDropdown { this.createBranchPath = this.wrapperEl.dataset.createBranchPath; this.createMrPath = this.wrapperEl.dataset.createMrPath; - if (target === 'branch') { + if (target === INPUT_TARGET_BRANCH) { this.branchIsValid = true; } else { this.refIsValid = true; @@ -473,7 +514,7 @@ export default class CreateMergeRequestDropdown { showAvailableMessage(target) { const { input, message } = this.getTargetData(target); - const text = target === 'branch' ? __('Branch name') : __('Source'); + const text = target === INPUT_TARGET_BRANCH ? __('Branch name') : __('Source'); this.removeMessage(target); input.classList.add('gl-field-success-outline'); @@ -484,7 +525,7 @@ export default class CreateMergeRequestDropdown { showCheckingMessage(target) { const { message } = this.getTargetData(target); - const text = target === 'branch' ? __('branch name') : __('source'); + const text = target === INPUT_TARGET_BRANCH ? __('branch name') : __('source'); this.removeMessage(target); message.classList.add('gl-text-gray-600'); @@ -492,10 +533,9 @@ export default class CreateMergeRequestDropdown { message.style.display = 'inline-block'; } - showNotAvailableMessage(target) { + showNotAvailableMessage(target, validationType = VALIDATION_TYPE_BRANCH_UNAVAILABLE) { const { input, message } = this.getTargetData(target); - const text = - target === 'branch' ? __('Branch is already taken') : __('Source is not available'); + const text = getValidationError(target, input, validationType); this.removeMessage(target); input.classList.add('gl-field-error-outline'); @@ -511,35 +551,38 @@ export default class CreateMergeRequestDropdown { updateBranchName(suggestedBranchName) { this.branchInput.value = suggestedBranchName; - this.updateCreatePaths('branch', suggestedBranchName); + this.updateInputState(INPUT_TARGET_BRANCH, suggestedBranchName, ''); + this.updateCreatePaths(INPUT_TARGET_BRANCH, suggestedBranchName); } updateInputState(target, ref, result) { // target - 'branch' or 'ref' - which the input field we are searching a ref for. // ref - string - what a user typed. // result - string - what has been found on backend. + this.branchIsValid = true; + this.refIsValid = true; + + if (target === INPUT_TARGET_BRANCH) this.updateTargetBranchInput(ref, result); + + if (target === INPUT_TARGET_REF) this.updateRefInput(ref, result); + + if (this.inputsAreValid()) { + this.enable(); + } else { + this.disableCreateAction(); + } + } - // If a found branch equals exact the same text a user typed, - // that means a new branch cannot be created as it already exists. + updateRefInput(ref, result) { if (ref === result) { - if (target === 'branch') { - this.branchIsValid = false; - this.showNotAvailableMessage('branch'); - } else { - this.refIsValid = true; - this.refInput.dataset.value = ref; - this.showAvailableMessage('ref'); - this.updateCreatePaths(target, ref); - } - } else if (target === 'branch') { - this.branchIsValid = true; - this.showAvailableMessage('branch'); - this.updateCreatePaths(target, ref); + this.refInput.dataset.value = ref; + this.showAvailableMessage(INPUT_TARGET_REF); + this.updateCreatePaths(INPUT_TARGET_REF, ref); } else { this.refIsValid = false; this.refInput.dataset.value = ref; this.disableCreateAction(); - this.showNotAvailableMessage('ref'); + this.showNotAvailableMessage(INPUT_TARGET_REF); // Show ref hint. if (result) { @@ -547,11 +590,23 @@ export default class CreateMergeRequestDropdown { this.refInput.setSelectionRange(ref.length, result.length); } } + } - if (this.inputsAreValid()) { - this.enable(); + updateTargetBranchInput(ref, result) { + const isInvalidString = containsInvalidCharacters(ref); + + if (ref !== result && !isInvalidString) { + // If a found branch equals exact the same text a user typed, + // Or user typed input contains invalid chars, + // that means a new branch cannot be created as it already exists. + this.showAvailableMessage(INPUT_TARGET_BRANCH, VALIDATION_TYPE_BRANCH_UNAVAILABLE); + this.updateCreatePaths(INPUT_TARGET_BRANCH, ref); + } else if (isInvalidString) { + this.branchIsValid = false; + this.showNotAvailableMessage(INPUT_TARGET_BRANCH, VALIDATION_TYPE_INVALID_CHARS); } else { - this.disableCreateAction(); + this.branchIsValid = false; + this.showNotAvailableMessage(INPUT_TARGET_BRANCH); } } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 9c86b87a01a740..f89b1fbd69aaac 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8015,6 +8015,15 @@ msgstr "" msgid "Can't be empty" msgstr "" +msgid "Can't contain %{chars}" +msgstr "" + +msgid "Can't contain spaces" +msgstr "" + +msgid "Can't contain spaces, %{chars}" +msgstr "" + msgid "Can't create snippet: %{err}" msgstr "" -- GitLab From c6fe09e4d45d0f516a8c7885b846e68b94aa3c0f Mon Sep 17 00:00:00 2001 From: Ezekiel Kigbo Date: Fri, 3 Feb 2023 17:47:31 +1100 Subject: [PATCH 2/5] Add feature specs for invalid branch name --- ...r_creates_branch_and_merge_request_spec.rb | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/spec/features/issues/user_creates_branch_and_merge_request_spec.rb b/spec/features/issues/user_creates_branch_and_merge_request_spec.rb index bbc14368d82dc1..6325f226ccf1f8 100644 --- a/spec/features/issues/user_creates_branch_and_merge_request_spec.rb +++ b/spec/features/issues/user_creates_branch_and_merge_request_spec.rb @@ -113,6 +113,26 @@ expect(page).to have_current_path project_tree_path(project, branch_name), ignore_query: true end end + + context 'when branch name is invalid' do + shared_examples 'has error message' do |dropdown| + it 'has error message' do + select_dropdown_option(dropdown, 'custom-branch-name w~th ^bad chars?') + + wait_for_requests + + expect(page).to have_text("Can't contain spaces, ~, ^, ?") + end + end + + context 'when creating a merge request', :sidekiq_might_not_need_inline do + it_behaves_like 'has error message', 'create-mr' + end + + context 'when creating a branch', :sidekiq_might_not_need_inline do + it_behaves_like 'has error message', 'create-branch' + end + end end context "when there is a referenced merge request" do -- GitLab From 4e388a21111ac95efd921bcb24b9635032e42d81 Mon Sep 17 00:00:00 2001 From: Ezekiel Kigbo Date: Fri, 3 Feb 2023 17:49:04 +1100 Subject: [PATCH 3/5] Refactor brnach validation code Refactors the branch validation code, moving it to text_utility also replaces the regex with a simpler array of characters to check for. Adds unit tests for the utils --- .../javascripts/lib/utils/text_utility.js | 47 +++++++++++++++++++ spec/frontend/lib/utils/text_utility_spec.js | 32 +++++++++++++ 2 files changed, 79 insertions(+) diff --git a/app/assets/javascripts/lib/utils/text_utility.js b/app/assets/javascripts/lib/utils/text_utility.js index 367180714dfb8f..fdfc7efc67d838 100644 --- a/app/assets/javascripts/lib/utils/text_utility.js +++ b/app/assets/javascripts/lib/utils/text_utility.js @@ -1,4 +1,5 @@ import { isString, memoize } from 'lodash'; +import { sprintf, __ } from '~/locale'; import { base64ToBuffer, bufferToBase64 } from '~/authentication/webauthn/util'; import { TRUNCATE_WIDTH_DEFAULT_WIDTH, @@ -525,3 +526,49 @@ export function base64DecodeUnicode(str) { const decoder = new TextDecoder('utf8'); return decoder.decode(base64ToBuffer(str)); } + +// returns an array of errors (if there are any) +const INVALID_BRANCH_NAME_CHARS = [' ', '~', '^', ':', '?', '*', '[', '..', '@{', '\\', '//']; + +/** + * Returns an array of invalid characters found in a branch name + * + * @param {String} name branch name to check + * @return {Array} Array of invalid characters found + */ +export const findInvalidBranchNameCharacters = (name) => { + const invalidChars = []; + + INVALID_BRANCH_NAME_CHARS.forEach((pattern) => { + if (name.indexOf(pattern) > -1) { + invalidChars.push(pattern); + } + }); + + return invalidChars; +}; + +/** + * Returns a string describing validation errors for a branch name + * + * @param {Array} invalidChars Array of invalid characters that were found + * @return {String} Error message describing on the invalid characters found + */ +export const humanizeBranchValidationErrors = (invalidChars = []) => { + let msg = ''; + + const chars = invalidChars.filter((c) => INVALID_BRANCH_NAME_CHARS.includes(c)); + if (!chars.length) return ''; + + if (chars.includes(' ')) { + msg = + chars.length > 1 + ? sprintf(__("Can't contain spaces, %{chars}"), { + chars: chars.filter((c) => c !== ' ').join(', '), + }) + : __("Can't contain spaces"); + } else { + msg = sprintf(__("Can't contain %{chars}"), { chars: chars.join(', ') }); + } + return msg; +}; diff --git a/spec/frontend/lib/utils/text_utility_spec.js b/spec/frontend/lib/utils/text_utility_spec.js index f2572ca0ad29dc..71a84d567911b0 100644 --- a/spec/frontend/lib/utils/text_utility_spec.js +++ b/spec/frontend/lib/utils/text_utility_spec.js @@ -398,4 +398,36 @@ describe('text_utility', () => { expect(textUtils.base64DecodeUnicode('8J+YgA==')).toBe('😀'); }); }); + + describe('findInvalidBranchNameCharacters', () => { + const invalidChars = [' ', '~', '^', ':', '?', '*', '[', '..', '@{', '\\', '//']; + const badBranchName = 'branch-with all these ~ ^ : ? * [ ] \\ // .. @{ } //'; + const goodBranch = 'branch-with-no-errrors'; + + it('returns an array of invalid characters in a branch name', () => { + const chars = textUtils.findInvalidBranchNameCharacters(badBranchName); + chars.forEach((char) => { + expect(invalidChars).toContain(char); + }); + }); + + it('returns an empty array with no invalid characters', () => { + expect(textUtils.findInvalidBranchNameCharacters(goodBranch)).toEqual([]); + }); + }); + + describe('humanizeBranchValidationErrors', () => { + it.each` + errors | message + ${[' ']} | ${"Can't contain spaces"} + ${['?', '//', ' ']} | ${"Can't contain spaces, ?, //"} + ${['\\', '[', '..']} | ${"Can't contain \\, [, .."} + `('returns an $message with $errors', ({ errors, message }) => { + expect(textUtils.humanizeBranchValidationErrors(errors)).toEqual(message); + }); + + it('returns an empty string with no invalid characters', () => { + expect(textUtils.humanizeBranchValidationErrors([])).toEqual(''); + }); + }); }); -- GitLab From 5346d382b6ec0199f5d1efd2bb426b1163c214b1 Mon Sep 17 00:00:00 2001 From: Ezekiel Kigbo Date: Fri, 3 Feb 2023 17:52:34 +1100 Subject: [PATCH 4/5] Use branch validation checks in create MR dropdown --- .../issues/create_merge_request_dropdown.js | 30 ++++++------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/issues/create_merge_request_dropdown.js b/app/assets/javascripts/issues/create_merge_request_dropdown.js index 4808cc4bf99763..2c48eab95f212a 100644 --- a/app/assets/javascripts/issues/create_merge_request_dropdown.js +++ b/app/assets/javascripts/issues/create_merge_request_dropdown.js @@ -11,6 +11,10 @@ import { createAlert } from '~/flash'; import axios from '~/lib/utils/axios_utils'; import { __, sprintf } from '~/locale'; import { mergeUrlParams } from '~/lib/utils/url_utility'; +import { + findInvalidBranchNameCharacters, + humanizeBranchValidationErrors, +} from '~/lib/utils/text_utility'; import api from '~/api'; // Todo: Remove this when fixing issue in input_setter plugin @@ -25,8 +29,6 @@ const VALIDATION_TYPE_INVALID_CHARS = 'invalid_chars'; const INPUT_TARGET_BRANCH = 'branch'; const INPUT_TARGET_REF = 'ref'; -const INVALID_CHARS_PATTERN = /(\s|~|\^|:|\?|\*|\[|\\|\.\.|@\{|\/{2,})/g; - function createEndpoint(projectPath, endpoint) { if (canCreateConfidentialMergeRequest()) { return endpoint.replace( @@ -39,21 +41,11 @@ function createEndpoint(projectPath, endpoint) { } function getValidationError(target, inputValue, validationType) { - const invalidChars = inputValue.value.match(INVALID_CHARS_PATTERN); + const invalidChars = findInvalidBranchNameCharacters(inputValue.value); let text; + if (invalidChars && validationType === VALIDATION_TYPE_INVALID_CHARS) { - if (invalidChars.includes(' ')) { - const excludeSpaces = invalidChars.filter((v) => v !== ' '); - if (excludeSpaces.length) { - text = sprintf(__("Can't contain spaces, %{chars}"), { - chars: excludeSpaces.join(', '), - }); - } else { - text = sprintf(__("Can't contain spaces")); - } - } else { - text = sprintf(__("Can't contain %{chars}"), { chars: invalidChars.join(',') }); - } + text = humanizeBranchValidationErrors(invalidChars); } if (validationType === VALIDATION_TYPE_BRANCH_UNAVAILABLE) { @@ -65,11 +57,6 @@ function getValidationError(target, inputValue, validationType) { return text; } - -function containsInvalidCharacters(branchName) { - return INVALID_CHARS_PATTERN.test(branchName); -} - export default class CreateMergeRequestDropdown { constructor(wrapperEl) { this.wrapperEl = wrapperEl; @@ -593,7 +580,8 @@ export default class CreateMergeRequestDropdown { } updateTargetBranchInput(ref, result) { - const isInvalidString = containsInvalidCharacters(ref); + const branchNameErrors = findInvalidBranchNameCharacters(ref); + const isInvalidString = branchNameErrors.length; if (ref !== result && !isInvalidString) { // If a found branch equals exact the same text a user typed, -- GitLab From 4be366865b065c5943a45aff0ee7fcdc6ded9ee9 Mon Sep 17 00:00:00 2001 From: Ezekiel Kigbo Date: Wed, 22 Feb 2023 15:42:28 +1100 Subject: [PATCH 5/5] Address maintainer review comments --- .../issues/create_merge_request_dropdown.js | 9 +++---- .../javascripts/lib/utils/text_utility.js | 24 ++++++++----------- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/app/assets/javascripts/issues/create_merge_request_dropdown.js b/app/assets/javascripts/issues/create_merge_request_dropdown.js index 2c48eab95f212a..caf82e482ea4e4 100644 --- a/app/assets/javascripts/issues/create_merge_request_dropdown.js +++ b/app/assets/javascripts/issues/create_merge_request_dropdown.js @@ -546,11 +546,7 @@ export default class CreateMergeRequestDropdown { // target - 'branch' or 'ref' - which the input field we are searching a ref for. // ref - string - what a user typed. // result - string - what has been found on backend. - this.branchIsValid = true; - this.refIsValid = true; - if (target === INPUT_TARGET_BRANCH) this.updateTargetBranchInput(ref, result); - if (target === INPUT_TARGET_REF) this.updateRefInput(ref, result); if (this.inputsAreValid()) { @@ -561,8 +557,9 @@ export default class CreateMergeRequestDropdown { } updateRefInput(ref, result) { + this.refInput.dataset.value = ref; if (ref === result) { - this.refInput.dataset.value = ref; + this.refIsValid = true; this.showAvailableMessage(INPUT_TARGET_REF); this.updateCreatePaths(INPUT_TARGET_REF, ref); } else { @@ -582,8 +579,8 @@ export default class CreateMergeRequestDropdown { updateTargetBranchInput(ref, result) { const branchNameErrors = findInvalidBranchNameCharacters(ref); const isInvalidString = branchNameErrors.length; - if (ref !== result && !isInvalidString) { + this.branchIsValid = true; // If a found branch equals exact the same text a user typed, // Or user typed input contains invalid chars, // that means a new branch cannot be created as it already exists. diff --git a/app/assets/javascripts/lib/utils/text_utility.js b/app/assets/javascripts/lib/utils/text_utility.js index fdfc7efc67d838..1bed38b7dbe028 100644 --- a/app/assets/javascripts/lib/utils/text_utility.js +++ b/app/assets/javascripts/lib/utils/text_utility.js @@ -555,20 +555,16 @@ export const findInvalidBranchNameCharacters = (name) => { * @return {String} Error message describing on the invalid characters found */ export const humanizeBranchValidationErrors = (invalidChars = []) => { - let msg = ''; - const chars = invalidChars.filter((c) => INVALID_BRANCH_NAME_CHARS.includes(c)); - if (!chars.length) return ''; - - if (chars.includes(' ')) { - msg = - chars.length > 1 - ? sprintf(__("Can't contain spaces, %{chars}"), { - chars: chars.filter((c) => c !== ' ').join(', '), - }) - : __("Can't contain spaces"); - } else { - msg = sprintf(__("Can't contain %{chars}"), { chars: chars.join(', ') }); + + if (chars.length && !chars.includes(' ')) { + return sprintf(__("Can't contain %{chars}"), { chars: chars.join(', ') }); + } else if (chars.includes(' ') && chars.length <= 1) { + return __("Can't contain spaces"); + } else if (chars.includes(' ') && chars.length > 1) { + return sprintf(__("Can't contain spaces, %{chars}"), { + chars: chars.filter((c) => c !== ' ').join(', '), + }); } - return msg; + return ''; }; -- GitLab