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

Add log link next to status message #367

Merged
merged 4 commits into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 2 additions & 4 deletions src/features/artifacts/components/ArtifactsItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { useTheme } from "@mui/material/styles";

import { Artifact } from "../../../common/models";
import { PrefContext } from "../../../preferences";
import { isPathAbsolute } from "../../../utils/helpers";
import { artifactBaseUrl } from "../../../utils/helpers";

interface IArtifactsProps {
/**
Expand All @@ -17,9 +17,7 @@ interface IArtifactsProps {

export const ArtifactItem = ({ artifact }: IArtifactsProps) => {
const pref = React.useContext(PrefContext);
const url = isPathAbsolute(pref.apiUrl)
? pref.apiUrl
: `${window.location.origin}${pref.apiUrl}`;
const url = artifactBaseUrl(pref.apiUrl, window.location.origin);
gabalafou marked this conversation as resolved.
Show resolved Hide resolved
const route = new URL(artifact.route, url).toString();
const theme = useTheme();

Expand Down
57 changes: 44 additions & 13 deletions src/features/metadata/components/EnvBuilds.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,14 @@ 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";

interface IData {
export interface IData {
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved
currentBuildId: number;
selectedBuildId: number;
builds: IBuild[];
Expand All @@ -21,6 +27,34 @@ export const EnvBuilds = ({
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>
);
}

return (
<>
<StyledMetadataItem
Expand All @@ -31,7 +65,7 @@ export const EnvBuilds = ({
>
{mode === "edit" ? "Change active environment version:" : "Builds:"}
</StyledMetadataItem>
{currentBuild && (
{currentBuild ? (
<>
<Build builds={envBuilds} selectedBuildId={selectedBuildId} />
<StyledMetadataItem
Expand All @@ -41,30 +75,27 @@ export const EnvBuilds = ({
fontWeight: 500,
paddingBottom: "0"
}}
data-testid="build-status"
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved
>
Status: {""}
<Typography component="span" sx={{ fontSize: "13px" }}>
{currentBuild.status_info ? (
<>
{currentBuild.status} ({currentBuild.status_info})
</>
) : (
<>{currentBuild.status}</>
)}
{(currentBuild.status === "Building" ||
{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>
</>
)}
{!currentBuild && (
) : (
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, how come this is needed? Are there cases where currentBuild is undefined? Looking through buildMapper it seems like the currentBuildId should always have a corresponding item in builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... that's a good question. I don't know... Pagination maybe?

It doesn't help that the variable currentBuild does not actually correspond the currentBuildId build, see line 28 above:

  const currentBuild = envBuilds.find(build => build.id === selectedBuildId);

So maybe there's some way you can select a build id that isn't in the builds for that environment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have the context to know whether an additional refactor here is appropriate, but you might. If you feel like this could be cleaner, please make an issue for it. In the meantime, let's get this merged.

<CircularProgress
size={20}
sx={{ marginLeft: "15px", marginTop: "6px", marginBottom: "7px" }}
Expand Down
1 change: 1 addition & 0 deletions src/features/metadata/mocks/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./mockBuilds";
30 changes: 30 additions & 0 deletions src/features/metadata/mocks/mockBuilds.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import type { Build, CondaSpecification } from "../../../common/models";

function createBuild(id: number, status: string) {
return {
id,
environment_id: 1,
specification: {
id: 1,
name: "test",
spec: {} as CondaSpecification,
sha256:
"697434666a1c747d80df95189ad5750c1496287871779d2a2919db6cb768a182",
created_on: "2022-11-08T14:28:05.655564"
},
packages: [],
status,
status_info: null,
size: 0,
scheduled_on: "2022-11-08T14:28:05.655564",
started_on: "2022-11-08T14:28:05.655564",
ended_on: "2022-11-08T14:28:05.655564",
build_artifacts: []
};
}

const completedBuild = createBuild(1, "COMPLETED");
const failedBuild = createBuild(2, "FAILED");
const buildingBuild = createBuild(3, "BUILDING");

export const mockBuilds: Build[] = [completedBuild, failedBuild, buildingBuild];
27 changes: 27 additions & 0 deletions src/features/metadata/stories/EnvBuilds.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import type { Story, ComponentMeta } from "@storybook/react";
import { Provider } from "react-redux";
import React from "react";

import { store } from "../../../store";
import { EnvBuilds } from "../components";
import { mockBuilds } from "../mocks";

export default {
component: EnvBuilds
} as ComponentMeta<typeof EnvBuilds>;

export const Primary = {
args: {
currentBuildId: 1,
selectedBuildId: 1,
mode: "read-only",
builds: mockBuilds
},
decorators: [
(Story: Story) => (
<Provider store={store}>
<Story />
</Provider>
)
]
};
10 changes: 9 additions & 1 deletion src/utils/helpers/parseArtifactList.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ export const parseArtifacts = (artifact_list: string[] | undefined) => {
});
};

export const isPathAbsolute = (path: string) => {
const isPathAbsolute = (path: string) => {
return new RegExp("^(?:[a-z]+:)?//", "i").test(path);
};

export const artifactBaseUrl = (apiUrl: string, baseUrl: string) => {
if (isPathAbsolute(apiUrl)) {
return apiUrl;
} else {
return `${baseUrl}${apiUrl}`;
}
};
49 changes: 47 additions & 2 deletions test/metadata/EnvBuilds.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ describe("<EnvBuilds />", () => {
it("should render component", () => {
const component = render(
<Provider store={store}>
<EnvBuilds selectedBuildId={1} builds={[BUILD]} />
<EnvBuilds
currentBuildId={1}
selectedBuildId={1}
builds={[BUILD]}
mode="read-only"
/>
</Provider>
);
expect(component.container).toHaveTextContent("Builds");
Expand All @@ -18,10 +23,50 @@ describe("<EnvBuilds />", () => {
it("should show a progress bar if builds are not available", () => {
const component = render(
<Provider store={store}>
<EnvBuilds selectedBuildId={1} builds={[]} />
<EnvBuilds
currentBuildId={1}
selectedBuildId={1}
builds={[]}
mode="read-only"
/>
</Provider>
);
const progressBar = component.getByRole("progressbar");
expect(progressBar).toBeInTheDocument();
});

it("should render link to log if selected build failed", () => {
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved
const failedBuild = { ...BUILD, status: "FAILED" };
const { getByTestId, getByRole } = render(
<Provider store={store}>
<EnvBuilds
currentBuildId={1}
selectedBuildId={1}
builds={[failedBuild]}
mode="read-only"
/>
</Provider>
);
expect(getByRole("link", { name: "Log" })).toBeInTheDocument();
expect(getByTestId("build-status")).toHaveTextContent(
/^Status: Failed\. Log$/
);
});

it("should not render log link for normal build", () => {
const { getByTestId, queryByRole } = render(
<Provider store={store}>
<EnvBuilds
currentBuildId={1}
selectedBuildId={1}
builds={[BUILD]}
mode="read-only"
/>
</Provider>
);
expect(queryByRole("link", { name: "Log" })).not.toBeInTheDocument();
expect(getByTestId("build-status")).toHaveTextContent(
/^Status: Completed$/
);
});
});
3 changes: 2 additions & 1 deletion test/testutils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ export const BUILD = {
scheduled_on: "2022-11-08T14:28:05.655564",
started_on: "2022-11-08T14:28:05.655564",
ended_on: "2022-11-08T14:28:05.655564",
build_artifacts: []
build_artifacts: [],
status_info: null
};

export const mockTheme = (children: any) => {
Expand Down
Loading