diff --git a/app/assets/javascripts/notes/store/legacy_notes/actions.js b/app/assets/javascripts/notes/store/legacy_notes/actions.js index 8d6814cc38e7e943b6f7c7168c444da8d46fdc11..1824cf0792b6ca756f5c284e4e980fb522920089 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -411,6 +411,7 @@ export function resolveDiscussion({ discussionId }) { } export function toggleResolveNote({ endpoint, isResolved, discussion }) { + const discussionIdMatch = (endpoint || '').match(/discussions\/([^/]+)\/resolve$/); const method = isResolved ? constants.UNRESOLVE_NOTE_METHOD_NAME : constants.RESOLVE_NOTE_METHOD_NAME; @@ -422,6 +423,24 @@ export function toggleResolveNote({ endpoint, isResolved, discussion }) { this.updateResolvableDiscussionsCounts(); this.updateMergeRequestWidget(); + + if (!isResolved && discussion && discussionIdMatch) { + const discussions = this.discussions || []; + const discussionId = discussionIdMatch[1]; + const actualDiscussion = discussions.find((d) => d.id === discussionId); + const currentLineCode = actualDiscussion.line_code; + const lineDiscussions = discussions.filter((d) => d.line_code === currentLineCode); + const allLineDiscussionsResolved = lineDiscussions.every((d) => d.resolved); + + if (allLineDiscussionsResolved && currentLineCode && lineDiscussions.length > 0) { + // tryStore only used for migration, refactor the store to avoid using this helper + this.tryStore('legacyDiffs').toggleLineDiscussions({ + lineCode: actualDiscussion.line_code, + fileHash: actualDiscussion.diff_file?.file_hash, + expanded: false, + }); + } + } }); } diff --git a/qa/qa/page/merge_request/show.rb b/qa/qa/page/merge_request/show.rb index d558bf58514de80c61f1c8afdfa9244225c0b408..e5ea16be682e2c40c5db2372f6c0123a6fa4322e 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 abc5b51c53b801ea68870db4603d0de0c32fc2b8..5cd78fa717b4333016fd6cd19b32677801dd04cd 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 6bd1c7d226edb9b074491f63d8e39cd462f189c1..ac9c8640460e258d021fa0578b2e3963ccb22e47 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/merge_request/user_suggests_changes_on_diff_spec.rb b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb index dd9dc31c2d4f6a0f3ba4685de46361a05d7c06f5..9a2ec37d5b3b37492b7c8182aa5073e248ba84fd 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_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_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_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_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..1fdbc0ac59b7d4caf28500b256e53ee10504a9f5 100644 --- a/spec/frontend/notes/store/legacy_notes/actions_spec.js +++ b/spec/frontend/notes/store/legacy_notes/actions_spec.js @@ -43,7 +43,6 @@ jest.mock('~/alert', () => ({ dismiss: mockAlertDismiss, })), })); - jest.mock('~/vue_shared/plugins/global_toast'); describe('Actions Notes Store', () => { @@ -639,6 +638,169 @@ describe('Actions Notes Store', () => { ], ); }); + + describe('auto-collapse', () => { + let legacyDiffsStore; + let mockToggleLineDiscussions; + + beforeEach(() => { + legacyDiffsStore = useLegacyDiffs(); + mockToggleLineDiscussions = jest.fn(); + legacyDiffsStore.toggleLineDiscussions = mockToggleLineDiscussions; + }); + + it('collapses line discussions when all discussions on the line are resolved', () => { + const localRes = { + resolved: true, + expanded: true, + discussion_id: '1', + id: '1', + }; + + axiosMock.reset(); + axiosMock.onAny().reply(HTTP_STATUS_OK, localRes); + axiosMock.onPut(`${TEST_HOST}/discussions/1/resolve`).replyOnce(200, localRes); + + return testAction( + store.toggleResolveNote, + { endpoint: `${TEST_HOST}/discussions/1/resolve`, isResolved: false, discussion: true }, + { + discussions: [ + { + resolved: true, + discussion_id: '1', + id: '1', + individual_note: true, + notes: [], + line_code: 'abc123', + diff_file: { file_hash: 'file123' }, + }, + { + resolved: true, + discussion_id: '2', + id: '2', + individual_note: true, + notes: [], + line_code: 'abc123', + }, + ], + }, + [ + { + type: store[types.UPDATE_DISCUSSION], + payload: localRes, + }, + ], + [ + { + type: store.updateResolvableDiscussionsCounts, + }, + { + type: store.updateMergeRequestWidget, + }, + ], + ).then(() => { + expect(mockToggleLineDiscussions).toHaveBeenCalledWith({ + lineCode: 'abc123', + fileHash: 'file123', + expanded: false, + }); + }); + }); + + it('does not collapse when there are unresolved discussions on the same line', () => { + return testAction( + store.toggleResolveNote, + { endpoint: `${TEST_HOST}`, isResolved: false, discussion: true }, + { + discussions: [ + { + resolved: true, + discussion_id: 1, + id: 1, + individual_note: true, + notes: [], + line_code: 'abc123', + }, + { + resolved: false, + discussion_id: 2, + id: 2, + individual_note: true, + notes: [], + line_code: 'abc123', + }, + ], + }, + [ + { + type: store[types.UPDATE_DISCUSSION], + payload: res, + }, + ], + [ + { + type: store.updateResolvableDiscussionsCounts, + }, + { + type: store.updateMergeRequestWidget, + }, + ], + ); + }); + + it('does not collapse when unresolving a discussion', () => { + return testAction( + store.toggleResolveNote, + { endpoint: `${TEST_HOST}`, isResolved: true, discussion: true }, + { + discussions: [ + { resolved: false, discussion_id: 1, id: 1, individual_note: true, notes: [] }, + ], + }, + [ + { + type: store[types.UPDATE_DISCUSSION], + payload: res, + }, + ], + [ + { + type: store.updateResolvableDiscussionsCounts, + }, + { + type: store.updateMergeRequestWidget, + }, + ], + ); + }); + + it('does not collapse when discussion parameter is false', () => { + return testAction( + store.toggleResolveNote, + { endpoint: `${TEST_HOST}`, isResolved: false, discussion: false }, + { + discussions: [ + { resolved: true, discussion_id: 1, id: 1, individual_note: true, notes: [] }, + ], + }, + [ + { + type: store[types.UPDATE_NOTE], + payload: res, + }, + ], + [ + { + type: store.updateResolvableDiscussionsCounts, + }, + { + type: store.updateMergeRequestWidget, + }, + ], + ); + }); + }); }); }); diff --git a/spec/support/helpers/merge_request_diff_helpers.rb b/spec/support/helpers/merge_request_diff_helpers.rb index bbd9382fcc2defe92c3fb4343aaea430c7a0371d..dd296bba31bda88fb756330f534537716b6aa15f 100644 --- a/spec/support/helpers/merge_request_diff_helpers.rb +++ b/spec/support/helpers/merge_request_diff_helpers.rb @@ -93,4 +93,12 @@ def find_by_scrolling(selector, **options) end element end + + def expand_collapsed_discussions + first('.js-diff-comment-avatar').click + end + + def expand_all_collapsed_discussions + all('.js-diff-comment-avatar').each(&:click) + end end