Skip to content
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

Provide pageable users of space endpoint #2491

Merged
merged 2 commits into from
Sep 8, 2021
Merged

Conversation

moleske
Copy link
Member

@moleske moleske commented Sep 3, 2021

  • /v3/spaces/:guid/users now exists with filters

[#2489]

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:

  • An explanation of the use cases your change solves

  • Links to any other associated PRs

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

@ctlong

@moleske moleske linked an issue Sep 3, 2021 that may be closed by this pull request
- /v3/spaces/:guid/users now exists with filters

[#2489]

Signed-off-by: Carson Long <lcarson@vmware.com>
Copy link
Contributor

@sweinstein22 sweinstein22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior Check

  • deployed changes to a bosh lite
  • ran cf curl /v3/users and cf curl /v3/spaces/:guid/users to confirm that only a subset of users was returned. User missing from the space level call was the network-policy
$ cf curl /v3/spaces/369cc547-b6b8-4ccb-a3c3-987845b04049/users | grep total_results
    "total_results": 3,

$ cf curl /v3/users | grep total_results
    "total_results": 4,
  • confirmed that permissions for each role are correct, results are identical for all roles except OrgAuditor, which receives a 404. This matches the error code received by OrgAuditors when curling /v3/spaces/:guid.

Docs Check

Looks good, ran docs locally and they render as expected

app/fetchers/space_users_list_fetcher.rb Outdated Show resolved Hide resolved
@sweinstein22 sweinstein22 linked an issue Sep 3, 2021 that may be closed by this pull request
* filtering users associated with the space beforehand better matches
our existing patterns and allows us to use existing fetchers for filtering.
* introduced a SpaceRoles model backed by a union of role tables
associated with spaces. We believe this will help us refactor out some repeated code in the future.

[#179048322]
Copy link
Contributor

@sweinstein22 sweinstein22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running through acceptance again as the implementation changed pretty significantly.

Behavior Check

As a space supporter, confirmed that the list of users returned for a space is a subset of those returned from /v3/users:

$ cf curl /v3/spaces/369cc547-b6b8-4ccb-a3c3-987845b04049/users | grep total_results
    "total_results": 6,
$ cf curl /v3/users | grep total_results
    "total_results": 8,

Also confirmed that output of endpoint looked correct, and then checked to make sure there wasn't a diff in output between roles.

$ diff space-supporter space-manager

$ diff space-supporter space-dev

$ diff space-supporter space-auditor

$ diff space-supporter org-manager

Confirmed that org auditors get a 404:

$ cf curl /v3/spaces/369cc547-b6b8-4ccb-a3c3-987845b04049/users -v
REQUEST: [2021-09-08T09:58:20-07:00]
GET /v3/spaces/369cc547-b6b8-4ccb-a3c3-987845b04049/users HTTP/1.1
...

RESPONSE: [2021-09-08T09:58:20-07:00]
HTTP/1.1 404 Not Found
...
{
  "errors": [
    {
      "code": 10010,
      "detail": "Space not found",
      "title": "CF-ResourceNotFound"
    }
  ]
}

Code Check

Looks good! Thanks for rearranging things so we would have more consistent patterns, and for introducing a new model to minimize how much data processing we're doing

Docs Check

New endpoint is documented, renders as expected

@sweinstein22 sweinstein22 merged commit 29272a0 into main Sep 8, 2021
@sweinstein22 sweinstein22 deleted the space-users-endpoint branch September 8, 2021 17:05
MarcPaquette pushed a commit that referenced this pull request Sep 15, 2021
* Provide pageable users of space endpoint

* /v3/spaces/:guid/users now exists with filters
* filtering users associated with the space beforehand better matches
our existing patterns and allows us to use existing fetchers for filtering.
* introduced a SpaceRoles model backed by a union of role tables
associated with spaces. We believe this will help us refactor out some repeated code in the future.

[#2489]

Co-authored-by: Carson Long <lcarson@vmware.com>
Co-authored-by: Mona Mohebbi <mmohebbi@vmware.com>
Co-authored-by: Michael Oleske <moleske@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No pageable way to gets users by space or org
4 participants