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

johnpinto1 · 2025-09-25T14:48:44Z

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.

Under the current code an Org plan that is removed by owner or coowner will be visible by the Org admins for the Org in the CSV. The Org Admin, correctly, does not show plan. With this change in code the removed plan is no longer present in the CSV.
To test:
Look at a given removed plan before and after code change.

aaronskiba · 2025-11-27T16:57:56Z

app/controllers/paginable/plans_controller.rb

      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])


Given these changes, it makes sense to remove the redundant .where(Role.creator_condition) when @super_admin evaluates to false.
But what about when @super_admin evaluates to true? The previous code applied .where(Role.creator_condition) for super admins, but now it does not.

I have mage the changes to ensure super admins plans has .where(Role.creator_condition). Always good to have code checked by a person not involved in change.

aaronskiba · 2025-11-27T17:00:07Z

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.

I'm not 100% sure of that statement. Here is the deactivate! method in the Plan model.

app/models/plan.rb
  # Deactivates the plan (sets all roles to inactive and visibility to :private)
  #
  # Returns Boolean
  def deactivate!
    # If no other :creator, :administrator or :editor is attached
    # to the plan, then also deactivate all other active roles
    # and set the plan's visibility to :private
    if authors.empty?
      roles.where(active: true).update_all(active: false)
      self.visibility = Plan.visibilities[:privately_visible]
      save!
    else
      false
    end
  end

Based on this code, it seems that rather than just the creator role, ALL roles associated with the plan are set to active = false. However, maybe this change is still needed?

Just to clarify, is this code change only menat to affect the CSV results?

johnpinto1 · 2025-12-02T13:42:02Z

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.

I'm not 100% sure of that statement. Here is the deactivate! method in the Plan model.
app/models/plan.rb
  # Deactivates the plan (sets all roles to inactive and visibility to :private)
  #
  # Returns Boolean
  def deactivate!
    # If no other :creator, :administrator or :editor is attached
    # to the plan, then also deactivate all other active roles
    # and set the plan's visibility to :private
    if authors.empty?
      roles.where(active: true).update_all(active: false)
      self.visibility = Plan.visibilities[:privately_visible]
      save!
    else
      false
    end
  end
Based on this code, it seems that rather than just the creator role, ALL roles associated with the plan are set to active = false. However, maybe this change is still needed?

Just to clarify, is this code change only menat to affect the CSV results?

I have removed the clearly false comment in my commit.

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.

aaronskiba

NOTE: Updated this comment for accuracy.

.where(roles: { active: true }).where(Role.creator_condition)

irb(main):001> Role.creator_condition
=> "(\"roles\".\"access\" in (1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31))"

So it is verifying that there is at least one active creator on the plan. As an edge case, there do in fact appear to be plans with a completely absent creator role. I believe the following SQL query can be used to check your db:

SELECT p.*
FROM plans p
WHERE NOT EXISTS (
    SELECT 1
    FROM roles r
    WHERE r.plan_id = p.id
      AND r.access IN (1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31)
);

These plans mostly appear to be quite old in DMP Assistant's db. I can look further, but I assume these plans were created prior to the current bit field implementation in the Role model:

  ##
  # Define Bit Field Values
  # Column access
  has_flags 1 => :creator,            # 1
            2 => :administrator,      # 2
            3 => :editor,             # 4
            4 => :commenter,          # 8
            5 => :reviewer,           # 16
            column: 'access',
            check_for_column: !Rails.env.test?

Are we sure we want to filter out plans with absent creator roles?

johnpinto1 · 2025-12-03T10:06:47Z

Are we sure we want to filter out plans with absent creator roles?

This PR code change aligns the CSV for the Org admin with the current Org admin plans view.

Any further changes to will need to thought through. Do we want to do this here or address this in a separate issue?

aaronskiba · 2025-12-03T16:33:05Z

app/models/org.rb

+          .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)


I wonder if a refactor like the following would be a nice improvement:

def org_admin_plans 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

Thanks for refactor advice. Nice and neat. Will add commit with change.

