test that :if in module declaration controls module availability

and make rubocop happier

app/helpers/roles_helper.rb

@@ -44,7 +44,7 @@ module RolesHelper
   def group_permissions_by_module(perms)
     perms_by_module = perms.group_by { |p| p.project_module.to_s }
     ::OpenProject::AccessControl
-      .sorted_module_names(false)
+      .sorted_module_names(include_disabled: false)
       .select { |module_name| perms_by_module[module_name].present? }
       .map do |module_name|
       [module_name, perms_by_module[module_name]]

lib/open_project/access_control.rb

@@ -45,7 +45,7 @@ module OpenProject
       # Get a sorted array of module names
       #
       # @param include_disabled [boolean] Whether to return all modules or only those that are active (not disabled by config)
-      def sorted_module_names(include_disabled = true)
+      def sorted_module_names(include_disabled: true)
         modules
           .reject { |mod| !include_disabled && disabled_project_modules.include?(mod[:name]) }
           .sort_by { |a| [-a[:order], l_or_humanize(a[:name], prefix: 'project_module_')] }
@@ -99,12 +99,14 @@ module OpenProject
       end
 
       def available_project_modules
-        @available_project_modules ||= begin
-          (@permissions.reject(&:global?).map(&:project_module) + @project_modules_without_permissions)
+        @available_project_modules ||=
+          @permissions
+            .reject(&:global?)
+            .map(&:project_module)
+            .including(@project_modules_without_permissions)
             .uniq
             .compact
             .reject { |name| disabled_project_modules.include? name }
-        end
       end
 
       def disabled_project_modules
@@ -146,6 +148,7 @@ module OpenProject
 
       def clear_caches
         @available_project_modules = nil
+        @disabled_project_modules = nil
         @public_permissions = nil
         @members_only_permissions = nil
         @loggedin_only_permissions = nil

modules/bim/spec/lib/open_project/access_control_spec.rb

@@ -30,11 +30,11 @@ require 'spec_helper'
 
 describe OpenProject::AccessControl do
   before do
-    OpenProject::AccessControl.instance_variable_set(:'@disabled_project_modules', nil)
+    described_class.clear_caches
   end
 
   after do
-    OpenProject::AccessControl.instance_variable_set(:'@disabled_project_modules', nil)
+    described_class.clear_caches
   end
 
   describe '.sorted_module_names' do
@@ -54,7 +54,7 @@ describe OpenProject::AccessControl do
 
       context 'if excluding disabled modules' do
         it 'does not include the bim module' do
-          expect(subject.sorted_module_names(false))
+          expect(subject.sorted_module_names(include_disabled: false))
             .not_to include('bim')
         end
       end
@@ -76,7 +76,7 @@ describe OpenProject::AccessControl do
 
       context 'if excluding disabled modules' do
         it 'includes the bim module' do
-          expect(subject.sorted_module_names(false))
+          expect(subject.sorted_module_names(include_disabled: false))
             .to include('bim')
         end
       end

spec/lib/open_project/access_control_spec.rb

@@ -29,17 +29,6 @@
 require 'spec_helper'
 
 describe OpenProject::AccessControl do
-  def stash_access_control_permissions
-    @stashed_permissions = OpenProject::AccessControl.permissions.dup
-    OpenProject::AccessControl.clear_caches
-    OpenProject::AccessControl.instance_variable_get(:@permissions).clear
-  end
-
-  def restore_access_control_permissions
-    OpenProject::AccessControl.instance_variable_set(:@permissions, @stashed_permissions)
-    OpenProject::AccessControl.clear_caches
-  end
-
   def setup_global_permissions
     OpenProject::AccessControl.map do |map|
       map.permission :proj0, { dont: :care }, require: :member, contract_actions: { foo: :create }
@@ -66,26 +55,26 @@ describe OpenProject::AccessControl do
   end
 
   describe '.remove_modules_permissions' do
-    let!(:all_former_permissions) { OpenProject::AccessControl.permissions }
+    let!(:all_former_permissions) { described_class.permissions }
     let!(:former_repository_permissions) do
