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

Add partial search by usernames to GET /organizations/:guid/users #2920

Merged
merged 2 commits into from
Aug 31, 2022

Conversation

a-b
Copy link
Member

@a-b a-b commented Aug 10, 2022

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:

Add partial search by usernames to GET /organizations/:guid/users

For partial_usernames parameter, we are using UAA /Users endpoint that requires scim.read scope.

An explanation of the use cases your change solves

This will let us to find usernames by substring e.g. GET /organizations/:guid/users?partial_usernames=foo

Links to any other associated PRs

CF-D PR

  • 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

Coauthored by @jdgonzaleza

For partial_usernames parameter we are using UAA /Users endpoint that
require `scim.read` scope. This change is reflected in CF-D
[PR](cloudfoundry/cf-deployment#984)

Co-authored-by: Juan Diego Gonzalez <[email protected]>
Co-authored-by: Alexander Berezovsky <[email protected]>
lib/cloud_controller/uaa/uaa_client.rb Outdated Show resolved Hide resolved
if precise_username_match
results = query(:user_id, includeInactive: true, filter: filter_string)
else
results = query(:user, filter: filter_string, attributes: 'id')
Copy link
Member

Choose a reason for hiding this comment

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

So for partial usernames, only active ones will be returned, right?

app/controllers/v3/organizations_controller.rb Outdated Show resolved Hide resolved
spec/unit/messages/users_list_message_spec.rb Show resolved Hide resolved
expect(message.partial_usernames).to eq(%w[bob])
end
end

context 'guids, usernames, origins are nil' do
let(:params) do
{
Copy link
Member

Choose a reason for hiding this comment

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

The context 'guids, usernames, origins must be arrays' should not be underneath context 'guids, usernames, origins are nil'.

Choose a reason for hiding this comment

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

We can move the test

spec/unit/messages/users_list_message_spec.rb Show resolved Hide resolved
spec/unit/messages/users_list_message_spec.rb Show resolved Hide resolved
spec/unit/messages/users_list_message_spec.rb Show resolved Hide resolved
spec/request/organizations_spec.rb Outdated Show resolved Hide resolved
spec/request/organizations_spec.rb Outdated Show resolved Hide resolved
@pivotalgeorge pivotalgeorge force-pushed the feature/users-partial-search branch from a8c4168 to ec28a76 Compare August 16, 2022 20:05
@a-b a-b force-pushed the feature/users-partial-search branch from ec28a76 to 92242e8 Compare August 16, 2022 20:30
@dalvarado dalvarado self-requested a review August 16, 2022 21:43
Copy link
Contributor

@dalvarado dalvarado left a comment

Choose a reason for hiding this comment

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

Looks good to @MerricdeLauney and me. We'll merge once you finalize the open discussions with Philip.

docs/v3/package-lock.json Outdated Show resolved Hide resolved
@jdgonzaleza jdgonzaleza force-pushed the feature/users-partial-search branch from 92242e8 to 51fbbc4 Compare August 17, 2022 15:34
@philippthun
Copy link
Member

@jdgonzaleza I think you deleted most of the docs/v3/package-lock.json file now instead of reverting the small change you did before. Otherwise it looks good to me.

Co-authored-by: Juan Diego Gonzalez <[email protected]>
Co-authored-by: Alexander Berezovsky <[email protected]>
@jdgonzaleza jdgonzaleza force-pushed the feature/users-partial-search branch from 51fbbc4 to 4580e86 Compare August 17, 2022 15:51
@a-b
Copy link
Member Author

a-b commented Aug 24, 2022

Are there any other pending implementation issues blocking us from merging this PR?

@a-b a-b requested review from dalvarado, jdgonzaleza and moleske and removed request for ctlong August 24, 2022 23:37
Copy link
Contributor

@pivotalgeorge pivotalgeorge left a comment

Choose a reason for hiding this comment

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

the new filter parameter works when retrieving paginated user results in apps-manager

@a-b a-b requested review from jdgonzaleza and removed request for MerricdeLauney and jdgonzaleza August 26, 2022 17:17
Copy link

@jdgonzaleza jdgonzaleza left a comment

Choose a reason for hiding this comment

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

LGTM

@MerricdeLauney MerricdeLauney merged commit 9eb0f87 into main Aug 31, 2022
@MerricdeLauney MerricdeLauney deleted the feature/users-partial-search branch August 31, 2022 18:42
fhambrec pushed a commit to ZPascal/cloud_controller_ng that referenced this pull request Oct 20, 2022
…oudfoundry#2920)

* Add partial search by usernames to GET /organizations/:guid/users

For partial_usernames parameter we are using UAA /Users endpoint that
require `scim.read` scope. This change is reflected in CF-D
[PR](cloudfoundry/cf-deployment#984)

Co-authored-by: Alexander Berezovsky <[email protected]>
Co-authored-by: Juan Diego Gonzalez <[email protected]>
will-gant pushed a commit to sap-contributions/cloud_controller_ng that referenced this pull request Dec 16, 2022
…oudfoundry#2920)

* Add partial search by usernames to GET /organizations/:guid/users

For partial_usernames parameter we are using UAA /Users endpoint that
require `scim.read` scope. This change is reflected in CF-D
[PR](cloudfoundry/cf-deployment#984)

Co-authored-by: Alexander Berezovsky <[email protected]>
Co-authored-by: Juan Diego Gonzalez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants