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.

Sign up for GitHub

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

Web: rename generic variable name to a helpful name #47722

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Web: rename generic variable name to a helpful name #47722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...ges/shared/components/AccessRequests/NewRequest/RequestCheckout/RequestCheckout.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export const Empty = () => {
<MemoryRouter>
<RequestCheckoutWithSlider
{...baseProps}
data={[]}
pendingAccessRequests={[]}
selectedReviewers={selectedReviewers}
setSelectedReviewers={setSelectedReviewers}
maxDuration={maxDuration}
Expand Down Expand Up @@ -177,7 +177,7 @@ const baseProps: RequestCheckoutWithSliderProps = {
],
setSelectedReviewers: () => null,
createRequest: () => null,
data: [
pendingAccessRequests: [
{
kind: 'app',
name: 'app-name',
Expand Down
2 changes: 1 addition & 1 deletion ...ages/shared/components/AccessRequests/NewRequest/RequestCheckout/RequestCheckout.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ const props: RequestCheckoutWithSliderProps = {
selectedReviewers: [],
setSelectedReviewers: () => null,
createRequest: () => null,
data: [],
pendingAccessRequests: [],
clearAttempt: () => null,
onClose: () => null,
toggleResource: () => null,
Expand Down
13 changes: 8 additions & 5 deletions web/packages/shared/components/AccessRequests/NewRequest/RequestCheckout/RequestCheckout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export function RequestCheckout<T extends PendingListItem>({
setPendingRequestTtl,
pendingRequestTtlOptions,
dryRunResponse,
data,
pendingAccessRequests,
showClusterNameColumn,
createAttempt,
fetchResourceRequestRolesAttempt,
Expand Down Expand Up @@ -190,12 +190,14 @@ export function RequestCheckout<T extends PendingListItem>({
selectedResourceRequestRoles.length < 1;

const submitBtnDisabled =
data.length === 0 ||
pendingAccessRequests.length === 0 ||
createAttempt.status === 'processing' ||
isInvalidRoleSelection ||
fetchResourceRequestRolesAttempt.status === 'failed' ||
fetchResourceRequestRolesAttempt.status === 'processing';

const numPendingAccessRequests = pendingAccessRequests.length;

const DefaultHeader = () => {
return (
<Flex mb={3} alignItems="center">
Expand All @@ -208,7 +210,8 @@ export function RequestCheckout<T extends PendingListItem>({
/>
<Box>
<H2>
{data.length} {pluralize(data.length, 'Resource')} Selected
{numPendingAccessRequests}{' '}
{pluralize(numPendingAccessRequests, 'Resource')} Selected
</H2>
</Box>
</Flex>
Expand Down Expand Up @@ -254,7 +257,7 @@ export function RequestCheckout<T extends PendingListItem>({
<Alert kind="danger" children={createAttempt.statusText} />
)}
<StyledTable
data={data}
data={pendingAccessRequests}
columns={[
{
key: 'clusterName',
Expand Down Expand Up @@ -791,7 +794,7 @@ export type RequestCheckoutProps<T extends PendingListItem = PendingListItem> =
isResourceRequest: boolean;
requireReason: boolean;
selectedReviewers: ReviewerOption[];
data: T[];
pendingAccessRequests: T[];
showClusterNameColumn?: boolean;
createRequest: (req: CreateRequest) => void;
fetchStatus: 'loading' | 'loaded';
Expand Down
24 changes: 16 additions & 8 deletions web/packages/teleterm/src/ui/AccessRequestCheckout/AccessRequestCheckout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export function AccessRequestCheckout() {
fetchResourceRolesAttempt,
setSelectedResourceRequestRoles,
clearCreateAttempt,
data,
pendingAccessRequests,
shouldShowClusterNameColumn,
selectedReviewers,
setSelectedReviewers,
Expand All @@ -104,7 +104,7 @@ export function AccessRequestCheckout() {
onStartTimeChange,
} = useAccessRequestCheckout();

const isRoleRequest = data[0]?.kind === 'role';
const isRoleRequest = pendingAccessRequests[0]?.kind === 'role';
const transitionRef = useRef<HTMLDivElement>();

function closeCheckout() {
Expand All @@ -113,10 +113,15 @@ export function AccessRequestCheckout() {

// We should rather detect how much space we have,
// but for simplicity we only count items.
const moreToShow = Math.max(data.length - MAX_RESOURCES_IN_BAR_TO_SHOW, 0);
const moreToShow = Math.max(
pendingAccessRequests.length - MAX_RESOURCES_IN_BAR_TO_SHOW,
0
);
const numPendingAccessRequests = pendingAccessRequests.length;

return (
<>
{data.length > 0 && !isCollapsed() && (
{pendingAccessRequests.length > 0 && !isCollapsed() && (
<Box
px={3}
py={2}
Expand All @@ -134,12 +139,15 @@ export function AccessRequestCheckout() {
>
<Flex flexDirection="column" minWidth={0}>
<Text mb={1}>
{data.length}{' '}
{pluralize(data.length, isRoleRequest ? 'role' : 'resource')}{' '}
{numPendingAccessRequests}{' '}
{pluralize(
numPendingAccessRequests,
isRoleRequest ? 'role' : 'resource'
)}{' '}
added to access request:
</Text>
<Flex gap={1} flexWrap="wrap">
{data
{pendingAccessRequests
.slice(0, MAX_RESOURCES_IN_BAR_TO_SHOW)
.map(c => {
let resource = {
Expand Down Expand Up @@ -238,7 +246,7 @@ export function AccessRequestCheckout() {
})
}
reset={reset}
data={data}
pendingAccessRequests={pendingAccessRequests}
showClusterNameColumn={shouldShowClusterNameColumn}
createAttempt={createRequestAttempt}
resourceRequestRoles={resourceRequestRoles}
Expand Down
34 changes: 21 additions & 13 deletions web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Continuing on the topic started in my comment (#47347 (comment)), this is a good start and it addresses the point I had about data being a vague term. Long term we'd need to address things like
AccessRequestCheckout receiving unfiltered data and then filtering it again.

What I was getting onto in that comment is that there are concepts in the code waiting to be discovered and given names. Resorting to using terms like "data" means that the next person reading that code will have to figure out what's what by themselves.

I'm yet to use the actual UI you've built in those PRs myself, but from reading the code, it seems that there are some items within pendingAccessRequests that are meant to be used when rendered on screen and some that are meant to be used when communicating with the backend. If that's the case, then those items are good examples of two specific concepts for which we could find specific names. Sometimes prepending a variable name with an adjective (e.g. data and filteredData) can only do so much and we have invent new concepts to move forward.

Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export default function useAccessRequestCheckout() {
workspaceAccessRequest?.getPendingAccessRequest();

useEffect(() => {
// Do a new dry run per changes to pending data
// Do a new dry run per changes to pending access requests
// to get the latest time options and latest calculated
// suggested reviewers.
// Options and reviewers can change depending on the selected
Expand All @@ -106,12 +106,13 @@ export default function useAccessRequestCheckout() {
return;
}

const data = getPendingAccessRequestsPerResource(pendingAccessRequest);
const pendingAccessRequests =
getPendingAccessRequestsPerResource(pendingAccessRequest);
runFetchResourceRoles(() =>
retryWithRelogin(ctx, clusterUri, async () => {
const { response } = await ctx.tshd.getRequestableRoles({
clusterUri: rootClusterUri,
resourceIds: data
resourceIds: pendingAccessRequests
.filter(d => d.kind !== 'role')
.map(d => ({
// We have to use id, not name.
Expand Down Expand Up @@ -148,17 +149,17 @@ export default function useAccessRequestCheckout() {
function getPendingAccessRequestsPerResource(
pendingRequest: PendingAccessRequest
): PendingListItemWithOriginalItem[] {
const data: PendingListItemWithOriginalItem[] = [];
const pendingAccessRequests: PendingListItemWithOriginalItem[] = [];
if (!workspaceAccessRequest) {
return data;
return pendingAccessRequests;
}

switch (pendingRequest.kind) {
case 'role': {
const clusterName =
ctx.clustersService.findCluster(rootClusterUri)?.name;
pendingRequest.roles.forEach(role => {
data.push({
pendingAccessRequests.push({
kind: 'role',
id: role,
name: role,
Expand All @@ -171,7 +172,7 @@ export default function useAccessRequestCheckout() {
pendingRequest.resources.forEach(resourceRequest => {
const { kind, id, name } =
extractResourceRequestProperties(resourceRequest);
data.push({
pendingAccessRequests.push({
kind,
id,
name,
Expand All @@ -183,7 +184,7 @@ export default function useAccessRequestCheckout() {
});
}
}
return data;
return pendingAccessRequests;
}

function isCollapsed() {
Expand Down Expand Up @@ -221,21 +222,24 @@ export default function useAccessRequestCheckout() {
* Shared logic used both during dry runs and regular access request creation.
*/
function prepareAndCreateRequest(req: CreateRequest) {
const data = getPendingAccessRequestsPerResource(pendingAccessRequest);
const pendingAccessRequests =
getPendingAccessRequestsPerResource(pendingAccessRequest);
const params: CreateAccessRequestRequest = {
rootClusterUri,
reason: req.reason,
suggestedReviewers: req.suggestedReviewers || [],
dryRun: req.dryRun,
resourceIds: data
resourceIds: pendingAccessRequests
.filter(d => d.kind !== 'role')
.map(d => ({
name: d.id,
clusterName: d.clusterName,
kind: d.kind,
subResourceName: '',
})),
roles: data.filter(d => d.kind === 'role').map(d => d.name),
roles: pendingAccessRequests
.filter(d => d.kind === 'role')
.map(d => d.name),
assumeStartTime: req.start && Timestamp.fromDate(req.start),
maxDuration: req.maxDuration && Timestamp.fromDate(req.maxDuration),
requestTtl: req.requestTTL && Timestamp.fromDate(req.requestTTL),
Expand All @@ -250,7 +254,10 @@ export default function useAccessRequestCheckout() {

return retryWithRelogin(ctx, clusterUri, () =>
ctx.clustersService.createAccessRequest(params).then(({ response }) => {
return { accessRequest: response.request, requestedCount: data.length };
return {
accessRequest: response.request,
requestedCount: pendingAccessRequests.length,
};
})
).catch(e => {
setCreateRequestAttempt({ status: 'failed', statusText: e.message });
Expand Down Expand Up @@ -337,7 +344,8 @@ export default function useAccessRequestCheckout() {
isCollapsed,
assumedRequests: getAssumedRequests(),
toggleResource,
data: getPendingAccessRequestsPerResource(pendingAccessRequest),
pendingAccessRequests:
getPendingAccessRequestsPerResource(pendingAccessRequest),
shouldShowClusterNameColumn,
createRequest,
reset,
Expand Down
Loading