Commit 2a6b0c26 authored by Terri Chu's avatar Terri Chu Committed by Bob Van Landuyt

Only set note visibility_level to private if it belongs to a project

Fix for any Elasticsearch notes documents which are not associated
with a project (Epic, Personal Snippet)
parent ac8623c2
---
title: Only set note visibility_level if associated to a project
merge_request: 55111
author:
type: fixed
...@@ -54,9 +54,11 @@ module Elastic ...@@ -54,9 +54,11 @@ module Elastic
nil nil
end end
# protect against missing project_feature and set visibility to PRIVATE # protect against missing project and project_feature and set visibility to PRIVATE
# if the project_feature is missing on a project # if the project_feature is missing on a project
def safely_read_project_feature_for_elasticsearch(feature) def safely_read_project_feature_for_elasticsearch(feature)
return unless target.project
if target.project.project_feature if target.project.project_feature
target.project.project_feature.access_level(feature) target.project.project_feature.access_level(feature)
else else
......
...@@ -22,9 +22,10 @@ module Elastic ...@@ -22,9 +22,10 @@ module Elastic
} }
end end
# only attempt to set project permissions if associated to a project
# do not add the permission fields unless the `remove_permissions_data_from_notes_documents` # do not add the permission fields unless the `remove_permissions_data_from_notes_documents`
# migration has completed otherwise the migration will never finish # migration has completed otherwise the migration will never finish
if Elastic::DataMigrationService.migration_has_finished?(:remove_permissions_data_from_notes_documents) if target.project && Elastic::DataMigrationService.migration_has_finished?(:remove_permissions_data_from_notes_documents)
data['visibility_level'] = target.project.visibility_level data['visibility_level'] = target.project.visibility_level
merge_project_feature_access_level(data, noteable) merge_project_feature_access_level(data, noteable)
end end
......
...@@ -128,10 +128,10 @@ RSpec.describe Note, :elastic do ...@@ -128,10 +128,10 @@ RSpec.describe Note, :elastic do
expect { note.__elasticsearch__.as_indexed_json }.not_to raise_error expect { note.__elasticsearch__.as_indexed_json }.not_to raise_error
end end
where(:note_type, :permission, :access_level) do where(:note_type, :project_permission, :access_level) do
:note_on_issue | ProjectFeature::ENABLED | 'issues_access_level' :note_on_issue | ProjectFeature::ENABLED | 'issues_access_level'
:note_on_project_snippet | ProjectFeature::DISABLED | 'snippets_access_level' :note_on_project_snippet | ProjectFeature::DISABLED | 'snippets_access_level'
:note_on_personal_snippet | ProjectFeature::DISABLED | 'snippets_access_level' :note_on_personal_snippet | nil | false
:note_on_merge_request | ProjectFeature::PUBLIC | 'merge_requests_access_level' :note_on_merge_request | ProjectFeature::PUBLIC | 'merge_requests_access_level'
:note_on_commit | ProjectFeature::PRIVATE | 'repository_access_level' :note_on_commit | ProjectFeature::PRIVATE | 'repository_access_level'
:diff_note_on_merge_request | ProjectFeature::PUBLIC | 'merge_requests_access_level' :diff_note_on_merge_request | ProjectFeature::PUBLIC | 'merge_requests_access_level'
...@@ -141,25 +141,26 @@ RSpec.describe Note, :elastic do ...@@ -141,25 +141,26 @@ RSpec.describe Note, :elastic do
:legacy_diff_note_on_commit | ProjectFeature::PRIVATE | 'repository_access_level' :legacy_diff_note_on_commit | ProjectFeature::PRIVATE | 'repository_access_level'
:note_on_alert | ProjectFeature::PRIVATE | false :note_on_alert | ProjectFeature::PRIVATE | false
:note_on_design | ProjectFeature::ENABLED | false :note_on_design | ProjectFeature::ENABLED | false
:note_on_epic | ProjectFeature::ENABLED | false :note_on_epic | nil | false
:note_on_vulnerability | ProjectFeature::PRIVATE | false :note_on_vulnerability | ProjectFeature::PRIVATE | false
:discussion_note_on_vulnerability | ProjectFeature::PRIVATE | false :discussion_note_on_vulnerability | ProjectFeature::PRIVATE | false
:discussion_note_on_merge_request | ProjectFeature::PUBLIC | 'merge_requests_access_level' :discussion_note_on_merge_request | ProjectFeature::PUBLIC | 'merge_requests_access_level'
:discussion_note_on_issue | ProjectFeature::ENABLED | 'issues_access_level' :discussion_note_on_issue | ProjectFeature::ENABLED | 'issues_access_level'
:discussion_note_on_project_snippet | ProjectFeature::DISABLED | 'snippets_access_level' :discussion_note_on_project_snippet | ProjectFeature::DISABLED | 'snippets_access_level'
:discussion_note_on_personal_snippet | ProjectFeature::DISABLED | 'snippets_access_level' :discussion_note_on_personal_snippet | nil | false
:note_on_merge_request | ProjectFeature::PUBLIC | 'merge_requests_access_level' :note_on_merge_request | ProjectFeature::PUBLIC | 'merge_requests_access_level'
:discussion_note_on_commit | ProjectFeature::PRIVATE | 'repository_access_level' :discussion_note_on_commit | ProjectFeature::PRIVATE | 'repository_access_level'
:track_mr_picking_note | ProjectFeature::PUBLIC | 'merge_requests_access_level' :track_mr_picking_note | ProjectFeature::PUBLIC | 'merge_requests_access_level'
end end
with_them do with_them do
let_it_be(:project) { create(:project, :repository) } let!(:note) { create(note_type) } # rubocop:disable Rails/SaveBang
let!(:note) { create(note_type, project: project) } # rubocop:disable Rails/SaveBang let(:project) { note.project }
let(:note_json) { note.__elasticsearch__.as_indexed_json } let(:note_json) { note.__elasticsearch__.as_indexed_json }
before do before do
project.project_feature.update_attribute(access_level.to_sym, permission) if access_level.present? project.project_feature.update_attribute(access_level.to_sym, project_permission) if access_level.present?
end end
it 'does not contain permissions if remove_permissions_data_from_notes_documents is not finished' do it 'does not contain permissions if remove_permissions_data_from_notes_documents is not finished' do
...@@ -174,11 +175,15 @@ RSpec.describe Note, :elastic do ...@@ -174,11 +175,15 @@ RSpec.describe Note, :elastic do
it 'contains the correct permissions', :aggregate_failures do it 'contains the correct permissions', :aggregate_failures do
if access_level if access_level
expect(note_json).to have_key(access_level) expect(note_json).to have_key(access_level)
expect(note_json[access_level]).to eq(permission) expect(note_json[access_level]).to eq(project_permission)
end end
expect(note_json).to have_key('visibility_level') if project_permission
expect(note_json['visibility_level']).to eq(project.visibility_level) expect(note_json).to have_key('visibility_level')
expect(note_json['visibility_level']).to eq(project.visibility_level)
else
expect(note_json).not_to have_key('visibility_level')
end
end end
end end
end end
......
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