Merge branch 'sandish/gitlab-ce-update_ret_val' into 'master'

Ensure updating project settings shows a flash message on success

See merge request !8579

7 files changed, 85 insertions, 114 deletions

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index d5ee503c44c..444ff837bb3 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -42,19 +42,16 @@ class ProjectsController < Projects::ApplicationController
   end
 
   def update
-    status = ::Projects::UpdateService.new(@project, current_user, project_params).execute
+    result = ::Projects::UpdateService.new(@project, current_user, project_params).execute
 
     # Refresh the repo in case anything changed
-    @repository = project.repository
+    @repository = @project.repository
 
     respond_to do |format|
-      if status
+      if result[:status] == :success
         flash[:notice] = "Project '#{@project.name}' was successfully updated."
         format.html do
-          redirect_to(
-            edit_project_path(@project),
-            notice: "Project '#{@project.name}' was successfully updated."
-          )
+          redirect_to(edit_project_path(@project))
         end
       else
         format.html { render 'edit' }
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index 8a6af8d8ada..842e23eb6b6 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -9,7 +9,7 @@ module Projects
             Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
 
           deny_visibility_level(project, new_visibility)
-          return project
+          return error('Visibility level unallowed')
         end
       end
 
@@ -23,6 +23,10 @@ module Projects
         if project.previous_changes.include?('path')
           project.rename_repo
         end
+
+        success
+      else
+        error('Project could not be updated')
       end
     end
   end
diff --git a/changelogs/unreleased/sandish-gitlab-ce-update_ret_val.yml b/changelogs/unreleased/sandish-gitlab-ce-update_ret_val.yml
new file mode 100644
index 00000000000..7107ddfd982
--- /dev/null
+++ b/changelogs/unreleased/sandish-gitlab-ce-update_ret_val.yml
@@ -0,0 +1,4 @@
+---
+title: Ensure updating project settings shows a flash message on success
+merge_request: 8579
+author: Sandish Chen
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 7c66c340562..941f47114a4 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -295,13 +295,13 @@ module API
         authorize! :rename_project, user_project if attrs[:name].present?
         authorize! :change_visibility_level, user_project if attrs[:visibility_level].present?
 
-        ::Projects::UpdateService.new(user_project, current_user, attrs).execute
+        result = ::Projects::UpdateService.new(user_project, current_user, attrs).execute
 
-        if user_project.errors.any?
-          render_validation_error!(user_project)
-        else
+        if result[:status] == :success
           present user_project, with: Entities::Project,
                                 user_can_admin_project: can?(current_user, :admin_project, user_project)
+        else
+          render_validation_error!(user_project)
         end
       end
 
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index 5ddcaa60dc6..d0a63aa9403 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -245,7 +245,7 @@ describe ProjectsController do
 
       expect(project.repository.path).to include(new_path)
       expect(assigns(:repository).path).to eq(project.repository.path)
-      expect(response).to have_http_status(200)
+      expect(response).to have_http_status(302)
     end
   end
 
diff --git a/spec/features/projects/project_settings_spec.rb b/spec/features/projects/project_settings_spec.rb
index bf60cca4ea4..55d5d082c6e 100644
--- a/spec/features/projects/project_settings_spec.rb
+++ b/spec/features/projects/project_settings_spec.rb
@@ -21,6 +21,16 @@ describe 'Edit Project Settings', feature: true do
       expect(page).to have_content "Name can contain only letters, digits, emojis, '_', '.', dash, space. It must start with letter, digit, emoji or '_'."
       expect(page).to have_button 'Save changes'
     end
+
+    scenario 'shows a successful notice when the project is updated' do
+      visit edit_namespace_project_path(project.namespace, project)
+
+      fill_in 'project_name_edit', with: 'hello world'
+
+      click_button 'Save changes'
+
+      expect(page).to have_content "Project 'hello world' was successfully updated."
+    end
   end
 
   describe 'Rename repository' do
diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb
index e139be19140..caa23962519 100644
--- a/spec/services/projects/update_service_spec.rb
+++ b/spec/services/projects/update_service_spec.rb
@@ -1,145 +1,101 @@
 require 'spec_helper'
 
 describe Projects::UpdateService, services: true do
-  describe :update_by_user do
-    before do
-      @user = create :user
-      @admin = create :user, admin: true
-      @project = create :project, creator_id: @user.id, namespace: @user.namespace
-      @opts = {}
-    end
+  let(:user) { create(:user) }
+  let(:admin) { create(:admin) }
+  let(:project) { create(:project, creator_id: user.id, namespace: user.namespace) }
 
-    context 'is private when updated to private' do
-      before do
-        @created_private = @project.private?
+  describe 'update_by_user' do
+    context 'when visibility_level is INTERNAL' do
+      it 'updates the project to internal' do
+        result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL)
 
-        @opts.merge!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
-        update_project(@project, @user, @opts)
+        expect(result).to eq({ status: :success })
+        expect(project).to be_internal
       end
-
-      it { expect(@created_private).to be_truthy }
-      it { expect(@project.private?).to be_truthy }
     end
 
