Issue #496 - Fix for search bug of Plans on the "My Dashboard".

[johnpinto1]

Changes:

• In the search scope in the Plan model the clause
  where(Role.creator_condition)
  has been removed.

• Paginable::PlansController method org_admin_other_user the paginable_renderiseblock has
  scope: Plan.active(@user)
  replaced by
  scope: Plan.active(@user).where(Role.creator_condition).

• In PublicPagesController method plan_index
  @plans = Plan.publicly_visible.includes(:template)
  has been replaced by
  @plans = Plan.where(Role.creator_condition).publicly_visible.includes(:template).

[johnpinto1]

A bug i found that I can't work out the reason why it occurs. Full details here
https://github.com/DigitalCurationCentre/DMPonline-Service/issues/496#issuecomment-897466348

[raycarrick-ed]

I may be being slow here but I can't see what either condition is doing. The "where(Role.creator_condition)" means only select plans that have a creator? The "where(roles: {active: true})" means select plans that have at least one active role? is this really an surrogate for "select only plans that are active" but in a roundabout way? What would be wrong with just removing the where clause? Presumably something or it wouldn't be there.

[briri]

It is really confusing.

When a user clicks the 'Remove' link to get rid of a plan from their 'My Dashboard' page, the plan doesn't actually get deleted. So it really just sets role.active = false in the DB. So they are still technically connected to the plan, it is just a way to hide the plan from them in the UI. I think the logic may have been that if the Plan were publicly or organisationally visible and someone exported the PDF (and now JSON), it still wants to show the creator info (even though they no longer want to see it on their dashboard)

For whatever reason, that creator_condition was added to this search method at some point. I cannot think of a use case where we want to limit the search to just those with a creator.

If the plan has no active roles though then everyone has said they don't want to see it, so should it be considered 'deleted'? I'm not sure.

There are paginable Plan tables (with search functionality) in the following places:

• My Dashboard - shows Plans the current user's currently has an 'active' role on (creator, co-owner, editor, commenter)
• My Dashboard - shows the organisationally visible plans for the current user
• Admin -> Plans - shows all of the plans owned/co-owned by one of the Org's users
• Admin -> Users - shows the Plans for the user being edited (not sure if these are creator or would show if the user is just an editor or commenter)
• Public Plans - shows any Plan marked as publicly_visible
• We'd have to look at the API v0 and v1 to see if they use that search method (I think v0 might)

Perhaps what we want here is to remove that where(Role.creator_condition) entirely from the search method as @raycarrick-ed suggests and then let the controller further restrict the results. Something like this (psuedocode) in the controller :

# Controller code
results = Plan.search(params[:search_term])
                      .where(Role.creator_condition)

We would need to do the same in all of those places listed above though.

[johnpinto1]

@raycarrick-ed @briri based on your comments I have updated code with changes
Changes:

• In the search scope in the Plan model the clause
  where(Role.creator_condition)
  has been removed.

• Paginable::PlansController method org_admin_other_user the paginable_renderiseblock has
  scope: Plan.active(@user)
  replaced by
  scope: Plan.active(@user).where(Role.creator_condition).

• In PublicPagesController method plan_index
  @plans = Plan.publicly_visible.includes(:template)
  has been replaced by
  @plans = Plan.where(Role.creator_condition).publicly_visible.includes(:template).

• In API v0 PlansController in the index method
  replaced
  org_admin_plans = @user.org.org_admin_plans)
  with
  org_admin_plans = @user.org.org_admin_plans).where(Role.creator_condition)

Further, I don't think I have make any changes here for this comment by @briri

• Admin -> Plans - shows all of the plans owned/co-owned by one of the Org's users:
  The Paginable::PlansController method org_admin already has the following lines with .where(Role.creator_condition) in send line:
  plans = @super_admin ? Plan.all : current_user.org.org_admin_plans
  plans = plans.joins(:template, roles: [user: :org]).where(Role.creator_condition)

• We'd have to look at the API v0 and v1 to see if they use that search method (I think v0 might)
  In app/controllers/api/v0/plans_controller.rb not sure if where(Role.creator_condition) is applicable in the index method:
  org_admin_plans = @user.org.org_admin_plans
  @plans = org_admin_plans.includes([{ roles: :user }, { answers: :question_options },
  template: [{ phases: {
  sections: { questions: %i[question_format themes] }
  } }, :org]])

[briri]

can we discuss this @magdalenadrafiova @mariapraetzellis @pherterich in the Wednesday call? We need to determine agree upon what each Plans table should show

[mariapraetzellis]

We discussed this in our team meeting. @johnpinto1

My Dashboard - shows Plans the current user's currently has an 'active' role on (creator, co-owner, editor, commenter)

• agreed this is the correct behavior

My Dashboard - shows the organisationally visible plans for the current user
-- agreed this is the correct behavior

Admin -> Plans - shows all of the plans owned/co-owned by one of the Org's users

• agreed this is the correct behavior
  -there is an exception to this in the code that are two configurable settings that allows an instance to set this so private plans do not display for admins or super admins

Admin -> Users - shows the Plans for the user being edited (not sure if these are creator or would show if the user is just an editor or commenter)

• This is the correct behavior but we do have an additional feature to add as described below. We will make a new issue for this additional feature.
• We should add a new column that shows the user's role in each plan. This would also need to adhere to the configuration settings that allows an instance to set this so private plans do not display for admins or super admins.

Public Plans - shows any Plan marked as publicly_visible

• This is the correct behavior

We'd have to look at the API v0 and v1 to see if they use that search method (I think v0 might)

• Please check API v0 & V1 to make sure they're following the configuration settings

[raycarrick-ed]

Explanatory comment on the roadmap instance settings.

In general, org admins should be able to see all plans connected to users in their org and super admins should be able to see all plans in the roadmap instance.

There is a special case, where org admins and super admins should not be allowed to see plans marked private or test. These two special cases are keyed of the org_admin_read_all and super_admin_read_all settings in the instance config (_dmproadmap.rb).

If set to true, then the default case happens i.e. they get to see plans that are private or test.
If set to false then we should exclude plans marked private or test.

At present, these two settings will be the same (i.e. both true or both false) but they don't have to be e.g. you could envisage a policy that says org admins should not be able to see private plans but super admins should. So they need to be independent.

This needs to be taken into account wherever we show plans to org_admin or superadmin.

[briri]

The way we manage which plans are available within the APIs may be of interest.

The Pundit Policies define a 'Scope' that reviews the current user's permissions and their affiliation.
https://github.com/DMPRoadmap/roadmap/blob/master/app/policies/api/v1/plans_policy.rb

Adding a similar scope method to the app/policies/plans_policy.rb that uses the new org_admin_plans and other methods you added for us recently might be useful. Its just a suggestion though. There are several use cases outlined above and this approach may prove to be more complicated/confusing to manage.

Here's an example of it being used in the controller:

roadmap/app/controllers/api/v1/plans_controller.rb

Line 87 in 5cb9c30

plans = Api::V1::PlansPolicy::Scope.new(client, Plan).resolve

[johnpinto1]

@briri @raycarrick-ed DO NOT MERGE STILL IN PROGRESS.

AS REQUIREMENTS EXPANDED AND CHANGED. KEEPING OPEN FOR THE CHANGES REQUESTED.

[johnpinto1]

CLOSING - All comments linked to https://github.com/DigitalCurationCentre/DMPonline-Service/issues/496