Merge remote-tracking branch 'origin/master' into approval-integration

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' }

app/models/project.rb

@@ -1145,7 +1145,7 @@ class Project < ActiveRecord::Base
                                         "refs/heads/#{branch}",
                                         force: true)
     repository.copy_gitattributes(branch)
-    repository.expire_avatar_cache
+    repository.after_change_head
     reload_default_branch
   end
 

app/models/repository.rb

@@ -495,6 +495,11 @@ class Repository
     expire_content_cache
   end
 
+  # Runs code after the HEAD of a repository is changed.
+  def after_change_head
+    expire_method_caches(METHOD_CACHES_FOR_FILE_TYPES.keys)
+  end
+
   # Runs code after a repository has been forked/imported.
   def after_import
     expire_content_cache

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
 
@@ -30,6 +30,10 @@ module Projects
         if project.previous_changes.include?('path')
           project.rename_repo
         end
+
+        success
+      else
+        error('Project could not be updated')
       end
     end
   end

changelogs/unreleased/dot-in-project-queries.yml

+---
+title: Allow API query to find projects with dots in their name
+merge_request:
+author: Bruno Melli

changelogs/unreleased/pmq20-gitlab-ce-psvr-head-cache.yml

+---
+title: Expire related caches after changing HEAD
+merge_request:
+author: Minqi Pan

changelogs/unreleased/sandish-gitlab-ce-update_ret_val.yml

+---
+title: Ensure updating project settings shows a flash message on success
+merge_request: 8579
+author: Sandish Chen

db/migrate/20161226122833_remove_dot_git_from_usernames.rb

@@ -14,9 +14,11 @@ class RemoveDotGitFromUsernames < ActiveRecord::Migration
       namespace_id = user['namespace_id']
       path_was = user['username']
       path_was_wildcard = quote_string("#{path_was}/%")
+      path = quote_string(new_path(path_was))
 
-      path = move_namespace(namespace_id, path_was, path)
+      move_namespace(namespace_id, path_was, path)
 
+      begin
         execute "UPDATE routes SET path = '#{path}' WHERE source_type = 'Namespace' AND source_id = #{namespace_id}"
         execute "UPDATE namespaces SET path = '#{path}' WHERE id = #{namespace_id}"
         execute "UPDATE users SET username = '#{path}' WHERE id = #{id}"
@@ -25,6 +27,13 @@ class RemoveDotGitFromUsernames < ActiveRecord::Migration
           new_path = "#{path}/#{route['path'].split('/').last}"
           execute "UPDATE routes SET path = '#{new_path}' WHERE id = #{route['id']}"
         end
+      rescue => e
+        say("Couldn't update routes for path #{path_was} to #{path}")
+        # Move namespace back
+        move_namespace(namespace_id, path, path_was)
+
+        raise e
+      end
     end
   end
 
@@ -44,23 +53,30 @@ class RemoveDotGitFromUsernames < ActiveRecord::Migration
     select_all("SELECT id, path FROM routes WHERE path = '#{quote_string(path)}'").present?
   end
 
-  def path_exists?(repository_storage_path, path)
-    gitlab_shell.exists?(repository_storage_path, path)
+  def path_exists?(path, repository_storage_path)
+    repository_storage_path && gitlab_shell.exists?(repository_storage_path, path)
   end
 
   # Accepts invalid path like test.git and returns test_git or
   # test_git1 if test_git already taken
-  def rename_path(repository_storage_path, path)
+  def new_path(path)
     # To stay closer with original name and reduce risk of duplicates
     # we rename suffix instead of removing it
     path = path.sub(/\.git\z/, '_git')
 
-    counter = 0
-    base = path
+    check_routes(path.dup, 0, path)
+  end
+
+  def check_routes(base, counter, path)
+    route_exists = route_exists?(path)
 
-    while route_exists?(path) || path_exists?(repository_storage_path, path)
+    Gitlab.config.repositories.storages.each_value do |storage|
+      if route_exists || path_exists?(path, storage)
         counter += 1
         path = "#{base}#{counter}"
+
+        return check_routes(base, counter, path)
+      end
     end
 
     path
@@ -76,8 +92,6 @@ class RemoveDotGitFromUsernames < ActiveRecord::Migration
       # Ensure old directory exists before moving it
       gitlab_shell.add_namespace(repository_storage_path, path_was)
 
-      path = quote_string(rename_path(repository_storage_path, path_was))
-
       unless gitlab_shell.mv_namespace(repository_storage_path, path_was, path)
         Rails.logger.error "Exception moving path #{repository_storage_path} from #{path_was} to #{path}"
 
