Refactor method Org#plans?

[nicolasfranck]

Please complete the following fields as applicable:

Expected behaviour:

The method Org#plans should only return plans of the current Org.

Actual behaviour:

It also returns plans of other orgs.

Besides, it generates very large queries, that are reexecuted every time someone uses org.plans in his code. If I remove this method, and so use the more simple (and cached) relation plans, the global page loading time goes from 4s to 40ms.

Reason: The relation plans in the model Org is defined, but overridden a few lines further (https://github.com/DMPRoadmap/roadmap/blob/master/app/models/org.rb#L287). But that last method is slow and does not work as expected.

How it works now:

• Select all roles with active:true, access in set of administrative, and user_id in list of org.users.pluck(:id)
• Pluck attribute plan_id, make it unique, and store it in plan_ids
• Fetch all plans with id in list of plan_ids

Why it does not work: some users can have administrative rights to plans that actually belong to another organisation

Is this intended behaviour?

Steps to reproduce:

[briri]

Hi @nicolasfranck this behavior is desired as it allows for scenarios where a DMP is a collaboration between multiple organizations. For example if a user from Org A creates a DMP and then invites fellow collaborators from Org B. We have many areas in the code that rely on this.

We do however agree that overriding the default association org.plans may not have been the best choice as it can be misleading and return unexpected results. Perhaps a different name for the method like related_plans would have been better.

We also agree that the query is inefficient and could use improvement.

[nicolasfranck]

That explains why it is just a list of plans without adding links to the "show" or "edit" page.

So instead of being a list of plans belong to this current organisation, it is a list of plans that members of this organisation have administrative rights to. Is it important for org-admins to know that certain members have "private" access to plans outside of the organisation (without being able to look more closely)?

Where is this behaviour used, the statistics?

Just curious

[briri]

Hi @nicolasfranck a cursory search for that method shows that its used on the statistics, policies (used in conjunction with the Devise and Pundit gems for authorization), the API, and several areas of page navigation.

> grep -r 'rg\.plans' app 
app/models/plan.rb:    plan_ids = user.org.plans.where(complete: true).pluck(:id).uniq
app/policies/api/v1/plans_policy.rb:            ids += client.org.plans.organisationally_visible.pluck(:id)
app/policies/api/v1/plans_policy.rb:            ids += client.org.plans.pluck(:id) if client.can_org_admin?
app/policies/plan_policy.rb:       @user.org.plans.include?(@plan))
app/controllers/paginable/plans_controller.rb:    plans = @super_admin ? Plan.all : current_user.org.plans
app/controllers/api/v0/statistics_controller.rb:               @user.org.plans.where(complete: true)
app/controllers/api/v0/statistics_controller.rb:               @user.org.plans
app/controllers/api/v0/statistics_controller.rb:    scoped = @user.org.plans
app/controllers/api/v0/statistics_controller.rb:    @org_plans = @user.org.plans
app/controllers/api/v0/plans_controller.rb:    @plans = @user.org.plans.includes([{ roles: :user }, { answers: :question_options },
app/controllers/org_admin/plans_controller.rb:    @plans = @super_admin ? Plan.all.page(1) : current_user.org.plans.page(1)
app/controllers/org_admin/plans_controller.rb:      org.plans.includes(template: :org).order(updated_at: :desc).each do |plan|
app/views/plans/_navigation.html.erb:  <% if plan.administerable_by?(current_user) || (current_user.can_org_admin? && current_user.org.plans.include?(plan)) %>
app/views/plans/_navigation.html.erb:  <% if (plan.administerable_by?(current_user) || (current_user.can_org_admin? && current_user.org.plans.include?(plan))) && plan.owner_and_coowners.include?(current_user) && plan.owner.org.feedback_enabled? %>

My guess is that it would be worthwhile to further investigate where and how it is used throughout the site. My guess is that in some situations we could use the original .plans association that comes with defining the association on the Model so that it returns all Plans where org_id = ?.

I'll update the PR with some ides.

[briri]

Leaving this one open. We should still address this. The PR #2726 improves load performance but overriding Org.plans is confusing.

[briri]

I ran into this issue when working on the functionality to merge Orgs. The PR #2763 uses to_be_merged.plans.update_all(org_id: id), but this does not truly update all of the Plans that need to be updated. If this issue is still unresolved we will need to change that line to Plan.where(org_id: to_be_merged.id).update_all(org_id: id) to ensure that all of the plans are updated properly

[nicolasfranck]

Something else that I saw: the list of plans that belongs to an organisation is only based on access permission Role.administrator (co-owner). Shouldn't Role.creator also be included?

[nicolasfranck]

Something else that I saw: the list of plans that belongs to an organisation is only based on access permission Role.administrator (co-owner). Shouldn't Role.creator also be included?

Never mind, apparently the other (underlying) permissions are also applied in Plan#add_user!