Issue #3561 - Fix for bug "Discrepancy in Org Admin view of the

CHANGELOG.md

@@ -15,6 +15,7 @@
 - Bump rexml from 3.4.1 to 3.4.2 [#3562](https://github.com/DMPRoadmap/roadmap/pull/3562)
 - Bump tmp from 0.2.3 to 0.2.4 [#3548](https://github.com/DMPRoadmap/roadmap/pull/3548)
 - Update Gems and Address New Rubocop Offences [#3572](https://github.com/DMPRoadmap/roadmap/pull/3572)
+- Fix for a discrepancy in Org Admin view of the Organisation Plans and the related CSV Download. [#3561] (https://github.com/DMPRoadmap/roadmap/issues/3561)
 
 ## v5.0.1
 - Updated seeds.rb file for identifier_schemes to include context value and removed logo_url and idenitifier_prefix for Shibboleth (as it was causing issues with SSO). [#3525](https://github.com/DMPRoadmap/roadmap/pull/3525)

app/controllers/paginable/plans_controller.rb

@@ -45,8 +45,9 @@ def org_admin
       # check if current user if super_admin
       @super_admin = current_user.can_super_admin?
       @clicked_through = params[:click_through].present?
-      plans = @super_admin ? Plan.all : current_user.org.org_admin_plans
-      plans = plans.joins(:template, roles: [user: :org]).where(Role.creator_condition)
+
+      plans = @super_admin ? Plan.where(Role.creator_condition) : current_user.org.org_admin_plans
+      plans = plans.joins(:template, roles: [user: :org])
 
       paginable_renderise(
         partial: 'org_admin',

app/models/org.rb

@@ -288,17 +288,17 @@ def org_admins
   # This replaces the old plans method. We now use the native plans method and this.
   # rubocop:disable Metrics/AbcSize
   def org_admin_plans
-    combined_plan_ids = (native_plan_ids + affiliated_plan_ids).flatten.uniq
-
-    if Rails.configuration.x.plans.org_admins_read_all
-      Plan.includes(:template, :phases, :roles, :users).where(id: combined_plan_ids)
-          .where(roles: { active: true })
-    else
-      Plan.includes(:template, :phases, :roles, :users).where(id: combined_plan_ids)
-          .where.not(visibility: Plan.visibilities[:privately_visible])
-          .where.not(visibility: Plan.visibilities[:is_test])
-          .where(roles: { active: true })
+    scope = Plan.includes(:template, :phases, :roles, :users)
+                .where(id: (native_plan_ids + affiliated_plan_ids).uniq)
+                .where(roles: { active: true })
+                .where(Role.creator_condition)
+
+    unless Rails.configuration.x.plans.org_admins_read_all
+      scope = scope.where.not(visibility: Plan.visibilities[:privately_visible])
+                   .where.not(visibility: Plan.visibilities[:is_test])
     end
+
+    scope
   end
   # rubocop:enable Metrics/AbcSize
 

spec/factories/plans.rb

@@ -61,37 +61,20 @@
       answers { 0 }
       guidance_groups { 0 }
     end
-    trait :creator do
-      after(:create) do |obj|
-        obj.roles << create(:role, :creator, user: create(:user, org: create(:org)))
-      end
-    end
-    trait :commenter do
-      after(:create) do |obj|
-        obj.roles << create(:role, :commenter, user: create(:user, org: create(:org)))
-      end
-    end
-    trait :organisationally_visible do
-      after(:create) do |plan|
-        plan.update(visibility: Plan.visibilities[:organisationally_visible])
-      end
-    end
-
-    trait :publicly_visible do
-      after(:create) do |plan|
-        plan.update(visibility: Plan.visibilities[:publicly_visible])
-      end
-    end
 
-    trait :is_test do
-      after(:create) do |plan|
-        plan.update(visibility: Plan.visibilities[:is_test])
+    %i[creator administrator editor commenter reviewer].each do |role|
+      trait role do
+        after(:create) do |obj|
+          obj.roles << create(:role, role, user: create(:user, org: create(:org)))
+        end
       end
     end
 
-    trait :privately_visible do
-      after(:create) do |plan|
-        plan.update(visibility: Plan.visibilities[:privately_visible])
+    %i[organisationally_visible publicly_visible is_test privately_visible].each do |visibility|
+      trait visibility do
+        after(:create) do |obj|
+          obj.update(visibility: Plan.visibilities[visibility])
+        end
       end
     end
 

spec/models/org_spec.rb

@@ -343,80 +343,103 @@
   end
 
   describe '#org_admin_plans' do
-    Rails.configuration.x.plans.org_admins_read_all = true
-    let!(:org) { create(:org) }
-    let!(:plan) { create(:plan, org: org, visibility: 'publicly_visible') }
-    let!(:user) { create(:user, org: org) }
-
-    subject { org.org_admin_plans }
+    # Helper for returning all plans from the `plans` hash
+    def all_plans
+      plans.values.flat_map(&:values)
+    end
 
-    context 'when user belongs to Org and plan owner with role :creator' do
-      before do
-        create(:role, :creator, user: user, plan: plan)
-        plan.add_user!(user.id, :creator)
-      end
+    # Helper for deactivating roles (creator or administrator) from plans
+    def deactivate_roles_for_plans(plans, role_condition)
+      Role.where(plan_id: plans.map(&:id)).where(role_condition).update_all(active: false)
+    end
 
-      it { is_expected.to include(plan) }
+    def create_plans_for(org)
+      {
+        public: create(:plan, :creator, :publicly_visible, org: org),
+        org: create(:plan, :creator, :organisationally_visible, org: org),
+        private: create(:plan, :creator, :privately_visible, org: org),
+        test: create(:plan, :creator, :is_test, org: org)
+      }
     end
 
-    context 'when user belongs to Org and plan user with role :administrator' do
-      before do
-        plan.add_user!(user.id, :administrator)
+    let!(:org) { create(:org) }
+    let!(:org_user) { create(:user, org: org) }
+    let!(:other_org) { create(:org) }
+
+    # org_admin_plans consists of "native" and "affiliated" plans
+    # - native plans have plan.org == org
+    # - affiliated plans have an active administrator role for a user where user.org == org
+    let!(:plans) do
+      {
+        native: create_plans_for(org),
+        affiliated: create_plans_for(other_org)
+      }.tap do |hash|
+        # Add the required administrator role for the `affiliated` plans
+        hash[:affiliated].each_value do |plan|
+          plan.add_user!(org_user.id, :administrator)
+        end
       end
-
-      it {
-        is_expected.to include(plan)
-      }
     end
 
-    context 'user belongs to Org and plan user with role :editor, but not :creator and :admin' do
+    let!(:other_org_plan) { create(:plan, :creator, :publicly_visible, org: other_org) }
+
+    subject { org.org_admin_plans }
+
+    shared_examples 'org_admin_plans expectations' do |org_admins_read_all: true|
       before do
-        plan.add_user!(user.id, :editor)
+        Rails.configuration.x.plans.org_admins_read_all = org_admins_read_all
       end
 
-      it { is_expected.to include(plan) }
+      it 'includes/excludes the expected plans' do
+        expect(subject).to include(*Array(included))
+        expect(subject).not_to include(*Array(excluded))
+      end
     end
 
-    context 'user belongs to Org and plan user with role :commenter, but not :creator and :admin' do
-      before do
-        plan.add_user!(user.id, :commenter)
+    context 'default context with org_admins_read_all = true' do
+      include_examples 'org_admin_plans expectations' do
+        let(:included) { all_plans }
+        let(:excluded) { other_org_plan }
       end
-
-      it { is_expected.to include(plan) }
     end
 
-    context 'user belongs to Org and plan user with role :reviewer, but not :creator and :admin' do
-      before do
-        plan.add_user!(user.id, :reviewer)
+    context 'default context with org_admins_read_all = false' do
+      let(:private_and_test_plans) do
+        plans.fetch_values(:native, :affiliated).flat_map do |h|
+          h.values_at(:private, :test)
+        end
       end
 
-      it { is_expected.to include(plan) }
+      include_examples 'org_admin_plans expectations', org_admins_read_all: false do
+        let(:included) { (all_plans - private_and_test_plans).flatten }
+        let(:excluded) { private_and_test_plans + [other_org_plan] }
+      end
     end
 
-    context 'read_all is false, visibility private and user org_admin' do
+    context 'creator role is deactivated for some native and affiliated plans' do
+      # Deactivate the creator role for both a native and an affiliated plan
+      let(:plans_to_deactivate) { [plans[:native][:public], plans[:affiliated][:public]] }
       before do
-        Rails.configuration.x.plans.org_admins_read_all = false
-        @perm = build(:perm)
-        @perm.name = 'grant_permissions'
-        user.perms << @perm
-        plan.add_user!(user.id, :reviewer)
-        plan.privately_visible!
+        deactivate_roles_for_plans(plans_to_deactivate, Role.creator_condition)
       end
 
-      it { is_expected.not_to include(plan) }
+      include_examples 'org_admin_plans expectations' do
+        let(:included) { (all_plans - plans_to_deactivate).flatten }
+        let(:excluded) { plans_to_deactivate + [other_org_plan] }
+      end
     end
 
-    context 'read_all is false, visibility public and user org_admin' do
+    context 'administrator role is deactivated for some affiliated plans' do
+      # Deactivate the administrator role for some affiliated plans
+      let(:plans_to_deactivate) { [plans[:affiliated][:public], plans[:affiliated][:org]] }
       before do
-        Rails.configuration.x.plans.org_admins_read_all = false
-        @perm = build(:perm)
-        @perm.name = 'grant_permissions'
-        user.perms << @perm
-        plan.add_user!(user.id, :reviewer)
-        plan.publicly_visible!
+        deactivate_roles_for_plans(plans_to_deactivate, Role.administrator_condition)
       end
 
-      it { is_expected.to include(plan) }
+      include_examples 'org_admin_plans expectations' do
+        let(:included) { (all_plans - plans_to_deactivate).flatten }
+        let(:excluded) { plans_to_deactivate + [other_org_plan] }
+      end
     end
   end