aaronskiba · 2025-12-03T17:07:13Z

app/controllers/paginable/plans_controller.rb

-      plans = @super_admin ? Plan.all : current_user.org.org_admin_plans
-      plans = plans.joins(:template, roles: [user: :org]).where(Role.creator_condition)


How about the following for a refactor?

plans = @super_admin ? Plan.where(Role.creator_condition) : current_user.org.org_admin_plans plans = plans.joins(:template, roles: [user: :org])

This code change in a new commit with another bit of re-factored code cited above.

aaronskiba · 2025-12-03T17:39:58Z

spec/models/org_spec.rb

+    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)


You're trying to add a :coowner here, but it looks like a :commenter is being added instead:

# Creates a role for the specified user (will update the user's # existing role if it already exists) # # Expects a User.id and access_type from the following list: # :creator, :administrator, :editor, :commenter # # Returns Boolean def add_user!(user_id, access_type = :commenter) user = User.where(id: user_id).first if user.present? role = Role.find_or_initialize_by(user_id: user_id, plan_id: id) # Access is cumulative, so set the appropriate flags # (e.g. an administrator can also edit and comment) case access_type when :creator role.creator = true role.administrator = true role.editor = true when :administrator role.administrator = true role.editor = true when :editor role.editor = true end role.commenter = true role.save else false end end

386: org1plan4.roles.map { |r| r.update(active: false) if r.user_id == owner_id } 387: byebug => 388: end 389: 390: it { is_expected.to include(org1plan1, org1plan2, org1plan3) } 391: it { is_expected.not_to include(org1plan4) } 392: end (byebug) roles = org1plan4.roles #<ActiveRecord::Associations::CollectionProxy [#<Role id: 4, user_id: 4, plan_id: 7, created_at: "2025-12-03 17:37:57.852288000 +0000", updated_at: "2025-12-03 17:37:57.855932000 +0000", access: 15, active: false>, #<Role id: 6, user_id: 6, plan_id: 7, created_at: "2025-12-03 17:37:57.987364000 +0000", updated_at: "2025-12-03 17:37:57.987364000 +0000", access: 8, active: true>]> (byebug) roles.last.creator? false (byebug) roles.last.administrator? false (byebug) roles.last.editor? false (byebug) roles.last.commenter? true (byebug) roles.last.reviewer? false (byebug)

My bad. Only allowed access_types ":creator, :administrator, :editor, :commenter". Will update code.

Updated tests to only add access_type :administrator for a coowner test.

Changes: - Refactored Org model org_admin_plans() method. - Refactored Paginable Plans controller method org_admin().

coowner being used incorrectly as an Role access_type. A coowner is any user apart from the plan creator with the Role administrator.

aaronskiba · 2025-12-04T17:05:25Z

Are we sure we want to filter out plans with absent creator roles?

This PR code change aligns the CSV for the Org admin with the current Org admin plans view.

Any further changes to will need to thought through. Do we want to do this here or address this in a separate issue?

Sorry, I've updated that previous comment I made, as it was partly inaccurate. (However, there are in fact active plans with non-existent creator roles.)

I am thinking of an alternate way to align the CSV with the Org Admin plans. This PR currently does so by applying .where(Role.creator_condition) in def org_admin_plans. Is removing .where(Role.creator_condition) altogether another way to accomplish that? I wish I knew why .where(Role.creator_condition) was first introduced, because I'm not sure why active plans with inactive creators would not be returned by org_admin_plans.

aaronskiba · 2025-12-04T17:25:44Z

Are we sure we want to filter out plans with absent creator roles?

This PR code change aligns the CSV for the Org admin with the current Org admin plans view.
Any further changes to will need to thought through. Do we want to do this here or address this in a separate issue?

Sorry, I've updated that previous comment I made, as it was partly inaccurate. (However, there are in fact active plans with non-existent creator roles.)

