Merge branch 'feature/service-desk-project-admin' into 'master'

only require project admin for service desk

Closes #2137

See merge request !1662

app/assets/javascripts/projects/settings_service_desk/components/service_desk_setting.js

@@ -18,11 +18,6 @@ export default {
       required: false,
       default: null,
     },
-    isInstanceAdmin: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
   },
 
   methods: {
@@ -40,7 +35,6 @@ export default {
             ref="enabled-checkbox"
             type="checkbox"
             id="service-desk-enabled-checkbox"
-            :disabled="!isInstanceAdmin"
             :checked="isEnabled"
             @change="onCheckboxToggle($event)">
           <span class="descr">
@@ -48,12 +42,6 @@ export default {
           </span>
         </label>
       </div>
-      <p
-        ref="only-instance-admin-activate-message"
-        v-if="!isInstanceAdmin"
-        class="settings-message">
-        Only instance admins can activate/deactivate Service Desk
-      </p>
       <template v-if="isEnabled">
         <div
           class="panel-slim panel-default">

app/assets/javascripts/projects/settings_service_desk/service_desk_root.js

@@ -12,13 +12,10 @@ class ServiceDeskRoot {
       this.wrapperElement.dataset.enabled !== 'false';
     const incomingEmail = this.wrapperElement.dataset.incomingEmail;
     const endpoint = this.wrapperElement.dataset.endpoint;
-    const isInstanceAdmin = typeof this.wrapperElement.dataset.isInstanceAdmin !== 'undefined' &&
-      this.wrapperElement.dataset.isInstanceAdmin !== 'false';
 
     this.store = new ServiceDeskStore({
       isEnabled,
       incomingEmail,
-      isInstanceAdmin,
     });
     this.service = new ServiceDeskService(endpoint);
   }
@@ -51,8 +48,7 @@ class ServiceDeskRoot {
         <service-desk-setting
           :isEnabled="isEnabled"
           :incomingEmail="incomingEmail"
-          :fetchError="fetchError"
-          :isInstanceAdmin="isInstanceAdmin" />
+          :fetchError="fetchError" />
       `,
       components: {
         'service-desk-setting': ServiceDeskSetting,

app/assets/javascripts/projects/settings_service_desk/stores/service_desk_store.js

@@ -4,7 +4,6 @@ class ServiceDeskStore {
       isEnabled: false,
       incomingEmail: '',
       fetchError: null,
-      isInstanceAdmin: false,
     }, initialState);
   }
 
@@ -19,10 +18,6 @@ class ServiceDeskStore {
   setFetchError(value) {
     this.state.fetchError = value;
   }
-
-  setIsInstanceAdmin(value) {
-    this.state.isInstanceAdmin = value;
-  }
 }
 
 export default ServiceDeskStore;

app/controllers/projects/service_desk_controller.rb

 class Projects::ServiceDeskController < Projects::ApplicationController
-  before_action :authorize_admin_instance!, only: :update
-  before_action :authorize_admin_project!, only: :show
+  before_action :authorize_admin_project!
 
   def show
     json_response
@@ -22,8 +21,4 @@ class Projects::ServiceDeskController < Projects::ApplicationController
       format.json { render json: service_desk_attributes }
     end
   end
-
-  def authorize_admin_instance!
-    return render_404 unless current_user.admin?
-  end
 end

app/views/projects/edit.html.haml

@@ -134,8 +134,7 @@
               = link_to icon('question-circle'), help_page_path('user/project/service_desk')
             .js-service-desk-setting-root{ data: { endpoint: namespace_project_service_desk_path(@project.namespace, @project),
               enabled: @project.service_desk_enabled,
-              incoming_email: (@project.service_desk_address if @project.service_desk_enabled),
-              is_instance_admin: current_user.admin? } }
+              incoming_email: (@project.service_desk_address if @project.service_desk_enabled) } }
 
         %hr
         %fieldset.features.append-bottom-default

spec/controllers/projects/service_desk_controller_spec.rb

@@ -51,10 +51,11 @@ describe Projects::ServiceDeskController do
       expect(response.status).to eq(200)
     end
 
-    context 'when user is not admin' do
-      before { user.update(admin: false) }
+    context 'when user cannot admin the project' do
+      let(:other_user) { create(:user) }
 
       it 'renders 404' do
+        sign_in(other_user)
         put :update, namespace_id: project.namespace.to_param, project_id: project, service_desk_enabled: true, format: :json
 
         expect(response.status).to eq(404)

spec/features/projects/settings/ee/service_desk_setting_spec.rb

@@ -3,63 +3,25 @@ require 'spec_helper'
 describe 'Service Desk Setting', js: true, feature: true do
   include WaitForAjax
 
-  describe 'as project master/admin' do
-    let(:project) { create(:project_empty_repo, :private) }
-    let(:user) { create(:user) }
+  let(:project) { create(:project_empty_repo, :private) }
+  let(:user) { create(:user) }
 
-    before do
-      project.add_master(user)
-      login_as(user)
-      allow_any_instance_of(License).to receive(:add_on?).and_call_original
-      allow_any_instance_of(License).to receive(:add_on?).with('GitLab_ServiceDesk') { true }
-    end
+  before do
+    project.add_master(user)
+    login_as(user)
+    allow_any_instance_of(License).to receive(:add_on?).and_call_original
+    allow_any_instance_of(License).to receive(:add_on?).with('GitLab_ServiceDesk') { true }
 
-    describe 'when disabled' do
-      before do
-        visit edit_namespace_project_path(project.namespace, project)
-      end
-
-      it 'shows disabled activation checkbox' do
-        expect(page).to have_selector("#service-desk-enabled-checkbox[disabled]")
-      end
-    end
-
-    describe 'when enabled' do
-      before do
-        project.update(service_desk_enabled: true)
-        visit edit_namespace_project_path(project.namespace, project)
-      end
-
-      it 'shows disabled activation checkbox' do
-        expect(page).to have_selector("#service-desk-enabled-checkbox[disabled]")
-      end
-
-      it 'shows service_desk_address when enabled' do
-        expect(find('.js-service-desk-setting-wrapper .panel-body')).to have_content(project.service_desk_address)
-      end
-    end
+    visit edit_namespace_project_path(project.namespace, project)
   end
 
-  describe 'as instance admin' do
-    let(:project) { create(:project_empty_repo, :private) }
-    let(:user) { create(:user, :admin) }
-
-    before do
-      login_as(user)
-      allow_any_instance_of(License).to receive(:add_on?).and_call_original
-      allow_any_instance_of(License).to receive(:add_on?).with('GitLab_ServiceDesk') { true }
-
-      visit edit_namespace_project_path(project.namespace, project)
-    end
-
-    it 'shows activation checkbox' do
-      expect(page).to have_selector("#service-desk-enabled-checkbox")
-    end
+  it 'shows activation checkbox' do
+    expect(page).to have_selector("#service-desk-enabled-checkbox")
+  end
 
-    it 'shows incoming email after activating' do
-      find("#service-desk-enabled-checkbox").click
-      wait_for_ajax
-      expect(find('.js-service-desk-setting-wrapper .panel-body')).to have_content(project.service_desk_address)
-    end
+  it 'shows incoming email after activating' do
+    find("#service-desk-enabled-checkbox").click
+    wait_for_ajax
+    expect(find('.js-service-desk-setting-wrapper .panel-body')).to have_content(project.service_desk_address)
   end
 end

spec/javascripts/projects/settings_service_desk/components/service_desk_setting_spec.js

@@ -31,12 +31,8 @@ describe('ServiceDeskSetting', () => {
           el = vm.$el;
         });
 
-        it('should see disabled activation checkbox', () => {
-          expect(vm.$refs['enabled-checkbox'].getAttribute('disabled')).toEqual('disabled');
-        });
-
-        it('should see only instance admin can activate/deactivate message', () => {
-          expect(vm.$refs['only-instance-admin-activate-message']).toBeDefined();
+        it('should see activation checkbox (not disabled)', () => {
+          expect(vm.$refs['enabled-checkbox'].getAttribute('disabled')).toEqual(null);
         });
 
         it('should see main panel with the email info', () => {
@@ -53,20 +49,6 @@ describe('ServiceDeskSetting', () => {
           expect(vm.$refs['recommend-protect-email-from-spam-message']).toBeDefined();
         });
       });
-
-      describe('as instance admin', () => {
-        beforeEach(() => {
-          vm = createComponent({
-            isEnabled: true,
-            isInstanceAdmin: true,
-          });
-          el = vm.$el;
-        });
-
-        it('should see activation checkbox (not disabled)', () => {
-          expect(vm.$refs['enabled-checkbox'].getAttribute('disabled')).toEqual(null);
-        });
-      });
     });
 
     describe('with incomingEmail', () => {

spec/javascripts/projects/settings_service_desk/stores/service_desk_store_spec.js

@@ -50,22 +50,4 @@ describe('ServiceDeskStore', () => {
       expect(store.state.fetchError).toEqual(err);
     });
   });
-
-  describe('setIsInstanceAdmin', () => {
-    it('defaults to false', () => {
-      expect(store.state.isInstanceAdmin).toEqual(false);
-    });
-
-    it('set true', () => {
-      store.setIsInstanceAdmin(true);
-
-      expect(store.state.isInstanceAdmin).toEqual(true);
-    });
-
-    it('set false', () => {
-      store.setIsInstanceAdmin(false);
-
-      expect(store.state.isInstanceAdmin).toEqual(false);
-    });
-  });
 });