-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor method Org#plans? #2724
Comments
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 We also agree that the query is inefficient and could use improvement. |
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 Where is this behaviour used, the statistics? Just curious |
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 I'll update the PR with some ides. |
Leaving this one open. We should still address this. The PR #2726 improves load performance but overriding |
I ran into this issue when working on the functionality to merge Orgs. The PR #2763 uses |
Something else that I saw: the list of plans that belongs to an organisation is only based on access permission |
Never mind, apparently the other (underlying) permissions are also applied in Plan#add_user! |
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 modelOrg
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:
active
:true
, access in set ofadministrative
, anduser_id
in list oforg.users.pluck(:id)
plan_id
, make it unique, and store it inplan_ids
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:
The text was updated successfully, but these errors were encountered: