From 79d4571eed384cc9d55e4469d43abde713643385 Mon Sep 17 00:00:00 2001 From: John Pinto Date: Wed, 24 Sep 2025 16:40:37 +0100 Subject: [PATCH 1/5] 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. - 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. --- CHANGELOG.md | 1 + app/controllers/paginable/plans_controller.rb | 10 +- app/models/org.rb | 2 + spec/factories/plans.rb | 15 +++ spec/models/org_spec.rb | 101 +++++++++++++----- 5 files changed, 99 insertions(+), 30 deletions(-) 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..4920830125 100644 --- a/app/controllers/paginable/plans_controller.rb +++ b/app/controllers/paginable/plans_controller.rb @@ -45,8 +45,14 @@ 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 = if @super_admin + Plan.all.joins(:template, roles: [user: :org]) + .where(Role.creator_condition) + else + current_user.org.org_admin_plans + .joins(:template, roles: [user: :org]) + end paginable_renderise( partial: 'org_admin', diff --git a/app/models/org.rb b/app/models/org.rb index 6a6eac55d2..2986fa2d83 100644 --- a/app/models/org.rb +++ b/app/models/org.rb @@ -293,11 +293,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 diff --git a/spec/factories/plans.rb b/spec/factories/plans.rb index c07d002ec7..f10fb96641 100644 --- a/spec/factories/plans.rb +++ b/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]) diff --git a/spec/models/org_spec.rb b/spec/models/org_spec.rb index 7299c18e2a..cc6de6dfef 100644 --- a/spec/models/org_spec.rb +++ b/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 From 0c98319cb9cb730e9ece33f91990ac821842a4a1 Mon Sep 17 00:00:00 2001 From: John Pinto Date: Thu, 4 Dec 2025 14:06:07 +0000 Subject: [PATCH 2/5] Issue #3561 - Refactoring code based on comments bt @aaronskiba Changes: - Refactored Org model org_admin_plans() method. - Refactored Paginable Plans controller method org_admin(). --- app/controllers/paginable/plans_controller.rb | 9 ++------ app/models/org.rb | 22 +++++++++---------- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/app/controllers/paginable/plans_controller.rb b/app/controllers/paginable/plans_controller.rb index 4920830125..78bcda44ab 100644 --- a/app/controllers/paginable/plans_controller.rb +++ b/app/controllers/paginable/plans_controller.rb @@ -46,13 +46,8 @@ def org_admin @super_admin = current_user.can_super_admin? @clicked_through = params[:click_through].present? - plans = if @super_admin - Plan.all.joins(:template, roles: [user: :org]) - .where(Role.creator_condition) - else - current_user.org.org_admin_plans - .joins(:template, roles: [user: :org]) - end + 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 2986fa2d83..8c64b97cfa 100644 --- a/app/models/org.rb +++ b/app/models/org.rb @@ -288,19 +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 }) - .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) + 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 From fc48b6c0f4f919625331625bc7d81b1d7a08bb02 Mon Sep 17 00:00:00 2001 From: John Pinto Date: Thu, 4 Dec 2025 14:37:56 +0000 Subject: [PATCH 3/5] Updated RSpec spec/models/org_spec.rb to fix a bug associated with coowner being used incorrectly as an Role access_type. A coowner is any user apart from the plan creator with the Role administrator. --- spec/models/org_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/models/org_spec.rb b/spec/models/org_spec.rb index cc6de6dfef..b8713c84c8 100644 --- a/spec/models/org_spec.rb +++ b/spec/models/org_spec.rb @@ -377,11 +377,12 @@ it { is_expected.not_to include(org1plan4) } end - context 'when user belongs to Org and a plan removed by creator, but cowner still active.' do + context 'when user belongs to Org and a plan removed by creator, but coowner still active.' do before do Rails.configuration.x.plans.org_admins_read_all = true coowner = create(:user, org: org1) - org1plan4.add_user!(coowner.id, :coowner) + # Add coowner to the plan by giving user the role administrator + org1plan4.add_user!(coowner.id, :administrator) owner_id = org1plan4.owner.id org1plan4.roles.map { |r| r.update(active: false) if r.user_id == owner_id } end From 983b86aac4386052b1ee94cefd25ab0416062f60 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Sat, 6 Dec 2025 20:01:39 -0700 Subject: [PATCH 4/5] Refactor role & visibility traits in Plan factory DRY up FactoryBot traits for roles and plan visibility by consolidating repetitive definitions into iteration loops. --- spec/factories/plans.rb | 52 ++++++++--------------------------------- 1 file changed, 10 insertions(+), 42 deletions(-) diff --git a/spec/factories/plans.rb b/spec/factories/plans.rb index f10fb96641..bc44c44794 100644 --- a/spec/factories/plans.rb +++ b/spec/factories/plans.rb @@ -61,52 +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 :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]) - 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 From 58be1ff9640c2a0c3719ff8c205248e114f6e05b Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Sun, 7 Dec 2025 13:53:11 -0700 Subject: [PATCH 5/5] Rework `org_admin_plans` spec Redesign org_admin_plans tests for clarity and maintainability: - Introduce helper methods for plan creation and role deactivation. - Organize plans into native vs affiliated hashes for clearer context. - Use shared examples to consolidate repetitive expectations. --- spec/models/org_spec.rb | 157 +++++++++++++++++----------------------- 1 file changed, 67 insertions(+), 90 deletions(-) diff --git a/spec/models/org_spec.rb b/spec/models/org_spec.rb index b8713c84c8..5e81eb3dc2 100644 --- a/spec/models/org_spec.rb +++ b/spec/models/org_spec.rb @@ -343,126 +343,103 @@ end describe '#org_admin_plans' do - Rails.configuration.x.plans.org_admins_read_all = true - # 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) } - - # 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 - 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) } + # 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 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 + # 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(org1plan1, org1plan2, org1plan3) } - it { is_expected.not_to include(org1plan4) } + 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 a plan removed by creator, but coowner still active.' do - before do - Rails.configuration.x.plans.org_admins_read_all = true - coowner = create(:user, org: org1) - # Add coowner to the plan by giving user the role administrator - org1plan4.add_user!(coowner.id, :administrator) - owner_id = org1plan4.owner.id - org1plan4.roles.map { |r| r.update(active: false) if r.user_id == owner_id } + 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(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 + let!(:other_org_plan) { create(:plan, :creator, :publicly_visible, org: other_org) } - it { - is_expected.to include(org1plan1, org1plan2, org1plan3, org1plan4, @plan) - } - end + subject { org.org_admin_plans } - context 'user belongs to Org and plan user with role :editor, but not :creator and :admin' do + shared_examples 'org_admin_plans expectations' do |org_admins_read_all: true| before do - 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) + Rails.configuration.x.plans.org_admins_read_all = org_admins_read_all end - it { is_expected.not_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 - 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) + 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.not_to include(@plan) } end - context 'user belongs to Org and plan user with role :reviewer, but not :creator and :admin' do - before do - 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) + 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.not_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 - @user = create(:user, :org_admin, org: org1) - @plan = create(:plan, :creator, :privately_visible, org: org1) - @plan.add_user!(@user.id, :reviewer) + 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 - @user = create(:user, :org_admin, org: org1) - @plan = create(:plan, :creator, :publicly_visible, org: org1) - @plan.add_user!(@user.id, :reviewer) + 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