I am thinking of an alternate way to align the CSV with the Org Admin plans. This PR currently does so by applying .where(Role.creator_condition) in def org_admin_plans. Is removing .where(Role.creator_condition) altogether another way to accomplish that? I wish I knew why .where(Role.creator_condition) was first introduced, because I'm not sure why active plans with inactive creators would not be returned by org_admin_plans.

I tried digging back into the app/controllers/paginable/plans_controller.rb history and found this: #2434
Sorry @martaribeiro, this is going way back, but do you have any thoughts or recollections on why .where(Role.creator_condition) may have been introduced here.

johnpinto1 · 2025-12-04T17:27:50Z

Are we sure we want to filter out plans with absent creator roles?

This PR code change aligns the CSV for the Org admin with the current Org admin plans view.
Any further changes to will need to thought through. Do we want to do this here or address this in a separate issue?

Sorry, I've updated that previous comment I made, as it was partly inaccurate. (However, there are in fact active plans with non-existent creator roles.)

I am thinking of an alternate way to align the CSV with the Org Admin plans. This PR currently does so by applying .where(Role.creator_condition) in def org_admin_plans. Is removing .where(Role.creator_condition) altogether another way to accomplish that? I wish I knew why .where(Role.creator_condition) was first introduced, because I'm not sure why active plans with inactive creators would not be returned by org_admin_plans.

I tried digging back into the app/controllers/paginable/plans_controller.rb history and found this: #2434
Sorry @martaribeiro, this is going way back, but do you have any thoughts or recollections on why .where(Role.creator_condition) may have been introduced here.

Let us research the history of the code evolution to answer this intelligently. However, we in DMPOnline focused on aligning what the Org admins saw in the UI with the CSV download for this PR. I suspect we need a new issue for further changes.

aaronskiba · 2025-12-04T19:44:53Z

Are we sure we want to filter out plans with absent creator roles?

This PR code change aligns the CSV for the Org admin with the current Org admin plans view.
Any further changes to will need to thought through. Do we want to do this here or address this in a separate issue?

Sorry, I've updated that previous comment I made, as it was partly inaccurate. (However, there are in fact active plans with non-existent creator roles.)
I am thinking of an alternate way to align the CSV with the Org Admin plans. This PR currently does so by applying .where(Role.creator_condition) in def org_admin_plans. Is removing .where(Role.creator_condition) altogether another way to accomplish that? I wish I knew why .where(Role.creator_condition) was first introduced, because I'm not sure why active plans with inactive creators would not be returned by org_admin_plans.

I tried digging back into the app/controllers/paginable/plans_controller.rb history and found this: #2434
Sorry @martaribeiro, this is going way back, but do you have any thoughts or recollections on why .where(Role.creator_condition) may have been introduced here.

Let us research the history of the code evolution to answer this intelligently. However, we in DMPOnline focused on aligning what the Org admins saw in the UI with the CSV download for this PR. I suspect we need a new issue for further changes.

I guess the two will be aligned whether we add or remove .where(Role.creator_condition) from org_admin_plans. Maybe keeping it for now, as you are doing here, errs on the safer side.

Maybe an example scenario like the following would help: Suppose Marta is the org_admin, and John and I belong to her org. I create a plan, mark it organisationally visible, and add John as a co-owner. Marta can see this plan. Later I delete myself from the plan, leaving John as an active co-owner but with no remaining active “creator” role on the record. Should Marta still see it in the Org Admin plans page and CSV?

I don't know if this PR provides any further insights. It made a change that replaced .where(Role.creator_condition) with .where(roles: { active: true }) in scope :search
#3023

johnpinto1 · 2025-12-05T11:11:02Z

Are we sure we want to filter out plans with absent creator roles?

This PR code change aligns the CSV for the Org admin with the current Org admin plans view.
Any further changes to will need to thought through. Do we want to do this here or address this in a separate issue?

Sorry, I've updated that previous comment I made, as it was partly inaccurate. (However, there are in fact active plans with non-existent creator roles.)
I am thinking of an alternate way to align the CSV with the Org Admin plans. This PR currently does so by applying .where(Role.creator_condition) in def org_admin_plans. Is removing .where(Role.creator_condition) altogether another way to accomplish that? I wish I knew why .where(Role.creator_condition) was first introduced, because I'm not sure why active plans with inactive creators would not be returned by org_admin_plans.

