diff options
94 files changed, 1059 insertions, 617 deletions
diff --git a/app/assets/javascripts/boards/components/boards_selector.vue b/app/assets/javascripts/boards/components/boards_selector.vue index 69343cd78d8..6dbb1ea0050 100644 --- a/app/assets/javascripts/boards/components/boards_selector.vue +++ b/app/assets/javascripts/boards/components/boards_selector.vue @@ -14,8 +14,6 @@ import { mapActions, mapGetters, mapState } from 'vuex'; import BoardForm from 'ee_else_ce/boards/components/board_form.vue'; import { getIdFromGraphQLId } from '~/graphql_shared/utils'; -import axios from '~/lib/utils/axios_utils'; -import httpStatusCodes from '~/lib/utils/http_status'; import { s__ } from '~/locale'; import eventHub from '../eventhub'; @@ -23,6 +21,8 @@ import groupBoardsQuery from '../graphql/group_boards.query.graphql'; import projectBoardsQuery from '../graphql/project_boards.query.graphql'; import groupBoardQuery from '../graphql/group_board.query.graphql'; import projectBoardQuery from '../graphql/project_board.query.graphql'; +import groupRecentBoardsQuery from '../graphql/group_recent_boards.query.graphql'; +import projectRecentBoardsQuery from '../graphql/project_recent_boards.query.graphql'; const MIN_BOARDS_TO_VIEW_RECENT = 10; @@ -40,7 +40,7 @@ export default { directives: { GlModalDirective, }, - inject: ['fullPath', 'recentBoardsEndpoint'], + inject: ['fullPath'], props: { throttleDuration: { type: Number, @@ -158,6 +158,10 @@ export default { this.scrollFadeInitialized = false; this.$nextTick(this.setScrollFade); }, + recentBoards() { + this.scrollFadeInitialized = false; + this.$nextTick(this.setScrollFade); + }, }, created() { eventHub.$on('showBoardModal', this.showPage); @@ -173,11 +177,11 @@ export default { cancel() { this.showPage(''); }, - boardUpdate(data) { + boardUpdate(data, boardType) { if (!data?.[this.parentType]) { return []; } - return data[this.parentType].boards.edges.map(({ node }) => ({ + return data[this.parentType][boardType].edges.map(({ node }) => ({ id: getIdFromGraphQLId(node.id), name: node.name, })); @@ -185,6 +189,9 @@ export default { boardQuery() { return this.isGroupBoard ? groupBoardsQuery : projectBoardsQuery; }, + recentBoardsQuery() { + return this.isGroupBoard ? groupRecentBoardsQuery : projectRecentBoardsQuery; + }, loadBoards(toggleDropdown = true) { if (toggleDropdown && this.boards.length > 0) { return; @@ -196,39 +203,20 @@ export default { }, query: this.boardQuery, loadingKey: 'loadingBoards', - update: this.boardUpdate, + update: (data) => this.boardUpdate(data, 'boards'), }); this.loadRecentBoards(); }, loadRecentBoards() { - this.loadingRecentBoards = true; - // Follow up to fetch recent boards using GraphQL - // https://gitlab.com/gitlab-org/gitlab/-/issues/300985 - axios - .get(this.recentBoardsEndpoint) - .then((res) => { - this.recentBoards = res.data; - }) - .catch((err) => { - /** - * If user is unauthorized we'd still want to resolve the - * request to display all boards. - */ - if (err?.response?.status === httpStatusCodes.UNAUTHORIZED) { - this.recentBoards = []; // recent boards are empty - return; - } - throw err; - }) - .then(() => this.$nextTick()) // Wait for boards list in DOM - .then(() => { - this.setScrollFade(); - }) - .catch(() => {}) - .finally(() => { - this.loadingRecentBoards = false; - }); + this.$apollo.addSmartQuery('recentBoards', { + variables() { + return { fullPath: this.fullPath }; + }, + query: this.recentBoardsQuery, + loadingKey: 'loadingRecentBoards', + update: (data) => this.boardUpdate(data, 'recentIssueBoards'), + }); }, isScrolledUp() { const { content } = this.$refs; diff --git a/app/assets/javascripts/boards/graphql/group_recent_boards.query.graphql b/app/assets/javascripts/boards/graphql/group_recent_boards.query.graphql new file mode 100644 index 00000000000..827c08486b1 --- /dev/null +++ b/app/assets/javascripts/boards/graphql/group_recent_boards.query.graphql @@ -0,0 +1,14 @@ +#import "ee_else_ce/boards/graphql/board.fragment.graphql" + +query group_recent_boards($fullPath: ID!) { + group(fullPath: $fullPath) { + id + recentIssueBoards { + edges { + node { + ...BoardFragment + } + } + } + } +} diff --git a/app/assets/javascripts/boards/graphql/project_recent_boards.query.graphql b/app/assets/javascripts/boards/graphql/project_recent_boards.query.graphql new file mode 100644 index 00000000000..4d38e9b0498 --- /dev/null +++ b/app/assets/javascripts/boards/graphql/project_recent_boards.query.graphql @@ -0,0 +1,14 @@ +#import "ee_else_ce/boards/graphql/board.fragment.graphql" + +query project_recent_boards($fullPath: ID!) { + project(fullPath: $fullPath) { + id + recentIssueBoards { + edges { + node { + ...BoardFragment + } + } + } + } +} diff --git a/app/assets/javascripts/boards/index.js b/app/assets/javascripts/boards/index.js index ded3bfded86..9f44380781e 100644 --- a/app/assets/javascripts/boards/index.js +++ b/app/assets/javascripts/boards/index.js @@ -144,7 +144,6 @@ export default () => { mountMultipleBoardsSwitcher({ fullPath: $boardApp.dataset.fullPath, rootPath: $boardApp.dataset.boardsEndpoint, - recentBoardsEndpoint: $boardApp.dataset.recentBoardsEndpoint, allowScopedLabels: $boardApp.dataset.scopedLabels, labelsManagePath: $boardApp.dataset.labelsManagePath, }); diff --git a/app/assets/javascripts/boards/mount_multiple_boards_switcher.js b/app/assets/javascripts/boards/mount_multiple_boards_switcher.js index 26dd8b99f98..3838b4f2a83 100644 --- a/app/assets/javascripts/boards/mount_multiple_boards_switcher.js +++ b/app/assets/javascripts/boards/mount_multiple_boards_switcher.js @@ -24,7 +24,6 @@ export default (params = {}) => { provide: { fullPath: params.fullPath, rootPath: params.rootPath, - recentBoardsEndpoint: params.recentBoardsEndpoint, allowScopedLabels: params.allowScopedLabels, labelsManagePath: params.labelsManagePath, allowLabelCreate: parseBoolean(dataset.canAdminBoard), diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index a8ca17ab4dd..5707e4d67f9 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -53,6 +53,7 @@ import DiffFile from './diff_file.vue'; import HiddenFilesWarning from './hidden_files_warning.vue'; import NoChanges from './no_changes.vue'; import TreeList from './tree_list.vue'; +import VirtualScrollerScrollSync from './virtual_scroller_scroll_sync'; export default { name: 'DiffsApp', @@ -62,8 +63,7 @@ export default { DynamicScrollerItem: () => import('vendor/vue-virtual-scroller').then(({ DynamicScrollerItem }) => DynamicScrollerItem), PreRenderer: () => import('./pre_renderer.vue').then((PreRenderer) => PreRenderer), - VirtualScrollerScrollSync: () => - import('./virtual_scroller_scroll_sync').then((VSSSync) => VSSSync), + VirtualScrollerScrollSync, CompareVersions, DiffFile, NoChanges, @@ -406,10 +406,8 @@ export default { this.unsubscribeFromEvents(); this.removeEventListeners(); - if (window.gon?.features?.diffsVirtualScrolling) { - diffsEventHub.$off('scrollToFileHash', this.scrollVirtualScrollerToFileHash); - diffsEventHub.$off('scrollToIndex', this.scrollVirtualScrollerToIndex); - } + diffsEventHub.$off('scrollToFileHash', this.scrollVirtualScrollerToFileHash); + diffsEventHub.$off('scrollToIndex', this.scrollVirtualScrollerToIndex); }, methods: { ...mapActions(['startTaskList']), @@ -522,32 +520,27 @@ export default { ); } - if ( - window.gon?.features?.diffsVirtualScrolling || - window.gon?.features?.usageDataDiffSearches - ) { - let keydownTime; - Mousetrap.bind(['mod+f', 'mod+g'], () => { - keydownTime = new Date().getTime(); - }); + let keydownTime; + Mousetrap.bind(['mod+f', 'mod+g'], () => { + keydownTime = new Date().getTime(); + }); - window.addEventListener('blur', () => { - if (keydownTime) { - const delta = new Date().getTime() - keydownTime; + window.addEventListener('blur', () => { + if (keydownTime) { + const delta = new Date().getTime() - keydownTime; - // To make sure the user is using the find function we need to wait for blur - // and max 1000ms to be sure it the search box is filtered - if (delta >= 0 && delta < 1000) { - this.disableVirtualScroller(); + // To make sure the user is using the find function we need to wait for blur + // and max 1000ms to be sure it the search box is filtered + if (delta >= 0 && delta < 1000) { + this.disableVirtualScroller(); - if (window.gon?.features?.usageDataDiffSearches) { - api.trackRedisHllUserEvent('i_code_review_user_searches_diff'); - api.trackRedisCounterEvent('diff_searches'); - } + if (window.gon?.features?.usageDataDiffSearches) { + api.trackRedisHllUserEvent('i_code_review_user_searches_diff'); + api.trackRedisCounterEvent('diff_searches'); } } - }); - } + } + }); }, removeEventListeners() { Mousetrap.unbind(keysFor(MR_PREVIOUS_FILE_IN_DIFF)); @@ -589,8 +582,6 @@ export default { this.virtualScrollCurrentIndex = -1; }, scrollVirtualScrollerToDiffNote() { - if (!window.gon?.features?.diffsVirtualScrolling) return; - const id = window?.location?.hash; if (id.startsWith('#note_')) { @@ -605,11 +596,7 @@ export default { } }, subscribeToVirtualScrollingEvents() { - if ( - window.gon?.features?.diffsVirtualScrolling && - this.shouldShow && - !this.subscribedToVirtualScrollingEvents - ) { + if (this.shouldShow && !this.subscribedToVirtualScrollingEvents) { diffsEventHub.$on('scrollToFileHash', this.scrollVirtualScrollerToFileHash); diffsEventHub.$on('scrollToIndex', this.scrollVirtualScrollerToIndex); diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue index 0e8506cd896..3cf1f69b08c 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -209,7 +209,7 @@ export default { handler(val) { const el = this.$el.closest('.vue-recycle-scroller__item-view'); - if (this.glFeatures.diffsVirtualScrolling && el) { + if (el) { // We can't add a style with Vue because of the way the virtual // scroller library renders the diff files el.style.zIndex = val ? '1' : null; diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index c2eefad8f40..bbe27c0dbd6 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -29,8 +29,6 @@ export const UNFOLD_COUNT = 20; export const COUNT_OF_AVATARS_IN_GUTTER = 3; export const LENGTH_OF_AVATAR_TOOLTIP = 17; -export const LINES_TO_BE_RENDERED_DIRECTLY = 100; - export const DIFF_FILE_SYMLINK_MODE = '120000'; export const DIFF_FILE_DELETED_MODE = '0'; diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js index dfe29e767c9..1691da34c6d 100644 --- a/app/assets/javascripts/diffs/index.js +++ b/app/assets/javascripts/diffs/index.js @@ -1,8 +1,7 @@ import Vue from 'vue'; import { mapActions, mapState, mapGetters } from 'vuex'; -import { getCookie, setCookie, parseBoolean, removeCookie } from '~/lib/utils/common_utils'; +import { getCookie, parseBoolean, removeCookie } from '~/lib/utils/common_utils'; -import { getParameterValues } from '~/lib/utils/url_utility'; import eventHub from '../notes/event_hub'; import diffsApp from './components/app.vue'; @@ -74,11 +73,6 @@ export default function initDiffsApp(store) { trackClick: false, }); } - - const vScrollingParam = getParameterValues('virtual_scrolling')[0]; - if (vScrollingParam === 'false' || vScrollingParam === 'true') { - setCookie('diffs_virtual_scrolling', vScrollingParam); - } }, methods: { ...mapActions('diffs', ['setRenderTreeList', 'setShowWhitespace']), diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 572949e585a..e967be23f42 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -125,7 +125,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => { commit(types.SET_DIFF_DATA_BATCH, { diff_files }); commit(types.SET_BATCH_LOADING_STATE, 'loaded'); - if (window.gon?.features?.diffsVirtualScrolling && !scrolledVirtualScroller) { + if (!scrolledVirtualScroller) { const index = state.diffFiles.findIndex( (f) => f.file_hash === hash || f[INLINE_DIFF_LINES_KEY].find((l) => l.line_code === hash), @@ -195,9 +195,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => { commit(types.SET_BATCH_LOADING_STATE, 'error'); }); - return getBatch().then( - () => !window.gon?.features?.diffsVirtualScrolling && handleLocationHash(), - ); + return getBatch(); }; export const fetchDiffFilesMeta = ({ commit, state }) => { @@ -529,7 +527,7 @@ export const setCurrentFileHash = ({ commit }, hash) => { commit(types.SET_CURRENT_DIFF_FILE, hash); }; -export const scrollToFile = ({ state, commit, getters }, { path, setHash = true }) => { +export const scrollToFile = ({ state, commit, getters }, { path }) => { if (!state.treeEntries[path]) return; const { fileHash } = state.treeEntries[path]; @@ -539,11 +537,9 @@ export const scrollToFile = ({ state, commit, getters }, { path, setHash = true if (getters.isVirtualScrollingEnabled) { eventHub.$emit('scrollToFileHash', fileHash); - if (setHash) { - setTimeout(() => { - window.history.replaceState(null, null, `#${fileHash}`); - }); - } + setTimeout(() => { + window.history.replaceState(null, null, `#${fileHash}`); + }); } else { document.location.hash = fileHash; diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index 83c7f8c814b..3a85c1a9fe1 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -1,6 +1,5 @@ -import { getCookie } from '~/lib/utils/common_utils'; -import { getParameterValues } from '~/lib/utils/url_utility'; import { __, n__ } from '~/locale'; +import { getParameterValues } from '~/lib/utils/url_utility'; import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE, @@ -175,21 +174,11 @@ export function suggestionCommitMessage(state, _, rootState) { } export const isVirtualScrollingEnabled = (state) => { - const vSrollerCookie = getCookie('diffs_virtual_scrolling'); - - if (state.disableVirtualScroller) { + if (state.disableVirtualScroller || getParameterValues('virtual_scrolling')[0] === 'false') { return false; } - if (vSrollerCookie) { - return vSrollerCookie === 'true'; - } - - return ( - !state.viewDiffsFileByFile && - (window.gon?.features?.diffsVirtualScrolling || - getParameterValues('virtual_scrolling')[0] === 'true') - ); + return !state.viewDiffsFileByFile; }; export const isBatchLoading = (state) => state.batchLoadingState === 'loading'; diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 3f1af68e37a..f2028892a5f 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -9,7 +9,6 @@ import { NEW_LINE_TYPE, OLD_LINE_TYPE, MATCH_LINE_TYPE, - LINES_TO_BE_RENDERED_DIRECTLY, INLINE_DIFF_LINES_KEY, CONFLICT_OUR, CONFLICT_THEIR, @@ -380,16 +379,9 @@ function prepareDiffFileLines(file) { return file; } -function finalizeDiffFile(file, index) { - let renderIt = Boolean(window.gon?.features?.diffsVirtualScrolling); - - if (!window.gon?.features?.diffsVirtualScrolling) { - renderIt = - index < 3 ? file[INLINE_DIFF_LINES_KEY].length < LINES_TO_BE_RENDERED_DIRECTLY : false; - } - +function finalizeDiffFile(file) { Object.assign(file, { - renderIt, + renderIt: true, isShowingFullFile: false, isLoadingFullFile: false, discussions: [], @@ -417,15 +409,13 @@ export function prepareDiffData({ diff, priorFiles = [], meta = false }) { .map((file, index, allFiles) => prepareRawDiffFile({ file, allFiles, meta })) .map(ensureBasicDiffFileLines) .map(prepareDiffFileLines) - .map((file, index) => finalizeDiffFile(file, priorFiles.length + index)); + .map((file) => finalizeDiffFile(file)); return deduplicateFilesList([...priorFiles, ...cleanedFiles]); } export function getDiffPositionByLineCode(diffFiles) { - let lines = []; - - lines = diffFiles.reduce((acc, diffFile) => { + const lines = diffFiles.reduce((acc, diffFile) => { diffFile[INLINE_DIFF_LINES_KEY].forEach((line) => { acc.push({ file: diffFile, line }); }); diff --git a/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js b/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js index d00e6e59cf5..28a3c54cc8f 100644 --- a/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js +++ b/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js @@ -13,6 +13,21 @@ export default (IssuableTokenKeys, disableTargetBranchFilter = false) => { IssuableTokenKeys.tokenKeys.splice(2, 0, reviewerToken); IssuableTokenKeys.tokenKeysWithAlternative.splice(2, 0, reviewerToken); + if (window.gon?.features?.mrAttentionRequests) { + const attentionRequestedToken = { + formattedKey: __('Attention'), + key: 'attention', + type: 'string', + param: '', + symbol: '@', + icon: 'user', + tag: '@attention', + hideNotEqual: true, + }; + IssuableTokenKeys.tokenKeys.splice(2, 0, attentionRequestedToken); + IssuableTokenKeys.tokenKeysWithAlternative.splice(2, 0, attentionRequestedToken); + } + const draftToken = { token: { formattedKey: __('Draft'), diff --git a/app/assets/javascripts/filtered_search/available_dropdown_mappings.js b/app/assets/javascripts/filtered_search/available_dropdown_mappings.js index 3cd4d48a4a3..09cef74477c 100644 --- a/app/assets/javascripts/filtered_search/available_dropdown_mappings.js +++ b/app/assets/javascripts/filtered_search/available_dropdown_mappings.js @@ -77,6 +77,11 @@ export default class AvailableDropdownMappings { gl: DropdownUser, element: this.container.querySelector('#js-dropdown-reviewer'), }, + attention: { + reference: null, + gl: DropdownUser, + element: this.container.getElementById('js-dropdown-attention-requested'), + }, 'approved-by': { reference: null, gl: DropdownUser, diff --git a/app/assets/javascripts/filtered_search/constants.js b/app/assets/javascripts/filtered_search/constants.js index e2d6936acbd..f8b5910de9e 100644 --- a/app/assets/javascripts/filtered_search/constants.js +++ b/app/assets/javascripts/filtered_search/constants.js @@ -1,4 +1,4 @@ -export const USER_TOKEN_TYPES = ['author', 'assignee', 'approved-by', 'reviewer']; +export const USER_TOKEN_TYPES = ['author', 'assignee', 'approved-by', 'reviewer', 'attention']; export const DROPDOWN_TYPE = { hint: 'hint', diff --git a/app/assets/javascripts/notes/mixins/discussion_navigation.js b/app/assets/javascripts/notes/mixins/discussion_navigation.js index ad529eb99b6..93236b05100 100644 --- a/app/assets/javascripts/notes/mixins/discussion_navigation.js +++ b/app/assets/javascripts/notes/mixins/discussion_navigation.js @@ -3,8 +3,6 @@ import { scrollToElementWithContext, scrollToElement } from '~/lib/utils/common_ import { updateHistory } from '../../lib/utils/url_utility'; import eventHub from '../event_hub'; -const isDiffsVirtualScrollingEnabled = () => window.gon?.features?.diffsVirtualScrolling; - /** * @param {string} selector * @returns {boolean} @@ -15,7 +13,7 @@ function scrollTo(selector, { withoutContext = false } = {}) { if (el) { scrollFunction(el, { - behavior: isDiffsVirtualScrollingEnabled() ? 'auto' : 'smooth', + behavior: 'auto', }); return true; } @@ -31,7 +29,7 @@ function updateUrlWithNoteId(noteId) { replace: true, }; - if (noteId && isDiffsVirtualScrollingEnabled()) { + if (noteId) { // Temporarily mask the ID to avoid the browser default // scrolling taking over which is broken with virtual // scrolling enabled. @@ -115,17 +113,13 @@ function handleDiscussionJump(self, fn, discussionId = self.currentDiscussionId) const isDiffView = window.mrTabs.currentAction === 'diffs'; const targetId = fn(discussionId, isDiffView); const discussion = self.getDiscussion(targetId); - const setHash = !isDiffView && !isDiffsVirtualScrollingEnabled(); const discussionFilePath = discussion?.diff_file?.file_path; - if (isDiffsVirtualScrollingEnabled()) { - window.location.hash = ''; - } + window.location.hash = ''; if (discussionFilePath) { self.scrollToFile({ path: discussionFilePath, - setHash, }); } diff --git a/app/assets/javascripts/vue_merge_request_widget/components/deployment/deployment_actions.vue b/app/assets/javascripts/vue_merge_request_widget/components/deployment/deployment_actions.vue index 5ef7c2f72e0..7ba387c79b1 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/deployment/deployment_actions.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/deployment/deployment_actions.vue @@ -1,5 +1,6 @@ <script> import createFlash from '~/flash'; +import { confirmAction } from '~/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal'; import { visitUrl } from '~/lib/utils/url_utility'; import { __, s__ } from '~/locale'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; @@ -79,6 +80,7 @@ export default { [STOPPING]: { actionName: STOPPING, buttonText: s__('MrDeploymentActions|Stop environment'), + buttonVariant: 'danger', busyText: __('This environment is being deployed'), confirmMessage: __('Are you sure you want to stop this environment?'), errorMessage: __('Something went wrong while stopping this environment. Please try again.'), @@ -86,6 +88,7 @@ export default { [DEPLOYING]: { actionName: DEPLOYING, buttonText: s__('MrDeploymentActions|Deploy'), + buttonVariant: 'confirm', busyText: __('This environment is being deployed'), confirmMessage: __('Are you sure you want to deploy this environment?'), errorMessage: __('Something went wrong while deploying this environment. Please try again.'), @@ -93,14 +96,27 @@ export default { [REDEPLOYING]: { actionName: REDEPLOYING, buttonText: s__('MrDeploymentActions|Re-deploy'), + buttonVariant: 'confirm', busyText: __('This environment is being re-deployed'), confirmMessage: __('Are you sure you want to re-deploy this environment?'), errorMessage: __('Something went wrong while deploying this environment. Please try again.'), }, }, methods: { - executeAction(endpoint, { actionName, confirmMessage, errorMessage }) { - const isConfirmed = confirm(confirmMessage); //eslint-disable-line + async executeAction( + endpoint, + { + actionName, + buttonText: primaryBtnText, + buttonVariant: primaryBtnVariant, + confirmMessage, + errorMessage, + }, + ) { + const isConfirmed = await confirmAction(confirmMessage, { + primaryBtnVariant, + primaryBtnText, + }); if (isConfirmed) { this.actionInProgress = actionName; diff --git a/app/assets/stylesheets/utilities.scss b/app/assets/stylesheets/utilities.scss index eb9f7829436..d6aa63be996 100644 --- a/app/assets/stylesheets/utilities.scss +++ b/app/assets/stylesheets/utilities.scss @@ -278,3 +278,33 @@ $gl-line-height-42: px-to-rem(42px); .gl-pr-10 { padding-right: $gl-spacing-scale-10; } + +/* Will be moved to @gitlab/ui by https://gitlab.com/gitlab-org/gitlab-ui/-/issues/1709 */ +.gl-md-grid-template-columns-2 { + @include media-breakpoint-up(md) { + grid-template-columns: 1fr 1fr; + } +} + +.gl-gap-6 { + gap: $gl-spacing-scale-6; +} + +$gl-spacing-scale-48: 48 * $grid-size; + +.gl-max-w-48 { + max-width: $gl-spacing-scale-48; +} + +$gl-spacing-scale-75: 75 * $grid-size; + +.gl-max-w-75 { + max-width: $gl-spacing-scale-75; +} + +.gl-md-pt-11 { + @include media-breakpoint-up(md) { + padding-top: $gl-spacing-scale-11 !important; // only need !important for now so that it overrides styles from @gitlab/ui which currently take precedence + } +} +/* End gitlab-ui#1709 */ diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 2ecd17db487..f94da77609f 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -20,6 +20,10 @@ class DashboardController < Dashboard::ApplicationController urgency :low, [:merge_requests] + before_action only: [:merge_requests] do + push_frontend_feature_flag(:mr_attention_requests, default_enabled: :yaml) + end + def activity respond_to do |format| format.html diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index b85ed29cc2a..05452ef6578 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -30,6 +30,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo before_action :set_issuables_index, only: [:index] before_action :authenticate_user!, only: [:assign_related_issues] before_action :check_user_can_push_to_source_branch!, only: [:rebase] + before_action only: [:index, :show] do + push_frontend_feature_flag(:mr_attention_requests, project, default_enabled: :yaml) + end + before_action only: [:show] do push_frontend_feature_flag(:file_identifier_hash) push_frontend_feature_flag(:merge_request_widget_graphql, @project, default_enabled: :yaml) @@ -38,9 +42,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo push_frontend_feature_flag(:paginated_notes, @project, default_enabled: :yaml) push_frontend_feature_flag(:confidential_notes, @project, default_enabled: :yaml) push_frontend_feature_flag(:improved_emoji_picker, project, default_enabled: :yaml) - push_frontend_feature_flag(:diffs_virtual_scrolling, project, default_enabled: :yaml) push_frontend_feature_flag(:restructured_mr_widget, project, default_enabled: :yaml) - push_frontend_feature_flag(:mr_attention_requests, project, default_enabled: :yaml) push_frontend_feature_flag(:refactor_mr_widgets_extensions, @project, default_enabled: :yaml) push_frontend_feature_flag(:rebase_without_ci_ui, @project, default_enabled: :yaml) push_frontend_feature_flag(:rearrange_pipelines_table, project, default_enabled: :yaml) @@ -50,6 +52,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo push_frontend_feature_flag(:usage_data_diff_searches, @project, default_enabled: :yaml) end + before_action do + push_frontend_feature_flag(:permit_all_shared_groups_for_approval, @project, default_enabled: :yaml) + end + around_action :allow_gitaly_ref_name_caching, only: [:index, :show, :discussions] after_action :log_merge_request_show, only: [:show] diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb index 81e4ab7014d..06feefb9059 100644 --- a/app/finders/merge_requests_finder.rb +++ b/app/finders/merge_requests_finder.rb @@ -44,7 +44,8 @@ class MergeRequestsFinder < IssuableFinder :reviewer_id, :reviewer_username, :target_branch, - :wip + :wip, + :attention ] end @@ -69,6 +70,7 @@ class MergeRequestsFinder < IssuableFinder items = by_approvals(items) items = by_deployments(items) items = by_reviewer(items) + items = by_attention(items) by_source_project_id(items) end @@ -218,6 +220,12 @@ class MergeRequestsFinder < IssuableFinder end end + def by_attention(items) + return items unless params.attention? + + items.attention(params.attention) + end + def parse_datetime(input) # To work around http://www.ruby-lang.org/en/news/2021/11/15/date-parsing-method-regexp-dos-cve-2021-41817/ DateTime.parse(input.byteslice(0, 128)) if input diff --git a/app/finders/merge_requests_finder/params.rb b/app/finders/merge_requests_finder/params.rb index e44e96054d3..1c6a425c8af 100644 --- a/app/finders/merge_requests_finder/params.rb +++ b/app/finders/merge_requests_finder/params.rb @@ -21,5 +21,11 @@ class MergeRequestsFinder end end end + + def attention + strong_memoize(:attention) do + User.find_by_username(params[:attention]) + end + end end end diff --git a/app/finders/projects/members/effective_access_level_finder.rb b/app/finders/projects/members/effective_access_level_finder.rb index d17609ff59f..4538fc4c855 100644 --- a/app/finders/projects/members/effective_access_level_finder.rb +++ b/app/finders/projects/members/effective_access_level_finder.rb @@ -40,7 +40,7 @@ module Projects avenues = [authorizable_project_members] avenues << if project.personal? - project_owner + project_owner_acting_as_maintainer else authorizable_group_members end @@ -85,15 +85,9 @@ module Projects Member.from_union(members) end - # workaround until we migrate Project#owners to have membership with - # OWNER access level - def project_owner + def project_owner_acting_as_maintainer user_id = project.namespace.owner.id - access_level = if ::Feature.enabled?(:personal_project_owner_with_owner_access, default_enabled: :yaml) - Gitlab::Access::OWNER - else - Gitlab::Access::MAINTAINER - end + access_level = Gitlab::Access::MAINTAINER Member .from(generate_from_statement([[user_id, access_level]])) # rubocop: disable CodeReuse/ActiveRecord diff --git a/app/helpers/boards_helper.rb b/app/helpers/boards_helper.rb index 57da04b38cc..28cd61e10d9 100644 --- a/app/helpers/boards_helper.rb +++ b/app/helpers/boards_helper.rb @@ -17,7 +17,6 @@ module BoardsHelper can_update: can_update?.to_s, can_admin_list: can_admin_list?.to_s, time_tracking_limit_to_hours: Gitlab::CurrentSettings.time_tracking_limit_to_hours.to_s, - recent_boards_endpoint: recent_boards_path, parent: current_board_parent.model_name.param_key, group_id: group_id, labels_filter_base_path: build_issue_link_base, @@ -128,10 +127,6 @@ module BoardsHelper } end - def recent_boards_path - recent_project_boards_path(@project) if current_board_parent.is_a?(Project) - end - def serializer CurrentBoardSerializer.new end diff --git a/app/models/concerns/select_for_project_authorization.rb b/app/models/concerns/select_for_project_authorization.rb index e176e29f9d9..49342e30db6 100644 --- a/app/models/concerns/select_for_project_authorization.rb +++ b/app/models/concerns/select_for_project_authorization.rb @@ -8,14 +8,8 @@ module SelectForProjectAuthorization select("projects.id AS project_id", "members.access_level") end - # workaround until we migrate Project#owners to have membership with - # OWNER access level - def select_project_owner_for_project_authorization - if ::Feature.enabled?(:personal_project_owner_with_owner_access, default_enabled: :yaml) - select(["projects.id AS project_id", "#{Gitlab::Access::OWNER} AS access_level"]) - else - select(["projects.id AS project_id", "#{Gitlab::Access::MAINTAINER} AS access_level"]) - end + def select_as_maintainer_for_project_authorization + select(["projects.id AS project_id", "#{Gitlab::Access::MAINTAINER} AS access_level"]) end end end diff --git a/app/models/container_repository.rb b/app/models/container_repository.rb index 7e28c9e079f..38bd8143bf7 100644 --- a/app/models/container_repository.rb +++ b/app/models/container_repository.rb @@ -274,9 +274,7 @@ class ContainerRepository < ApplicationRecord def retry_aborted_migration return unless migration_state == 'import_aborted' - import_status = gitlab_api_client.import_status(self.path) - - case import_status + case external_import_status when 'native' raise NativeImportError when 'import_in_progress' @@ -322,6 +320,12 @@ class ContainerRepository < ApplicationRecord [migration_pre_import_done_at, migration_import_done_at, migration_aborted_at].compact.max end + def external_import_status + strong_memoize(:import_status) do + gitlab_api_client.import_status(self.path) + end + end + # rubocop: disable CodeReuse/ServiceClass def registry @registry ||= begin diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 3826e496320..29540cbde2f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -406,6 +406,17 @@ class MergeRequest < ApplicationRecord ) end + scope :attention, ->(user) do + # rubocop: disable Gitlab/Union + union = Gitlab::SQL::Union.new([ + MergeRequestReviewer.select(:merge_request_id).where(user_id: user.id, state: MergeRequestReviewer.states[:attention_requested]), + MergeRequestAssignee.select(:merge_request_id).where(user_id: user.id, state: MergeRequestAssignee.states[:attention_requested]) + ]) + # rubocop: enable Gitlab/Union + + with(Gitlab::SQL::CTE.new(:reviewers_and_assignees, union).to_arel).where('merge_requests.id in (select merge_request_id from reviewers_and_assignees)') + end + def self.total_time_to_merge join_metrics .merge(MergeRequest::Metrics.with_valid_time_to_merge) diff --git a/app/models/namespaces/traversal/linear_scopes.rb b/app/models/namespaces/traversal/linear_scopes.rb index fed86c020dd..09d69a5f77a 100644 --- a/app/models/namespaces/traversal/linear_scopes.rb +++ b/app/models/namespaces/traversal/linear_scopes.rb @@ -75,6 +75,12 @@ module Namespaces end end + def self_and_hierarchy + return super unless use_traversal_ids_for_self_and_hierarchy_scopes? + + unscoped.from_union([all.self_and_ancestors, all.self_and_descendants(include_self: false)]) + end + def order_by_depth(hierarchy_order) return all unless hierarchy_order @@ -114,6 +120,11 @@ module Namespaces use_traversal_ids? end + def use_traversal_ids_for_self_and_hierarchy_scopes? + Feature.enabled?(:use_traversal_ids_for_self_and_hierarchy_scopes, default_enabled: :yaml) && + use_traversal_ids? + end + def self_and_descendants_with_comparison_operators(include_self: true) base = all.select( :traversal_ids, diff --git a/app/models/namespaces/traversal/recursive_scopes.rb b/app/models/namespaces/traversal/recursive_scopes.rb index 583c53f8221..c6f09a4d134 100644 --- a/app/models/namespaces/traversal/recursive_scopes.rb +++ b/app/models/namespaces/traversal/recursive_scopes.rb @@ -53,6 +53,11 @@ module Namespaces self_and_descendants(include_self: include_self).as_ids end alias_method :recursive_self_and_descendant_ids, :self_and_descendant_ids + + def self_and_hierarchy + Gitlab::ObjectHierarchy.new(all).all_objects + end + alias_method :recursive_self_and_hierarchy, :self_and_hierarchy end end end diff --git a/app/models/project.rb b/app/models/project.rb index 87913d53280..512c6ac1acb 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -459,7 +459,7 @@ class Project < ApplicationRecord delegate :name, to: :owner, allow_nil: true, prefix: true delegate :members, to: :team, prefix: true delegate :add_user, :add_users, to: :team - delegate :add_guest, :add_reporter, :add_developer, :add_maintainer, :add_owner, :add_role, to: :team + delegate :add_guest, :add_reporter, :add_developer, :add_maintainer, :add_role, to: :team delegate :group_runners_enabled, :group_runners_enabled=, to: :ci_cd_settings, allow_nil: true delegate :root_ancestor, to: :namespace, allow_nil: true delegate :last_pipeline, to: :commit, allow_nil: true diff --git a/app/models/project_team.rb b/app/models/project_team.rb index ee5ecc2dd3c..c3c7508df9f 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -23,10 +23,6 @@ class ProjectTeam add_user(user, :maintainer, current_user: current_user) end - def add_owner(user, current_user: nil) - add_user(user, :owner, current_user: current_user) - end - def add_role(user, role, current_user: nil) public_send(:"add_#{role}", user, current_user: current_user) # rubocop:disable GitlabSecurity/PublicSend end @@ -107,9 +103,7 @@ class ProjectTeam if group group.owners else - # workaround until we migrate Project#owners to have membership with - # OWNER access level - Array.wrap(fetch_members(Gitlab::Access::OWNER)) | Array.wrap(project.owner) + [project.owner] end end diff --git a/app/services/members/projects/creator_service.rb b/app/services/members/projects/creator_service.rb index 4dba81acf73..2e974177075 100644 --- a/app/services/members/projects/creator_service.rb +++ b/app/services/members/projects/creator_service.rb @@ -4,7 +4,7 @@ module Members module Projects class CreatorService < Members::CreatorService def self.access_levels - Gitlab::Access.sym_options_with_owner + Gitlab::Access.sym_options end private diff --git a/app/services/notification_recipients/builder/project_maintainers.rb b/app/services/notification_recipients/builder/project_maintainers.rb index a295929a1a9..e8f22c00a83 100644 --- a/app/services/notification_recipients/builder/project_maintainers.rb +++ b/app/services/notification_recipients/builder/project_maintainers.rb @@ -14,7 +14,6 @@ module NotificationRecipients return [] unless project add_recipients(project.team.maintainers, :mention, nil) - add_recipients(project.team.owners, :mention, nil) end def acting_user diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index ecae90e576d..c885369dfec 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -147,11 +147,7 @@ module Projects priority: UserProjectAccessChangedService::LOW_PRIORITY ) else - if ::Feature.enabled?(:personal_project_owner_with_owner_access, default_enabled: :yaml) - @project.add_owner(@project.namespace.owner, current_user: current_user) - else - @project.add_maintainer(@project.namespace.owner, current_user: current_user) - end + @project.add_maintainer(@project.namespace.owner, current_user: current_user) end end diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml index 81a3dc5e4ab..990f40d6aa2 100644 --- a/app/views/layouts/header/_default.html.haml +++ b/app/views/layouts/header/_default.html.haml @@ -107,7 +107,7 @@ -# We'll eventually migrate to .gl-display-none: https://gitlab.com/gitlab-org/gitlab/-/issues/351792. = gl_badge_tag({ size: :sm, variant: :info }, { class: "js-todos-count gl-ml-n2#{(' hidden' if todos_pending_count == 0)}", "aria-label": _("Todos count") }) do = todos_count_format(todos_pending_count) - %li.nav-item.header-help.dropdown.d-none.d-md-block{ **tracking_attrs('main_navigation', 'click_question_mark_link', 'navigation') } + %li.nav-item.header-help.dropdown.d-none.d-md-block{ data: { track_action: 'click_question_mark_link', track_label: 'main_navigation', track_property: 'navigation', track_experiment: 'cross_stage_fdm' } } = link_to help_path, class: 'header-help-dropdown-toggle gl-relative', data: { toggle: "dropdown" } do %span.gl-sr-only = s_('Nav|Help') diff --git a/app/views/layouts/header/_help_dropdown.html.haml b/app/views/layouts/header/_help_dropdown.html.haml index 738bca2f2cc..3a8f9c1ae8d 100644 --- a/app/views/layouts/header/_help_dropdown.html.haml +++ b/app/views/layouts/header/_help_dropdown.html.haml @@ -1,6 +1,7 @@ %ul - if current_user_menu?(:help) = render 'layouts/header/gitlab_version' + = render_if_exists 'layouts/header/help_dropdown/cross_stage_fdm' = render 'layouts/header/whats_new_dropdown_item' %li = link_to _("Help"), help_path diff --git a/app/views/shared/issuable/_search_bar.html.haml b/app/views/shared/issuable/_search_bar.html.haml index 88ebf563e1b..b02c6b65359 100644 --- a/app/views/shared/issuable/_search_bar.html.haml +++ b/app/views/shared/issuable/_search_bar.html.haml @@ -107,6 +107,16 @@ = render 'shared/issuable/user_dropdown_item', user: User.new(username: '{{username}}', name: '{{name}}'), avatar: { lazy: true, url: '{{avatar_url}}' } + - if Feature.enabled?(:mr_attention_requests, default_enabled: :yaml) + #js-dropdown-attention-requested.filtered-search-input-dropdown-menu.dropdown-menu + - if current_user + %ul{ data: { dropdown: true } } + = render 'shared/issuable/user_dropdown_item', + user: current_user + %ul.filter-dropdown{ data: { dynamic: true, dropdown: true } } + = render 'shared/issuable/user_dropdown_item', + user: User.new(username: '{{username}}', name: '{{name}}'), + avatar: { lazy: true, url: '{{avatar_url}}' } = render_if_exists 'shared/issuable/approver_dropdown' = render_if_exists 'shared/issuable/approved_by_dropdown' #js-dropdown-milestone.filtered-search-input-dropdown-menu.dropdown-menu diff --git a/app/workers/container_registry/migration/guard_worker.rb b/app/workers/container_registry/migration/guard_worker.rb index 1237b6058e4..77ae111c1cb 100644 --- a/app/workers/container_registry/migration/guard_worker.rb +++ b/app/workers/container_registry/migration/guard_worker.rb @@ -21,18 +21,68 @@ module ContainerRegistry repositories = ::ContainerRepository.with_stale_migration(step_before_timestamp) .limit(max_capacity) + aborts_count = 0 + long_running_migration_ids = [] # the #to_a is safe as the amount of entries is limited. # In addition, we're calling #each in the next line and we don't want two different SQL queries for these two lines log_extra_metadata_on_done(:stale_migrations_count, repositories.to_a.size) repositories.each do |repository| - repository.abort_import + if abortable?(repository) + repository.abort_import + aborts_count += 1 + else + long_running_migration_ids << repository.id if long_running_migration?(repository) + end + end + + log_extra_metadata_on_done(:aborted_stale_migrations_count, aborts_count) + + if long_running_migration_ids.any? + log_extra_metadata_on_done(:long_running_stale_migration_container_repository_ids, long_running_migration_ids) end end private + # This can ping the Container Registry API. + # We loop on a set of repositories to calls this function (see #perform) + # In the worst case scenario, we have a n+1 API calls situation here. + # + # This is reasonable because the maximum amount of repositories looped + # on is `25`. See ::ContainerRegistry::Migration.capacity. + # + # TODO We can remove this n+1 situation by having a Container Registry API + # endpoint that accepts multiple repository paths at once. This is issue + # https://gitlab.com/gitlab-org/container-registry/-/issues/582 + def abortable?(repository) + # early return to save one Container Registry API request + return true unless repository.importing? || repository.pre_importing? + return true unless external_migration_in_progress?(repository) + + false + end + + def long_running_migration?(repository) + migration_start_timestamp(repository).before?(long_running_migration_threshold) + end + + def external_migration_in_progress?(repository) + status = repository.external_import_status + + (status == 'pre_import_in_progress' && repository.pre_importing?) || + (status == 'import_in_progress' && repository.importing?) + end + + def migration_start_timestamp(repository) + if repository.pre_importing? + repository.migration_pre_import_started_at + else + repository.migration_import_started_at + end + end + def step_before_timestamp ::ContainerRegistry::Migration.max_step_duration.seconds.ago end @@ -42,6 +92,10 @@ module ContainerRegistry # is not properly applied ::ContainerRegistry::Migration.capacity * 2 end + + def long_running_migration_threshold + @threshold ||= 30.minutes.ago + end end end end diff --git a/config/feature_flags/development/diffs_virtual_scrolling.yml b/config/feature_flags/development/diffs_virtual_scrolling.yml deleted file mode 100644 index add1297d8b8..00000000000 --- a/config/feature_flags/development/diffs_virtual_scrolling.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: diffs_virtual_scrolling -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60312 -rollout_issue_url: -milestone: '13.12' -type: development -group: group::code review -default_enabled: true diff --git a/config/feature_flags/development/permit_all_shared_groups_for_approval.yml b/config/feature_flags/development/permit_all_shared_groups_for_approval.yml new file mode 100644 index 00000000000..4ea3b7f696b --- /dev/null +++ b/config/feature_flags/development/permit_all_shared_groups_for_approval.yml @@ -0,0 +1,8 @@ +--- +name: permit_all_shared_groups_for_approval +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80655 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/352766 +milestone: '14.8' +type: development +group: group::source code +default_enabled: false diff --git a/config/feature_flags/development/personal_project_owner_with_owner_access.yml b/config/feature_flags/development/use_traversal_ids_for_self_and_hierarchy_scopes.yml index a82521e88e5..bdbfe33b16d 100644 --- a/config/feature_flags/development/personal_project_owner_with_owner_access.yml +++ b/config/feature_flags/development/use_traversal_ids_for_self_and_hierarchy_scopes.yml @@ -1,7 +1,7 @@ --- -name: personal_project_owner_with_owner_access -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78193 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/351919 +name: use_traversal_ids_for_self_and_hierarchy_scopes +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80045 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/352120 milestone: '14.8' type: development group: group::workspace diff --git a/db/post_migrate/20220127112243_add_index_to_merge_request_assignees_state.rb b/db/post_migrate/20220127112243_add_index_to_merge_request_assignees_state.rb new file mode 100644 index 00000000000..e6bb43af760 --- /dev/null +++ b/db/post_migrate/20220127112243_add_index_to_merge_request_assignees_state.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddIndexToMergeRequestAssigneesState < Gitlab::Database::Migration[1.0] + disable_ddl_transaction! + + INDEX_NAME = 'index_on_merge_request_assignees_state' + + def up + add_concurrent_index :merge_request_assignees, :state, where: 'state = 2', name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :merge_request_assignees, INDEX_NAME + end +end diff --git a/db/post_migrate/20220127112412_add_index_to_merge_request_reviewers_state.rb b/db/post_migrate/20220127112412_add_index_to_merge_request_reviewers_state.rb new file mode 100644 index 00000000000..13f4e05c15b --- /dev/null +++ b/db/post_migrate/20220127112412_add_index_to_merge_request_reviewers_state.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddIndexToMergeRequestReviewersState < Gitlab::Database::Migration[1.0] + disable_ddl_transaction! + + INDEX_NAME = 'index_on_merge_request_reviewers_state' + + def up + add_concurrent_index :merge_request_reviewers, :state, where: 'state = 2', name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :merge_request_reviewers, INDEX_NAME + end +end diff --git a/db/schema_migrations/20220127112243 b/db/schema_migrations/20220127112243 new file mode 100644 index 00000000000..2c591bdde58 --- /dev/null +++ b/db/schema_migrations/20220127112243 @@ -0,0 +1 @@ +7707b9bcdcd7ec28af31b90355905eb8971c12a90c4334b0ae214e45fac9645a
\ No newline at end of file diff --git a/db/schema_migrations/20220127112412 b/db/schema_migrations/20220127112412 new file mode 100644 index 00000000000..af58ff48473 --- /dev/null +++ b/db/schema_migrations/20220127112412 @@ -0,0 +1 @@ +350409be3f491b61a1d757dbd839b48d088339883e8e019d00ad90e6adc350e6
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 5a909d655fa..6460a376664 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -27129,6 +27129,10 @@ CREATE UNIQUE INDEX index_on_instance_statistics_recorded_at_and_identifier ON a CREATE INDEX index_on_label_links_all_columns ON label_links USING btree (target_id, label_id, target_type); +CREATE INDEX index_on_merge_request_assignees_state ON merge_request_assignees USING btree (state) WHERE (state = 2); + +CREATE INDEX index_on_merge_request_reviewers_state ON merge_request_reviewers USING btree (state) WHERE (state = 2); + CREATE INDEX index_on_merge_requests_for_latest_diffs ON merge_requests USING btree (target_project_id) INCLUDE (id, latest_merge_request_diff_id); COMMENT ON INDEX index_on_merge_requests_for_latest_diffs IS 'Index used to efficiently obtain the oldest merge request for a commit SHA'; diff --git a/doc/topics/gitlab_flow.md b/doc/topics/gitlab_flow.md index b6206b301f7..3bca33b32b7 100644 --- a/doc/topics/gitlab_flow.md +++ b/doc/topics/gitlab_flow.md @@ -71,7 +71,7 @@ For example, many projects do releases but don't need to do hotfixes. ## GitHub flow as a simpler alternative In reaction to Git flow, GitHub created a simpler alternative. -[GitHub flow](https://guides.github.com/introduction/flow/index.html) has only feature branches and a `main` branch: +[GitHub flow](https://docs.github.com/en/get-started/quickstart/github-flow) has only feature branches and a `main` branch: ```mermaid graph TD @@ -341,7 +341,7 @@ However, as discussed in [the section about rebasing](#squashing-commits-with-re Rebasing could create more work, as every time you rebase, you may need to resolve the same conflicts. Sometimes you can reuse recorded resolutions (`rerere`), but merging is better, because you only have to resolve conflicts once. -Atlassian has a more thorough explanation of the tradeoffs between merging and rebasing [on their blog](https://www.atlassian.com/blog/git/git-team-workflows-merge-or-rebase). +Atlassian has [a more thorough explanation of the tradeoffs between merging and rebasing](https://www.atlassian.com/blog/git/git-team-workflows-merge-or-rebase) on their blog. A good way to prevent creating many merge commits is to not frequently merge `main` into the feature branch. There are three reasons to merge in `main`: utilizing new code, resolving merge conflicts, and updating long-running branches. @@ -403,7 +403,7 @@ git commit -m 'Properly escape special characters in XML generation' An example of a good commit message is: "Combine templates to reduce duplicate code in the user views." The words "change," "improve," "fix," and "refactor" don't add much information to a commit message. -For more information about formatting commit messages, please see this excellent [blog post by Tim Pope](https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html). +For more information, please see Tim Pope's excellent [note about formatting commit messages](https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html). To add more context to a commit message, consider adding information regarding the origin of the change. For example, the URL of a GitLab issue, or a Jira issue number, diff --git a/doc/update/index.md b/doc/update/index.md index a5aa563a17a..45b5f36bc5a 100644 --- a/doc/update/index.md +++ b/doc/update/index.md @@ -351,7 +351,8 @@ or [init scripts](upgrading_from_source.md#configure-sysv-init-script) by [follo In 14.5 we introduce a set of migrations that wrap up this process by making sure that all remaining jobs over the `merge_request_diff_commits` table are completed. These jobs will have already been processed in most cases so that no extra time is necessary during an upgrade to 14.5. - But if there are remaining jobs, the deployment may take a few extra minutes to complete. + However, if there are remaining jobs or you haven't already upgraded to 14.1, + the deployment may take multiple hours to complete. All merge request diff commits will automatically incorporate these changes, and there are no additional requirements to perform the upgrade. @@ -439,6 +440,11 @@ for how to proceed. with self-managed installations, and ensure 14.0.0-14.0.4 installations do not encounter issues with [batched background migrations](#batched-background-migrations). +- Upgrading to GitLab [14.5](#1450) (or later) may take a lot longer if you do not upgrade to at least 14.1 + first. The 14.1 merge request diff commits database migration can take hours to run, but runs in the + background while GitLab is in use. GitLab instances upgraded directly from 14.0 to 14.5 or later must + run the migration in the foreground and therefore take a lot longer to complete. + - See [Maintenance mode issue in GitLab 13.9 to 14.4](#maintenance-mode-issue-in-gitlab-139-to-144). ### 14.0.0 diff --git a/doc/user/clusters/agent/troubleshooting.md b/doc/user/clusters/agent/troubleshooting.md index d5777267582..2c9f98b7c45 100644 --- a/doc/user/clusters/agent/troubleshooting.md +++ b/doc/user/clusters/agent/troubleshooting.md @@ -11,7 +11,7 @@ When you are using the GitLab Agent for Kubernetes, you might experience issues You can start by viewing the service logs: ```shell -kubectl logs -f -l=app=gitlab-kubernetes-agent -n gitlab-kubernetes-agent +kubectl logs -f -l=app=gitlab-agent -n gitlab-kubernetes-agent ``` If you are a GitLab administrator, you can also view the [GitLab Agent Server logs](../../../administration/clusters/kas.md#troubleshooting). diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 36c81f8ca9b..a00ac13c87b 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -33,27 +33,14 @@ usernames. A GitLab administrator can configure the GitLab instance to ## Project members permissions -> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/219299) in GitLab 14.8, personal namespace owners appear with Owner role in new projects in their namespace. Introduced [with a flag](../administration/feature_flags.md) named `personal_project_owner_with_owner_access`. Disabled by default. - -FLAG: -On self-managed GitLab, personal namespace owners appearing with the Owner role in new projects in their namespace is disabled. To make it available, -ask an administrator to [enable the feature flag](../administration/feature_flags.md) named `personal_project_owner_with_owner_access`. -The feature is not ready for production use. -On GitLab.com, this feature is not available. - A user's role determines what permissions they have on a project. The Owner role provides all permissions but is available only: - For group owners. The role is inherited for a group's projects. - For Administrators. -Personal [namespace](group/index.md#namespaces) owners: - -- Are displayed as having the Maintainer role on projects in the namespace, but have the same permissions as a user with the Owner role. -- (Disabled by default) In GitLab 14.8 and later, for new projects in the namespace, are displayed as having the Owner role. - -For more information about how to manage project members, see -[members of a project](project/members/index.md). +Personal namespace owners have the same permissions as an Owner, but are displayed with the Maintainer role on projects created in their personal namespace. +For more information, see [projects members documentation](project/members/index.md). The following table lists project permissions available for each role: diff --git a/doc/user/search/index.md b/doc/user/search/index.md index 408b55a6fe7..0a799843d3c 100644 --- a/doc/user/search/index.md +++ b/doc/user/search/index.md @@ -32,24 +32,33 @@ in the search field in the upper right corner: ### Filter issue and merge request lists -> - Filter by Epics was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/195704) in GitLab Ultimate 12.9. -> - Filter by child Epics was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/9029) in GitLab Ultimate 13.0. -> - Filter by Iterations was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/118742) in GitLab 13.6. Moved to GitLab Premium in 13.9. +> - Filtering by epics was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/195704) in GitLab 12.9. +> - Filtering by child epics was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/9029) in GitLab 13.0. +> - Filtering by iterations was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/118742) in GitLab 13.6. Moved from GitLab Ultimate to Premium in 13.9. +> - Filtering by type was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/322755) in GitLab 13.10 [with a flag](../../administration/feature_flags.md) named `vue_issues_list`. Disabled by default. Follow these steps to filter the **Issues** and **Merge requests** list pages in projects and groups: 1. Click in the field **Search or filter results...**. 1. In the dropdown menu that appears, select the attribute you wish to filter by: - - Author - Assignee - - [Milestone](../project/milestones/index.md) + - Author + - Confidential + - [Epic and child Epic](../group/epics/index.md) (available only for the group the Epic was created, not for [higher group levels](https://gitlab.com/gitlab-org/gitlab/-/issues/233729)). - [Iteration](../group/iterations/index.md) - - Release - [Label](../project/labels.md) + - [Milestone](../project/milestones/index.md) - My-reaction - - Confidential - - [Epic and child Epic](../group/epics/index.md) (available only for the group the Epic was created, not for [higher group levels](https://gitlab.com/gitlab-org/gitlab/-/issues/233729)). + - Release + - Type + + FLAG: + On self-managed GitLab, by default filtering by type is not available. + To make it available per group, ask an administrator to [enable the feature flag](../../administration/feature_flags.md) named `vue_issues_list`. + On GitLab.com, this feature is not available. + + - Weight - Search for this text 1. Select or type the operator to use for filtering the attribute. The following operators are available: diff --git a/lib/container_registry/gitlab_api_client.rb b/lib/container_registry/gitlab_api_client.rb index aa3d8ff47c4..20b8e1d419b 100644 --- a/lib/container_registry/gitlab_api_client.rb +++ b/lib/container_registry/gitlab_api_client.rb @@ -25,6 +25,7 @@ module ContainerRegistry end end + # https://gitlab.com/gitlab-org/container-registry/-/blob/master/docs-gitlab/api.md#compliance-check def supports_gitlab_api? strong_memoize(:supports_gitlab_api) do registry_features = Gitlab::CurrentSettings.container_registry_features || [] @@ -35,16 +36,19 @@ module ContainerRegistry end end + # https://gitlab.com/gitlab-org/container-registry/-/blob/master/docs-gitlab/api.md#import-repository def pre_import_repository(path) response = start_import_for(path, pre: true) IMPORT_RESPONSES.fetch(response.status, :error) end + # https://gitlab.com/gitlab-org/container-registry/-/blob/master/docs-gitlab/api.md#import-repository def import_repository(path) response = start_import_for(path, pre: false) IMPORT_RESPONSES.fetch(response.status, :error) end + # https://gitlab.com/gitlab-org/container-registry/-/blob/master/docs-gitlab/api.md#get-repository-import-status def import_status(path) body_hash = response_body(faraday.get(import_url_for(path))) body_hash['status'] || 'error' diff --git a/lib/container_registry/migration.rb b/lib/container_registry/migration.rb index 322804418eb..b03c94e5ebf 100644 --- a/lib/container_registry/migration.rb +++ b/lib/container_registry/migration.rb @@ -34,6 +34,11 @@ module ContainerRegistry end def self.capacity + # Increasing capacity numbers will increase the n+1 API calls we can have + # in ContainerRegistry::Migration::GuardWorker#external_migration_in_progress? + # + # TODO: See https://gitlab.com/gitlab-org/container-registry/-/issues/582 + # return 25 if Feature.enabled?(:container_registry_migration_phase2_capacity_25) return 10 if Feature.enabled?(:container_registry_migration_phase2_capacity_10) return 1 if Feature.enabled?(:container_registry_migration_phase2_capacity_1) diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index d0b426aeb60..3e09d488bc3 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -33,13 +33,7 @@ module Gitlab MAINTAINER_SUBGROUP_ACCESS = 1 class << self - def values - if ::Feature.enabled?(:personal_project_owner_with_owner_access, default_enabled: :yaml) - options_with_owner.values - else - options.values - end - end + delegate :values, to: :options def all_values options_with_owner.values diff --git a/lib/gitlab/project_authorizations.rb b/lib/gitlab/project_authorizations.rb index 1d7b179baf0..121626ced56 100644 --- a/lib/gitlab/project_authorizations.rb +++ b/lib/gitlab/project_authorizations.rb @@ -22,7 +22,7 @@ module Gitlab user.projects_with_active_memberships.select_for_project_authorization, # The personal projects of the user. - user.personal_projects.select_project_owner_for_project_authorization, + user.personal_projects.select_as_maintainer_for_project_authorization, # Projects that belong directly to any of the groups the user has # access to. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index a7b0b2ff934..76a86981cc8 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4999,6 +4999,9 @@ msgstr[1] "" msgid "Attaching the file failed." msgstr "" +msgid "Attention" +msgstr "" + msgid "Audit Events" msgstr "" @@ -18605,6 +18608,9 @@ msgstr "" msgid "InProductMarketing|3 ways to dive into GitLab CI/CD" msgstr "" +msgid "InProductMarketing|Access advanced features, build more efficiently, strengthen security and compliance." +msgstr "" + msgid "InProductMarketing|Actually, GitLab makes the team work (better)" msgstr "" @@ -18638,6 +18644,9 @@ msgstr "" msgid "InProductMarketing|Code owners and required merge approvals are part of the paid tiers of GitLab. You can start a free 30-day trial of GitLab Ultimate and enable these features in less than 5 minutes with no credit card required." msgstr "" +msgid "InProductMarketing|Collaboration across stages in GitLab" +msgstr "" + msgid "InProductMarketing|Create a custom CI runner with just a few clicks" msgstr "" @@ -18662,6 +18671,12 @@ msgstr "" msgid "InProductMarketing|Dig in and create a project and a repo" msgstr "" +msgid "InProductMarketing|Discover Premium & Ultimate" +msgstr "" + +msgid "InProductMarketing|Discover Premium & Ultimate." +msgstr "" + msgid "InProductMarketing|Do you have a minute?" msgstr "" @@ -18869,6 +18884,9 @@ msgstr "" msgid "InProductMarketing|Start a Self-Managed trial" msgstr "" +msgid "InProductMarketing|Start a free trial" +msgstr "" + msgid "InProductMarketing|Start a free trial of GitLab Ultimate – no credit card required" msgstr "" @@ -40305,6 +40323,15 @@ msgstr "" msgid "VulnerabilityManagement|Create Jira issue" msgstr "" +msgid "VulnerabilityManagement|Enter a name" +msgstr "" + +msgid "VulnerabilityManagement|Enter the CVE or CWE code" +msgstr "" + +msgid "VulnerabilityManagement|Enter the CVE or CWE identifier URL" +msgstr "" + msgid "VulnerabilityManagement|Fetching linked Jira issues" msgstr "" @@ -40329,6 +40356,12 @@ msgstr "" msgid "VulnerabilityManagement|Select a method" msgstr "" +msgid "VulnerabilityManagement|Select a severity level" +msgstr "" + +msgid "VulnerabilityManagement|Select a status" +msgstr "" + msgid "VulnerabilityManagement|Severity is a required field" msgstr "" diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb index d6af5976743..d8ef95cf11a 100644 --- a/spec/controllers/projects/project_members_controller_spec.rb +++ b/spec/controllers/projects/project_members_controller_spec.rb @@ -665,7 +665,7 @@ RSpec.describe Projects::ProjectMembersController do sign_in(user) end - it 'creates a member' do + it 'does not create a member' do expect do post :create, params: { user_ids: stranger.id, @@ -673,9 +673,7 @@ RSpec.describe Projects::ProjectMembersController do access_level: Member::OWNER, project_id: project } - end.to change { project.members.count }.by(1) - - expect(project.team_members).to include(user) + end.to change { project.members.count }.by(0) end end diff --git a/spec/features/merge_request/batch_comments_spec.rb b/spec/features/merge_request/batch_comments_spec.rb index a4aae7d8020..eb98c7d5061 100644 --- a/spec/features/merge_request/batch_comments_spec.rb +++ b/spec/features/merge_request/batch_comments_spec.rb @@ -32,7 +32,7 @@ RSpec.describe 'Merge request > Batch comments', :js do expect(page).to have_selector('[data-testid="review_bar_component"]') - expect(find('.review-bar-content .btn-confirm')).to have_content('1') + expect(find('[data-testid="review_bar_component"] .btn-confirm')).to have_content('1') end it 'publishes review' do @@ -150,10 +150,6 @@ RSpec.describe 'Merge request > Batch comments', :js do it 'adds draft comments to both sides' do write_parallel_comment('2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9') - - # make sure line 9 is in the view - execute_script("window.scrollBy(0, -200)") - write_parallel_comment('2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9', button_text: 'Add to review', text: 'Another wrong line') expect(find('.new .draft-note-component')).to have_content('Line is wrong') @@ -255,13 +251,15 @@ RSpec.describe 'Merge request > Batch comments', :js do end def write_diff_comment(**params) - click_diff_line(find("[id='#{sample_compare.changes[0][:line_code]}']")) + click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[0][:line_code]}']")) write_comment(**params) end def write_parallel_comment(line, **params) - find("div[id='#{line}']").hover + line_element = find_by_scrolling("[id='#{line}']") + scroll_to_elements_bottom(line_element) + line_element.hover find(".js-add-diff-note-button").click write_comment(selector: "form[data-line-code='#{line}']", **params) diff --git a/spec/features/merge_request/user_comments_on_diff_spec.rb b/spec/features/merge_request/user_comments_on_diff_spec.rb index 29c346593d5..c06019f6b77 100644 --- a/spec/features/merge_request/user_comments_on_diff_spec.rb +++ b/spec/features/merge_request/user_comments_on_diff_spec.rb @@ -25,14 +25,15 @@ RSpec.describe 'User comments on a diff', :js do context 'when toggling inline comments' do context 'in a single file' do it 'hides a comment' do - click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + line_element = find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']").find(:xpath, "..") + click_diff_line(line_element) page.within('.js-discussion-note-form') do fill_in('note_note', with: 'Line is wrong') click_button('Add comment now') end - page.within('.diff-files-holder > div:nth-child(6)') do + page.within(line_element.ancestor('[data-path]')) do expect(page).to have_content('Line is wrong') find('.js-diff-more-actions').click @@ -45,7 +46,9 @@ RSpec.describe 'User comments on a diff', :js do context 'in multiple files' do it 'toggles comments' do - click_diff_line(find("[id='#{sample_compare.changes[0][:line_code]}']")) + first_line_element = find_by_scrolling("[id='#{sample_compare.changes[0][:line_code]}']").find(:xpath, "..") + first_root_element = first_line_element.ancestor('[data-path]') + click_diff_line(first_line_element) page.within('.js-discussion-note-form') do fill_in('note_note', with: 'Line is correct') @@ -54,11 +57,14 @@ RSpec.describe 'User comments on a diff', :js do wait_for_requests - page.within('.diff-files-holder > div:nth-child(5) .note-body > .note-text') do + page.within(first_root_element) do expect(page).to have_content('Line is correct') end - click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + second_line_element = find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']") + second_root_element = second_line_element.ancestor('[data-path]') + + click_diff_line(second_line_element) page.within('.js-discussion-note-form') do fill_in('note_note', with: 'Line is wrong') @@ -68,7 +74,7 @@ RSpec.describe 'User comments on a diff', :js do wait_for_requests # Hide the comment. - page.within('.diff-files-holder > div:nth-child(6)') do + page.within(second_root_element) do find('.js-diff-more-actions').click click_button 'Hide comments on this file' @@ -77,37 +83,36 @@ RSpec.describe 'User comments on a diff', :js do # At this moment a user should see only one comment. # The other one should be hidden. - page.within('.diff-files-holder > div:nth-child(5) .note-body > .note-text') do + page.within(first_root_element) do expect(page).to have_content('Line is correct') end # Show the comment. - page.within('.diff-files-holder > div:nth-child(6)') do + page.within(second_root_element) do find('.js-diff-more-actions').click click_button 'Show comments on this file' end # Now both the comments should be shown. - page.within('.diff-files-holder > div:nth-child(6) .note-body > .note-text') do + page.within(second_root_element) do expect(page).to have_content('Line is wrong') end - page.within('.diff-files-holder > div:nth-child(5) .note-body > .note-text') do + page.within(first_root_element) do expect(page).to have_content('Line is correct') end # Check the same comments in the side-by-side view. - execute_script("window.scrollTo(0,0);") find('.js-show-diff-settings').click click_button 'Side-by-side' wait_for_requests - page.within('.diff-files-holder > div:nth-child(6) .parallel .note-body > .note-text') do + page.within(second_root_element) do expect(page).to have_content('Line is wrong') end - page.within('.diff-files-holder > div:nth-child(5) .parallel .note-body > .note-text') do + page.within(first_root_element) do expect(page).to have_content('Line is correct') end end @@ -121,7 +126,7 @@ RSpec.describe 'User comments on a diff', :js do context 'when adding multiline comments' do it 'saves a multiline comment' do - click_diff_line(find("[id='#{sample_commit.line_code}']")) + click_diff_line(find_by_scrolling("[id='#{sample_commit.line_code}']").find(:xpath, '..')) add_comment('-13', '+14') end @@ -133,13 +138,13 @@ RSpec.describe 'User comments on a diff', :js do # In `files/ruby/popen.rb` it 'allows comments for changes involving both sides' do # click +15, select -13 add and verify comment - click_diff_line(find('div[data-path="files/ruby/popen.rb"] .right-side a[data-linenumber="15"]').find(:xpath, '../../..'), 'right') + click_diff_line(find_by_scrolling('div[data-path="files/ruby/popen.rb"] .right-side a[data-linenumber="15"]').find(:xpath, '../../..'), 'right') add_comment('-13', '+15') end it 'allows comments on previously hidden lines at the top of a file', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/285294' do # Click -9, expand up, select 1 add and verify comment - page.within('[data-path="files/ruby/popen.rb"]') do + page.within find_by_scrolling('[data-path="files/ruby/popen.rb"]') do all('.js-unfold-all')[0].click end click_diff_line(find('div[data-path="files/ruby/popen.rb"] .left-side a[data-linenumber="9"]').find(:xpath, '../..'), 'left') @@ -148,7 +153,7 @@ RSpec.describe 'User comments on a diff', :js do it 'allows comments on previously hidden lines the middle of a file' do # Click 27, expand up, select 18, add and verify comment - page.within('[data-path="files/ruby/popen.rb"]') do + page.within find_by_scrolling('[data-path="files/ruby/popen.rb"]') do all('.js-unfold-all')[1].click end click_diff_line(find('div[data-path="files/ruby/popen.rb"] .left-side a[data-linenumber="21"]').find(:xpath, '../..'), 'left') @@ -157,7 +162,7 @@ RSpec.describe 'User comments on a diff', :js do it 'allows comments on previously hidden lines at the bottom of a file' do # Click +28, expand down, select 37 add and verify comment - page.within('[data-path="files/ruby/popen.rb"]') do + page.within find_by_scrolling('[data-path="files/ruby/popen.rb"]') do all('.js-unfold-down:not([disabled])')[1].click end click_diff_line(find('div[data-path="files/ruby/popen.rb"] .left-side a[data-linenumber="30"]').find(:xpath, '../..'), 'left') @@ -198,7 +203,7 @@ RSpec.describe 'User comments on a diff', :js do context 'when editing comments' do it 'edits a comment' do - click_diff_line(find("[id='#{sample_commit.line_code}']")) + click_diff_line(find_by_scrolling("[id='#{sample_commit.line_code}']")) page.within('.js-discussion-note-form') do fill_in(:note_note, with: 'Line is wrong') @@ -224,7 +229,7 @@ RSpec.describe 'User comments on a diff', :js do context 'when deleting comments' do it 'deletes a comment' do - click_diff_line(find("[id='#{sample_commit.line_code}']")) + click_diff_line(find_by_scrolling("[id='#{sample_commit.line_code}']")) page.within('.js-discussion-note-form') do fill_in(:note_note, with: 'Line is wrong') diff --git a/spec/features/merge_request/user_expands_diff_spec.rb b/spec/features/merge_request/user_expands_diff_spec.rb index 52554f11d28..25c9584350d 100644 --- a/spec/features/merge_request/user_expands_diff_spec.rb +++ b/spec/features/merge_request/user_expands_diff_spec.rb @@ -7,7 +7,7 @@ RSpec.describe 'User expands diff', :js do let(:merge_request) { create(:merge_request, source_branch: 'expand-collapse-files', source_project: project, target_project: project) } before do - allow(Gitlab::CurrentSettings).to receive(:diff_max_patch_bytes).and_return(100.kilobytes) + allow(Gitlab::CurrentSettings).to receive(:diff_max_patch_bytes).and_return(100.bytes) visit(diffs_project_merge_request_path(project, merge_request)) @@ -15,7 +15,7 @@ RSpec.describe 'User expands diff', :js do end it 'allows user to expand diff' do - page.within find('[id="19763941ab80e8c09871c0a425f0560d9053bcb3"]') do + page.within find("[id='4c76a1271e41072d7da9fe40bf0f79f7384d472a']") do find('[data-testid="expand-button"]').click wait_for_requests diff --git a/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb b/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb index ffb9694e806..7d67cde4bbb 100644 --- a/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb +++ b/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb @@ -15,16 +15,14 @@ RSpec.describe 'Batch diffs', :js do visit diffs_project_merge_request_path(merge_request.project, merge_request) wait_for_requests - # Add discussion to first line of first file - click_diff_line(find('.diff-file.file-holder:first-of-type .line_holder .left-side:first-of-type')) - page.within('.js-discussion-note-form') do + click_diff_line(get_first_diff.find('[data-testid="left-side"]', match: :first)) + page.within get_first_diff.find('.js-discussion-note-form') do fill_in('note_note', with: 'First Line Comment') click_button('Add comment now') end - # Add discussion to first line of last file - click_diff_line(find('.diff-file.file-holder:last-of-type .line_holder .left-side:first-of-type')) - page.within('.js-discussion-note-form') do + click_diff_line(get_second_diff.find('[data-testid="left-side"]', match: :first)) + page.within get_second_diff.find('.js-discussion-note-form') do fill_in('note_note', with: 'Last Line Comment') click_button('Add comment now') end @@ -36,17 +34,14 @@ RSpec.describe 'Batch diffs', :js do # Reload so we know the discussions are persisting across batch loads visit page.current_url - # Wait for JS to settle wait_for_requests - expect(page).to have_selector('.diff-files-holder .file-holder', count: 39) - # Confirm discussions are applied to appropriate files (should be contained in multiple diff pages) - page.within('.diff-file.file-holder:first-of-type .notes .timeline-entry .note .note-text') do + page.within get_first_diff.find('.notes .timeline-entry .note .note-text') do expect(page).to have_content('First Line Comment') end - page.within('.diff-file.file-holder:last-of-type .notes .timeline-entry .note .note-text') do + page.within get_second_diff.find('.notes .timeline-entry .note .note-text') do expect(page).to have_content('Last Line Comment') end end @@ -54,7 +49,7 @@ RSpec.describe 'Batch diffs', :js do context 'when user visits a URL with a link directly to to a discussion' do context 'which is in the first batched page of diffs' do it 'scrolls to the correct discussion' do - page.within('.diff-file.file-holder:first-of-type') do + page.within get_first_diff do click_link('just now') end @@ -63,15 +58,15 @@ RSpec.describe 'Batch diffs', :js do wait_for_requests # Confirm scrolled to correct UI element - expect(page.find('.diff-file.file-holder:first-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_falsey - expect(page.find('.diff-file.file-holder:last-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_truthy + expect(get_first_diff.find('.discussion-notes .timeline-entry li.note[id]').obscured?).to be_falsey + expect(get_second_diff.find('.discussion-notes .timeline-entry li.note[id]').obscured?).to be_truthy end end context 'which is in at least page 2 of the batched pages of diffs' do it 'scrolls to the correct discussion', quarantine: { issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/293814' } do - page.within('.diff-file.file-holder:last-of-type') do + page.within get_first_diff do click_link('just now') end @@ -80,8 +75,8 @@ RSpec.describe 'Batch diffs', :js do wait_for_requests # Confirm scrolled to correct UI element - expect(page.find('.diff-file.file-holder:first-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_truthy - expect(page.find('.diff-file.file-holder:last-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_falsey + expect(get_first_diff.find('.discussion-notes .timeline-entry li.note[id]').obscured?).to be_truthy + expect(get_second_diff.find('.discussion-notes .timeline-entry li.note[id]').obscured?).to be_falsey end end end @@ -95,15 +90,21 @@ RSpec.describe 'Batch diffs', :js do end it 'has the correct discussions applied to files across batched pages' do - expect(page).to have_selector('.diff-files-holder .file-holder', count: 39) - - page.within('.diff-file.file-holder:first-of-type .notes .timeline-entry .note .note-text') do + page.within get_first_diff.find('.notes .timeline-entry .note .note-text') do expect(page).to have_content('First Line Comment') end - page.within('.diff-file.file-holder:last-of-type .notes .timeline-entry .note .note-text') do + page.within get_second_diff.find('.notes .timeline-entry .note .note-text') do expect(page).to have_content('Last Line Comment') end end end + + def get_first_diff + find('#a9b6f940524f646951cc28d954aa41f814f95d4f') + end + + def get_second_diff + find('#b285a86891571c7fdbf1f82e840816079de1cc8b') + end end diff --git a/spec/features/merge_request/user_posts_diff_notes_spec.rb b/spec/features/merge_request/user_posts_diff_notes_spec.rb index 76431d04e72..d803aec5895 100644 --- a/spec/features/merge_request/user_posts_diff_notes_spec.rb +++ b/spec/features/merge_request/user_posts_diff_notes_spec.rb @@ -29,54 +29,54 @@ RSpec.describe 'Merge request > User posts diff notes', :js do context 'with an old line on the left and no line on the right' do it 'allows commenting on the left side' do - should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]'), 'left') + should_allow_commenting(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]'), 'left') end it 'does not allow commenting on the right side' do - should_not_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'right') + should_not_allow_commenting(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'right') end end context 'with no line on the left and a new line on the right' do it 'does not allow commenting on the left side' do - should_not_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'left') + should_not_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'left') end it 'allows commenting on the right side' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'right') + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'right') end end context 'with an old line on the left and a new line on the right' do it 'allows commenting on the left side', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/199050' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'left') + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'left') end it 'allows commenting on the right side' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'right') + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'right') end end context 'with an unchanged line on the left and an unchanged line on the right' do it 'allows commenting on the left side', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/196826' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]', match: :first).find(:xpath, '..'), 'left') + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]', match: :first).find(:xpath, '..'), 'left') end it 'allows commenting on the right side' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]', match: :first).find(:xpath, '..'), 'right') + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]', match: :first).find(:xpath, '..'), 'right') end end context 'with a match line' do it 'does not allow commenting' do - line_holder = find('.match', match: :first) + line_holder = find_by_scrolling('.match', match: :first) match_should_not_allow_commenting(line_holder) end end context 'with an unfolded line' do before do - page.within('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do + page.within find_by_scrolling('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do find('.js-unfold', match: :first).click end @@ -84,12 +84,12 @@ RSpec.describe 'Merge request > User posts diff notes', :js do end it 'allows commenting on the left side' do - should_allow_commenting(first('#a5cc2925ca8258af241be7e5b0381edf30266302 .line_holder [data-testid="left-side"]')) + should_allow_commenting(find_by_scrolling('#a5cc2925ca8258af241be7e5b0381edf30266302').first('.line_holder [data-testid="left-side"]')) end it 'allows commenting on the right side' do # Automatically shifts comment box to left side. - should_allow_commenting(first('#a5cc2925ca8258af241be7e5b0381edf30266302 .line_holder [data-testid="right-side"]')) + should_allow_commenting(find_by_scrolling('#a5cc2925ca8258af241be7e5b0381edf30266302').first('.line_holder [data-testid="right-side"]')) end end end @@ -101,44 +101,44 @@ RSpec.describe 'Merge request > User posts diff notes', :js do context 'after deleteing a note' do it 'allows commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) accept_gl_confirm(button_text: 'Delete Comment') do first('button.more-actions-toggle').click first('.js-note-delete').click end - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) end end context 'with a new line' do it 'allows commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) end end context 'with an old line' do it 'allows commenting' do - should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]')) + should_allow_commenting(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]')) end end context 'with an unchanged line' do it 'allows commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) end end context 'with a match line' do it 'does not allow commenting' do - match_should_not_allow_commenting(find('.match', match: :first)) + match_should_not_allow_commenting(find_by_scrolling('.match', match: :first)) end end context 'with an unfolded line' do before do - page.within('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do + page.within find_by_scrolling('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do find('.js-unfold', match: :first).click end @@ -147,7 +147,7 @@ RSpec.describe 'Merge request > User posts diff notes', :js do # The first `.js-unfold` unfolds upwards, therefore the first # `.line_holder` will be an unfolded line. - let(:line_holder) { first('[id="a5cc2925ca8258af241be7e5b0381edf30266302_1_1"]') } + let(:line_holder) { find_by_scrolling('[id="a5cc2925ca8258af241be7e5b0381edf30266302_1_1"]') } it 'allows commenting' do should_allow_commenting line_holder @@ -157,7 +157,7 @@ RSpec.describe 'Merge request > User posts diff notes', :js do context 'when hovering over a diff discussion' do before do visit diffs_project_merge_request_path(project, merge_request, view: 'inline') - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) visit project_merge_request_path(project, merge_request) end @@ -174,7 +174,7 @@ RSpec.describe 'Merge request > User posts diff notes', :js do context 'with a new line' do it 'allows dismissing a comment' do - should_allow_dismissing_a_comment(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) + should_allow_dismissing_a_comment(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) end end end @@ -182,13 +182,13 @@ RSpec.describe 'Merge request > User posts diff notes', :js do describe 'with multiple note forms' do before do visit diffs_project_merge_request_path(project, merge_request, view: 'inline') - click_diff_line(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) - click_diff_line(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]')) + click_diff_line(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) + click_diff_line(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]')) end describe 'posting a note' do it 'adds as discussion' do - should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'), asset_form_reset: false) + should_allow_commenting(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'), asset_form_reset: false) expect(page).to have_css('.notes_holder .note.note-discussion', count: 1) expect(page).to have_field('Reply…') end @@ -203,25 +203,25 @@ RSpec.describe 'Merge request > User posts diff notes', :js do context 'with a new line' do it 'allows commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) end end context 'with an old line' do it 'allows commenting' do - should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]')) + should_allow_commenting(find_by_scrolling('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]')) end end context 'with an unchanged line' do it 'allows commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) + should_allow_commenting(find_by_scrolling('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) end end context 'with a match line' do it 'does not allow commenting' do - match_should_not_allow_commenting(find('.match', match: :first)) + match_should_not_allow_commenting(find_by_scrolling('.match', match: :first)) end end end diff --git a/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb b/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb index 261d1ad325b..33c5a936b8d 100644 --- a/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb +++ b/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb @@ -6,6 +6,7 @@ include Spec::Support::Helpers::ModalHelpers # rubocop:disable Style/MixinUsage RSpec.describe 'Merge request > User sees avatars on diff notes', :js do include NoteInteractionHelpers include Spec::Support::Helpers::ModalHelpers + include MergeRequestDiffHelpers let(:project) { create(:project, :public, :repository) } let(:user) { project.creator } @@ -135,6 +136,7 @@ RSpec.describe 'Merge request > User sees avatars on diff notes', :js do end it 'adds avatar when commenting' do + find_by_scrolling('[data-discussion-id]', match: :first) find_field('Reply…', match: :first).click page.within '.js-discussion-note-form' do @@ -154,6 +156,7 @@ RSpec.describe 'Merge request > User sees avatars on diff notes', :js do it 'adds multiple comments' do 3.times do + find_by_scrolling('[data-discussion-id]', match: :first) find_field('Reply…', match: :first).click page.within '.js-discussion-note-form' do @@ -192,7 +195,7 @@ RSpec.describe 'Merge request > User sees avatars on diff notes', :js do end def find_line(line_code) - line = find("[id='#{line_code}']") + line = find_by_scrolling("[id='#{line_code}']") line = line.find(:xpath, 'preceding-sibling::*[1][self::td]/preceding-sibling::*[1][self::td]') if line.tag_name == 'td' line end diff --git a/spec/features/merge_request/user_sees_deployment_widget_spec.rb b/spec/features/merge_request/user_sees_deployment_widget_spec.rb index 345404cc28f..01cc58777ba 100644 --- a/spec/features/merge_request/user_sees_deployment_widget_spec.rb +++ b/spec/features/merge_request/user_sees_deployment_widget_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe 'Merge request > User sees deployment widget', :js do + include Spec::Support::Helpers::ModalHelpers + describe 'when merge request has associated environments' do let(:user) { create(:user) } let(:project) { create(:project, :repository) } @@ -118,7 +120,9 @@ RSpec.describe 'Merge request > User sees deployment widget', :js do end it 'does start build when stop button clicked' do - accept_confirm { find('.js-stop-env').click } + accept_gl_confirm(button_text: 'Stop environment') do + find('.js-stop-env').click + end expect(page).to have_content('close_app') end diff --git a/spec/features/merge_request/user_sees_diff_spec.rb b/spec/features/merge_request/user_sees_diff_spec.rb index 5e022598ac3..7cd9ef80874 100644 --- a/spec/features/merge_request/user_sees_diff_spec.rb +++ b/spec/features/merge_request/user_sees_diff_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe 'Merge request > User sees diff', :js do include ProjectForksHelper include RepoHelpers + include MergeRequestDiffHelpers let(:project) { create(:project, :public, :repository) } let(:merge_request) { create(:merge_request, source_project: project) } @@ -58,12 +59,12 @@ RSpec.describe 'Merge request > User sees diff', :js do let(:changelog_id) { Digest::SHA1.hexdigest("CHANGELOG") } context 'as author' do - it 'shows direct edit link', :sidekiq_might_not_need_inline do + it 'contains direct edit link', :sidekiq_might_not_need_inline do sign_in(author_user) visit diffs_project_merge_request_path(project, merge_request) # Throws `Capybara::Poltergeist::InvalidSelector` if we try to use `#hash` syntax - expect(page).to have_selector("[id=\"#{changelog_id}\"] .js-edit-blob", visible: false) + expect(page).to have_selector(".js-edit-blob", visible: false) end end @@ -72,6 +73,8 @@ RSpec.describe 'Merge request > User sees diff', :js do sign_in(user) visit diffs_project_merge_request_path(project, merge_request) + find_by_scrolling("[id=\"#{changelog_id}\"]") + # Throws `Capybara::Poltergeist::InvalidSelector` if we try to use `#hash` syntax find("[id=\"#{changelog_id}\"] .js-diff-more-actions").click find("[id=\"#{changelog_id}\"] .js-edit-blob").click @@ -107,6 +110,7 @@ RSpec.describe 'Merge request > User sees diff', :js do CONTENT file_name = 'xss_file.rs' + file_hash = Digest::SHA1.hexdigest(file_name) create_file('master', file_name, file_content) merge_request = create(:merge_request, source_project: project) @@ -116,6 +120,8 @@ RSpec.describe 'Merge request > User sees diff', :js do visit diffs_project_merge_request_path(project, merge_request) + find_by_scrolling("[id='#{file_hash}']") + expect(page).to have_text("function foo<input> {") expect(page).to have_css(".line[lang='rust'] .k") end @@ -136,7 +142,7 @@ RSpec.describe 'Merge request > User sees diff', :js do end context 'when file is an image', :js do - let(:file_name) { 'files/lfs/image.png' } + let(:file_name) { 'a/image.png' } it 'shows an error message' do expect(page).not_to have_content('could not be displayed because it is stored in LFS') @@ -144,7 +150,7 @@ RSpec.describe 'Merge request > User sees diff', :js do end context 'when file is not an image' do - let(:file_name) { 'files/lfs/ruby.rb' } + let(:file_name) { 'a/ruby.rb' } it 'shows an error message' do expect(page).to have_content('This source diff could not be displayed because it is stored in LFS') @@ -153,7 +159,14 @@ RSpec.describe 'Merge request > User sees diff', :js do end context 'when LFS is not enabled' do + let(:file_name) { 'a/lfs_object.iso' } + before do + allow(Gitlab.config.lfs).to receive(:disabled).and_return(true) + project.update_attribute(:lfs_enabled, false) + + create_file('master', file_name, project.repository.blob_at('master', 'files/lfs/lfs_object.iso').data) + visit diffs_project_merge_request_path(project, merge_request) end diff --git a/spec/features/merge_request/user_sees_versions_spec.rb b/spec/features/merge_request/user_sees_versions_spec.rb index 5abf4e2f5ad..2b856811e02 100644 --- a/spec/features/merge_request/user_sees_versions_spec.rb +++ b/spec/features/merge_request/user_sees_versions_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe 'Merge request > User sees versions', :js do + include MergeRequestDiffHelpers + let(:merge_request) do create(:merge_request).tap do |mr| mr.merge_request_diff.destroy! @@ -27,8 +29,12 @@ RSpec.describe 'Merge request > User sees versions', :js do diff_file_selector = ".diff-file[id='#{file_id}']" line_code = "#{file_id}_#{line_code}" - page.within(diff_file_selector) do - first("[id='#{line_code}']").hover + page.within find_by_scrolling(diff_file_selector) do + line_code_element = first("[id='#{line_code}']") + # scrolling to element's bottom is required in order for .hover action to work + # otherwise, the element could be hidden underneath a sticky header + scroll_to_elements_bottom(line_code_element) + line_code_element.hover first("[id='#{line_code}'] [role='button']").click page.within("form[data-line-code='#{line_code}']") do diff --git a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb index 690a292937a..beb658bb7a0 100644 --- a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb +++ b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb @@ -34,7 +34,7 @@ RSpec.describe 'User comments on a diff', :js do context 'single suggestion note' do it 'hides suggestion popover' do - click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']")) expect(page).to have_selector('.diff-suggest-popover') @@ -46,7 +46,7 @@ RSpec.describe 'User comments on a diff', :js do end it 'suggestion is presented' do - click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']")) page.within('.js-discussion-note-form') do fill_in('note_note', with: "```suggestion\n# change to a comment\n```") @@ -74,7 +74,7 @@ RSpec.describe 'User comments on a diff', :js do end it 'allows suggestions in replies' do - click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']")) page.within('.js-discussion-note-form') do fill_in('note_note', with: "```suggestion\n# change to a comment\n```") @@ -91,7 +91,7 @@ RSpec.describe 'User comments on a diff', :js do end it 'suggestion is appliable' do - click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']")) page.within('.js-discussion-note-form') do fill_in('note_note', with: "```suggestion\n# change to a comment\n```") @@ -273,7 +273,7 @@ RSpec.describe 'User comments on a diff', :js do context 'multiple suggestions in a single note' do it 'suggestions are presented', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/258989' do - click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']")) page.within('.js-discussion-note-form') do fill_in('note_note', with: "```suggestion\n# change to a comment\n```\n```suggestion:-2\n# or that\n# heh\n```") @@ -316,7 +316,7 @@ RSpec.describe 'User comments on a diff', :js do context 'multi-line suggestions' do before do - click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) + click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[1][:line_code]}']")) page.within('.js-discussion-note-form') do fill_in('note_note', with: "```suggestion:-3+5\n# change to a\n# comment\n# with\n# broken\n# lines\n```") diff --git a/spec/finders/merge_requests_finder/params_spec.rb b/spec/finders/merge_requests_finder/params_spec.rb new file mode 100644 index 00000000000..8c285972f48 --- /dev/null +++ b/spec/finders/merge_requests_finder/params_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequestsFinder::Params do + let(:user) { create(:user) } + + subject { described_class.new(params, user, MergeRequest) } + + describe 'attention' do + context 'attention param exists' do + let(:params) { { attention: user.username } } + + it { expect(subject.attention).to eq(user) } + end + + context 'attention param does not exist' do + let(:params) { { attention: nil } } + + it { expect(subject.attention).to eq(nil) } + end + end +end diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index 0b6c438fd02..1f63f69a411 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -408,6 +408,22 @@ RSpec.describe MergeRequestsFinder do end end + context 'attention' do + subject { described_class.new(user, params).execute } + + before do + reviewer = merge_request1.find_reviewer(user2) + reviewer.update!(state: :reviewed) + end + + context 'by username' do + let(:params) { { attention: user2.username } } + let(:expected_mr) { [merge_request2, merge_request3] } + + it { is_expected.to contain_exactly(*expected_mr) } + end + end + context 'reviewer filtering' do subject { described_class.new(user, params).execute } diff --git a/spec/finders/projects/members/effective_access_level_finder_spec.rb b/spec/finders/projects/members/effective_access_level_finder_spec.rb index 446b0f8f9a2..33fbb5aca30 100644 --- a/spec/finders/projects/members/effective_access_level_finder_spec.rb +++ b/spec/finders/projects/members/effective_access_level_finder_spec.rb @@ -11,40 +11,21 @@ RSpec.describe Projects::Members::EffectiveAccessLevelFinder, '#execute' do context 'for a personal project' do let_it_be(:project) { create(:project) } - shared_examples_for 'includes access level of the owner of the project' do - context 'when personal_project_owner_with_owner_access feature flag is enabled' do - it 'includes access level of the owner of the project as Owner' do - expect(subject).to( - contain_exactly( - hash_including( - 'user_id' => project.namespace.owner.id, - 'access_level' => Gitlab::Access::OWNER - ) - ) - ) - end - end - - context 'when personal_project_owner_with_owner_access feature flag is disabled' do - before do - stub_feature_flags(personal_project_owner_with_owner_access: false) - end - - it 'includes access level of the owner of the project as Maintainer' do - expect(subject).to( - contain_exactly( - hash_including( - 'user_id' => project.namespace.owner.id, - 'access_level' => Gitlab::Access::MAINTAINER - ) + shared_examples_for 'includes access level of the owner of the project as Maintainer' do + it 'includes access level of the owner of the project as Maintainer' do + expect(subject).to( + contain_exactly( + hash_including( + 'user_id' => project.namespace.owner.id, + 'access_level' => Gitlab::Access::MAINTAINER ) ) - end + ) end end context 'when the project owner is a member of the project' do - it_behaves_like 'includes access level of the owner of the project' + it_behaves_like 'includes access level of the owner of the project as Maintainer' end context 'when the project owner is not explicitly a member of the project' do @@ -52,7 +33,7 @@ RSpec.describe Projects::Members::EffectiveAccessLevelFinder, '#execute' do project.members.find_by(user_id: project.namespace.owner.id).destroy! end - it_behaves_like 'includes access level of the owner of the project' + it_behaves_like 'includes access level of the owner of the project as Maintainer' end end @@ -103,32 +84,17 @@ RSpec.describe Projects::Members::EffectiveAccessLevelFinder, '#execute' do context 'for a project within a group' do context 'project in a root group' do - context 'includes access levels of users who are direct members of the parent group' do - it 'when access level is developer' do - group_member = create(:group_member, :developer, source: group) + it 'includes access levels of users who are direct members of the parent group' do + group_member = create(:group_member, :developer, source: group) - expect(subject).to( - include( - hash_including( - 'user_id' => group_member.user.id, - 'access_level' => Gitlab::Access::DEVELOPER - ) - ) - ) - end - - it 'when access level is owner' do - group_member = create(:group_member, :owner, source: group) - - expect(subject).to( - include( - hash_including( - 'user_id' => group_member.user.id, - 'access_level' => Gitlab::Access::OWNER - ) + expect(subject).to( + include( + hash_including( + 'user_id' => group_member.user.id, + 'access_level' => Gitlab::Access::DEVELOPER ) ) - end + ) end end diff --git a/spec/frontend/boards/components/boards_selector_spec.js b/spec/frontend/boards/components/boards_selector_spec.js index 9cf7c5774bf..26a5bf34595 100644 --- a/spec/frontend/boards/components/boards_selector_spec.js +++ b/spec/frontend/boards/components/boards_selector_spec.js @@ -1,43 +1,40 @@ import { GlDropdown, GlLoadingIcon, GlDropdownSectionHeader } from '@gitlab/ui'; import { mount } from '@vue/test-utils'; -import MockAdapter from 'axios-mock-adapter'; import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; import Vuex from 'vuex'; import { TEST_HOST } from 'spec/test_constants'; import BoardsSelector from '~/boards/components/boards_selector.vue'; +import { BoardType } from '~/boards/constants'; import groupBoardQuery from '~/boards/graphql/group_board.query.graphql'; import projectBoardQuery from '~/boards/graphql/project_board.query.graphql'; +import groupBoardsQuery from '~/boards/graphql/group_boards.query.graphql'; +import projectBoardsQuery from '~/boards/graphql/project_boards.query.graphql'; +import groupRecentBoardsQuery from '~/boards/graphql/group_recent_boards.query.graphql'; +import projectRecentBoardsQuery from '~/boards/graphql/project_recent_boards.query.graphql'; import defaultStore from '~/boards/stores'; -import axios from '~/lib/utils/axios_utils'; import createMockApollo from 'helpers/mock_apollo_helper'; -import { mockGroupBoardResponse, mockProjectBoardResponse } from '../mock_data'; +import { + mockGroupBoardResponse, + mockProjectBoardResponse, + mockGroupAllBoardsResponse, + mockProjectAllBoardsResponse, + mockGroupRecentBoardsResponse, + mockProjectRecentBoardsResponse, + mockSmallProjectAllBoardsResponse, + mockEmptyProjectRecentBoardsResponse, + boards, + recentIssueBoards, +} from '../mock_data'; const throttleDuration = 1; Vue.use(VueApollo); -function boardGenerator(n) { - return new Array(n).fill().map((board, index) => { - const id = `${index}`; - const name = `board${id}`; - - return { - id, - name, - }; - }); -} - describe('BoardsSelector', () => { let wrapper; - let allBoardsResponse; - let recentBoardsResponse; - let mock; let fakeApollo; let store; - const boards = boardGenerator(20); - const recentBoards = boardGenerator(5); const createStore = ({ isGroupBoard = false, isProjectBoard = false } = {}) => { store = new Vuex.Store({ @@ -63,17 +60,43 @@ describe('BoardsSelector', () => { }; const getDropdownItems = () => wrapper.findAll('.js-dropdown-item'); - const getDropdownHeaders = () => wrapper.findAll(GlDropdownSectionHeader); - const getLoadingIcon = () => wrapper.find(GlLoadingIcon); - const findDropdown = () => wrapper.find(GlDropdown); + const getDropdownHeaders = () => wrapper.findAllComponents(GlDropdownSectionHeader); + const getLoadingIcon = () => wrapper.findComponent(GlLoadingIcon); + const findDropdown = () => wrapper.findComponent(GlDropdown); const projectBoardQueryHandlerSuccess = jest.fn().mockResolvedValue(mockProjectBoardResponse); const groupBoardQueryHandlerSuccess = jest.fn().mockResolvedValue(mockGroupBoardResponse); - const createComponent = () => { + const projectBoardsQueryHandlerSuccess = jest + .fn() + .mockResolvedValue(mockProjectAllBoardsResponse); + const groupBoardsQueryHandlerSuccess = jest.fn().mockResolvedValue(mockGroupAllBoardsResponse); + + const projectRecentBoardsQueryHandlerSuccess = jest + .fn() + .mockResolvedValue(mockProjectRecentBoardsResponse); + const groupRecentBoardsQueryHandlerSuccess = jest + .fn() + .mockResolvedValue(mockGroupRecentBoardsResponse); + + const smallBoardsQueryHandlerSuccess = jest + .fn() + .mockResolvedValue(mockSmallProjectAllBoardsResponse); + const emptyRecentBoardsQueryHandlerSuccess = jest + .fn() + .mockResolvedValue(mockEmptyProjectRecentBoardsResponse); + + const createComponent = ({ + projectBoardsQueryHandler = projectBoardsQueryHandlerSuccess, + projectRecentBoardsQueryHandler = projectRecentBoardsQueryHandlerSuccess, + } = {}) => { fakeApollo = createMockApollo([ [projectBoardQuery, projectBoardQueryHandlerSuccess], [groupBoardQuery, groupBoardQueryHandlerSuccess], + [projectBoardsQuery, projectBoardsQueryHandler], + [groupBoardsQuery, groupBoardsQueryHandlerSuccess], + [projectRecentBoardsQuery, projectRecentBoardsQueryHandler], + [groupRecentBoardsQuery, groupRecentBoardsQueryHandlerSuccess], ]); wrapper = mount(BoardsSelector, { @@ -91,67 +114,34 @@ describe('BoardsSelector', () => { attachTo: document.body, provide: { fullPath: '', - recentBoardsEndpoint: `${TEST_HOST}/recent`, }, }); - - wrapper.vm.$apollo.addSmartQuery = jest.fn((_, options) => { - // setData usage is discouraged. See https://gitlab.com/groups/gitlab-org/-/epics/7330 for details - // eslint-disable-next-line no-restricted-syntax - wrapper.setData({ - [options.loadingKey]: true, - }); - }); }; afterEach(() => { wrapper.destroy(); - wrapper = null; - mock.restore(); + fakeApollo = null; }); - describe('fetching all boards', () => { + describe('template', () => { beforeEach(() => { - mock = new MockAdapter(axios); - - allBoardsResponse = Promise.resolve({ - data: { - group: { - boards: { - edges: boards.map((board) => ({ node: board })), - }, - }, - }, - }); - recentBoardsResponse = Promise.resolve({ - data: recentBoards, - }); - - createStore(); + createStore({ isProjectBoard: true }); createComponent(); - - mock.onGet(`${TEST_HOST}/recent`).replyOnce(200, recentBoards); }); describe('loading', () => { - beforeEach(async () => { - // Wait for current board to be loaded - await nextTick(); - - // Emits gl-dropdown show event to simulate the dropdown is opened at initialization time - findDropdown().vm.$emit('show'); - }); - // we are testing loading state, so don't resolve responses until after the tests afterEach(async () => { - await Promise.all([allBoardsResponse, recentBoardsResponse]); await nextTick(); }); it('shows loading spinner', () => { + // Emits gl-dropdown show event to simulate the dropdown is opened at initialization time + findDropdown().vm.$emit('show'); + + expect(getLoadingIcon().exists()).toBe(true); expect(getDropdownHeaders()).toHaveLength(0); expect(getDropdownItems()).toHaveLength(0); - expect(getLoadingIcon().exists()).toBe(true); }); }); @@ -163,16 +153,13 @@ describe('BoardsSelector', () => { // Emits gl-dropdown show event to simulate the dropdown is opened at initialization time findDropdown().vm.$emit('show'); - // setData usage is discouraged. See https://gitlab.com/groups/gitlab-org/-/epics/7330 for details - // eslint-disable-next-line no-restricted-syntax - await wrapper.setData({ - loadingBoards: false, - loadingRecentBoards: false, - }); - await Promise.all([allBoardsResponse, recentBoardsResponse]); await nextTick(); }); + it('fetches all issue boards', () => { + expect(projectBoardsQueryHandlerSuccess).toHaveBeenCalled(); + }); + it('hides loading spinner', async () => { await nextTick(); expect(getLoadingIcon().exists()).toBe(false); @@ -180,22 +167,17 @@ describe('BoardsSelector', () => { describe('filtering', () => { beforeEach(async () => { - // setData usage is discouraged. See https://gitlab.com/groups/gitlab-org/-/epics/7330 for details - // eslint-disable-next-line no-restricted-syntax - wrapper.setData({ - boards, - }); - await nextTick(); }); it('shows all boards without filtering', () => { - expect(getDropdownItems()).toHaveLength(boards.length + recentBoards.length); + expect(getDropdownItems()).toHaveLength(boards.length + recentIssueBoards.length); }); it('shows only matching boards when filtering', async () => { const filterTerm = 'board1'; - const expectedCount = boards.filter((board) => board.name.includes(filterTerm)).length; + const expectedCount = boards.filter((board) => board.node.name.includes(filterTerm)) + .length; fillSearchBox(filterTerm); @@ -214,32 +196,21 @@ describe('BoardsSelector', () => { describe('recent boards section', () => { it('shows only when boards are greater than 10', async () => { - // setData usage is discouraged. See https://gitlab.com/groups/gitlab-org/-/epics/7330 for details - // eslint-disable-next-line no-restricted-syntax - wrapper.setData({ - boards, - }); - await nextTick(); + expect(projectRecentBoardsQueryHandlerSuccess).toHaveBeenCalled(); expect(getDropdownHeaders()).toHaveLength(2); }); it('does not show when boards are less than 10', async () => { - // setData usage is discouraged. See https://gitlab.com/groups/gitlab-org/-/epics/7330 for details - // eslint-disable-next-line no-restricted-syntax - wrapper.setData({ - boards: boards.slice(0, 5), - }); + createComponent({ projectBoardsQueryHandler: smallBoardsQueryHandlerSuccess }); await nextTick(); expect(getDropdownHeaders()).toHaveLength(0); }); - it('does not show when recentBoards api returns empty array', async () => { - // setData usage is discouraged. See https://gitlab.com/groups/gitlab-org/-/epics/7330 for details - // eslint-disable-next-line no-restricted-syntax - wrapper.setData({ - recentBoards: [], + it('does not show when recentIssueBoards api returns empty array', async () => { + createComponent({ + projectRecentBoardsQueryHandler: emptyRecentBoardsQueryHandlerSuccess, }); await nextTick(); @@ -256,15 +227,39 @@ describe('BoardsSelector', () => { }); }); + describe('fetching all boards', () => { + it.each` + boardType | queryHandler | notCalledHandler + ${BoardType.group} | ${groupBoardsQueryHandlerSuccess} | ${projectBoardsQueryHandlerSuccess} + ${BoardType.project} | ${projectBoardsQueryHandlerSuccess} | ${groupBoardsQueryHandlerSuccess} + `('fetches $boardType boards', async ({ boardType, queryHandler, notCalledHandler }) => { + createStore({ + isProjectBoard: boardType === BoardType.project, + isGroupBoard: boardType === BoardType.group, + }); + createComponent(); + + await nextTick(); + + // Emits gl-dropdown show event to simulate the dropdown is opened at initialization time + findDropdown().vm.$emit('show'); + + await nextTick(); + + expect(queryHandler).toHaveBeenCalled(); + expect(notCalledHandler).not.toHaveBeenCalled(); + }); + }); + describe('fetching current board', () => { it.each` - boardType | queryHandler | notCalledHandler - ${'group'} | ${groupBoardQueryHandlerSuccess} | ${projectBoardQueryHandlerSuccess} - ${'project'} | ${projectBoardQueryHandlerSuccess} | ${groupBoardQueryHandlerSuccess} + boardType | queryHandler | notCalledHandler + ${BoardType.group} | ${groupBoardQueryHandlerSuccess} | ${projectBoardQueryHandlerSuccess} + ${BoardType.project} | ${projectBoardQueryHandlerSuccess} | ${groupBoardQueryHandlerSuccess} `('fetches $boardType board', async ({ boardType, queryHandler, notCalledHandler }) => { createStore({ - isProjectBoard: boardType === 'project', - isGroupBoard: boardType === 'group', + isProjectBoard: boardType === BoardType.project, + isGroupBoard: boardType === BoardType.group, }); createComponent(); diff --git a/spec/frontend/boards/mock_data.js b/spec/frontend/boards/mock_data.js index 25496582707..24096fddea6 100644 --- a/spec/frontend/boards/mock_data.js +++ b/spec/frontend/boards/mock_data.js @@ -29,6 +29,85 @@ export const listObj = { }, }; +function boardGenerator(n) { + return new Array(n).fill().map((board, index) => { + const id = `${index}`; + const name = `board${id}`; + + return { + node: { + id, + name, + weight: 0, + __typename: 'Board', + }, + }; + }); +} + +export const boards = boardGenerator(20); +export const recentIssueBoards = boardGenerator(5); + +export const mockSmallProjectAllBoardsResponse = { + data: { + project: { + id: 'gid://gitlab/Project/114', + boards: { edges: boardGenerator(3) }, + __typename: 'Project', + }, + }, +}; + +export const mockEmptyProjectRecentBoardsResponse = { + data: { + project: { + id: 'gid://gitlab/Project/114', + recentIssueBoards: { edges: [] }, + __typename: 'Project', + }, + }, +}; + +export const mockGroupAllBoardsResponse = { + data: { + group: { + id: 'gid://gitlab/Group/114', + boards: { edges: boards }, + __typename: 'Group', + }, + }, +}; + +export const mockProjectAllBoardsResponse = { + data: { + project: { + id: 'gid://gitlab/Project/1', + boards: { edges: boards }, + __typename: 'Project', + }, + }, +}; + +export const mockGroupRecentBoardsResponse = { + data: { + group: { + id: 'gid://gitlab/Group/114', + recentIssueBoards: { edges: recentIssueBoards }, + __typename: 'Group', + }, + }, +}; + +export const mockProjectRecentBoardsResponse = { + data: { + project: { + id: 'gid://gitlab/Project/1', + recentIssueBoards: { edges: recentIssueBoards }, + __typename: 'Project', + }, + }, +}; + export const mockGroupBoardResponse = { data: { workspace: { diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index dda758898e3..76e4a944d87 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -69,6 +69,12 @@ describe('diffs/components/app', () => { }, provide, store, + stubs: { + DynamicScroller: { + template: `<div><slot :item="$store.state.diffs.diffFiles[0]"></slot></div>`, + }, + DynamicScrollerItem: true, + }, }); } diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index c53789b4800..d6a2aa104cd 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -202,7 +202,7 @@ describe('DiffsStoreActions', () => { testAction( fetchDiffFilesBatch, {}, - { endpointBatch, diffViewType: 'inline' }, + { endpointBatch, diffViewType: 'inline', diffFiles: [] }, [ { type: types.SET_BATCH_LOADING_STATE, payload: 'loading' }, { type: types.SET_RETRIEVING_BATCHES, payload: true }, diff --git a/spec/frontend/notes/mixins/discussion_navigation_spec.js b/spec/frontend/notes/mixins/discussion_navigation_spec.js index 1f71947b3cd..aba80789a01 100644 --- a/spec/frontend/notes/mixins/discussion_navigation_spec.js +++ b/spec/frontend/notes/mixins/discussion_navigation_spec.js @@ -143,7 +143,7 @@ describe('Discussion navigation mixin', () => { it('scrolls to element', () => { expect(utils.scrollToElement).toHaveBeenCalledWith( findDiscussion('div.discussion', expected), - { behavior: 'smooth' }, + { behavior: 'auto' }, ); }); }); @@ -171,7 +171,7 @@ describe('Discussion navigation mixin', () => { expect(utils.scrollToElementWithContext).toHaveBeenCalledWith( findDiscussion('ul.notes', expected), - { behavior: 'smooth' }, + { behavior: 'auto' }, ); }); }); @@ -212,21 +212,15 @@ describe('Discussion navigation mixin', () => { it('scrolls to discussion', () => { expect(utils.scrollToElement).toHaveBeenCalledWith( findDiscussion('div.discussion', expected), - { behavior: 'smooth' }, + { behavior: 'auto' }, ); }); }); }); }); - describe.each` - diffsVirtualScrolling - ${false} - ${true} - `('virtual scrolling feature is $diffsVirtualScrolling', ({ diffsVirtualScrolling }) => { + describe('virtual scrolling feature', () => { beforeEach(() => { - window.gon = { features: { diffsVirtualScrolling } }; - jest.spyOn(store, 'dispatch'); store.state.notes.currentDiscussionId = 'a'; @@ -238,22 +232,22 @@ describe('Discussion navigation mixin', () => { window.location.hash = ''; }); - it('resets location hash if diffsVirtualScrolling flag is true', async () => { + it('resets location hash', async () => { wrapper.vm.jumpToNextDiscussion(); await nextTick(); - expect(window.location.hash).toBe(diffsVirtualScrolling ? '' : '#test'); + expect(window.location.hash).toBe(''); }); it.each` - tabValue | hashValue - ${'diffs'} | ${false} - ${'show'} | ${!diffsVirtualScrolling} - ${'other'} | ${!diffsVirtualScrolling} + tabValue + ${'diffs'} + ${'show'} + ${'other'} `( 'calls scrollToFile with setHash as $hashValue when the tab is $tabValue', - async ({ hashValue, tabValue }) => { + async ({ tabValue }) => { window.mrTabs.currentAction = tabValue; wrapper.vm.jumpToNextDiscussion(); @@ -262,7 +256,6 @@ describe('Discussion navigation mixin', () => { expect(store.dispatch).toHaveBeenCalledWith('diffs/scrollToFile', { path: 'test.js', - setHash: hashValue, }); }, ); diff --git a/spec/frontend/vue_mr_widget/deployment/deployment_actions_spec.js b/spec/frontend/vue_mr_widget/deployment/deployment_actions_spec.js index 31ade17e50a..a285d26f404 100644 --- a/spec/frontend/vue_mr_widget/deployment/deployment_actions_spec.js +++ b/spec/frontend/vue_mr_widget/deployment/deployment_actions_spec.js @@ -1,5 +1,7 @@ import { mount } from '@vue/test-utils'; +import waitForPromises from 'helpers/wait_for_promises'; import createFlash from '~/flash'; +import { confirmAction } from '~/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal'; import { visitUrl } from '~/lib/utils/url_utility'; import { CREATED, @@ -20,6 +22,11 @@ import { jest.mock('~/flash'); jest.mock('~/lib/utils/url_utility'); +jest.mock('~/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal', () => { + return { + confirmAction: jest.fn(), + }; +}); describe('DeploymentAction component', () => { let wrapper; @@ -51,6 +58,7 @@ describe('DeploymentAction component', () => { afterEach(() => { wrapper.destroy(); + confirmAction.mockReset(); }); describe('actions do not appear when conditions are unmet', () => { @@ -95,16 +103,6 @@ describe('DeploymentAction component', () => { '$configConst action', ({ configConst, computedDeploymentStatus, displayConditionChanges, finderFn, endpoint }) => { describe(`${configConst} action`, () => { - const confirmAction = () => { - jest.spyOn(window, 'confirm').mockReturnValueOnce(true); - finderFn().trigger('click'); - }; - - const rejectAction = () => { - jest.spyOn(window, 'confirm').mockReturnValueOnce(false); - finderFn().trigger('click'); - }; - beforeEach(() => { factory({ propsData: { @@ -125,13 +123,18 @@ describe('DeploymentAction component', () => { describe('should show a confirm dialog but not call executeInlineAction when declined', () => { beforeEach(() => { executeActionSpy.mockResolvedValueOnce(); - rejectAction(); + confirmAction.mockResolvedValueOnce(false); + finderFn().trigger('click'); }); it('should show the confirm dialog', () => { - expect(window.confirm).toHaveBeenCalled(); - expect(window.confirm).toHaveBeenCalledWith( + expect(confirmAction).toHaveBeenCalled(); + expect(confirmAction).toHaveBeenCalledWith( actionButtonMocks[configConst].confirmMessage, + { + primaryBtnVariant: actionButtonMocks[configConst].buttonVariant, + primaryBtnText: actionButtonMocks[configConst].buttonText, + }, ); }); @@ -143,13 +146,18 @@ describe('DeploymentAction component', () => { describe('should show a confirm dialog and call executeInlineAction when accepted', () => { beforeEach(() => { executeActionSpy.mockResolvedValueOnce(); - confirmAction(); + confirmAction.mockResolvedValueOnce(true); + finderFn().trigger('click'); }); it('should show the confirm dialog', () => { - expect(window.confirm).toHaveBeenCalled(); - expect(window.confirm).toHaveBeenCalledWith( + expect(confirmAction).toHaveBeenCalled(); + expect(confirmAction).toHaveBeenCalledWith( actionButtonMocks[configConst].confirmMessage, + { + primaryBtnVariant: actionButtonMocks[configConst].buttonVariant, + primaryBtnText: actionButtonMocks[configConst].buttonText, + }, ); }); @@ -164,11 +172,15 @@ describe('DeploymentAction component', () => { describe('response includes redirect_url', () => { const url = '/root/example'; - beforeEach(() => { + beforeEach(async () => { executeActionSpy.mockResolvedValueOnce({ data: { redirect_url: url }, }); - confirmAction(); + + await waitForPromises(); + + confirmAction.mockResolvedValueOnce(true); + finderFn().trigger('click'); }); it('calls visit url with the redirect_url', () => { @@ -178,9 +190,13 @@ describe('DeploymentAction component', () => { }); describe('it should call the executeAction method ', () => { - beforeEach(() => { + beforeEach(async () => { jest.spyOn(wrapper.vm, 'executeAction').mockImplementation(); - confirmAction(); + + await waitForPromises(); + + confirmAction.mockResolvedValueOnce(true); + finderFn().trigger('click'); }); it('calls with the expected arguments', () => { @@ -193,9 +209,13 @@ describe('DeploymentAction component', () => { }); describe('when executeInlineAction errors', () => { - beforeEach(() => { + beforeEach(async () => { executeActionSpy.mockRejectedValueOnce(); - confirmAction(); + + await waitForPromises(); + + confirmAction.mockResolvedValueOnce(true); + finderFn().trigger('click'); }); it('should call createFlash with error message', () => { diff --git a/spec/frontend/vue_mr_widget/deployment/deployment_mock_data.js b/spec/frontend/vue_mr_widget/deployment/deployment_mock_data.js index 2083dc88681..e98b1160ae4 100644 --- a/spec/frontend/vue_mr_widget/deployment/deployment_mock_data.js +++ b/spec/frontend/vue_mr_widget/deployment/deployment_mock_data.js @@ -9,6 +9,7 @@ const actionButtonMocks = { [STOPPING]: { actionName: STOPPING, buttonText: 'Stop environment', + buttonVariant: 'danger', busyText: 'This environment is being deployed', confirmMessage: 'Are you sure you want to stop this environment?', errorMessage: 'Something went wrong while stopping this environment. Please try again.', @@ -16,6 +17,7 @@ const actionButtonMocks = { [DEPLOYING]: { actionName: DEPLOYING, buttonText: 'Deploy', + buttonVariant: 'confirm', busyText: 'This environment is being deployed', confirmMessage: 'Are you sure you want to deploy this environment?', errorMessage: 'Something went wrong while deploying this environment. Please try again.', @@ -23,6 +25,7 @@ const actionButtonMocks = { [REDEPLOYING]: { actionName: REDEPLOYING, buttonText: 'Re-deploy', + buttonVariant: 'confirm', busyText: 'This environment is being re-deployed', confirmMessage: 'Are you sure you want to re-deploy this environment?', errorMessage: 'Something went wrong while deploying this environment. Please try again.', diff --git a/spec/lib/gitlab/project_authorizations_spec.rb b/spec/lib/gitlab/project_authorizations_spec.rb index 8630762e06f..7852470196b 100644 --- a/spec/lib/gitlab/project_authorizations_spec.rb +++ b/spec/lib/gitlab/project_authorizations_spec.rb @@ -34,28 +34,12 @@ RSpec.describe Gitlab::ProjectAuthorizations do .to include(owned_project.id, other_project.id, group_project.id) end - context 'when personal_project_owner_with_owner_access feature flag is enabled' do - it 'includes the correct access levels' do - mapping = map_access_levels(authorizations) - - expect(mapping[owned_project.id]).to eq(Gitlab::Access::OWNER) - expect(mapping[other_project.id]).to eq(Gitlab::Access::REPORTER) - expect(mapping[group_project.id]).to eq(Gitlab::Access::DEVELOPER) - end - end - - context 'when personal_project_owner_with_owner_access feature flag is disabled' do - before do - stub_feature_flags(personal_project_owner_with_owner_access: false) - end - - it 'includes the correct access levels' do - mapping = map_access_levels(authorizations) + it 'includes the correct access levels' do + mapping = map_access_levels(authorizations) - expect(mapping[owned_project.id]).to eq(Gitlab::Access::MAINTAINER) - expect(mapping[other_project.id]).to eq(Gitlab::Access::REPORTER) - expect(mapping[group_project.id]).to eq(Gitlab::Access::DEVELOPER) - end + expect(mapping[owned_project.id]).to eq(Gitlab::Access::MAINTAINER) + expect(mapping[other_project.id]).to eq(Gitlab::Access::REPORTER) + expect(mapping[group_project.id]).to eq(Gitlab::Access::DEVELOPER) end end diff --git a/spec/models/container_repository_spec.rb b/spec/models/container_repository_spec.rb index 77de9e83f0a..3524c810ff4 100644 --- a/spec/models/container_repository_spec.rb +++ b/spec/models/container_repository_spec.rb @@ -1179,6 +1179,16 @@ RSpec.describe ContainerRepository, :aggregate_failures do end end + describe '#external_import_status' do + subject { repository.external_import_status } + + it 'returns the response from the client' do + expect(repository.gitlab_api_client).to receive(:import_status).with(repository.path).and_return('test') + + expect(subject).to eq('test') + end + end + describe '.with_stale_migration' do let_it_be(:repository) { create(:container_repository) } let_it_be(:stale_pre_importing_old_timestamp) { create(:container_repository, :pre_importing, migration_pre_import_started_at: 10.minutes.ago) } diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index e9a38755692..f2f2023a992 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -144,6 +144,20 @@ RSpec.describe MergeRequest, factory_default: :keep do end end + describe '.attention' do + let_it_be(:merge_request5) { create(:merge_request, :unique_branches, assignees: [user2])} + let_it_be(:merge_request6) { create(:merge_request, :unique_branches, assignees: [user2])} + + before do + assignee = merge_request6.find_assignee(user2) + assignee.update!(state: :reviewed) + end + + it 'returns MRs that have any attention requests' do + expect(described_class.attention(user2)).to eq([merge_request2, merge_request5]) + end + end + describe '.drafts' do it 'returns MRs where draft == true' do expect(described_class.drafts).to eq([merge_request4]) diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index 5b11f9d828a..bfdebbc33df 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -225,7 +225,7 @@ RSpec.describe ProjectTeam do let_it_be(:maintainer) { create(:user) } let_it_be(:developer) { create(:user) } let_it_be(:guest) { create(:user) } - let_it_be(:project) { create(:project, group: create(:group)) } + let_it_be(:project) { create(:project, namespace: maintainer.namespace) } let_it_be(:access_levels) { [Gitlab::Access::DEVELOPER, Gitlab::Access::MAINTAINER] } subject(:members_with_access_levels) { project.team.members_with_access_levels(access_levels) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6e7cccfccf6..e4f25c79e53 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -3717,7 +3717,7 @@ RSpec.describe User do context 'with min_access_level' do let!(:user) { create(:user) } - let!(:project) { create(:project, :private, group: create(:group)) } + let!(:project) { create(:project, :private, namespace: user.namespace) } before do project.add_developer(user) diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 5c867b4580b..6186a43f992 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -675,30 +675,13 @@ RSpec.describe API::Members do end context 'adding owner to project' do - context 'when personal_project_owner_with_owner_access feature flag is enabled' do - it 'returns created status' do - expect do - post api("/projects/#{project.id}/members", maintainer), - params: { user_id: stranger.id, access_level: Member::OWNER } - - expect(response).to have_gitlab_http_status(:created) - end.to change { project.members.count }.by(1) - end - end - - context 'when personal_project_owner_with_owner_access feature flag is disabled' do - before do - stub_feature_flags(personal_project_owner_with_owner_access: false) - end - - it 'returns created status' do - expect do - post api("/projects/#{project.id}/members", maintainer), - params: { user_id: stranger.id, access_level: Member::OWNER } + it 'returns 403' do + expect do + post api("/projects/#{project.id}/members", maintainer), + params: { user_id: stranger.id, access_level: Member::OWNER } - expect(response).to have_gitlab_http_status(:bad_request) - end.not_to change { project.members.count } - end + expect(response).to have_gitlab_http_status(:bad_request) + end.not_to change { project.members.count } end end diff --git a/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb b/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb index 537d1986384..c6b184bd003 100644 --- a/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb +++ b/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb @@ -40,7 +40,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do it 'is called' do ProjectAuthorization.delete_all - expect(callback).to receive(:call).with(project.id, Gitlab::Access::OWNER).once + expect(callback).to receive(:call).with(project.id, Gitlab::Access::MAINTAINER).once service.execute end @@ -60,20 +60,20 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do to_be_removed = [project2.id] to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER } + { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } ] expect(service.execute).to eq([to_be_removed, to_be_added]) end it 'finds duplicate entries that has to be removed' do - [Gitlab::Access::OWNER, Gitlab::Access::REPORTER].each do |access_level| + [Gitlab::Access::MAINTAINER, Gitlab::Access::REPORTER].each do |access_level| user.project_authorizations.create!(project: project, access_level: access_level) end to_be_removed = [project.id] to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER } + { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } ] expect(service.execute).to eq([to_be_removed, to_be_added]) @@ -85,7 +85,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do to_be_removed = [project.id] to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER } + { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } ] expect(service.execute).to eq([to_be_removed, to_be_added]) @@ -143,16 +143,16 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do end it 'sets the keys to the project IDs' do - expect(hash.keys).to match_array([project.id]) + expect(hash.keys).to eq([project.id]) end it 'sets the values to the access levels' do - expect(hash.values).to match_array([Gitlab::Access::OWNER]) + expect(hash.values).to eq([Gitlab::Access::MAINTAINER]) end context 'personal projects' do it 'includes the project with the right access level' do - expect(hash[project.id]).to eq(Gitlab::Access::OWNER) + expect(hash[project.id]).to eq(Gitlab::Access::MAINTAINER) end end @@ -242,7 +242,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do value = hash.values[0] expect(value.project_id).to eq(project.id) - expect(value.access_level).to eq(Gitlab::Access::OWNER) + expect(value.access_level).to eq(Gitlab::Access::MAINTAINER) end end @@ -267,7 +267,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do end it 'includes the access level for every row' do - expect(row.access_level).to eq(Gitlab::Access::OWNER) + expect(row.access_level).to eq(Gitlab::Access::MAINTAINER) end end end @@ -283,7 +283,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do rows = service.fresh_authorizations.to_a expect(rows.length).to eq(1) - expect(rows.first.access_level).to eq(Gitlab::Access::OWNER) + expect(rows.first.access_level).to eq(Gitlab::Access::MAINTAINER) end context 'every returned row' do @@ -294,7 +294,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do end it 'includes the access level' do - expect(row.access_level).to eq(Gitlab::Access::OWNER) + expect(row.access_level).to eq(Gitlab::Access::MAINTAINER) end end end diff --git a/spec/services/members/projects/creator_service_spec.rb b/spec/services/members/projects/creator_service_spec.rb index 7ba183759bc..c6917a21bcd 100644 --- a/spec/services/members/projects/creator_service_spec.rb +++ b/spec/services/members/projects/creator_service_spec.rb @@ -9,8 +9,8 @@ RSpec.describe Members::Projects::CreatorService do end describe '.access_levels' do - it 'returns Gitlab::Access.sym_options_with_owner' do - expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner) + it 'returns Gitlab::Access.sym_options' do + expect(described_class.access_levels).to eq(Gitlab::Access.sym_options) end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index d12b70d403a..9cbc16f0c95 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -3312,7 +3312,7 @@ RSpec.describe NotificationService, :mailer do describe "##{sym}" do subject(:notify!) { notification.send(sym, domain) } - it 'emails current watching maintainers and owners' do + it 'emails current watching maintainers' do expect(Notify).to receive(:"#{sym}_email").at_least(:once).and_call_original notify! @@ -3410,7 +3410,7 @@ RSpec.describe NotificationService, :mailer do reset_delivered_emails! end - it 'emails current watching maintainers and owners' do + it 'emails current watching maintainers' do notification.remote_mirror_update_failed(remote_mirror) should_only_email(u_maintainer1, u_maintainer2, u_owner) diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 41c82ef6a2f..43a9d69f3bb 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -116,34 +116,14 @@ RSpec.describe Projects::CreateService, '#execute' do end context 'user namespace' do - context 'when personal_project_owner_with_owner_access feature flag is enabled' do - it 'creates a project in user namespace' do - project = create_project(user, opts) - - expect(project).to be_valid - expect(project.first_owner).to eq(user) - expect(project.team.maintainers).not_to include(user) - expect(project.team.owners).to contain_exactly(user) - expect(project.namespace).to eq(user.namespace) - expect(project.project_namespace).to be_in_sync_with_project(project) - end - end - - context 'when personal_project_owner_with_owner_access feature flag is disabled' do - before do - stub_feature_flags(personal_project_owner_with_owner_access: false) - end - - it 'creates a project in user namespace' do - project = create_project(user, opts) + it 'creates a project in user namespace' do + project = create_project(user, opts) - expect(project).to be_valid - expect(project.first_owner).to eq(user) - expect(project.team.maintainers).to contain_exactly(user) - expect(project.team.owners).to contain_exactly(user) - expect(project.namespace).to eq(user.namespace) - expect(project.project_namespace).to be_in_sync_with_project(project) - end + expect(project).to be_valid + expect(project.first_owner).to eq(user) + expect(project.team.maintainers).to include(user) + expect(project.namespace).to eq(user.namespace) + expect(project.project_namespace).to be_in_sync_with_project(project) end end @@ -182,7 +162,7 @@ RSpec.describe Projects::CreateService, '#execute' do expect(project).to be_persisted expect(project.owner).to eq(user) expect(project.first_owner).to eq(user) - expect(project.team.owners).to contain_exactly(user) + expect(project.team.maintainers).to contain_exactly(user) expect(project.namespace).to eq(user.namespace) expect(project.project_namespace).to be_in_sync_with_project(project) end diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index b8fd2455445..a31902c7f16 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -52,7 +52,7 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do it 'is called' do ProjectAuthorization.delete_all - expect(callback).to receive(:call).with(project.id, Gitlab::Access::OWNER).once + expect(callback).to receive(:call).with(project.id, Gitlab::Access::MAINTAINER).once service.execute end @@ -73,7 +73,7 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do to_be_removed = [project_authorization.project_id] to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER } + { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } ] expect(service).to receive(:update_authorizations) @@ -83,14 +83,14 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do end it 'removes duplicate entries' do - [Gitlab::Access::OWNER, Gitlab::Access::REPORTER].each do |access_level| + [Gitlab::Access::MAINTAINER, Gitlab::Access::REPORTER].each do |access_level| user.project_authorizations.create!(project: project, access_level: access_level) end to_be_removed = [project.id] to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER } + { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } ] expect(service).to( receive(:update_authorizations) @@ -103,7 +103,7 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do project_authorization = ProjectAuthorization.where( project_id: project.id, user_id: user.id, - access_level: Gitlab::Access::OWNER) + access_level: Gitlab::Access::MAINTAINER) expect(project_authorization).to exist end @@ -116,7 +116,7 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do to_be_removed = [project_authorization.project_id] to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER } + { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } ] expect(service).to receive(:update_authorizations) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index f823683c274..37e9ef1d994 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -294,8 +294,6 @@ RSpec.configure do |config| # See https://gitlab.com/gitlab-org/gitlab/-/issues/33867 stub_feature_flags(file_identifier_hash: false) - stub_feature_flags(diffs_virtual_scrolling: false) - # The following `vue_issues_list` stub can be removed # once the Vue issues page has feature parity with the current Haml page stub_feature_flags(vue_issues_list: false) diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb index 30afde7efed..7515c789add 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -1,8 +1,11 @@ # frozen_string_literal: true module MergeRequestDiffHelpers + PageEndReached = Class.new(StandardError) + def click_diff_line(line_holder, diff_side = nil) line = get_line_components(line_holder, diff_side) + scroll_to_elements_bottom(line_holder) line_holder.hover line[:num].find('.js-add-diff-note-button').click end @@ -27,4 +30,55 @@ module MergeRequestDiffHelpers line_holder.find('.diff-line-num', match: :first) { content: line_holder.all('.line_content')[side_index], num: line_holder.all('.diff-line-num')[side_index] } end + + def has_reached_page_end + evaluate_script("(window.innerHeight + window.scrollY) >= document.body.offsetHeight") + end + + def scroll_to_elements_bottom(element) + evaluate_script("(function(el) { + window.scrollBy(0, el.getBoundingClientRect().bottom - window.innerHeight); + })(arguments[0]);", element.native) + end + + # we're not using Capybara's .obscured here because it also checks if the element is clickable + def within_viewport?(element) + evaluate_script("(function(el) { + var rect = el.getBoundingClientRect(); + return ( + rect.bottom >= 0 && + rect.right >= 0 && + rect.top <= (window.innerHeight || document.documentElement.clientHeight) && + rect.left <= (window.innerWidth || document.documentElement.clientWidth) + ); + })(arguments[0]);", element.native) + end + + def find_within_viewport(selector, **options) + begin + element = find(selector, **options, wait: 2) + rescue Capybara::ElementNotFound + return + end + return element if within_viewport?(element) + + nil + end + + def find_by_scrolling(selector, **options) + element = find_within_viewport(selector, **options) + return element if element + + page.execute_script "window.scrollTo(0,0)" + until element + + if has_reached_page_end + raise PageEndReached, "Failed to find any elements matching a selector '#{selector}' by scrolling. Page end reached." + end + + page.execute_script "window.scrollBy(0,window.innerHeight/1.5)" + element = find_within_viewport(selector, **options) + end + element + end end diff --git a/spec/support/helpers/note_interaction_helpers.rb b/spec/support/helpers/note_interaction_helpers.rb index a4322618cd3..fa2705a64fa 100644 --- a/spec/support/helpers/note_interaction_helpers.rb +++ b/spec/support/helpers/note_interaction_helpers.rb @@ -1,8 +1,10 @@ # frozen_string_literal: true module NoteInteractionHelpers + include MergeRequestDiffHelpers + def open_more_actions_dropdown(note) - note_element = find("#note_#{note.id}") + note_element = find_by_scrolling("#note_#{note.id}") note_element.find('.more-actions-toggle').click note_element.find('.more-actions .dropdown-menu li', match: :first) diff --git a/spec/support/shared_examples/features/comments_on_merge_request_files_shared_examples.rb b/spec/support/shared_examples/features/comments_on_merge_request_files_shared_examples.rb index e0a032b1a43..8a07e52019c 100644 --- a/spec/support/shared_examples/features/comments_on_merge_request_files_shared_examples.rb +++ b/spec/support/shared_examples/features/comments_on_merge_request_files_shared_examples.rb @@ -2,7 +2,7 @@ RSpec.shared_examples 'comment on merge request file' do it 'adds a comment' do - click_diff_line(find("[id='#{sample_commit.line_code}']")) + click_diff_line(find_by_scrolling("[id='#{sample_commit.line_code}']")) page.within('.js-discussion-note-form') do fill_in(:note_note, with: 'Line is wrong') diff --git a/spec/support/shared_examples/namespaces/traversal_scope_examples.rb b/spec/support/shared_examples/namespaces/traversal_scope_examples.rb index b43b7946e69..bcb5464ed5b 100644 --- a/spec/support/shared_examples/namespaces/traversal_scope_examples.rb +++ b/spec/support/shared_examples/namespaces/traversal_scope_examples.rb @@ -299,4 +299,51 @@ RSpec.shared_examples 'namespace traversal scopes' do include_examples '.self_and_descendant_ids' end end + + shared_examples '.self_and_hierarchy' do + let(:base_scope) { Group.where(id: base_groups) } + + subject { base_scope.self_and_hierarchy } + + context 'with ancestors only' do + let(:base_groups) { [group_1, group_2] } + + it { is_expected.to match_array(groups) } + end + + context 'with descendants only' do + let(:base_groups) { [deep_nested_group_1, deep_nested_group_2] } + + it { is_expected.to match_array(groups) } + end + + context 'nodes with both ancestors and descendants' do + let(:base_groups) { [nested_group_1, nested_group_2] } + + it { is_expected.to match_array(groups) } + end + + context 'with duplicate base groups' do + let(:base_groups) { [nested_group_1, nested_group_1] } + + it { is_expected.to contain_exactly(group_1, nested_group_1, deep_nested_group_1) } + end + end + + describe '.self_and_hierarchy' do + it_behaves_like '.self_and_hierarchy' + + context "use_traversal_ids_for_self_and_hierarchy_scopes feature flag is false" do + before do + stub_feature_flags(use_traversal_ids_for_self_and_hierarchy_scopes: false) + end + + it_behaves_like '.self_and_hierarchy' + + it 'make recursive queries' do + base_groups = Group.where(id: nested_group_1) + expect { base_groups.self_and_hierarchy.load }.to make_queries_matching(/WITH RECURSIVE/) + end + end + end end diff --git a/spec/workers/container_registry/migration/guard_worker_spec.rb b/spec/workers/container_registry/migration/guard_worker_spec.rb index 480e8adbd5c..7d1df320d4e 100644 --- a/spec/workers/container_registry/migration/guard_worker_spec.rb +++ b/spec/workers/container_registry/migration/guard_worker_spec.rb @@ -26,11 +26,30 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do allow(::Gitlab).to receive(:com?).and_return(true) end + shared_examples 'not aborting any migration' do + it 'will not abort the migration' do + expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 1) + expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_stale_migrations_count, 0) + expect(worker).to receive(:log_extra_metadata_on_done).with(:long_running_stale_migration_container_repository_ids, [stale_migration.id]) + + expect { subject } + .to not_change(pre_importing_migrations, :count) + .and not_change(pre_import_done_migrations, :count) + .and not_change(importing_migrations, :count) + .and not_change(import_done_migrations, :count) + .and not_change(import_aborted_migrations, :count) + .and not_change { stale_migration.reload.migration_state } + .and not_change { ongoing_migration.migration_state } + end + end + context 'with no stale migrations' do it_behaves_like 'an idempotent worker' it 'will not update any migration state' do expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 0) + expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_stale_migrations_count, 0) + expect { subject } .to not_change(pre_importing_migrations, :count) .and not_change(pre_import_done_migrations, :count) @@ -41,10 +60,19 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do context 'with pre_importing stale migrations' do let(:ongoing_migration) { create(:container_repository, :pre_importing) } - let(:stale_migration) { create(:container_repository, :pre_importing, migration_pre_import_started_at: 10.minutes.ago) } + let(:stale_migration) { create(:container_repository, :pre_importing, migration_pre_import_started_at: 35.minutes.ago) } + let(:import_status) { 'test' } + + before do + allow_next_instance_of(ContainerRegistry::GitlabApiClient) do |client| + allow(client).to receive(:import_status).and_return(import_status) + end + end it 'will abort the migration' do expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 1) + expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_stale_migrations_count, 1) + expect { subject } .to change(pre_importing_migrations, :count).by(-1) .and not_change(pre_import_done_migrations, :count) @@ -54,18 +82,26 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do .and change { stale_migration.reload.migration_state }.from('pre_importing').to('import_aborted') .and not_change { ongoing_migration.migration_state } end + + context 'the client returns pre_import_in_progress' do + let(:import_status) { 'pre_import_in_progress' } + + it_behaves_like 'not aborting any migration' + end end context 'with pre_import_done stale migrations' do let(:ongoing_migration) { create(:container_repository, :pre_import_done) } - let(:stale_migration) { create(:container_repository, :pre_import_done, migration_pre_import_done_at: 10.minutes.ago) } + let(:stale_migration) { create(:container_repository, :pre_import_done, migration_pre_import_done_at: 35.minutes.ago) } before do allow(::ContainerRegistry::Migration).to receive(:max_step_duration).and_return(5.minutes) - expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 1) end it 'will abort the migration' do + expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 1) + expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_stale_migrations_count, 1) + expect { subject } .to not_change(pre_importing_migrations, :count) .and change(pre_import_done_migrations, :count).by(-1) @@ -79,14 +115,19 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do context 'with importing stale migrations' do let(:ongoing_migration) { create(:container_repository, :importing) } - let(:stale_migration) { create(:container_repository, :importing, migration_import_started_at: 10.minutes.ago) } + let(:stale_migration) { create(:container_repository, :importing, migration_import_started_at: 35.minutes.ago) } + let(:import_status) { 'test' } before do - allow(::ContainerRegistry::Migration).to receive(:max_step_duration).and_return(5.minutes) - expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 1) + allow_next_instance_of(ContainerRegistry::GitlabApiClient) do |client| + allow(client).to receive(:import_status).and_return(import_status) + end end it 'will abort the migration' do + expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 1) + expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_stale_migrations_count, 1) + expect { subject } .to not_change(pre_importing_migrations, :count) .and not_change(pre_import_done_migrations, :count) @@ -96,6 +137,12 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do .and change { stale_migration.reload.migration_state }.from('importing').to('import_aborted') .and not_change { ongoing_migration.migration_state } end + + context 'the client returns import_in_progress' do + let(:import_status) { 'import_in_progress' } + + it_behaves_like 'not aborting any migration' + end end end |