Bug 462 api not possible to get plan content

[johnpinto1]

Fixes #462: Open API - not possible to get the content, plans throwing error

Changes proposed in this PR:
- Reverted to replacing customised plans() method on Org model with
the native plans() method.
- Created a new org_admin_plans() method in Org model to replace old
customised plans() for Org Admins. This picks up plans of users in Org
that are co-owners of plans with plan.org_id of a different org.
- Fixed a null pointer bug that broke the json output ig the Data contact was empty app/views/api/v0/plans/index.json.jbuilder.
- Renamed method calls in case of Org Admins that used org.plans() to org.org_admin_plans().

[johnpinto1]

This is just to start conversation, as it will need critiquing. Still need Tests fixing.

[raycarrick-ed]

As far as I can see this looks ok. Adds in the plans that were missing and fixed the JSON failure. So looks good to me.

But test failure needs investigated a bit. Either test is wrong or new code: which is it?

[briri]

thanks for patching this @johnpinto1. Agree with Ray, just need to clean up those few tests. I'm guessing they may just need to be updated to use your new method instead of org.plans

[johnpinto1]

Fixing tests are proving trickier than expected. May take a day or two.
I have not pushed this code but to sort test failures for

rspec ./spec/models/plan_spec.rb:264 # Plan.organisationally_or_publicly_visible when plan visibility is publicly_visible includes publicly_visible plans

rspec ./spec/models/plan_spec.rb:285 # Plan.organisationally_or_publicly_visible when plan visibility is organisationally_visible includes organisationally_visible plans

I had to change model Plan plan_ids = user.org.plans.where(complete: true).pluck(:id).uniq
to plan_ids = user.org.org_admin_plans.where(complete: true).pluck(:id).uniq

Is this a valid change? I guess it used the old plans() method is my only rationale for this.

# Retrieves any plan organisationally or publicly visible for a given org id
  scope :organisationally_or_publicly_visible, lambda { |user|
    plan_ids = user.org.org_admin_plans.where(complete: true).pluck(:id).uniq
    includes(:template, roles: :user)
      .where(id: plan_ids, visibility: [
               visibilities[:organisationally_visible],
               visibilities[:publicly_visible]
             ])
      .where(
        "NOT EXISTS (SELECT 1 FROM roles WHERE plan_id = plans.id AND user_id = ?)",
        user.id
      )
  }

[johnpinto1]

@raycarrick @briri The change breaks these 3 tests in org_rspec.rb after changing subject to use org_admin_plans(). Incidentally, it fails with native plans() too. This is because the native plans() method plans are now added to org_admin_plans(). Advice required on how to proceed.

Failed examples:

rspec ./spec/models/org_spec.rb:405 # Org#plans user belongs to Org and plan user with role :commenter, but not :creator and :admin is expected not to include #<Plan id: 1, title: "utilize B2B vortals", template_id: 1, created_at: "2021-07-08 09:12:58", update...ant_id: nil, start_date: "2021-07-08 09:12:58", end_date: "2023-07-08 09:12:58", api_client_id: nil>
rspec ./spec/models/org_spec.rb:395 # Org#plans user belongs to Org and plan user with role :editor, but not :creator and :admin is expected not to include #<Plan id: 3, title: "synergize cross-platform markets", template_id: 3, created_at: "2021-07-08 09:1...ant_id: nil, start_date: "2021-07-08 09:12:58", end_date: "2023-07-08 09:12:58", api_client_id: nil>
rspec ./spec/models/org_spec.rb:415 # Org#plans user belongs to Org and plan user with role :reviewer, but not :creator and :admin is expected not to include #<Plan id: 4, title: "deliver enterprise e-commerce", template_id: 4, created_at: "2021-07-08 09:12:5...ant_id: nil, start_date: "2021-07-08 09:12:58", end_date: "2023-07-08 09:12:58", api_client_id: nil>

Relevant tests:

describe "#plans" do

    let!(:org) { create(:org) }
    let!(:plan) { create(:plan, org: org) }
    let!(:user) { create(:user, org: org) }

    subject { org.org_admin_plans }

....

context "user belongs to Org and plan user with role :editor, but not :creator and :admin" do

      before do
        plan.add_user!(user.id, :editor)
      end

      it { is_expected.not_to include(plan) }

    end

    context "user belongs to Org and plan user with role :commenter, but not :creator and :admin" do

      before do
        plan.add_user!(user.id, :commenter)
      end

      it { is_expected.not_to include(plan) }

    end

    context "user belongs to Org and plan user with role :reviewer, but not :creator and :admin" do

      before do
        plan.add_user!(user.id, :reviewer)
      end

      it { is_expected.not_to include(plan) }

    end

  end

[briri]

I'd suggest replacing those old tests with a full set for each of the new methods. To ensure that we have all of the scenarios covered

[johnpinto1]

@raycarrick-ed @briri I fixed the tests. But I have a worry now all the tests for plans in spec/models/org_spec.rb
expect plans to be found. For these contexts:

• "when user belongs to Org and plan owner with role :creator" (Previously, did not include plan)
• "when user belongs to Org and plan user with role :administrator"
• "user belongs to Org and plan user with role :editor, but not :creator and :admin"
• "user belongs to Org and plan user with role :commenter, but not :creator and :admin" (Previously, did not include plan)
• "user belongs to Org and plan user with role :reviewer, but not :creator and :admin" (Previously, did not include plan)

[briri]

thanks @johnpinto1. I think its ok to update these tests to reflect the changes you have made. Those tests were written back in 2018.

[johnpinto1]

@briri Thanks for update. @raycarrick-ed I believe this is now complete. So either merge.