Address comments

cloudfoundry · Jan 8, 2025 · f161ec2 · f161ec2
1 parent 8ac1631
commit f161ec2
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 32 deletions.
diff --git a/api/handlers/service_plan_test.go b/api/handlers/service_plan_test.go
@@ -5,7 +5,6 @@ import (
 	"net/http"
 	"strings"
 
-	apierrors "code.cloudfoundry.org/korifi/api/errors"
 	. "code.cloudfoundry.org/korifi/api/handlers"
 	"code.cloudfoundry.org/korifi/api/handlers/fake"
 	"code.cloudfoundry.org/korifi/api/payloads"
@@ -353,10 +352,6 @@ var _ = Describe("ServicePlan", func() {
 	})
 
 	Describe("DELETE /v3/service_plans/{guid}/visibility/{org-guid}", func() {
-		BeforeEach(func() {
-			servicePlanRepo.DeletePlanVisibilityReturns(nil)
-		})
-
 		JustBeforeEach(func() {
 			req, err := http.NewRequestWithContext(ctx, "DELETE", "/v3/service_plans/my-service-plan/visibility/org-guid", nil)
 			Expect(err).NotTo(HaveOccurred())
@@ -373,16 +368,6 @@ var _ = Describe("ServicePlan", func() {
 			Expect(rr).To(HaveHTTPStatus(http.StatusNoContent))
 		})
 
-		When("deleting the visibility fails with not found", func() {
-			BeforeEach(func() {
-				servicePlanRepo.DeletePlanVisibilityReturns(apierrors.NewNotFoundError(nil, repositories.ServicePlanVisibilityResourceType))
-			})
-
-			It("returns 404 Not Found", func() {
-				expectNotFoundError("Service Plan Visibility")
-			})
-		})
-
 		When("deleting the visibility fails with an error", func() {
 			BeforeEach(func() {
 				servicePlanRepo.DeletePlanVisibilityReturns(errors.New("visibility-err"))

diff --git a/api/repositories/service_plan_repository_test.go b/api/repositories/service_plan_repository_test.go
@@ -621,14 +621,14 @@ var _ = Describe("ServicePlanRepo", func() {
 				})).To(Succeed())
 			})
 
-			When("the user is not authorized", func() {
-				BeforeEach(func() {
-					visibilityErr = repo.DeletePlanVisibility(ctx, authInfo, repositories.DeleteServicePlanVisibilityMessage{
-						PlanGUID: planGUID,
-						OrgGUID:  anotherOrg.Name,
-					})
+			JustBeforeEach(func() {
+				visibilityErr = repo.DeletePlanVisibility(ctx, authInfo, repositories.DeleteServicePlanVisibilityMessage{
+					PlanGUID: planGUID,
+					OrgGUID:  anotherOrg.Name,
 				})
+			})
 
+			When("the user is not authorized", func() {
 				It("returns unauthorized error", func() {
 					Expect(visibilityErr).To(matchers.WrapErrorAssignableToTypeOf(apierrors.ForbiddenError{}))
 				})
@@ -640,13 +640,6 @@ var _ = Describe("ServicePlanRepo", func() {
 				})
 
 				When("the plan and org visibility exist", func() {
-					BeforeEach(func() {
-						visibilityErr = repo.DeletePlanVisibility(ctx, authInfo, repositories.DeleteServicePlanVisibilityMessage{
-							PlanGUID: planGUID,
-							OrgGUID:  anotherOrg.Name,
-						})
-					})
-
 					It("deletes the plan visibility in kubernetes", func() {
 						Expect(visibilityErr).ToNot(HaveOccurred())
 
@@ -657,7 +650,7 @@ var _ = Describe("ServicePlanRepo", func() {
 							},
 						}
 						Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(servicePlan), servicePlan)).To(Succeed())
-						Expect(servicePlan.Spec.Visibility.Organizations).To(Equal([]string{cfOrg.Name}))
+						Expect(servicePlan.Spec.Visibility.Organizations).To(ConsistOf(cfOrg.Name))
 					})
 				})
 
@@ -675,7 +668,7 @@ var _ = Describe("ServicePlanRepo", func() {
 				})
 
 				When("the org does not exist", func() {
-					BeforeEach(func() {
+					JustBeforeEach(func() {
 						visibilityErr = repo.DeletePlanVisibility(ctx, authInfo, repositories.DeleteServicePlanVisibilityMessage{
 							PlanGUID: planGUID,
 							OrgGUID:  "does-not-exist",
@@ -692,7 +685,7 @@ var _ = Describe("ServicePlanRepo", func() {
 							},
 						}
 						Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(servicePlan), servicePlan)).To(Succeed())
-						Expect(servicePlan.Spec.Visibility.Organizations).To(Equal([]string{cfOrg.Name, anotherOrg.Name}))
+						Expect(servicePlan.Spec.Visibility.Organizations).To(ConsistOf(cfOrg.Name))
 					})
 				})
 			})

diff --git a/tests/e2e/e2e_suite_test.go b/tests/e2e/e2e_suite_test.go
@@ -15,6 +15,7 @@ import (
 	"testing"
 	"time"
 
+	"code.cloudfoundry.org/korifi/model/services"
 	"code.cloudfoundry.org/korifi/tests/helpers"
 	"code.cloudfoundry.org/korifi/tests/helpers/fail_handler"
 
@@ -293,7 +294,8 @@ type cfErr struct {
 }
 
 type planVisibilityResource struct {
-	Type string `json:"type"`
+	Type          string                            `json:"type"`
+	Organizations []services.VisibilityOrganization `json:"organizations"`
 }
 
 func TestE2E(t *testing.T) {

diff --git a/tests/e2e/service_plans_test.go b/tests/e2e/service_plans_test.go
@@ -5,6 +5,7 @@ import (
 	"net/http"
 
 	korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
+	"code.cloudfoundry.org/korifi/model/services"
 	"code.cloudfoundry.org/korifi/tests/helpers/broker"
 	"github.com/BooleanCat/go-functional/v2/it/itx"
 	"github.com/go-resty/resty/v2"
@@ -121,6 +122,36 @@ var _ = Describe("Service Plans", func() {
 				))
 			})
 		})
+
+		Describe("Delete Visibility", func() {
+			JustBeforeEach(func() {
+				var err error
+
+				resp, err = adminClient.R().
+					SetBody(planVisibilityResource{
+						Type: "organization",
+						Organizations: []services.VisibilityOrganization{{
+							GUID: "org-guid",
+						}},
+					}).
+					Post(fmt.Sprintf("/v3/service_plans/%s/visibility", planGUID))
+				Expect(err).NotTo(HaveOccurred())
+				Expect(resp).To(SatisfyAll(
+					HaveRestyStatusCode(http.StatusOK),
+				))
+
+				resp, err = adminClient.R().
+					SetResult(&result).
+					Delete(fmt.Sprintf("/v3/service_plans/%s/visibility/%s", planGUID, "org-guid"))
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("deletes the plan visibility", func() {
+				Expect(resp).To(SatisfyAll(
+					HaveRestyStatusCode(http.StatusNoContent),
+				))
+			})
+		})
 	})
 
 	Describe("Delete", func() {