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

Connect: fix access request searching logic #43159

Merged
merged 2 commits into from
Jun 24, 2024
Merged

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Jun 18, 2024

using the search bar in the list of access requests wasn't working and threw errors about accessing invalid fields.

the same web UI's matcher was updated about a year ago but of course teleterm got missed (😅 ). moved it into a shared file.

moved from here: https://github.com/gravitational/teleport.e/pull/4429

changelog: Fix input searching logic for Connect's access request listing view

@kimlisa kimlisa added backport/branch/v14 no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 backport/branch/v16 labels Jun 18, 2024
@github-actions github-actions bot requested review from avatus and gzdunek June 18, 2024 06:09
@kimlisa kimlisa requested review from ravicious and removed request for avatus June 18, 2024 06:11
@kimlisa
Copy link
Contributor Author

kimlisa commented Jun 18, 2024

should i include a changelog? no one complained for a year


if (propName === 'resources') {
return targetValue.some((r: any) =>
Object.keys(r).some(k => r[k].toUpperCase().includes(searchValue))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the issue

Copy link
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

should i include a changelog? no one complained for a year

I think we should still include it.

Thanks for the fix, in general we should switch this screen to the server-side filtering, but I don't require you to do it here :)


import { AccessRequest, Resource } from 'shared/services/accessRequests';

export function requestMatcher(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used only in teleterm, so I don't think we should keep this function in shared - we can fix the one we have in DocumentAccessRequests/RequestList/RequestList.tsx.
I would also add TODO about replacing it with server-side filtering.

Copy link
Contributor Author

@kimlisa kimlisa Jun 24, 2024

Choose a reason for hiding this comment

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

added TODO comment (keeping file in shared because of comment)

export function requestMatcher(
targetValue: any,
searchValue: string,
propName: keyof AccessRequest & string
Copy link
Contributor

Choose a reason for hiding this comment

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

& string is not needed.

@kimlisa kimlisa removed the no-changelog Indicates that a PR does not require a changelog entry label Jun 18, 2024

import { AccessRequest, Resource } from 'shared/services/accessRequests';

export function requestMatcher(
Copy link
Member

Choose a reason for hiding this comment

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

I tried to rewrite it to make it more safe, but I ultimately couldn't find a good solution. It'd have been trivial if requestMatcher received the whole AccessRequest object rather than the value of propName:

export function requestMatcher(
  accessRequest: AccessRequest,
  searchValue: string,
  propName: keyof AccessRequest
) {
  if (propName === 'roles') {
    return accessRequest[propName].some(role =>
      role.toUpperCase().includes(searchValue)
    );
  }

  if (propName === 'resources') {
    return accessRequest[propName].some(r =>
      Object.values(r.id)
        .concat(Object.values(r.details.hostname || {}))
        .concat(Object.values(r.details.friendlyName || {}))
        .some(v => v.toUpperCase().includes(searchValue))
    );
  }
}

@kimlisa kimlisa force-pushed the lisa/fix-teleterm-ar-search branch from 13b6f35 to 3066dca Compare June 24, 2024 19:16
@kimlisa kimlisa enabled auto-merge June 24, 2024 19:17
@kimlisa kimlisa added this pull request to the merge queue Jun 24, 2024
Merged via the queue into master with commit b40c3d4 Jun 24, 2024
38 checks passed
@kimlisa kimlisa deleted the lisa/fix-teleterm-ar-search branch June 24, 2024 19:34
@public-teleport-github-review-bot

@kimlisa See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Create PR
branch/v16 Create PR

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.

3 participants