Merge branch 'dz-refactor-project-members-controller-spec' into 'master'

Refactor project_members_controller_spec

Make tests more readable, DRY and closer to RSpec best practices.

Same as https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6985 but for project_members_controller_spec now

See merge request !6989

spec/controllers/projects/project_members_controller_spec.rb

 require('spec_helper')
 
 describe Projects::ProjectMembersController do
-  describe '#apply_import' do
-    let(:project) { create(:project) }
-    let(:another_project) { create(:project, :private) }
-    let(:user) { create(:user) }
-    let(:member) { create(:user) }
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :public) }
 
-    before do
-      project.team << [user, :master]
-      another_project.team << [member, :guest]
-      sign_in(user)
-    end
+  describe 'GET index' do
+    it 'renders index with 200 status code' do
+      get :index, namespace_id: project.namespace, project_id: project
 
-    shared_context 'import applied' do
-      before do
-        post(:apply_import, namespace_id: project.namespace,
-                            project_id: project,
-                            source_project_id: another_project.id)
-      end
-    end
-
-    context 'when user can access source project members' do
-      before { another_project.team << [user, :guest] }
-      include_context 'import applied'
-
-      it 'imports source project members' do
-        expect(project.team_members).to include member
-        expect(response).to set_flash.to 'Successfully imported'
-        expect(response).to redirect_to(
-          namespace_project_project_members_path(project.namespace, project)
-        )
-      end
-    end
-
-    context 'when user is not member of a source project' do
-      include_context 'import applied'
-
-      it 'does not import team members' do
-        expect(project.team_members).not_to include member
-      end
-
-      it 'responds with not found' do
-        expect(response.status).to eq 404
-      end
+      expect(response).to have_http_status(200)
+      expect(response).to render_template(:index)
     end
   end
 
-  describe '#index' do
-    context 'when user is member' do
-      before do
-        project = create(:project, :private)
-        member = create(:user)
-        project.team << [member, :guest]
-        sign_in(member)
-
-        get :index, namespace_id: project.namespace, project_id: project
-      end
-
-      it { expect(response).to have_http_status(200) }
-    end
-  end
+  describe 'DELETE destroy' do
+    let(:member) { create(:project_member, :developer, project: project) }
 
-  describe '#destroy' do
-    let(:project) { create(:project, :public) }
+    before { sign_in(user) }
 
     context 'when member is not found' do
       it 'returns 404' do
@@ -76,18 +29,8 @@ describe Projects::ProjectMembersController do
     end
 
     context 'when member is found' do
-      let(:user) { create(:user) }
-      let(:team_user) { create(:user) }
-      let(:member) do
-        project.team << [team_user, :developer]
-        project.members.find_by(user_id: team_user.id)
-      end
-
       context 'when user does not have enough rights' do
-        before do
-          project.team << [user, :developer]
-          sign_in(user)
-        end
+        before { project.team << [user, :developer] }
 
         it 'returns 404' do
           delete :destroy, namespace_id: project.namespace,
@@ -95,15 +38,12 @@ describe Projects::ProjectMembersController do
                            id: member
 
           expect(response).to have_http_status(404)
-          expect(project.users).to include team_user
+          expect(project.members).to include member
         end
       end
 
       context 'when user has enough rights' do
-        before do
-          project.team << [user, :master]
-          sign_in(user)
-        end
+        before { project.team << [user, :master] }
 
         it '[HTML] removes user from members' do
           delete :destroy, namespace_id: project.namespace,
@@ -113,7 +53,7 @@ describe Projects::ProjectMembersController do
           expect(response).to redirect_to(
             namespace_project_project_members_path(project.namespace, project)
           )
-          expect(project.users).not_to include team_user
+          expect(project.members).not_to include member
         end
 
         it '[JS] removes user from members' do
@@ -122,19 +62,16 @@ describe Projects::ProjectMembersController do
                                  id: member
 
           expect(response).to be_success
-          expect(project.users).not_to include team_user
+          expect(project.members).not_to include member
         end
       end
     end
   end
 
-  describe '#leave' do
-    let(:project) { create(:project, :public) }
-    let(:user) { create(:user) }
+  describe 'DELETE leave' do
+    before { sign_in(user) }
 
     context 'when member is not found' do
-      before { sign_in(user) }
-
       it 'returns 404' do
         delete :leave, namespace_id: project.namespace,
                        project_id: project
@@ -145,10 +82,7 @@ describe Projects::ProjectMembersController do
 
     context 'when member is found' do
       context 'and is not an owner' do
