diff options
84 files changed, 1674 insertions, 948 deletions
diff --git a/.eslintrc.yml b/.eslintrc.yml index f8bc2a3ae94..827604ced9a 100644 --- a/.eslintrc.yml +++ b/.eslintrc.yml @@ -27,7 +27,6 @@ rules: - ignore: # https://gitlab.com/gitlab-org/gitlab/issues/38226 - '^ee_component/' - import/order: off # Disabled for now, to make the airbnb-base 12.1.0 -> 13.1.0 update smoother no-else-return: - error diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index ca594b71054..f020e0af588 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1483,7 +1483,6 @@ Rails/SaveBang: - 'spec/support/shared_examples/models/members_notifications_shared_example.rb' - 'spec/support/shared_examples/models/mentionable_shared_examples.rb' - 'spec/support/shared_examples/models/project_latest_successful_build_for_shared_examples.rb' - - 'spec/support/shared_examples/models/relative_positioning_shared_examples.rb' - 'spec/support/shared_examples/models/slack_mattermost_notifications_shared_examples.rb' - 'spec/support/shared_examples/models/update_project_statistics_shared_examples.rb' - 'spec/support/shared_examples/models/with_uploads_shared_examples.rb' diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 3d4bf7f0ea0..9a336d1b698 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -14370f86aaddaf173ff205148704eab118d8d181 +56251315578cd17c0ceebcb911e8d3ddb159afca diff --git a/app/assets/javascripts/alert_management/components/alert_details.vue b/app/assets/javascripts/alert_management/components/alert_details.vue index 49eff61e12f..5d260fcc200 100644 --- a/app/assets/javascripts/alert_management/components/alert_details.vue +++ b/app/assets/javascripts/alert_management/components/alert_details.vue @@ -113,8 +113,8 @@ export default { errored: false, sidebarStatus: false, isErrorDismissed: false, - createIssueError: '', - issueCreationInProgress: false, + createIncidentError: '', + incidentCreationInProgress: false, sidebarErrorMessage: '', }; }, @@ -172,8 +172,8 @@ export default { this.errored = true; this.sidebarErrorMessage = errorMessage; }, - createIssue() { - this.issueCreationInProgress = true; + createIncident() { + this.incidentCreationInProgress = true; this.$apollo .mutate({ @@ -185,18 +185,18 @@ export default { }) .then(({ data: { createAlertIssue: { errors, issue } } }) => { if (errors?.length) { - [this.createIssueError] = errors; - this.issueCreationInProgress = false; + [this.createIncidentError] = errors; + this.incidentCreationInProgress = false; } else if (issue) { - visitUrl(this.issuePath(issue.iid)); + visitUrl(this.incidentPath(issue.iid)); } }) .catch(error => { - this.createIssueError = error; - this.issueCreationInProgress = false; + this.createIncidentError = error; + this.incidentCreationInProgress = false; }); }, - issuePath(issueId) { + incidentPath(issueId) { return joinPaths(this.projectIssuesPath, issueId); }, trackPageViews() { @@ -213,12 +213,12 @@ export default { <p v-html="sidebarErrorMessage || $options.i18n.errorMsg"></p> </gl-alert> <gl-alert - v-if="createIssueError" + v-if="createIncidentError" variant="danger" - data-testid="issueCreationError" - @dismiss="createIssueError = null" + data-testid="incidentCreationError" + @dismiss="createIncidentError = null" > - {{ createIssueError }} + {{ createIncidentError }} </gl-alert> <div v-if="loading"><gl-loading-icon size="lg" class="gl-mt-5" /></div> <div @@ -244,24 +244,24 @@ export default { </div> <gl-button v-if="alert.issueIid" - class="gl-mt-3 mt-sm-0 align-self-center align-self-sm-baseline alert-details-issue-button" - data-testid="viewIssueBtn" - :href="issuePath(alert.issueIid)" + class="gl-mt-3 mt-sm-0 align-self-center align-self-sm-baseline alert-details-incident-button" + data-testid="viewIncidentBtn" + :href="incidentPath(alert.issueIid)" category="primary" variant="success" > - {{ s__('AlertManagement|View issue') }} + {{ s__('AlertManagement|View incident') }} </gl-button> <gl-button v-else - class="gl-mt-3 mt-sm-0 align-self-center align-self-sm-baseline alert-details-issue-button" - data-testid="createIssueBtn" - :loading="issueCreationInProgress" + class="gl-mt-3 mt-sm-0 align-self-center align-self-sm-baseline alert-details-incident-button" + data-testid="createIncidentBtn" + :loading="incidentCreationInProgress" category="primary" variant="success" - @click="createIssue()" + @click="createIncident()" > - {{ s__('AlertManagement|Create issue') }} + {{ s__('AlertManagement|Create incident') }} </gl-button> <gl-button :aria-label="__('Toggle sidebar')" diff --git a/app/assets/javascripts/alert_management/components/alert_management_empty_state.vue b/app/assets/javascripts/alert_management/components/alert_management_empty_state.vue index 13b6a8e6653..68443166f40 100644 --- a/app/assets/javascripts/alert_management/components/alert_management_empty_state.vue +++ b/app/assets/javascripts/alert_management/components/alert_management_empty_state.vue @@ -1,6 +1,7 @@ <script> -import { GlEmptyState, GlButton } from '@gitlab/ui'; +import { GlEmptyState, GlButton, GlLink } from '@gitlab/ui'; import { s__ } from '~/locale'; +import alertsHelpUrlQuery from '../graphql/queries/alert_help_url.query.graphql'; export default { i18n: { @@ -25,6 +26,12 @@ export default { components: { GlEmptyState, GlButton, + GlLink, + }, + apollo: { + alertsHelpUrl: { + query: alertsHelpUrlQuery, + }, }, props: { enableAlertManagementPath: { @@ -50,6 +57,11 @@ export default { default: '', }, }, + data() { + return { + alertsHelpUrl: '', + }; + }, computed: { emptyState() { return { @@ -71,13 +83,9 @@ export default { <template #description> <div class="gl-display-block"> <span>{{ emptyState.info }}</span> - <a - v-if="!opsgenieMvcEnabled" - href="/help/user/project/operations/alert_management.html" - target="_blank" - > + <gl-link v-if="!opsgenieMvcEnabled" :href="alertsHelpUrl" target="_blank"> {{ $options.i18n.moreInformation }} - </a> + </gl-link> </div> <div v-if="alertsCanBeEnabled" class="gl-display-block center gl-pt-4"> <gl-button category="primary" variant="success" :href="emptyState.link"> diff --git a/app/assets/javascripts/alert_management/graphql/queries/alert_help_url.query.graphql b/app/assets/javascripts/alert_management/graphql/queries/alert_help_url.query.graphql new file mode 100644 index 00000000000..05a8bc7c736 --- /dev/null +++ b/app/assets/javascripts/alert_management/graphql/queries/alert_help_url.query.graphql @@ -0,0 +1,3 @@ +query alertsHelpUrl { + alertsHelpUrl @client +} diff --git a/app/assets/javascripts/alert_management/list.js b/app/assets/javascripts/alert_management/list.js index ebdb4954ab7..e180ab5f7e3 100644 --- a/app/assets/javascripts/alert_management/list.js +++ b/app/assets/javascripts/alert_management/list.js @@ -16,6 +16,7 @@ export default () => { enableAlertManagementPath, emptyAlertSvgPath, populatingAlertsHelpUrl, + alertsHelpUrl, opsgenieMvcTargetUrl, } = domEl.dataset; let { alertManagementEnabled, userCanEnableAlertManagement, opsgenieMvcEnabled } = domEl.dataset; @@ -41,6 +42,12 @@ export default () => { ), }); + apolloProvider.clients.defaultClient.cache.writeData({ + data: { + alertsHelpUrl, + }, + }); + return new Vue({ el: selector, apolloProvider, diff --git a/app/assets/javascripts/alerts_settings/components/alerts_settings_form.vue b/app/assets/javascripts/alerts_settings/components/alerts_settings_form.vue index 3c1ffde41c8..f0bb8b0a90f 100644 --- a/app/assets/javascripts/alerts_settings/components/alerts_settings_form.vue +++ b/app/assets/javascripts/alerts_settings/components/alerts_settings_form.vue @@ -51,52 +51,26 @@ export default { 'gl-modal': GlModalDirective, }, mixins: [glFeatureFlagsMixin()], - props: { - prometheus: { - type: Object, - required: true, - validator: ({ activated }) => { - return activated !== undefined; - }, - }, - generic: { - type: Object, - required: true, - validator: ({ formPath }) => { - return formPath !== undefined; - }, - }, - opsgenie: { - type: Object, - required: true, - }, - }, + inject: ['prometheus', 'generic', 'opsgenie'], data() { return { - activated: { - generic: this.generic.activated, - prometheus: this.prometheus.activated, - opsgenie: this.opsgenie?.activated, - }, loading: false, - authorizationKey: { - generic: this.generic.initialAuthorizationKey, - prometheus: this.prometheus.prometheusAuthorizationKey, - }, selectedEndpoint: serviceOptions[0].value, options: serviceOptions, - targetUrl: null, + active: false, + authKey: '', + targetUrl: '', feedback: { variant: 'danger', - feedbackMessage: null, + feedbackMessage: '', isFeedbackDismissed: false, }, - serverError: null, testAlert: { json: null, error: null, }, canSaveForm: false, + serverError: null, }; }, computed: { @@ -123,24 +97,24 @@ export default { case 'generic': { return { url: this.generic.url, - authKey: this.authorizationKey.generic, - active: this.activated.generic, - resetKey: this.resetGenericKey.bind(this), + authKey: this.generic.authorizationKey, + activated: this.generic.activated, + resetKey: this.resetKey.bind(this), }; } case 'prometheus': { return { url: this.prometheus.prometheusUrl, - authKey: this.authorizationKey.prometheus, - active: this.activated.prometheus, - resetKey: this.resetPrometheusKey.bind(this), + authKey: this.prometheus.authorizationKey, + activated: this.prometheus.activated, + resetKey: this.resetKey.bind(this, 'prometheus'), targetUrl: this.prometheus.prometheusApiUrl, }; } case 'opsgenie': { return { targetUrl: this.opsgenie.opsgenieMvcTargetUrl, - active: this.activated.opsgenie, + activated: this.opsgenie.activated, }; } default: { @@ -164,7 +138,7 @@ export default { return this.testAlert.error === null; }, canTestAlert() { - return this.selectedService.active && this.testAlert.json !== null; + return this.active && this.testAlert.json !== null; }, canSaveConfig() { return !this.loading && this.canSaveForm; @@ -187,19 +161,21 @@ export default { }, mounted() { if ( - this.activated.prometheus || - this.activated.generic || + this.prometheus.activated || + this.generic.activated || !this.opsgenie.opsgenieMvcIsAvailable ) { this.removeOpsGenieOption(); - } else if (this.activated.opsgenie) { + } else if (this.opsgenie.activated) { this.setOpsgenieAsDefault(); } + this.active = this.selectedService.activated; + this.authKey = this.selectedService.authKey ?? ''; }, methods: { - createUserErrorMessage(errors) { + createUserErrorMessage(errors = { error: [''] }) { // eslint-disable-next-line prefer-destructuring - this.serverError = Object.values(errors)[0][0]; + this.serverError = errors.error[0]; }, setOpsgenieAsDefault() { this.options = this.options.map(el => { @@ -224,41 +200,38 @@ export default { resetFormValues() { this.testAlert.json = null; this.targetUrl = this.selectedService.targetUrl; + this.active = this.selectedService.activated; }, dismissFeedback() { this.serverError = null; this.feedback = { ...this.feedback, feedbackMessage: null }; this.isFeedbackDismissed = false; }, - resetGenericKey() { - return service - .updateGenericKey({ endpoint: this.generic.formPath, params: { service: { token: '' } } }) + resetKey(key) { + const fn = key === 'prometheus' ? this.resetPrometheusKey() : this.resetGenericKey(); + + return fn .then(({ data: { token } }) => { - this.authorizationKey.generic = token; + this.authKey = token; this.setFeedback({ feedbackMessage: this.$options.i18n.authKeyRest, variant: 'success' }); }) .catch(() => { this.setFeedback({ feedbackMessage: this.$options.i18n.errorKeyMsg, variant: 'danger' }); }); }, + resetGenericKey() { + this.dismissFeedback(); + return service.updateGenericKey({ + endpoint: this.generic.formPath, + params: { service: { token: '' } }, + }); + }, resetPrometheusKey() { - return service - .updatePrometheusKey({ endpoint: this.prometheus.prometheusResetKeyPath }) - .then(({ data: { token } }) => { - this.authorizationKey.prometheus = token; - this.setFeedback({ feedbackMessage: this.$options.i18n.authKeyRest, variant: 'success' }); - }) - .catch(() => { - this.setFeedback({ feedbackMessage: this.$options.i18n.errorKeyMsg, variant: 'danger' }); - }); + return service.updatePrometheusKey({ endpoint: this.prometheus.prometheusResetKeyPath }); }, toggleService(value) { this.canSaveForm = true; - if (this.isPrometheus) { - this.activated.prometheus = value; - } else { - this.activated[this.selectedEndpoint] = value; - } + this.active = value; }, toggle(value) { return this.isPrometheus ? this.togglePrometheusActive(value) : this.toggleActivated(value); @@ -273,7 +246,7 @@ export default { : { service: { active: value } }, }) .then(() => { - this.activated[this.selectedEndpoint] = value; + this.active = value; this.toggleSuccess(value); if (!this.isOpsgenie && value) { @@ -316,7 +289,7 @@ export default { }, }) .then(() => { - this.activated.prometheus = value; + this.active = value; this.toggleSuccess(value); this.removeOpsGenieOption(); }) @@ -358,6 +331,7 @@ export default { }, validateTestAlert() { this.loading = true; + this.dismissFeedback(); this.validateJson(); return service .updateTestAlert({ @@ -382,7 +356,8 @@ export default { }); }, onSubmit() { - this.toggle(this.selectedService.active); + this.dismissFeedback(); + this.toggle(this.active); }, onReset() { this.testAlert.json = null; @@ -391,7 +366,7 @@ export default { if (this.canSaveForm) { this.canSaveForm = false; - this.activated[this.selectedEndpoint] = this[this.selectedEndpoint].activated; + this.active = this.selectedService.activated; } }, }, @@ -409,7 +384,7 @@ export default { variant="danger" category="primary" class="gl-display-block gl-mt-3" - @click="toggle(selectedService.active)" + @click="toggle(active)" > {{ __('Save anyway') }} </gl-button> @@ -457,7 +432,7 @@ export default { id="activated" :disabled-input="loading" :is-loading="loading" - :value="selectedService.active" + :value="active" @change="toggleService" /> </gl-form-group> @@ -472,7 +447,7 @@ export default { v-model="targetUrl" type="url" :placeholder="baseUrlPlaceholder" - :disabled="!selectedService.active" + :disabled="!active" /> <span class="gl-text-gray-200"> {{ $options.i18n.apiBaseUrlHelpText }} @@ -498,28 +473,18 @@ export default { label-for="authorization-key" label-class="label-bold" > - <gl-form-input-group - id="authorization-key" - class="gl-mb-2" - readonly - :value="selectedService.authKey" - > + <gl-form-input-group id="authorization-key" class="gl-mb-2" readonly :value="authKey"> <template #append> <clipboard-button - :text="selectedService.authKey || ''" + :text="authKey" :title="$options.i18n.copyToClipboard" class="gl-m-0!" /> </template> </gl-form-input-group> - <div class="gl-display-flex gl-justify-content-end"> - <gl-button - v-gl-modal.authKeyModal - :disabled="!selectedService.active" - class="gl-mt-3" - >{{ $options.i18n.resetKey }}</gl-button - > - </div> + <gl-button v-gl-modal.authKeyModal :disabled="!active" class="gl-mt-3">{{ + $options.i18n.resetKey + }}</gl-button> <gl-modal modal-id="authKeyModal" :title="$options.i18n.resetKey" @@ -539,7 +504,7 @@ export default { <gl-form-textarea id="alert-json" v-model.trim="testAlert.json" - :disabled="!selectedService.active" + :disabled="!active" :state="jsonIsValid" :placeholder="$options.i18n.alertJsonPlaceholder" rows="6" diff --git a/app/assets/javascripts/alerts_settings/constants.js b/app/assets/javascripts/alerts_settings/constants.js index d15e8619df4..fc669995875 100644 --- a/app/assets/javascripts/alerts_settings/constants.js +++ b/app/assets/javascripts/alerts_settings/constants.js @@ -35,7 +35,9 @@ export const i18n = { testAlertSuccess: s__( 'AlertSettings|Test alert sent successfully. If you have made other changes, please save them now.', ), - authKeyRest: s__('AlertSettings|Authorization key has been successfully reset'), + authKeyRest: s__( + 'AlertSettings|Authorization key has been successfully reset. Please save your changes now.', + ), }; export const serviceOptions = [ diff --git a/app/assets/javascripts/alerts_settings/index.js b/app/assets/javascripts/alerts_settings/index.js index a4c2bf6b18e..8d1d342d229 100644 --- a/app/assets/javascripts/alerts_settings/index.js +++ b/app/assets/javascripts/alerts_settings/index.js @@ -31,37 +31,37 @@ export default el => { const opsgenieMvcActivated = parseBoolean(opsgenieMvcEnabled); const opsgenieMvcIsAvailable = parseBoolean(opsgenieMvcAvailable); - const props = { - prometheus: { - activated: prometheusIsActivated, - prometheusUrl, - prometheusAuthorizationKey, - prometheusFormPath, - prometheusResetKeyPath, - prometheusApiUrl, - }, - generic: { - alertsSetupUrl, - alertsUsageUrl, - activated: genericActivated, - formPath, - initialAuthorizationKey: authorizationKey, - url, - }, - opsgenie: { - formPath: opsgenieMvcFormPath, - activated: opsgenieMvcActivated, - opsgenieMvcTargetUrl, - opsgenieMvcIsAvailable, - }, - }; - return new Vue({ el, + provide: { + prometheus: { + activated: prometheusIsActivated, + prometheusUrl, + authorizationKey: prometheusAuthorizationKey, + prometheusFormPath, + prometheusResetKeyPath, + prometheusApiUrl, + }, + generic: { + alertsSetupUrl, + alertsUsageUrl, + activated: genericActivated, + formPath, + authorizationKey, + url, + }, + opsgenie: { + formPath: opsgenieMvcFormPath, + activated: opsgenieMvcActivated, + opsgenieMvcTargetUrl, + opsgenieMvcIsAvailable, + }, + }, + components: { + AlertSettingsForm, + }, render(createElement) { - return createElement(AlertSettingsForm, { - props, - }); + return createElement('alert-settings-form'); }, }); }; diff --git a/app/assets/javascripts/badges/components/badge_settings.vue b/app/assets/javascripts/badges/components/badge_settings.vue index dc107cb44a1..8b22a51ae29 100644 --- a/app/assets/javascripts/badges/components/badge_settings.vue +++ b/app/assets/javascripts/badges/components/badge_settings.vue @@ -1,12 +1,12 @@ <script> import { mapState, mapActions } from 'vuex'; +import { GlSprintf } from '@gitlab/ui'; import createFlash from '~/flash'; import { s__ } from '~/locale'; import DeprecatedModal2 from '~/vue_shared/components/deprecated_modal_2.vue'; import Badge from './badge.vue'; import BadgeForm from './badge_form.vue'; import BadgeList from './badge_list.vue'; -import { GlSprintf } from '@gitlab/ui'; export default { name: 'BadgeSettings', diff --git a/app/assets/javascripts/create_cluster/gke_cluster/components/gke_project_id_dropdown.vue b/app/assets/javascripts/create_cluster/gke_cluster/components/gke_project_id_dropdown.vue index 88d04f9ed45..979628d683d 100644 --- a/app/assets/javascripts/create_cluster/gke_cluster/components/gke_project_id_dropdown.vue +++ b/app/assets/javascripts/create_cluster/gke_cluster/components/gke_project_id_dropdown.vue @@ -1,7 +1,7 @@ <script> import { mapState, mapGetters, mapActions } from 'vuex'; -import { s__ } from '~/locale'; import { GlSprintf, GlLink } from '@gitlab/ui'; +import { s__ } from '~/locale'; import gkeDropdownMixin from './gke_dropdown_mixin'; diff --git a/app/assets/javascripts/incidents/components/incidents_list.vue b/app/assets/javascripts/incidents/components/incidents_list.vue index 157e6f7b0ba..46852e4ddd9 100644 --- a/app/assets/javascripts/incidents/components/incidents_list.vue +++ b/app/assets/javascripts/incidents/components/incidents_list.vue @@ -14,6 +14,7 @@ import { GlTabs, GlTab, GlBadge, + GlEmptyState, } from '@gitlab/ui'; import { debounce } from 'lodash'; import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue'; @@ -80,6 +81,7 @@ export default { GlTab, PublishedCell: () => import('ee_component/incidents/components/published_cell.vue'), GlBadge, + GlEmptyState, }, directives: { GlTooltip: GlTooltipDirective, @@ -91,6 +93,7 @@ export default { 'incidentType', 'issuePath', 'publishedAvailable', + 'emptyListSvgPath', ], apollo: { incidents: { @@ -149,7 +152,7 @@ export default { }, computed: { showErrorMsg() { - return this.errored && !this.isErrorAlertDismissed && this.incidentsCount?.all === 0; + return this.errored && !this.isErrorAlertDismissed; }, loading() { return this.$apollo.queries.incidents.loading; @@ -202,6 +205,9 @@ export default { ] : this.$options.fields; }, + isEmpty() { + return !this.incidents.list?.length; + }, }, methods: { onInputChange: debounce(function debounceSearch(input) { @@ -273,6 +279,7 @@ export default { </gl-tabs> <gl-button + v-if="!isEmpty" class="gl-my-3 gl-mr-5 create-incident-button" data-testid="createIncidentBtn" data-qa-selector="create_incident_button" @@ -373,7 +380,17 @@ export default { </template> <template #empty> - {{ $options.i18n.noIncidents }} + <gl-empty-state + v-if="!errored" + :title="$options.i18n.emptyState.title" + :svg-path="emptyListSvgPath" + :description="$options.i18n.emptyState.description" + :primary-button-link="newIncidentPath" + :primary-button-text="$options.i18n.createIncidentBtnLabel" + /> + <span v-else> + {{ $options.i18n.noIncidents }} + </span> </template> </gl-table> diff --git a/app/assets/javascripts/incidents/constants.js b/app/assets/javascripts/incidents/constants.js index dc90f30991c..02d8172533d 100644 --- a/app/assets/javascripts/incidents/constants.js +++ b/app/assets/javascripts/incidents/constants.js @@ -7,6 +7,12 @@ export const I18N = { createIncidentBtnLabel: s__('IncidentManagement|Create incident'), unPublished: s__('IncidentManagement|Unpublished'), searchPlaceholder: __('Search results…'), + emptyState: { + title: s__('IncidentManagement|Display your incidents in a dedicated view'), + description: s__( + 'IncidentManagement|All alerts promoted to incidents will automatically be displayed within the list. You can also create a new incident using the button below.', + ), + }, }; export const INCIDENT_STATUS_TABS = [ diff --git a/app/assets/javascripts/incidents/list.js b/app/assets/javascripts/incidents/list.js index ac49ad63ad1..7505d07449c 100644 --- a/app/assets/javascripts/incidents/list.js +++ b/app/assets/javascripts/incidents/list.js @@ -15,6 +15,7 @@ export default () => { incidentType, issuePath, publishedAvailable, + emptyListSvgPath, } = domEl.dataset; const apolloProvider = new VueApollo({ @@ -30,6 +31,7 @@ export default () => { newIssuePath, issuePath, publishedAvailable, + emptyListSvgPath, }, apolloProvider, components: { diff --git a/app/assets/javascripts/issuables_list/components/issuables_list_app.vue b/app/assets/javascripts/issuables_list/components/issuables_list_app.vue index 4e1ee74f4f3..dd1e2e3a153 100644 --- a/app/assets/javascripts/issuables_list/components/issuables_list_app.vue +++ b/app/assets/javascripts/issuables_list/components/issuables_list_app.vue @@ -1,6 +1,11 @@ <script> import { toNumber, omit } from 'lodash'; -import { GlEmptyState, GlPagination, GlSkeletonLoading } from '@gitlab/ui'; +import { + GlEmptyState, + GlPagination, + GlSkeletonLoading, + GlSafeHtmlDirective as SafeHtml, +} from '@gitlab/ui'; import flash from '~/flash'; import axios from '~/lib/utils/axios_utils'; import { @@ -23,9 +28,13 @@ import { } from '../constants'; import { setUrlParams } from '~/lib/utils/url_utility'; import issueableEventHub from '../eventhub'; +import { emptyStateHelper } from '../service_desk_helper'; export default { LOADING_LIST_ITEMS_LENGTH, + directives: { + SafeHtml, + }, components: { GlEmptyState, GlPagination, @@ -39,15 +48,9 @@ export default { required: false, default: false, }, - createIssuePath: { - type: String, - required: false, - default: '', - }, - emptySvgPath: { - type: String, - required: false, - default: '', + emptyStateMeta: { + type: Object, + required: true, }, endpoint: { type: String, @@ -94,28 +97,40 @@ export default { emptyState() { if (this.issuables.length) { return {}; // Empty state shouldn't be shown here - } else if (this.hasFilters) { + } + + if (this.isServiceDesk) { + return emptyStateHelper(this.emptyStateMeta); + } + + if (this.hasFilters) { return { title: __('Sorry, your filter produced no results'), + svgPath: this.emptyStateMeta.svgPath, description: __('To widen your search, change or remove filters above'), - primaryLink: this.createIssuePath, + primaryLink: this.emptyStateMeta.createIssuePath, primaryText: __('New issue'), }; - } else if (this.filters.state === 'opened') { + } + + if (this.filters.state === 'opened') { return { title: __('There are no open issues'), + svgPath: this.emptyStateMeta.svgPath, description: __('To keep this project going, create a new issue'), - primaryLink: this.createIssuePath, + primaryLink: this.emptyStateMeta.createIssuePath, primaryText: __('New issue'), }; } else if (this.filters.state === 'closed') { return { title: __('There are no closed issues'), + svgPath: this.emptyStateMeta.svgPath, }; } return { title: __('There are no issues to show'), + svgPath: this.emptyStateMeta.svgPath, description: __( 'The Issue Tracker is the place to add things that need to be improved or solved in a project. You can register or sign in to create issues for this project.', ), @@ -157,6 +172,9 @@ export default { nextPage: this.paginationNext, }; }, + isServiceDesk() { + return this.type === 'service_desk'; + }, isJira() { return this.type === 'jira'; }, @@ -394,10 +412,13 @@ export default { <gl-empty-state v-else :title="emptyState.title" - :description="emptyState.description" - :svg-path="emptySvgPath" + :svg-path="emptyState.svgPath" :primary-button-link="emptyState.primaryLink" :primary-button-text="emptyState.primaryText" - /> + > + <template #description> + <div v-safe-html="emptyState.description"></div> + </template> + </gl-empty-state> </div> </template> diff --git a/app/assets/javascripts/issuables_list/index.js b/app/assets/javascripts/issuables_list/index.js index 40252c10d5f..fa23d6c0eed 100644 --- a/app/assets/javascripts/issuables_list/index.js +++ b/app/assets/javascripts/issuables_list/index.js @@ -1,7 +1,7 @@ import Vue from 'vue'; import VueApollo from 'vue-apollo'; import createDefaultClient from '~/lib/graphql'; -import { parseBoolean } from '~/lib/utils/common_utils'; +import { parseBoolean, convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; import IssuableListRootApp from './components/issuable_list_root_app.vue'; import IssuablesListApp from './components/issuables_list_app.vue'; @@ -41,7 +41,7 @@ function mountIssuablesListApp() { } document.querySelectorAll('.js-issuables-list').forEach(el => { - const { canBulkEdit, ...data } = el.dataset; + const { canBulkEdit, emptyStateMeta = {}, ...data } = el.dataset; return new Vue({ el, @@ -49,6 +49,10 @@ function mountIssuablesListApp() { return createElement(IssuablesListApp, { props: { ...data, + emptyStateMeta: + Object.keys(emptyStateMeta).length !== 0 + ? convertObjectPropsToCamelCase(JSON.parse(emptyStateMeta)) + : {}, canBulkEdit: Boolean(canBulkEdit), }, }); diff --git a/app/assets/javascripts/issuables_list/service_desk_helper.js b/app/assets/javascripts/issuables_list/service_desk_helper.js new file mode 100644 index 00000000000..4b4a38c2205 --- /dev/null +++ b/app/assets/javascripts/issuables_list/service_desk_helper.js @@ -0,0 +1,55 @@ +import { __ } from '~/locale'; + +/** + * Returns the attributes used for gl-empty-state in the Service Desk issues list. + */ +// eslint-disable-next-line import/prefer-default-export +export function emptyStateHelper(emptyStateMeta) { + const { isServiceDeskSupported, svgPath, serviceDeskHelpPage } = emptyStateMeta; + + if (isServiceDeskSupported) { + const title = __( + 'Use Service Desk to connect with your users (e.g. to offer customer support) through email right inside GitLab', + ); + const commonMessage = __( + 'Those emails automatically become issues (with the comments becoming the email conversation) listed here.', + ); + const commonDescription = ` + <span>${commonMessage}</span> + <a href="${serviceDeskHelpPage}">${__('Read more')}</a>`; + + if (emptyStateMeta.canEditProjectSettings && emptyStateMeta.isServiceDeskEnabled) { + return { + title, + svgPath, + description: `<p>${__('Have your users email')} <code>${ + emptyStateMeta.serviceDeskAddress + }</code></p> ${commonDescription}`, + }; + } + + if (emptyStateMeta.canEditProjectSettings && !emptyStateMeta.isServiceDeskEnabled) { + return { + title, + svgPath, + description: commonDescription, + primaryLink: emptyStateMeta.editProjectPage, + primaryText: __('Turn on Service Desk'), + }; + } + + return { + title, + svgPath, + description: commonDescription, + }; + } + + return { + title: __('Service Desk is enabled but not yet active'), + svgPath, + description: __('You must set up incoming email before it becomes active.'), + primaryLink: emptyStateMeta.incomingEmailHelpPage, + primaryText: __('More information'), + }; +} diff --git a/app/assets/javascripts/pages/projects/issues/service_desk/index.js b/app/assets/javascripts/pages/projects/issues/service_desk/index.js index 56054f5fc80..9304d9b6832 100644 --- a/app/assets/javascripts/pages/projects/issues/service_desk/index.js +++ b/app/assets/javascripts/pages/projects/issues/service_desk/index.js @@ -1,11 +1,17 @@ import FilteredSearchServiceDesk from './filtered_search'; +import initIssuablesList from '~/issuables_list'; document.addEventListener('DOMContentLoaded', () => { const supportBotData = JSON.parse( document.querySelector('.js-service-desk-issues').dataset.supportBot, ); - const filteredSearchManager = new FilteredSearchServiceDesk(supportBotData); + if (document.querySelector('.filtered-search')) { + const filteredSearchManager = new FilteredSearchServiceDesk(supportBotData); + filteredSearchManager.setup(); + } - filteredSearchManager.setup(); + if (gon.features?.vueIssuablesList) { + initIssuablesList(); + } }); diff --git a/app/assets/javascripts/pages/projects/labels/components/promote_label_modal.vue b/app/assets/javascripts/pages/projects/labels/components/promote_label_modal.vue index 385da528d89..97ca92420bd 100644 --- a/app/assets/javascripts/pages/projects/labels/components/promote_label_modal.vue +++ b/app/assets/javascripts/pages/projects/labels/components/promote_label_modal.vue @@ -1,11 +1,11 @@ <script> +import { GlSprintf } from '@gitlab/ui'; import axios from '~/lib/utils/axios_utils'; import createFlash from '~/flash'; import DeprecatedModal2 from '~/vue_shared/components/deprecated_modal_2.vue'; import { s__, sprintf } from '~/locale'; import { visitUrl } from '~/lib/utils/url_utility'; import eventHub from '../event_hub'; -import { GlSprintf } from '@gitlab/ui'; export default { components: { diff --git a/app/assets/javascripts/sidebar/components/confidential/edit_form.vue b/app/assets/javascripts/sidebar/components/confidential/edit_form.vue index 0a542f03fcb..17e44cf0e1d 100644 --- a/app/assets/javascripts/sidebar/components/confidential/edit_form.vue +++ b/app/assets/javascripts/sidebar/components/confidential/edit_form.vue @@ -1,7 +1,7 @@ <script> +import { GlSprintf } from '@gitlab/ui'; import editFormButtons from './edit_form_buttons.vue'; import { __ } from '../../../locale'; -import { GlSprintf } from '@gitlab/ui'; export default { components: { diff --git a/app/assets/javascripts/sidebar/components/lock/edit_form.vue b/app/assets/javascripts/sidebar/components/lock/edit_form.vue index 49bf28147be..65b51169420 100644 --- a/app/assets/javascripts/sidebar/components/lock/edit_form.vue +++ b/app/assets/javascripts/sidebar/components/lock/edit_form.vue @@ -1,6 +1,6 @@ <script> -import editFormButtons from './edit_form_buttons.vue'; import { GlSprintf } from '@gitlab/ui'; +import editFormButtons from './edit_form_buttons.vue'; export default { components: { diff --git a/app/assets/javascripts/vue_merge_request_widget/components/source_branch_removal_status.vue b/app/assets/javascripts/vue_merge_request_widget/components/source_branch_removal_status.vue index 439baa5b6c5..dab0540f44e 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/source_branch_removal_status.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/source_branch_removal_status.vue @@ -1,7 +1,7 @@ <script> +import { GlSprintf } from '@gitlab/ui'; import tooltip from '../../vue_shared/directives/tooltip'; import { __ } from '../../locale'; -import { GlSprintf } from '@gitlab/ui'; export default { i18n: { diff --git a/app/assets/stylesheets/pages/alert_management/details.scss b/app/assets/stylesheets/pages/alert_management/details.scss index a78c378d5ec..0f889935583 100644 --- a/app/assets/stylesheets/pages/alert_management/details.scss +++ b/app/assets/stylesheets/pages/alert_management/details.scss @@ -35,7 +35,7 @@ } @include media-breakpoint-down(xs) { - .alert-details-issue-button { + .alert-details-incident-button { width: 100%; } } diff --git a/app/helpers/projects/alert_management_helper.rb b/app/helpers/projects/alert_management_helper.rb index d6e8e738a1c..c2f0b8854e1 100644 --- a/app/helpers/projects/alert_management_helper.rb +++ b/app/helpers/projects/alert_management_helper.rb @@ -5,7 +5,8 @@ module Projects::AlertManagementHelper { 'project-path' => project.full_path, 'enable-alert-management-path' => project_settings_operations_path(project, anchor: 'js-alert-management-settings'), - 'populating-alerts-help-url' => help_page_url('user/project/operations/alert_management.html', anchor: 'enable-alert-management'), + 'alerts-help-url' => help_page_url('operations/incident_management/index.md'), + 'populating-alerts-help-url' => help_page_url('operations/incident_management/index.md', anchor: 'enable-alert-management'), 'empty-alert-svg-path' => image_path('illustrations/alert-management-empty-state.svg'), 'user-can-enable-alert-management' => can?(current_user, :admin_operations, project).to_s, 'alert-management-enabled' => alert_management_enabled?(project).to_s diff --git a/app/helpers/projects/incidents_helper.rb b/app/helpers/projects/incidents_helper.rb index 8cdd65fecb5..e96f0f5a384 100644 --- a/app/helpers/projects/incidents_helper.rb +++ b/app/helpers/projects/incidents_helper.rb @@ -7,7 +7,8 @@ module Projects::IncidentsHelper 'new-issue-path' => new_project_issue_path(project), 'incident-template-name' => 'incident', 'incident-type' => 'incident', - 'issue-path' => project_issues_path(project) + 'issue-path' => project_issues_path(project), + 'empty-list-svg-path' => image_path('illustrations/incident-empty-state.svg') } end end diff --git a/app/helpers/projects/issues/service_desk_helper.rb b/app/helpers/projects/issues/service_desk_helper.rb new file mode 100644 index 00000000000..0f87e5ed837 --- /dev/null +++ b/app/helpers/projects/issues/service_desk_helper.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Projects::Issues::ServiceDeskHelper + def service_desk_meta(project) + empty_state_meta = { + is_service_desk_supported: Gitlab::ServiceDesk.supported?, + is_service_desk_enabled: project.service_desk_enabled?, + can_edit_project_settings: can?(current_user, :admin_project, project) + } + + if Gitlab::ServiceDesk.supported? + empty_state_meta.merge(supported_meta(project)) + else + empty_state_meta.merge(unsupported_meta(project)) + end + end + + private + + def supported_meta(project) + { + service_desk_address: project.service_desk_address, + service_desk_help_page: help_page_path('user/project/service_desk'), + edit_project_page: edit_project_path(project), + svg_path: image_path('illustrations/service_desk_empty.svg') + } + end + + def unsupported_meta(project) + { + incoming_email_help_page: help_page_path('administration/incoming_email', anchor: 'set-it-up'), + svg_path: image_path('illustrations/service-desk-setup.svg') + } + end +end diff --git a/app/models/ci/group.rb b/app/models/ci/group.rb index 779c6c0396f..f0c035635b9 100644 --- a/app/models/ci/group.rb +++ b/app/models/ci/group.rb @@ -24,15 +24,9 @@ module Ci def status strong_memoize(:status) do - if ::Gitlab::Ci::Features.composite_status?(project) - Gitlab::Ci::Status::Composite - .new(@jobs) - .status - else - CommitStatus - .where(id: @jobs) - .legacy_status - end + Gitlab::Ci::Status::Composite + .new(@jobs) + .status end end diff --git a/app/models/ci/legacy_stage.rb b/app/models/ci/legacy_stage.rb index 250306e2be4..df4368eccd5 100644 --- a/app/models/ci/legacy_stage.rb +++ b/app/models/ci/legacy_stage.rb @@ -32,7 +32,7 @@ module Ci end def status - @status ||= statuses.latest.slow_composite_status(project: project) + @status ||= statuses.latest.composite_status end def detailed_status(current_user) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index b2d4236ca6b..7762328d274 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -418,24 +418,6 @@ module Ci false end - def legacy_stages_using_sql - # TODO, this needs refactoring, see gitlab-foss#26481. - stages_query = statuses - .group('stage').select(:stage).order('max(stage_idx)') - - status_sql = statuses.latest.where('stage=sg.stage').legacy_status_sql - - warnings_sql = statuses.latest.select('COUNT(*)') - .where('stage=sg.stage').failed_but_allowed.to_sql - - stages_with_statuses = CommitStatus.from(stages_query, :sg) - .pluck('sg.stage', Arel.sql(status_sql), Arel.sql("(#{warnings_sql})")) - - stages_with_statuses.map do |stage| - Ci::LegacyStage.new(self, Hash[%i[name status warnings].zip(stage)]) - end - end - def legacy_stages_using_composite_status stages = latest_statuses_ordered_by_stage.group_by(&:stage) @@ -456,11 +438,7 @@ module Ci # TODO: Remove usage of this method in templates def legacy_stages - if ::Gitlab::Ci::Features.composite_status?(project) - legacy_stages_using_composite_status - else - legacy_stages_using_sql - end + legacy_stages_using_composite_status end def valid_commit_sha diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index cc9e32aa4c0..cc6bd1870b9 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -138,7 +138,7 @@ module Ci end def latest_stage_status - statuses.latest.slow_composite_status(project: project) || 'skipped' + statuses.latest.composite_status || 'skipped' end end end diff --git a/app/models/concerns/ci/has_status.rb b/app/models/concerns/ci/has_status.rb index c52807ec501..1cc2e8a51e3 100644 --- a/app/models/concerns/ci/has_status.rb +++ b/app/models/concerns/ci/has_status.rb @@ -20,60 +20,10 @@ module Ci UnknownStatusError = Class.new(StandardError) class_methods do - def legacy_status_sql - scope_relevant = respond_to?(:exclude_ignored) ? exclude_ignored : all - scope_warnings = respond_to?(:failed_but_allowed) ? failed_but_allowed : none - - builds = scope_relevant.select('count(*)').to_sql - created = scope_relevant.created.select('count(*)').to_sql - success = scope_relevant.success.select('count(*)').to_sql - manual = scope_relevant.manual.select('count(*)').to_sql - scheduled = scope_relevant.scheduled.select('count(*)').to_sql - preparing = scope_relevant.preparing.select('count(*)').to_sql - waiting_for_resource = scope_relevant.waiting_for_resource.select('count(*)').to_sql - pending = scope_relevant.pending.select('count(*)').to_sql - running = scope_relevant.running.select('count(*)').to_sql - skipped = scope_relevant.skipped.select('count(*)').to_sql - canceled = scope_relevant.canceled.select('count(*)').to_sql - warnings = scope_warnings.select('count(*) > 0').to_sql.presence || 'false' - - Arel.sql( - "(CASE - WHEN (#{builds})=(#{skipped}) AND (#{warnings}) THEN 'success' - WHEN (#{builds})=(#{skipped}) THEN 'skipped' - WHEN (#{builds})=(#{success}) THEN 'success' - WHEN (#{builds})=(#{created}) THEN 'created' - WHEN (#{builds})=(#{preparing}) THEN 'preparing' - WHEN (#{builds})=(#{success})+(#{skipped}) THEN 'success' - WHEN (#{builds})=(#{success})+(#{skipped})+(#{canceled}) THEN 'canceled' - WHEN (#{builds})=(#{created})+(#{skipped})+(#{pending}) THEN 'pending' - WHEN (#{running})+(#{pending})>0 THEN 'running' - WHEN (#{waiting_for_resource})>0 THEN 'waiting_for_resource' - WHEN (#{manual})>0 THEN 'manual' - WHEN (#{scheduled})>0 THEN 'scheduled' - WHEN (#{preparing})>0 THEN 'preparing' - WHEN (#{created})>0 THEN 'running' - ELSE 'failed' - END)" - ) - end - - def legacy_status - all.pluck(legacy_status_sql).first - end - - # This method should not be used. - # This method performs expensive calculation of status: - # 1. By plucking all related objects, - # 2. Or executes expensive SQL query - def slow_composite_status(project:) - if ::Gitlab::Ci::Features.composite_status?(project) - Gitlab::Ci::Status::Composite - .new(all, with_allow_failure: columns_hash.key?('allow_failure')) - .status - else - legacy_status - end + def composite_status + Gitlab::Ci::Status::Composite + .new(all, with_allow_failure: columns_hash.key?('allow_failure')) + .status end def started_at diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb index ca64907f04b..3fe8c0fe328 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/app/models/concerns/relative_positioning.rb @@ -3,11 +3,15 @@ # This module makes it possible to handle items as a list, where the order of items can be easily altered # Requirements: # -# - Only works for ActiveRecord models -# - relative_position integer field must present on the model -# - This module uses GROUP BY: the model should have a parent relation, example: project -> issues, project is the parent relation (issues table has a parent_id column) +# The model must have the following named columns: +# - id: integer +# - relative_position: integer # -# Setup like this in the body of your class: +# The model must support a concept of siblings via a child->parent relationship, +# to enable rebalancing and `GROUP BY` in queries. +# - example: project -> issues, project is the parent relation (issues table has a parent_id column) +# +# Two class methods must be defined when including this concern: # # include RelativePositioning # @@ -24,66 +28,162 @@ module RelativePositioning extend ActiveSupport::Concern - MIN_POSITION = 0 - START_POSITION = Gitlab::Database::MAX_INT_VALUE / 2 - MAX_POSITION = Gitlab::Database::MAX_INT_VALUE - IDEAL_DISTANCE = 500 + STEPS = 10 + IDEAL_DISTANCE = 2**(STEPS - 1) + 1 - class_methods do - def move_nulls_to_end(objects) - objects = objects.reject(&:relative_position) - return if objects.empty? + MIN_POSITION = Gitlab::Database::MIN_INT_VALUE + START_POSITION = 0 + MAX_POSITION = Gitlab::Database::MAX_INT_VALUE - self.transaction do - max_relative_position = objects.first.max_relative_position + MAX_GAP = IDEAL_DISTANCE * 2 + MIN_GAP = 2 - objects.each do |object| - relative_position = position_between(max_relative_position || START_POSITION, MAX_POSITION) - object.update_column(:relative_position, relative_position) + NoSpaceLeft = Class.new(StandardError) - max_relative_position = relative_position - end - end + class_methods do + def move_nulls_to_end(objects) + move_nulls(objects, at_end: true) end def move_nulls_to_start(objects) - objects = objects.reject(&:relative_position) - return if objects.empty? - - self.transaction do - min_relative_position = objects.first.min_relative_position - - objects.reverse_each do |object| - relative_position = position_between(MIN_POSITION, min_relative_position || START_POSITION) - object.update_column(:relative_position, relative_position) - - min_relative_position = relative_position - end - end + move_nulls(objects, at_end: false) end # This method takes two integer values (positions) and # calculates the position between them. The range is huge as - # the maximum integer value is 2147483647. We are incrementing position by IDEAL_DISTANCE * 2 every time - # when we have enough space. If distance is less than IDEAL_DISTANCE, we are calculating an average number. + # the maximum integer value is 2147483647. + # + # We avoid open ranges by clamping the range to [MIN_POSITION, MAX_POSITION]. + # + # Then we handle one of three cases: + # - If the gap is too small, we raise NoSpaceLeft + # - If the gap is larger than MAX_GAP, we place the new position at most + # IDEAL_DISTANCE from the edge of the gap. + # - otherwise we place the new position at the midpoint. + # + # The new position will always satisfy: pos_before <= midpoint <= pos_after + # + # As a precondition, the gap between pos_before and pos_after MUST be >= 2. + # If the gap is too small, NoSpaceLeft is raised. + # + # This class method should only be called by instance methods of this module, which + # include handling for minimum gap size. + # + # @raises NoSpaceLeft + # @api private def position_between(pos_before, pos_after) pos_before ||= MIN_POSITION pos_after ||= MAX_POSITION pos_before, pos_after = [pos_before, pos_after].sort - halfway = (pos_after + pos_before) / 2 - distance_to_halfway = pos_after - halfway + gap_width = pos_after - pos_before + midpoint = [pos_after - 1, pos_before + (gap_width / 2)].min - if distance_to_halfway < IDEAL_DISTANCE - halfway - else + if gap_width < MIN_GAP + raise NoSpaceLeft + elsif gap_width > MAX_GAP if pos_before == MIN_POSITION pos_after - IDEAL_DISTANCE elsif pos_after == MAX_POSITION pos_before + IDEAL_DISTANCE else - halfway + midpoint + end + else + midpoint + end + end + + private + + # @api private + def gap_size(object, gaps:, at_end:, starting_from:) + total_width = IDEAL_DISTANCE * gaps + size = if at_end && starting_from + total_width >= MAX_POSITION + (MAX_POSITION - starting_from) / gaps + elsif !at_end && starting_from - total_width <= MIN_POSITION + (starting_from - MIN_POSITION) / gaps + else + IDEAL_DISTANCE + end + + # Shift max elements leftwards if there isn't enough space + return [size, starting_from] if size >= MIN_GAP + + order = at_end ? :desc : :asc + terminus = object + .send(:relative_siblings) # rubocop:disable GitlabSecurity/PublicSend + .where('relative_position IS NOT NULL') + .order(relative_position: order) + .first + + if at_end + terminus.move_sequence_before(true) + max_relative_position = terminus.reset.relative_position + [[(MAX_POSITION - max_relative_position) / gaps, IDEAL_DISTANCE].min, max_relative_position] + else + terminus.move_sequence_after(true) + min_relative_position = terminus.reset.relative_position + [[(min_relative_position - MIN_POSITION) / gaps, IDEAL_DISTANCE].min, min_relative_position] + end + end + + # @api private + # @param [Array<RelativePositioning>] objects The objects to give positions to. The relative + # order will be preserved (i.e. when this method returns, + # objects.first.relative_position < objects.last.relative_position) + # @param [Boolean] at_end: The placement. + # If `true`, then all objects with `null` positions are placed _after_ + # all siblings with positions. If `false`, all objects with `null` + # positions are placed _before_ all siblings with positions. + def move_nulls(objects, at_end:) + objects = objects.reject(&:relative_position) + return if objects.empty? + + representative = objects.first + number_of_gaps = objects.size + 1 # 1 at left, one between each, and one at right + position = if at_end + representative.max_relative_position + else + representative.min_relative_position + end + + position ||= START_POSITION # If there are no positioned siblings, start from START_POSITION + + gap, position = gap_size(representative, gaps: number_of_gaps, at_end: at_end, starting_from: position) + + # Raise if we could not make enough space + raise NoSpaceLeft if gap < MIN_GAP + + indexed = objects.each_with_index.to_a + starting_from = at_end ? position : position - (gap * number_of_gaps) + + # Some classes are polymorphic, and not all siblings are in the same table. + by_model = indexed.group_by { |pair| pair.first.class } + + by_model.each do |model, pairs| + model.transaction do + pairs.each_slice(100) do |batch| + # These are known to be integers, one from the DB, and the other + # calculated by us, and thus safe to interpolate + values = batch.map do |obj, i| + pos = starting_from + gap * (i + 1) + obj.relative_position = pos + "(#{obj.id}, #{pos})" + end.join(', ') + + model.connection.exec_query(<<~SQL, "UPDATE #{model.table_name} positions") + WITH cte(cte_id, new_pos) AS ( + SELECT * + FROM (VALUES #{values}) as t (id, pos) + ) + UPDATE #{model.table_name} + SET relative_position = cte.new_pos + FROM cte + WHERE cte_id = id + SQL + end end end end @@ -97,11 +197,12 @@ module RelativePositioning calculate_relative_position('MAX', &block) end - def prev_relative_position + def prev_relative_position(ignoring: nil) prev_pos = nil if self.relative_position prev_pos = max_relative_position do |relation| + relation = relation.id_not_in(ignoring.id) if ignoring.present? relation.where('relative_position < ?', self.relative_position) end end @@ -109,11 +210,12 @@ module RelativePositioning prev_pos end - def next_relative_position + def next_relative_position(ignoring: nil) next_pos = nil if self.relative_position next_pos = min_relative_position do |relation| + relation = relation.id_not_in(ignoring.id) if ignoring.present? relation.where('relative_position > ?', self.relative_position) end end @@ -125,24 +227,44 @@ module RelativePositioning return move_after(before) unless after return move_before(after) unless before - # If there is no place to insert an item we need to create one by moving the item - # before this and all preceding items until there is a gap before, after = after, before if after.relative_position < before.relative_position - if (after.relative_position - before.relative_position) < 2 - after.move_sequence_before - before.reset + + pos_left = before.relative_position + pos_right = after.relative_position + + if pos_right - pos_left < MIN_GAP + # Not enough room! Make space by shifting all previous elements to the left + # if there is enough space, else to the right + gap = after.send(:find_next_gap_before) # rubocop:disable GitlabSecurity/PublicSend + + if gap.present? + after.move_sequence_before(next_gap: gap) + pos_left -= optimum_delta_for_gap(gap) + else + before.move_sequence_after + pos_right = after.reset.relative_position + end end - self.relative_position = self.class.position_between(before.relative_position, after.relative_position) + new_position = self.class.position_between(pos_left, pos_right) + + self.relative_position = new_position end def move_after(before = self) pos_before = before.relative_position - pos_after = before.next_relative_position + pos_after = before.next_relative_position(ignoring: self) + + if pos_before == MAX_POSITION || gap_too_small?(pos_after, pos_before) + gap = before.send(:find_next_gap_after) # rubocop:disable GitlabSecurity/PublicSend - if pos_after && (pos_after - pos_before) < 2 - before.move_sequence_after - pos_after = before.next_relative_position + if gap.nil? + before.move_sequence_before(true) + pos_before = before.reset.relative_position + else + before.move_sequence_after(next_gap: gap) + pos_after += optimum_delta_for_gap(gap) + end end self.relative_position = self.class.position_between(pos_before, pos_after) @@ -150,80 +272,186 @@ module RelativePositioning def move_before(after = self) pos_after = after.relative_position - pos_before = after.prev_relative_position + pos_before = after.prev_relative_position(ignoring: self) - if pos_before && (pos_after - pos_before) < 2 - after.move_sequence_before - pos_before = after.prev_relative_position + if pos_after == MIN_POSITION || gap_too_small?(pos_before, pos_after) + gap = after.send(:find_next_gap_before) # rubocop:disable GitlabSecurity/PublicSend + + if gap.nil? + after.move_sequence_after(true) + pos_after = after.reset.relative_position + else + after.move_sequence_before(next_gap: gap) + pos_before -= optimum_delta_for_gap(gap) + end end self.relative_position = self.class.position_between(pos_before, pos_after) end def move_to_end - self.relative_position = self.class.position_between(max_relative_position || START_POSITION, MAX_POSITION) + max_pos = max_relative_position + + if max_pos.nil? + self.relative_position = START_POSITION + elsif gap_too_small?(max_pos, MAX_POSITION) + max = relative_siblings.order(Gitlab::Database.nulls_last_order('relative_position', 'DESC')).first + max.move_sequence_before(true) + max.reset + self.relative_position = self.class.position_between(max.relative_position, MAX_POSITION) + else + self.relative_position = self.class.position_between(max_pos, MAX_POSITION) + end end def move_to_start - self.relative_position = self.class.position_between(min_relative_position || START_POSITION, MIN_POSITION) + min_pos = min_relative_position + + if min_pos.nil? + self.relative_position = START_POSITION + elsif gap_too_small?(min_pos, MIN_POSITION) + min = relative_siblings.order(Gitlab::Database.nulls_last_order('relative_position', 'ASC')).first + min.move_sequence_after(true) + min.reset + self.relative_position = self.class.position_between(MIN_POSITION, min.relative_position) + else + self.relative_position = self.class.position_between(MIN_POSITION, min_pos) + end end # Moves the sequence before the current item to the middle of the next gap - # For example, we have 5 11 12 13 14 15 and the current item is 15 - # This moves the sequence 11 12 13 14 to 8 9 10 11 - def move_sequence_before - next_gap = find_next_gap_before + # For example, we have + # + # 5 . . . . . 11 12 13 14 [15] 16 . 17 + # ----------- + # + # This moves the sequence [11 12 13 14] to [8 9 10 11], so we have: + # + # 5 . . 8 9 10 11 . . . [15] 16 . 17 + # --------- + # + # Creating a gap to the left of the current item. We can understand this as + # dividing the 5 spaces between 5 and 11 into two smaller gaps of 2 and 3. + # + # If `include_self` is true, the current item will also be moved, creating a + # gap to the right of the current item: + # + # 5 . . 8 9 10 11 [14] . . . 16 . 17 + # -------------- + # + # As an optimization, the gap can be precalculated and passed to this method. + # + # @api private + # @raises NoSpaceLeft if the sequence cannot be moved + def move_sequence_before(include_self = false, next_gap: find_next_gap_before) + raise NoSpaceLeft unless next_gap.present? + delta = optimum_delta_for_gap(next_gap) - move_sequence(next_gap[:start], relative_position, -delta) + move_sequence(next_gap[:start], relative_position, -delta, include_self) end # Moves the sequence after the current item to the middle of the next gap - # For example, we have 11 12 13 14 15 21 and the current item is 11 - # This moves the sequence 12 13 14 15 to 15 16 17 18 - def move_sequence_after - next_gap = find_next_gap_after + # For example, we have: + # + # 8 . 10 [11] 12 13 14 15 . . . . . 21 + # ----------- + # + # This moves the sequence [12 13 14 15] to [15 16 17 18], so we have: + # + # 8 . 10 [11] . . . 15 16 17 18 . . 21 + # ----------- + # + # Creating a gap to the right of the current item. We can understand this as + # dividing the 5 spaces between 15 and 21 into two smaller gaps of 3 and 2. + # + # If `include_self` is true, the current item will also be moved, creating a + # gap to the left of the current item: + # + # 8 . 10 . . . [14] 15 16 17 18 . . 21 + # ---------------- + # + # As an optimization, the gap can be precalculated and passed to this method. + # + # @api private + # @raises NoSpaceLeft if the sequence cannot be moved + def move_sequence_after(include_self = false, next_gap: find_next_gap_after) + raise NoSpaceLeft unless next_gap.present? + delta = optimum_delta_for_gap(next_gap) - move_sequence(relative_position, next_gap[:start], delta) + move_sequence(relative_position, next_gap[:start], delta, include_self) end private - # Supposing that we have a sequence of items: 1 5 11 12 13 and the current item is 13 - # This would return: `{ start: 11, end: 5 }` + def gap_too_small?(pos_a, pos_b) + return false unless pos_a && pos_b + + (pos_a - pos_b).abs < MIN_GAP + end + + # Find the first suitable gap to the left of the current position. + # + # Satisfies the relations: + # - gap[:start] <= relative_position + # - abs(gap[:start] - gap[:end]) >= MIN_GAP + # - MIN_POSITION <= gap[:start] <= MAX_POSITION + # - MIN_POSITION <= gap[:end] <= MAX_POSITION + # + # Supposing that the current item is 13, and we have a sequence of items: + # + # 1 . . . 5 . . . . 11 12 [13] 14 . . 17 + # ^---------^ + # + # Then we return: `{ start: 11, end: 5 }` + # + # Here start refers to the end of the gap closest to the current item. def find_next_gap_before items_with_next_pos = scoped_items .select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position DESC) AS next_pos') .where('relative_position <= ?', relative_position) .order(relative_position: :desc) - find_next_gap(items_with_next_pos).tap do |gap| - gap[:end] ||= MIN_POSITION - end + find_next_gap(items_with_next_pos, MIN_POSITION) end - # Supposing that we have a sequence of items: 13 14 15 20 24 and the current item is 13 - # This would return: `{ start: 15, end: 20 }` + # Find the first suitable gap to the right of the current position. + # + # Satisfies the relations: + # - gap[:start] >= relative_position + # - abs(gap[:start] - gap[:end]) >= MIN_GAP + # - MIN_POSITION <= gap[:start] <= MAX_POSITION + # - MIN_POSITION <= gap[:end] <= MAX_POSITION + # + # Supposing the current item is 13, and that we have a sequence of items: + # + # 9 . . . [13] 14 15 . . . . 20 . . . 24 + # ^---------^ + # + # Then we return: `{ start: 15, end: 20 }` + # + # Here start refers to the end of the gap closest to the current item. def find_next_gap_after items_with_next_pos = scoped_items .select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position ASC) AS next_pos') .where('relative_position >= ?', relative_position) .order(:relative_position) - find_next_gap(items_with_next_pos).tap do |gap| - gap[:end] ||= MAX_POSITION - end + find_next_gap(items_with_next_pos, MAX_POSITION) end - def find_next_gap(items_with_next_pos) - gap = self.class.from(items_with_next_pos, :items_with_next_pos) - .where('ABS(pos - next_pos) > 1 OR next_pos IS NULL') - .limit(1) - .pluck(:pos, :next_pos) - .first + def find_next_gap(items_with_next_pos, end_is_nil) + gap = self.class + .from(items_with_next_pos, :items) + .where('next_pos IS NULL OR ABS(pos::bigint - next_pos::bigint) >= ?', MIN_GAP) + .limit(1) + .pluck(:pos, :next_pos) + .first - { start: gap[0], end: gap[1] } + return if gap.nil? || gap.first == end_is_nil + + { start: gap.first, end: gap.second || end_is_nil } end def optimum_delta_for_gap(gap) @@ -232,9 +460,10 @@ module RelativePositioning [delta, IDEAL_DISTANCE].min end - def move_sequence(start_pos, end_pos, delta) - scoped_items - .where.not(id: self.id) + def move_sequence(start_pos, end_pos, delta, include_self = false) + relation = include_self ? scoped_items : relative_siblings + + relation .where('relative_position BETWEEN ? AND ?', start_pos, end_pos) .update_all("relative_position = relative_position + #{delta}") end @@ -255,6 +484,10 @@ module RelativePositioning .first&.last end + def relative_siblings(relation = scoped_items) + relation.id_not_in(id) + end + def scoped_items self.class.relative_positioning_query_base(self) end diff --git a/app/views/groups/issues.html.haml b/app/views/groups/issues.html.haml index 04e0ff30dc9..1358e848154 100644 --- a/app/views/groups/issues.html.haml +++ b/app/views/groups/issues.html.haml @@ -25,7 +25,7 @@ - if Feature.enabled?(:vue_issuables_list, @group) .js-issuables-list{ data: { endpoint: expose_url(api_v4_groups_issues_path(id: @group.id)), 'can-bulk-edit': @can_bulk_update.to_json, - 'empty-svg-path': image_path('illustrations/issues.svg'), + 'empty-state-meta': { svg_path: image_path('illustrations/issues.svg') }, 'sort-key': @sort } } - else = render 'shared/issues' diff --git a/app/views/projects/_import_project_pane.html.haml b/app/views/projects/_import_project_pane.html.haml index 4769a7c89c2..dd7971f6db0 100644 --- a/app/views/projects/_import_project_pane.html.haml +++ b/app/views/projects/_import_project_pane.html.haml @@ -46,7 +46,7 @@ - if fogbugz_import_enabled? %div = link_to new_import_fogbugz_path, class: 'btn import_fogbugz', **tracking_attrs(track_label, 'click_button', 'fogbugz') do - = icon('bug', text: 'Fogbugz') + = icon('bug', text: 'FogBugz') - if gitea_import_enabled? %div diff --git a/app/views/projects/issues/_issues.html.haml b/app/views/projects/issues/_issues.html.haml index f086982d3f2..1e24b08ece2 100644 --- a/app/views/projects/issues/_issues.html.haml +++ b/app/views/projects/issues/_issues.html.haml @@ -1,9 +1,13 @@ - if Feature.enabled?(:vue_issuables_list, @project) - .js-issuables-list{ data: { endpoint: expose_url(api_v4_projects_issues_path(id: @project.id)), - 'create_issue_path': expose_url(new_project_issue_path(@project)), + - data_endpoint = local_assigns.fetch(:data_endpoint, expose_path(api_v4_projects_issues_path(id: @project.id))) + - default_empty_state_meta = { create_issue_path: new_project_issue_path(@project), svg_path: image_path('illustrations/issues.svg') } + - data_empty_state_meta = local_assigns.fetch(:data_empty_state_meta, default_empty_state_meta) + - type = local_assigns.fetch(:type, '') + .js-issuables-list{ data: { endpoint: data_endpoint, + 'empty-state-meta': data_empty_state_meta.to_json, 'can-bulk-edit': @can_bulk_update.to_json, - 'empty-svg-path': image_path('illustrations/issues.svg'), - 'sort-key': @sort } } + 'sort-key': @sort, + 'type': type } } - else - empty_state_path = local_assigns.fetch(:empty_state_path, 'shared/empty_states/issues') %ul.content-list.issues-list.issuable-list{ class: ("manual-ordering" if @sort == 'relative_position') } diff --git a/app/views/projects/issues/_service_desk_empty_state.html.haml b/app/views/projects/issues/_service_desk_empty_state.html.haml new file mode 100644 index 00000000000..4f004439f45 --- /dev/null +++ b/app/views/projects/issues/_service_desk_empty_state.html.haml @@ -0,0 +1,33 @@ +- service_desk_enabled = @project.service_desk_enabled? + +- can_edit_project_settings = can?(current_user, :admin_project, @project) +- title_text = _("Use Service Desk to connect with your users (e.g. to offer customer support) through email right inside GitLab") + +- if Gitlab::ServiceDesk.supported? + .empty-state + .svg-content + = render 'shared/empty_states/icons/service_desk_empty_state.svg' + + .text-content + %h4= title_text + + - if can_edit_project_settings && service_desk_enabled + %p + = _("Have your users email") + %code= @project.service_desk_address + + %span= _("Those emails automatically become issues (with the comments becoming the email conversation) listed here.") + = link_to _('Read more'), help_page_path('user/project/service_desk') + + - if can_edit_project_settings && !service_desk_enabled + .text-center + = link_to _("Turn on Service Desk"), edit_project_path(@project), class: 'btn btn-success' +- else + .empty-state + .svg-content + = render 'shared/empty_states/icons/service_desk_setup.svg' + .text-content + %h4= _('Service Desk is enabled but not yet active') + %p + = _("You must set up incoming email before it becomes active.") + = link_to _('More information'), help_page_path('administration/incoming_email', anchor: 'set-it-up') diff --git a/app/views/projects/issues/_service_desk_info_content.html.haml b/app/views/projects/issues/_service_desk_info_content.html.haml index 7eff0277e83..7fa2f3fab00 100644 --- a/app/views/projects/issues/_service_desk_info_content.html.haml +++ b/app/views/projects/issues/_service_desk_info_content.html.haml @@ -1,39 +1,23 @@ -- is_empty_state = @issues.blank? - service_desk_enabled = @project.service_desk_enabled? -- callout_selector = is_empty_state ? 'empty-state' : 'non-empty-state media' -- svg_path = !is_empty_state ? 'shared/empty_states/icons/service_desk_callout.svg' : 'shared/empty_states/icons/service_desk_empty_state.svg' - can_edit_project_settings = can?(current_user, :admin_project, @project) - title_text = _("Use Service Desk to connect with your users (e.g. to offer customer support) through email right inside GitLab") -- if Gitlab::ServiceDesk.supported? - %div{ class: "#{callout_selector}" } - .svg-content - = render svg_path +.non-empty-state.media + .svg-content + = render 'shared/empty_states/icons/service_desk_callout.svg' - %div{ class: is_empty_state ? "text-content" : "gl-mt-3 gl-ml-3" } - - if is_empty_state - %h4= title_text - - else - %h5= title_text + .gl-mt-3.gl-ml-3 + %h5= title_text - - if can_edit_project_settings && service_desk_enabled - %p - = _("Have your users email") - %code= @project.service_desk_address + - if can_edit_project_settings && service_desk_enabled + %p + = _("Have your users email") + %code= @project.service_desk_address - %span= _("Those emails automatically become issues (with the comments becoming the email conversation) listed here.") - = link_to _('Read more'), help_page_path('user/project/service_desk') + %span= _("Those emails automatically become issues (with the comments becoming the email conversation) listed here.") + = link_to _('Read more'), help_page_path('user/project/service_desk') - - if can_edit_project_settings && !service_desk_enabled - %div{ class: is_empty_state ? "text-center" : "gl-mt-3" } - = link_to _("Turn on Service Desk"), edit_project_path(@project), class: 'btn btn-success' -- else - .empty-state - .svg-content - = render 'shared/empty_states/icons/service_desk_setup.svg' - .text-content - %h4= _('Service Desk is enabled but not yet active') - %p - = _("You must set up incoming email before it becomes active.") - = link_to _('More information'), help_page_path('administration/incoming_email', anchor: 'set-it-up') + - if can_edit_project_settings && !service_desk_enabled + .gl-mt-3 + = link_to _("Turn on Service Desk"), edit_project_path(@project), class: 'btn btn-success' diff --git a/app/views/projects/issues/service_desk.html.haml b/app/views/projects/issues/service_desk.html.haml index 9b0b3ebc9e0..bd260bdf143 100644 --- a/app/views/projects/issues/service_desk.html.haml +++ b/app/views/projects/issues/service_desk.html.haml @@ -5,9 +5,11 @@ - content_for :breadcrumbs_extra do = render "projects/issues/nav_btns", show_export_button: false, show_rss_button: false -- support_bot_attrs = UserSerializer.new.represent(User.support_bot).to_json +- support_bot_attrs = { service_desk_enabled: @project.service_desk_enabled?, **UserSerializer.new.represent(User.support_bot) }.to_json -%div{ class: "js-service-desk-issues service-desk-issues", data: { support_bot: support_bot_attrs } } +- data_endpoint = "#{expose_path(api_v4_projects_issues_path(id: @project.id))}?author_id=#{User.support_bot.id}" + +%div{ class: "js-service-desk-issues service-desk-issues", data: { support_bot: support_bot_attrs, service_desk_meta: service_desk_meta(@project) } } .top-area = render 'shared/issuable/nav', type: :issues .nav-controls.d-block.d-sm-none @@ -15,7 +17,15 @@ - if @issues.present? = render 'shared/issuable/search_bar', type: :issues - = render 'service_desk_info_content' + - if Gitlab::ServiceDesk.supported? + = render 'service_desk_info_content' + -# TODO Remove empty_state_path once vue_issuables_list FF is removed. + -# https://gitlab.com/gitlab-org/gitlab/-/issues/235652 + -# `empty_state_path` is used to render the empty state in the HAML version of issuables list. .issues-holder - = render 'projects/issues/issues', empty_state_path: 'service_desk_info_content' + = render 'projects/issues/issues', + empty_state_path: 'service_desk_empty_state', + data_endpoint: data_endpoint, + data_empty_state_meta: service_desk_meta(@project), + type: 'service_desk' diff --git a/changelogs/unreleased/196066-add-milestone-expired-info-be.yml b/changelogs/unreleased/196066-add-milestone-expired-info-be.yml new file mode 100644 index 00000000000..5e656e61c43 --- /dev/null +++ b/changelogs/unreleased/196066-add-milestone-expired-info-be.yml @@ -0,0 +1,5 @@ +--- +title: Add include_parent_milestones param to project and group milestones API endpoints +merge_request: 38800 +author: +type: added diff --git a/changelogs/unreleased/218536-removing-ci_composite_status.yml b/changelogs/unreleased/218536-removing-ci_composite_status.yml new file mode 100644 index 00000000000..e0e164785fa --- /dev/null +++ b/changelogs/unreleased/218536-removing-ci_composite_status.yml @@ -0,0 +1,5 @@ +--- +title: Remove FF ci_composite_status and related codes +merge_request: 39498 +author: +type: other diff --git a/changelogs/unreleased/227678-create-incident-button-alert.yml b/changelogs/unreleased/227678-create-incident-button-alert.yml new file mode 100644 index 00000000000..39365e29f52 --- /dev/null +++ b/changelogs/unreleased/227678-create-incident-button-alert.yml @@ -0,0 +1,5 @@ +--- +title: Rename create issue button to create incidents in ALert details +merge_request: 39684 +author: +type: changed diff --git a/changelogs/unreleased/229636-empty-incidents-list.yml b/changelogs/unreleased/229636-empty-incidents-list.yml new file mode 100644 index 00000000000..5925a1abf3d --- /dev/null +++ b/changelogs/unreleased/229636-empty-incidents-list.yml @@ -0,0 +1,5 @@ +--- +title: Empty State for the Incident list +merge_request: 39718 +author: +type: added diff --git a/changelogs/unreleased/234066-create-issuelink-for-vulnerabilities-that-do-not-have-them.yml b/changelogs/unreleased/234066-create-issuelink-for-vulnerabilities-that-do-not-have-them.yml deleted file mode 100644 index c577de4d079..00000000000 --- a/changelogs/unreleased/234066-create-issuelink-for-vulnerabilities-that-do-not-have-them.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Create IssueLink for Vulnerabilities that do not have them -merge_request: 39098 -author: -type: fixed diff --git a/changelogs/unreleased/ajk-relative-positioning-improvements.yml b/changelogs/unreleased/ajk-relative-positioning-improvements.yml new file mode 100644 index 00000000000..20b2c4b82b7 --- /dev/null +++ b/changelogs/unreleased/ajk-relative-positioning-improvements.yml @@ -0,0 +1,5 @@ +--- +title: Performance and robustness improvements for relative positioning +merge_request: 39807 +author: +type: performance diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile index bf5c6e4c821..80e832da508 100644 --- a/danger/roulette/Dangerfile +++ b/danger/roulette/Dangerfile @@ -40,7 +40,6 @@ MARKDOWN OPTIONAL_REVIEW_TEMPLATE = "%{role} review is optional for %{category}".freeze NOT_AVAILABLE_TEMPLATE = 'No %{role} available'.freeze -TIMEZONE_EXPERIMENT = true def note_for_spins_role(spins, role) spins.each do |spin| @@ -81,12 +80,10 @@ categories << :database if helper.gitlab_helper&.mr_labels&.include?('database') if changes.any? project = helper.project_name - timezone_roulette_spins = roulette.spin(project, categories, timezone_experiment: true) random_roulette_spins = roulette.spin(project, categories, timezone_experiment: false) - rows = timezone_roulette_spins.map do |spin| - fallback_spin = random_roulette_spins.find { |random_roulette_spins| random_roulette_spins.category == spin.category } - markdown_row_for_spins(spin.category, [spin, fallback_spin]) + rows = random_roulette_spins.map do |spin| + markdown_row_for_spins(spin.category, [spin]) end markdown(MESSAGE) diff --git a/db/post_migrate/20200811130000_create_index_vulnerabilities_feedback_issue_id_not_null.rb b/db/post_migrate/20200811130000_create_index_vulnerabilities_feedback_issue_id_not_null.rb deleted file mode 100644 index 118076eb254..00000000000 --- a/db/post_migrate/20200811130000_create_index_vulnerabilities_feedback_issue_id_not_null.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -class CreateIndexVulnerabilitiesFeedbackIssueIdNotNull < ActiveRecord::Migration[6.0] - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_concurrent_index :vulnerability_feedback, :id, where: 'issue_id IS NOT NULL', - name: "index_vulnerability_feedback_on_issue_id_not_null" - end - - def down - remove_concurrent_index_by_name :vulnerability_feedback, - :index_vulnerability_feedback_on_issue_id_not_null - end -end diff --git a/db/post_migrate/20200811130433_create_missing_vulnerabilities_issue_links.rb b/db/post_migrate/20200811130433_create_missing_vulnerabilities_issue_links.rb deleted file mode 100644 index 7da8204deed..00000000000 --- a/db/post_migrate/20200811130433_create_missing_vulnerabilities_issue_links.rb +++ /dev/null @@ -1,49 +0,0 @@ -# frozen_string_literal: true - -class CreateMissingVulnerabilitiesIssueLinks < ActiveRecord::Migration[6.0] - class VulnerabilitiesFeedback < ActiveRecord::Base - include EachBatch - self.table_name = 'vulnerability_feedback' - end - - class VulnerabilitiesIssueLink < ActiveRecord::Base - self.table_name = 'vulnerability_issue_links' - LINK_TYPE_CREATED = 2 - end - - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - # https://github.com/rails/rails/issues/35493 - VulnerabilitiesFeedback.where('issue_id IS NOT NULL').each_batch do |relation| - timestamp = Time.now - values = relation - .joins("JOIN vulnerability_occurrences vo ON vo.project_id = vulnerability_feedback.project_id AND vo.report_type = vulnerability_feedback.category AND encode(vo.project_fingerprint, 'hex') = vulnerability_feedback.project_fingerprint") - .pluck(:vulnerability_id, :issue_id) - .map do |v_id, i_id| - { - vulnerability_id: v_id, - issue_id: i_id, - link_type: VulnerabilitiesIssueLink::LINK_TYPE_CREATED, - created_at: timestamp, - updated_at: timestamp - } - end - - next if values.empty? - - VulnerabilitiesIssueLink.insert_all( - values, - returning: false, - unique_by: %i[vulnerability_id issue_id] - ) - end - end - - def down - end -end diff --git a/db/schema_migrations/20200811130000 b/db/schema_migrations/20200811130000 deleted file mode 100644 index a7df459c416..00000000000 --- a/db/schema_migrations/20200811130000 +++ /dev/null @@ -1 +0,0 @@ -36d3db5618a56a0ea03272563fe254590d6af1f7d2610a1f01a5054b1cda1a7d
\ No newline at end of file diff --git a/db/schema_migrations/20200811130433 b/db/schema_migrations/20200811130433 deleted file mode 100644 index 303468e8949..00000000000 --- a/db/schema_migrations/20200811130433 +++ /dev/null @@ -1 +0,0 @@ -e8fc0809b5bd3248dc625602deeaaef16e2db6b33d8eaf51fdcc1c67dee49e17
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index b423b9bdeac..e94e4e151bd 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20971,8 +20971,6 @@ CREATE INDEX index_vulnerability_feedback_on_comment_author_id ON public.vulnera CREATE INDEX index_vulnerability_feedback_on_issue_id ON public.vulnerability_feedback USING btree (issue_id); -CREATE INDEX index_vulnerability_feedback_on_issue_id_not_null ON public.vulnerability_feedback USING btree (id) WHERE (issue_id IS NOT NULL); - CREATE INDEX index_vulnerability_feedback_on_merge_request_id ON public.vulnerability_feedback USING btree (merge_request_id); CREATE INDEX index_vulnerability_feedback_on_pipeline_id ON public.vulnerability_feedback USING btree (pipeline_id); diff --git a/doc/api/group_milestones.md b/doc/api/group_milestones.md index e157655a713..e992637f4f0 100644 --- a/doc/api/group_milestones.md +++ b/doc/api/group_milestones.md @@ -27,13 +27,14 @@ GET /groups/:id/milestones?search=version Parameters: -| Attribute | Type | Required | Description | -| --------- | ------ | -------- | ----------- | -| `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user | -| `iids[]` | integer array | no | Return only the milestones having the given `iid` | -| `state` | string | no | Return only `active` or `closed` milestones | -| `title` | string | no | Return only the milestones having the given `title` | -| `search` | string | no | Return only milestones with a title or description matching the provided string | +| Attribute | Type | Required | Description | +| --------- | ------ | -------- | ----------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user | +| `iids[]` | integer array | no | Return only the milestones having the given `iid` (Note: ignored if `include_parent_milestones` is set as `true`) | +| `state` | string | no | Return only `active` or `closed` milestones | +| `title` | string | no | Return only the milestones having the given `title` | +| `search` | string | no | Return only milestones with a title or description matching the provided string | +| `include_parent_milestones` | boolean | optional | Include milestones from parent group and its ancestors. Introduced in [GitLab 13.4](https://gitlab.com/gitlab-org/gitlab/-/issues/196066) | ```shell curl --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/5/milestones" diff --git a/doc/api/milestones.md b/doc/api/milestones.md index b5702c7d6e0..7b4d1cc331d 100644 --- a/doc/api/milestones.md +++ b/doc/api/milestones.md @@ -25,13 +25,14 @@ GET /projects/:id/milestones?search=version Parameters: -| Attribute | Type | Required | Description | -| --------- | ------ | -------- | ----------- | -| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `iids[]` | integer array | optional | Return only the milestones having the given `iid` | -| `state` | string | optional | Return only `active` or `closed` milestones | -| `title` | string | optional | Return only the milestones having the given `title` | -| `search` | string | optional | Return only milestones with a title or description matching the provided string | +| Attribute | Type | Required | Description | +| ---------------------------- | ------ | -------- | ----------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `iids[]` | integer array | optional | Return only the milestones having the given `iid` (Note: ignored if `include_parent_milestones` is set as `true`) | +| `state` | string | optional | Return only `active` or `closed` milestones | +| `title` | string | optional | Return only the milestones having the given `title` | +| `search` | string | optional | Return only milestones with a title or description matching the provided string | +| `include_parent_milestones` | boolean | optional | Include group milestones from parent group and its ancestors. Introduced in [GitLab 13.4](https://gitlab.com/gitlab-org/gitlab/-/issues/196066) | ```shell curl --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/5/milestones" diff --git a/doc/user/group/epics/index.md b/doc/user/group/epics/index.md index 7f5a4e2b6da..04b57d13828 100644 --- a/doc/user/group/epics/index.md +++ b/doc/user/group/epics/index.md @@ -70,7 +70,7 @@ to add an issue to an epic, reorder issues, move issues between epics, or promot ## Issue health status in Epic tree **(ULTIMATE)** > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/199184) in [GitLab Ultimate](https://about.gitlab.com/pricing/) 12.10. -> - [Improved](https://gitlab.com/gitlab-org/gitlab/-/issues/220867) in [GitLab Ultimate](https://about.gitlab.com/pricing/) 13.3. The health status of a closed issue will be hidden. +> - The health status of a closed issue [will be hidden](https://gitlab.com/gitlab-org/gitlab/-/issues/220867) in [GitLab Ultimate](https://about.gitlab.com/pricing/) 13.3 or later. You can report on and quickly respond to the health of individual issues and epics by setting a red, amber, or green [health status on an issue](../../project/issues/index.md#health-status-ultimate), diff --git a/lib/api/milestone_responses.rb b/lib/api/milestone_responses.rb index 8ff885983bc..b337b992841 100644 --- a/lib/api/milestone_responses.rb +++ b/lib/api/milestone_responses.rb @@ -14,10 +14,12 @@ module API params :list_params do optional :state, type: String, values: %w[active closed all], default: 'all', - desc: 'Return "active", "closed", or "all" milestones' + desc: 'Return "active", "closed", or "all" milestones' optional :iids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IIDs of the milestones' optional :title, type: String, desc: 'The title of the milestones' optional :search, type: String, desc: 'The search criteria for the title or description of the milestone' + optional :include_parent_milestones, type: Grape::API::Boolean, default: false, + desc: 'Include group milestones from parent and its ancestors' use :pagination end @@ -25,15 +27,18 @@ module API requires :milestone_id, type: Integer, desc: 'The milestone ID number' optional :title, type: String, desc: 'The title of the milestone' optional :state_event, type: String, values: %w[close activate], - desc: 'The state event of the milestone ' + desc: 'The state event of the milestone ' use :optional_params at_least_one_of :title, :description, :start_date, :due_date, :state_event end def list_milestones_for(parent) - milestones = parent.milestones.order_id_desc + milestones = init_milestones_collection(parent) milestones = Milestone.filter_by_state(milestones, params[:state]) - milestones = filter_by_iid(milestones, params[:iids]) if params[:iids].present? + if params[:iids].present? && !params[:include_parent_milestones] + milestones = filter_by_iid(milestones, params[:iids]) + end + milestones = filter_by_title(milestones, params[:title]) if params[:title] milestones = filter_by_search(milestones, params[:search]) if params[:search] @@ -96,6 +101,41 @@ module API [MergeRequestsFinder, Entities::MergeRequestBasic] end end + + def init_milestones_collection(parent) + milestones = if params[:include_parent_milestones].present? + parent_and_ancestors_milestones(parent) + else + parent.milestones + end + + milestones.order_id_desc + end + + def parent_and_ancestors_milestones(parent) + project_id, group_ids = if parent.is_a?(Project) + [parent.id, project_group_ids(parent)] + else + [nil, parent_group_ids(parent)] + end + + Milestone.for_projects_and_groups(project_id, group_ids) + end + + def project_group_ids(parent) + group = parent.group + return unless group.present? + + group.self_and_ancestors.select(:id) + end + + def parent_group_ids(group) + return unless group.present? + + group.self_and_ancestors + .public_or_visible_to_user(current_user) + .select(:id) + end end end end diff --git a/lib/gitlab/ci/features.rb b/lib/gitlab/ci/features.rb index 38767acf716..2f6667d3600 100644 --- a/lib/gitlab/ci/features.rb +++ b/lib/gitlab/ci/features.rb @@ -18,10 +18,6 @@ module Gitlab ::Feature.enabled?(:ci_instance_variables_ui, default_enabled: true) end - def self.composite_status?(project) - ::Feature.enabled?(:ci_composite_status, project, default_enabled: true) - end - def self.pipeline_latest? ::Feature.enabled?(:ci_pipeline_latest, default_enabled: true) end diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index 859b53b9887..e7df9fd27f0 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -22,6 +22,7 @@ module Gitlab # https://www.postgresql.org/docs/9.2/static/datatype-numeric.html MAX_INT_VALUE = 2147483647 + MIN_INT_VALUE = -2147483648 # The max value between MySQL's TIMESTAMP and PostgreSQL's timestampz: # https://www.postgresql.org/docs/9.1/static/datatype-datetime.html diff --git a/locale/gitlab.pot b/locale/gitlab.pot index f3237f7aea3..4ccd3ffb06c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2108,7 +2108,7 @@ msgstr "" msgid "AlertManagement|Authorize external service" msgstr "" -msgid "AlertManagement|Create issue" +msgid "AlertManagement|Create incident" msgstr "" msgid "AlertManagement|Critical" @@ -2234,7 +2234,7 @@ msgstr "" msgid "AlertManagement|View alerts in Opsgenie" msgstr "" -msgid "AlertManagement|View issue" +msgid "AlertManagement|View incident" msgstr "" msgid "AlertManagement|You have enabled the Opsgenie integration. Your alerts will be visible directly in Opsgenie." @@ -2264,7 +2264,7 @@ msgstr "" msgid "AlertSettings|Authorization key" msgstr "" -msgid "AlertSettings|Authorization key has been successfully reset" +msgid "AlertSettings|Authorization key has been successfully reset. Please save your changes now." msgstr "" msgid "AlertSettings|Copy" @@ -12959,6 +12959,9 @@ msgstr "" msgid "IncidentManagement|All" msgstr "" +msgid "IncidentManagement|All alerts promoted to incidents will automatically be displayed within the list. You can also create a new incident using the button below." +msgstr "" + msgid "IncidentManagement|Assignees" msgstr "" @@ -12971,6 +12974,9 @@ msgstr "" msgid "IncidentManagement|Date created" msgstr "" +msgid "IncidentManagement|Display your incidents in a dedicated view" +msgstr "" + msgid "IncidentManagement|Incident" msgstr "" diff --git a/spec/features/boards/issue_ordering_spec.rb b/spec/features/boards/issue_ordering_spec.rb index 03a76d9d3fd..87d29eed68d 100644 --- a/spec/features/boards/issue_ordering_spec.rb +++ b/spec/features/boards/issue_ordering_spec.rb @@ -21,7 +21,7 @@ RSpec.describe 'Issue Boards', :js do end context 'un-ordered issues' do - let!(:issue4) { create(:labeled_issue, project: project, labels: [label]) } + let!(:issue4) { create(:labeled_issue, project: project, labels: [label], relative_position: nil) } before do visit project_board_path(project, board) diff --git a/spec/features/issues/service_desk_spec.rb b/spec/features/issues/service_desk_spec.rb index 0995aa11654..2912ac33625 100644 --- a/spec/features/issues/service_desk_spec.rb +++ b/spec/features/issues/service_desk_spec.rb @@ -7,8 +7,6 @@ RSpec.describe 'Service Desk Issue Tracker', :js do let(:user) { create(:user) } before do - stub_feature_flags(vue_issuables_list: false) - allow(Gitlab::IncomingEmail).to receive(:enabled?).and_return(true) allow(Gitlab::IncomingEmail).to receive(:supports_wildcard?).and_return(true) @@ -78,11 +76,9 @@ RSpec.describe 'Service Desk Issue Tracker', :js do context 'when service desk has been activated' do context 'when there are no issues' do describe 'service desk info content' do - before do + it 'displays the large info box, documentation, and the address' do visit service_desk_project_issues_path(project) - end - it 'displays the large info box, documentation, and the address' do aggregate_failures do expect(page).to have_css('.empty-state') expect(page).to have_link('Read more', href: help_page_path('user/project/service_desk')) diff --git a/spec/frontend/alert_management/components/alert_details_spec.js b/spec/frontend/alert_management/components/alert_details_spec.js index f91f08cae63..2c4ed100a56 100644 --- a/spec/frontend/alert_management/components/alert_details_spec.js +++ b/spec/frontend/alert_management/components/alert_details_spec.js @@ -63,9 +63,9 @@ describe('AlertDetails', () => { mock.restore(); }); - const findCreateIssueBtn = () => wrapper.find('[data-testid="createIssueBtn"]'); - const findViewIssueBtn = () => wrapper.find('[data-testid="viewIssueBtn"]'); - const findIssueCreationAlert = () => wrapper.find('[data-testid="issueCreationError"]'); + const findCreateIncidentBtn = () => wrapper.find('[data-testid="createIncidentBtn"]'); + const findViewIncidentBtn = () => wrapper.find('[data-testid="viewIncidentBtn"]'); + const findIncidentCreationAlert = () => wrapper.find('[data-testid="incidentCreationError"]'); describe('Alert details', () => { describe('when alert is null', () => { @@ -135,18 +135,20 @@ describe('AlertDetails', () => { }); }); - describe('Create issue from alert', () => { - it('should display "View issue" button that links the issue page when issue exists', () => { + describe('Create incident from alert', () => { + it('should display "View incident" button that links the incident page when incident exists', () => { const issueIid = '3'; mountComponent({ data: { alert: { ...mockAlert, issueIid }, sidebarStatus: false }, }); - expect(findViewIssueBtn().exists()).toBe(true); - expect(findViewIssueBtn().attributes('href')).toBe(joinPaths(projectIssuesPath, issueIid)); - expect(findCreateIssueBtn().exists()).toBe(false); + expect(findViewIncidentBtn().exists()).toBe(true); + expect(findViewIncidentBtn().attributes('href')).toBe( + joinPaths(projectIssuesPath, issueIid), + ); + expect(findCreateIncidentBtn().exists()).toBe(false); }); - it('should display "Create issue" button when issue doesn\'t exist yet', () => { + it('should display "Create incident" button when incident doesn\'t exist yet', () => { const issueIid = null; mountComponent({ mountMethod: mount, @@ -154,8 +156,8 @@ describe('AlertDetails', () => { }); return wrapper.vm.$nextTick().then(() => { - expect(findViewIssueBtn().exists()).toBe(false); - expect(findCreateIssueBtn().exists()).toBe(true); + expect(findViewIncidentBtn().exists()).toBe(false); + expect(findCreateIncidentBtn().exists()).toBe(true); }); }); @@ -165,7 +167,7 @@ describe('AlertDetails', () => { .spyOn(wrapper.vm.$apollo, 'mutate') .mockResolvedValue({ data: { createAlertIssue: { issue: { iid: issueIid } } } }); - findCreateIssueBtn().trigger('click'); + findCreateIncidentBtn().trigger('click'); expect(wrapper.vm.$apollo.mutate).toHaveBeenCalledWith({ mutation: createIssueMutation, variables: { @@ -175,7 +177,7 @@ describe('AlertDetails', () => { }); }); - it('shows error alert when issue creation fails ', () => { + it('shows error alert when incident creation fails ', () => { const errorMsg = 'Something went wrong'; mountComponent({ mountMethod: mount, @@ -183,10 +185,10 @@ describe('AlertDetails', () => { }); jest.spyOn(wrapper.vm.$apollo, 'mutate').mockRejectedValue(errorMsg); - findCreateIssueBtn().trigger('click'); + findCreateIncidentBtn().trigger('click'); setImmediate(() => { - expect(findIssueCreationAlert().text()).toBe(errorMsg); + expect(findIncidentCreationAlert().text()).toBe(errorMsg); }); }); }); diff --git a/spec/frontend/alert_management/components/alert_management_empty_state_spec.js b/spec/frontend/alert_management/components/alert_management_empty_state_spec.js index 0d1214211d3..6712282503d 100644 --- a/spec/frontend/alert_management/components/alert_management_empty_state_spec.js +++ b/spec/frontend/alert_management/components/alert_management_empty_state_spec.js @@ -15,6 +15,7 @@ describe('AlertManagementEmptyState', () => { wrapper = shallowMount(AlertManagementEmptyState, { propsData: { enableAlertManagementPath: '/link', + alertsHelpUrl: '/link', emptyAlertSvgPath: 'illustration/path', ...props, }, diff --git a/spec/frontend/alert_management/components/alert_management_list_wrapper_spec.js b/spec/frontend/alert_management/components/alert_management_list_wrapper_spec.js index 4644406c037..c36107c28ce 100644 --- a/spec/frontend/alert_management/components/alert_management_list_wrapper_spec.js +++ b/spec/frontend/alert_management/components/alert_management_list_wrapper_spec.js @@ -19,6 +19,7 @@ describe('AlertManagementList', () => { propsData: { projectPath: 'gitlab-org/gitlab', enableAlertManagementPath: '/link', + alertsHelpUrl: '/link', populatingAlertsHelpUrl: '/help/help-page.md#populating-alert-data', emptyAlertSvgPath: 'illustration/path', ...props, diff --git a/spec/frontend/alert_settings/__snapshots__/alert_settings_form_spec.js.snap b/spec/frontend/alert_settings/__snapshots__/alert_settings_form_spec.js.snap index f271aeb5d95..16e92bf505a 100644 --- a/spec/frontend/alert_settings/__snapshots__/alert_settings_form_spec.js.snap +++ b/spec/frontend/alert_settings/__snapshots__/alert_settings_form_spec.js.snap @@ -26,9 +26,7 @@ exports[`AlertsSettingsForm with default values renders the initial template 1`] </gl-form-group-stub> <gl-form-group-stub label=\\"Authorization key\\" label-for=\\"authorization-key\\" label-class=\\"label-bold\\"> <gl-form-input-group-stub value=\\"abcedfg123\\" predefinedoptions=\\"[object Object]\\" id=\\"authorization-key\\" readonly=\\"\\" class=\\"gl-mb-2\\"></gl-form-input-group-stub> - <div class=\\"gl-display-flex gl-justify-content-end\\"> - <gl-button-stub category=\\"primary\\" variant=\\"default\\" size=\\"medium\\" icon=\\"\\" disabled=\\"true\\" class=\\"gl-mt-3\\" role=\\"button\\" tabindex=\\"0\\">Reset key</gl-button-stub> - </div> + <gl-button-stub category=\\"primary\\" variant=\\"default\\" size=\\"medium\\" icon=\\"\\" disabled=\\"true\\" class=\\"gl-mt-3\\" role=\\"button\\" tabindex=\\"0\\">Reset key</gl-button-stub> <gl-modal-stub modalid=\\"authKeyModal\\" titletag=\\"h4\\" modalclass=\\"\\" size=\\"md\\" title=\\"Reset key\\" ok-title=\\"Reset key\\" ok-variant=\\"danger\\"> Resetting the authorization key for this project will require updating the authorization key in every alert source it is enabled in. </gl-modal-stub> diff --git a/spec/frontend/alert_settings/alert_settings_form_spec.js b/spec/frontend/alert_settings/alert_settings_form_spec.js index 5a04d768645..87a631bda56 100644 --- a/spec/frontend/alert_settings/alert_settings_form_spec.js +++ b/spec/frontend/alert_settings/alert_settings_form_spec.js @@ -11,41 +11,36 @@ const KEY = 'abcedfg123'; const INVALID_URL = 'http://invalid'; const ACTIVATED = false; -const defaultProps = { - generic: { - initialAuthorizationKey: KEY, - formPath: INVALID_URL, - url: GENERIC_URL, - alertsSetupUrl: INVALID_URL, - alertsUsageUrl: INVALID_URL, - activated: ACTIVATED, - }, - prometheus: { - prometheusAuthorizationKey: KEY, - prometheusFormPath: INVALID_URL, - prometheusUrl: PROMETHEUS_URL, - activated: ACTIVATED, - }, - opsgenie: { - opsgenieMvcIsAvailable: true, - formPath: INVALID_URL, - activated: ACTIVATED, - opsgenieMvcTargetUrl: GENERIC_URL, - }, -}; - describe('AlertsSettingsForm', () => { let wrapper; let mockAxios; - const createComponent = (props = defaultProps, { methods } = {}, data) => { + const createComponent = ({ methods } = {}, data) => { wrapper = shallowMount(AlertsSettingsForm, { data() { return { ...data }; }, - propsData: { - ...defaultProps, - ...props, + provide: { + generic: { + authorizationKey: KEY, + formPath: INVALID_URL, + url: GENERIC_URL, + alertsSetupUrl: INVALID_URL, + alertsUsageUrl: INVALID_URL, + activated: ACTIVATED, + }, + prometheus: { + authorizationKey: KEY, + prometheusFormPath: INVALID_URL, + prometheusUrl: PROMETHEUS_URL, + activated: ACTIVATED, + }, + opsgenie: { + opsgenieMvcIsAvailable: true, + formPath: INVALID_URL, + activated: ACTIVATED, + opsgenieMvcTargetUrl: GENERIC_URL, + }, }, methods, }); @@ -83,32 +78,33 @@ describe('AlertsSettingsForm', () => { describe('reset key', () => { it('triggers resetKey method', () => { - const resetGenericKey = jest.fn(); - const methods = { resetGenericKey }; - createComponent(defaultProps, { methods }); + const resetKey = jest.fn(); + const methods = { resetKey }; + createComponent({ methods }); wrapper.find(GlModal).vm.$emit('ok'); - expect(resetGenericKey).toHaveBeenCalled(); + expect(resetKey).toHaveBeenCalled(); }); it('updates the authorization key on success', () => { - const formPath = 'some/path'; - mockAxios.onPut(formPath, { service: { token: '' } }).replyOnce(200, { token: 'newToken' }); - createComponent({ generic: { ...defaultProps.generic, formPath } }); + createComponent( + {}, + { + authKey: 'newToken', + }, + ); - return wrapper.vm.resetGenericKey().then(() => { - expect(findAuthorizationKey().attributes('value')).toBe('newToken'); - }); + expect(findAuthorizationKey().attributes('value')).toBe('newToken'); }); it('shows a alert message on error', () => { const formPath = 'some/path'; mockAxios.onPut(formPath).replyOnce(404); - createComponent({ generic: { ...defaultProps.generic, formPath } }); + createComponent(); - return wrapper.vm.resetGenericKey().then(() => { + return wrapper.vm.resetKey().then(() => { expect(wrapper.find(GlAlert).exists()).toBe(true); }); }); @@ -118,22 +114,18 @@ describe('AlertsSettingsForm', () => { it('triggers toggleActivated method', () => { const toggleService = jest.fn(); const methods = { toggleService }; - createComponent(defaultProps, { methods }); + createComponent({ methods }); wrapper.find(ToggleButton).vm.$emit('change', true); - expect(toggleService).toHaveBeenCalled(); }); describe('error is encountered', () => { - beforeEach(() => { + it('restores previous value', () => { const formPath = 'some/path'; mockAxios.onPut(formPath).replyOnce(500); - }); - - it('restores previous value', () => { - createComponent({ generic: { ...defaultProps.generic, initialActivated: false } }); - return wrapper.vm.resetGenericKey().then(() => { + createComponent(); + return wrapper.vm.resetKey().then(() => { expect(wrapper.find(ToggleButton).props('value')).toBe(false); }); }); @@ -143,7 +135,6 @@ describe('AlertsSettingsForm', () => { describe('prometheus is active', () => { beforeEach(() => { createComponent( - { prometheus: { ...defaultProps.prometheus, prometheusIsActivated: true } }, {}, { selectedEndpoint: 'prometheus', @@ -164,10 +155,9 @@ describe('AlertsSettingsForm', () => { }); }); - describe('opsgenie is active', () => { + describe('Opsgenie is active', () => { beforeEach(() => { createComponent( - { opsgenie: { ...defaultProps.opsgenie, opsgenieMvcActivated: true } }, {}, { selectedEndpoint: 'opsgenie', @@ -175,15 +165,14 @@ describe('AlertsSettingsForm', () => { ); }); - it('shows a input for the opsgenie target URL', () => { + it('shows a input for the Opsgenie target URL', () => { expect(findApiUrl().exists()).toBe(true); - expect(findSelect().attributes('value')).toBe('opsgenie'); }); }); describe('trigger test alert', () => { beforeEach(() => { - createComponent({ generic: { ...defaultProps.generic, initialActivated: true } }, {}, true); + createComponent({}); }); it('should enable the JSON input', () => { @@ -191,30 +180,19 @@ describe('AlertsSettingsForm', () => { expect(findJsonInput().props('value')).toBe(null); }); - it('should validate JSON input', () => { - createComponent({ generic: { ...defaultProps.generic } }, true, { + it('should validate JSON input', async () => { + createComponent(true, { testAlertJson: '{ "value": "test" }', }); findJsonInput().vm.$emit('change'); - return wrapper.vm.$nextTick().then(() => { - expect(findJsonInput().attributes('state')).toBe('true'); - }); - }); - - describe('alert service is toggled', () => { - it('should show a info alert if successful', () => { - const formPath = 'some/path'; - const toggleService = true; - mockAxios.onPut(formPath).replyOnce(200); - createComponent({ generic: { ...defaultProps.generic, formPath } }); + await wrapper.vm.$nextTick(); - return wrapper.vm.toggleActivated(toggleService).then(() => { - expect(wrapper.find(GlAlert).attributes('variant')).toBe('info'); - }); - }); + expect(findJsonInput().attributes('state')).toBe('true'); + }); + describe('alert service is toggled', () => { it('should show a error alert if failed', () => { const formPath = 'some/path'; const toggleService = true; @@ -222,9 +200,10 @@ describe('AlertsSettingsForm', () => { errors: 'Error message to display', }); - createComponent({ generic: { ...defaultProps.generic, formPath } }); + createComponent(); return wrapper.vm.toggleActivated(toggleService).then(() => { + expect(wrapper.vm.active).toBe(false); expect(wrapper.find(GlAlert).attributes('variant')).toBe('danger'); }); }); diff --git a/spec/frontend/incidents/components/incidents_list_spec.js b/spec/frontend/incidents/components/incidents_list_spec.js index 34dc61772d4..33ddd06d6d9 100644 --- a/spec/frontend/incidents/components/incidents_list_spec.js +++ b/spec/frontend/incidents/components/incidents_list_spec.js @@ -9,6 +9,7 @@ import { GlTab, GlTabs, GlBadge, + GlEmptyState, } from '@gitlab/ui'; import { visitUrl, joinPaths, mergeUrlParams } from '~/lib/utils/url_utility'; import IncidentsList from '~/incidents/components/incidents_list.vue'; @@ -25,6 +26,7 @@ jest.mock('~/lib/utils/url_utility', () => ({ describe('Incidents List', () => { let wrapper; const newIssuePath = 'namespace/project/-/issues/new'; + const emptyListSvgPath = '/assets/empty.svg'; const incidentTemplateName = 'incident'; const incidentType = 'incident'; const incidentsCount = { @@ -48,6 +50,7 @@ describe('Incidents List', () => { const findStatusFilterTabs = () => wrapper.findAll(GlTab); const findStatusFilterBadge = () => wrapper.findAll(GlBadge); const findStatusTabs = () => wrapper.find(GlTabs); + const findEmptyState = () => wrapper.find(GlEmptyState); function mountComponent({ data = { incidents: [], incidentsCount: {} }, loading = false }) { wrapper = mount(IncidentsList, { @@ -70,6 +73,7 @@ describe('Incidents List', () => { incidentType, issuePath: '/project/isssues', publishedAvailable: true, + emptyListSvgPath, }, stubs: { GlButton: true, @@ -97,7 +101,7 @@ describe('Incidents List', () => { data: { incidents: { list: [] }, incidentsCount: {} }, loading: false, }); - expect(findTable().text()).toContain(I18N.noIncidents); + expect(findEmptyState().exists()).toBe(true); }); it('shows error state', () => { @@ -164,7 +168,7 @@ describe('Incidents List', () => { describe('Create Incident', () => { beforeEach(() => { mountComponent({ - data: { incidents: { list: [] }, incidentsCount: {} }, + data: { incidents: { list: mockIncidents }, incidentsCount: {} }, loading: false, }); }); diff --git a/spec/frontend/issuables_list/components/__snapshots__/issuables_list_app_spec.js.snap b/spec/frontend/issuables_list/components/__snapshots__/issuables_list_app_spec.js.snap index 3e445319746..c327b7de827 100644 --- a/spec/frontend/issuables_list/components/__snapshots__/issuables_list_app_spec.js.snap +++ b/spec/frontend/issuables_list/components/__snapshots__/issuables_list_app_spec.js.snap @@ -2,7 +2,6 @@ exports[`Issuables list component with empty issues response with all state should display a catch-all if there are no issues to show 1`] = ` <gl-empty-state-stub - description="The Issue Tracker is the place to add things that need to be improved or solved in a project. You can register or sign in to create issues for this project." svgpath="/emptySvg" title="There are no issues to show" /> diff --git a/spec/frontend/issuables_list/components/issuables_list_app_spec.js b/spec/frontend/issuables_list/components/issuables_list_app_spec.js index 9f4995a54ee..7b4c237c01b 100644 --- a/spec/frontend/issuables_list/components/issuables_list_app_spec.js +++ b/spec/frontend/issuables_list/components/issuables_list_app_spec.js @@ -21,7 +21,7 @@ jest.mock('~/lib/utils/common_utils', () => ({ const TEST_LOCATION = `${TEST_HOST}/issues`; const TEST_ENDPOINT = '/issues'; const TEST_CREATE_ISSUES_PATH = '/createIssue'; -const TEST_EMPTY_SVG_PATH = '/emptySvg'; +const TEST_SVG_PATH = '/emptySvg'; const setUrl = query => { window.location.href = `${TEST_LOCATION}${query}`; @@ -48,11 +48,15 @@ describe('Issuables list component', () => { }; const factory = (props = { sortKey: 'priority' }) => { + const emptyStateMeta = { + createIssuePath: TEST_CREATE_ISSUES_PATH, + svgPath: TEST_SVG_PATH, + }; + wrapper = shallowMount(IssuablesListApp, { propsData: { endpoint: TEST_ENDPOINT, - createIssuePath: TEST_CREATE_ISSUES_PATH, - emptySvgPath: TEST_EMPTY_SVG_PATH, + emptyStateMeta, ...props, }, }); @@ -117,9 +121,10 @@ describe('Issuables list component', () => { expect(wrapper.vm).toMatchObject({ // Props canBulkEdit: false, - createIssuePath: TEST_CREATE_ISSUES_PATH, - emptySvgPath: TEST_EMPTY_SVG_PATH, - + emptyStateMeta: { + createIssuePath: TEST_CREATE_ISSUES_PATH, + svgPath: TEST_SVG_PATH, + }, // Data filters: { state: 'opened', diff --git a/spec/frontend/pages/dashboard/projects/index/components/customize_homepage_banner_spec.js b/spec/frontend/pages/dashboard/projects/index/components/customize_homepage_banner_spec.js index c21525442d2..b3a297ac2c5 100644 --- a/spec/frontend/pages/dashboard/projects/index/components/customize_homepage_banner_spec.js +++ b/spec/frontend/pages/dashboard/projects/index/components/customize_homepage_banner_spec.js @@ -1,7 +1,7 @@ import { shallowMount } from '@vue/test-utils'; -import CustomizeHomepageBanner from '~/pages/dashboard/projects/index/components/customize_homepage_banner.vue'; import { GlBanner } from '@gitlab/ui'; import MockAdapter from 'axios-mock-adapter'; +import CustomizeHomepageBanner from '~/pages/dashboard/projects/index/components/customize_homepage_banner.vue'; import axios from '~/lib/utils/axios_utils'; const svgPath = '/illustrations/background'; diff --git a/spec/frontend/snippets/components/snippet_blob_edit_spec.js b/spec/frontend/snippets/components/snippet_blob_edit_spec.js index 29497012ddf..99529e0cf3f 100644 --- a/spec/frontend/snippets/components/snippet_blob_edit_spec.js +++ b/spec/frontend/snippets/components/snippet_blob_edit_spec.js @@ -2,13 +2,13 @@ import { GlLoadingIcon } from '@gitlab/ui'; import { shallowMount } from '@vue/test-utils'; import AxiosMockAdapter from 'axios-mock-adapter'; import waitForPromises from 'helpers/wait_for_promises'; +import { TEST_HOST } from 'helpers/test_constants'; import SnippetBlobEdit from '~/snippets/components/snippet_blob_edit.vue'; import BlobHeaderEdit from '~/blob/components/blob_edit_header.vue'; import BlobContentEdit from '~/blob/components/blob_edit_content.vue'; import axios from '~/lib/utils/axios_utils'; import { joinPaths } from '~/lib/utils/url_utility'; import createFlash from '~/flash'; -import { TEST_HOST } from 'helpers/test_constants'; jest.mock('~/flash'); diff --git a/spec/helpers/projects/alert_management_helper_spec.rb b/spec/helpers/projects/alert_management_helper_spec.rb index 20464d7f64d..183f0438c35 100644 --- a/spec/helpers/projects/alert_management_helper_spec.rb +++ b/spec/helpers/projects/alert_management_helper_spec.rb @@ -28,7 +28,8 @@ RSpec.describe Projects::AlertManagementHelper do expect(helper.alert_management_data(current_user, project)).to match( 'project-path' => project_path, 'enable-alert-management-path' => setting_path, - 'populating-alerts-help-url' => 'http://test.host/help/user/project/operations/alert_management.html#enable-alert-management', + 'alerts-help-url' => 'http://test.host/help/operations/incident_management/index.md', + 'populating-alerts-help-url' => 'http://test.host/help/operations/incident_management/index.md#enable-alert-management', 'empty-alert-svg-path' => match_asset_path('/assets/illustrations/alert-management-empty-state.svg'), 'user-can-enable-alert-management' => 'true', 'alert-management-enabled' => 'false' diff --git a/spec/helpers/projects/incidents_helper_spec.rb b/spec/helpers/projects/incidents_helper_spec.rb index 8c9e2c0108e..0affa67a902 100644 --- a/spec/helpers/projects/incidents_helper_spec.rb +++ b/spec/helpers/projects/incidents_helper_spec.rb @@ -19,7 +19,8 @@ RSpec.describe Projects::IncidentsHelper do 'new-issue-path' => new_issue_path, 'incident-template-name' => 'incident', 'incident-type' => 'incident', - 'issue-path' => issue_path + 'issue-path' => issue_path, + 'empty-list-svg-path' => match_asset_path('/assets/illustrations/incident-empty-state.svg') ) end end diff --git a/spec/helpers/projects/issues/service_desk_helper_spec.rb b/spec/helpers/projects/issues/service_desk_helper_spec.rb new file mode 100644 index 00000000000..3f488fe692d --- /dev/null +++ b/spec/helpers/projects/issues/service_desk_helper_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::Issues::ServiceDeskHelper do + let_it_be(:project) { create(:project, :public, service_desk_enabled: true) } + let(:user) { build_stubbed(:user) } + let(:current_user) { user } + + describe '#service_desk_meta' do + subject { helper.service_desk_meta(project) } + + context "when service desk is supported and user can edit project settings" do + before do + allow(Gitlab::IncomingEmail).to receive(:enabled?).and_return(true) + allow(Gitlab::IncomingEmail).to receive(:supports_wildcard?).and_return(true) + allow(helper).to receive(:current_user).and_return(user) + allow(helper).to receive(:can?).with(current_user, :admin_project, project).and_return(true) + end + + it { + is_expected.to eq({ + is_service_desk_supported: true, + is_service_desk_enabled: true, + can_edit_project_settings: true, + service_desk_address: project.service_desk_address, + service_desk_help_page: help_page_path('user/project/service_desk'), + edit_project_page: edit_project_path(project), + svg_path: ActionController::Base.helpers.image_path('illustrations/service_desk_empty.svg') + }) + } + end + + context "when service desk is not supported and user cannot edit project settings" do + before do + allow(Gitlab::IncomingEmail).to receive(:enabled?).and_return(false) + allow(Gitlab::IncomingEmail).to receive(:supports_wildcard?).and_return(false) + allow(helper).to receive(:current_user).and_return(user) + allow(helper).to receive(:can?).with(current_user, :admin_project, project).and_return(false) + end + + it { + is_expected.to eq({ + is_service_desk_supported: false, + is_service_desk_enabled: false, + can_edit_project_settings: false, + incoming_email_help_page: help_page_path('administration/incoming_email', anchor: 'set-it-up'), + svg_path: ActionController::Base.helpers.image_path('illustrations/service-desk-setup.svg') + }) + } + end + end +end diff --git a/spec/lib/gitlab/ci/status/composite_spec.rb b/spec/lib/gitlab/ci/status/composite_spec.rb index 47bbc4169b6..e1dcd05373f 100644 --- a/spec/lib/gitlab/ci/status/composite_spec.rb +++ b/spec/lib/gitlab/ci/status/composite_spec.rb @@ -16,48 +16,61 @@ RSpec.describe Gitlab::Ci::Status::Composite do end describe '#status' do - shared_examples 'compares composite with SQL status' do - it 'returns exactly the same result' do - builds = Ci::Build.where(id: all_statuses) + using RSpec::Parameterized::TableSyntax - expect(composite_status.status).to eq(builds.legacy_status) - expect(composite_status.warnings?).to eq(builds.failed_but_allowed.any?) + shared_examples 'compares status and warnings' do + let(:composite_status) do + described_class.new(all_statuses) + end + + it 'returns status and warnings?' do + expect(composite_status.status).to eq(result) + expect(composite_status.warnings?).to eq(has_warnings) end end - shared_examples 'validate all combinations' do |perms| - Ci::HasStatus::STATUSES_ENUM.keys.combination(perms).each do |statuses| - context "with #{statuses.join(",")}" do - it_behaves_like 'compares composite with SQL status' do - let(:all_statuses) do - statuses.map { |status| @statuses[status] } - end - - let(:composite_status) do - described_class.new(all_statuses) - end - end - - Ci::HasStatus::STATUSES_ENUM.each do |allow_failure_status, _| - context "and allow_failure #{allow_failure_status}" do - it_behaves_like 'compares composite with SQL status' do - let(:all_statuses) do - statuses.map { |status| @statuses[status] } + - [@statuses_with_allow_failure[allow_failure_status]] - end - - let(:composite_status) do - described_class.new(all_statuses) - end - end - end - end + context 'allow_failure: false' do + where(:build_statuses, :result, :has_warnings) do + %i(skipped) | 'skipped' | false + %i(skipped success) | 'success' | false + %i(created) | 'created' | false + %i(preparing) | 'preparing' | false + %i(canceled success skipped) | 'canceled' | false + %i(pending created skipped) | 'pending' | false + %i(pending created skipped success) | 'running' | false + %i(running created skipped success) | 'running' | false + %i(success waiting_for_resource) | 'waiting_for_resource' | false + %i(success manual) | 'manual' | false + %i(success scheduled) | 'scheduled' | false + %i(created preparing) | 'preparing' | false + %i(created success pending) | 'running' | false + %i(skipped success failed) | 'failed' | false + end + + with_them do + let(:all_statuses) do + build_statuses.map { |status| @statuses[status] } end + + it_behaves_like 'compares status and warnings' end end - it_behaves_like 'validate all combinations', 0 - it_behaves_like 'validate all combinations', 1 - it_behaves_like 'validate all combinations', 2 + context 'allow_failure: true' do + where(:build_statuses, :result, :has_warnings) do + %i(manual) | 'skipped' | false + %i(skipped failed) | 'success' | true + %i(created failed) | 'created' | true + %i(preparing manual) | 'preparing' | false + end + + with_them do + let(:all_statuses) do + build_statuses.map { |status| @statuses_with_allow_failure[status] } + end + + it_behaves_like 'compares status and warnings' + end + end end end diff --git a/spec/migrations/20200811130433_create_missing_vulnerabilities_issue_links_spec.rb b/spec/migrations/20200811130433_create_missing_vulnerabilities_issue_links_spec.rb deleted file mode 100644 index 885f9c45072..00000000000 --- a/spec/migrations/20200811130433_create_missing_vulnerabilities_issue_links_spec.rb +++ /dev/null @@ -1,122 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' -require Rails.root.join('db', 'post_migrate', '20200811130433_create_missing_vulnerabilities_issue_links.rb') - -RSpec.describe CreateMissingVulnerabilitiesIssueLinks, :migration do - let(:namespace) { table(:namespaces).create!(name: 'user', path: 'user') } - let(:users) { table(:users) } - let(:user) { create_user! } - let(:project) { table(:projects).create!(id: 123, namespace_id: namespace.id) } - let(:scanner) { table(:vulnerability_scanners).create!(project_id: project.id, external_id: 'test', name: 'test scanner') } - let(:issue) { table(:issues).create!(id: 123, project_id: project.id) } - let(:vulnerabilities) { table(:vulnerabilities) } - let(:vulnerabilities_findings) { table(:vulnerability_occurrences) } - let(:vulnerability_feedback) { table(:vulnerability_feedback) } - let(:vulnerability_issue_links) { table(:vulnerability_issue_links) } - let(:vulnerability_identifier) { table(:vulnerability_identifiers).create!(project_id: project.id, external_type: 'test', external_id: 'test', fingerprint: 'test', name: 'test') } - - let!(:vulnerability) do - create_vulnerability!( - project_id: project.id, - author_id: user.id - ) - end - - before do - create_finding!( - vulnerability_id: vulnerability.id, - project_id: project.id, - scanner_id: scanner.id, - primary_identifier_id: vulnerability_identifier.id - ) - create_feedback!( - issue_id: issue.id, - project_id: project.id, - author_id: user.id - ) - end - - context 'with no Vulnerabilities::IssueLinks present' do - it 'creates missing Vulnerabilities::IssueLinks' do - expect(vulnerability_issue_links.count).to eq(0) - - migrate! - - expect(vulnerability_issue_links.count).to eq(1) - end - end - - context 'when an Vulnerabilities::IssueLink already exists' do - before do - vulnerability_issue_links.create!(vulnerability_id: vulnerability.id, issue_id: issue.id) - end - - it 'creates no duplicates' do - expect(vulnerability_issue_links.count).to eq(1) - - migrate! - - expect(vulnerability_issue_links.count).to eq(1) - end - end - - private - - def create_vulnerability!(project_id:, author_id:, title: 'test', severity: 7, confidence: 7, report_type: 0) - vulnerabilities.create!( - project_id: project_id, - author_id: author_id, - title: title, - severity: severity, - confidence: confidence, - report_type: report_type - ) - end - - # rubocop:disable Metrics/ParameterLists - def create_finding!( - vulnerability_id:, project_id:, scanner_id:, primary_identifier_id:, - name: "test", severity: 7, confidence: 7, report_type: 0, - project_fingerprint: '123qweasdzxc', location_fingerprint: 'test', - metadata_version: 'test', raw_metadata: 'test', uuid: 'test') - vulnerabilities_findings.create!( - vulnerability_id: vulnerability_id, - project_id: project_id, - name: name, - severity: severity, - confidence: confidence, - report_type: report_type, - project_fingerprint: project_fingerprint, - scanner_id: scanner.id, - primary_identifier_id: vulnerability_identifier.id, - location_fingerprint: location_fingerprint, - metadata_version: metadata_version, - raw_metadata: raw_metadata, - uuid: uuid - ) - end - # rubocop:enable Metrics/ParameterLists - - # project_fingerprint on Vulnerabilities::Finding is a bytea and we need to match this - def create_feedback!(issue_id:, project_id:, author_id:, feedback_type: 1, category: 0, project_fingerprint: '3132337177656173647a7863') - vulnerability_feedback.create!( - feedback_type: feedback_type, - issue_id: issue.id, - category: category, - project_fingerprint: project_fingerprint, - project_id: project_id, - author_id: author_id - ) - end - - def create_user!(name: "Example User", email: "user@example.com", user_type: nil, created_at: Time.now, confirmed_at: Time.now) - users.create!( - name: name, - email: email, - username: name, - projects_limit: 0, - user_type: user_type, - confirmed_at: confirmed_at - ) - end -end diff --git a/spec/models/analytics/cycle_analytics/project_stage_spec.rb b/spec/models/analytics/cycle_analytics/project_stage_spec.rb index 2e024011553..4675f037957 100644 --- a/spec/models/analytics/cycle_analytics/project_stage_spec.rb +++ b/spec/models/analytics/cycle_analytics/project_stage_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Analytics::CycleAnalytics::ProjectStage do context 'relative positioning' do it_behaves_like 'a class that supports relative positioning' do - let(:project) { build(:project) } + let_it_be(:project) { create(:project) } let(:factory) { :cycle_analytics_project_stage } let(:default_params) { { project: project } } end diff --git a/spec/models/ci/group_spec.rb b/spec/models/ci/group_spec.rb index dc9aee906ea..c20b7e61044 100644 --- a/spec/models/ci/group_spec.rb +++ b/spec/models/ci/group_spec.rb @@ -29,24 +29,8 @@ RSpec.describe Ci::Group do [create(:ci_build, :failed)] end - context 'when ci_composite_status is enabled' do - before do - stub_feature_flags(ci_composite_status: true) - end - - it 'returns a failed status' do - expect(subject.status).to eq('failed') - end - end - - context 'when ci_composite_status is disabled' do - before do - stub_feature_flags(ci_composite_status: false) - end - - it 'returns a failed status' do - expect(subject.status).to eq('failed') - end + it 'returns a failed status' do + expect(subject.status).to eq('failed') end end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 28626cdf13a..b4e80fa7588 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -939,69 +939,59 @@ RSpec.describe Ci::Pipeline, :mailer do subject { pipeline.legacy_stages } - where(:ci_composite_status) do - [false, true] + context 'stages list' do + it 'returns ordered list of stages' do + expect(subject.map(&:name)).to eq(%w[build test deploy]) + end end - with_them do - before do - stub_feature_flags(ci_composite_status: ci_composite_status) + context 'stages with statuses' do + let(:statuses) do + subject.map { |stage| [stage.name, stage.status] } end - context 'stages list' do - it 'returns ordered list of stages' do - expect(subject.map(&:name)).to eq(%w[build test deploy]) - end + it 'returns list of stages with correct statuses' do + expect(statuses).to eq([%w(build failed), + %w(test success), + %w(deploy running)]) end - context 'stages with statuses' do - let(:statuses) do - subject.map { |stage| [stage.name, stage.status] } + context 'when commit status is retried' do + before do + create(:commit_status, pipeline: pipeline, + stage: 'build', + name: 'mac', + stage_idx: 0, + status: 'success') + + Ci::ProcessPipelineService + .new(pipeline) + .execute end - it 'returns list of stages with correct statuses' do - expect(statuses).to eq([%w(build failed), + it 'ignores the previous state' do + expect(statuses).to eq([%w(build success), %w(test success), %w(deploy running)]) end - - context 'when commit status is retried' do - before do - create(:commit_status, pipeline: pipeline, - stage: 'build', - name: 'mac', - stage_idx: 0, - status: 'success') - - Ci::ProcessPipelineService - .new(pipeline) - .execute - end - - it 'ignores the previous state' do - expect(statuses).to eq([%w(build success), - %w(test success), - %w(deploy running)]) - end - end end + end - context 'when there is a stage with warnings' do - before do - create(:commit_status, pipeline: pipeline, - stage: 'deploy', - name: 'prod:2', - stage_idx: 2, - status: 'failed', - allow_failure: true) - end + context 'when there is a stage with warnings' do + before do + create(:commit_status, pipeline: pipeline, + stage: 'deploy', + name: 'prod:2', + stage_idx: 2, + status: 'failed', + allow_failure: true) + end - it 'populates stage with correct number of warnings' do - deploy_stage = pipeline.legacy_stages.third + it 'populates stage with correct number of warnings' do + deploy_stage = pipeline.legacy_stages.third - expect(deploy_stage).not_to receive(:statuses) - expect(deploy_stage).to have_warnings - end + expect(deploy_stage).not_to receive(:statuses) + expect(deploy_stage).to have_warnings end end end diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index c5bc7fb04ab..7f893d6a100 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -422,7 +422,7 @@ RSpec.describe CommitStatus do end it 'returns a correct compound status' do - expect(described_class.all.slow_composite_status(project: project)).to eq 'running' + expect(described_class.all.composite_status).to eq 'running' end end @@ -432,7 +432,7 @@ RSpec.describe CommitStatus do end it 'returns status that indicates success' do - expect(described_class.all.slow_composite_status(project: project)).to eq 'success' + expect(described_class.all.composite_status).to eq 'success' end end @@ -443,7 +443,7 @@ RSpec.describe CommitStatus do end it 'returns status according to the scope' do - expect(described_class.latest.slow_composite_status(project: project)).to eq 'success' + expect(described_class.latest.composite_status).to eq 'success' end end end diff --git a/spec/models/concerns/ci/has_status_spec.rb b/spec/models/concerns/ci/has_status_spec.rb index fe46b63781d..b16420bc658 100644 --- a/spec/models/concerns/ci/has_status_spec.rb +++ b/spec/models/concerns/ci/has_status_spec.rb @@ -3,10 +3,10 @@ require 'spec_helper' RSpec.describe Ci::HasStatus do - describe '.slow_composite_status' do + describe '.composite_status' do using RSpec::Parameterized::TableSyntax - subject { CommitStatus.slow_composite_status(project: nil) } + subject { CommitStatus.composite_status } shared_examples 'build status summary' do context 'all successful' do @@ -184,26 +184,16 @@ RSpec.describe Ci::HasStatus do end end - where(:ci_composite_status) do - [false, true] - end - - with_them do - before do - stub_feature_flags(ci_composite_status: ci_composite_status) - end + context 'ci build statuses' do + let(:type) { :ci_build } - context 'ci build statuses' do - let(:type) { :ci_build } - - it_behaves_like 'build status summary' - end + it_behaves_like 'build status summary' + end - context 'generic commit statuses' do - let(:type) { :generic_commit_status } + context 'generic commit statuses' do + let(:type) { :generic_commit_status } - it_behaves_like 'build status summary' - end + it_behaves_like 'build status summary' end end @@ -400,12 +390,4 @@ RSpec.describe Ci::HasStatus do end end end - - describe '.legacy_status_sql' do - subject { Ci::Build.legacy_status_sql } - - it 'returns SQL' do - puts subject - end - end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 586ef9da98e..59634524e74 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Issue do include ExternalAuthorizationServiceHelpers let_it_be(:user) { create(:user) } + let_it_be(:reusable_project) { create(:project) } describe "Associations" do it { is_expected.to belong_to(:milestone) } @@ -145,13 +146,13 @@ RSpec.describe Issue do end describe '#order_by_position_and_priority' do - let(:project) { create :project } + let(:project) { reusable_project } let(:p1) { create(:label, title: 'P1', project: project, priority: 1) } let(:p2) { create(:label, title: 'P2', project: project, priority: 2) } let!(:issue1) { create(:labeled_issue, project: project, labels: [p1]) } let!(:issue2) { create(:labeled_issue, project: project, labels: [p2]) } - let!(:issue3) { create(:issue, project: project, relative_position: 100) } - let!(:issue4) { create(:issue, project: project, relative_position: 200) } + let!(:issue3) { create(:issue, project: project, relative_position: -200) } + let!(:issue4) { create(:issue, project: project, relative_position: -100) } it 'returns ordered list' do expect(project.issues.order_by_position_and_priority) @@ -160,10 +161,10 @@ RSpec.describe Issue do end describe '#sort' do - let(:project) { create(:project) } + let(:project) { reusable_project } context "by relative_position" do - let!(:issue) { create(:issue, project: project) } + let!(:issue) { create(:issue, project: project, relative_position: nil) } let!(:issue2) { create(:issue, project: project, relative_position: 2) } let!(:issue3) { create(:issue, project: project, relative_position: 1) } @@ -1027,7 +1028,7 @@ RSpec.describe Issue do context "relative positioning" do it_behaves_like "a class that supports relative positioning" do - let(:project) { create(:project) } + let_it_be(:project) { create(:project) } let(:factory) { :issue } let(:default_params) { { project: project } } end diff --git a/spec/requests/api/group_milestones_spec.rb b/spec/requests/api/group_milestones_spec.rb index 2b361f2b503..7ed6e1a295f 100644 --- a/spec/requests/api/group_milestones_spec.rb +++ b/spec/requests/api/group_milestones_spec.rb @@ -3,15 +3,65 @@ require 'spec_helper' RSpec.describe API::GroupMilestones do - let(:user) { create(:user) } - let(:group) { create(:group, :private) } - let(:project) { create(:project, namespace: group) } - let!(:group_member) { create(:group_member, group: group, user: user) } - let!(:closed_milestone) { create(:closed_milestone, group: group, title: 'version1', description: 'closed milestone') } - let!(:milestone) { create(:milestone, group: group, title: 'version2', description: 'open milestone') } - - it_behaves_like 'group and project milestones', "/groups/:id/milestones" do - let(:route) { "/groups/#{group.id}/milestones" } + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group, :private) } + let_it_be(:project) { create(:project, namespace: group) } + let_it_be(:group_member) { create(:group_member, group: group, user: user) } + let_it_be(:closed_milestone) { create(:closed_milestone, group: group, title: 'version1', description: 'closed milestone') } + let_it_be(:milestone) { create(:milestone, group: group, title: 'version2', description: 'open milestone') } + let(:route) { "/groups/#{group.id}/milestones" } + + it_behaves_like 'group and project milestones', "/groups/:id/milestones" + + describe 'GET /groups/:id/milestones' do + context 'when include_parent_milestones is true' do + let_it_be(:ancestor_group) { create(:group, :private) } + let_it_be(:ancestor_group_milestone) { create(:milestone, group: ancestor_group) } + let_it_be(:params) { { include_parent_milestones: true } } + + before_all do + group.update(parent: ancestor_group) + end + + shared_examples 'listing all milestones' do + it 'returns correct list of milestones' do + get api(route, user), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(milestones.size) + expect(json_response.map { |entry| entry["id"] }).to eq(milestones.map(&:id)) + end + end + + context 'when user has access to ancestor groups' do + let(:milestones) { [ancestor_group_milestone, milestone, closed_milestone] } + + before do + ancestor_group.add_guest(user) + group.add_guest(user) + end + + it_behaves_like 'listing all milestones' + + context 'when iids param is present' do + let_it_be(:params) { { include_parent_milestones: true, iids: [milestone.iid] } } + + it_behaves_like 'listing all milestones' + end + end + + context 'when user has no access to ancestor groups' do + let(:user) { create(:user) } + + before do + group.add_guest(user) + end + + it_behaves_like 'listing all milestones' do + let(:milestones) { [milestone, closed_milestone] } + end + end + end end def setup_for_group diff --git a/spec/requests/api/project_milestones_spec.rb b/spec/requests/api/project_milestones_spec.rb index 30399866db6..d1e5df66b3f 100644 --- a/spec/requests/api/project_milestones_spec.rb +++ b/spec/requests/api/project_milestones_spec.rb @@ -3,17 +3,68 @@ require 'spec_helper' RSpec.describe API::ProjectMilestones do - let(:user) { create(:user) } - let!(:project) { create(:project, namespace: user.namespace ) } - let!(:closed_milestone) { create(:closed_milestone, project: project, title: 'version1', description: 'closed milestone') } - let!(:milestone) { create(:milestone, project: project, title: 'version2', description: 'open milestone') } + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, namespace: user.namespace ) } + let_it_be(:closed_milestone) { create(:closed_milestone, project: project, title: 'version1', description: 'closed milestone') } + let_it_be(:milestone) { create(:milestone, project: project, title: 'version2', description: 'open milestone') } + let_it_be(:route) { "/projects/#{project.id}/milestones" } before do project.add_developer(user) end - it_behaves_like 'group and project milestones', "/projects/:id/milestones" do - let(:route) { "/projects/#{project.id}/milestones" } + it_behaves_like 'group and project milestones', "/projects/:id/milestones" + + describe 'GET /projects/:id/milestones' do + context 'when include_parent_milestones is true' do + let_it_be(:ancestor_group) { create(:group, :private) } + let_it_be(:group) { create(:group, :private, parent: ancestor_group) } + let_it_be(:ancestor_group_milestone) { create(:milestone, group: ancestor_group) } + let_it_be(:group_milestone) { create(:milestone, group: group) } + let(:params) { { include_parent_milestones: true } } + + shared_examples 'listing all milestones' do + it 'returns correct list of milestones' do + get api(route, user), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(milestones.size) + expect(json_response.map { |entry| entry["id"] }).to eq(milestones.map(&:id)) + end + end + + context 'when project parent is a namespace' do + it_behaves_like 'listing all milestones' do + let(:milestones) { [milestone, closed_milestone] } + end + end + + context 'when project parent is a group' do + let(:milestones) { [group_milestone, ancestor_group_milestone, milestone, closed_milestone] } + + before_all do + project.update(namespace: group) + end + + it_behaves_like 'listing all milestones' + + context 'when iids param is present' do + let(:params) { { include_parent_milestones: true, iids: [group_milestone.iid] } } + + it_behaves_like 'listing all milestones' + end + + context 'when user is not a member of the private project' do + let(:external_user) { create(:user) } + + it 'returns a 404 error' do + get api(route, external_user), params: params + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + end end describe 'DELETE /projects/:id/milestones/:milestone_id' do diff --git a/spec/support/shared_examples/models/relative_positioning_shared_examples.rb b/spec/support/shared_examples/models/relative_positioning_shared_examples.rb index ab1467145a0..a96cb36d593 100644 --- a/spec/support/shared_examples/models/relative_positioning_shared_examples.rb +++ b/spec/support/shared_examples/models/relative_positioning_shared_examples.rb @@ -25,7 +25,6 @@ RSpec.shared_examples 'a class that supports relative positioning' do items = [item1, item2, item3] described_class.move_nulls_to_end(items) - items.map(&:reload) expect(items.sort_by(&:relative_position)).to eq(items) expect(item1.relative_position).to be(1000) @@ -35,22 +34,57 @@ RSpec.shared_examples 'a class that supports relative positioning' do expect(item3.next_relative_position).to be_nil end + it 'preserves relative position' do + item1.update!(relative_position: nil) + item2.update!(relative_position: nil) + + described_class.move_nulls_to_end([item1, item2]) + + expect(item1.relative_position).to be < item2.relative_position + end + it 'moves the item near the start position when there are no existing positions' do item1.update!(relative_position: nil) described_class.move_nulls_to_end([item1]) - item1.reload - - expect(item1.relative_position).to eq(described_class::START_POSITION + described_class::IDEAL_DISTANCE) + expect(item1.reset.relative_position).to eq(described_class::START_POSITION + described_class::IDEAL_DISTANCE) end it 'does not perform any moves if all items have their relative_position set' do item1.update!(relative_position: 1) + expect do + described_class.move_nulls_to_end([item1]) + end.not_to change { item1.reset.relative_position } + end + + it 'manages to move nulls to the end even if there is a sequence at the end' do + bunch = create_items_with_positions(run_at_end) + item1.update!(relative_position: nil) + described_class.move_nulls_to_end([item1]) - item1.reload - expect(item1.relative_position).to be(1) + items = [*bunch, item1] + items.each(&:reset) + + expect(items.map(&:relative_position)).to all(be_valid_position) + expect(items.sort_by(&:relative_position)).to eq(items) + end + + it 'does not have an N+1 issue' do + create_items_with_positions(10..12) + + a, b, c, d, e, f = create_items_with_positions([nil, nil, nil, nil, nil, nil]) + + baseline = ActiveRecord::QueryRecorder.new do + described_class.move_nulls_to_end([a, e]) + end + + expect { described_class.move_nulls_to_end([b, c, d]) } + .not_to exceed_query_limit(baseline) + + expect { described_class.move_nulls_to_end([f]) } + .not_to exceed_query_limit(baseline.count) end end @@ -78,11 +112,19 @@ RSpec.shared_examples 'a class that supports relative positioning' do item1.update!(relative_position: nil) described_class.move_nulls_to_start([item1]) - item1.reload expect(item1.relative_position).to eq(described_class::START_POSITION - described_class::IDEAL_DISTANCE) end + it 'preserves relative position' do + item1.update!(relative_position: nil) + item2.update!(relative_position: nil) + + described_class.move_nulls_to_start([item1, item2]) + + expect(item1.relative_position).to be < item2.relative_position + end + it 'does not perform any moves if all items have their relative_position set' do item1.update!(relative_position: 1) @@ -101,8 +143,8 @@ RSpec.shared_examples 'a class that supports relative positioning' do describe '#prev_relative_position' do it 'returns previous position if there is an item above' do - item1.update(relative_position: 5) - item2.update(relative_position: 15) + item1.update!(relative_position: 5) + item2.update!(relative_position: 15) expect(item2.prev_relative_position).to eq item1.relative_position end @@ -114,8 +156,8 @@ RSpec.shared_examples 'a class that supports relative positioning' do describe '#next_relative_position' do it 'returns next position if there is an item below' do - item1.update(relative_position: 5) - item2.update(relative_position: 15) + item1.update!(relative_position: 5) + item2.update!(relative_position: 15) expect(item1.next_relative_position).to eq item2.relative_position end @@ -125,9 +167,172 @@ RSpec.shared_examples 'a class that supports relative positioning' do end end + describe '#find_next_gap_before' do + context 'there is no gap' do + let(:items) { create_items_with_positions(run_at_start) } + + it 'returns nil' do + items.each do |item| + expect(item.send(:find_next_gap_before)).to be_nil + end + end + end + + context 'there is a sequence ending at MAX_POSITION' do + let(:items) { create_items_with_positions(run_at_end) } + + let(:gaps) do + items.map { |item| item.send(:find_next_gap_before) } + end + + it 'can find the gap at the start for any item in the sequence' do + gap = { start: items.first.relative_position, end: RelativePositioning::MIN_POSITION } + + expect(gaps).to all(eq(gap)) + end + + it 'respects lower bounds' do + gap = { start: items.first.relative_position, end: 10 } + new_item.update!(relative_position: 10) + + expect(gaps).to all(eq(gap)) + end + end + + specify do + item1.update!(relative_position: 5) + + (0..10).each do |pos| + item2.update!(relative_position: pos) + + gap = item2.send(:find_next_gap_before) + + expect(gap[:start]).to be <= item2.relative_position + expect((gap[:end] - gap[:start]).abs).to be >= RelativePositioning::MIN_GAP + expect(gap[:start]).to be_valid_position + expect(gap[:end]).to be_valid_position + end + end + + it 'deals with there not being any items to the left' do + create_items_with_positions([1, 2, 3]) + new_item.update!(relative_position: 0) + + expect(new_item.send(:find_next_gap_before)).to eq(start: 0, end: RelativePositioning::MIN_POSITION) + end + + it 'finds the next gap to the left, skipping adjacent values' do + create_items_with_positions([1, 9, 10]) + new_item.update!(relative_position: 11) + + expect(new_item.send(:find_next_gap_before)).to eq(start: 9, end: 1) + end + + it 'finds the next gap to the left' do + create_items_with_positions([2, 10]) + + new_item.update!(relative_position: 15) + expect(new_item.send(:find_next_gap_before)).to eq(start: 15, end: 10) + + new_item.update!(relative_position: 11) + expect(new_item.send(:find_next_gap_before)).to eq(start: 10, end: 2) + + new_item.update!(relative_position: 9) + expect(new_item.send(:find_next_gap_before)).to eq(start: 9, end: 2) + + new_item.update!(relative_position: 5) + expect(new_item.send(:find_next_gap_before)).to eq(start: 5, end: 2) + end + end + + describe '#find_next_gap_after' do + context 'there is no gap' do + let(:items) { create_items_with_positions(run_at_end) } + + it 'returns nil' do + items.each do |item| + expect(item.send(:find_next_gap_after)).to be_nil + end + end + end + + context 'there is a sequence starting at MIN_POSITION' do + let(:items) { create_items_with_positions(run_at_start) } + + let(:gaps) do + items.map { |item| item.send(:find_next_gap_after) } + end + + it 'can find the gap at the end for any item in the sequence' do + gap = { start: items.last.relative_position, end: RelativePositioning::MAX_POSITION } + + expect(gaps).to all(eq(gap)) + end + + it 'respects upper bounds' do + gap = { start: items.last.relative_position, end: 10 } + new_item.update!(relative_position: 10) + + expect(gaps).to all(eq(gap)) + end + end + + specify do + item1.update!(relative_position: 5) + + (0..10).each do |pos| + item2.update!(relative_position: pos) + + gap = item2.send(:find_next_gap_after) + + expect(gap[:start]).to be >= item2.relative_position + expect((gap[:end] - gap[:start]).abs).to be >= RelativePositioning::MIN_GAP + expect(gap[:start]).to be_valid_position + expect(gap[:end]).to be_valid_position + end + end + + it 'deals with there not being any items to the right' do + create_items_with_positions([1, 2, 3]) + new_item.update!(relative_position: 5) + + expect(new_item.send(:find_next_gap_after)).to eq(start: 5, end: RelativePositioning::MAX_POSITION) + end + + it 'finds the next gap to the right, skipping adjacent values' do + create_items_with_positions([1, 2, 10]) + new_item.update!(relative_position: 0) + + expect(new_item.send(:find_next_gap_after)).to eq(start: 2, end: 10) + end + + it 'finds the next gap to the right' do + create_items_with_positions([2, 10]) + + new_item.update!(relative_position: 0) + expect(new_item.send(:find_next_gap_after)).to eq(start: 0, end: 2) + + new_item.update!(relative_position: 1) + expect(new_item.send(:find_next_gap_after)).to eq(start: 2, end: 10) + + new_item.update!(relative_position: 3) + expect(new_item.send(:find_next_gap_after)).to eq(start: 3, end: 10) + + new_item.update!(relative_position: 5) + expect(new_item.send(:find_next_gap_after)).to eq(start: 5, end: 10) + end + end + describe '#move_before' do + let(:item3) { create(factory, default_params) } + it 'moves item before' do - [item2, item1].each(&:move_to_end) + [item2, item1].each do |item| + item.move_to_end + item.save! + end + + expect(item1.relative_position).to be > item2.relative_position item1.move_before(item2) @@ -135,12 +340,10 @@ RSpec.shared_examples 'a class that supports relative positioning' do end context 'when there is no space' do - let(:item3) { create(factory, default_params) } - before do - item1.update(relative_position: 1000) - item2.update(relative_position: 1001) - item3.update(relative_position: 1002) + item1.update!(relative_position: 1000) + item2.update!(relative_position: 1001) + item3.update!(relative_position: 1002) end it 'moves items correctly' do @@ -149,6 +352,73 @@ RSpec.shared_examples 'a class that supports relative positioning' do expect(item3.relative_position).to be_between(item1.reload.relative_position, item2.reload.relative_position).exclusive end end + + it 'can move the item before an item at the start' do + item1.update!(relative_position: RelativePositioning::START_POSITION) + + new_item.move_before(item1) + + expect(new_item.relative_position).to be_valid_position + expect(new_item.relative_position).to be < item1.reload.relative_position + end + + it 'can move the item before an item at MIN_POSITION' do + item1.update!(relative_position: RelativePositioning::MIN_POSITION) + + new_item.move_before(item1) + + expect(new_item.relative_position).to be >= RelativePositioning::MIN_POSITION + expect(new_item.relative_position).to be < item1.reload.relative_position + end + + it 'can move the item before an item bunched up at MIN_POSITION' do + item1, item2, item3 = create_items_with_positions(run_at_start) + + new_item.move_before(item3) + new_item.save! + + items = [item1, item2, new_item, item3] + + items.each do |item| + expect(item.reset.relative_position).to be_valid_position + end + + expect(items.sort_by(&:relative_position)).to eq(items) + end + + context 'leap-frogging to the left' do + before do + start = RelativePositioning::START_POSITION + item1.update!(relative_position: start - RelativePositioning::IDEAL_DISTANCE * 0) + item2.update!(relative_position: start - RelativePositioning::IDEAL_DISTANCE * 1) + item3.update!(relative_position: start - RelativePositioning::IDEAL_DISTANCE * 2) + end + + let(:item3) { create(factory, default_params) } + + def leap_frog(steps) + a = item1 + b = item2 + + steps.times do |i| + a.move_before(b) + a.save! + a, b = b, a + end + end + + it 'can leap-frog STEPS - 1 times before needing to rebalance' do + # This is less efficient than going right, due to the flooring of + # integer division + expect { leap_frog(RelativePositioning::STEPS - 1) } + .not_to change { item3.reload.relative_position } + end + + it 'rebalances after leap-frogging STEPS times' do + expect { leap_frog(RelativePositioning::STEPS) } + .to change { item3.reload.relative_position } + end + end end describe '#move_after' do @@ -164,9 +434,17 @@ RSpec.shared_examples 'a class that supports relative positioning' do let(:item3) { create(factory, default_params) } before do - item1.update(relative_position: 1000) - item2.update(relative_position: 1001) - item3.update(relative_position: 1002) + item1.update!(relative_position: 1000) + item2.update!(relative_position: 1001) + item3.update!(relative_position: 1002) + end + + it 'can move the item after an item at MAX_POSITION' do + item1.update!(relative_position: RelativePositioning::MAX_POSITION) + + new_item.move_after(item1) + expect(new_item.relative_position).to be_valid_position + expect(new_item.relative_position).to be > item1.reset.relative_position end it 'moves items correctly' do @@ -175,12 +453,96 @@ RSpec.shared_examples 'a class that supports relative positioning' do expect(item1.relative_position).to be_between(item2.reload.relative_position, item3.reload.relative_position).exclusive end end + + it 'can move the item after an item bunched up at MAX_POSITION' do + item1, item2, item3 = create_items_with_positions(run_at_end) + + new_item.move_after(item1) + new_item.save! + + items = [item1, new_item, item2, item3] + + items.each do |item| + expect(item.reset.relative_position).to be_valid_position + end + + expect(items.sort_by(&:relative_position)).to eq(items) + end + + context 'leap-frogging' do + before do + start = RelativePositioning::START_POSITION + item1.update!(relative_position: start + RelativePositioning::IDEAL_DISTANCE * 0) + item2.update!(relative_position: start + RelativePositioning::IDEAL_DISTANCE * 1) + item3.update!(relative_position: start + RelativePositioning::IDEAL_DISTANCE * 2) + end + + let(:item3) { create(factory, default_params) } + + def leap_frog(steps) + a = item1 + b = item2 + + steps.times do |i| + a.move_after(b) + a.save! + a, b = b, a + end + end + + it 'can leap-frog STEPS times before needing to rebalance' do + expect { leap_frog(RelativePositioning::STEPS) } + .not_to change { item3.reload.relative_position } + end + + it 'rebalances after leap-frogging STEPS+1 times' do + expect { leap_frog(RelativePositioning::STEPS + 1) } + .to change { item3.reload.relative_position } + end + end + end + + describe '#move_to_start' do + before do + [item1, item2].each do |item1| + item1.move_to_start && item1.save! + end + end + + it 'moves item to the end' do + new_item.move_to_start + + expect(new_item.relative_position).to be < item2.relative_position + end + + it 'rebalances when there is already an item at the MIN_POSITION' do + item2.update!(relative_position: RelativePositioning::MIN_POSITION) + + new_item.move_to_start + item2.reset + + expect(new_item.relative_position).to be < item2.relative_position + expect(new_item.relative_position).to be >= RelativePositioning::MIN_POSITION + end + + it 'deals with a run of elements at the start' do + item1.update!(relative_position: RelativePositioning::MIN_POSITION + 1) + item2.update!(relative_position: RelativePositioning::MIN_POSITION) + + new_item.move_to_start + item1.reset + item2.reset + + expect(item2.relative_position).to be < item1.relative_position + expect(new_item.relative_position).to be < item2.relative_position + expect(new_item.relative_position).to be >= RelativePositioning::MIN_POSITION + end end describe '#move_to_end' do before do [item1, item2].each do |item1| - item1.move_to_end && item1.save + item1.move_to_end && item1.save! end end @@ -189,12 +551,44 @@ RSpec.shared_examples 'a class that supports relative positioning' do expect(new_item.relative_position).to be > item2.relative_position end + + it 'rebalances when there is already an item at the MAX_POSITION' do + item2.update!(relative_position: RelativePositioning::MAX_POSITION) + + new_item.move_to_end + item2.reset + + expect(new_item.relative_position).to be > item2.relative_position + expect(new_item.relative_position).to be <= RelativePositioning::MAX_POSITION + end + + it 'deals with a run of elements at the end' do + item1.update!(relative_position: RelativePositioning::MAX_POSITION - 1) + item2.update!(relative_position: RelativePositioning::MAX_POSITION) + + new_item.move_to_end + item1.reset + item2.reset + + expect(item2.relative_position).to be > item1.relative_position + expect(new_item.relative_position).to be > item2.relative_position + expect(new_item.relative_position).to be <= RelativePositioning::MAX_POSITION + end end describe '#move_between' do before do - [item1, item2].each do |item1| - item1.move_to_end && item1.save + [item1, item2].each do |item| + item.move_to_end && item.save! + end + end + + shared_examples 'moves item between' do + it 'moves the middle item to between left and right' do + expect do + middle.move_between(left, right) + middle.save! + end.to change { between_exclusive?(left, middle, right) }.from(false).to(true) end end @@ -218,26 +612,26 @@ RSpec.shared_examples 'a class that supports relative positioning' do end it 'positions items even when after and before positions are the same' do - item2.update relative_position: item1.relative_position + item2.update! relative_position: item1.relative_position new_item.move_between(item1, item2) + [item1, item2].each(&:reset) expect(new_item.relative_position).to be > item1.relative_position expect(item1.relative_position).to be < item2.relative_position end - it 'positions items between other two if distance is 1' do - item2.update relative_position: item1.relative_position + 1 - - new_item.move_between(item1, item2) + context 'the two items are next to each other' do + let(:left) { item1 } + let(:middle) { new_item } + let(:right) { create_item(relative_position: item1.relative_position + 1) } - expect(new_item.relative_position).to be > item1.relative_position - expect(item1.relative_position).to be < item2.relative_position + it_behaves_like 'moves item between' end it 'positions item in the middle of other two if distance is big enough' do - item1.update relative_position: 6000 - item2.update relative_position: 10000 + item1.update! relative_position: 6000 + item2.update! relative_position: 10000 new_item.move_between(item1, item2) @@ -245,7 +639,8 @@ RSpec.shared_examples 'a class that supports relative positioning' do end it 'positions item closer to the middle if we are at the very top' do - item2.update relative_position: 6000 + item1.update!(relative_position: 6001) + item2.update!(relative_position: 6000) new_item.move_between(nil, item2) @@ -253,51 +648,53 @@ RSpec.shared_examples 'a class that supports relative positioning' do end it 'positions item closer to the middle if we are at the very bottom' do - new_item.update relative_position: 1 - item1.update relative_position: 6000 - item2.destroy + new_item.update!(relative_position: 1) + item1.update!(relative_position: 6000) + item2.update!(relative_position: 5999) new_item.move_between(item1, nil) expect(new_item.relative_position).to eq(6000 + RelativePositioning::IDEAL_DISTANCE) end - it 'positions item in the middle of other two if distance is not big enough' do - item1.update relative_position: 100 - item2.update relative_position: 400 + it 'positions item in the middle of other two' do + item1.update! relative_position: 100 + item2.update! relative_position: 400 new_item.move_between(item1, item2) expect(new_item.relative_position).to eq(250) end - it 'positions item in the middle of other two is there is no place' do - item1.update relative_position: 100 - item2.update relative_position: 101 - - new_item.move_between(item1, item2) + context 'there is no space' do + let(:middle) { new_item } + let(:left) { create_item(relative_position: 100) } + let(:right) { create_item(relative_position: 101) } - expect(new_item.relative_position).to be_between(item1.relative_position, item2.relative_position).exclusive + it_behaves_like 'moves item between' end - it 'uses rebalancing if there is no place' do - item1.update relative_position: 100 - item2.update relative_position: 101 - item3 = create_item(relative_position: 102) - new_item.update relative_position: 103 + context 'there is a bunch of items' do + let(:items) { create_items_with_positions(100..104) } + let(:left) { items[1] } + let(:middle) { items[3] } + let(:right) { items[2] } - new_item.move_between(item2, item3) - new_item.save! + it_behaves_like 'moves item between' - expect(new_item.relative_position).to be_between(item2.relative_position, item3.relative_position).exclusive - expect(item1.reload.relative_position).not_to eq(100) + it 'handles bunches correctly' do + middle.move_between(left, right) + middle.save! + + expect(items.first.reset.relative_position).to be < middle.relative_position + end end - it 'positions item right if we pass none-sequential parameters' do - item1.update relative_position: 99 - item2.update relative_position: 101 + it 'positions item right if we pass non-sequential parameters' do + item1.update! relative_position: 99 + item2.update! relative_position: 101 item3 = create_item(relative_position: 102) - new_item.update relative_position: 103 + new_item.update! relative_position: 103 new_item.move_between(item1, item3) new_item.save! @@ -329,6 +726,12 @@ RSpec.shared_examples 'a class that supports relative positioning' do expect(positions).to eq([90, 95, 96, 102]) end + it 'raises an error if there is no space' do + items = create_items_with_positions(run_at_start) + + expect { items.last.move_sequence_before }.to raise_error(RelativePositioning::NoSpaceLeft) + end + it 'finds a gap if there are unused positions' do items = create_items_with_positions([100, 101, 102]) @@ -336,7 +739,8 @@ RSpec.shared_examples 'a class that supports relative positioning' do items.last.save! positions = items.map { |item| item.reload.relative_position } - expect(positions).to eq([50, 51, 102]) + + expect(positions.last - positions.second).to be > RelativePositioning::MIN_GAP end end @@ -358,7 +762,33 @@ RSpec.shared_examples 'a class that supports relative positioning' do items.first.save! positions = items.map { |item| item.reload.relative_position } - expect(positions).to eq([100, 601, 602]) + expect(positions.second - positions.first).to be > RelativePositioning::MIN_GAP + end + + it 'raises an error if there is no space' do + items = create_items_with_positions(run_at_end) + + expect { items.first.move_sequence_after }.to raise_error(RelativePositioning::NoSpaceLeft) end end + + def be_valid_position + be_between(RelativePositioning::MIN_POSITION, RelativePositioning::MAX_POSITION) + end + + def between_exclusive?(left, middle, right) + a, b, c = [left, middle, right].map { |item| item.reset.relative_position } + return false if a.nil? || b.nil? + return a < b if c.nil? + + a < b && b < c + end + + def run_at_end(size = 3) + (RelativePositioning::MAX_POSITION - size)..RelativePositioning::MAX_POSITION + end + + def run_at_start(size = 3) + (RelativePositioning::MIN_POSITION..).take(size) + end end |