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

Hide review box if user cannot review access requests #49785

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

avatus
Copy link
Contributor

@avatus avatus commented Dec 4, 2024

oss counterpart for
https://github.com/gravitational/teleport.e/pull/5628

This adds some testing to the view as well as the equivalent to the web solution for Connect. Connect was missing the recently added ReviewRequests field in the user ACL, so I added it here.

Because this is handled in the tsh code, we don't have to worry about backward compatibility here for Connect right? If so, i could maybe do some default of true and only handle the false case but I don't think it's necessary

@avatus avatus requested review from ryanclark and gzdunek and removed request for kiosion and ryanclark December 4, 2024 19:53
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-49785.d3pp5qlev8mo18.amplifyapp.com

Comment on lines 55 to 59
describe('Request View', () => {
test('renders review box if user can review', async () => {
render(<RequestView {...sample} />);
expect(screen.getByTestId(reviewBoxId)).toBeInTheDocument();
});
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 component didn't have tests before. I added this to test the functionality in this PR but if you feel strongly, I'll add more tests to test the other functionally not related to this PR.

@gzdunek
Copy link
Contributor

gzdunek commented Dec 5, 2024

Because this is handled in the tsh code, we don't have to worry about backward compatibility here for Connect right? If so, i could maybe do some default of true and only handle the false case but I don't think it's necessary

From what I see, ACL.reviewRequests is calculated on the client side based on the user roles, so yeah we don't need to worry about backward compatibility.

@avatus avatus requested a review from gzdunek December 5, 2024 14:37
oss counterpart for
gravitational/teleport.e#5628

This adds some testing to the view as well as the equivalent to the web
solution for Connect. Connect was missing the [recently
added](#48536)
`ReviewRequests` field in the user ACL, so I added it here.

Because this is handled in the tsh code, we don't have to worry about
backward compatibility here for Connect right?
@avatus avatus force-pushed the avatus/review_request branch from 4b95a0d to 6ae11a3 Compare December 5, 2024 15:33
@avatus avatus enabled auto-merge December 5, 2024 15:33
Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

Sorry for a late review!

};

const props: RequestViewProps = {
user: 'loggedInUsername',
Copy link
Member

@ravicious ravicious Dec 9, 2024

Choose a reason for hiding this comment

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

FWIW, I'd put "Alice" here or whatever. When someone looks at this code for the first time, loggedInUsername might seem as if someone wanted to use a variable here.

Copy link
Contributor Author

@avatus avatus Dec 9, 2024

Choose a reason for hiding this comment

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

Thats a great idea. Just realized that auto-merge was on so I wasn't able to change this. However, I have another PR in a similar space coming up soon that I'll toss the change in (i'll skip it in the backports to not cause weird conflicts when I'm doing the next PR too)

@avatus avatus added this pull request to the merge queue Dec 9, 2024
Merged via the queue into master with commit b91395a Dec 9, 2024
44 checks passed
@avatus avatus deleted the avatus/review_request branch December 9, 2024 14:46
@public-teleport-github-review-bot

@avatus See the table below for backport results.

Branch Result
branch/v16 Failed
branch/v17 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