applies relevant changes to the code and code structure

5 files changed, 83 insertions, 61 deletions

diff --git a/spec/features/admin/admin_users_personal_access_tokens_spec.rb b/spec/features/admin/admin_users_impersonation_tokens_spec.rb
index 772aeebf43f..c37cf1178df 100644
--- a/spec/features/admin/admin_users_personal_access_tokens_spec.rb
+++ b/spec/features/admin/admin_users_impersonation_tokens_spec.rb
@@ -1,19 +1,19 @@
 require 'spec_helper'
 
-describe 'Admin > Users > Personal Access Tokens', feature: true, js: true do
+describe 'Admin > Users > Impersonation Tokens', feature: true, js: true do
   let(:admin) { create(:admin) }
   let!(:user) { create(:user) }
 
   def active_personal_access_tokens
-    find(".table.active-personal-access-tokens")
+    find(".table.active-impersonation-tokens")
   end
 
   def inactive_personal_access_tokens
-    find(".table.inactive-personal-access-tokens")
+    find(".table.inactive-impersonation-tokens")
   end
 
   def created_personal_access_token
-    find("#created-personal-access-token").value
+    find("#created-impersonation-token").value
   end
 
   def disallow_personal_access_token_saves!
@@ -28,7 +28,7 @@ describe 'Admin > Users > Personal Access Tokens', feature: true, js: true do
     it "allows creation of a token" do
       name = FFaker::Product.brand
 
-      visit admin_user_personal_access_tokens_path(user_id: user.username)
+      visit admin_user_impersonation_tokens_path(user_id: user.username)
       fill_in "Name", with: name
 
       # Set date to 1st of next month
@@ -40,31 +40,20 @@ describe 'Admin > Users > Personal Access Tokens', feature: true, js: true do
       check "api"
       check "read_user"
 
-      click_on "Create Personal Access Token"
+      expect { click_on "Create Impersonation Token" }.to change { PersonalAccessToken.impersonation.count }
       expect(active_personal_access_tokens).to have_text(name)
       expect(active_personal_access_tokens).to have_text('In')
       expect(active_personal_access_tokens).to have_text('api')
       expect(active_personal_access_tokens).to have_text('read_user')
-      expect(active_personal_access_tokens).to have_text('true')
-    end
-
-    context "when creation fails" do
-      it "displays an error message" do
-        disallow_personal_access_token_saves!
-        visit admin_user_personal_access_tokens_path(user_id: user.username)
-        fill_in "Name", with: FFaker::Product.brand
-
-        expect { click_on "Create Personal Access Token" }.not_to change { PersonalAccessToken.count }
-        expect(page).to have_content("Name cannot be nil")
-      end
     end
   end
 
   describe "inactive tokens" do
-    let!(:personal_access_token) { create(:personal_access_token, user: user) }
+    let!(:personal_access_token) { create(:impersonation_personal_access_token, user: user) }
+
+    it "allows revocation of an active impersonation token" do
+      visit admin_user_impersonation_tokens_path(user_id: user.username)
 
-    it "allows revocation of an active token" do
-      visit admin_user_personal_access_tokens_path(user_id: user.username)
       click_on "Revoke"
 
       expect(inactive_personal_access_tokens).to have_text(personal_access_token.name)
@@ -72,17 +61,20 @@ describe 'Admin > Users > Personal Access Tokens', feature: true, js: true do
 
     it "moves expired tokens to the 'inactive' section" do
       personal_access_token.update(expires_at: 5.days.ago)
-      visit admin_user_personal_access_tokens_path(user_id: user.username)
+
+      visit admin_user_impersonation_tokens_path(user_id: user.username)
 
       expect(inactive_personal_access_tokens).to have_text(personal_access_token.name)
     end
 
     context "when revocation fails" do
+      before { disallow_personal_access_token_saves! }
+
       it "displays an error message" do
-        disallow_personal_access_token_saves!
-        visit admin_user_personal_access_tokens_path(user_id: user.username)
+        visit admin_user_impersonation_tokens_path(user_id: user.username)
 
         click_on "Revoke"
+
         expect(active_personal_access_tokens).to have_text(personal_access_token.name)
         expect(page).to have_content("Could not revoke")
       end
diff --git a/spec/features/profiles/personal_access_tokens_spec.rb b/spec/features/profiles/personal_access_tokens_spec.rb
index bce4f7c9f3d..74e4e157dc5 100644
--- a/spec/features/profiles/personal_access_tokens_spec.rb
+++ b/spec/features/profiles/personal_access_tokens_spec.rb
@@ -26,7 +26,7 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do
   end
 
   describe "token creation" do
-    it "allows creation of a token" do
+    it "allows creation of a non impersonation token" do
       name = FFaker::Product.brand
 
       visit profile_personal_access_tokens_path
@@ -60,6 +60,18 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do
     end
   end
 
