diff --git a/app/assets/javascripts/diffs/components/diff_discussions.vue b/app/assets/javascripts/diffs/components/diff_discussions.vue index d1a773eaf84c6910aa6998155a9100b8b7d7f550..b35c0a784d89682ba0a641f85e26fb7e9dc6cf87 100644 --- a/app/assets/javascripts/diffs/components/diff_discussions.vue +++ b/app/assets/javascripts/diffs/components/diff_discussions.vue @@ -43,7 +43,7 @@ export default { return this.shouldCollapseDiscussions ? discussion.expandedOnDiff : true; }, toggleVisibility(discussion) { - this.toggleFileDiscussion(discussion); + this.toggleFileDiscussion({ discussion }); }, }, }; diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index ac804eec1c8af7da4914b3312afa4c70a15861df..e18b5b15cdadea8f77450b1583f9438246685ced 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -444,7 +444,7 @@ export default { this.addToReview(note, this.$options.FILE_DIFF_POSITION_TYPE, parentElement, errorCallback); }, toggleFileDiscussionVisibility() { - this.collapsedFileDiscussions.forEach((d) => this.toggleFileDiscussion(d)); + this.collapsedFileDiscussions.forEach((d) => this.toggleFileDiscussion({ discussion: d })); }, }, warningClasses: [ diff --git a/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue b/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue index 60d985edd33a2c4833501bf4b0aba22802f7076e..5ecf90eaa048cd91833425c15efde64538e4c575 100644 --- a/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue +++ b/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue @@ -85,6 +85,7 @@ export default { pseudo class="diff-comment-avatar js-diff-comment-avatar" css-classes="gl-bg-default" + data-testid="discussion-avatar" @click.native="$emit('toggleLineDiscussions')" /> { + if (!isResolved && discussion) { + this.handleDiffAutoCollapse({ endpoint }); + } + return result; + }); +} + +/** + * Handles auto-collapse logic for resolved discussions on the Diffs/Changes tab + * Determines the appropriate collapse action based on discussion type: + * - Line discussions: Collapses all discussions on a line when all are resolved + * - File discussions: Collapses file-level discussions when all on that file are resolved + * - Image point discussions: Collapses immediately (treated as individual) + * Uses grouped collapse behavior (waits for all related discussions to be resolved). + * + * @param {Object} params - Parameters + * @param {string} params.endpoint - The resolve endpoint URL (used to extract discussion ID) + */ +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/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index 58cfd8dcd06c5580713bb99a786fa5a0e112bf25..7d0ec3ada07a1bc5acbd311c982e20a95164068b 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -8,6 +8,7 @@ import { renderGFM } from '~/behaviors/markdown/render_gfm'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { useMrNotes } from '~/mr_notes/store/legacy_mr_notes'; import { useNotes } from '~/notes/store/legacy_notes'; +import { getCollapseActionFromEndpoint } from '~/notes/utils/discussion_collapse'; import NoteAttachment from './note_attachment.vue'; import NoteAwardsList from './note_awards_list.vue'; import NoteEditedText from './note_edited_text.vue'; @@ -164,7 +165,9 @@ export default { 'submitSuggestionBatch', 'addSuggestionInfoToBatch', 'removeSuggestionInfoFromBatch', + 'handleAutoCollapse', ]), + ...mapActions(useLegacyDiffs, ['handleDiffAutoCollapse']), renderGFM() { renderGFM(this.$refs['note-body']); }, @@ -175,6 +178,34 @@ export default { formCancelHandler(shouldConfirm, isDirty) { this.$emit('cancelForm', { shouldConfirm, isDirty }); }, + triggerAutoCollapse(discussionId) { + const discussion = this.getDiscussion(discussionId); + if (!discussion?.resolve_path) return; + + // Check if diff files are loaded to determine if we're on the diffs tab. + // While generally we prefer component-layer context via props, in this case + // note_body is deeply nested and used in many places, making prop-passing + // costly. This state check is reliable and contained to this helper. + const diffsStore = useLegacyDiffs(); + const diffFileLoaded = diffsStore.diffFiles.some( + (f) => f.file_hash === discussion.diff_file?.file_hash, + ); + + if (diffFileLoaded) { + this.handleDiffAutoCollapse({ endpoint: discussion.resolve_path }); + return; + } + + const collapseAction = getCollapseActionFromEndpoint( + discussion.resolve_path, + useNotes().discussions, + false, + ); + + if (collapseAction) { + this.handleAutoCollapse(collapseAction); + } + }, applySuggestion({ suggestionId, flashContainer, callback = () => {}, message }) { const { discussion_id: discussionId, id: noteId } = this.note; @@ -184,10 +215,25 @@ export default { suggestionId, flashContainer, message, - }).then(callback); + }).then((result) => { + if (this.isDiffNote) { + this.triggerAutoCollapse(discussionId); + } + callback(); + return result; + }); }, applySuggestionBatch({ message, flashContainer }) { - return this.submitSuggestionBatch({ message, flashContainer }); + const batchInfo = [...this.batchSuggestionsInfo]; + + return this.submitSuggestionBatch({ message, flashContainer }).then((result) => { + if (this.isDiffNote) { + batchInfo.forEach(({ discussionId }) => { + this.triggerAutoCollapse(discussionId); + }); + } + return result; + }); }, addSuggestionToBatch(suggestionId) { const { discussion_id: discussionId, id: noteId } = this.note; diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index 708740ccd1decd4a89d05ce488f197bc839a9337..79925c6b083be0a75eb9bd1bb4ddd749f494a2ac 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -84,7 +84,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, }; }, @@ -215,12 +214,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; @@ -326,6 +327,41 @@ export default { this.showReplyForm(text); }); }, + // 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..a5f5976005a11756cd13da28319778a8f883e1c7 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -23,6 +23,7 @@ import { TYPENAME_NOTE } from '~/graphql_shared/constants'; import { useBatchComments } from '~/batch_comments/store'; import { uuids } from '~/lib/utils/uuids'; import notesEventHub from '../../event_hub'; +import { getCollapseActionFromEndpoint } from '../../utils/discussion_collapse'; import promoteTimelineEvent from '../../graphql/promote_timeline_event.mutation.graphql'; @@ -410,7 +411,15 @@ export function resolveDiscussion({ discussionId }) { }); } -export function toggleResolveNote({ endpoint, isResolved, discussion }) { +/** + * Toggles the resolved state of a note or discussion + * @param {Object} params - Parameters + * @param {string} params.endpoint - The resolve/unresolve endpoint + * @param {boolean} params.isResolved - Current resolved state + * @param {boolean} params.discussion - Whether this is a discussion (vs single note) + * @param {boolean} params.skipAutoCollapse - Whether to skip auto-collapse (used when diffs store handles it) + */ +export function toggleResolveNote({ endpoint, isResolved, discussion, skipAutoCollapse = false }) { const method = isResolved ? constants.UNRESOLVE_NOTE_METHOD_NAME : constants.RESOLVE_NOTE_METHOD_NAME; @@ -420,8 +429,51 @@ export function toggleResolveNote({ endpoint, isResolved, discussion }) { this[mutationType](data); this.updateResolvableDiscussionsCounts(); - this.updateMergeRequestWidget(); + + if (!isResolved && discussion && !skipAutoCollapse) { + const collapseAction = getCollapseActionFromEndpoint(endpoint, this.discussions, false); + if (collapseAction) { + this.handleAutoCollapse(collapseAction); + } + } + }); +} + +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, + }); }); } 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..d82f9434a3fcfb4bcc49b694fd7ed0d9be1c3d53 --- /dev/null +++ b/app/assets/javascripts/notes/utils/discussion_collapse.js @@ -0,0 +1,110 @@ +/** + * 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$/); + if (!match) return null; + + const id = match[1]; + const numericId = Number(id); + + return Number.isNaN(numericId) ? id : numericId; +} + +/** + * 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 + * @param {boolean} groupDiscussions - Whether to group related discussions (true for Diffs tab, false for Overview tab) + * @returns {Object|null} - Single collapse action or null + */ +export function determineCollapseAction( + discussions, + resolvedDiscussionId, + groupDiscussions = true, +) { + 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; + const isImagePointDiscussion = + isValidDiscussion && resolvedDiscussion.position?.position_type === 'image'; + + // On Overview tab, treat all diff discussions as individual (like general discussions) + if (!groupDiscussions && (hasLineCode || hasFileHash)) { + return { + type: 'general', + discussionId: resolvedDiscussion.id, + }; + } + + 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 (isImagePointDiscussion) { + collapseAction = { + type: 'file', + discussions: [resolvedDiscussion], + fileHash: resolvedDiscussion.diff_file?.file_hash, + }; + } + + if (!hasLineCode && hasFileHash && !isImagePointDiscussion) { + const fileHash = resolvedDiscussion.diff_file.file_hash; + const fileDiscussions = discussions.filter( + (d) => + d.diff_file?.file_hash === fileHash && + !d.line_code && + d.position?.position_type !== 'image', + ); + 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; +} + +/** + * Gets collapse action from endpoint and discussions list + * @param {string} endpoint - The resolve endpoint URL + * @param {Array} discussions - Array of all discussions + * @param {boolean} groupDiscussions - Whether to group related discussions (true for Diffs tab, false for Overview tab) + * @returns {Object|null} - Collapse action or null + */ +export function getCollapseActionFromEndpoint(endpoint, discussions, groupDiscussions = true) { + const discussionId = extractDiscussionIdFromEndpoint(endpoint); + return discussionId ? determineCollapseAction(discussions, discussionId, groupDiscussions) : null; +} diff --git a/ee/app/assets/javascripts/diffs/mixins/image_diff.js b/ee/app/assets/javascripts/diffs/mixins/image_diff.js index 5c1bd649973fefc4fd1b931ff8ac370e4f1b625a..9d9d6c58ea19f6df8fa888dfe4ca1ada18c04b07 100644 --- a/ee/app/assets/javascripts/diffs/mixins/image_diff.js +++ b/ee/app/assets/javascripts/diffs/mixins/image_diff.js @@ -10,7 +10,7 @@ export default { methods: { ...mapActions(useLegacyDiffs, ['toggleFileDiscussion']), clickedToggle(discussion) { - this.toggleFileDiscussion(discussion); + this.toggleFileDiscussion({ discussion }); }, toggleText(discussion, index) { const count = index + 1; diff --git a/qa/qa/page/merge_request/show.rb b/qa/qa/page/merge_request/show.rb index 3da83424b1fd153bfcfae37d5cdaa4e95ae0312c..49db769139f5c36b3a8e36b99070efd667cd19fd 100644 --- a/qa/qa/page/merge_request/show.rb +++ b/qa/qa/page/merge_request/show.rb @@ -32,6 +32,10 @@ class Show < Page::Base element 'diff-tree-search' end + view 'app/assets/javascripts/diffs/components/diff_gutter_avatars.vue' do + element 'discussion-avatar' + end + view 'app/assets/javascripts/diffs/components/diff_file_header.vue' do element 'file-title-container' element 'options-dropdown-button' @@ -551,6 +555,26 @@ def has_exposed_artifact_with_name?(name) has_link?(name) end + def expand_collapsed_discussions + loop do + expand_button = all_elements('expand-replies-button', minimum: 0).first + break unless expand_button + + expand_button.click + wait_for_requests + end + + loop do + avatar = all_elements('discussion-avatar', minimum: 0) + .find { |el| el[:class].exclude?('diff-notes-collapse') } + + break unless avatar + + avatar.click + wait_for_requests + end + 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..289744e5bdc649e713130c7bc00751f7e5db63a7 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 @@ -43,9 +43,13 @@ 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' + end + + 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)) - 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)) + click_button '1 reply' + within_testid('reply-wrapper') do click_button 'Reopen thread' 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)) 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..e2dbe55264314429ce911121c7263198f4aae682 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) } @@ -44,9 +46,11 @@ it 'allows user to mark a note as resolved' do page.within '.diff-content .note' do find_by_testid('resolve-line-button').click + end - wait_for_requests + expand_auto_collapsed_replies + page.within '.diff-content .note' do expect(find_by_testid('resolve-line-button')['aria-label']).to eq("Resolved by #{user.name}") end @@ -74,6 +78,11 @@ it 'allows user to unresolve thread' do page.within '.diff-content' do find('button[data-testid="resolve-discussion-button"]').click + end + + expand_auto_collapsed_replies + + page.within '.diff-content' do click_button 'Reopen thread' end @@ -125,7 +134,7 @@ end it 'shows resolved thread when toggled', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/327439' do - find('.diff-comment-avatar-holders').click + expand_auto_collapsed_replies expect(find('.diffs .diff-file .notes_holder')).to be_visible end @@ -229,11 +238,13 @@ it 'updates updated text after resolving note' do page.within '.diff-content .note' do - resolve_button = find_by_testid('resolve-line-button') + find_by_testid('resolve-line-button').click + end - resolve_button.click - wait_for_requests + expand_auto_collapsed_replies + page.within '.diff-content .note' do + resolve_button = find_by_testid('resolve-line-button') expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}") end end @@ -254,9 +265,11 @@ it 'marks thread as resolved when resolving single note' do page.within("#note_#{note.id}") do first('[data-testid="resolve-line-button"]').click + end - wait_for_requests + expand_auto_collapsed_replies + page.within("#note_#{note.id}") do expect(first('[data-testid="resolve-line-button"]')['aria-label']).to eq("Resolved by #{user.name}") end @@ -266,15 +279,15 @@ end it 'resolves thread' do - resolve_buttons = page.all('.note [data-testid="resolve-line-button"]', count: 1) - resolve_buttons.each do |button| + page.all('.note [data-testid="resolve-line-button"]', count: 1).each do |button| button.click end wait_for_requests + expand_auto_collapsed_replies - resolve_buttons.each do |button| - expect(button['aria-label']).to eq("Resolved by #{user.name}") + page.all('.note [data-testid="resolve-line-button"]').each do + expect(page).to have_css("button[aria-label='Resolved by #{user.name}']") end page.within(first('.discussions-counter')) do @@ -339,11 +352,13 @@ it 'updates updated text after resolving note' do page.within('.diff-content .note', match: :first) do - resolve_button = find_by_testid('resolve-line-button') + find_by_testid('resolve-line-button').click + end - resolve_button.click - wait_for_requests + expand_auto_collapsed_replies + page.within('.diff-content .note', match: :first) do + resolve_button = find_by_testid('resolve-line-button') expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}") end end @@ -385,6 +400,8 @@ find_by_testid('resolve-line-button').click end + expand_auto_collapsed_replies + page.within '.diff-content' do expect(page).to have_selector('.btn', text: 'Reopen thread') end @@ -407,6 +424,11 @@ it 'allows user to unresolve thread' do page.within '.diff-content' do find('button[data-testid="resolve-discussion-button"]').click + end + + expand_auto_collapsed_replies + + page.within '.diff-content' do click_button 'Reopen thread' end @@ -434,9 +456,11 @@ it 'allows user to comment & unresolve thread' do page.within '.diff-content' do find('button[data-testid="resolve-discussion-button"]').click + end - wait_for_requests + expand_auto_collapsed_replies + page.within '.diff-content' do find_field('Reply…').click find('.js-unresolve-checkbox') @@ -492,6 +516,8 @@ find_by_testid('resolve-line-button').click end + expand_auto_collapsed_replies + page.within '.diff-content' do expect(page).to have_selector('.btn', text: 'Reopen thread') end 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/diffs/components/diff_discussions_spec.js b/spec/frontend/diffs/components/diff_discussions_spec.js index 89170bbb42743c130e501f57f51a24ada2a872b1..f97f87a2f2a843ed1f9fda1c339905081c553d59 100644 --- a/spec/frontend/diffs/components/diff_discussions_spec.js +++ b/spec/frontend/diffs/components/diff_discussions_spec.js @@ -72,7 +72,9 @@ describe('DiffDiscussions', () => { findDiffNotesToggle().trigger('click'); - expect(useLegacyDiffs().toggleFileDiscussion).toHaveBeenCalledWith(discussions[0]); + expect(useLegacyDiffs().toggleFileDiscussion).toHaveBeenCalledWith({ + discussion: discussions[0], + }); }); it('renders expand button when discussion is collapsed', () => { diff --git a/spec/frontend/diffs/components/image_diff_overlay_spec.js b/spec/frontend/diffs/components/image_diff_overlay_spec.js index 50b9ab6b56c00d899e972e9369b0404486f9c8d9..ed8841e7c7f227d2c75db949a824a7959a0c3434 100644 --- a/spec/frontend/diffs/components/image_diff_overlay_spec.js +++ b/spec/frontend/diffs/components/image_diff_overlay_spec.js @@ -115,12 +115,14 @@ describe('Diffs image diff overlay component', () => { getAllImageBadges().at(0).vm.$emit('click'); expect(useLegacyDiffs().toggleFileDiscussion).toHaveBeenCalledWith({ - id: '1', - position: { - height: 200, - width: 100, - x: 10, - y: 10, + discussion: { + id: '1', + position: { + height: 200, + width: 100, + x: 10, + y: 10, + }, }, }); }); diff --git a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js index 542bce7334a4762bc2d2bbb0ad004c3b42aa2666..c4f0b0a9ed075e23947e3ff875708f4eab294220 100644 --- a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js +++ b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js @@ -2786,4 +2786,72 @@ describe('legacyDiffs actions', () => { return testAction(store.setCurrentFileHash, 'note_filehash', {}, [], []); }); }); + + describe('resolveDiscussionWithAutoCollapse', () => { + let notesStore; + + beforeEach(() => { + notesStore = useNotes(); + notesStore.toggleResolveNote = jest.fn().mockResolvedValue({}); + store.handleDiffAutoCollapse = jest.fn(); + }); + + it.each` + isResolved | discussion | expectedCalls | description + ${false} | ${true} | ${1} | ${'calls handleDiffAutoCollapse when resolving a discussion'} + ${true} | ${true} | ${0} | ${'does not call handleDiffAutoCollapse when unresolving'} + ${false} | ${false} | ${0} | ${'does not call handleDiffAutoCollapse when resolving a single note'} + `('$description', async ({ isResolved, discussion, expectedCalls }) => { + const endpoint = '/discussions/discussion1/resolve'; + + await store.resolveDiscussionWithAutoCollapse({ endpoint, isResolved, discussion }); + + expect(notesStore.toggleResolveNote).toHaveBeenCalledWith({ + endpoint, + isResolved, + discussion, + skipAutoCollapse: true, + }); + expect(store.handleDiffAutoCollapse).toHaveBeenCalledTimes(expectedCalls); + }); + }); + + describe('handleDiffAutoCollapse', () => { + let notesStore; + + beforeEach(() => { + notesStore = useNotes(); + store.toggleLineDiscussions = jest.fn(); + store.toggleFileDiscussion = jest.fn(); + }); + + it.each` + discussionType | setupDiscussions | expectedAction | expectedCall + ${'line'} | ${[{ id: 'disc1', resolved: true, line_code: 'abc_1_1', diff_file: { file_hash: 'file123' } }]} | ${'toggleLineDiscussions'} | ${{ lineCode: 'abc_1_1', fileHash: 'file123', expanded: false }} + ${'file'} | ${[{ id: 'disc1', resolved: true, line_code: null, diff_file: { file_hash: 'file123' } }]} | ${'toggleFileDiscussion'} | ${{ discussion: expect.any(Object), expandedOnDiff: false }} + ${'image'} | ${[{ id: 'disc1', resolved: true, line_code: null, diff_file: { file_hash: 'file123' }, position: { position_type: 'image' } }]} | ${'toggleFileDiscussion'} | ${{ discussion: expect.any(Object), expandedOnDiff: false }} + `( + 'handles $discussionType discussions correctly', + async ({ setupDiscussions, expectedAction, expectedCall }) => { + notesStore.discussions = setupDiscussions; + + await store.handleDiffAutoCollapse({ endpoint: '/discussions/disc1/resolve' }); + + expect(store[expectedAction]).toHaveBeenCalledWith(expectedCall); + }, + ); + + it.each` + endpoint | discussions | description + ${'/invalid'} | ${[]} | ${'does nothing when endpoint is invalid'} + ${'/discussions/disc1/resolve'} | ${[{ id: 'disc1', resolved: true, line_code: 'abc_1_1', diff_file: { file_hash: 'file123' } }, { id: 'disc2', resolved: false, line_code: 'abc_1_1', diff_file: { file_hash: 'file123' } }]} | ${'does nothing when not all line discussions are resolved'} + `('$description', async ({ endpoint, discussions }) => { + notesStore.discussions = discussions; + + await store.handleDiffAutoCollapse({ endpoint }); + + expect(store.toggleLineDiscussions).not.toHaveBeenCalled(); + expect(store.toggleFileDiscussion).not.toHaveBeenCalled(); + }); + }); }); diff --git a/spec/frontend/notes/components/note_body_spec.js b/spec/frontend/notes/components/note_body_spec.js index b6f90b3a5e0bd4eebc722458216bd5fe655dcd7d..596a5453f7faccbb176d9e8dbfc4bf46939d0b3c 100644 --- a/spec/frontend/notes/components/note_body_spec.js +++ b/spec/frontend/notes/components/note_body_spec.js @@ -41,6 +41,7 @@ describe('issue_note_body component', () => { useLegacyDiffs(); useNotes().setNoteableData(noteableDataMock); useNotes().setNotesData(notesDataMock); + useNotes().discussions = []; useMrNotes(); createComponent(); }); @@ -139,6 +140,149 @@ describe('issue_note_body component', () => { }); }); + describe('applying suggestions that trigger thread auto-collapse', () => { + let diffsStore; + let notesStore; + let handleDiffAutoCollapse; + let handleAutoCollapse; + let submitSuggestion; + let submitSuggestionBatch; + + const createDiffNote = () => ({ + ...note, + type: 'DiffNote', + discussion_id: 'discussion1', + suggestions: [{ id: 'suggestion1' }], + }); + + const setupDiscussion = (resolvePathExists = true) => { + const discussion = { + id: 'discussion1', + resolved: true, + resolve_path: resolvePathExists ? '/discussions/discussion1/resolve' : null, + diff_file: { file_hash: 'file123' }, + notes: [], + }; + notesStore.discussions = [discussion]; + + return discussion; + }; + + beforeEach(() => { + diffsStore = useLegacyDiffs(); + notesStore = useNotes(); + + handleDiffAutoCollapse = jest.fn(); + handleAutoCollapse = jest.fn(); + submitSuggestion = jest.fn().mockResolvedValue({}); + submitSuggestionBatch = jest.fn().mockResolvedValue({}); + + diffsStore.handleDiffAutoCollapse = handleDiffAutoCollapse; + notesStore.handleAutoCollapse = handleAutoCollapse; + notesStore.submitSuggestion = submitSuggestion; + notesStore.submitSuggestionBatch = submitSuggestionBatch; + + setupDiscussion(); + }); + + describe('triggerAutoCollapse', () => { + it.each` + diffFilesLoaded | expectedDiffsCalls | expectedNotesCalls | description + ${true} | ${1} | ${0} | ${'calls diffs store handleDiffAutoCollapse when diff files are loaded'} + ${false} | ${0} | ${1} | ${'calls notes store handleAutoCollapse when diff files are not loaded'} + `('$description', ({ diffFilesLoaded, expectedDiffsCalls, expectedNotesCalls }) => { + diffsStore.diffFiles = diffFilesLoaded ? [{ file_hash: 'file123' }] : []; + + createComponent({ note: createDiffNote(), file: { file_path: 'test.js' } }); + + wrapper.vm.triggerAutoCollapse('discussion1'); + + expect(handleDiffAutoCollapse).toHaveBeenCalledTimes(expectedDiffsCalls); + expect(handleAutoCollapse).toHaveBeenCalledTimes(expectedNotesCalls); + }); + + it('does nothing when discussion has no resolve_path', () => { + setupDiscussion(false); + + createComponent({ note: createDiffNote(), file: { file_path: 'test.js' } }); + + wrapper.vm.triggerAutoCollapse('discussion1'); + + expect(handleDiffAutoCollapse).not.toHaveBeenCalled(); + expect(handleAutoCollapse).not.toHaveBeenCalled(); + }); + }); + + describe('applySuggestion', () => { + it('calls triggerAutoCollapse for DiffNote after successful suggestion application', async () => { + createComponent({ note: createDiffNote(), file: { file_path: 'test.js' } }); + const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); + + await wrapper.vm.applySuggestion({ + suggestionId: 'suggestion1', + flashContainer: null, + callback: jest.fn(), + message: '', + }); + + expect(submitSuggestion).toHaveBeenCalled(); + expect(spy).toHaveBeenCalledWith('discussion1'); + }); + + it('does not call triggerAutoCollapse for non-DiffNote', async () => { + const regularNote = { + ...note, + type: 'DiscussionNote', + suggestions: [{ id: 'suggestion1' }], + }; + createComponent({ note: regularNote, file: { file_path: 'test.js' } }); + const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); + + await wrapper.vm.applySuggestion({ + suggestionId: 'suggestion1', + flashContainer: null, + callback: jest.fn(), + message: '', + }); + + expect(submitSuggestion).toHaveBeenCalled(); + expect(spy).not.toHaveBeenCalled(); + }); + }); + + describe('applySuggestionBatch', () => { + beforeEach(() => { + notesStore.batchSuggestionsInfo = [ + { discussionId: 'discussion1' }, + { discussionId: 'discussion2' }, + ]; + }); + + it('calls triggerAutoCollapse for each discussion in batch for DiffNote', async () => { + createComponent({ note: createDiffNote(), file: { file_path: 'test.js' } }); + const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); + + await wrapper.vm.applySuggestionBatch({ message: '', flashContainer: null }); + + expect(submitSuggestionBatch).toHaveBeenCalled(); + expect(spy).toHaveBeenCalledTimes(2); + expect(spy).toHaveBeenCalledWith('discussion1'); + expect(spy).toHaveBeenCalledWith('discussion2'); + }); + + it('does not call triggerAutoCollapse for non-DiffNote', async () => { + const regularNote = { ...note, type: 'DiscussionNote' }; + createComponent({ note: regularNote, file: { file_path: 'test.js' } }); + const spy = jest.spyOn(wrapper.vm, 'triggerAutoCollapse'); + + await wrapper.vm.applySuggestionBatch({ message: '', flashContainer: null }); + + expect(submitSuggestionBatch).toHaveBeenCalled(); + expect(spy).not.toHaveBeenCalled(); + }); + }); + }); + describe('duo code review feedback', () => { it.each` userType | type | exists | existsText diff --git a/spec/frontend/notes/store/legacy_notes/actions_spec.js b/spec/frontend/notes/store/legacy_notes/actions_spec.js index 238f7a6dd15b50722af48bd34799dae3a4ad7635..501765df0a460caaa5b5857804c1b680727c66fc 100644 --- a/spec/frontend/notes/store/legacy_notes/actions_spec.js +++ b/spec/frontend/notes/store/legacy_notes/actions_spec.js @@ -640,6 +640,100 @@ describe('Actions Notes Store', () => { ); }); }); + + describe('with auto-collapse', () => { + const endpoint = `${TEST_HOST}/discussions/1/resolve`; + const result = { resolved: true, expanded: true, id: 1 }; + + beforeEach(() => { + axiosMock.onPut(endpoint).reply(HTTP_STATUS_OK, result); + store.handleAutoCollapse = jest.fn(); + store.discussions = [ + { + id: 1, + resolved: false, + resolve_path: endpoint, + }, + ]; + }); + + it.each` + isResolved | skipAutoCollapse | expectedCalls | description + ${false} | ${false} | ${1} | ${'calls handleAutoCollapse when resolving discussion'} + ${false} | ${true} | ${0} | ${'does not call handleAutoCollapse when skipAutoCollapse is true'} + ${true} | ${false} | ${0} | ${'does not call handleAutoCollapse when unresolving'} + `('$description', async ({ isResolved, skipAutoCollapse, expectedCalls }) => { + await store.toggleResolveNote({ endpoint, isResolved, discussion: true, skipAutoCollapse }); + + expect(store.handleAutoCollapse).toHaveBeenCalledTimes(expectedCalls); + }); + }); + }); + + 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', () => { 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..5cac70c750c43ab094b68adf822441e6ecb22843 --- /dev/null +++ b/spec/frontend/notes/utils/discussion_collapse_spec.js @@ -0,0 +1,309 @@ +import { + extractDiscussionIdFromEndpoint, + determineCollapseAction, + getCollapseActionFromEndpoint, +} 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('image point discussions', () => { + function createImagePointDiscussion({ id, resolved = true, fileHash = 'file123' }) { + return createDiscussion({ + id, + resolved, + line_code: null, + diff_file: { file_hash: fileHash }, + position: { position_type: 'image' }, + }); + } + + it('collapses image point discussions immediately as individual file actions', () => { + const discussions = [ + createImagePointDiscussion({ id: 'discussion1', resolved: true }), + createFileDiscussion({ id: 'discussion2', resolved: false }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'file', + discussions: [discussions[0]], + fileHash: 'file123', + }); + }); + + it('excludes image point discussions from file-level discussion grouping', () => { + const discussions = [ + createFileDiscussion({ id: 'discussion1', resolved: true }), + createImagePointDiscussion({ id: 'discussion2', resolved: false }), + ]; + + const result = determineCollapseAction(discussions, 'discussion1'); + + expect(result).toEqual({ + type: 'file', + discussions: [discussions[0]], + fileHash: 'file123', + }); + }); + }); + + describe('groupDiscussions parameter', () => { + it.each` + groupDiscussions | discussionType | hasOtherUnresolved | expectedResult | description + ${false} | ${'line'} | ${true} | ${{ type: 'general', discussionId: 'discussion1' }} | ${'returns general action for line discussions on Overview tab regardless of other discussions'} + ${false} | ${'file'} | ${true} | ${{ type: 'general', discussionId: 'discussion1' }} | ${'returns general action for file discussions on Overview tab regardless of other discussions'} + ${true} | ${'line'} | ${true} | ${null} | ${'returns null for line discussions on Diffs tab when others unresolved'} + ${undefined} | ${'line'} | ${false} | ${{ type: 'line', lineCode: 'abc_1_1', fileHash: 'file123' }} | ${'uses grouped behavior by default (groupDiscussions defaults to true)'} + `( + '$description', + ({ groupDiscussions, discussionType, hasOtherUnresolved, expectedResult }) => { + const discussions = + discussionType === 'line' + ? [ + createLineDiscussion({ id: 'discussion1', resolved: true, lineCode: 'abc_1_1' }), + ...(hasOtherUnresolved + ? [ + createLineDiscussion({ + id: 'discussion2', + resolved: false, + lineCode: 'abc_1_1', + }), + ] + : []), + ] + : [ + createFileDiscussion({ id: 'discussion1', resolved: true }), + ...(hasOtherUnresolved + ? [createFileDiscussion({ id: 'discussion2', resolved: false })] + : []), + ]; + + const result = determineCollapseAction(discussions, 'discussion1', groupDiscussions); + + expect(result).toEqual(expectedResult); + }, + ); + }); + + 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', + }); + }); + }); + }); + + describe('getCollapseActionFromEndpoint', () => { + it.each` + endpoint | groupDiscussions | discussionExists | expectedResult | description + ${'/discussions/discussion1/resolve'} | ${undefined} | ${true} | ${{ type: 'line', lineCode: 'abc_1_1', fileHash: 'file123' }} | ${'extracts ID and determines collapse action with default grouping'} + ${'/discussions/discussion1/resolve'} | ${false} | ${true} | ${{ type: 'general', discussionId: 'discussion1' }} | ${'passes groupDiscussions parameter through'} + ${'/invalid/endpoint'} | ${undefined} | ${true} | ${null} | ${'returns null for invalid endpoint'} + ${'/discussions/nonexistent/resolve'} | ${undefined} | ${false} | ${null} | ${'returns null when discussion not found'} + `('$description', ({ endpoint, groupDiscussions, discussionExists, expectedResult }) => { + const discussions = discussionExists + ? [createLineDiscussion({ id: 'discussion1' })] + : [createLineDiscussion({ id: 'other' })]; + + const result = getCollapseActionFromEndpoint(endpoint, discussions, groupDiscussions); + expect(result).toEqual(expectedResult); + }); + }); +}); diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb index ec9e42723af389daa6e26c3cf4a00ad3803a6062..6877bf325807fc3704951e5ca7e136ae7cad8c85 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -153,4 +153,30 @@ def find_in_panel_by_scrolling(selector, **options) end element end + + def expand_all_collapsed_discussions + allowing_for_delay(interval: 0.5, retries: 3) do + while has_selector?('.js-diff-comment-avatar:not(.diff-notes-collapse)', wait: 1) + begin + first('.js-diff-comment-avatar:not(.diff-notes-collapse)', wait: 1, minimum: 1).click + wait_for_requests + rescue Selenium::WebDriver::Error::StaleElementReferenceError + raise Capybara::ExpectationNotMet + end + end + end + end + + def expand_auto_collapsed_replies + allowing_for_delay(interval: 0.5, retries: 3) do + while has_selector?('[data-testid="expand-replies-button"]', wait: 1) + begin + first('[data-testid="expand-replies-button"]', wait: 1).click + wait_for_requests + rescue Selenium::WebDriver::Error::StaleElementReferenceError + raise Capybara::ExpectationNotMet + end + end + end + end end