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

    Organisation Plans and the related CSV Download".

    Changes:
    - where(Role.creator_condition) condition added to Plans retrieved in
      the Org model's org_admin_plans method. This ensures that Plans
    returned are only active plans. Note: if an owner or co-owner
    de-activates a plan the Role of the user is set to active: false, so any
    plan with creator with role (having active: false) would mean the Plan
    is removed.
    - To avoid duplication we removed .where(Role.creator_condition) in
      org_admin method in app/controllers/paginable/plans_controller.rb from
      line
       plans = plans.joins(:template, roles: [user::org]).where(Role.creator_condition).
    - Updated RSpec tests for Org method org_admin_plans() in spec/models/org_spec.rb.
    - Update CHANGELOG.

Author: John Pinto

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Changelog
 
+- 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)
 - Adjustments to style of select tags and plan download layout [#3509](https://github.com/DMPRoadmap/roadmap/pull/3509)

app/controllers/paginable/plans_controller.rb

@@ -46,7 +46,7 @@ def org_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 = plans.joins(:template, roles: [user: :org])
 
       paginable_renderise(
         partial: 'org_admin',

app/models/org.rb

@@ -292,11 +292,13 @@ def org_admin_plans
     if Rails.configuration.x.plans.org_admins_read_all
       Plan.includes(:template, :phases, :roles, :users).where(id: combined_plan_ids)
           .where(roles: { active: true })
+          .where(Role.creator_condition)
     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 })
+          .where(Role.creator_condition)
     end
   end
   # rubocop:enable Metrics/AbcSize

spec/factories/plans.rb

@@ -66,11 +66,26 @@
         obj.roles << create(:role, :creator, user: create(:user, org: create(:org)))
       end
     end
+    trait :administrator do
+      after(:create) do |obj|
+        obj.roles << create(:role, :administrator, user: create(:user, org: create(:org)))
+      end
+    end
+    trait :editor do
+      after(:create) do |obj|
+        obj.roles << create(:role, :editor, 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 :reviewer do
+      after(:create) do |obj|
+        obj.roles << create(:role, :reviewer, user: create(:user, org: create(:org)))
+      end
+    end
     trait :organisationally_visible do
       after(:create) do |plan|
         plan.update(visibility: Plan.visibilities[:organisationally_visible])

spec/models/org_spec.rb

@@ -344,79 +344,124 @@
 
   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) }
+    # Two Orgs
+    let!(:org1) { create(:org) }
+    let!(:org2) { create(:org) }
+
+    # Plans for org1
+    let!(:org1plan1) { create(:plan, :creator, :organisationally_visible, org: org1) }
+    let!(:org1plan2) { create(:plan, :creator, :privately_visible, org: org1) }
+    let!(:org1plan3) { create(:plan, :creator, :publicly_visible, org: org1) }
+    let!(:org1plan4) { create(:plan, :creator, :organisationally_visible, org: org1) }
 
-    subject { org.org_admin_plans }
+    # Plans for org2
+    let!(:org2plan1) { create(:plan, :creator, :organisationally_visible, org: org2) }
+
+    subject { org1.org_admin_plans }
 
     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)
+        Rails.configuration.x.plans.org_admins_read_all = true
       end
+      it { is_expected.to include(org1plan1, org1plan2, org1plan3, org1plan4) }
+      it { is_expected.not_to include(org2plan1) }
+    end
 
-      it { is_expected.to include(plan) }
+    context 'when user belongs to Org and a plan removed by creator assuming there are no coowners' do
+      before do
+        Rails.configuration.x.plans.org_admins_read_all = true
+        org1plan4.roles.map { |r| r.update(active: false) if r.user_id == org1plan4.owner.id }
+      end
+
+      it { is_expected.to include(org1plan1, org1plan2, org1plan3) }
+      it { is_expected.not_to include(org1plan4) }
     end
 
-    context 'when user belongs to Org and plan user with role :administrator' do
+    context 'when user belongs to Org and a plan removed by creator, but cowner still active.' do
       before do
-        plan.add_user!(user.id, :administrator)
+        Rails.configuration.x.plans.org_admins_read_all = true
+        coowner = create(:user, org: org1)
+        org1plan4.add_user!(coowner.id, :coowner)
+        owner_id = org1plan4.owner.id
+        org1plan4.roles.map { |r| r.update(active: false) if r.user_id == owner_id }
+      end
+
+      it { is_expected.to include(org1plan1, org1plan2, org1plan3) }
+      it { is_expected.not_to include(org1plan4) }
+    end
+
+    context 'when user belongs to Org, plan user with role :administrator, but plan creator from a different Org' do
+      before do
+        Rails.configuration.x.plans.org_admins_read_all = true
+        # Creator belongs to different org
+        @plan = create(:plan, :creator, :organisationally_visible, org: create(:org))
+        @plan.add_user!(create(:user, org: org1).id, :administrator)
       end
 
       it {
-        is_expected.to include(plan)
+        is_expected.to include(org1plan1, org1plan2, org1plan3, org1plan4, @plan)
       }
     end
 
     context 'user belongs to Org and plan user with role :editor, but not :creator and :admin' do
       before do
-        plan.add_user!(user.id, :editor)
+        Rails.configuration.x.plans.org_admins_read_all = true
+        # Creator and admin belongs to different orgs
+        @plan = create(:plan, :creator, :organisationally_visible, org: create(:org))
+        @plan.add_user!(create(:org).id, :administrator)
+        # Editor belongs to org1
+        @plan.add_user!(create(:user, org: org1).id, :editor)
       end
 
-      it { is_expected.to include(plan) }
+      it { is_expected.not_to include(@plan) }
     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)
+        Rails.configuration.x.plans.org_admins_read_all = true
+        # Creator and admin belongs to different orgs
+        @plan = create(:plan, :creator, :organisationally_visible, org: create(:org))
+        @plan.add_user!(create(:org).id, :administrator)
+        # Commenter belongs to org1
+        @plan.add_user!(create(:user, org: org1).id, :commentor)
       end
 
-      it { is_expected.to include(plan) }
+      it { is_expected.not_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)
+        Rails.configuration.x.plans.org_admins_read_all = true
+        # Creator and admin belongs to different orgs
+        @plan = create(:plan, :creator, :organisationally_visible, org: create(:org))
+        @plan.add_user!(create(:org).id, :administrator)
+        # Reviewer belongs to org1
+        @plan.add_user!(create(:user, org: org1).id, :reviewer)
       end
 
-      it { is_expected.to include(plan) }
+      it { is_expected.not_to include(@plan) }
     end
 
     context 'read_all is false, visibility private and user org_admin' do
       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!
+        @user = create(:user, :org_admin, org: org1)
+        @plan = create(:plan, :creator, :privately_visible, org: org1)
+        @plan.add_user!(@user.id, :reviewer)
       end
 
-      it { is_expected.not_to include(plan) }
+      it { is_expected.not_to include(@plan) }
     end
 
     context 'read_all is false, visibility public and user org_admin' do
       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!
+        @user = create(:user, :org_admin, org: org1)
+        @plan = create(:plan, :creator, :publicly_visible, org: org1)
+        @plan.add_user!(@user.id, :reviewer)
       end
 
-      it { is_expected.to include(plan) }
+      it { is_expected.to include(@plan) }
     end
   end