Improve code & add tests

ee/app/services/epic_issues/update_service.rb

@@ -28,7 +28,6 @@ module EpicIssues
     def issues_to_move
       @issues_to_move ||= epic.epic_issues
         .where('position >= ? AND position <= ? AND id != ?', from, to, epic_issue.id)
-        .order(:position)
     end
 
     def from
@@ -45,8 +44,11 @@ module EpicIssues
 
     def new_position
       @new_position ||= begin
-        replacing_issue = epic.epic_issues.order(:position).limit(1).offset(params[:position])[0]
-        replacing_issue.position
+        replacing_issue = epic.epic_issues.order(:position).limit(1).offset(params[:position])
+
+        return replacing_issue[0].position if replacing_issue.present?
+
+        epic.epic_issues.order(:position).last.position
       end
     end
   end

ee/app/services/issuable_links/list_service.rb

@@ -16,6 +16,11 @@ module IssuableLinks
 
     private
 
+    def relation_path(issue)
+      raise NotImplementedError
+    end
+
+
     def reference(issue)
       issue.to_reference(issuable.project)
     end

ee/lib/api/epic_issues.rb

@@ -2,12 +2,22 @@ module API
   class EpicIssues < Grape::API
     before do
       authenticate!
-      check_epics!
+      authorize_can_admin!
     end
 
     helpers do
-      def check_epics!
-        forbidden! unless ::License.feature_available?(:epics) # TODO: check for group feature instead
+      def authorize_can_admin!
+        forbidden! unless user_group.feature_available?(:epics) # TODO: check for group feature instead
+        authorize!(:admin_epic, epic)
+        forbidden! if link.epic != epic
+      end
+
+      def epic
+        @epic ||= user_group.epics.find_by(iid: params[:epic_iid])
+      end
+
+      def link
+        @link ||= EpicIssue.find(params[:epic_issue_id])
       end
     end
 
@@ -24,15 +34,10 @@ module API
         requires :position, type: Integer, desc: 'The new position of the issue in the epic (index starting with 0)'
       end
       put ':id/-/epics/:epic_iid/issues/:epic_issue_id' do
-        epic = user_group.epics.find_by(iid: params[:epic_iid])
-        authorize!(:admin_epic, epic)
-
-        link = EpicIssue.find(params[:epic_issue_id])
-        forbidden! if link.epic != epic
-
-        result = ::EpicIssues::UpdateService
-          .new(link, current_user, { position: params[:position].to_i }).execute
+        result = ::EpicIssues::UpdateService.new(link, current_user, { position: params[:position].to_i }).execute
 
+        # For now we return empty body
+        # The issues list in the correct order in body will be returned as part of #4250
         render_api_error!({ error: "Issue could not be moved!" }, 400) unless result
       end
     end

spec/ee/spec/controllers/groups/epic_issues_controller_spec.rb

@@ -13,31 +13,50 @@ describe Groups::EpicIssuesController do
     sign_in(user)
   end
 
-  describe 'GET #index' do
-    let!(:epic_issues) { create(:epic_issue, epic: epic, issue: issue) }
-
+  shared_examples 'unlicensed epics action' do
     before do
+      stub_licensed_features(epics: false)
       group.add_developer(user)
 
-      get :index, group_id: group, epic_id: epic.to_param
+      subject
     end
 
-    it 'returns status 200' do
-      expect(response.status).to eq(200)
+    it 'returns 400 status' do
+      expect(response).to have_gitlab_http_status(404)
     end
+  end
+
+  describe 'GET #index' do
+    let!(:epic_issues) { create(:epic_issue, epic: epic, issue: issue) }
+
+    subject { get :index, group_id: group, epic_id: epic.to_param }
+
+    it_behaves_like 'unlicensed epics action'
 
-    it 'returns the correct json' do
-      expected_result = [
-        {
-          'id' => issue.id,
-          'title' => issue.title,
-          'state' => issue.state,
-          'reference' => "#{project.full_path}##{issue.iid}",
-          'path' => "/#{project.full_path}/issues/#{issue.iid}",
-          'relation_path' => "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issues.id}"
-         }
-      ]
-      expect(JSON.parse(response.body)).to eq(expected_result)
+    context 'when epics feature is enabled' do
+      before do
+        group.add_developer(user)
+
+        subject
+      end
+
+      it 'returns status 200' do
+        expect(response.status).to eq(200)
+      end
+
+      it 'returns the correct json' do
+        expected_result = [
+          {
+            'id' => issue.id,
+            'title' => issue.title,
+            'state' => issue.state,
+            'reference' => "#{project.full_path}##{issue.iid}",
+            'path' => "/#{project.full_path}/issues/#{issue.iid}",
+            'relation_path' => "/groups/#{group.full_path}/-/epics/#{epic.iid}/issues/#{epic_issues.id}"
+           }
+        ]
+        expect(JSON.parse(response.body)).to eq(expected_result)
+      end
     end
   end
 
