turn the manage_types permission authoritative

This will for the time being break the other project settings permissions

app/controllers/project_settings/types_controller.rb

@@ -35,4 +35,14 @@ class ProjectSettings::TypesController < ProjectSettingsController
     @types = ::Type.all
     render template: 'project_settings/types'
   end
+
+  def update
+    if UpdateProjectsTypesService.new(@project).call(permitted_params.projects_type_ids)
+      flash[:notice] = I18n.t('notice_successful_update')
+    else
+      flash[:error] = @project.errors.full_messages
+    end
+
+    redirect_to settings_types_project_path(@project.identifier)
+  end
 end

app/controllers/project_settings_controller.rb

@@ -33,12 +33,4 @@ class ProjectSettingsController < ApplicationController
   before_action :authorize
 
   def show; end
-
-  private
-
-  def find_project
-    @project = Project.find(params[:id])
-  rescue ActiveRecord::RecordNotFound
-    render_404
-  end
 end

app/controllers/projects_controller.rb

@@ -33,7 +33,7 @@ class ProjectsController < ApplicationController
   menu_item :roadmap, only: :roadmap
 
   before_action :find_project, except: %i[index level_list new]
-  before_action :authorize, only: %i[modules types custom_fields copy]
+  before_action :authorize, only: %i[modules custom_fields copy]
   before_action :authorize_global, only: %i[new]
   before_action :require_admin, only: %i[archive unarchive destroy destroy_info]
 
@@ -101,16 +101,6 @@ class ProjectsController < ApplicationController
     end
   end
 
-  def types
-    if UpdateProjectsTypesService.new(@project).call(permitted_params.projects_type_ids)
-      flash[:notice] = I18n.t('notice_successful_update')
-    else
-      flash[:error] = @project.errors.full_messages
-    end
-
-    redirect_to settings_types_project_path(@project.identifier)
-  end
-
   def modules
     call = Projects::EnabledModulesService
            .new(model: @project, user: current_user)

app/helpers/project_settings_helper.rb

@@ -85,7 +85,7 @@ module ProjectSettingsHelper
         name: 'storage',
         action: { controller: '/project_settings/storage', action: 'show' },
         label: :label_required_disk_storage
-      },
+      }
     ]
   end
 end

app/views/project_settings/types.html.erb

@@ -33,10 +33,10 @@ See COPYRIGHT and LICENSE files for more details.
 
 <% if  Type.all.any? %>
   <%= form_for @project,
-               url: { controller: '/projects', action: 'types', id: @project },
+               url: { controller: '/project_settings/types', action: 'update', id: @project },
                method: :patch,
-               html: {id: 'types-form'} do |f| %>
-    <%= render partial: 'projects/form/types', locals: { f: f, project: @project, withControlls: true } %>
+               html: { id: 'types-form' } do |f| %>
+    <%= render partial: 'projects/form/types', locals: { f: f, project: @project } %>
 
   <% end %>
 

app/views/projects/form/_types.html.erb

@@ -108,8 +108,7 @@ See COPYRIGHT and LICENSE files for more details.
 
   </div>
 </div>
-<% if withControlls == true %>
-  <div class="generic-table--action-buttons">
-    <%= styled_button_tag t(:button_save), class: '-highlight -with-icon icon-checkmark' %>
-  </div>
-<% end %>
+
+<div class="generic-table--action-buttons">
+  <%= styled_button_tag t(:button_save), class: '-highlight -with-icon icon-checkmark' %>
+</div>

config/initializers/permissions.rb

@@ -30,18 +30,6 @@
 
 require 'open_project/access_control'
 
-def edit_project_hash
-  permissions = {
-    projects: %i[edit update custom_fields],
-    project_settings: [:show]
-  }
-
-  ProjectSettingsHelper.project_settings_tabs.each do |node|
-    permissions["project_settings/#{node[:name]}"] = [:show]
-  end
-  permissions
-end
-
 OpenProject::AccessControl.map do |map|
   map.project_module nil, order: 100 do
     map.permission :add_project,
@@ -86,7 +74,10 @@ OpenProject::AccessControl.map do |map|
                    public: true
 
     map.permission :edit_project,
-                   edit_project_hash,
+                   {
+                     projects: %i[edit update custom_fields],
+                     'project_settings/generic': [:show]
+                   },
                    require: :member,
                    contract_actions: { projects: %i[update] }
 
@@ -107,13 +98,15 @@ OpenProject::AccessControl.map do |map|
 
     map.permission :manage_versions,
                    {
-                     "project_settings/versions": [:show],
+                     'project_settings/versions': [:show],
                      versions: %i[new create edit update close_completed destroy]
                    },
                    require: :member
 
     map.permission :manage_types,
-                   { projects: :types },
+                   {
+                     'project_settings/types': %i[show update]
+                   },
                    require: :member
 
     map.permission :add_subprojects,

config/routes.rb

@@ -179,6 +179,7 @@ OpenProject::Application.routes.draw do
     member do
       ProjectSettingsHelper.project_settings_tabs.each do |tab|
         get "settings/#{tab[:name]}", controller: "project_settings/#{tab[:name]}", action: 'show', as: "settings_#{tab[:name]}"
+        patch "settings/#{tab[:name]}", controller: "project_settings/#{tab[:name]}", action: 'update', as: "update_settings_#{tab[:name]}"
       end
       get "settings"
 

spec/controllers/projects_controller_spec.rb

@@ -125,101 +125,6 @@ describe ProjectsController, type: :controller do
   end
 
   describe 'settings' do
-    render_views
-
-    describe '#type' do
-      let(:update_service) do
-        service = double('update service')
-
-        allow(UpdateProjectsTypesService).to receive(:new).with(project).and_return(service)
-
-        service
-      end
-      let(:user) { FactoryBot.create(:admin) }
-      let(:project) do
-        project = FactoryBot.build_stubbed(:project)
-
-        allow(Project).to receive(:find).and_return(project)
-
-        project
-      end
-
-      before do
-        allow(User).to receive(:current).and_return user
-      end
-
-      context 'on success' do
-        before do
-          expect(update_service).to receive(:call).with([1, 2, 3]).and_return true
-
-          patch :types, params: { id: project.id, project: { 'type_ids' => ['1', '2', '3'] } }
-        end
-
-        it 'sets a flash message' do
-          expect(flash[:notice]).to eql(I18n.t('notice_successful_update'))
-        end
-
-        it 'redirects to settings#types' do
-          expect(response).to redirect_to(controller: '/project_settings/types', id: project, action: 'show')
-        end
-      end
-
-      context 'on failure' do
-        let(:errors) { ActiveModel::Errors.new(project) }
-        let(:error_message) { 'error message' }
-
-        before do
-          expect(update_service).to receive(:call).with([1, 2, 3]).and_return false
-
-          # acts_as_url tries to access the errors object which we stub here
-          allow(project).to receive(:errors).and_return errors
-          allow(errors).to receive(:full_messages).and_return(error_message)
-
-          patch :types, params: { id: project.id, project: { 'type_ids' => ['1', '2', '3'] } }
-        end
-
-        it 'sets a flash message' do
-          expect(flash[:error]).to eql(error_message)
-        end
-
-        it 'redirects to settings#types' do
-          expect(response).to redirect_to(controller: '/project_settings/types', id: project, action: 'show')
-        end
-      end
-    end
-
-    describe '#destroy' do
-      let(:project) { FactoryBot.build_stubbed(:project) }
-      let(:request) { delete :destroy, params: { id: project.id } }
-
-      let(:service_result) { ::ServiceResult.new(success: success) }
-
-      before do
-        allow(Project).to receive(:find).and_return(project)
-        expect_any_instance_of(::Projects::ScheduleDeletionService)
-          .to receive(:call)
-                .and_return service_result
-      end
-
-      context 'when service call succeeds' do
-        let(:success) { true }
-        it 'prints success' do
-          request
-          expect(response).to be_redirect
-          expect(flash[:notice]).to be_present
-        end
-      end
-
-      context 'when service call fails' do
-        let(:success) { false }
-        it 'prints fail' do
-          request
-          expect(response).to be_redirect
-          expect(flash[:error]).to be_present
-        end
-      end
-    end
-
     describe '#custom_fields' do
       let(:project) { FactoryBot.create(:project) }
       let(:custom_field_1) { FactoryBot.create(:work_package_custom_field) }
@@ -265,6 +170,40 @@ describe ProjectsController, type: :controller do
     end
   end
 
+  describe '#destroy' do
+    render_views
+
+    let(:project) { FactoryBot.build_stubbed(:project) }
+    let(:request) { delete :destroy, params: { id: project.id } }
+
+    let(:service_result) { ::ServiceResult.new(success: success) }
+
+    before do
+      allow(Project).to receive(:find).and_return(project)
+      expect_any_instance_of(::Projects::ScheduleDeletionService)
+        .to receive(:call)
+              .and_return service_result
+    end
+
+    context 'when service call succeeds' do
+      let(:success) { true }
+      it 'prints success' do
+        request
+        expect(response).to be_redirect
+        expect(flash[:notice]).to be_present
+      end
+    end
+
+    context 'when service call fails' do
+      let(:success) { false }
+      it 'prints fail' do
+        request
+        expect(response).to be_redirect
+        expect(flash[:error]).to be_present
+      end
+    end
+  end
+
   describe 'with an existing project' do
     let(:project) { FactoryBot.create :project, identifier: 'blog' }
 

spec/features/projects/work_package_type_mgmt_spec.rb

@@ -29,7 +29,7 @@
 require 'spec_helper'
 
 describe 'Projects', 'work package type mgmt', type: :feature, js: true do
-  current_user { FactoryBot.create(:admin) }
+  current_user { FactoryBot.create(:user, member_in_project: project, member_with_permissions: %i[edit_project manage_types]) }
 
   let(:phase_type)     { FactoryBot.create(:type, name: 'Phase', is_default: true) }
   let(:milestone_type) { FactoryBot.create(:type, name: 'Milestone', is_default: false) }
@@ -41,9 +41,18 @@ describe 'Projects', 'work package type mgmt', type: :feature, js: true do
     click_on 'Project settings'
     click_on 'Work package types'
 
-    field_checked = find_field('Phase', visible: false)['checked']
-    expect(field_checked).to be_truthy
-    field_checked = find_field('Milestone', visible: false)['checked']
-    expect(field_checked).to be_truthy
+    expect(find_field('Phase', visible: false)['checked'])
+      .to be_truthy
+
+    expect(find_field('Milestone', visible: false)['checked'])
+      .to be_truthy
+
+    # Disable a type
+    find_field('Milestone', visible: false).click
+
+    click_button 'Save'
+
+    expect(find_field('Milestone', visible: false)['checked'])
+      .to be_falsey
   end
 end