+  describe 'active tokens' do
+    let!(:impersonation_token) { create(:impersonation_personal_access_token, user: user) }
+    let!(:personal_access_token) { create(:personal_access_token, user: user) }
+
+    it 'only shows non impersonated tokens' do
+      visit profile_personal_access_tokens_path
+
+      expect(active_personal_access_tokens).to have_text(personal_access_token.name)
+      expect(active_personal_access_tokens).not_to have_text(impersonation_token.name)
+    end
+  end
+
   describe "inactive tokens" do
     let!(:personal_access_token) { create(:personal_access_token, user: user) }
 
diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index c10c3bc3f31..b98a4d7fd1c 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -1,18 +1,21 @@
 require 'spec_helper'
 
 describe PersonalAccessToken, models: true do
-  describe ".generate" do
-    it "generates a random token" do
-      personal_access_token = PersonalAccessToken.generate({})
-      expect(personal_access_token.token).to be_present
+  describe '.build' do
+    let(:personal_access_token) { build(:personal_access_token) }
+    let(:invalid_personal_access_token) { build(:personal_access_token, token: nil) }
+
+    it 'is a valid personal access token' do
+      expect(personal_access_token).to be_valid
     end
 
-    it "doesn't save the record" do
-      personal_access_token = PersonalAccessToken.generate({})
-      expect(personal_access_token).not_to be_persisted
+    it 'ensures that the token is generated' do
+      invalid_personal_access_token.save!
+
+      expect(invalid_personal_access_token).to be_valid
+      expect(invalid_personal_access_token.token).not_to be_nil
     end
   end
-
   describe ".active?" do
     let(:active_personal_access_token) { build(:personal_access_token) }
     let(:revoked_personal_access_token) { build(:revoked_personal_access_token) }
diff --git a/spec/requests/api/personal_access_tokens_spec.rb b/spec/requests/api/personal_access_tokens_spec.rb
index f7a89a6539c..98c8794efa4 100644
--- a/spec/requests/api/personal_access_tokens_spec.rb
+++ b/spec/requests/api/personal_access_tokens_spec.rb
@@ -16,7 +16,7 @@ describe API::PersonalAccessTokens, api: true  do
 
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
-      expect(json_response.size).to eq(3)
+      expect(json_response.size).to eq(user.personal_access_tokens.count)
 
       json_personal_access_token = json_response.detect do |personal_access_token|
         personal_access_token['id'] == active_personal_access_token.id
@@ -73,7 +73,7 @@ describe API::PersonalAccessTokens, api: true  do
       expect(json_response['active']).to eq(false)
       expect(json_response['revoked']).to eq(false)
       expect(json_response['token']).to be_present
-      expect(PersonalAccessToken.find(personal_access_token_id)).not_to eq(nil)
+      expect(PersonalAccessToken.find(personal_access_token_id)).not_to be_nil
     end
   end
 
@@ -85,14 +85,14 @@ describe API::PersonalAccessTokens, api: true  do
       get api("/personal_access_tokens/#{not_found_token}", user)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'returns a 404 error if personal access token exists but it is a personal access tokens of another user' do
       get api("/personal_access_tokens/#{personal_access_token_of_another_user.id}", user)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'returns a personal access token and does not expose token in the json response' do
@@ -111,14 +111,14 @@ describe API::PersonalAccessTokens, api: true  do
       delete api("/personal_access_tokens/#{not_found_token}", user)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'returns a 404 error if personal access token exists but it is a personal access tokens of another user' do
       delete api("/personal_access_tokens/#{personal_access_token_of_another_user.id}", user)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'revokes a personal access token and does not expose token in the json response' do
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 0ebd5eb872e..f5b6d30b9f6 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -1158,7 +1158,7 @@ describe API::Users, api: true  do
     end
   end
 
-  describe 'GET /users/:user_id/personal_access_tokens' do
+  describe 'GET /users/:id/personal_access_tokens' do
     let!(:active_personal_access_token) { create(:personal_access_token, user: user) }
     let!(:revoked_personal_access_token) { create(:revoked_personal_access_token, user: user) }
     let!(:expired_personal_access_token) { create(:expired_personal_access_token, user: user) }
@@ -1178,12 +1178,12 @@ describe API::Users, api: true  do
       expect(json_response['message']).to eq('403 Forbidden')
     end
 
-    it 'returns an array of personal access tokens' do
+    it 'returns an array of non impersonated personal access tokens' do
       get api("/users/#{user.id}/personal_access_tokens", admin)
 
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
-      expect(json_response.size).to eq(4)
+      expect(json_response.size).to eq(user.personal_access_tokens.count)
       expect(json_response.detect do |personal_access_token|
         personal_access_token['id'] == active_personal_access_token.id
       end['token']).to eq(active_personal_access_token.token)
@@ -1194,6 +1194,7 @@ describe API::Users, api: true  do
 
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
+      expect(json_response.size).to eq(user.personal_access_tokens.active.count)
       expect(json_response).to all(include('active' => true))
     end
 
@@ -1202,6 +1203,7 @@ describe API::Users, api: true  do
 
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
+      expect(json_response.size).to eq(user.personal_access_tokens.inactive.count)
       expect(json_response).to all(include('active' => false))
     end
 
