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

Juju 7074/identities filter #1434

Merged
merged 7 commits into from
Nov 15, 2024

Conversation

SimoneDutto
Copy link
Contributor

Description

This pr adds filtering for rebac listIdentities on group name/id.

Address #1421

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

Notes for code reviewers

@SimoneDutto SimoneDutto requested a review from a team as a code owner November 13, 2024 13:53
internal/testutils/jimmtest/jimm_mock.go Outdated Show resolved Hide resolved
internal/jimmhttp/rebac_admin/identities_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ale8k ale8k left a comment

Choose a reason for hiding this comment

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

I need to request changes so we can remove the space but ping me once done and I'll approve (justh ave a think about my other comment)

const op = errors.Op("jimm.ListIdentities")

if !user.JimmAdmin {
return nil, errors.E(op, errors.CodeUnauthorized, "unauthorized")
}
identities, err := j.Database.ListIdentities(ctx, pagination.Limit(), pagination.Offset(), match)
var users []openfga.User
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel like I'm reading that book aha. Love it.

limit: 5,
offset: 0,
identities: []string{userNames[0]},
match: userNames[0][:5],
Copy link
Contributor

Choose a reason for hiding this comment

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

As we're reating another slice anyway rather than just [:1], either do: [:1] on 117 or hardcode the string in please.

I'm usually of the preference of hardcoding strings in tests as I feel like it makes them more readable rather than bob1->5. Also as this is a match, doing [0] and [:5] against a string is less readable (imo) than just getting "bob1".

Copy link
Contributor

@ale8k ale8k left a comment

Choose a reason for hiding this comment

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

lgtm

@SimoneDutto SimoneDutto added this pull request to the merge queue Nov 15, 2024
Merged via the queue into canonical:v3 with commit a2562bc Nov 15, 2024
4 checks passed
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.

4 participants