@@ -48,33 +67,37 @@ describe Groups::EpicIssuesController do
       post :create, group_id: group, epic_id: epic.to_param, issue_references: reference
     end
 
-    context 'when user has permissions to create requested association' do
-      before do
-        group.add_developer(user)
-      end
+    it_behaves_like 'unlicensed epics action'
 
-      it 'returns correct response for the correct issue reference' do
-        subject
-        list_service_response = EpicIssues::ListService.new(epic, user).execute
+    context 'when epics feature is enabled' do
+      context 'when user has permissions to create requested association' do
+        before do
+          group.add_developer(user)
+        end
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response).to eq('message' => nil, 'issues' => list_service_response.as_json)
-      end
+        it 'returns correct response for the correct issue reference' do
+          subject
+          list_service_response = EpicIssues::ListService.new(epic, user).execute
 
-      it 'creates a new EpicIssue record' do
-        expect { subject }.to change { EpicIssue.count }.from(0).to(1)
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response).to eq('message' => nil, 'issues' => list_service_response.as_json)
+        end
+
+        it 'creates a new EpicIssue record' do
+          expect { subject }.to change { EpicIssue.count }.from(0).to(1)
+        end
       end
-    end
 
-    context 'when user does not have permissions to create requested association' do
-      it 'returns correct response for the correct issue reference' do
-        subject
+      context 'when user does not have permissions to create requested association' do
+        it 'returns correct response for the correct issue reference' do
+          subject
 
-        expect(response).to have_gitlab_http_status(403)
-      end
+          expect(response).to have_gitlab_http_status(403)
+        end
 
-      it 'does not create a new EpicIssue record' do
-        expect { subject }.not_to change { EpicIssue.count }.from(0)
+        it 'does not create a new EpicIssue record' do
+          expect { subject }.not_to change { EpicIssue.count }.from(0)
+        end
       end
     end
   end
@@ -82,65 +105,67 @@ describe Groups::EpicIssuesController do
   describe 'DELETE #destroy' do
     let!(:epic_issue) { create(:epic_issue, epic: epic, issue: issue) }
 
-    subject do
-      delete :destroy, group_id: group, epic_id: epic.to_param, id: epic_issue.id
-    end
+    subject { delete :destroy, group_id: group, epic_id: epic.to_param, id: epic_issue.id }
 
-    context 'when user has permissions to delete the link' do
-      before do
-        group.add_developer(user)
-      end
+    it_behaves_like 'unlicensed epics action'
 
-      it 'returns status 200' do
-        subject
+    context 'when epics feature is enabled' do
+      context 'when user has permissions to delete the link' do
+        before do
+          group.add_developer(user)
+        end
 
-        expect(response.status).to eq(200)
-      end
+        it 'returns status 200' do
+          subject
 
-      it 'destroys the link' do
-        expect { subject }.to change { EpicIssue.count }.from(1).to(0)
+          expect(response.status).to eq(200)
+        end
+
+        it 'destroys the link' do
+          expect { subject }.to change { EpicIssue.count }.from(1).to(0)
+        end
       end
-    end
 
-    context 'when user does not have permissions to delete the link' do
-      it 'returns status 404' do
-        subject
+      context 'when user does not have permissions to delete the link' do
+        it 'returns status 404' do
+          subject
 
-        expect(response.status).to eq(403)
-      end
+          expect(response.status).to eq(403)
+        end
 
-      it 'does not destroy the link' do
-        expect { subject }.not_to change { EpicIssue.count }.from(1)
+        it 'does not destroy the link' do
+          expect { subject }.not_to change { EpicIssue.count }.from(1)
+        end
       end
-    end
 
-    context 'when the epic from the association does not equal epic from the path' do
-      subject do
-        delete :destroy, group_id: group, epic_id: another_epic.to_param, id: epic_issue.id
-      end
+      context 'when the epic from the association does not equal epic from the path' do
+        subject do
+          delete :destroy, group_id: group, epic_id: another_epic.to_param, id: epic_issue.id
+        end
 
-      let(:another_epic) { create(:epic, group: group) }
+        let(:another_epic) { create(:epic, group: group) }
 
-      before do
-        group.add_developer(user)
-      end
+        before do
+          group.add_developer(user)
+        end
 
-      it 'returns status 404' do
-        subject
+        it 'returns status 404' do
+          subject
 
