Merge branch 'osw-remove-unnused-data-from-diff-discussions' into 'master'
Remove un-used data from discussions endpoint Closes #54288 See merge request gitlab-org/gitlab-ce!23570
This commit is contained in:
commit
acc785f56e
|
@ -0,0 +1,101 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class DiffFileBaseEntity < Grape::Entity
|
||||
include RequestAwareEntity
|
||||
include BlobHelper
|
||||
include SubmoduleHelper
|
||||
include DiffHelper
|
||||
include TreeHelper
|
||||
include ChecksCollaboration
|
||||
include Gitlab::Utils::StrongMemoize
|
||||
|
||||
expose :content_sha
|
||||
expose :submodule?, as: :submodule
|
||||
|
||||
expose :submodule_link do |diff_file|
|
||||
memoized_submodule_links(diff_file).first
|
||||
end
|
||||
|
||||
expose :submodule_tree_url do |diff_file|
|
||||
memoized_submodule_links(diff_file).last
|
||||
end
|
||||
|
||||
expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
|
||||
merge_request = options[:merge_request]
|
||||
|
||||
options = merge_request.persisted? ? { from_merge_request_iid: merge_request.iid } : {}
|
||||
|
||||
next unless merge_request.source_project
|
||||
|
||||
project_edit_blob_path(merge_request.source_project,
|
||||
tree_join(merge_request.source_branch, diff_file.new_path),
|
||||
options)
|
||||
end
|
||||
|
||||
expose :old_path_html do |diff_file|
|
||||
old_path, _ = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
|
||||
old_path
|
||||
end
|
||||
|
||||
expose :new_path_html do |diff_file|
|
||||
_, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
|
||||
new_path
|
||||
end
|
||||
|
||||
expose :formatted_external_url, if: -> (_, options) { options[:environment] } do |diff_file|
|
||||
options[:environment].formatted_external_url
|
||||
end
|
||||
|
||||
expose :external_url, if: -> (_, options) { options[:environment] } do |diff_file|
|
||||
options[:environment].external_url_for(diff_file.new_path, diff_file.content_sha)
|
||||
end
|
||||
|
||||
expose :blob, using: BlobEntity
|
||||
|
||||
expose :can_modify_blob do |diff_file|
|
||||
merge_request = options[:merge_request]
|
||||
|
||||
next unless diff_file.blob
|
||||
|
||||
if merge_request&.source_project && current_user
|
||||
can_modify_blob?(diff_file.blob, merge_request.source_project, merge_request.source_branch)
|
||||
else
|
||||
false
|
||||
end
|
||||
end
|
||||
|
||||
expose :file_hash do |diff_file|
|
||||
Digest::SHA1.hexdigest(diff_file.file_path)
|
||||
end
|
||||
|
||||
expose :file_path
|
||||
expose :old_path
|
||||
expose :new_path
|
||||
expose :new_file?, as: :new_file
|
||||
expose :collapsed?, as: :collapsed
|
||||
expose :text?, as: :text
|
||||
expose :diff_refs
|
||||
expose :stored_externally?, as: :stored_externally
|
||||
expose :external_storage
|
||||
expose :renamed_file?, as: :renamed_file
|
||||
expose :deleted_file?, as: :deleted_file
|
||||
expose :mode_changed?, as: :mode_changed
|
||||
expose :a_mode
|
||||
expose :b_mode
|
||||
|
||||
private
|
||||
|
||||
def memoized_submodule_links(diff_file)
|
||||
strong_memoize(:submodule_links) do
|
||||
if diff_file.submodule?
|
||||
submodule_links(diff_file.blob, diff_file.content_sha, diff_file.repository)
|
||||
else
|
||||
[]
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def current_user
|
||||
request.current_user
|
||||
end
|
||||
end
|
|
@ -1,64 +1,12 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class DiffFileEntity < Grape::Entity
|
||||
include RequestAwareEntity
|
||||
class DiffFileEntity < DiffFileBaseEntity
|
||||
include CommitsHelper
|
||||
include DiffHelper
|
||||
include SubmoduleHelper
|
||||
include BlobHelper
|
||||
include IconsHelper
|
||||
include TreeHelper
|
||||
include ChecksCollaboration
|
||||
include Gitlab::Utils::StrongMemoize
|
||||
|
||||
expose :submodule?, as: :submodule
|
||||
|
||||
expose :submodule_link do |diff_file|
|
||||
memoized_submodule_links(diff_file).first
|
||||
end
|
||||
|
||||
expose :submodule_tree_url do |diff_file|
|
||||
memoized_submodule_links(diff_file).last
|
||||
end
|
||||
|
||||
expose :blob, using: BlobEntity
|
||||
|
||||
expose :can_modify_blob do |diff_file|
|
||||
merge_request = options[:merge_request]
|
||||
|
||||
next unless diff_file.blob
|
||||
|
||||
if merge_request&.source_project && current_user
|
||||
can_modify_blob?(diff_file.blob, merge_request.source_project, merge_request.source_branch)
|
||||
else
|
||||
false
|
||||
end
|
||||
end
|
||||
|
||||
expose :file_hash do |diff_file|
|
||||
Digest::SHA1.hexdigest(diff_file.file_path)
|
||||
end
|
||||
|
||||
expose :file_path
|
||||
expose :too_large?, as: :too_large
|
||||
expose :collapsed?, as: :collapsed
|
||||
expose :new_file?, as: :new_file
|
||||
|
||||
expose :deleted_file?, as: :deleted_file
|
||||
expose :renamed_file?, as: :renamed_file
|
||||
expose :mode_changed?, as: :mode_changed
|
||||
expose :old_path
|
||||
expose :new_path
|
||||
expose :mode_changed?, as: :mode_changed
|
||||
expose :a_mode
|
||||
expose :b_mode
|
||||
expose :text?, as: :text
|
||||
expose :added_lines
|
||||
expose :removed_lines
|
||||
expose :diff_refs
|
||||
expose :content_sha
|
||||
expose :stored_externally?, as: :stored_externally
|
||||
expose :external_storage
|
||||
|
||||
expose :load_collapsed_diff_url, if: -> (diff_file, options) { diff_file.text? && options[:merge_request] } do |diff_file|
|
||||
merge_request = options[:merge_request]
|
||||
|
@ -76,36 +24,6 @@ class DiffFileEntity < Grape::Entity
|
|||
)
|
||||
end
|
||||
|
||||
expose :formatted_external_url, if: -> (_, options) { options[:environment] } do |diff_file|
|
||||
options[:environment].formatted_external_url
|
||||
end
|
||||
|
||||
expose :external_url, if: -> (_, options) { options[:environment] } do |diff_file|
|
||||
options[:environment].external_url_for(diff_file.new_path, diff_file.content_sha)
|
||||
end
|
||||
|
||||
expose :old_path_html do |diff_file|
|
||||
old_path, _ = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
|
||||
old_path
|
||||
end
|
||||
|
||||
expose :new_path_html do |diff_file|
|
||||
_, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
|
||||
new_path
|
||||
end
|
||||
|
||||
expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
|
||||
merge_request = options[:merge_request]
|
||||
|
||||
options = merge_request.persisted? ? { from_merge_request_iid: merge_request.iid } : {}
|
||||
|
||||
next unless merge_request.source_project
|
||||
|
||||
project_edit_blob_path(merge_request.source_project,
|
||||
tree_join(merge_request.source_branch, diff_file.new_path),
|
||||
options)
|
||||
end
|
||||
|
||||
expose :view_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
|
||||
merge_request = options[:merge_request]
|
||||
|
||||
|
@ -146,18 +64,4 @@ class DiffFileEntity < Grape::Entity
|
|||
|
||||
# Used for parallel diffs
|
||||
expose :parallel_diff_lines, using: DiffLineParallelEntity, if: -> (diff_file, _) { diff_file.text? }
|
||||
|
||||
def current_user
|
||||
request.current_user
|
||||
end
|
||||
|
||||
def memoized_submodule_links(diff_file)
|
||||
strong_memoize(:submodule_links) do
|
||||
if diff_file.submodule?
|
||||
submodule_links(diff_file.blob, diff_file.content_sha, diff_file.repository)
|
||||
else
|
||||
[]
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -0,0 +1,4 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class DiscussionDiffFileEntity < DiffFileBaseEntity
|
||||
end
|
|
@ -36,7 +36,7 @@ class DiscussionEntity < Grape::Entity
|
|||
new_project_issue_path(discussion.project, merge_request_to_resolve_discussions_of: discussion.noteable.iid, discussion_to_resolve: discussion.id)
|
||||
end
|
||||
|
||||
expose :diff_file, using: DiffFileEntity, if: -> (d, _) { d.diff_discussion? }
|
||||
expose :diff_file, using: DiscussionDiffFileEntity, if: -> (d, _) { d.diff_discussion? }
|
||||
|
||||
expose :diff_discussion?, as: :diff_discussion
|
||||
|
||||
|
@ -46,19 +46,6 @@ class DiscussionEntity < Grape::Entity
|
|||
|
||||
expose :truncated_diff_lines, using: DiffLineEntity, if: -> (d, _) { d.diff_discussion? && d.on_text? && (d.expanded? || render_truncated_diff_lines?) }
|
||||
|
||||
expose :image_diff_html, if: -> (d, _) { d.diff_discussion? && d.on_image? } do |discussion|
|
||||
diff_file = discussion.diff_file
|
||||
partial = diff_file.new_file? || diff_file.deleted_file? ? 'single_image_diff' : 'replaced_image_diff'
|
||||
options[:context].render_to_string(
|
||||
partial: "projects/diffs/#{partial}",
|
||||
locals: { diff_file: diff_file,
|
||||
position: discussion.position.to_json,
|
||||
click_to_comment: false },
|
||||
layout: false,
|
||||
formats: [:html]
|
||||
)
|
||||
end
|
||||
|
||||
expose :for_commit?, as: :for_commit
|
||||
expose :commit_id
|
||||
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Remove unused data from discussions endpoint
|
||||
merge_request: 23570
|
||||
author:
|
||||
type: performance
|
|
@ -489,8 +489,6 @@ export default {
|
|||
diff_discussion: true,
|
||||
truncated_diff_lines:
|
||||
'<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="1">\n1\n</td>\n<td class="line_content new noteable_line"><span id="LC1" class="line" lang="plaintext"> - Bad dates</span>\n</td>\n</tr>\n<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="2">\n2\n</td>\n<td class="line_content new noteable_line"><span id="LC2" class="line" lang="plaintext"></span>\n</td>\n</tr>\n',
|
||||
image_diff_html:
|
||||
'<div class="image js-replaced-image" data="">\n<div class="two-up view">\n<div class="wrap">\n<div class="frame deleted">\n<img alt="CHANGELOG" src="http://localhost:3000/gitlab-org/gitlab-test/raw/e63f41fe459e62e1228fcef60d7189127aeba95a/CHANGELOG" />\n</div>\n<p class="image-info hide">\n<span class="meta-filesize">22.3 KB</span>\n|\n<strong>W:</strong>\n<span class="meta-width"></span>\n|\n<strong>H:</strong>\n<span class="meta-height"></span>\n</p>\n</div>\n<div class="wrap">\n<div class="added frame js-image-frame" data-note-type="DiffNote" data-position="{"base_sha":"e63f41fe459e62e1228fcef60d7189127aeba95a","start_sha":"d9eaefe5a676b820c57ff18cf5b68316025f7962","head_sha":"c48ee0d1bf3b30453f5b32250ce03134beaa6d13","old_path":"CHANGELOG","new_path":"CHANGELOG","position_type":"text","old_line":null,"new_line":2}">\n<img alt="CHANGELOG" draggable="false" src="http://localhost:3000/gitlab-org/gitlab-test/raw/c48ee0d1bf3b30453f5b32250ce03134beaa6d13/CHANGELOG" />\n</div>\n\n<p class="image-info hide">\n<span class="meta-filesize">22.3 KB</span>\n|\n<strong>W:</strong>\n<span class="meta-width"></span>\n|\n<strong>H:</strong>\n<span class="meta-height"></span>\n</p>\n</div>\n</div>\n<div class="swipe view hide">\n<div class="swipe-frame">\n<div class="frame deleted">\n<img alt="CHANGELOG" src="http://localhost:3000/gitlab-org/gitlab-test/raw/e63f41fe459e62e1228fcef60d7189127aeba95a/CHANGELOG" />\n</div>\n<div class="swipe-wrap">\n<div class="added frame js-image-frame" data-note-type="DiffNote" data-position="{"base_sha":"e63f41fe459e62e1228fcef60d7189127aeba95a","start_sha":"d9eaefe5a676b820c57ff18cf5b68316025f7962","head_sha":"c48ee0d1bf3b30453f5b32250ce03134beaa6d13","old_path":"CHANGELOG","new_path":"CHANGELOG","position_type":"text","old_line":null,"new_line":2}">\n<img alt="CHANGELOG" draggable="false" src="http://localhost:3000/gitlab-org/gitlab-test/raw/c48ee0d1bf3b30453f5b32250ce03134beaa6d13/CHANGELOG" />\n</div>\n\n</div>\n<span class="swipe-bar">\n<span class="top-handle"></span>\n<span class="bottom-handle"></span>\n</span>\n</div>\n</div>\n<div class="onion-skin view hide">\n<div class="onion-skin-frame">\n<div class="frame deleted">\n<img alt="CHANGELOG" src="http://localhost:3000/gitlab-org/gitlab-test/raw/e63f41fe459e62e1228fcef60d7189127aeba95a/CHANGELOG" />\n</div>\n<div class="added frame js-image-frame" data-note-type="DiffNote" data-position="{"base_sha":"e63f41fe459e62e1228fcef60d7189127aeba95a","start_sha":"d9eaefe5a676b820c57ff18cf5b68316025f7962","head_sha":"c48ee0d1bf3b30453f5b32250ce03134beaa6d13","old_path":"CHANGELOG","new_path":"CHANGELOG","position_type":"text","old_line":null,"new_line":2}">\n<img alt="CHANGELOG" draggable="false" src="http://localhost:3000/gitlab-org/gitlab-test/raw/c48ee0d1bf3b30453f5b32250ce03134beaa6d13/CHANGELOG" />\n</div>\n\n<div class="controls">\n<div class="transparent"></div>\n<div class="drag-track">\n<div class="dragger" style="left: 0px;"></div>\n</div>\n<div class="opaque"></div>\n</div>\n</div>\n</div>\n</div>\n<div class="view-modes hide">\n<ul class="view-modes-menu">\n<li class="two-up" data-mode="two-up">2-up</li>\n<li class="swipe" data-mode="swipe">Swipe</li>\n<li class="onion-skin" data-mode="onion-skin">Onion skin</li>\n</ul>\n</div>\n',
|
||||
};
|
||||
|
||||
export const imageDiffDiscussions = [
|
||||
|
|
|
@ -13,39 +13,6 @@ describe DiffFileEntity do
|
|||
|
||||
subject { entity.as_json }
|
||||
|
||||
shared_examples 'diff file entity' do
|
||||
it 'exposes correct attributes' do
|
||||
expect(subject).to include(
|
||||
:submodule, :submodule_link, :submodule_tree_url, :file_path,
|
||||
:deleted_file, :old_path, :new_path, :mode_changed,
|
||||
:a_mode, :b_mode, :text, :old_path_html,
|
||||
:new_path_html, :highlighted_diff_lines, :parallel_diff_lines,
|
||||
:blob, :file_hash, :added_lines, :removed_lines, :diff_refs, :content_sha,
|
||||
:stored_externally, :external_storage, :too_large, :collapsed, :new_file,
|
||||
:context_lines_path
|
||||
)
|
||||
end
|
||||
|
||||
it 'includes viewer' do
|
||||
expect(subject[:viewer].with_indifferent_access)
|
||||
.to match_schema('entities/diff_viewer')
|
||||
end
|
||||
|
||||
# Converted diff files from GitHub import does not contain blob file
|
||||
# and content sha.
|
||||
context 'when diff file does not have a blob and content sha' do
|
||||
it 'exposes some attributes as nil' do
|
||||
allow(diff_file).to receive(:content_sha).and_return(nil)
|
||||
allow(diff_file).to receive(:blob).and_return(nil)
|
||||
|
||||
expect(subject[:context_lines_path]).to be_nil
|
||||
expect(subject[:view_path]).to be_nil
|
||||
expect(subject[:highlighted_diff_lines]).to be_nil
|
||||
expect(subject[:can_modify_blob]).to be_nil
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when there is no merge request' do
|
||||
it_behaves_like 'diff file entity'
|
||||
end
|
||||
|
|
|
@ -0,0 +1,39 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
require 'spec_helper'
|
||||
|
||||
describe DiscussionDiffFileEntity do
|
||||
include RepoHelpers
|
||||
|
||||
let(:project) { create(:project, :repository) }
|
||||
let(:repository) { project.repository }
|
||||
let(:commit) { project.commit(sample_commit.id) }
|
||||
let(:diff_refs) { commit.diff_refs }
|
||||
let(:diff) { commit.raw_diffs.first }
|
||||
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
|
||||
let(:entity) { described_class.new(diff_file, request: {}) }
|
||||
|
||||
subject { entity.as_json }
|
||||
|
||||
context 'when there is no merge request' do
|
||||
it_behaves_like 'diff file discussion entity'
|
||||
end
|
||||
|
||||
context 'when there is a merge request' do
|
||||
let(:user) { create(:user) }
|
||||
let(:request) { EntityRequest.new(project: project, current_user: user) }
|
||||
let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
|
||||
let(:entity) { described_class.new(diff_file, request: request, merge_request: merge_request) }
|
||||
|
||||
it_behaves_like 'diff file discussion entity'
|
||||
|
||||
it 'exposes additional attributes' do
|
||||
expect(subject).to include(:edit_path)
|
||||
end
|
||||
|
||||
it 'exposes no diff lines' do
|
||||
expect(subject).not_to include(:highlighted_diff_lines,
|
||||
:parallel_diff_lines)
|
||||
end
|
||||
end
|
||||
end
|
|
@ -74,13 +74,5 @@ describe DiscussionEntity do
|
|||
:active
|
||||
)
|
||||
end
|
||||
|
||||
context 'when diff file is a image' do
|
||||
it 'exposes image attributes' do
|
||||
allow(discussion).to receive(:on_image?).and_return(true)
|
||||
|
||||
expect(subject.keys).to include(:image_diff_html)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -0,0 +1,46 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
shared_examples 'diff file base entity' do
|
||||
it 'exposes essential attributes' do
|
||||
expect(subject).to include(:content_sha, :submodule, :submodule_link,
|
||||
:submodule_tree_url, :old_path_html,
|
||||
:new_path_html, :blob, :can_modify_blob,
|
||||
:file_hash, :file_path, :old_path, :new_path,
|
||||
:collapsed, :text, :diff_refs, :stored_externally,
|
||||
:external_storage, :renamed_file, :deleted_file,
|
||||
:mode_changed, :a_mode, :b_mode, :new_file)
|
||||
end
|
||||
|
||||
# Converted diff files from GitHub import does not contain blob file
|
||||
# and content sha.
|
||||
context 'when diff file does not have a blob and content sha' do
|
||||
it 'exposes some attributes as nil' do
|
||||
allow(diff_file).to receive(:content_sha).and_return(nil)
|
||||
allow(diff_file).to receive(:blob).and_return(nil)
|
||||
|
||||
expect(subject[:context_lines_path]).to be_nil
|
||||
expect(subject[:view_path]).to be_nil
|
||||
expect(subject[:highlighted_diff_lines]).to be_nil
|
||||
expect(subject[:can_modify_blob]).to be_nil
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'diff file entity' do
|
||||
it_behaves_like 'diff file base entity'
|
||||
|
||||
it 'exposes correct attributes' do
|
||||
expect(subject).to include(:too_large, :added_lines, :removed_lines,
|
||||
:context_lines_path, :highlighted_diff_lines,
|
||||
:parallel_diff_lines)
|
||||
end
|
||||
|
||||
it 'includes viewer' do
|
||||
expect(subject[:viewer].with_indifferent_access)
|
||||
.to match_schema('entities/diff_viewer')
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'diff file discussion entity' do
|
||||
it_behaves_like 'diff file base entity'
|
||||
end
|
Loading…
Reference in New Issue