@@ -87,8 +101,14 @@ class RemoveDotGitFromUsernames < ActiveRecord::Migration
       end
     end
 
+    begin
       Gitlab::UploadsTransfer.new.rename_namespace(path_was, path)
-
-    path
+    rescue => e
+      if path.nil?
+        say("Couldn't find a storage path for #{namespace_id}, #{path_was} -- skipping")
+      else
+        raise e
+      end
+    end
   end
 end

features/admin/groups.feature

-@admin
-Feature: Admin Groups
-  Background:
-    Given I sign in as an admin
-    And I have group with projects
-    And User "John Doe" exists
-    And I visit admin groups page
-
-  Scenario: See group list
-    Then I should be all groups
-
-  Scenario: Create a group
-    When I click new group link
-    And submit form with new group info
-    Then I should be redirected to group page
-    And I should see newly created group
-
-  @javascript
-  Scenario: Add user into projects in group
-    When I visit admin group page
-    When I select user "John Doe" from user list as "Reporter"
-    Then I should see "John Doe" in team list in every project as "Reporter"
-
-  Scenario: Shared projects
-    Given group has shared projects
-    When I visit group page
-    Then I should see project shared with group
-
-  @javascript
-  Scenario: Invite user to a group by e-mail
-    When I visit admin group page
-    When I select user "johndoe@gitlab.com" from user list as "Reporter"
-    Then I should see "johndoe@gitlab.com" in team list in every project as "Reporter"
-
-  @javascript
-  Scenario: Signed in admin should be able to add himself to a group
-    Given "John Doe" is owner of group "Owned"
-    When I visit group "Owned" members page
-    When I select current user as "Developer"
-    Then I should see current user as "Developer"
-
-  @javascript
-  Scenario: Signed in admin should be able to remove himself from group
-    Given current user is developer of group "Owned"
-    When I visit group "Owned" members page
-    Then I should see current user as "Developer"
-    When I click on the "Remove User From Group" button for current user
-    When I visit group "Owned" members page
-    Then I should not see current user as "Developer"

features/admin/settings.feature

-@admin
-Feature: Admin Settings
-  Background:
-    Given I sign in as an admin
-    And I visit admin settings page
-
-  Scenario: Help text
-    When I set the help text
-    Then I should see the help text
-    And I go to help page
-    Then I should see the help text
-    And I logout
-    Then I should see the help text

features/steps/admin/groups.rb