-        expect(response.status).to eq(404)
-      end
+          expect(response.status).to eq(404)
+        end
 
-      it 'does not destroy the link' do
-        expect { subject }.not_to change { EpicIssue.count }.from(1)
+        it 'does not destroy the link' do
+          expect { subject }.not_to change { EpicIssue.count }.from(1)
+        end
       end
-    end
 
-    context 'when the epic_issue record does not exists' do
-      it 'returns status 404' do
-        delete :destroy, group_id: group, epic_id: epic.to_param, id: 9999
+      context 'when the epic_issue record does not exists' do
+        it 'returns status 404' do
+          delete :destroy, group_id: group, epic_id: epic.to_param, id: 9999
 
-        expect(response.status).to eq(403)
+          expect(response.status).to eq(403)
+        end
       end
     end
   end
@@ -154,61 +179,65 @@ describe Groups::EpicIssuesController do
       put :update, group_id: group, epic_id: epic.to_param, id: epic_issue1.id, epic: { position: 1 }
     end
 
-    context 'when user has permissions to admin the epic' do
-      before do
-        group.add_developer(user)
-      end
+    it_behaves_like 'unlicensed epics action'
 
-      it 'returns status 200' do
-        subject
+    context 'when epics feature is enabled' do
+      context 'when user has permissions to admin the epic' do
+        before do
+          group.add_developer(user)
+        end
 
-        expect(response.status).to eq(200)
-      end
+        it 'returns status 200' do
+          subject
 
-      it 'updates the issue position value' do
-        expect { subject }.to change { epic_issue1.reload.position }.from(1).to(2)
+          expect(response.status).to eq(200)
+        end
+
+        it 'updates the issue position value' do
+          expect { subject }.to change { epic_issue1.reload.position }.from(1).to(2)
+        end
       end
-    end
 
-    context 'when user does not have permissions to admin the epic' do
-      it 'returns status 404' do
-        subject
+      context 'when user does not have permissions to admin the epic' do
+        it 'returns status 404' do
+          subject
 
-        expect(response.status).to eq(403)
-      end
+          expect(response.status).to eq(403)
+        end
 
-      it 'does not change the position value' do
-        expect { subject }.not_to change { epic_issue1.reload.position }.from(1)
+        it 'does not change the position value' do
+          expect { subject }.not_to change { epic_issue1.reload.position }.from(1)
+        end
       end
-    end
 
-    context 'when the epic from the association does not equal epic from the path' do
-      subject do
-        put :update, group_id: group, epic_id: another_epic.to_param, id: epic_issue1.id, epic: { position: 2 }
-      end
+      context 'when the epic from the association does not equal epic from the path' do
+        subject do
+          put :update, group_id: group, epic_id: another_epic.to_param, id: epic_issue1.id, epic: { position: 2 }
+        end
 
-      let(:another_epic) { create(:epic, group: group) }
+        let(:another_epic) { create(:epic, group: group) }
 
-      before do
-        group.add_developer(user)
-      end
+        before do
+          group.add_developer(user)
+        end
 
-      it 'returns status 404' do
-        subject
+        it 'returns status 404' do
+          subject
 
-        expect(response.status).to eq(404)
-      end
+          expect(response.status).to eq(404)
+        end
 
-      it 'does not change the position value' do
-        expect { subject }.not_to change { epic_issue1.position }.from(1)
+        it 'does not change the position value' do
+          expect { subject }.not_to change { epic_issue1.position }.from(1)
+        end
       end
-    end
 
-    context 'when the epic_issue record does not exists' do
-      it 'returns status 404' do
-        delete :destroy, group_id: group, epic_id: epic.to_param, id: 9999
+      context 'when the epic_issue record does not exists' do
+        it 'returns status 404' do
+          delete :destroy, group_id: group, epic_id: epic.to_param, id: 9999
 
-        expect(response.status).to eq(403)
+          expect(response.status).to eq(403)
+        end
       end
     end
   end

spec/ee/spec/features/epics/epic_issues_spec.rb

@@ -13,8 +13,8 @@ describe 'Epic Issues', :js do
 
   let!(:epic_issues) do
     [
-      create(:epic_issue, epic: epic, issue: public_issue),
-      create(:epic_issue, epic: epic, issue: private_issue)
+      create(:epic_issue, epic: epic, issue: public_issue, position: 1),
+      create(:epic_issue, epic: epic, issue: private_issue, position: 2)
     ]
   end
 

spec/ee/spec/requests/api/epic_issues_spec.rb

@@ -3,7 +3,7 @@ require 'spec_helper'
 describe API::EpicIssues do
   let(:user) { create(:user) }
   let(:group) { create(:group) }
