diff --git a/app/assets/javascripts/issues/create_merge_request_dropdown.js b/app/assets/javascripts/issues/create_merge_request_dropdown.js index 27ba94c83813f0270f49ff7e00af46023d2d7748..caf82e482ea4e433138a3a0546fa624a3dcacab5 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 @@ -19,6 +23,12 @@ 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'; + function createEndpoint(projectPath, endpoint) { if (canCreateConfidentialMergeRequest()) { return endpoint.replace( @@ -30,6 +40,23 @@ function createEndpoint(projectPath, endpoint) { return endpoint; } +function getValidationError(target, inputValue, validationType) { + const invalidChars = findInvalidBranchNameCharacters(inputValue.value); + let text; + + if (invalidChars && validationType === VALIDATION_TYPE_INVALID_CHARS) { + text = humanizeBranchValidationErrors(invalidChars); + } + + if (validationType === VALIDATION_TYPE_BRANCH_UNAVAILABLE) { + text = + target === INPUT_TARGET_BRANCH + ? __('Branch is already taken') + : __('Source is not available'); + } + + return text; +} export default class CreateMergeRequestDropdown { constructor(wrapperEl) { this.wrapperEl = wrapperEl; @@ -124,18 +151,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 +302,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 +382,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 +410,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 +501,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 +512,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 +520,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 +538,35 @@ 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. + 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) { + this.refInput.dataset.value = ref; 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.refIsValid = true; + 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 +574,24 @@ export default class CreateMergeRequestDropdown { this.refInput.setSelectionRange(ref.length, result.length); } } + } - if (this.inputsAreValid()) { - this.enable(); + 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. + 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/app/assets/javascripts/lib/utils/text_utility.js b/app/assets/javascripts/lib/utils/text_utility.js index 367180714dfb8fbb792d630b769259e945ca4f1a..1bed38b7dbe028503fe394517a8df2e36f32fb03 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,45 @@ 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 = []) => { + const chars = invalidChars.filter((c) => INVALID_BRANCH_NAME_CHARS.includes(c)); + + 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 ''; +}; diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 9c86b87a01a740dad80cbeae57be096cbb060a47..f89b1fbd69aaacdef8e809b0f8a44ebf54258199 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 "" 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 bbc14368d82dc101d78fc3a1279e0b736b15d4ff..6325f226ccf1f8c4179b6bc0d22b6ad8fc3c78fd 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 diff --git a/spec/frontend/lib/utils/text_utility_spec.js b/spec/frontend/lib/utils/text_utility_spec.js index f2572ca0ad29dca1545644f090c02e68c8bc1229..71a84d567911b02145686dce3716a30c9a10ed00 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(''); + }); + }); });