diff --git a/app/actions/v3/service_plan_visibility_update.rb b/app/actions/v3/service_plan_visibility_update.rb index 4c8710321b0..5d3b94a8fd2 100644 --- a/app/actions/v3/service_plan_visibility_update.rb +++ b/app/actions/v3/service_plan_visibility_update.rb @@ -8,21 +8,16 @@ class UnprocessableRequest < ::StandardError end def update(service_plan, message, append_organizations: false) - requested_type = message.type + type = message.type requested_org_guids = message.organizations&.pluck(:guid) || [] - unprocessable!("cannot update plans with visibility type 'space'") if space?(service_plan.visibility_type) - - if append_organizations # i.e. POST request - unprocessable!("type must be 'organization'") unless org?(requested_type) - unprocessable!("can only append organizations to plans with visibility type 'organization'") unless org?(service_plan.visibility_type) - end + unprocessable!("cannot update plans with visibility type 'space'") if space?(service_plan) service_plan.db.transaction do service_plan.lock! - service_plan.public = public?(requested_type) + service_plan.public = public?(type) service_plan.save - if org?(requested_type) + if org?(type) update_service_plan_visibilities(service_plan, requested_org_guids, append_organizations) else service_plan.remove_all_service_plan_visibilities @@ -74,8 +69,8 @@ def unprocessable!(message) raise UnprocessableRequest.new(message) end - def space?(type) - type == VCAP::CloudController::ServicePlanVisibilityTypes::SPACE + def space?(service_plan) + service_plan.visibility_type == VCAP::CloudController::ServicePlanVisibilityTypes::SPACE end def org?(type) diff --git a/app/controllers/v3/service_plan_visibility_controller.rb b/app/controllers/v3/service_plan_visibility_controller.rb index 8a077bd0ad8..940f72ad194 100644 --- a/app/controllers/v3/service_plan_visibility_controller.rb +++ b/app/controllers/v3/service_plan_visibility_controller.rb @@ -59,7 +59,7 @@ def event_repository VCAP::CloudController::Repositories::ServiceEventRepository::WithUserActor.new(user_audit_info) end - def update_visibility(append_organizations: false) + def update_visibility(opts={}) service_plan = ServicePlanFetcher.fetch(hashed_params[:guid]) service_plan_not_found! unless service_plan.present? && visible_to_current_user?(plan: service_plan) unauthorized! unless current_user_can_write?(service_plan) @@ -67,7 +67,7 @@ def update_visibility(append_organizations: false) message = ServicePlanVisibilityUpdateMessage.new(hashed_params[:body]) bad_request!(message.errors.full_messages) unless message.valid? - updated_service_plan = V3::ServicePlanVisibilityUpdate.new.update(service_plan, message, append_organizations:) + updated_service_plan = V3::ServicePlanVisibilityUpdate.new.update(service_plan, message, **opts) event_repository.record_service_plan_update_visibility_event(service_plan, message.audit_hash) updated_service_plan rescue V3::ServicePlanVisibilityUpdate::UnprocessableRequest => e diff --git a/docs/v3/source/includes/resources/service_plan_visibility/_apply.md.erb b/docs/v3/source/includes/resources/service_plan_visibility/_apply.md.erb index bafd322bb8f..f791e816d98 100644 --- a/docs/v3/source/includes/resources/service_plan_visibility/_apply.md.erb +++ b/docs/v3/source/includes/resources/service_plan_visibility/_apply.md.erb @@ -28,7 +28,7 @@ Content-Type: application/json <%= yield_content :service_plan_visibility_response %> ``` -This endpoint appends to the existing list of organizations. See [PATCH service plan visibility endpoint](#update-a-service-plan-visibility) to replace the existing list or change the visibility `type`. +This endpoint applies a service plan visibility. It behaves similar to the [PATCH service plan visibility endpoint](#update-a-service-plan-visibility) but this endpoint will append to the existing list of organizations when the service plan is `organization` visible. #### Definition `POST /v3/service_plans/:guid/visibility` @@ -37,8 +37,8 @@ This endpoint appends to the existing list of organizations. See [PATCH service Name | Type | Description ---- | ---- | ----------- -**type** | _string_ | Denotes the visibility of the plan; must be `organization`, see [_list of visibility types_](#list-of-visibility-types) -**organizations** | _array of objects_ | Desired list of organizations GUIDs where the plan will be accessible; required. +**type** | _string_ | Denotes the visibility of the plan; can be `public`, `admin`, `organization`, see [_list of visibility types_](#list-of-visibility-types) +**organizations** | _array of objects_ | Desired list of organizations GUIDs where the plan will be accessible; required if `type` is `organization` #### Permitted roles | diff --git a/docs/v3/source/includes/resources/service_plan_visibility/_update.md.erb b/docs/v3/source/includes/resources/service_plan_visibility/_update.md.erb index 59f3fb1487e..de7a9747a19 100644 --- a/docs/v3/source/includes/resources/service_plan_visibility/_update.md.erb +++ b/docs/v3/source/includes/resources/service_plan_visibility/_update.md.erb @@ -28,7 +28,7 @@ Content-Type: application/json <%= yield_content :new_org_service_plan_visibility %> ``` -This endpoint updates a service plan visibility. When `type` is `organization`, it will **replace** the existing list of organizations. See [POST service plan visibility endpoint](#apply-a-service-plan-visibility) to append to the existing list. +This endpoint updates a service plan visibility. It behaves similar to the [POST service plan visibility endpoint](#apply-a-service-plan-visibility) but this endpoint will replace the existing list of organizations when the service plan is `organization` visible. #### Definition `PATCH /v3/service_plans/:guid/visibility` diff --git a/spec/request/service_plan_visibility_spec.rb b/spec/request/service_plan_visibility_spec.rb index faa11a93fac..8ebd9bc4d26 100644 --- a/spec/request/service_plan_visibility_spec.rb +++ b/spec/request/service_plan_visibility_spec.rb @@ -454,11 +454,21 @@ let(:service_plan) { VCAP::CloudController::ServicePlan.make(public: true) } let(:body) { { type: 'organization', organizations: [{ guid: org.guid }] } } - it 'returns a 422 bad request' do + it 'updates the visibility type AND add the orgs' do post api_url, body.to_json, admin_headers - expect(last_response).to have_status_code(422) - expect(parsed_response['errors'].first['detail']).to include("can only append organizations to plans with visibility type 'organization'") + expect(parsed_response['type']).to eq 'organization' + expect(parsed_response).not_to have_key('organizations') + end + + it 'creates an audit event' do + post api_url, body.to_json, admin_headers + event = VCAP::CloudController::Event.find(type: 'audit.service_plan_visibility.update') + expect(event).to be + expect(event.actee).to eq(service_plan.guid) + expect(event.data).to include({ + 'request' => body.with_indifferent_access + }) end end @@ -509,11 +519,24 @@ context 'when request type is not "organization"' do let(:body) { { type: 'public' } } - it 'returns a 422 bad request' do + it 'behaves like a PATCH' do post api_url, body.to_json, admin_headers + expect(last_response).to have_status_code(200) - expect(last_response).to have_status_code(422) - expect(parsed_response['errors'].first['detail']).to include("type must be 'organization'") + get api_url, {}, admin_headers + expect(parsed_response).to eq({ 'type' => 'public' }) + visibilities = VCAP::CloudController::ServicePlanVisibility.where(service_plan:).all + expect(visibilities).to be_empty + end + + it 'creates an audit event' do + post api_url, body.to_json, admin_headers + event = VCAP::CloudController::Event.find(type: 'audit.service_plan_visibility.update') + expect(event).to be + expect(event.actee).to eq(service_plan.guid) + expect(event.data).to include({ + 'request' => body.with_indifferent_access + }) end end diff --git a/spec/support/lifecycle_tests_helpers.rb b/spec/support/lifecycle_tests_helpers.rb index 98b17a12526..a3d3056b636 100644 --- a/spec/support/lifecycle_tests_helpers.rb +++ b/spec/support/lifecycle_tests_helpers.rb @@ -88,7 +88,7 @@ def catalog end def make_plan_visible(plan_guid) - patch "v3/service_plans/#{plan_guid}/visibility", { type: 'public' }.to_json, admin_headers + post "v3/service_plans/#{plan_guid}/visibility", { type: 'public' }.to_json, admin_headers expect(last_response).to have_status_code(200) get "v3/service_plans/#{plan_guid}/visibility", nil, admin_headers diff --git a/spec/unit/actions/v3/service_plan_visibility_update_spec.rb b/spec/unit/actions/v3/service_plan_visibility_update_spec.rb index a4475ea8343..4bf02fa5df8 100644 --- a/spec/unit/actions/v3/service_plan_visibility_update_spec.rb +++ b/spec/unit/actions/v3/service_plan_visibility_update_spec.rb @@ -19,17 +19,9 @@ module V3 updated_plan = subject.update(service_plan, message) expect(updated_plan.reload.visibility_type).to eq 'public' end - - context "when 'append_orgs' is set to true" do - it 'raises an error' do - expect do - subject.update(service_plan, message, append_organizations: true) - end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "type must be 'organization'") - end - end end - context 'and its being updated to "organization"' do + context 'and its being updated do "organization"' do let(:org_guid) { Organization.make.guid } let(:other_org_guid) { Organization.make.guid } let(:params) do @@ -46,14 +38,6 @@ module V3 expect(visible_org_guids).to contain_exactly(org_guid, other_org_guid) end - - context "when 'append_orgs' is set to true" do - it 'raises an error' do - expect do - subject.update(service_plan, message, append_organizations: true) - end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "can only append organizations to plans with visibility type 'organization'") - end - end end end @@ -67,17 +51,9 @@ module V3 updated_plan = subject.update(service_plan, message) expect(updated_plan.reload.visibility_type).to eq 'admin' end - - context "when 'append_orgs' is set to true" do - it 'raises an error' do - expect do - subject.update(service_plan, message, append_organizations: true) - end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "type must be 'organization'") - end - end end - context 'and its being updated to "organization"' do + context 'and its being updated do "organization"' do let(:org_guid) { Organization.make.guid } let(:other_org_guid) { Organization.make.guid } let(:params) do @@ -94,14 +70,6 @@ module V3 expect(visible_org_guids).to contain_exactly(org_guid, other_org_guid) end - - context "when 'append_orgs' is set to true" do - it 'raises an error' do - expect do - subject.update(service_plan, message, append_organizations: true) - end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "can only append organizations to plans with visibility type 'organization'") - end - end end end @@ -168,14 +136,6 @@ module V3 expect(updated_plan.service_plan_visibilities).to be_empty expect(ServicePlanVisibility.where(service_plan:).all).to be_empty end - - context "when 'append_orgs' is set to true" do - it 'raises an error' do - expect do - subject.update(service_plan, message, append_organizations: true) - end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "type must be 'organization'") - end - end end context 'and its being updated to "public"' do @@ -187,14 +147,6 @@ module V3 expect(updated_plan.service_plan_visibilities).to be_empty expect(ServicePlanVisibility.where(service_plan:).all).to be_empty end - - context "when 'append_orgs' is set to true" do - it 'raises an error' do - expect do - subject.update(service_plan, message, append_organizations: true) - end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "type must be 'organization'") - end - end end end