-class Spinach::Features::AdminGroups < Spinach::FeatureSteps
-  include SharedAuthentication
-  include SharedGroup
-  include SharedPaths
-  include SharedUser
-  include SharedActiveTab
-  include Select2Helper
-
-  When 'I visit admin group page' do
-    visit admin_group_path(current_group)
-  end
-
-  When 'I click new group link' do
-    click_link "New Group"
-  end
-
-  step 'I have group with projects' do
-    @group   = create(:group)
-    @project = create(:project, group: @group)
-    @event   = create(:closed_issue_event, project: @project)
-
-    @project.team << [current_user, :master]
-  end
-
-  step 'submit form with new group info' do
-    fill_in 'group_path', with: 'gitlab'
-    fill_in 'group_description', with: 'Group description'
-    click_button "Create group"
-  end
-
-  step 'I should see newly created group' do
-    expect(page).to have_content "Group: gitlab"
-    expect(page).to have_content "Group description"
-  end
-
-  step 'I should be redirected to group page' do
-    expect(current_path).to eq admin_group_path(Group.find_by(path: 'gitlab'))
-  end
-
-  When 'I select user "John Doe" from user list as "Reporter"' do
-    select2(user_john.id, from: "#user_ids", multiple: true)
-    page.within "#new_project_member" do
-      select "Reporter", from: "access_level"
-    end
-    click_button "Add users to group"
-  end
-
-  When 'I select user "johndoe@gitlab.com" from user list as "Reporter"' do
-    select2('johndoe@gitlab.com', from: "#user_ids", multiple: true)
-    page.within "#new_project_member" do
-      select "Reporter", from: "access_level"
-    end
-    click_button "Add users to group"
-  end
-
-  step 'I should see "John Doe" in team list in every project as "Reporter"' do
-    page.within ".group-users-list" do
-      expect(page).to have_content "John Doe"
-      expect(page).to have_content "Reporter"
-    end
-  end
-
-  step 'I should see "johndoe@gitlab.com" in team list in every project as "Reporter"' do
-    page.within ".group-users-list" do
-      expect(page).to have_content "johndoe@gitlab.com"
-      expect(page).to have_content "Invited by"
-      expect(page).to have_content "Reporter"
-    end
-  end
-
-  step 'I should be all groups' do
-    Group.all.each do |group|
-      expect(page).to have_content group.name
-    end
-  end
-
-  step 'group has shared projects' do
-    share_link = shared_project.project_group_links.new(group_access: Gitlab::Access::MASTER)
-    share_link.group_id = current_group.id
-    share_link.save!
-  end
-
-  step 'I visit group page' do
-    visit admin_group_path(current_group)
-  end
-
-  step 'I should see project shared with group' do
-    expect(page).to have_content(shared_project.name_with_namespace)
-    expect(page).to have_content "Projects shared with"
-  end
-
-  step 'we have user "John Doe" in group' do
-    current_group.add_reporter(user_john)
-  end
-
-  step 'I should not see "John Doe" in team list' do
-    page.within ".group-users-list" do
-      expect(page).not_to have_content "John Doe"
-    end
-  end
-
-  step 'I select current user as "Developer"' do
-    page.within ".users-group-form" do
-      select2(current_user.id, from: "#user_ids", multiple: true)
-      select "Developer", from: "access_level"
-    end
-
-    click_button "Add to group"
-  end
-
-  step 'I should see current user as "Developer"' do
-    page.within '.content-list' do
-      expect(page).to have_content(current_user.name)
-      expect(page).to have_content('Developer')
-    end
-  end
-
-  step 'I click on the "Remove User From Group" button for current user' do
-    find(:css, 'li', text: current_user.name).find(:css, 'a.btn-remove').click
-    # poltergeist always confirms popups.
-  end
-
-  step 'I should not see current user as "Developer"' do
-    page.within '.content-list' do
-      expect(page).not_to have_content(current_user.name)
-      expect(page).not_to have_content('Developer')
-    end
-  end
-
-  protected
-
-  def current_group
-    @group ||= Group.first
-  end
-
-  def shared_project
-    @shared_project ||= create(:empty_project)
-  end
-
-  def user_john
-    @user_john ||= User.find_by(name: "John Doe")
-  end
-end

features/steps/shared/paths.rb

@@ -195,10 +195,6 @@ module SharedPaths
     visit admin_background_jobs_path
   end
 
-  step 'I visit admin groups page' do
-    visit admin_groups_path
-  end
-
   step 'I visit admin teams page' do
     visit admin_teams_path
   end

lib/api/helpers.rb

@@ -301,7 +301,7 @@ module API
         header['X-Sendfile'] = path
         body
       else
-        file FileStreamer.new(path)
+        path
       end
     end
 

lib/api/projects.rb

@@ -163,7 +163,7 @@ module API
         use :sort_params
         use :pagination
       end
-      get "/search/:query" do
+      get "/search/:query", requirements: { query: /[^\/]+/ } do
         search_service = Search::GlobalService.new(current_user, search: params[:query]).execute
         projects = search_service.objects('projects', params[:page])
         projects = projects.reorder(params[:order_by] => params[:sort])
@@ -301,13 +301,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
 

spec/controllers/projects_controller_spec.rb

@@ -266,7 +266,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
 

spec/features/admin/admin_groups_spec.rb

 require 'spec_helper'
 
 feature 'Admin Groups', feature: true do
+  include Select2Helper
+
   let(:internal) { Gitlab::VisibilityLevel::INTERNAL }
+  let(:user) { create :user }
+  let!(:group) { create :group }
+  let!(:current_user) { login_as :admin }
 
   before do
-    login_as(:admin)
-
     stub_application_setting(default_group_visibility: internal)
   end
 
+  describe 'list' do
+    it 'renders groups' do
+      visit admin_groups_path
+
+      expect(page).to have_content(group.name)
+    end
+  end
+
   describe 'create a group' do
+    it 'creates new group' do
+      visit admin_groups_path
+
+      click_link "New Group"
+      fill_in 'group_path', with: 'gitlab'
+      fill_in 'group_description', with: 'Group description'
+      click_button "Create group"
+
+      expect(current_path).to eq admin_group_path(Group.find_by(path: 'gitlab'))
+      expect(page).to have_content('Group: gitlab')
+      expect(page).to have_content('Group description')
+    end
+
     scenario 'shows the visibility level radio populated with the default value' do
       visit new_admin_group_path
 