-        before do
-          project.team << [user, :developer]
-          sign_in(user)
-        end
+        before { project.team << [user, :developer] }
 
         it 'removes user from members' do
           delete :leave, namespace_id: project.namespace,
@@ -161,11 +95,9 @@ describe Projects::ProjectMembersController do
       end
 
       context 'and is an owner' do
-        before do
-          project.update(namespace_id: user.namespace_id)
-          project.team << [user, :master, user]
-          sign_in(user)
-        end
+        let(:project) { create(:project, namespace: user.namespace) }
+
+        before { project.team << [user, :master] }
 
         it 'cannot remove himself from the project' do
           delete :leave, namespace_id: project.namespace,
@@ -176,10 +108,7 @@ describe Projects::ProjectMembersController do
       end
 
       context 'and is a requester' do
-        before do
-          project.request_access(user)
-          sign_in(user)
-        end
+        before { project.request_access(user) }
 
         it 'removes user from members' do
           delete :leave, namespace_id: project.namespace,
@@ -194,13 +123,8 @@ describe Projects::ProjectMembersController do
     end
   end
 
-  describe '#request_access' do
-    let(:project) { create(:project, :public) }
-    let(:user) { create(:user) }
-
-    before do
-      sign_in(user)
-    end
+  describe 'POST request_access' do
+    before { sign_in(user) }
 
     it 'creates a new ProjectMember that is not a team member' do
       post :request_access, namespace_id: project.namespace,
@@ -215,8 +139,10 @@ describe Projects::ProjectMembersController do
     end
   end
 
-  describe '#approve' do
-    let(:project) { create(:project, :public) }
+  describe 'POST approve' do
+    let(:member) { create(:project_member, :access_request, project: project) }
+
+    before { sign_in(user) }
 
     context 'when member is not found' do
       it 'returns 404' do
@@ -229,18 +155,8 @@ describe Projects::ProjectMembersController do
     end
 
     context 'when member is found' do
-      let(:user) { create(:user) }
-      let(:team_requester) { create(:user) }
-      let(:member) do
-        project.request_access(team_requester)
-        project.requesters.find_by(user_id: team_requester.id)
-      end
-
       context 'when user does not have enough rights' do
-        before do
-          project.team << [user, :developer]
-          sign_in(user)
-        end
+        before { project.team << [user, :developer] }
 
         it 'returns 404' do
           post :approve_access_request, namespace_id: project.namespace,
@@ -248,15 +164,12 @@ describe Projects::ProjectMembersController do
                                         id: member
 
           expect(response).to have_http_status(404)
-          expect(project.users).not_to include team_requester
+          expect(project.members).not_to include member
         end
       end
 
       context 'when user has enough rights' do
-        before do
-          project.team << [user, :master]
-          sign_in(user)
-        end
+        before { project.team << [user, :master] }
 
         it 'adds user to members' do
           post :approve_access_request, namespace_id: project.namespace,
@@ -266,9 +179,53 @@ describe Projects::ProjectMembersController do
           expect(response).to redirect_to(
             namespace_project_project_members_path(project.namespace, project)
           )
-          expect(project.users).to include team_requester
+          expect(project.members).to include member
         end
       end
     end
   end
+
+  describe 'POST apply_import' do
+    let(:another_project) { create(:project, :private) }
+    let(:member) { create(:user) }
+
+    before do
+      project.team << [user, :master]
+      another_project.team << [member, :guest]
+      sign_in(user)
+    end
+
+    shared_context 'import applied' do
+      before do
+        post(:apply_import, namespace_id: project.namespace,
+                            project_id: project,
+                            source_project_id: another_project.id)
+      end
+    end
+
+    context 'when user can access source project members' do
+      before { another_project.team << [user, :guest] }
+      include_context 'import applied'
+
+      it 'imports source project members' do
+        expect(project.team_members).to include member
+        expect(response).to set_flash.to 'Successfully imported'
+        expect(response).to redirect_to(
+          namespace_project_project_members_path(project.namespace, project)
+        )
+      end
+    end
+
+    context 'when user is not member of a source project' do
+      include_context 'import applied'
+
+      it 'does not import team members' do
+        expect(project.team_members).not_to include member
+      end
+
+      it 'responds with not found' do
+        expect(response.status).to eq 404
+      end
+    end
+  end
 end

spec/factories/project_members.rb

@@ -8,5 +8,6 @@ FactoryGirl.define do
     trait(:reporter)  { access_level ProjectMember::REPORTER }
     trait(:developer) { access_level ProjectMember::DEVELOPER }
     trait(:master)    { access_level ProjectMember::MASTER }
+    trait(:access_request) { requested_at Time.now }
   end
 end