From ac53b960cdd3854aa82dddf61ca6eb414698797a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Wed, 2 Oct 2019 14:50:39 +0200 Subject: [PATCH 1/3] Improve project update This removes a special handling for overrides, but rather looks at relations, and executes correct handler --- app/models/project.rb | 11 ++++ .../import_export/project_tree_restorer.rb | 64 +++++++------------ 2 files changed, 35 insertions(+), 40 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 80628cc07df562..e596d584d53111 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2258,6 +2258,17 @@ def closest_setting(name) setting end + # TODO: unit test this method + def correct_visibility_level + if group && group.visibility_level < visibility_level + self.visibility_level = group.visibility_level + end + + if Gitlab::CurrentSettings.restricted_visibility_levels.include?(visibility_level) + self.visibility_level = Gitlab::VisibilityLevel::PRIVATE + end + end + private def closest_namespace_setting(name) diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb index 9beb84a2422670..285336a69c14ff 100644 --- a/lib/gitlab/import_export/project_tree_restorer.rb +++ b/lib/gitlab/import_export/project_tree_restorer.rb @@ -32,7 +32,7 @@ def restore ActiveRecord::Base.uncached do ActiveRecord::Base.no_touching do - update_project_params + update_project_params! create_relations end end @@ -70,7 +70,7 @@ def merge_requests_mapping # the configuration yaml file too. # Finally, it updates each attribute in the newly imported project. def create_relations - project_relations_without_project_members.each do |relation_key, relation_definition| + project_relations.each do |relation_key, relation_definition| relation_key_s = relation_key.to_s if relation_definition.present? @@ -124,56 +124,40 @@ def remove_feature_dependent_sub_relations!(_relation_item) # no-op end - def project_relations_without_project_members - # We remove `project_members` as they are deserialized separately - project_relations.except(:project_members) - end - def project_relations - reader.attributes_finder.find_relations_tree(:project) + @project_relations ||= reader.attributes_finder.find_relations_tree(:project) end - def update_project_params + def update_project_params! Gitlab::Timeless.timeless(@project) do - @project.update(project_params) - end - end + project_params = @tree_hash.reject do |key, value| + project_relations.include?(key.to_sym) + end - def project_params - @project_params ||= begin - attrs = json_params.merge(override_params).merge(visibility_level, external_label) + project_params = project_params.merge(present_project_override_params) # Cleaning all imported and overridden params - Gitlab::ImportExport::AttributeCleaner.clean(relation_hash: attrs, - relation_class: Project, - excluded_keys: excluded_keys_for_relation(:project)) + project_params = Gitlab::ImportExport::AttributeCleaner.clean( + relation_hash: project_params, + relation_class: Project, + excluded_keys: excluded_keys_for_relation(:project)) + + @project.assign_attributes(project_params) + @project.correct_visibility_level + @project.save! end end - def override_params - @override_params ||= @project.import_data&.data&.fetch('override_params', nil) || {} - end - - def json_params - @json_params ||= @tree_hash.reject do |key, value| - # return params that are not 1 to many or 1 to 1 relations - value.respond_to?(:each) && !Project.column_names.include?(key) - end - end - - def visibility_level - level = override_params['visibility_level'] || json_params['visibility_level'] || @project.visibility_level - level = @project.group.visibility_level if @project.group && level.to_i > @project.group.visibility_level - level = Gitlab::VisibilityLevel::PRIVATE if level == Gitlab::VisibilityLevel::INTERNAL && Gitlab::CurrentSettings.restricted_visibility_levels.include?(level) - - { 'visibility_level' => level } + def present_project_override_params + # we filter out the empty strings from the overrides + # keeping the default values configured + project_override_params.transform_values do |value| + value.is_a?(String) ? value.presence : value + end.compact end - def external_label - label = override_params['external_authorization_classification_label'].presence || - json_params['external_authorization_classification_label'].presence - - { 'external_authorization_classification_label' => label } + def project_override_params + @project_override_params ||= @project.import_data&.data&.fetch('override_params', nil) || {} end # Given a relation hash containing one or more models and its relationships, -- GitLab From 5243de823f6182a26f63fc312f36c047e28ac220 Mon Sep 17 00:00:00 2001 From: Aleksei Lipniagov Date: Mon, 7 Oct 2019 17:14:21 +0300 Subject: [PATCH 2/3] Add unit tests --- app/models/project.rb | 1 - spec/models/project_spec.rb | 55 +++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index e596d584d53111..f15a1b305d1645 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2258,7 +2258,6 @@ def closest_setting(name) setting end - # TODO: unit test this method def correct_visibility_level if group && group.visibility_level < visibility_level self.visibility_level = group.visibility_level diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2870c55c7c6aed..e6a2f15d75c16f 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -5179,6 +5179,61 @@ def enable_lfs end end + describe '#correct_visibility_level' do + context 'when has a group' do + let(:group) { create(:group, visibility_level: group_visibility_level) } + let(:project) { build(:project, namespace: group, visibility_level: project_visibility_level) } + + context 'when the group `visibility_level` is more strict' do + let(:group_visibility_level) { Gitlab::VisibilityLevel::PRIVATE } + let(:project_visibility_level) { Gitlab::VisibilityLevel::INTERNAL } + + it 'sets `visibility_level` value from the group' do + expect { project.correct_visibility_level } + .to change { project.visibility_level } + .to(Gitlab::VisibilityLevel::PRIVATE) + end + end + + context 'when the group `visibility_level` is less strict' do + let(:group_visibility_level) { Gitlab::VisibilityLevel::INTERNAL } + let(:project_visibility_level) { Gitlab::VisibilityLevel::PRIVATE } + + it 'does not change the value of the `visibility_level` field' do + expect { project.correct_visibility_level } + .not_to change { project.visibility_level } + end + end + end + + context 'when `restricted_visibility_levels` of the GitLab instance exist' do + before do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL]) + end + + let(:project) { build(:project, visibility_level: project_visibility_level) } + + context 'when `visibility_level` is included into `restricted_visibility_levels`' do + let(:project_visibility_level) { Gitlab::VisibilityLevel::INTERNAL } + + it 'sets `visibility_level` value to `PRIVATE`' do + expect { project.correct_visibility_level } + .to change { project.visibility_level } + .to(Gitlab::VisibilityLevel::PRIVATE) + end + end + + context 'when `restricted_visibility_levels` does not include `visibility_level`' do + let(:project_visibility_level) { Gitlab::VisibilityLevel::PUBLIC } + + it 'does not change the value of the `visibility_level` field' do + expect { project.correct_visibility_level } + .to not_change { project.visibility_level } + end + end + end + end + def rugged_config rugged_repo(project.repository).config end -- GitLab From 9c9dafc6dbb5518cbd25df0df8fce258dc8c5657 Mon Sep 17 00:00:00 2001 From: Aleksei Lipniagov Date: Thu, 10 Oct 2019 12:36:38 +0300 Subject: [PATCH 3/3] Rename into `drop_visibility_level!` --- app/models/project.rb | 2 +- lib/gitlab/import_export/project_tree_restorer.rb | 2 +- spec/models/project_spec.rb | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index f15a1b305d1645..26ed9b0cd1e4fd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2258,7 +2258,7 @@ def closest_setting(name) setting end - def correct_visibility_level + def drop_visibility_level! if group && group.visibility_level < visibility_level self.visibility_level = group.visibility_level end diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb index 285336a69c14ff..3fa5765fd4a5c2 100644 --- a/lib/gitlab/import_export/project_tree_restorer.rb +++ b/lib/gitlab/import_export/project_tree_restorer.rb @@ -143,7 +143,7 @@ def update_project_params! excluded_keys: excluded_keys_for_relation(:project)) @project.assign_attributes(project_params) - @project.correct_visibility_level + @project.drop_visibility_level! @project.save! end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e6a2f15d75c16f..ddba5fca12cfef 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -5179,7 +5179,7 @@ def enable_lfs end end - describe '#correct_visibility_level' do + describe '#drop_visibility_level!' do context 'when has a group' do let(:group) { create(:group, visibility_level: group_visibility_level) } let(:project) { build(:project, namespace: group, visibility_level: project_visibility_level) } @@ -5189,7 +5189,7 @@ def enable_lfs let(:project_visibility_level) { Gitlab::VisibilityLevel::INTERNAL } it 'sets `visibility_level` value from the group' do - expect { project.correct_visibility_level } + expect { project.drop_visibility_level! } .to change { project.visibility_level } .to(Gitlab::VisibilityLevel::PRIVATE) end @@ -5200,7 +5200,7 @@ def enable_lfs let(:project_visibility_level) { Gitlab::VisibilityLevel::PRIVATE } it 'does not change the value of the `visibility_level` field' do - expect { project.correct_visibility_level } + expect { project.drop_visibility_level! } .not_to change { project.visibility_level } end end @@ -5217,7 +5217,7 @@ def enable_lfs let(:project_visibility_level) { Gitlab::VisibilityLevel::INTERNAL } it 'sets `visibility_level` value to `PRIVATE`' do - expect { project.correct_visibility_level } + expect { project.drop_visibility_level! } .to change { project.visibility_level } .to(Gitlab::VisibilityLevel::PRIVATE) end @@ -5227,7 +5227,7 @@ def enable_lfs let(:project_visibility_level) { Gitlab::VisibilityLevel::PUBLIC } it 'does not change the value of the `visibility_level` field' do - expect { project.correct_visibility_level } + expect { project.drop_visibility_level! } .to not_change { project.visibility_level } end end -- GitLab