Revert "Tighten the semantics of POST /v3/service_plans/:guid/visibility (#4609)" (#4975)

This reverts commit 1306e1c.

Author: philippthun

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)

app/controllers/v3/service_plan_visibility_controller.rb

@@ -59,15 +59,15 @@ 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)
 
     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

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
  |

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`

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
 

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

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