diff options
Diffstat (limited to 'app')
10 files changed, 223 insertions, 48 deletions
diff --git a/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue b/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue index 32fe841f16e..316408adfb2 100644 --- a/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue +++ b/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue @@ -65,12 +65,6 @@ export default { modalActionText() { return this.variableBeingEdited ? __('Update variable') : __('Add variable'); }, - primaryAction() { - return { - text: this.modalActionText, - attributes: { variant: 'success', disabled: !this.canSubmit }, - }; - }, maskedFeedback() { return __('This variable can not be masked'); }, @@ -120,6 +114,8 @@ export default { ref="modal" :modal-id="$options.modalId" :title="modalActionText" + static + lazy @hidden="resetModalHandler" > <form> @@ -127,7 +123,7 @@ export default { <gl-form-input id="ci-variable-key" v-model="variableData.key" - data-qa-selector="variable_key" + data-qa-selector="ci_variable_key_field" /> </gl-form-group> @@ -142,7 +138,7 @@ export default { v-model="variableData.secret_value" rows="3" max-rows="6" - data-qa-selector="variable_value" + data-qa-selector="ci_variable_value_field" /> </gl-form-group> @@ -189,7 +185,7 @@ export default { <gl-form-checkbox ref="masked-ci-variable" v-model="variableData.masked" - data-qa-selector="variable_masked" + data-qa-selector="ci_variable_masked_checkbox" > {{ __('Mask variable') }} <gl-link href="/help/ci/variables/README#masked-variables"> @@ -218,6 +214,7 @@ export default { ref="deleteCiVariable" category="secondary" variant="danger" + data-qa-selector="ci_variable_delete_button" @click="deleteVarAndClose" >{{ __('Delete variable') }}</gl-deprecated-button > @@ -225,6 +222,7 @@ export default { ref="updateOrAddVariable" :disabled="!canSubmit" variant="success" + data-qa-selector="ci_variable_save_button" @click="updateOrAddVariable" >{{ modalActionText }} </gl-deprecated-button> diff --git a/app/assets/javascripts/ci_variable_list/components/ci_variable_table.vue b/app/assets/javascripts/ci_variable_list/components/ci_variable_table.vue index b374d950c1f..7eb791f97e4 100644 --- a/app/assets/javascripts/ci_variable_list/components/ci_variable_table.vue +++ b/app/assets/javascripts/ci_variable_list/components/ci_variable_table.vue @@ -26,7 +26,6 @@ export default { { key: 'value', label: s__('CiVariables|Value'), - tdClass: 'qa-ci-variable-input-value', customStyle: { width: '40%' }, }, { @@ -89,6 +88,7 @@ export default { :fields="fields" :items="variables" tbody-tr-class="js-ci-variable-row" + data-qa-selector="ci_variable_table_content" sort-by="key" sort-direction="asc" stacked="lg" @@ -150,6 +150,7 @@ export default { <gl-deprecated-button ref="edit-ci-variable" v-gl-modal-directive="$options.modalId" + data-qa-selector="edit_ci_variable_button" @click="editVariable(item)" > <gl-icon :size="$options.iconSize" name="pencil" /> @@ -168,7 +169,7 @@ export default { <gl-deprecated-button v-if="tableIsNotEmpty" ref="secret-value-reveal-button" - data-qa-selector="reveal_ci_variable_value" + data-qa-selector="reveal_ci_variable_value_button" class="append-right-8" @click="toggleValues(!valuesHidden)" >{{ valuesButtonText }}</gl-deprecated-button @@ -176,7 +177,7 @@ export default { <gl-deprecated-button ref="add-ci-variable" v-gl-modal-directive="$options.modalId" - data-qa-selector="add_ci_variable" + data-qa-selector="add_ci_variable_button" variant="success" >{{ __('Add Variable') }}</gl-deprecated-button > diff --git a/app/assets/javascripts/lib/utils/text_utility.js b/app/assets/javascripts/lib/utils/text_utility.js index f857e618d89..86714471823 100644 --- a/app/assets/javascripts/lib/utils/text_utility.js +++ b/app/assets/javascripts/lib/utils/text_utility.js @@ -232,3 +232,11 @@ export const truncateNamespace = (string = '') => { return namespace; }; + +/** + * Tests that the input is a String and has at least + * one non-whitespace character + * @param {String} obj The object to test + * @returns {Boolean} + */ +export const hasContent = obj => isString(obj) && obj.trim() !== ''; diff --git a/app/assets/javascripts/releases/components/app_edit.vue b/app/assets/javascripts/releases/components/app_edit.vue index 06e388002e4..df356c18417 100644 --- a/app/assets/javascripts/releases/components/app_edit.vue +++ b/app/assets/javascripts/releases/components/app_edit.vue @@ -1,6 +1,6 @@ <script> -import { mapState, mapActions } from 'vuex'; -import { GlDeprecatedButton, GlLink, GlFormInput, GlFormGroup } from '@gitlab/ui'; +import { mapState, mapActions, mapGetters } from 'vuex'; +import { GlNewButton, GlFormInput, GlFormGroup } from '@gitlab/ui'; import { escape as esc } from 'lodash'; import { __, sprintf } from '~/locale'; import MarkdownField from '~/vue_shared/components/markdown/field.vue'; @@ -15,8 +15,7 @@ export default { components: { GlFormInput, GlFormGroup, - GlDeprecatedButton, - GlLink, + GlNewButton, MarkdownField, AssetLinksForm, }, @@ -27,12 +26,14 @@ export default { computed: { ...mapState('detail', [ 'isFetchingRelease', + 'isUpdatingRelease', 'fetchError', 'markdownDocsPath', 'markdownPreviewPath', 'releasesPagePath', 'updateReleaseApiDocsPath', ]), + ...mapGetters('detail', ['isValid']), showForm() { return !this.isFetchingRelease && !this.fetchError; }, @@ -87,6 +88,9 @@ export default { showAssetLinksForm() { return this.glFeatures.releaseAssetLinkEditing; }, + isSaveChangesDisabled() { + return this.isUpdatingRelease || !this.isValid; + }, }, created() { this.fetchRelease(); @@ -163,17 +167,19 @@ export default { <asset-links-form v-if="showAssetLinksForm" /> <div class="d-flex pt-3"> - <gl-deprecated-button - class="mr-auto js-submit-button" + <gl-new-button + class="mr-auto js-no-auto-disable" + category="primary" variant="success" type="submit" :aria-label="__('Save changes')" + :disabled="isSaveChangesDisabled" > {{ __('Save changes') }} - </gl-deprecated-button> - <gl-link :href="cancelPath" class="js-cancel-button btn btn-default"> + </gl-new-button> + <gl-new-button :href="cancelPath" class="js-cancel-button"> {{ __('Cancel') }} - </gl-link> + </gl-new-button> </div> </form> </div> diff --git a/app/assets/javascripts/releases/components/asset_links_form.vue b/app/assets/javascripts/releases/components/asset_links_form.vue index 7299fd24ec5..6ca700c2b30 100644 --- a/app/assets/javascripts/releases/components/asset_links_form.vue +++ b/app/assets/javascripts/releases/components/asset_links_form.vue @@ -1,10 +1,10 @@ <script> -import { mapState, mapActions } from 'vuex'; +import { mapState, mapActions, mapGetters } from 'vuex'; import { GlSprintf, GlLink, GlFormGroup, - GlDeprecatedButton, + GlNewButton, GlIcon, GlTooltipDirective, GlFormInput, @@ -12,13 +12,14 @@ import { export default { name: 'AssetLinksForm', - components: { GlSprintf, GlLink, GlFormGroup, GlDeprecatedButton, GlIcon, GlFormInput }, + components: { GlSprintf, GlLink, GlFormGroup, GlNewButton, GlIcon, GlFormInput }, directives: { GlTooltip: GlTooltipDirective }, computed: { ...mapState('detail', ['release', 'releaseAssetsDocsPath']), + ...mapGetters('detail', ['validationErrors']), }, created() { - this.addEmptyAssetLink(); + this.ensureAtLeastOneLink(); }, methods: { ...mapActions('detail', [ @@ -32,6 +33,7 @@ export default { }, onRemoveClicked(linkId) { this.removeAssetLink(linkId); + this.ensureAtLeastOneLink(); }, onUrlInput(linkIdToUpdate, newUrl) { this.updateAssetLinkUrl({ linkIdToUpdate, newUrl }); @@ -39,6 +41,37 @@ export default { onLinkTitleInput(linkIdToUpdate, newName) { this.updateAssetLinkName({ linkIdToUpdate, newName }); }, + hasDuplicateUrl(link) { + return Boolean(this.getLinkErrors(link).isDuplicate); + }, + hasBadFormat(link) { + return Boolean(this.getLinkErrors(link).isBadFormat); + }, + hasEmptyUrl(link) { + return Boolean(this.getLinkErrors(link).isUrlEmpty); + }, + hasEmptyName(link) { + return Boolean(this.getLinkErrors(link).isNameEmpty); + }, + getLinkErrors(link) { + return this.validationErrors.assets.links[link.id] || {}; + }, + isUrlValid(link) { + return !this.hasDuplicateUrl(link) && !this.hasBadFormat(link) && !this.hasEmptyUrl(link); + }, + isNameValid(link) { + return !this.hasEmptyName(link); + }, + + /** + * Make sure the form is never completely empty by adding an + * empty row if the form contains 0 links + */ + ensureAtLeastOneLink() { + if (this.release.assets.links.length === 0) { + this.addEmptyAssetLink(); + } + }, }, }; </script> @@ -69,60 +102,93 @@ export default { <p> {{ __( - 'Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance.', + 'Point to any links you like: documentation, built binaries, or other related materials. These can be internal or external links from your GitLab instance. Duplicate URLs are not allowed.', ) }} </p> <div v-for="(link, index) in release.assets.links" :key="link.id" - class="d-flex flex-column flex-sm-row align-items-stretch align-items-sm-end" + class="row flex-column flex-sm-row align-items-stretch align-items-sm-start" > <gl-form-group - class="url-field form-group flex-grow-1 mr-sm-4" + class="url-field form-group col" :label="__('URL')" :label-for="`asset-url-${index}`" > <gl-form-input :id="`asset-url-${index}`" + ref="urlInput" :value="link.url" type="text" class="form-control" + :state="isUrlValid(link)" @change="onUrlInput(link.id, $event)" /> + <template #invalid-feedback> + <span v-if="hasEmptyUrl(link)" class="invalid-feedback d-inline"> + {{ __('URL is required') }} + </span> + <span v-else-if="hasBadFormat(link)" class="invalid-feedback d-inline"> + <gl-sprintf + :message=" + __( + 'URL must start with %{codeStart}http://%{codeEnd}, %{codeStart}https://%{codeEnd}, or %{codeStart}ftp://%{codeEnd}', + ) + " + > + <template #code="{ content }"> + <code>{{ content }}</code> + </template> + </gl-sprintf> + </span> + <span v-else-if="hasDuplicateUrl(link)" class="invalid-feedback d-inline"> + {{ __('This URL is already used for another link; duplicate URLs are not allowed') }} + </span> + </template> </gl-form-group> <gl-form-group - class="link-title-field flex-grow-1 mr-sm-4" + class="link-title-field col" :label="__('Link title')" :label-for="`asset-link-name-${index}`" > <gl-form-input :id="`asset-link-name-${index}`" + ref="nameInput" :value="link.name" type="text" class="form-control" + :state="isNameValid(link)" @change="onLinkTitleInput(link.id, $event)" /> + <template v-slot:invalid-feedback> + <span v-if="hasEmptyName(link)" class="invalid-feedback d-inline"> + {{ __('Link title is required') }} + </span> + </template> </gl-form-group> - <gl-deprecated-button - v-gl-tooltip - class="mb-5 mb-sm-3 flex-grow-0 flex-shrink-0 remove-button" - :aria-label="__('Remove asset link')" - :title="__('Remove asset link')" - @click="onRemoveClicked(link.id)" - > - <gl-icon class="m-0" name="remove" /> - <span class="d-inline d-sm-none">{{ __('Remove asset link') }}</span> - </gl-deprecated-button> + <div class="mb-5 mb-sm-3 mt-sm-4 col col-sm-auto"> + <gl-new-button + v-gl-tooltip + class="remove-button w-100" + :aria-label="__('Remove asset link')" + :title="__('Remove asset link')" + @click="onRemoveClicked(link.id)" + > + <gl-icon class="mr-1 mr-sm-0 mb-1" :size="16" name="remove" /> + <span class="d-inline d-sm-none">{{ __('Remove asset link') }}</span> + </gl-new-button> + </div> </div> - <gl-deprecated-button + <gl-new-button + ref="addAnotherLinkButton" variant="link" class="align-self-end mb-5 mb-sm-0" @click="onAddAnotherClicked" > {{ __('Add another link') }} - </gl-deprecated-button> + </gl-new-button> </div> </template> diff --git a/app/assets/javascripts/releases/stores/modules/detail/getters.js b/app/assets/javascripts/releases/stores/modules/detail/getters.js index 562284dc48d..84dc2fca4be 100644 --- a/app/assets/javascripts/releases/stores/modules/detail/getters.js +++ b/app/assets/javascripts/releases/stores/modules/detail/getters.js @@ -1,9 +1,13 @@ +import { isEmpty } from 'lodash'; +import { hasContent } from '~/lib/utils/text_utility'; + /** + * @param {Object} link The link to test * @returns {Boolean} `true` if the release link is empty, i.e. it has * empty (or whitespace-only) values for both `url` and `name`. * Otherwise, `false`. */ -const isEmptyReleaseLink = l => !/\S/.test(l.url) && !/\S/.test(l.name); +const isEmptyReleaseLink = link => !hasContent(link.url) && !hasContent(link.name); /** Returns all release links that aren't empty */ export const releaseLinksToCreate = state => { @@ -22,3 +26,67 @@ export const releaseLinksToDelete = state => { return state.originalRelease.assets.links; }; + +/** Returns all validation errors on the release object */ +export const validationErrors = state => { + const errors = { + assets: { + links: {}, + }, + }; + + if (!state.release) { + return errors; + } + + // Each key of this object is a URL, and the value is an + // array of Release link objects that share this URL. + // This is used for detecting duplicate URLs. + const urlToLinksMap = new Map(); + + state.release.assets.links.forEach(link => { + errors.assets.links[link.id] = {}; + + // Only validate non-empty URLs + if (isEmptyReleaseLink(link)) { + return; + } + + if (!hasContent(link.url)) { + errors.assets.links[link.id].isUrlEmpty = true; + } + + if (!hasContent(link.name)) { + errors.assets.links[link.id].isNameEmpty = true; + } + + const normalizedUrl = link.url.trim().toLowerCase(); + + // Compare each URL to every other URL and flag any duplicates + if (urlToLinksMap.has(normalizedUrl)) { + // a duplicate URL was found! + + // add a validation error for each link that shares this URL + const duplicates = urlToLinksMap.get(normalizedUrl); + duplicates.push(link); + duplicates.forEach(duplicateLink => { + errors.assets.links[duplicateLink.id].isDuplicate = true; + }); + } else { + // no duplicate URL was found + + urlToLinksMap.set(normalizedUrl, [link]); + } + + if (!/^(http|https|ftp):\/\//.test(normalizedUrl)) { + errors.assets.links[link.id].isBadFormat = true; + } + }); + + return errors; +}; + +/** Returns whether or not the release object is valid */ +export const isValid = (_state, getters) => { + return Object.values(getters.validationErrors.assets.links).every(isEmpty); +}; diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index c418b11ab13..34af1ecd6a5 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -9,6 +9,7 @@ class Import::GithubController < Import::BaseController before_action :expire_etag_cache, only: [:status, :create] rescue_from Octokit::Unauthorized, with: :provider_unauthorized + rescue_from Octokit::TooManyRequests, with: :provider_rate_limit def new if !ci_cd_only? && github_import_configured? && logged_in_with_provider? @@ -142,6 +143,13 @@ class Import::GithubController < Import::BaseController alert: "Access denied to your #{Gitlab::ImportSources.title(provider.to_s)} account." end + def provider_rate_limit(exception) + reset_time = Time.at(exception.response_headers['x-ratelimit-reset'].to_i) + session[access_token_key] = nil + redirect_to new_import_url, + alert: _("GitHub API rate limit exceeded. Try again after %{reset_time}") % { reset_time: reset_time } + end + def access_token_key :"#{provider}_access_token" end @@ -180,7 +188,7 @@ class Import::GithubController < Import::BaseController end def client_options - {} + { wait_for_rate_limit_reset: false } end def extra_import_params diff --git a/app/models/jira_import_state.rb b/app/models/jira_import_state.rb index ec1b8f03d36..543ee77917c 100644 --- a/app/models/jira_import_state.rb +++ b/app/models/jira_import_state.rb @@ -12,6 +12,8 @@ class JiraImportState < ApplicationRecord belongs_to :user belongs_to :label + scope :by_jira_project_key, -> (jira_project_key) { where(jira_project_key: jira_project_key) } + validates :project, presence: true validates :jira_project_key, presence: true validates :jira_project_name, presence: true diff --git a/app/models/project.rb b/app/models/project.rb index ee4cc6157eb..443b44dd023 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1190,14 +1190,14 @@ class Project < ApplicationRecord end def external_issue_tracker - if has_external_issue_tracker.nil? # To populate existing projects + if has_external_issue_tracker.nil? cache_has_external_issue_tracker end if has_external_issue_tracker? - return @external_issue_tracker if defined?(@external_issue_tracker) - - @external_issue_tracker = services.external_issue_trackers.first + strong_memoize(:external_issue_tracker) do + services.external_issue_trackers.first + end else nil end @@ -1217,7 +1217,7 @@ class Project < ApplicationRecord def external_wiki if has_external_wiki.nil? - cache_has_external_wiki # Populate + cache_has_external_wiki end if has_external_wiki diff --git a/app/services/jira_import/start_import_service.rb b/app/services/jira_import/start_import_service.rb index 6accc3cfffc..e8d9e6734bd 100644 --- a/app/services/jira_import/start_import_service.rb +++ b/app/services/jira_import/start_import_service.rb @@ -33,8 +33,10 @@ module JiraImport end def build_jira_import + label = create_import_label(project) project.jira_imports.build( user: user, + label: label, jira_project_key: jira_project_key, # we do not have the jira_project_name or jira_project_xid yet so just set a mock value, # we will once https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28190 @@ -43,6 +45,22 @@ module JiraImport ) end + def create_import_label(project) + label = ::Labels::CreateService.new(build_label_attrs(project)).execute(project: project) + raise Projects::ImportService::Error, _('Failed to create import label for jira import.') if label.blank? + + label + end + + def build_label_attrs(project) + import_start_time = Time.zone.now + jira_imports_for_project = project.jira_imports.by_jira_project_key(jira_project_key).size + 1 + title = "jira-import::#{jira_project_key}-#{jira_imports_for_project}" + description = "Label for issues that were imported from jira on #{import_start_time.strftime('%Y-%m-%d %H:%M:%S')}" + color = "#{Label.color_for(title)}" + { title: title, description: description, color: color } + end + def validate return build_error_response(_('Jira import feature is disabled.')) unless project.jira_issues_import_feature_flag_enabled? return build_error_response(_('You do not have permissions to run the import.')) unless user.can?(:admin_project, project) |