Add DELETE for org in a Service Plan Visibility

[klapkov]

Is there a related GitHub Issue?

#3645

What is this change about?

Here we add the ability to remove an organization from a service plan visibility

Does this PR introduce a breaking change?

No

Acceptance Steps

Tag your pair, your PM, and/or team

[georgethebeatle]

Left some comments inline

[georgethebeatle]

This BeforeEach is unnecessary as the service plan repository fake will return nil by default

[georgethebeatle]

In my opinion it would be more readable if the path is a plain single string:

ServicePlanVisibilityOrgPath = "/v3/service_plans/{guid}/visibility/{org-guid}"

Having it plain makes it easier to get what endpoints are implemented with a single glance. It also makes it easier to extract this information with a script

[georgethebeatle]

The handler does not process not found any differently than a generic error, so this test does not add more value then the one below.

[georgethebeatle]

This could go to a Describe-global JustBeforeEach as it is invoking the code under test. This will also reduce duplication

[georgethebeatle]

Here we could conveniently use gomega's ConsistOf matcher

Expect(servicePlan.Spec.Visibility.Organizations).To(ConsistOf(cfOrg.Name))

[georgethebeatle]

See above comment about usingConsistsOf

[danail-branekov]

In addition to @georgethebeatle's comments, as this is a new endpoint, I would also like to have an e2e test that excercises deleting orgs from plan visibility

PS: In the spirit of e2e tests, the new test should cover the very basic happy path, i.e. don't bother testing error cases (not authorised, or org not found)

[klapkov]

@danail-branekov @georgethebeatle Did some changes, let me know if all is good so I can fix the conflicts.

[danail-branekov]

I have added some comments on the tests sctrucuture, looks all right otherwise.

[danail-branekov]

If you create a variable for the delete message and default it to the value you use in the describe's JustBeforeEach in the BeforeEach, then in each context's BeforeEach you could just override the message and not invoke the method under test (DeleteServicePlanVisibility) over and over again.

I.e.

Describe("DeletePlanVisibility", func() {
  var deleteMessage repositories.DeleteServicePlanVisibilityMessage
  BeforeEach(func(){
    deleteMessage = repositories.DeleteServicePlanVisibilityMessage{
					PlanGUID: planGUID,
					OrgGUID:  anotherOrg.Name,
				}

  })

  JustBeforeEach(func(){
     visibilityErr = repo.DeletePlanVisibility(ctx, authInfo,  deleteMessage)
  })

  .........
  
  When("the plan does not exist", func() {
    BeforeEach(func(){
      deleteMessage.PlanGUID = "i-do-not-exist"
    })

    It(......)
  })

  When("the org does not exist", func() {
    BeforeEach(func(){
      deleteMessage.OrgGUID = "i-do-not-exist"
    })

    It(......)
  })
})

[klapkov]

@danail-branekov Done with the changes

[danail-branekov]

The change looks good, you would unfortunately have to resolve the conflicts