diff --git a/CHANGELOG.md b/CHANGELOG.md index b5637083df..6f42f258d3 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/app/controllers/paginable/plans_controller.rb b/app/controllers/paginable/plans_controller.rb index acdf1ae1af..78bcda44ab 100644 --- a/app/controllers/paginable/plans_controller.rb +++ b/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', diff --git a/app/models/org.rb b/app/models/org.rb index 6a6eac55d2..8c64b97cfa 100644 --- a/app/models/org.rb +++ b/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 diff --git a/spec/factories/plans.rb b/spec/factories/plans.rb index c07d002ec7..bc44c44794 100644 --- a/spec/factories/plans.rb +++ b/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 diff --git a/spec/models/org_spec.rb b/spec/models/org_spec.rb index 7299c18e2a..5e81eb3dc2 100644 --- a/spec/models/org_spec.rb +++ b/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