I tried digging back into the app/controllers/paginable/plans_controller.rb history and found this: #2434
Sorry @martaribeiro, this is going way back, but do you have any thoughts or recollections on why .where(Role.creator_condition) may have been introduced here.

Let us research the history of the code evolution to answer this intelligently. However, we in DMPOnline focused on aligning what the Org admins saw in the UI with the CSV download for this PR. I suspect we need a new issue for further changes.

I guess the two will be aligned whether we add or remove .where(Role.creator_condition) from org_admin_plans. Maybe keeping it for now, as you are doing here, errs on the safer side.

Maybe an example scenario like the following would help: Suppose Marta is the org_admin, and John and I belong to her org. I create a plan, mark it organisationally visible, and add John as a co-owner. Marta can see this plan. Later I delete myself from the plan, leaving John as an active co-owner but with no remaining active “creator” role on the record. Should Marta still see it in the Org Admin plans page and CSV?

I don't know if this PR provides any further insights. It made a change that replaced .where(Role.creator_condition) with .where(roles: { active: true }) in scope :search #3023

This PR as I have said was to address the discrepancy between view and CSV because the CSV was using a different call. This just aligned the call. The users were finding it strange that the CSV had extra plans not visible in UI.

aaronskiba · 2025-12-05T19:37:06Z

This PR as I have said was to address the discrepancy between view and CSV because the CSV was using a different call. This just aligned the call. The users were finding it strange that the CSV had extra plans not visible in UI.

Sounds good. Sorry, yes, my review/questions might be a bit overkill. This only changes/reduces the CSV results to align with the org admin view.

johnpinto1 self-assigned this Sep 25, 2025

johnpinto1 requested review from aaronskiba and momo3404 September 25, 2025 14:59

aaronskiba reviewed Nov 27, 2025

View reviewed changes

johnpinto1 force-pushed the issue/3561 branch from 4ac02d2 to 65828ca Compare December 2, 2025 13:28

johnpinto1 force-pushed the issue/3561 branch from 65828ca to 79d4571 Compare December 2, 2025 13:46

aaronskiba reviewed Dec 2, 2025

View reviewed changes

aaronskiba reviewed Dec 3, 2025

View reviewed changes

johnpinto1 added 2 commits December 4, 2025 14:25

Issue #3561 - Refactoring code based on comments bt @aaronskiba

0c98319

Changes: - Refactored Org model org_admin_plans() method. - Refactored Paginable Plans controller method org_admin().

Updated RSpec spec/models/org_spec.rb to fix a bug associated with

fc48b6c

coowner being used incorrectly as an Role access_type. A coowner is any user apart from the plan creator with the Role administrator.

johnpinto1 force-pushed the issue/3561 branch from f84ad69 to fc48b6c Compare December 4, 2025 14:56

		plans = @super_admin ? Plan.all : current_user.org.org_admin_plans
		plans = plans.joins(:template, roles: [user: :org]).where(Role.creator_condition)

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

Are you sure you want to change the base?

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

Conversation

johnpinto1 commented Sep 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronskiba commented Nov 27, 2025

Uh oh!

johnpinto1 commented Dec 2, 2025

Uh oh!

aaronskiba left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnpinto1 commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronskiba Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronskiba commented Dec 4, 2025

Uh oh!

aaronskiba commented Dec 4, 2025

Uh oh!

johnpinto1 commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronskiba commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnpinto1 commented Dec 5, 2025

Uh oh!

aaronskiba commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aaronskiba left a comment •

edited

Loading

johnpinto1 commented Dec 3, 2025 •

edited

Loading

aaronskiba Dec 3, 2025 •

edited

Loading

johnpinto1 commented Dec 4, 2025 •

edited

Loading

aaronskiba commented Dec 4, 2025 •

edited

Loading