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

<EnvBuildStatus /> component #377

Merged
merged 9 commits into from
Mar 26, 2024
Merged

<EnvBuildStatus /> component #377

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7d72003
Add log link next to status message
gabalafou Feb 9, 2024
03b4c64
lint
gabalafou Mar 5, 2024
fde86ae
code cleanup
gabalafou Mar 5, 2024
7caa80f
lint
gabalafou Mar 5, 2024
30d7ffa
refactor buildMapper
gabalafou Mar 12, 2024
c03078f
more refaction
gabalafou Mar 14, 2024
8847ffb
better type annotation
gabalafou Mar 14, 2024
ed12797
PR review
gabalafou Mar 26, 2024
3d44501
fix type annotation
gabalafou Mar 26, 2024
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
19 changes: 11 additions & 8 deletions src/features/metadata/components/BuildDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,25 @@ import ArrowDropDownIcon from "@mui/icons-material/ArrowDropDown";
import IconButton from "@mui/material/IconButton";
import { useAppDispatch } from "../../../hooks";
import { currentBuildIdChanged } from "..";
import { Build } from "../../../common/models";
import { buildDatetimeStatus } from "../../../utils/helpers/buildMapper";

interface IBuildProps {
/**
* @param builds list of builds
* @param currentBuildId id of the current build
* @param onChangeStatus update the build status
* @param selectedBuildId id of the build selected from the dropdown
*/
builds: {
id: number;
name: string;
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved
status: string;
}[];
builds: Build[];
currentBuildId: number;
selectedBuildId: number;
}