-      module_permissions = OpenProject::AccessControl.modules_permissions(['repository'])
+      module_permissions = described_class.modules_permissions(['repository'])
 
       module_permissions.select do |permission|
         permission.project_module == :repository
       end
     end
 
-    subject { OpenProject::AccessControl }
+    subject { described_class }
 
     before do
-      OpenProject::AccessControl.remove_modules_permissions(:repository)
+      described_class.remove_modules_permissions(:repository)
     end
 
     after do
-      raise 'Test outdated' unless OpenProject::AccessControl.instance_variable_defined?(:@permissions)
+      raise 'Test outdated' unless described_class.instance_variable_defined?(:@permissions)
 
-      OpenProject::AccessControl.instance_variable_set(:@permissions, all_former_permissions)
-      OpenProject::AccessControl.clear_caches
+      described_class.instance_variable_set(:@permissions, all_former_permissions)
+      described_class.clear_caches
     end
 
     it 'removes from global permissions' do
@@ -104,7 +93,7 @@ describe OpenProject::AccessControl do
       expect(subject.loggedin_only_permissions).not_to include(former_repository_permissions)
     end
 
-    it 'should disable repository module' do
+    it 'disables repository module' do
       expect(subject.available_project_modules).not_to include(:repository)
     end
   end
@@ -112,7 +101,7 @@ describe OpenProject::AccessControl do
   describe '#permissions' do
     it 'is an array of permissions' do
       expect(described_class.permissions)
-        .to(be_all{ |p| p.is_a?(OpenProject::AccessControl::Permission) })
+        .to all(be_instance_of(OpenProject::AccessControl::Permission))
     end
   end
 
@@ -121,18 +110,18 @@ describe OpenProject::AccessControl do
       subject { described_class.permission(:view_work_packages) }
 
       it 'is a permission' do
-        is_expected
+        expect(subject)
           .to be_a(OpenProject::AccessControl::Permission)
       end
 
       it 'is the permission with the queried for name' do
         expect(subject.name)
-          .to eql(:view_work_packages)
+          .to eq(:view_work_packages)
       end
 
       it 'belongs to a project module' do
         expect(subject.project_module)
-          .to eql(:work_package_tracking)
+          .to eq(:work_package_tracking)
       end
     end
 
@@ -140,13 +129,13 @@ describe OpenProject::AccessControl do
       subject { described_class.permission(:edit_project) }
 
       it 'is a permission' do
-        is_expected
+        expect(subject)
           .to be_a(OpenProject::AccessControl::Permission)
       end
 
       it 'is the permission with the queried for name' do
         expect(subject.name)
-          .to eql(:edit_project)
+          .to eq(:edit_project)
       end
 
       it 'belongs to a project module' do
@@ -165,7 +154,7 @@ describe OpenProject::AccessControl do
     context 'for a permission with a prerequisite' do
       subject { described_class.permission(:edit_work_packages) }
 
-      it 'denotes the prerequiresites' do
+      it 'denotes the prerequisites' do
         expect(subject.dependencies)
           .to match_array([:view_work_packages])
       end
@@ -174,7 +163,7 @@ describe OpenProject::AccessControl do
     context 'for a permission without a prerequisite' do
       subject { described_class.permission(:view_work_packages) }
 
-      it 'denotes the prerequiresites' do
+      it 'denotes the prerequisites' do
         expect(subject.dependencies)
           .to be_empty
       end
@@ -182,65 +171,76 @@ describe OpenProject::AccessControl do
   end
 
   describe '.modules' do
-    before do
-      stash_access_control_permissions
+    include_context 'with blank access control state'
 
+    before do
       setup_global_permissions
     end
 
-    after do
-      restore_access_control_permissions
-    end
-
     it 'can store dependencies' do
-      expect(OpenProject::AccessControl.modules.detect { |m| m[:name] == :dependent_module }[:dependencies])
+      expect(described_class.modules.detect { |m| m[:name] == :dependent_module }[:dependencies])
         .to match_array(%i[project_module])
     end
   end
 
   describe '#global_permissions' do
-    before do
-      stash_access_control_permissions
+    include_context 'with blank access control state'
 
+    before do
       setup_global_permissions
     end
 