-    context 'is internal when updated to internal' do
-      before do
-        @created_private = @project.private?
-
-        @opts.merge!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
-        update_project(@project, @user, @opts)
+    context 'when visibility_level is PUBLIC' do
+      it 'updates the project to public' do
+        result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+        expect(result).to eq({ status: :success })
+        expect(project).to be_public
       end
-
-      it { expect(@created_private).to be_truthy }
-      it { expect(@project.internal?).to be_truthy }
     end
 
-    context 'is public when updated to public' do
+    context 'when visibility levels are restricted to PUBLIC only' do
       before do
-        @created_private = @project.private?
-
-        @opts.merge!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
-        update_project(@project, @user, @opts)
-      end
-
-      it { expect(@created_private).to be_truthy }
-      it { expect(@project.public?).to be_truthy }
-    end
-
-    context 'respect configured visibility restrictions setting' do
-      before(:each) do
         stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
       end
 
-      context 'is private when updated to private' do
-        before do
-          @created_private = @project.private?
-
-          @opts.merge!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
-          update_project(@project, @user, @opts)
+      context 'when visibility_level is INTERNAL' do
+        it 'updates the project to internal' do
+          result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          expect(result).to eq({ status: :success })
+          expect(project).to be_internal
         end
-
-        it { expect(@created_private).to be_truthy }
-        it { expect(@project.private?).to be_truthy }
       end
 
-      context 'is internal when updated to internal' do
-        before do
-          @created_private = @project.private?
+      context 'when visibility_level is PUBLIC' do
+        it 'does not update the project to public' do
+          result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
 
-          @opts.merge!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
-          update_project(@project, @user, @opts)
+          expect(result).to eq({ status: :error, message: 'Visibility level unallowed' })
+          expect(project).to be_private
         end
 
-        it { expect(@created_private).to be_truthy }
-        it { expect(@project.internal?).to be_truthy }
-      end
-
-      context 'is private when updated to public' do
-        before do
-          @created_private = @project.private?
-
-          @opts.merge!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
-          update_project(@project, @user, @opts)
+        context 'when updated by an admin' do
+          it 'updates the project to public' do
+            result = update_project(project, admin, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+            expect(result).to eq({ status: :success })
+            expect(project).to be_public
+          end
         end
-
-        it { expect(@created_private).to be_truthy }
-        it { expect(@project.private?).to be_truthy }
-      end
-
-      context 'is public when updated to public by admin' do
-        before do
-          @created_private = @project.private?
-
-          @opts.merge!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
-          update_project(@project, @admin, @opts)
-        end
-
-        it { expect(@created_private).to be_truthy }
-        it { expect(@project.public?).to be_truthy }
       end
     end
   end
 
-  describe :visibility_level do
-    let(:user) { create :user, admin: true }
+  describe 'visibility_level' do
     let(:project) { create(:project, :internal) }
     let(:forked_project) { create(:forked_project_with_submodules, :internal) }
-    let(:opts) { {} }
 
     before do
       forked_project.build_forked_project_link(forked_to_project_id: forked_project.id, forked_from_project_id: project.id)
       forked_project.save
-
-      @created_internal = project.internal?
-      @fork_created_internal = forked_project.internal?
     end
 
-    context 'updates forks visibility level when parent set to more restrictive' do
-      before do
-        opts.merge!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
-        update_project(project, user, opts).inspect
-      end
+    it 'updates forks visibility level when parent set to more restrictive' do
+      opts = { visibility_level: Gitlab::VisibilityLevel::PRIVATE }
+
+      expect(project).to be_internal
+      expect(forked_project).to be_internal
 
-      it { expect(@created_internal).to be_truthy }
-      it { expect(@fork_created_internal).to be_truthy }
-      it { expect(project.private?).to be_truthy }
-      it { expect(project.forks.first.private?).to be_truthy }
+      expect(update_project(project, admin, opts)).to eq({ status: :success })
+
+      expect(project).to be_private
+      expect(forked_project.reload).to be_private
     end
 
-    context 'does not update forks visibility level when parent set to less restrictive' do
-      before do
-        opts.merge!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
-        update_project(project, user, opts).inspect
-      end
+    it 'does not update forks visibility level when parent set to less restrictive' do
+      opts = { visibility_level: Gitlab::VisibilityLevel::PUBLIC }
+
+      expect(project).to be_internal
+      expect(forked_project).to be_internal
 
-      it { expect(@created_internal).to be_truthy }
-      it { expect(@fork_created_internal).to be_truthy }
-      it { expect(project.public?).to be_truthy }
-      it { expect(project.forks.first.internal?).to be_truthy }
+      expect(update_project(project, admin, opts)).to eq({ status: :success })
+
+      expect(project).to be_public
+      expect(forked_project.reload).to be_internal
     end
   end
 
+  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' })
+  end
+
   def update_project(project, user, opts)
-    Projects::UpdateService.new(project, user, opts).execute
+    described_class.new(project, user, opts).execute
   end
 end