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

[johnpinto1]

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]

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]

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.

[johnpinto1]

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]

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]

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: https://github.com/DMPRoadmap/roadmap/pull/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]

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: https://github.com/DMPRoadmap/roadmap/pull/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]

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 https://github.com/DMPRoadmap/roadmap/pull/3023

[johnpinto1]

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]

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]

Glad you added some test coverage for org_admin_plans. I was going to add some comments for that as well as some refactoring recommendations in spec/factories/plans.rb.

I ended up creating a branch off of yours and tried to code things up instead. Hope that's okay and that I didn't go too far with my changes.

I made two commits on this branch: https://github.com/DMPRoadmap/roadmap/tree/aaron/issue/3561

Let me know what you think. If it works, maybe we could just cherry-pick those commits onto this PR's branch.

That looks like nice concise re-factoring of my code. So merge.

[johnpinto1]

That looks like nice concise re-factoring of my code. So merge.

@aaronskiba Nice that is merged.