-  let(:project) { create(:project, group: group) }
+  let(:project) { create(:project, :public, group: group) }
   let(:epic) { create(:epic, group: group) }
   let(:issues) { create_list(:issue, 2, project: project) }
   let!(:epic_issue1) { create(:epic_issue, epic: epic, issue: issues[0], position: 1) }
@@ -11,53 +11,68 @@ describe API::EpicIssues do
   describe 'PUT /groups/:id/-/epics/:epic_iid/issues/:epic_issue_id' do
     let(:url) { "/groups/#{group.path}/-/epics/#{epic.iid}/issues/#{epic_issue1.id}?position=1" }
 
-    context 'when an error occurs' do
-      it 'returns 401 unauthorized error for non autehnticated user' do
-        put api(url)
-
-        expect(response).to have_gitlab_http_status(401)
-      end
-
-      it 'returns 404 not found error for user who does not have permissions to see the group' do
-        group.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
-        put api(url, user)
-
-        expect(response).to have_gitlab_http_status(404)
-      end
-
-      it 'returns 403 forbidden error for user who does can not move the issue' do
-        group.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
-        put api(url, user)
-
-        expect(response).to have_gitlab_http_status(404)
-      end
-
-      it 'returns 403 forbidden error for the link of another epic' do
+    context 'when epics feature is disabled' do
+      it 'returns 403 forbidden error' do
         group.add_developer(user)
-        another_epic = create(:epic, group: group)
-        url = "/groups/#{group.path}/-/epics/#{another_epic.iid}/issues/#{epic_issue1.id}?position=1"
         put api(url, user)
 
         expect(response).to have_gitlab_http_status(403)
       end
     end
 
-    context 'when the request is correct' do
-      let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issues[1], position: 2) }
-
+    context 'when epics feature is enabled' do
       before do
         stub_licensed_features(epics: true)
-        group.add_developer(user)
-        put api(url, user)
       end
 
-      it 'returns 200 status' do
-        expect(response).to have_gitlab_http_status(200)
+      context 'when an error occurs' do
+        it 'returns 401 unauthorized error for non authenticated user' do
+          put api(url)
+
+          expect(response).to have_gitlab_http_status(401)
+        end
+
+        it 'returns 404 not found error for a user without permissions to see the group' do
+          project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+          group.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+          put api(url, user)
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+
+        it 'returns 403 forbidden error for a user who can not move the issue' do
+          put api(url, user)
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+
+        it 'returns 403 forbidden error for the link of another epic' do
+          group.add_developer(user)
+          another_epic = create(:epic, group: group)
+          url = "/groups/#{group.path}/-/epics/#{another_epic.iid}/issues/#{epic_issue1.id}?position=1"
+
+          put api(url, user)
+
+          expect(response).to have_gitlab_http_status(403)
+        end
       end
 
-      it 'updates the positions values' do
-        expect(epic_issue1.reload.position).to eq(2)
-        expect(epic_issue2.reload.position).to eq(1)
+      context 'when the request is correct' do
+        let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issues[1], position: 2) }
+
+        before do
+          group.add_developer(user)
+          put api(url, user)
+        end
+
+        it 'returns 200 status' do
+          expect(response).to have_gitlab_http_status(200)
+        end
+
+        it 'updates the positions values' do
+          expect(epic_issue1.reload.position).to eq(2)
+          expect(epic_issue2.reload.position).to eq(1)
+        end
       end
     end
   end

spec/ee/spec/services/epic_issues/create_service_spec.rb

@@ -187,7 +187,7 @@ describe EpicIssues::CreateService do
               expect(subject).to eq(status: :success)
             end
 
-            it 'creates 2 system notes for each assiues' do
+            it 'creates 2 system notes for each issue' do
               expect { subject }.to change { Note.count }.from(0).to(4)
             end
           end

spec/ee/spec/services/epic_issues/update_service_spec.rb

@@ -104,5 +104,27 @@ describe EpicIssues::UpdateService do
         end
       end
     end
+
+    context 'moving issues to the last position' do
+      context 'when index of the last possition is correct' do
+        before do
+          order_issue(epic_issue1, 3)
+        end
+
+        it 'orders issues correctly' do
+          expect(ordered_epics).to eq([epic_issue2, epic_issue3, epic_issue4, epic_issue1])
+        end
+      end
+
+      context 'when index of the last possition is too high' do
+        before do
+          order_issue(epic_issue1, 100)
+        end
+
+        it 'orders issues correctly' do
+          expect(ordered_epics).to eq([epic_issue2, epic_issue3, epic_issue4, epic_issue1])
+        end
+      end
+    end
   end
 end