export const Build = ({ builds, selectedBuildId }: IBuildProps) => {
export const BuildDropdown = ({
builds,
currentBuildId,
selectedBuildId
}: IBuildProps) => {
const dispatch = useAppDispatch();
const { palette } = useTheme();
const [open, setOpen] = useState(false);
Expand Down Expand Up @@ -92,7 +95,7 @@ export const Build = ({ builds, selectedBuildId }: IBuildProps) => {
{builds
? builds.map(build => (
<MenuItem key={build.id} value={build.id}>
{build.name}
{buildDatetimeStatus(build, currentBuildId)}
</MenuItem>
))
: null}
Expand Down
74 changes: 74 additions & 0 deletions src/features/metadata/components/EnvBuildStatus.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import React from "react";
import { CircularProgress, Typography } from "@mui/material";
import Link from "@mui/material/Link";
import OpenInNewIcon from "@mui/icons-material/OpenInNew";
import { Artifact, Build } from "../../../common/models";
import { PrefContext } from "../../../preferences";
import { StyledMetadataItem } from "../../../styles/StyledMetadataItem";
import artifactList from "../../../utils/helpers/artifact";
import { artifactBaseUrl } from "../../../utils/helpers/parseArtifactList";
import { buildStatus } from "../../../utils/helpers/buildMapper";

const LogLink = ({ logArtifact }: { logArtifact: Artifact }) => {
const pref = React.useContext(PrefContext);
const url = new URL(
logArtifact.route,
artifactBaseUrl(pref.apiUrl, window.location.origin)
);
return (
<Link
href={url.toString()}
target="_blank"
sx={{
display: "inline-flex",
verticalAlign: "bottom", // align link (icon plus link text) with non-link text on the same line
alignItems: "center" // align icon and text within link
}}
>
<OpenInNewIcon sx={{ mr: 0.5 }} fontSize="inherit" />
Log
</Link>
);
};

export interface IEnvBuildStatusProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the usual protocol around exporting these one-off interfaces? I know some projects do so and others don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this isn't needed. It was needed by Storybook when I was importing this component into Storybook, but then I decided not to create a story for this component.

Suggested change
export interface IEnvBuildStatusProps {
interface IEnvBuildStatusProps {

build: Build;
}

export const EnvBuildStatus = ({ build }: IEnvBuildStatusProps) => {
const logArtifact: Artifact | never = artifactList(build.id, ["LOGS"])[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked it out and this seems good, but I can't get over how weird this looks because indexing outside an array's length returns an undefined:

const foo = [1, 2, 3]
foo[8] // Returns undefined

But from what I can tell typescript infers the type for [] to be never[] which is just strange. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No you're right. My type annotation was wrong. Fixed.


return (
<StyledMetadataItem
sx={{
marginTop: "0",
fontSize: "13px",
fontWeight: 500,
paddingBottom: "0"
}}
data-testid="build-status"
>
Status: {""}
<Typography component="span" sx={{ fontSize: "13px" }}>
{buildStatus(build)}
{build.status_info && ` (${build.status_info})`}
{build.status === "BUILDING" || build.status === "QUEUED" ? (
<CircularProgress
size={10}
sx={{
marginLeft: "8px"
}}
/>
) : (
// If the selected build is a failed build, render the link to the build log.
build.status === "FAILED" &&
logArtifact && (
<>
. <LogLink logArtifact={logArtifact} />
</>
)
)}
</Typography>
</StyledMetadataItem>
);
};
79 changes: 11 additions & 68 deletions src/features/metadata/components/EnvBuilds.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,9 @@
import React from "react";
import { CircularProgress, Typography } from "@mui/material";
import { CircularProgress } from "@mui/material";
import { StyledMetadataItem } from "../../../styles/StyledMetadataItem";
import { Build as IBuild } from "../../../common/models";
import { Build } from "../../../features/metadata/components";
import { buildMapper } from "../../../utils/helpers/buildMapper";
import Link from "@mui/material/Link";
import OpenInNewIcon from "@mui/icons-material/OpenInNew";
import { artifactBaseUrl } from "../../../utils/helpers";
import { PrefContext } from "../../../preferences";
import artifactList from "../../../utils/helpers/artifact";
import { Artifact } from "../../../common/models";
import { BuildDropdown } from "../../../features/metadata/components";
import { EnvBuildStatus } from "./EnvBuildStatus";

export interface IData {
currentBuildId: number;
Expand All @@ -24,37 +18,7 @@ export const EnvBuilds = ({
builds,
mode
}: IData) => {
const envBuilds = builds.length ? buildMapper(builds, currentBuildId) : [];
const currentBuild = envBuilds.find(build => build.id === selectedBuildId);

// If the selected build is a failed build, we will render the link to the build log.
let logLink;
const showLogLink = currentBuild?.status === "Failed";
const logArtifact: Artifact | never = artifactList(currentBuild?.id, [
"LOGS"
])[0];
if (showLogLink && logArtifact) {
const pref = React.useContext(PrefContext);
const url = new URL(
logArtifact.route,
artifactBaseUrl(pref.apiUrl, window.location.origin)
);
logLink = (
<Link
href={url.toString()}
target="_blank"
sx={{
display: "inline-flex",
verticalAlign: "bottom", // align link (icon plus link text) with non-link text on the same line
alignItems: "center" // align icon and text within link
}}
>
<OpenInNewIcon sx={{ mr: 0.5 }} fontSize="inherit" />
Log
</Link>
);
}

const selectedBuild = builds.find(build => build.id === selectedBuildId);
return (
<>
<StyledMetadataItem
Expand All @@ -65,35 +29,14 @@ export const EnvBuilds = ({
>
{mode === "edit" ? "Change active environment version:" : "Builds:"}
</StyledMetadataItem>
{currentBuild ? (
{selectedBuild ? (
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved
<>
<Build builds={envBuilds} selectedBuildId={selectedBuildId} />
<StyledMetadataItem
sx={{
marginTop: "0",
fontSize: "13px",
fontWeight: 500,
paddingBottom: "0"
}}
data-testid="build-status"
>
Status: {""}
<Typography component="span" sx={{ fontSize: "13px" }}>
{currentBuild.status}
{currentBuild.status_info && ` (${currentBuild.status_info})`}
{((currentBuild.status === "Building" ||
currentBuild.status === "Queued") && (
<CircularProgress
size={10}
sx={{
marginLeft: "8px"
}}
/>
)) ||
// If the selected build is a failed build, render the link to the build log.
(showLogLink && <>. {logLink}</>)}
</Typography>
</StyledMetadataItem>
<BuildDropdown
builds={builds}
currentBuildId={currentBuildId}
selectedBuildId={selectedBuildId}
/>
<EnvBuildStatus build={selectedBuild} />
</>
) : (
<CircularProgress
Expand Down
1 change: 1 addition & 0 deletions src/features/metadata/components/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ export * from "./EnvMetadata";
export * from "./BuildDropdown";
export * from "./Description";
export * from "./EnvBuilds";
export * from "./EnvBuildStatus";
97 changes: 41 additions & 56 deletions src/utils/helpers/buildMapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { format, utcToZonedTime } from "date-fns-tz";
import { Build } from "../../common/models";

const STATUS_OPTIONS: any = {
const STATUS_OPTIONS: { [key: Build["status"]]: string } = {
COMPLETED: "Available",
QUEUED: "Queued",
FAILED: "Failed",
Expand All @@ -10,17 +10,19 @@ const STATUS_OPTIONS: any = {

const TIMEZONE = Intl.DateTimeFormat().resolvedOptions().timeZone;

const isBuilding = (status: string) => {
const BUILD_STATUS = ["BUILDING"];
return BUILD_STATUS.includes(status);
};

const isQueued = (status: string) => {
const BUILD_STATUS = ["QUEUED"];
return BUILD_STATUS.includes(status);
};
const isCompleted = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is just a matter of preference but based just on this function's name I'd expect this to return a bool.

One other thing: this is only being called in buildStatus below, do we need to break this out into a separate function? You're already checking the value of status there, so this would be a bit shorter if you weren't checking it again here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'm fine with that, I'll update in a subsequent commit

status,
ended_on,
scheduled_on
}: Pick<Build, "status" | "ended_on" | "scheduled_on">) => {
let duration = 0;
if (ended_on && scheduled_on) {
const startTime = new Date(scheduled_on);
const endTime = new Date(ended_on);
duration = (endTime.valueOf() - startTime.valueOf()) / 60000;
duration = Math.round(duration);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you really only need to do this if status === "COMPLETED", so this can be moved inside the branch below.


const isCompleted = (status: string, duration: number) => {
if (status === "COMPLETED") {
if (duration > 0) {
return `Completed in ${duration} min`;
Expand All @@ -40,51 +42,34 @@ const dateToTimezone = (date: string) => {
});
};

export const buildMapper = (data: Build[], currentBuildId: number) => {
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved
return data.map(
({ id, status, status_info, ended_on, scheduled_on }: Build) => {
let duration = 0;
if (ended_on && scheduled_on) {
const startTime = new Date(scheduled_on);
const endTime = new Date(ended_on);
duration = (endTime.valueOf() - startTime.valueOf()) / 60000;
duration = Math.round(duration);
}
if (id === currentBuildId) {
return {
id,
name: `${dateToTimezone(ended_on ?? scheduled_on)} - Active`,
status: isCompleted(status, duration),
status_info
};
}

if (isBuilding(status)) {
return {
id,
name: `${dateToTimezone(scheduled_on)} - Building`,
status: "Building",
status_info
};
}

if (isQueued(status)) {
return {
id,
name: `${dateToTimezone(scheduled_on)} - Queued`,
status: "Building",
status_info
};
}
export const buildDatetimeStatus = (
{ id, status, ended_on, scheduled_on }: Build,
currentBuildId: number
): string => {
if (id === currentBuildId) {
return `${dateToTimezone(ended_on ?? scheduled_on)} - Active`;
} else if (status === "BUILDING") {
return `${dateToTimezone(scheduled_on)} - Building`;
} else if (status === "QUEUED") {
return `${dateToTimezone(scheduled_on)} - Queued`;
} else {
return `${dateToTimezone(ended_on ?? scheduled_on)} - ${
STATUS_OPTIONS[status]
}`;
}
};

return {
id,
name: `${dateToTimezone(ended_on ?? scheduled_on)} - ${
STATUS_OPTIONS[status]
}`,
status: isCompleted(status, duration),
status_info
};
export const buildStatus = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to combine the BUILDING and QUEUED statuses? It feels like it would be useful to have these distinct states represented distinctly in the UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your suggestion makes sense but I'm a little afraid of it breaking something.

I also thought that maybe it had been a product decision to collapse those statuses for the end user. But that's not stated in the PR where it was introduced.

I'll ask about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update. I asked @kcpevey on Slack and she said it looks like a mistake, so I've changed it and we'll see if anybody complains or it breaks anything.

status,
ended_on,
scheduled_on
}: Build): string => {
switch (status) {
case "BUILDING":
case "QUEUED":
return "Building";
default: {
return isCompleted({ status, ended_on, scheduled_on });
}
);
}
};
Loading
Loading