use relation org.plans instead of method, which works faster and more correct

[nicolasfranck]

Fixes issue #2724 .

[nicolasfranck]

mm, there seems to be something wrong with the integration tests for postgresql and mysql..

[briri]

it looks like there was a change to the way GitHub actions wants us to dynamically set env variables. I have submitted a fix for this in #2728. The dev team will be reviewing these PRs tomorrow.

[briri]

Hi @nicolasfranck and thanks for sending these PRs over! We are a small team so your contributions and suggestions are appreciated.

The org.plans method is used fairly extensively across the site and so removing the above will have many unintended consequences.

A quick search shows it is used for statistics, security policies, UI navigation logic, and the API:

> 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? %>

We agree that Org.plans should default to the standard ActiveModel relationship functionality and just return any Plans where org_id = ?. We would still need to retain the other though and determine which one to use in each scenario. It will take some work for us to do the proper analysis.

[xsrust]

Updating this PR as well, see #2741 (comment) for details

[nicolasfranck]

Ok, now it works like before, but now org.plans is cached so the crazy query is not reexecuted every time someone uses that statement.

[xsrust]

Thanks @nicolasfranck
I think the cache approach should alleviate some of the performance issues in the meantime, but longer-term we should definitely consider making this a custom-relationship and renaming to something like org.plans_where_administrator (or something more concise) and then defining org.plans through the association.

[briri]

👍 I think this approach can be useful elsewhere as well.

[nicolasfranck]

org.plans seems to be replaced by org.org_admin_plans on many places in release 3.0.3, except for this line.
Is that line doing the right thing then?