-    after do
-      restore_access_control_permissions
-    end
-
-    it { expect(OpenProject::AccessControl.global_permissions.size).to eq(3) }
-    it { expect(OpenProject::AccessControl.global_permissions.collect(&:name)).to include(:global0) }
-    it { expect(OpenProject::AccessControl.global_permissions.collect(&:name)).to include(:global1) }
-    it { expect(OpenProject::AccessControl.global_permissions.collect(&:name)).to include(:global2) }
+    it { expect(described_class.global_permissions.size).to eq(3) }
+    it { expect(described_class.global_permissions.collect(&:name)).to include(:global0) }
+    it { expect(described_class.global_permissions.collect(&:name)).to include(:global1) }
+    it { expect(described_class.global_permissions.collect(&:name)).to include(:global2) }
   end
 
   describe '.available_project_modules' do
-    before do
-      stash_access_control_permissions
+    include_context 'with blank access control state'
 
+    before do
       setup_global_permissions
     end
 
-    after do
-      restore_access_control_permissions
-    end
+    it { expect(described_class.available_project_modules).not_to include(:global_module) }
+    it { expect(described_class.available_project_modules).to include(:mixed_module) }
 
-    it { expect(OpenProject::AccessControl.available_project_modules).not_to include(:global_module) }
-    it { expect(OpenProject::AccessControl.available_project_modules).to include(:mixed_module) }
+    context 'when a module :if is returning false' do
+      before do
+        described_class.map do |map|
+          map.project_module :dynamic_module, if: ->(*) { module_available } do |mod|
+            mod.permission :perm_d1, { dont: :care }
+          end
+        end
+      end
+
+      context 'with if: true' do
+        let(:module_available) { true }
+
+        it 'is considered available' do
+          described_class.available_project_modules
+          expect(described_class.available_project_modules).to include(:dynamic_module)
+        end
+      end
+
+      context 'with if: false' do
+        let(:module_available) { false }
+
+        it 'is not considered available anymore' do
+          expect(described_class.available_project_modules).not_to include(:dynamic_module)
+        end
+      end
+    end
   end
 
   describe '#contract_actions_map' do
-    before do
-      stash_access_control_permissions
+    include_context 'with blank access control state'
 
+    before do
       setup_global_permissions
     end
 
-    after do
-      restore_access_control_permissions
-    end
-
     it 'contains all contract actions grouped by the permission' do
       expect(subject.contract_actions_map)
         .to eql(global2: { actions: { baz: [:destroy] }, global: true, module_name: :mixed_module, grant_to_admin: true },
@@ -250,16 +250,12 @@ describe OpenProject::AccessControl do
   end
 
   describe '.grant_to_admin?' do
-    before do
-      stash_access_control_permissions
+    include_context 'with blank access control state'
 
+    before do
       setup_global_permissions
     end
 
-    after do
-      restore_access_control_permissions
-    end
-
     context 'for a granted permission (default)' do
       it 'is granted' do
         expect(described_class)

spec/support/shared/with_blank_access_control_state.rb

@@ -0,0 +1,54 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2022 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+# Makes OpenProject::AccessControl clean before each example, like if no
+# permission initialization code was run at all, and restore it after each
+# example.
+RSpec.shared_context 'with blank access control state' do
+  around do |example|
+    stash = stash_instance_variables(OpenProject::AccessControl, :@permissions, :@modules, :@project_modules_without_permissions)
+    OpenProject::AccessControl.clear_caches
+    example.run
+  ensure
+    unstash_instance_variables(OpenProject::AccessControl, stash)
+    OpenProject::AccessControl.clear_caches
+  end
+
+  def stash_instance_variables(instance, *instance_variables)
+    instance_variables.each.with_object({}) do |instance_variable, stash|
+      stash[instance_variable] = instance.instance_variable_get(instance_variable)
+      instance.remove_instance_variable(instance_variable) if stash[instance_variable]
+    end
+  end
+
+  def unstash_instance_variables(instance, stash)
+    stash.each do |instance_variable, value|
+      instance.instance_variable_set(instance_variable, value)
+    end
+  end
+end