Merge branch 'master' into 'docs-specific-review-examples'docs-specific-review-examples

# Conflicts:
#   doc/development/code_review.md

7 files changed, 271 insertions, 44 deletions

diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb
new file mode 100644
index 00000000000..402ea057cc5
--- /dev/null
+++ b/spec/requests/api/group_variables_spec.rb
@@ -0,0 +1,221 @@
+require 'spec_helper'
+
+describe API::GroupVariables do
+  let(:group) { create(:group) }
+  let(:user) { create(:user) }
+
+  describe 'GET /groups/:id/variables' do
+    let!(:variable) { create(:ci_group_variable, group: group) }
+
+    context 'authorized user with proper permissions' do
+      before do
+        group.add_master(user)
+      end
+
+      it 'returns group variables' do
+        get api("/groups/#{group.id}/variables", user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_a(Array)
+      end
+    end
+
+    context 'authorized user with invalid permissions' do
+      it 'does not return group variables' do
+        get api("/groups/#{group.id}/variables", user)
+
+        expect(response).to have_http_status(403)
+      end
+    end
+
+    context 'unauthorized user' do
+      it 'does not return group variables' do
+        get api("/groups/#{group.id}/variables")
+
+        expect(response).to have_http_status(401)
+      end
+    end
+  end
+
+  describe 'GET /groups/:id/variables/:key' do
+    let!(:variable) { create(:ci_group_variable, group: group) }
+
+    context 'authorized user with proper permissions' do
+      before do
+        group.add_master(user)
+      end
+
+      it 'returns group variable details' do
+        get api("/groups/#{group.id}/variables/#{variable.key}", user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response['value']).to eq(variable.value)
+        expect(json_response['protected']).to eq(variable.protected?)
+      end
+
+      it 'responds with 404 Not Found if requesting non-existing variable' do
+        get api("/groups/#{group.id}/variables/non_existing_variable", user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'authorized user with invalid permissions' do
+      it 'does not return group variable details' do
+        get api("/groups/#{group.id}/variables/#{variable.key}", user)
+
+        expect(response).to have_http_status(403)
+      end
+    end
+
+    context 'unauthorized user' do
+      it 'does not return group variable details' do
+        get api("/groups/#{group.id}/variables/#{variable.key}")
+
+        expect(response).to have_http_status(401)
+      end
+    end
+  end
+
+  describe 'POST /groups/:id/variables' do
+    context 'authorized user with proper permissions' do
+      let!(:variable) { create(:ci_group_variable, group: group) }
+
+      before do
+        group.add_master(user)
+      end
+
+      it 'creates variable' do
+        expect do
+          post api("/groups/#{group.id}/variables", user), key: 'TEST_VARIABLE_2', value: 'VALUE_2', protected: true
+        end.to change{group.variables.count}.by(1)
+
+        expect(response).to have_http_status(201)
+        expect(json_response['key']).to eq('TEST_VARIABLE_2')
+        expect(json_response['value']).to eq('VALUE_2')
+        expect(json_response['protected']).to be_truthy
+      end
+
+      it 'creates variable with optional attributes' do
+        expect do
+          post api("/groups/#{group.id}/variables", user), key: 'TEST_VARIABLE_2', value: 'VALUE_2'
+        end.to change{group.variables.count}.by(1)
+
+        expect(response).to have_http_status(201)
+        expect(json_response['key']).to eq('TEST_VARIABLE_2')
+        expect(json_response['value']).to eq('VALUE_2')
+        expect(json_response['protected']).to be_falsey
+      end
+
+      it 'does not allow to duplicate variable key' do
+        expect do
+          post api("/groups/#{group.id}/variables", user), key: variable.key, value: 'VALUE_2'
+        end.to change{group.variables.count}.by(0)
+
+        expect(response).to have_http_status(400)
+      end
+    end
+
+    context 'authorized user with invalid permissions' do
+      it 'does not create variable' do
+        post api("/groups/#{group.id}/variables", user)
+
+        expect(response).to have_http_status(403)
+      end
+    end
+
+    context 'unauthorized user' do
+      it 'does not create variable' do
+        post api("/groups/#{group.id}/variables")
+
+        expect(response).to have_http_status(401)
+      end
+    end
+  end
+
+  describe 'PUT /groups/:id/variables/:key' do
+    let!(:variable) { create(:ci_group_variable, group: group) }
+
+    context 'authorized user with proper permissions' do
+      before do
+        group.add_master(user)
+      end
+
+      it 'updates variable data' do
+        initial_variable = group.variables.first
+        value_before = initial_variable.value
+
+        put api("/groups/#{group.id}/variables/#{variable.key}", user), value: 'VALUE_1_UP', protected: true
+
+        updated_variable = group.variables.first
+
+        expect(response).to have_http_status(200)
+        expect(value_before).to eq(variable.value)
+        expect(updated_variable.value).to eq('VALUE_1_UP')
+        expect(updated_variable).to be_protected
+      end
+
+      it 'responds with 404 Not Found if requesting non-existing variable' do
+        put api("/groups/#{group.id}/variables/non_existing_variable", user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'authorized user with invalid permissions' do
+      it 'does not update variable' do
+        put api("/groups/#{group.id}/variables/#{variable.key}", user)
+
+        expect(response).to have_http_status(403)
+      end
+    end
+
+    context 'unauthorized user' do
+      it 'does not update variable' do
+        put api("/groups/#{group.id}/variables/#{variable.key}")
+
+        expect(response).to have_http_status(401)
+      end
+    end
+  end
+
+  describe 'DELETE /groups/:id/variables/:key' do
+    let!(:variable) { create(:ci_group_variable, group: group) }
+
+    context 'authorized user with proper permissions' do
+      before do
+        group.add_master(user)
+      end
+
+      it 'deletes variable' do
+        expect do
+          delete api("/groups/#{group.id}/variables/#{variable.key}", user)
+
+          expect(response).to have_http_status(204)
+        end.to change{group.variables.count}.by(-1)
+      end
+
+      it 'responds with 404 Not Found if requesting non-existing variable' do
+        delete api("/groups/#{group.id}/variables/non_existing_variable", user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'authorized user with invalid permissions' do
+      it 'does not delete variable' do
+        delete api("/groups/#{group.id}/variables/#{variable.key}", user)
+
+        expect(response).to have_http_status(403)
+      end
+    end
+
+    context 'unauthorized user' do
+      it 'does not delete variable' do
+        delete api("/groups/#{group.id}/variables/#{variable.key}")
+
+        expect(response).to have_http_status(401)
+      end
+    end
+  end
+end
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index ce9b9ac1eb3..fb312d3cb7d 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -301,7 +301,7 @@ describe API::Internal do
 
         context 'project as /namespace/project' do
           it do
-            pull(key, project_with_repo_path('/' + project.path_with_namespace))
+            pull(key, project_with_repo_path('/' + project.full_path))
 
             expect(response).to have_http_status(200)
             expect(json_response["status"]).to be_truthy
@@ -312,7 +312,7 @@ describe API::Internal do
 
         context 'project as namespace/project' do
           it do
-            pull(key, project_with_repo_path(project.path_with_namespace))
+            pull(key, project_with_repo_path(project.full_path))
 
             expect(response).to have_http_status(200)
             expect(json_response["status"]).to be_truthy
diff --git a/spec/requests/api/notification_settings_spec.rb b/spec/requests/api/notification_settings_spec.rb
index f619b7e6eaf..d0e7a82e607 100644
--- a/spec/requests/api/notification_settings_spec.rb
+++ b/spec/requests/api/notification_settings_spec.rb
@@ -72,8 +72,8 @@ describe API::NotificationSettings do
 
       expect(response).to have_http_status(200)
       expect(json_response['level']).to eq(user.reload.notification_settings_for(project).level)
-      expect(json_response['events']['new_note']).to eq(true)
-      expect(json_response['events']['new_issue']).to eq(false)
+      expect(json_response['events']['new_note']).to be_truthy
+      expect(json_response['events']['new_issue']).to be_falsey
     end
   end
 
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 66b165b438b..2dc7be22f8f 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -16,38 +16,44 @@ describe API::Users do
       it "returns authorization error when the `username` parameter is not passed" do
         get api("/users")
 
-        expect(response).to have_http_status(403)
+        expect(response).to have_gitlab_http_status(403)
       end
 
       it "returns the user when a valid `username` parameter is passed" do
-        user = create(:user)
-
         get api("/users"), username: user.username
 
-        expect(response).to have_http_status(200)
+        expect(response).to have_gitlab_http_status(200)
         expect(json_response).to be_an Array
         expect(json_response.size).to eq(1)
         expect(json_response[0]['id']).to eq(user.id)
         expect(json_response[0]['username']).to eq(user.username)
       end
 
-      it "returns authorization error when the `username` parameter refers to an inaccessible user" do
-        user = create(:user)
-
-        stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
-
-        get api("/users"), username: user.username
-
-        expect(response).to have_http_status(403)
-      end
-
       it "returns an empty response when an invalid `username` parameter is passed" do
         get api("/users"), username: 'invalid'
 
-        expect(response).to have_http_status(200)
+        expect(response).to have_gitlab_http_status(200)
         expect(json_response).to be_an Array
         expect(json_response.size).to eq(0)
       end
+
+      context "when public level is restricted" do
+        before do
+          stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
+        end
+
+        it "returns authorization error when the `username` parameter refers to an inaccessible user" do
+          get api("/users"), username: user.username
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+
+        it "returns authorization error when the `username` parameter is not passed" do
+          get api("/users")
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
     end
 
     context "when authenticated" do
@@ -58,10 +64,10 @@ describe API::Users do
         end
 
         context 'when authenticate as a regular user' do
-          it "renders 403" do
+          it "renders 200" do
             get api("/users", user)
 
-            expect(response).to have_gitlab_http_status(403)
+            expect(response).to have_gitlab_http_status(200)
           end
         end
 
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index d4a3e8b13e1..d5c16d8f601 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -123,7 +123,7 @@ describe 'Git HTTP requests' do
 
     context "when requesting the Wiki" do
       let(:wiki) { ProjectWiki.new(project) }
-      let(:path) { "/#{wiki.repository.path_with_namespace}.git" }
+      let(:path) { "/#{wiki.repository.full_path}.git" }
 
       context "when the project is public" do
         let(:project) { create(:project, :repository, :public, :wiki_enabled) }
@@ -139,7 +139,7 @@ describe 'Git HTTP requests' do
             download(path) do |response|
               json_body = ActiveSupport::JSON.decode(response.body)
 
-              expect(json_body['RepoPath']).to include(wiki.repository.path_with_namespace)
+              expect(json_body['RepoPath']).to include(wiki.repository.full_path)
             end
           end
         end
@@ -222,7 +222,7 @@ describe 'Git HTTP requests' do
     end
 
     context "when the project exists" do
-      let(:path) { "#{project.path_with_namespace}.git" }
+      let(:path) { "#{project.full_path}.git" }
 
       context "when the project is public" do
         let(:project) { create(:project, :repository, :public) }
@@ -286,7 +286,7 @@ describe 'Git HTTP requests' do
         context 'when the request is not from gitlab-workhorse' do
           it 'raises an exception' do
             expect do
-              get("/#{project.path_with_namespace}.git/info/refs?service=git-upload-pack")
+              get("/#{project.full_path}.git/info/refs?service=git-upload-pack")
             end.to raise_error(JWT::DecodeError)
           end
         end
@@ -294,7 +294,7 @@ describe 'Git HTTP requests' do
         context 'when the repo is public' do
           context 'but the repo is disabled' do
             let(:project) { create(:project, :public, :repository, :repository_disabled) }
-            let(:path) { "#{project.path_with_namespace}.git" }
+            let(:path) { "#{project.full_path}.git" }
             let(:env) { {} }
 
             it_behaves_like 'pulls require Basic HTTP Authentication'
@@ -303,7 +303,7 @@ describe 'Git HTTP requests' do
 
           context 'but the repo is enabled' do
             let(:project) { create(:project, :public, :repository, :repository_enabled) }
-            let(:path) { "#{project.path_with_namespace}.git" }
+            let(:path) { "#{project.full_path}.git" }
             let(:env) { {} }
 
             it_behaves_like 'pulls are allowed'
@@ -421,7 +421,7 @@ describe 'Git HTTP requests' do
                   @token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "api")
                 end
 
-                let(:path) { "#{project.path_with_namespace}.git" }
+                let(:path) { "#{project.full_path}.git" }
                 let(:env) { { user: 'oauth2', password: @token.token } }
 
                 it_behaves_like 'pulls are allowed'
@@ -431,7 +431,7 @@ describe 'Git HTTP requests' do
               context 'when user has 2FA enabled' do
                 let(:user) { create(:user, :two_factor) }
                 let(:access_token) { create(:personal_access_token, user: user) }
-                let(:path) { "#{project.path_with_namespace}.git" }
+                let(:path) { "#{project.full_path}.git" }
 
                 before do
                   project.team << [user, :master]
@@ -580,7 +580,7 @@ describe 'Git HTTP requests' do
           end
 
           context 'when build created by system is authenticated' do
-            let(:path) { "#{project.path_with_namespace}.git" }
+            let(:path) { "#{project.full_path}.git" }
             let(:env) { { user: 'gitlab-ci-token', password: build.token } }
 
             it_behaves_like 'pulls are allowed'
@@ -602,7 +602,7 @@ describe 'Git HTTP requests' do
             # We are "authenticated" as CI using a valid token here. But we are
             # not authorized to see any other project, so return "not found".
             it "rejects pulls for other project with 404 Not Found" do
-              clone_get("#{other_project.path_with_namespace}.git", env)
+              clone_get("#{other_project.full_path}.git", env)
 
               expect(response).to have_http_status(:not_found)
               expect(response.body).to eq(git_access_error(:project_not_found))
@@ -616,7 +616,7 @@ describe 'Git HTTP requests' do
             end
 
             shared_examples 'can download code only' do
-              let(:path) { "#{project.path_with_namespace}.git" }
+              let(:path) { "#{project.full_path}.git" }
               let(:env) { { user: 'gitlab-ci-token', password: build.token } }
 
               it_behaves_like 'pulls are allowed'
@@ -646,7 +646,7 @@ describe 'Git HTTP requests' do
               it_behaves_like 'can download code only'
 
               it 'downloads from other project get status 403' do
-                clone_get "#{other_project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
+                clone_get "#{other_project.full_path}.git", user: 'gitlab-ci-token', password: build.token
 
                 expect(response).to have_http_status(:forbidden)
               end
@@ -658,7 +658,7 @@ describe 'Git HTTP requests' do
               it_behaves_like 'can download code only'
 
               it 'downloads from other project get status 404' do
-                clone_get "#{other_project.path_with_namespace}.git", user: 'gitlab-ci-token', password: build.token
+                clone_get "#{other_project.full_path}.git", user: 'gitlab-ci-token', password: build.token
 
                 expect(response).to have_http_status(:not_found)
               end
@@ -671,7 +671,7 @@ describe 'Git HTTP requests' do
         let(:project) { create(:project, :repository, :public, path: 'project.git-project') }
 
         context "GET info/refs" do
-          let(:path) { "/#{project.path_with_namespace}/info/refs" }
+          let(:path) { "/#{project.full_path}/info/refs" }
 
           context "when no params are added" do
             before do
@@ -679,7 +679,7 @@ describe 'Git HTTP requests' do
             end
 
             it "redirects to the .git suffix version" do
-              expect(response).to redirect_to("/#{project.path_with_namespace}.git/info/refs")
+              expect(response).to redirect_to("/#{project.full_path}.git/info/refs")
             end
           end
 
@@ -691,7 +691,7 @@ describe 'Git HTTP requests' do
             end
 
             it "redirects to the .git suffix version" do
-              expect(response).to redirect_to("/#{project.path_with_namespace}.git/info/refs?service=#{params[:service]}")
+              expect(response).to redirect_to("/#{project.full_path}.git/info/refs?service=#{params[:service]}")
             end
           end
 
@@ -703,7 +703,7 @@ describe 'Git HTTP requests' do
             end
 
             it "redirects to the .git suffix version" do
-              expect(response).to redirect_to("/#{project.path_with_namespace}.git/info/refs?service=#{params[:service]}")
+              expect(response).to redirect_to("/#{project.full_path}.git/info/refs?service=#{params[:service]}")
             end
           end
 
@@ -722,13 +722,13 @@ describe 'Git HTTP requests' do
 
         context "POST git-upload-pack" do
           it "fails to find a route" do
-            expect { clone_post(project.path_with_namespace) }.to raise_error(ActionController::RoutingError)
+            expect { clone_post(project.full_path) }.to raise_error(ActionController::RoutingError)
           end
         end
 
         context "POST git-receive-pack" do
           it "fails to find a route" do
-            expect { push_post(project.path_with_namespace) }.to raise_error(ActionController::RoutingError)
+            expect { push_post(project.full_path) }.to raise_error(ActionController::RoutingError)
           end
         end
       end
@@ -744,7 +744,7 @@ describe 'Git HTTP requests' do
               Blob.decorate(Gitlab::Git::Blob.find(project.repository, 'master', 'bar/branch-test.txt'), project)
             end
 
-            get "/#{project.path_with_namespace}/blob/master/info/refs"
+            get "/#{project.full_path}/blob/master/info/refs"
           end
 
           it "returns the file" do
@@ -754,7 +754,7 @@ describe 'Git HTTP requests' do
 
         context "when the file does not exist" do
           before do
-            get "/#{project.path_with_namespace}/blob/master/info/refs"
+            get "/#{project.full_path}/blob/master/info/refs"
           end
 
           it "returns not found" do
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index 697b150ab34..4f1a90750b3 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -701,7 +701,7 @@ describe 'Git LFS API and storage' do
               expect(json_response['objects']).to be_kind_of(Array)
               expect(json_response['objects'].first['oid']).to eq("91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897")
               expect(json_response['objects'].first['size']).to eq(1575078)
-              expect(json_response['objects'].first['actions']['upload']['href']).to eq("#{Gitlab.config.gitlab.url}/#{project.path_with_namespace}.git/gitlab-lfs/objects/91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897/1575078")
+              expect(json_response['objects'].first['actions']['upload']['href']).to eq("#{Gitlab.config.gitlab.url}/#{project.full_path}.git/gitlab-lfs/objects/91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897/1575078")
               expect(json_response['objects'].first['actions']['upload']['header']).to eq('Authorization' => authorization)
             end
           end
diff --git a/spec/requests/request_profiler_spec.rb b/spec/requests/request_profiler_spec.rb
index 51fbfecec4b..9afeb2983b0 100644
--- a/spec/requests/request_profiler_spec.rb
+++ b/spec/requests/request_profiler_spec.rb
@@ -15,7 +15,7 @@ describe 'Request Profiler' do
     it 'creates a profile of the request' do
       project = create(:project, namespace: user.namespace)
       time    = Time.now
-      path    = "/#{project.path_with_namespace}"
+      path    = "/#{project.full_path}"
 
       Timecop.freeze(time) do
         get path, nil, 'X-Profile-Token' => Gitlab::RequestProfiler.profile_token