-
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
Bug 462 api not possible to get plan content #2965
Bug 462 api not possible to get plan content #2965
Conversation
This is just to start conversation, as it will need critiquing. Still need Tests fixing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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 |
Fixing tests are proving trickier than expected. May take a day or two. 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 Is this a valid change? I guess it used the old plans() method is my only rationale for this.
|
0db9379
to
f4dc902
Compare
@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> Relevant tests:
|
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 |
bb591ca
to
6831095
Compare
… failing in some cases. Changes: - 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(). - The Plan model scope :organisationally_or_publicly_visible uses the or_admin_plans() method. - The tests org_spec.rb and plan_spec.rb updated to reflect changed output from new native org.plans() method customised org.org_admin_plans().
6831095
to
525d7a2
Compare
@raycarrick-ed @briri I fixed the tests. But I have a worry now all the tests for plans in spec/models/org_spec.rb
|
thanks @johnpinto1. I think its ok to update these tests to reflect the changes you have made. Those tests were written back in 2018. |
@briri Thanks for update. @raycarrick-ed I believe this is now complete. So either merge. |
…_api_not_possible_to_get_plan_content Bug 462 api not possible to get plan content
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().