@@ -37,6 +61,91 @@ feature 'Admin Groups', feature: true do
     end
   end
 
+  describe 'add user into a group', js: true do
+    shared_context 'adds user into a group' do
+      it do
+        visit admin_group_path(group)
+
+        select2(user_selector, from: '#user_ids', multiple: true)
+        page.within '#new_project_member' do
+          select2(Gitlab::Access::REPORTER, from: '#access_level')
+        end
+        click_button "Add users to group"
+        page.within ".group-users-list" do
+          expect(page).to have_content(user.name)
+          expect(page).to have_content('Reporter')
+        end
+      end
+    end
+
+    it_behaves_like 'adds user into a group' do
+      let(:user_selector) { user.id }
+    end
+
+    it_behaves_like 'adds user into a group' do
+      let(:user_selector) { user.email }
+    end
+  end
+
+  describe 'add admin himself to a group' do
+    before do
+      group.add_user(:user, Gitlab::Access::OWNER)
+    end
+
+    it 'adds admin a to a group as developer', js: true do
+      visit group_group_members_path(group)
+
+      page.within '.users-group-form' do
+        select2(current_user.id, from: '#user_ids', multiple: true)
+        select 'Developer', from: 'access_level'
+      end
+
+      click_button 'Add to group'
+
+      page.within '.content-list' do
+        expect(page).to have_content(current_user.name)
+        expect(page).to have_content('Developer')
+      end
+    end
+  end
+
+  describe 'admin remove himself from a group', js: true do
+    it 'removes admin from the group' do
+      group.add_user(current_user, Gitlab::Access::DEVELOPER)
+
+      visit group_group_members_path(group)
+
+      page.within '.content-list' do
+        expect(page).to have_content(current_user.name)
+        expect(page).to have_content('Developer')
+      end
+
+      find(:css, 'li', text: current_user.name).find(:css, 'a.btn-remove').click
+
+      visit group_group_members_path(group)
+
+      page.within '.content-list' do
+        expect(page).not_to have_content(current_user.name)
+        expect(page).not_to have_content('Developer')
+      end
+    end
+  end
+
+  describe 'shared projects' do
+    it 'renders shared project' do
+      empty_project = create(:empty_project)
+      empty_project.project_group_links.create!(
+        group_access: Gitlab::Access::MASTER,
+        group: group
+      )
+
+      visit admin_group_path(group)
+
+      expect(page).to have_content(empty_project.name_with_namespace)
+      expect(page).to have_content('Projects shared with')
+    end
+  end
+
   def expect_selected_visibility(level)
     selector = "#group_visibility_level_#{level}[checked=checked]"
 

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

spec/migrations/remove_dot_git_from_usernames.rb → spec/migrations/remove_dot_git_from_usernames_spec.rb

@@ -5,17 +5,11 @@ require Rails.root.join('db', 'migrate', '20161226122833_remove_dot_git_from_use
 
 describe RemoveDotGitFromUsernames do
   let(:user) { create(:user) }
-
-  describe '#up' do
   let(:migration) { described_class.new }
 
+  describe '#up' do
     before do
-      namespace = user.namespace
-      namespace.path = 'test.git'
-      namespace.save!(validate: false)
-
-      user.username = 'test.git'
-      user.save!(validate: false)
+      update_namespace(user, 'test.git')
     end
 
     it 'renames user with .git in username' do
@@ -26,4 +20,38 @@ describe RemoveDotGitFromUsernames do
       expect(user.namespace.route.path).to eq('test_git')
     end
   end
+
+  context 'when new path exists already' do
+    describe '#up' do
+      let(:user2) { create(:user) }
+
+      before do
+        update_namespace(user, 'test.git')
+        update_namespace(user2, 'test_git')
+
+        storages = { 'default' => 'tmp/tests/custom_repositories' }
+
+        allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
+        allow(migration).to receive(:route_exists?).with('test_git').and_return(true)
+        allow(migration).to receive(:route_exists?).with('test_git1').and_return(false)
+      end
+
+      it 'renames user with .git in username' do
+        migration.up
+
+        expect(user.reload.username).to eq('test_git1')
+        expect(user.namespace.reload.path).to eq('test_git1')
+        expect(user.namespace.route.path).to eq('test_git1')
+      end
+    end
+  end
+
+  def update_namespace(user, path)
+    namespace = user.namespace
+    namespace.path = path
+    namespace.save!(validate: false)
+
+    user.username = path
+    user.save!(validate: false)
+  end
 end

spec/models/project_spec.rb

@@ -1886,11 +1886,13 @@ describe Project, models: true do
     end
   end
 
-  describe 'change_head' do
+  describe '#change_head' do
     let(:project) { create(:project) }
 
-    it 'calls the before_change_head method' do
+    it 'calls the before_change_head and after_change_head methods' do
       expect(project.repository).to receive(:before_change_head)
+      expect(project.repository).to receive(:after_change_head)
+
       project.change_head(project.default_branch)
     end
 
@@ -1906,11 +1908,6 @@ describe Project, models: true do
       project.change_head(project.default_branch)
     end
 
-    it 'expires the avatar cache' do
-      expect(project.repository).to receive(:expire_avatar_cache)
-      project.change_head(project.default_branch)
-    end
-
     it 'reloads the default branch' do
       expect(project).to receive(:reload_default_branch)
       project.change_head(project.default_branch)

spec/models/repository_spec.rb

@@ -1176,6 +1176,24 @@ describe Repository, models: true do
     end
   end
 
+  describe '#after_change_head' do
+    it 'flushes the readme cache' do
+      expect(repository).to receive(:expire_method_caches).with([
+        :readme,
+        :changelog,
+        :license,
+        :contributing,
+        :version,
+        :gitignore,
+        :koding,
+        :gitlab_ci,
+        :avatar
+      ])
+
+      repository.after_change_head
+    end
+  end
+
   describe '#before_push_tag' do
     it 'flushes the cache' do
       expect(repository).to receive(:expire_statistics_caches)
@@ -1581,14 +1599,6 @@ describe Repository, models: true do
     end
   end
 
-  describe '#expire_avatar_cache' do
-    it 'expires the cache' do
-      expect(repository).to receive(:expire_method_caches).with(%i(avatar))
-
-      repository.expire_avatar_cache
-    end
-  end
-
   describe '#file_on_head' do
     context 'with a non-existing repository' do
       it 'returns nil' do

spec/requests/api/projects_spec.rb

@@ -1095,32 +1095,37 @@ describe API::Projects, api: true  do
     let!(:unfound_internal) { create(:empty_project, :internal, name: 'unfound internal') }
     let!(:public)           { create(:empty_project, :public, name: "public #{query}") }
     let!(:unfound_public)   { create(:empty_project, :public, name: 'unfound public') }
+    let!(:one_dot_two)      { create(:empty_project, :public, name: "one.dot.two") }
 
     shared_examples_for 'project search response' do |args = {}|
       it 'returns project search responses' do
-        get api("/projects/search/#{query}", current_user)
+        get api("/projects/search/#{args[:query]}", current_user)
 
         expect(response).to have_http_status(200)
         expect(json_response).to be_an Array
         expect(json_response.size).to eq(args[:results])
-        json_response.each { |project| expect(project['name']).to match(args[:match_regex] || /.*query.*/) }
+        json_response.each { |project| expect(project['name']).to match(args[:match_regex] || /.*#{args[:query]}.*/) }
       end
     end
 
     context 'when unauthenticated' do
-      it_behaves_like 'project search response', results: 1 do
+      it_behaves_like 'project search response', query: 'query', results: 1 do
         let(:current_user) { nil }
       end
     end
 
     context 'when authenticated' do
-      it_behaves_like 'project search response', results: 6 do
+      it_behaves_like 'project search response', query: 'query', results: 6 do
         let(:current_user) { user }
       end
+      it_behaves_like 'project search response', query: 'one.dot.two', results: 1 do
+        let(:current_user) { user }
+      end
+      
     end
 
     context 'when authenticated as a different user' do
-      it_behaves_like 'project search response', results: 2, match_regex: /(internal|public) query/ do
+      it_behaves_like 'project search response', query: 'query', results: 2, match_regex: /(internal|public) query/ do
         let(:current_user) { user2 }
       end
     end

spec/services/projects/update_service_spec.rb

 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 }
+        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
-
-      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)
         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 }
 
-      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 }
-    end
+      expect(project).to be_internal
+      expect(forked_project).to be_internal
 
-    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
+      expect(update_project(project, admin, opts)).to eq({ status: :success })
+
+      expect(project).to be_private
+      expect(forked_project.reload).to be_private
     end
 
-      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 }
+    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
+
+      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
 
   describe 'repository_storage' do
@@ -172,6 +128,6 @@ describe Projects::UpdateService, services: true do
   end
 
   def update_project(project, user, opts)
-    Projects::UpdateService.new(project, user, opts).execute
+    described_class.new(project, user, opts).execute
   end
 end