Commit ac398f7e authored by Paul Slaughter's avatar Paul Slaughter Committed by David O'Regan

Fix draft image comments showing in MR overview

- When draft top-level comments were added we
  only looked at `line_code` to see if the note was
  associated with a file. This overlooked images.
- The fix was to look at `file_path`
- We also add a feature spec for this and a
  factory helper
parent 1fd3674a
...@@ -8,7 +8,7 @@ const getDraftComments = (state) => { ...@@ -8,7 +8,7 @@ const getDraftComments = (state) => {
} }
return state.batchComments.drafts return state.batchComments.drafts
.filter((draft) => !draft.line_code && !draft.discussion_id) .filter((draft) => !draft.file_path && !draft.discussion_id)
.map((x) => ({ .map((x) => ({
...x, ...x,
// Treat a top-level draft note as individual_note so it's not included in // Treat a top-level draft note as individual_note so it's not included in
......
...@@ -9,17 +9,30 @@ FactoryBot.define do ...@@ -9,17 +9,30 @@ FactoryBot.define do
transient do transient do
line_number { 14 } line_number { 14 }
diff_refs { merge_request.try(:diff_refs) } diff_refs { merge_request.try(:diff_refs) }
path { "files/ruby/popen.rb" }
end end
position do position do
Gitlab::Diff::Position.new( Gitlab::Diff::Position.new(
old_path: "files/ruby/popen.rb", old_path: path,
new_path: "files/ruby/popen.rb", new_path: path,
old_line: nil, old_line: nil,
new_line: line_number, new_line: line_number,
diff_refs: diff_refs diff_refs: diff_refs
) )
end end
factory :draft_note_on_image_diff do
transient do
path { "files/images/any_image.png" }
end
position do
association(:image_diff_position,
file: path,
diff_refs: diff_refs)
end
end
end end
factory :draft_note_on_discussion, traits: [:on_discussion] factory :draft_note_on_discussion, traits: [:on_discussion]
......
...@@ -86,6 +86,19 @@ RSpec.describe 'Merge request > Batch comments', :js do ...@@ -86,6 +86,19 @@ RSpec.describe 'Merge request > Batch comments', :js do
expect(page).to have_selector('.draft-note-component', text: 'Testing update') expect(page).to have_selector('.draft-note-component', text: 'Testing update')
end end
context 'with image and file draft note' do
let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project) }
let!(:draft_on_text) { create(:draft_note_on_text_diff, merge_request: merge_request, author: user, path: 'README.md', note: 'Lorem ipsum on text...') }
let!(:draft_on_image) { create(:draft_note_on_image_diff, merge_request: merge_request, author: user, path: 'files/images/ee_repo_logo.png', note: 'Lorem ipsum on an image...') }
it 'does not show in overview' do
visit_overview
expect(page).to have_no_text(draft_on_text.note)
expect(page).to have_no_text(draft_on_image.note)
end
end
context 'adding single comment to review' do context 'adding single comment to review' do
before do before do
visit_overview visit_overview
......
...@@ -408,10 +408,9 @@ describe('note_app', () => { ...@@ -408,10 +408,9 @@ describe('note_app', () => {
store = createStore(); store = createStore();
store.registerModule('batchComments', batchComments()); store.registerModule('batchComments', batchComments());
store.state.batchComments.drafts = [ store.state.batchComments.drafts = [
{ line_code: 1, isDraft: true }, mockData.draftDiffDiscussion,
{ discussion_id: 1, isDraft: true }, mockData.draftReply,
{ note: 'A', isDraft: true }, ...mockData.draftComments,
{ note: 'B', isDraft: true },
]; ];
store.state.isLoading = false; store.state.isLoading = false;
wrapper = shallowMount(NotesApp, { wrapper = shallowMount(NotesApp, {
...@@ -426,10 +425,9 @@ describe('note_app', () => { ...@@ -426,10 +425,9 @@ describe('note_app', () => {
it('correctly finds only draft comments', () => { it('correctly finds only draft comments', () => {
const drafts = wrapper.findAll(DraftNote).wrappers; const drafts = wrapper.findAll(DraftNote).wrappers;
expect(drafts.map((x) => x.props('draft'))).toEqual([ expect(drafts.map((x) => x.props('draft'))).toEqual(
expect.objectContaining({ note: 'A' }), mockData.draftComments.map(({ note }) => expect.objectContaining({ note })),
expect.objectContaining({ note: 'B' }), );
]);
}); });
}); });
}); });
...@@ -1273,10 +1273,16 @@ export const batchSuggestionsInfoMock = [ ...@@ -1273,10 +1273,16 @@ export const batchSuggestionsInfoMock = [
]; ];
export const draftComments = [ export const draftComments = [
{ id: 7, note: 'test draft note' }, { id: 7, note: 'test draft note', isDraft: true },
{ id: 9, note: 'draft note 2' }, { id: 9, note: 'draft note 2', isDraft: true },
]; ];
export const draftReply = { id: 8, note: 'draft reply', discussion_id: 1 }; export const draftReply = { id: 8, note: 'draft reply', discussion_id: 1, isDraft: true };
export const draftDiffDiscussion = { id: 6, note: 'draft diff discussion', line_code: 1 }; export const draftDiffDiscussion = {
id: 6,
note: 'draft diff discussion',
line_code: 1,
file_path: 'lib/foo.rb',
isDraft: true,
};
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment