diff options
author | Grzegorz Bizon <grzegorz@gitlab.com> | 2017-09-16 05:09:20 +0000 |
---|---|---|
committer | Grzegorz Bizon <grzegorz@gitlab.com> | 2017-09-16 05:09:20 +0000 |
commit | a70c76df8fd746e5a83b305acbbc1c260955e332 (patch) | |
tree | c9c62667c33676ad74e33257264191fc7186bde3 | |
parent | c433b1917ec8c90d43972c6ec85d4af3fb030694 (diff) | |
parent | 25c959f94d3dda63120256735163e4a142b8711e (diff) | |
download | gitlab-ce-a70c76df8fd746e5a83b305acbbc1c260955e332.tar.gz |
Merge branch 'issue_37640' into 'master'
Fix project feature being deleted when updating project with invalid visibility level
Closes #37640
See merge request gitlab-org/gitlab-ce!14234
-rw-r--r-- | app/models/project.rb | 4 | ||||
-rw-r--r-- | app/models/project_feature.rb | 2 | ||||
-rw-r--r-- | app/services/projects/update_service.rb | 5 | ||||
-rw-r--r-- | changelogs/unreleased/issue_37640.yml | 6 | ||||
-rw-r--r-- | db/post_migrate/20170913180600_fix_projects_without_project_feature.rb | 33 | ||||
-rw-r--r-- | spec/services/projects/update_service_spec.rb | 23 |
6 files changed, 67 insertions, 6 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index add272cc5ce..94ae0acbe1a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -161,7 +161,7 @@ class Project < ActiveRecord::Base has_many :notification_settings, as: :source, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_one :import_data, class_name: 'ProjectImportData', inverse_of: :project, autosave: true - has_one :project_feature + has_one :project_feature, inverse_of: :project has_one :statistics, class_name: 'ProjectStatistics' # Container repositories need to remove data from the container registry, @@ -190,7 +190,7 @@ class Project < ActiveRecord::Base has_one :auto_devops, class_name: 'ProjectAutoDevops' accepts_nested_attributes_for :variables, allow_destroy: true - accepts_nested_attributes_for :project_feature + accepts_nested_attributes_for :project_feature, update_only: true accepts_nested_attributes_for :import_data accepts_nested_attributes_for :auto_devops diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index fb1db0255aa..bfb8d703ec9 100644 --- a/app/models/project_feature.rb +++ b/app/models/project_feature.rb @@ -41,6 +41,8 @@ class ProjectFeature < ActiveRecord::Base # http://stackoverflow.com/questions/1540645/how-to-disable-default-scope-for-a-belongs-to belongs_to :project, -> { unscope(where: :pending_delete) } + validates :project, presence: true + validate :repository_children_level default_value_for :builds_access_level, value: ENABLED, allows_nil: false diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index cb4ffcab778..13e292a18bf 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -24,7 +24,10 @@ module Projects success else - error('Project could not be updated!') + model_errors = project.errors.full_messages.to_sentence + error_message = model_errors.presence || 'Project could not be updated!' + + error(error_message) end end diff --git a/changelogs/unreleased/issue_37640.yml b/changelogs/unreleased/issue_37640.yml new file mode 100644 index 00000000000..d806ed64bed --- /dev/null +++ b/changelogs/unreleased/issue_37640.yml @@ -0,0 +1,6 @@ +--- +title: Fix project feature being deleted when updating project with invalid visibility + level +merge_request: +author: +type: fixed diff --git a/db/post_migrate/20170913180600_fix_projects_without_project_feature.rb b/db/post_migrate/20170913180600_fix_projects_without_project_feature.rb new file mode 100644 index 00000000000..bfa9ad80c7d --- /dev/null +++ b/db/post_migrate/20170913180600_fix_projects_without_project_feature.rb @@ -0,0 +1,33 @@ +class FixProjectsWithoutProjectFeature < ActiveRecord::Migration + DOWNTIME = false + + def up + # Deletes corrupted project features + sql = "DELETE FROM project_features WHERE project_id IS NULL" + execute(sql) + + # Creates missing project features with private visibility + sql = + %Q{ + INSERT INTO project_features(project_id, repository_access_level, issues_access_level, merge_requests_access_level, wiki_access_level, + builds_access_level, snippets_access_level, created_at, updated_at) + SELECT projects.id as project_id, + 10 as repository_access_level, + 10 as issues_access_level, + 10 as merge_requests_access_level, + 10 as wiki_access_level, + 10 as builds_access_level , + 10 as snippets_access_level, + projects.created_at, + projects.updated_at + FROM projects + LEFT OUTER JOIN project_features ON project_features.project_id = projects.id + WHERE (project_features.id IS NULL) + } + + execute(sql) + end + + def down + end +end diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 92cc9a37795..c551083ac90 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -57,6 +57,21 @@ describe Projects::UpdateService, '#execute' do end end end + + context 'When project visibility is higher than parent group' do + let(:group) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } + + before do + project.update(namespace: group, visibility_level: group.visibility_level) + end + + it 'does not update project visibility level' do + result = update_project(project, admin, visibility_level: Gitlab::VisibilityLevel::PUBLIC) + + expect(result).to eq({ status: :error, message: 'Visibility level public is not allowed in a internal group.' }) + expect(project.reload).to be_internal + end + end end describe 'when updating project that has forks' do @@ -148,7 +163,7 @@ describe Projects::UpdateService, '#execute' do result = update_project(project, admin, path: 'existing') expect(result).to include(status: :error) - expect(result[:message]).to match('Project could not be updated!') + expect(result[:message]).to match('There is already a repository with that name on disk') expect(project).not_to be_valid expect(project.errors.messages).to have_key(:base) expect(project.errors.messages[:base]).to include('There is already a repository with that name on disk') @@ -159,8 +174,10 @@ describe Projects::UpdateService, '#execute' do it 'returns an error result when record cannot be updated' do result = update_project(project, admin, { name: 'foo&bar' }) - expect(result).to eq({ status: :error, - message: 'Project could not be updated!' }) + expect(result).to eq({ + status: :error, + message: "Name can contain only letters, digits, emojis, '_', '.', dash, space. It must start with letter, digit, emoji or '_'." + }) end end |