diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js index a05be1f97671c7fe26e3db4d8690e9c285b71105..2b553d160d0967196dcb0fe676ba86b8a76efce3 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js @@ -23,6 +23,10 @@ import { lineExists, } from '~/diffs/utils/diff_file'; import { useNotes } from '~/notes/store/legacy_notes'; +import { + extractDiscussionIdFromEndpoint, + determineCollapseAction, +} from '~/notes/utils/discussion_collapse'; import { INLINE_DIFF_VIEW_TYPE, DIFF_VIEW_COOKIE_NAME, @@ -1270,3 +1274,55 @@ export function expandAllFiles() { export function collapseAllFiles() { this[types.SET_COLLAPSED_STATE_FOR_ALL_FILES]({ collapsed: true }); } + +/** + * Resolves a discussion with auto-collapse behavior for diffs context + * Reuses the core resolve logic from notes store, then applies diff-specific collapse + * @param {Object} params - { endpoint, isResolved, discussion } + */ +export function resolveDiscussionWithAutoCollapse({ endpoint, isResolved, discussion }) { + return useNotes() + .toggleResolveNote({ endpoint, isResolved, discussion }) + .then((result) => { + if (!isResolved && discussion) { + this.handleDiffAutoCollapse({ endpoint }); + } + return result; + }); +} + +/** + * Handles auto-collapse logic specific to diffs context + * @param {Object} params - { endpoint } + */ +export function handleDiffAutoCollapse({ endpoint }) { + const discussionId = extractDiscussionIdFromEndpoint(endpoint); + if (!discussionId) return; + + const { discussions } = useNotes(); + const collapseAction = determineCollapseAction(discussions, discussionId); + if (!collapseAction) return; + + const actionHandlers = { + line: (action) => { + this.toggleLineDiscussions({ + lineCode: action.lineCode, + fileHash: action.fileHash, + expanded: false, + }); + }, + file: (action) => { + action.discussions.forEach((discussion) => { + this.toggleFileDiscussion({ + discussion, + expandedOnDiff: false, + }); + }); + }, + }; + + const handler = actionHandlers[collapseAction.type]; + if (handler) { + handler(collapseAction); + } +} diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index 267e22e00afb4482bd581087a7be9c48e88b96a4..26daae6f208a9bfa084e8e36ac65e3401f7747de 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -83,7 +83,6 @@ export default { return { isReplying: false, isResolving: false, - // eslint-disable-next-line vue/no-unused-properties -- `resolveAsThread` is used by the `resolvable` mixin resolveAsThread: true, }; }, @@ -214,12 +213,14 @@ export default { ...mapActions(useNotes, [ 'saveNote', 'removePlaceholderNotes', - // eslint-disable-next-line vue/no-unused-properties -- toggleResolveNote() used by the `Resolvable` mixin 'toggleResolveNote', 'removeConvertedDiscussion', 'expandDiscussion', ]), ...mapActions(useLegacyDiffs, ['getLinesForDiscussion']), + resolveDiscussionWithAutoCollapse(...args) { + return useLegacyDiffs().resolveDiscussionWithAutoCollapse(...args); + }, showReplyForm(text) { this.isReplying = true; @@ -318,6 +319,41 @@ export default { this.showReplyForm(); } }, + // Override resolvable mixin's handler to use component-layer decision + resolveHandler(resolvedState = false) { + if (this.note && this.note.isDraft) { + return this.$emit('toggleResolveStatus'); + } + + this.isResolving = true; + const isResolved = this.discussionResolved || resolvedState; + const discussion = this.resolveAsThread; + let endpoint = + discussion && this.discussion ? this.discussion.resolve_path : `${this.note.path}/resolve`; + + if (this.discussionResolvePath) { + endpoint = this.discussionResolvePath; + } + + // Use diffs store for diffs tab, notes store for overview tab + const resolveAction = this.isOverviewTab + ? this.toggleResolveNote + : this.resolveDiscussionWithAutoCollapse; + + return resolveAction({ endpoint, isResolved, discussion }) + .then(() => { + this.isResolving = false; + }) + .catch(() => { + this.isResolving = false; + + const msg = __('Something went wrong while resolving this discussion. Please try again.'); + createAlert({ + message: msg, + parent: this.$el, + }); + }); + }, }, }; diff --git a/app/assets/javascripts/notes/store/legacy_notes/actions.js b/app/assets/javascripts/notes/store/legacy_notes/actions.js index 8d6814cc38e7e943b6f7c7168c444da8d46fdc11..07340fe34399ba20ea529b7ba5819d814af07d0b 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -425,6 +425,43 @@ export function toggleResolveNote({ endpoint, isResolved, discussion }) { }); } +export function handleAutoCollapse(collapseAction) { + const actionHandlers = { + line: (action) => { + this.collapseLineDiscussions(action.lineCode); + }, + + file: (action) => { + action.discussions.forEach((discussion) => { + this.toggleDiscussion({ + discussionId: discussion.id, + forceExpanded: false, + }); + }); + }, + + general: (action) => { + this.toggleDiscussion({ + discussionId: action.discussionId, + forceExpanded: false, + }); + }, + }; + + const handler = actionHandlers[collapseAction.type] || (() => {}); + handler(collapseAction); +} + +export function collapseLineDiscussions(lineCode) { + const lineDiscussions = this.discussions.filter((d) => d.line_code === lineCode); + lineDiscussions.forEach((discussion) => { + this.toggleDiscussion({ + discussionId: discussion.id, + forceExpanded: false, + }); + }); +} + export function closeIssuable() { this.toggleStateButtonLoading(true); return axios.put(this.notesData.closePath).then(({ data }) => { diff --git a/app/assets/javascripts/notes/utils/discussion_collapse.js b/app/assets/javascripts/notes/utils/discussion_collapse.js new file mode 100644 index 0000000000000000000000000000000000000000..a1ba3d11cbb0b91e3668abf2de175b21287525fe --- /dev/null +++ b/app/assets/javascripts/notes/utils/discussion_collapse.js @@ -0,0 +1,67 @@ +/** + * Extracts discussion ID from a resolve endpoint URL + * @param {string} endpoint - The resolve endpoint URL (e.g., "/discussions/abc123/resolve") + * @returns {string|null} - The discussion ID or null if not found + */ +export function extractDiscussionIdFromEndpoint(endpoint) { + if (!endpoint) { + return null; + } + + const match = endpoint.match(/discussions\/([^/]+)\/resolve$/); + return match ? match[1] : null; +} + +/** + * Determines what collapse action should be taken for a resolved discussion + * @param {Array} discussions - Array of all discussions + * @param {string} resolvedDiscussionId - The discussion that was just resolved + * @returns {Object|null} - Single collapse action or null + */ +export function determineCollapseAction(discussions, resolvedDiscussionId) { + const resolvedDiscussion = discussions.find((d) => d.id === resolvedDiscussionId); + let collapseAction = null; + + const isValidDiscussion = resolvedDiscussion && resolvedDiscussion.resolved; + const hasLineCode = isValidDiscussion && Boolean(resolvedDiscussion.line_code); + const hasFileHash = isValidDiscussion && Boolean(resolvedDiscussion.diff_file?.file_hash); + const isGeneralDiscussion = isValidDiscussion && !hasLineCode && !hasFileHash; + + if (hasLineCode) { + const lineDiscussions = discussions.filter((d) => d.line_code === resolvedDiscussion.line_code); + const allLineDiscussionsResolved = lineDiscussions.every((d) => d.resolved); + + collapseAction = allLineDiscussionsResolved + ? { + type: 'line', + lineCode: resolvedDiscussion.line_code, + fileHash: resolvedDiscussion.diff_file?.file_hash, + } + : null; + } + + if (!hasLineCode && hasFileHash) { + const fileHash = resolvedDiscussion.diff_file.file_hash; + const fileDiscussions = discussions.filter( + (d) => d.diff_file?.file_hash === fileHash && !d.line_code, + ); + const allFileDiscussionsResolved = fileDiscussions.every((d) => d.resolved); + + collapseAction = allFileDiscussionsResolved + ? { + type: 'file', + discussions: fileDiscussions, + fileHash, + } + : null; + } + + if (isGeneralDiscussion) { + collapseAction = { + type: 'general', + discussionId: resolvedDiscussion.id, + }; + } + + return collapseAction; +} diff --git a/qa/qa/page/merge_request/show.rb b/qa/qa/page/merge_request/show.rb index efa1e2dd1132a95681b39909cb9e12d5d96949a6..d39ca2dc7273cc450b0a490cc112ac40ad590539 100644 --- a/qa/qa/page/merge_request/show.rb +++ b/qa/qa/page/merge_request/show.rb @@ -551,6 +551,10 @@ def has_exposed_artifact_with_name?(name) has_link?(name) end + def expand_collapsed_discussions + all_elements('left-discussions', minimum: 1).each(&:click) + end + private def submit_commit diff --git a/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/batch_suggestion_spec.rb b/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/batch_suggestion_spec.rb index 2b86a7863e8c501125a900248522e78a19d1d3e3..ccd7f4ba6301f1a560e0c2bb81dcc72e5aa1fd78 100644 --- a/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/batch_suggestion_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/batch_suggestion_spec.rb @@ -42,6 +42,8 @@ module QA 4.times { merge_request.add_suggestion_to_batch } merge_request.apply_suggestion_with_message("Custom commit message") + merge_request.expand_collapsed_discussions + expect(merge_request).to have_suggestions_applied(4) end end diff --git a/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/custom_commit_suggestion_spec.rb b/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/custom_commit_suggestion_spec.rb index 7323a4bb3de45f082f4ea7b1723293483d33f59d..ff7a9b9b98273f7217f86f3dbab6d1b3431ba59b 100644 --- a/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/custom_commit_suggestion_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/custom_commit_suggestion_spec.rb @@ -38,6 +38,8 @@ module QA merge_request.click_diffs_tab merge_request.apply_suggestion_with_message(commit_message) + merge_request.expand_collapsed_discussions + expect(merge_request).to have_suggestions_applied merge_request.click_commits_tab diff --git a/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb index 854db4bb52ef485c26b33c35cefd49ef5af04de3..fdd5c9ead73c56069a3c3bc118c0a91cf1f7f891 100644 --- a/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb +++ b/spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe 'Resolve an open thread in a merge request by creating an issue', :js, feature_category: :team_planning do + include Features::NotesHelpers + let(:user) { create(:user) } let(:project) { create(:project, :repository, only_allow_merge_if_all_discussions_are_resolved: true) } let(:merge_request) { create(:merge_request, source_project: project) } @@ -43,6 +45,7 @@ def resolve_discussion_selector expect(page).to have_link('Create issue to resolve thread', href: new_project_issue_path(project, discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid, merge_request_id: merge_request.id)) click_button 'Resolve thread' + expand_all_collapsed_discussions expect(page).not_to have_link('Create issue to resolve thread', href: new_project_issue_path(project, discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid, merge_request_id: merge_request.id)) diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb index 8747479e197f639abff37a32932f72dce2657e40..8c9c7440db7dfa9996078df0d97fcc857fae1797 100644 --- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb +++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe 'Merge request > User resolves diff notes and threads', :js, feature_category: :code_review_workflow do + include MergeRequestDiffHelpers + let(:project) { create(:project, :public, :repository) } let(:user) { project.creator } let(:guest) { create(:user) } @@ -46,6 +48,7 @@ find_by_testid('resolve-line-button').click wait_for_requests + expand_all_collapsed_discussions expect(find_by_testid('resolve-line-button')['aria-label']).to eq("Resolved by #{user.name}") end @@ -74,6 +77,7 @@ it 'allows user to unresolve thread' do page.within '.diff-content' do find('button[data-testid="resolve-discussion-button"]').click + expand_all_collapsed_discussions click_button 'Reopen thread' end @@ -233,6 +237,7 @@ resolve_button.click wait_for_requests + expand_all_collapsed_discussions expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}") end @@ -256,6 +261,7 @@ first('[data-testid="resolve-line-button"]').click wait_for_requests + expand_all_collapsed_discussions expect(first('[data-testid="resolve-line-button"]')['aria-label']).to eq("Resolved by #{user.name}") end @@ -272,6 +278,7 @@ end wait_for_requests + expand_all_collapsed_discussions resolve_buttons.each do |button| expect(button['aria-label']).to eq("Resolved by #{user.name}") 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 dd9dc31c2d4f6a0f3ba4685de46361a05d7c06f5..b7aa8ecadf2c942e1ed8de6be2b04dab19d16769 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 @@ -105,7 +105,11 @@ def expect_suggestion_has_content(element, expected_changing_content, expected_s click_button('Apply suggestion') click_button('Apply') wait_for_requests + end + + expand_all_collapsed_discussions + page.within('.diff-discussions') do expect(page).to have_content('Applied') end end @@ -210,6 +214,8 @@ def hash(path) wait_for_requests end + expand_all_collapsed_discussions + expect(page).to have_content('Applied').twice end end @@ -272,7 +278,11 @@ def hash(path) all('button', text: 'Apply suggestion').first.click click_button('Apply') wait_for_requests + end + + expand_all_collapsed_discussions + page.within(container) do expect(page).to have_content('Applied').once expect(page).to have_button('Apply suggestion').once @@ -392,7 +402,11 @@ def hash(path) click_button('Apply suggestion') click_button('Apply') wait_for_requests + end + + expand_all_collapsed_discussions + page.within("[id='#{hash}']") do expect(page).to have_content('Applied') end end @@ -404,7 +418,11 @@ def hash(path) click_button('Apply suggestion') click_button('Apply') wait_for_requests + end + + expand_all_collapsed_discussions + page.within("[id='#{hash}']") do expect(page).to have_content('Reopen thread') end end diff --git a/spec/frontend/notes/store/legacy_notes/actions_spec.js b/spec/frontend/notes/store/legacy_notes/actions_spec.js index 238f7a6dd15b50722af48bd34799dae3a4ad7635..cce19c35841d047baca07fc5105977e0754af83f 100644 --- a/spec/frontend/notes/store/legacy_notes/actions_spec.js +++ b/spec/frontend/notes/store/legacy_notes/actions_spec.js @@ -642,6 +642,72 @@ describe('Actions Notes Store', () => { }); }); + describe('handleAutoCollapse', () => { + beforeEach(() => { + store.toggleDiscussion = jest.fn(); + store.collapseLineDiscussions = jest.fn(); + }); + + it.each` + type | action | expectedCall + ${'line'} | ${{ type: 'line', lineCode: 'abc_1_1', fileHash: 'file123' }} | ${['collapseLineDiscussions', 'abc_1_1']} + ${'general'} | ${{ type: 'general', discussionId: 'discussion1' }} | ${['toggleDiscussion', { discussionId: 'discussion1', forceExpanded: false }]} + `('handles $type action type', ({ action, expectedCall }) => { + store.handleAutoCollapse(action); + + expect(store[expectedCall[0]]).toHaveBeenCalledWith(expectedCall[1]); + }); + + it('handles file action type with multiple discussions', () => { + const fileAction = { + type: 'file', + discussions: [{ id: 'discussion1' }, { id: 'discussion2' }], + }; + + store.handleAutoCollapse(fileAction); + + expect(store.toggleDiscussion).toHaveBeenCalledTimes(2); + expect(store.toggleDiscussion).toHaveBeenCalledWith({ + discussionId: 'discussion1', + forceExpanded: false, + }); + expect(store.toggleDiscussion).toHaveBeenCalledWith({ + discussionId: 'discussion2', + forceExpanded: false, + }); + }); + + it('handles unknown action types gracefully', () => { + expect(() => store.handleAutoCollapse({ type: 'unknown' })).not.toThrow(); + }); + }); + + describe('collapseLineDiscussions', () => { + beforeEach(() => { + store.toggleDiscussion = jest.fn(); + }); + + it('collapses only discussions matching the line_code', () => { + store.discussions = [ + { id: 'discussion1', line_code: 'abc_1_1' }, + { id: 'discussion2', line_code: 'abc_1_1' }, + { id: 'discussion3', line_code: 'abc_2_2' }, + ]; + + store.collapseLineDiscussions('abc_1_1'); + + expect(store.toggleDiscussion).toHaveBeenCalledTimes(2); + expect(store.toggleDiscussion).toHaveBeenCalledWith({ + discussionId: 'discussion1', + forceExpanded: false, + }); + expect(store.toggleDiscussion).toHaveBeenCalledWith({ + discussionId: 'discussion2', + forceExpanded: false, + }); + }); + }); + describe('updateMergeRequestWidget', () => { it('calls mrWidget checkStatus', () => { jest.spyOn(mrWidgetEventHub, '$emit').mockImplementation(() => {}); diff --git a/spec/frontend/notes/utils/discussion_collapse_spec.js b/spec/frontend/notes/utils/discussion_collapse_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..5827b3dd2a8345d229722dbc2ba08d47979a3d4b --- /dev/null +++ b/spec/frontend/notes/utils/discussion_collapse_spec.js @@ -0,0 +1,211 @@ +import { + extractDiscussionIdFromEndpoint, + determineCollapseAction, +} from '~/notes/utils/discussion_collapse'; + +function createDiscussion(properties = {}) { + return { + id: 'discussion1', + resolved: true, + line_code: null, + diff_file: null, + ...properties, + }; +} + +function createLineDiscussion({ id, resolved = true, lineCode = 'abc_1_1', fileHash = 'file123' }) { + return createDiscussion({ + id, + resolved, + line_code: lineCode, + diff_file: { file_hash: fileHash }, + }); +} + +function createFileDiscussion({ id, resolved = true, fileHash = 'file123' }) { + return createDiscussion({ + id, + resolved, + line_code: null, + diff_file: { file_hash: fileHash }, + }); +} + +function createGeneralDiscussion({ id, resolved = true }) { + return createDiscussion({ + id, + resolved, + line_code: null, + diff_file: null, + }); +} + +describe('Discussion Collapse Utilities', () => { + describe('extractDiscussionIdFromEndpoint', () => { + it.each` + endpoint | expectedResult | description + ${'/discussions/abc123/resolve'} | ${'abc123'} | ${'returns discussion ID from valid endpoint'} + ${'/discussions/abc-123_xyz/resolve'} | ${'abc-123_xyz'} | ${'returns discussion ID with special characters'} + ${'/namespace/project/-/merge_requests/1/discussions/def456/resolve'} | ${'def456'} | ${'returns discussion ID from endpoint with leading path'} + ${'/discussions/abc123/unresolve'} | ${null} | ${'returns null for invalid endpoint format'} + ${'/discussions/abc123'} | ${null} | ${'returns null for endpoint without resolve suffix'} + ${null} | ${null} | ${'returns null for null input'} + ${undefined} | ${null} | ${'returns null for undefined input'} + ${''} | ${null} | ${'returns null for empty string'} + ${'/discussions//resolve'} | ${null} | ${'returns null for malformed endpoint'} + ${'/discussions/abc123/resolve/extra'} | ${null} | ${'returns null for endpoint with extra path segments'} + ${'/discussions/abc123/resolve?param=value'} | ${null} | ${'returns null for endpoint with query parameters'} + `('$description', ({ endpoint, expectedResult }) => { + expect(extractDiscussionIdFromEndpoint(endpoint)).toBe(expectedResult); + }); + }); + + describe('determineCollapseAction', () => { + describe('line discussions', () => { + it('returns line action when all discussions on a line are resolved', () => { + const discussions = [ + createLineDiscussion({ id: 'discussion1' }), + createLineDiscussion({ id: 'discussion2' }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'line', + lineCode: 'abc_1_1', + fileHash: 'file123', + }); + }); + + it('filters discussions by line_code and ignores other lines', () => { + const discussions = [ + createLineDiscussion({ id: 'discussion1', resolved: true, lineCode: 'abc_1_1' }), + createLineDiscussion({ id: 'discussion2', resolved: true, lineCode: 'abc_2_2' }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'line', + lineCode: 'abc_1_1', + fileHash: 'file123', + }); + }); + }); + + describe('file-level discussions', () => { + it('returns file action when all file discussions are resolved', () => { + const discussions = [ + createFileDiscussion({ id: 'discussion1' }), + createFileDiscussion({ id: 'discussion2' }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'file', + discussions: [discussions[0], discussions[1]], + fileHash: 'file123', + }); + }); + + it('filters file discussions by file_hash and excludes line discussions', () => { + const discussions = [ + createFileDiscussion({ id: 'discussion1', resolved: true, fileHash: 'file123' }), + createFileDiscussion({ id: 'discussion2', resolved: true, fileHash: 'file456' }), + createLineDiscussion({ + id: 'discussion3', + resolved: true, + lineCode: 'abc_1_1', + fileHash: 'file123', + }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'file', + discussions: [discussions[0]], + fileHash: 'file123', + }); + }); + }); + + describe('general discussions', () => { + it('returns general action for discussions without line_code or file_hash', () => { + const discussions = [createGeneralDiscussion({ id: 'discussion1' })]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'general', + discussionId: 'discussion1', + }); + }); + }); + + describe('edge cases', () => { + it('returns null when discussion is not found', () => { + const discussions = [createLineDiscussion({ id: 'discussion1' })]; + + expect(determineCollapseAction(discussions, 'nonexistent')).toBeNull(); + }); + + it('prioritizes line_code over file_hash when both exist', () => { + const discussions = [createLineDiscussion({ id: 'discussion1' })]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result.type).toBe('line'); + }); + + it('handles falsy values correctly for line_code and file_hash', () => { + const discussionsWithEmptyLineCode = [ + createDiscussion({ + id: 'discussion1', + line_code: '', + diff_file: { file_hash: 'file123' }, + }), + ]; + const discussionsWithEmptyFileHash = [ + createDiscussion({ + id: 'discussion2', + diff_file: { file_hash: '' }, + }), + ]; + const discussionsWithNullDiffFile = [ + createDiscussion({ + id: 'discussion3', + line_code: 'abc_1_1', + diff_file: null, + }), + ]; + const discussionsWithEmptyDiffFile = [ + createDiscussion({ + id: 'discussion4', + diff_file: {}, + }), + ]; + + expect(determineCollapseAction(discussionsWithEmptyLineCode, 'discussion1')).toEqual({ + type: 'file', + discussions: [discussionsWithEmptyLineCode[0]], + fileHash: 'file123', + }); + expect(determineCollapseAction(discussionsWithEmptyFileHash, 'discussion2')).toEqual({ + type: 'general', + discussionId: 'discussion2', + }); + expect(determineCollapseAction(discussionsWithNullDiffFile, 'discussion3')).toEqual({ + type: 'line', + lineCode: 'abc_1_1', + fileHash: undefined, + }); + expect(determineCollapseAction(discussionsWithEmptyDiffFile, 'discussion4')).toEqual({ + type: 'general', + discussionId: 'discussion4', + }); + }); + }); + }); +}); diff --git a/spec/support/helpers/features/notes_helpers.rb b/spec/support/helpers/features/notes_helpers.rb index 55c1a29bef84d96b628ff90d45b4cb3ab5ceae1f..869e6f9bce38487100d277d614aa327625f9f478 100644 --- a/spec/support/helpers/features/notes_helpers.rb +++ b/spec/support/helpers/features/notes_helpers.rb @@ -53,5 +53,22 @@ def preview_note(text, issuable_type = nil) yield if block_given? end end + + def expand_all_collapsed_discussions + max_expansions = 50 + expansions = 0 + + allowing_for_delay(interval: 0.5, retries: 3) do + while has_selector?('.js-diff-comment-avatar', wait: 1) && expansions < max_expansions + begin + first('.js-diff-comment-avatar', wait: 1, minimum: 1).click + wait_for_requests + expansions += 1 + rescue Selenium::WebDriver::Error::StaleElementReferenceError + raise Capybara::ExpectationNotMet + end + end + end + end end end diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb index bbd9382fcc2defe92c3fb4343aaea430c7a0371d..a24da0faee73ad16f6dce0e5b9fe3eca91ee2fb7 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -93,4 +93,21 @@ def find_by_scrolling(selector, **options) end element end + + def expand_all_collapsed_discussions + max_expansions = 50 + expansions = 0 + + allowing_for_delay(interval: 0.5, retries: 3) do + while has_selector?('.js-diff-comment-avatar', wait: 1) && expansions < max_expansions + begin + first('.js-diff-comment-avatar', wait: 1, minimum: 1).click + wait_for_requests + expansions += 1 + rescue Selenium::WebDriver::Error::StaleElementReferenceError + raise Capybara::ExpectationNotMet + end + end + end + end end