@@ -1210,17 +1212,18 @@ describe API::Users, api: true  do
 
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
+      expect(json_response.size).to eq(user.personal_access_tokens.impersonation.count)
       expect(json_response).to all(include('impersonation' => true))
     end
   end
 
-  describe 'POST /users/:user_id/personal_access_tokens' do
+  describe 'POST /users/:id/personal_access_tokens' do
     let(:name) { 'my new pat' }
     let(:expires_at) { '2016-12-28' }
     let(:scopes) { ['api', 'read_user'] }
     let(:impersonation) { true }
 
-    it 'returns validation error if personal access token miss some attributes' do
+    it 'returns validation error if personal access token misses some attributes' do
       post api("/users/#{user.id}/personal_access_tokens", admin)
 
       expect(response).to have_http_status(400)
@@ -1253,23 +1256,20 @@ describe API::Users, api: true  do
         impersonation: impersonation
 
       expect(response).to have_http_status(201)
-
-      personal_access_token_id = json_response['id']
-
       expect(json_response['name']).to eq(name)
       expect(json_response['scopes']).to eq(scopes)
       expect(json_response['expires_at']).to eq(expires_at)
       expect(json_response['id']).to be_present
       expect(json_response['created_at']).to be_present
-      expect(json_response['active']).to eq(false)
-      expect(json_response['revoked']).to eq(false)
+      expect(json_response['active']).to be_falsey
+      expect(json_response['revoked']).to be_falsey
       expect(json_response['token']).to be_present
       expect(json_response['impersonation']).to eq(impersonation)
-      expect(PersonalAccessToken.and_impersonation_tokens.find(personal_access_token_id)).not_to eq(nil)
+      expect(PersonalAccessToken.with_impersonation_tokens.find(json_response['id'])).not_to be_nil
     end
   end
 
-  describe 'GET /users/:user_id/personal_access_tokens/:personal_access_token_id' do
+  describe 'GET /users/:id/personal_access_tokens/:personal_access_token_id' do
     let!(:personal_access_token) { create(:personal_access_token, user: user, revoked: false) }
     let!(:impersonation_token) { create(:impersonation_personal_access_token, user: user, revoked: false) }
 
@@ -1284,7 +1284,7 @@ describe API::Users, api: true  do
       get api("/users/#{user.id}/personal_access_tokens/#{not_existing_pat_id}", admin)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'returns a 403 error when authenticated as normal user' do
@@ -1299,17 +1299,24 @@ describe API::Users, api: true  do
 
       expect(response).to have_http_status(200)
       expect(json_response['token']).to be_present
+      expect(json_response['impersonation']).to be_falsey
     end
 
-    it 'returns an impersonation token' do
+    it 'does not return an impersonation token without the specified field' do
       get api("/users/#{user.id}/personal_access_tokens/#{impersonation_token.id}", admin)
 
+      expect(response).to have_http_status(404)
+    end
+
+    it 'returns an impersonation token' do
+      get api("/users/#{user.id}/personal_access_tokens/#{impersonation_token.id}?impersonation=true", admin)
+
       expect(response).to have_http_status(200)
-      expect(json_response['impersonation']).to eq(true)
+      expect(json_response['impersonation']).to be_truthy
     end
   end
 
-  describe 'DELETE /users/:user_id/personal_access_tokens/:personal_access_token_id' do
+  describe 'DELETE /users/:id/personal_access_tokens/:personal_access_token_id' do
     let!(:personal_access_token) { create(:personal_access_token, user: user, revoked: false) }
     let!(:impersonation_token) { create(:impersonation_personal_access_token, user: user, revoked: false) }
 
@@ -1324,7 +1331,7 @@ describe API::Users, api: true  do
       delete api("/users/#{user.id}/personal_access_tokens/#{not_existing_pat_id}", admin)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'returns a 403 error when authenticated as normal user' do
@@ -1338,16 +1345,24 @@ describe API::Users, api: true  do
       delete api("/users/#{user.id}/personal_access_tokens/#{personal_access_token.id}", admin)
 
       expect(response).to have_http_status(204)
-      expect(personal_access_token.revoked).to eq(false)
-      expect(personal_access_token.reload.revoked).to eq(true)
+      expect(personal_access_token.revoked).to be_falsey
+      expect(personal_access_token.reload.revoked).to be_truthy
     end
 
-    it 'revokes an impersonation token' do
+    it 'does not find impersonated token without specified field' do
       delete api("/users/#{user.id}/personal_access_tokens/#{impersonation_token.id}", admin)
 
+      expect(response).to have_http_status(404)
+      expect(impersonation_token.revoked).to be_falsey
+      expect(impersonation_token.reload.revoked).to be_falsey
+    end
+
+    it 'revokes an impersonation token' do
+      delete api("/users/#{user.id}/personal_access_tokens/#{impersonation_token.id}?impersonation=true", admin)
+
       expect(response).to have_http_status(204)
-      expect(impersonation_token.revoked).to eq(false)
-      expect(impersonation_token.reload.revoked).to eq(true)
+      expect(impersonation_token.revoked).to be_falsey
+      expect(impersonation_token.reload.revoked).to be_truthy
     end
   end
 end