-
Notifications
You must be signed in to change notification settings - Fork 125
feat(access-tokens): add filtering by scope/user and track creator #7430
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @adambenhassen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the management of access tokens by introducing robust filtering options and improving auditability. Users can now easily locate specific tokens by filtering based on their scope (organization, project, or personal) and, for personal tokens, by the creator. The addition of a 'Created By' field provides crucial context for each token, making it easier to track and manage access within the organization. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📚 Storybook DeploymentThe latest changes are available as preview in: https://pr-7430.hive-storybook.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces filtering for access tokens by scope and user, and adds tracking for token creators. The changes are well-implemented across the backend, database, and frontend, and include a comprehensive set of integration tests. My review includes suggestions to reduce code duplication in tests, align with backend architectural guidelines, and a minor code simplification.
packages/services/api/src/modules/organization/resolvers/OrganizationAccessToken.ts
Show resolved
Hide resolved
packages/services/api/src/modules/organization/resolvers/Organization.ts
Show resolved
Hide resolved
packages/services/api/src/modules/organization/resolvers/PersonalAccessToken.ts
Show resolved
Hide resolved
packages/services/api/src/modules/organization/resolvers/ProjectAccessToken.ts
Show resolved
Hide resolved
💻 Website PreviewThe latest changes are available as preview in: https://pr-7430.hive-landing-page.pages.dev |
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the AccessToken interface have createdBy added as well?
| /** | ||
| * Whether only access tokens on the organization scope should be included in the result. | ||
| * @deprecated Use `filter: { scopes: ['ORGANIZATION'] }` instead. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice callout
|
|
||
| const AccessTokensTable_MoreAccessTokensQuery = graphql(` | ||
| query AccessTokensTable_MoreAccessTokensQuery($organizationSlug: String!, $after: String) { | ||
| query AccessTokensTable_MoreAccessTokensQuery( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was broken even before this change, but loading more doesn't work. I tested locally w/11 tokens.
| description: args.description, | ||
| assignedResources, | ||
| permissions, | ||
| createdByUserId: actor.type === 'user' ? actor.user.id : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be outside the scope of this ticket, but here's something to consider:
I keep encountering these situations where we rely on the author's ID to show who created something.
I wonder if the token were created using another token, should we store that token's "createdByUserId" here? Or maybe in the future we store a "createdByToken"?
For personal access tokens, I feel like storing the userId is appropriate, but for org or project scoped tokens it makes less sense.
| checked={validatedScopeFilter.includes(scope)} | ||
| onCheckedChange={() => handleScopeToggle(scope)} | ||
| > | ||
| {scopeLabels[scope]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For multi-selects I think it's important to keep the menu visible on click to allow users to select multiple without having to reopen.
There's a similar filter for the status on schema proposals for reference. In terms of style -- I'm find with either design for the menu/checkboxes, but I think we should be consistent between pages. So perhaps someone else can weigh in on the style.
| </Button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent align="start"> | ||
| <DropdownMenuLabel>Filter by scope</DropdownMenuLabel> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these labels are redundant with the button text. My preference is to remove the label.
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
|
|
||
| {/* User Filter Dropdown (only show if there are users) */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was exploring using a user filter for proposals as well -- you should be able to use it for this menu if you'd like. It includes search functionality so that pagination is less of an issue packages/web/app/src/components/target/proposals/user-filter.tsx
I had intended it to also allow multiple users to be selected though, so your userId filter would need to be an array or this behavior would need tweaked.
If you want to keep this component, then please move it to its own component file to keep this page a bit more concise.
| export function AccessTokensSubPage(props: AccessTokensSubPageProps): React.ReactNode { | ||
| // URL-based filter state | ||
| const [scopeFilter, setScopeFilter] = useSearchParamsFilter<string[]>('scope', []); | ||
| const [userFilter, setUserFilter] = useSearchParamsFilter<string>('user', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you're tracking filters in the url.
| <Table.TableCell> | ||
| {'createdBy' in edge.node && edge.node.createdBy ? ( | ||
| <span className="text-sm"> | ||
| {edge.node.createdBy.displayName || edge.node.createdBy.email} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fallback to the fullName before showing just the email?
| }; | ||
|
|
||
| const activeFilterCount = | ||
| (validatedScopeFilter.length > 0 && validatedScopeFilter.length < 3 ? 1 : 0) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I like that you show the filter count but it's a little odd when selecting the 3rd and the count disappears. I think I'd prefer if the count says 3 even though the filter itself is equivalent to not having a selection.
Closes #7239
Description
Adds filtering capability to the organization access tokens page and tracks who created each token.
Features