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

fix: Add message explaining that build logs are deleted after 180 days. #4402

Merged
merged 1 commit into from
Mar 12, 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
37 changes: 17 additions & 20 deletions frontend/components/site/CommitSummary.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import React, { useState, useEffect } from 'react';
import React from 'react';
import PropTypes from 'prop-types';

import api from '../../util/federalistApi';
import { IconBranch } from '../icons';
import LoadingIndicator from '../LoadingIndicator';
import { timeFrom, dateAndTime } from '../../util/datetime';
Expand All @@ -27,23 +26,8 @@ function buildShaLink(owner, repo, sha) {
);
}

function CommitSummary({ buildId }) {
const [build, setBuild] = useState({
isLoading: true,
buildDetails: null,
});
const { isLoading, buildDetails } = build;

useEffect(() => {
if (!buildDetails) {
api.fetchBuild(buildId).then(data => setBuild({
isLoading: false,
buildDetails: data,
}));
}
}, [buildDetails]);

if (isLoading) {
function CommitSummary({ buildDetails }) {
if (!buildDetails) {
return <LoadingIndicator size="mini" text="Getting commit details..." />;
}

Expand Down Expand Up @@ -74,7 +58,20 @@ function CommitSummary({ buildId }) {
);
}
CommitSummary.propTypes = {
buildId: PropTypes.number.isRequired,
buildDetails: PropTypes.shape({
site: PropTypes.shape({
owner: PropTypes.string.isRequired,
repository: PropTypes.string.isRequired,
}).isRequired,
branch: PropTypes.string.isRequired,
username: PropTypes.string.isRequired,
clonedCommitSha: PropTypes.string.isRequired,
createdAt: PropTypes.instanceOf(Date).isRequired,
}),
};

CommitSummary.defaultProps = {
buildDetails: null,
};

export { CommitSummary };
Expand Down
34 changes: 24 additions & 10 deletions frontend/components/site/siteBuildLogs.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,42 @@
import React from 'react';
import { Link, useParams } from 'react-router-dom';

import { useBuildLogs } from '../../hooks';
import { useBuildLogs, useBuildDetails } from '../../hooks';
import LoadingIndicator from '../LoadingIndicator';
import SiteBuildLogTable from './siteBuildLogTable';
import DownloadBuildLogsButton from './downloadBuildLogsButton';
import CommitSummary from './CommitSummary';

export const REFRESH_INTERVAL = 15 * 1000;
const BUILD_LOG_RETENTION_LIMIT = 180 * 24 * 60 * 60 * 1000; // 180 days in milliseconds

function buildTooOld(build) {
return (new Date() - new Date(build.completedAt)) > BUILD_LOG_RETENTION_LIMIT;
}

function getSiteBuildLogTable(buildDetails, logs, state) {
if (logs && logs?.length > 0) {
return <SiteBuildLogTable buildLogs={logs} buildState={state} />;
}
if (buildTooOld(buildDetails)) {
return <SiteBuildLogTable buildLogs={['Builds more than 180 days old are deleted according to platform policy.']} />;
}
return <SiteBuildLogTable buildLogs={['This build does not have any build logs.']} />;
}

const SiteBuildLogs = () => {
const { buildId: buildIdStr } = useParams();
const buildId = parseInt(buildIdStr, 10);
const { logs, state } = useBuildLogs(buildId);
const { buildDetails, isLoading } = useBuildDetails(buildId);

if (isLoading || !logs) {
return <LoadingIndicator size="mini" text="Getting build log details..." />;
}

return (
<div>
<CommitSummary buildId={buildId} />
<CommitSummary buildDetails={buildDetails} />
<div className="log-tools">
<ul className="usa-unstyled-list">
{(process.env.FEATURE_BUILD_TASKS === 'active') && (
Expand All @@ -26,14 +47,7 @@ const SiteBuildLogs = () => {
<li><DownloadBuildLogsButton buildId={buildId} buildLogsData={logs} /></li>
</ul>
</div>
{(!logs || logs?.length === 0) && (
<div>
<SiteBuildLogTable buildLogs={['This build does not have any build logs.']} />
</div>
)}
{(logs && logs?.length > 0) && (
<SiteBuildLogTable buildLogs={logs} buildState={state} />
)}
{ getSiteBuildLogTable(buildDetails, logs, state) }
</div>
);
};
Expand Down
5 changes: 4 additions & 1 deletion frontend/components/site/siteBuildTasks.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import prettyBytes from 'pretty-bytes';
import {
IconCheckCircle, IconClock, IconExclamationCircle, IconSpinner,
} from '../icons';
import { useBuildDetails } from '../../hooks';
import CommitSummary from './CommitSummary';
import api from '../../util/federalistApi';

Expand Down Expand Up @@ -55,6 +56,8 @@ const SiteBuildTasks = () => {
const buildId = parseInt(buildIdStr, 10);
const [buildTasks, setBuildTasks] = useState([]);

const { buildDetails } = useBuildDetails(buildId);

let intervalHandle;
useEffect(() => {
function fetchTasks(thisBuildId) {
Expand All @@ -76,7 +79,7 @@ const SiteBuildTasks = () => {

return (
<div>
<CommitSummary buildId={buildId} />
<CommitSummary buildDetails={buildDetails} />
<div className="log-tools">
<ul className="usa-unstyled-list">
<li>
Expand Down
1 change: 1 addition & 0 deletions frontend/hooks/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export * from './useBuildDetails';
export * from './useBuildLogs';
export * from './useSiteBranchConfigs';
export * from './useSiteDomains';
23 changes: 23 additions & 0 deletions frontend/hooks/useBuildDetails.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/* eslint-disable import/prefer-default-export */
import { useEffect, useState } from 'react';
import api from '../util/federalistApi';

const initResultsState = {
buildDetails: null,
isLoading: true,
};

export const useBuildDetails = (id) => {
const [results, setResults] = useState(initResultsState);

useEffect(() => {
if (!results.buildDetails) {
api.fetchBuild(id).then(data => setResults({
isLoading: false,
buildDetails: data,
}));
}
}, [results]);

return results;
};
81 changes: 30 additions & 51 deletions test/frontend/components/site/CommitSummary.test.js
Original file line number Diff line number Diff line change
@@ -1,79 +1,58 @@
import React from 'react';
import { expect, assert } from 'chai';
import proxyquire from 'proxyquire';
import sinon from 'sinon';
import { mount } from 'enzyme';

import api from '../../../../frontend/util/federalistApi';
import LoadingIndicator from '../../../../frontend/components/LoadingIndicator';

proxyquire.noCallThru();

const fetchBuildMock = sinon.stub();
const buildActions = {
fetchBuild: fetchBuildMock,
};
const CommitSummary = proxyquire(
'../../../../frontend/components/site/CommitSummary',
{
'../icons': {
IconBranch: () => <span />,
},
'../../actions/buildActions': buildActions,
}
).default;

const defaultProps = {
buildId: 1,
buildDetails: {
site: {
owner: 'user',
repository: 'repo',
},
branch: 'branch',
username: 'username',
clonedCommitSha: 'sha4567890abcdef',
createdAt: new Date(),
}
};

describe('<CommitSummary />', () => {
afterEach(() => {
sinon.restore();
});

it('should exist', () => {
assert.isDefined(CommitSummary);
});

it('renders a loading state whie loading', () => {
const stub = sinon.stub(api, 'fetchBuild');
stub.resolves([]);

const wrapper = mount(<CommitSummary {...defaultProps} />);
it('renders a loading state while loading', () => {
const wrapper = mount(<CommitSummary { ...{ buildDetails: null } } />);
expect(wrapper.find(LoadingIndicator)).to.have.length(1);
});

// no useEffect in tests
// it('requests build information once on load', () => {
// const wrapper = mountStore(<CommitSummary {...defaultProps} />, defaultState);
// const buildId = 1;
// expect(fetchBuildMock.callCount).to.be.greaterThanOrEqual(1);
// fetchBuildMock.resetHistory();
// sinon.restore();
// });

// describe('after load', () => {
// let wrapper;
// let loadedState = lodashClonedeep(defaultState);
// loadedState.build = {
// isLoading: false,
// data: { ...defaultBuildData }
// };

// it('renders the branch and github user name for the commit', () => {
// wrapper = mountStore(<CommitSummary {...defaultProps} />, loadedState);
// expect(wrapper.find('.commit-branch')).to.have.length(1);
// expect(wrapper.find('.commit-branch').text()).to.contain(defaultBuildData.branch);
// expect(wrapper.find('.commit-username')).to.have.length(1);
// expect(wrapper.find('.commit-username').text()).to.equal(defaultBuildData.username);
// });

// it('formats a sha link correctly and limits to first 7 chars', () => {
// wrapper = mountStore(<CommitSummary {...defaultProps} />, loadedState);
// expect(wrapper.find('.sha-link')).to.have.length(1);
// expect(defaultBuildData.clonedCommitSha).to.contain(wrapper.find('.sha-link').text());
// expect(wrapper.find('.sha-link').text()).to.have.length(7);
// });
// });
describe('after load', () => {
const build = defaultProps.buildDetails;

it('renders the branch and github user name for the commit', () => {
const wrapper = mount(<CommitSummary {...defaultProps} />);
expect(wrapper.find('.commit-branch')).to.have.length(1);
expect(wrapper.find('.commit-branch').text()).to.contain(build.branch);
expect(wrapper.find('.commit-username')).to.have.length(1);
expect(wrapper.find('.commit-username').text()).to.equal(build.username);
});

it('formats a sha link correctly and limits to first 7 chars', () => {
const wrapper = mount(<CommitSummary {...defaultProps} />);
expect(wrapper.find('.sha-link')).to.have.length(1);
expect(build.clonedCommitSha).to.contain(wrapper.find('.sha-link').text());
expect(wrapper.find('.sha-link').text()).to.have.length(7);
});
});
});
51 changes: 48 additions & 3 deletions test/frontend/components/site/siteBuildLogs.test.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,50 @@
import React from 'react';
import { expect } from 'chai';
import { expect, assert } from 'chai';
import { shallow } from 'enzyme';
import sinon from 'sinon';

import api from '../../../../frontend/util/federalistApi';
import { SiteBuildLogs } from '../../../../frontend/components/site/siteBuildLogs';
import LoadingIndicator from '../../../../frontend/components/LoadingIndicator';

let props;

// Initial work for when the containing .skip can be removed
const defaultBuild = {
site: {
owner: 'user',
repository: 'repo',
},
branch: 'branch',
username: 'username',
clonedCommitSha: 'sha4567890abcdef',
createdAt: new Date(),
};

describe.skip('<SiteBuildLogs/>', () => {
beforeEach(() => {
props = {
buildId: '123'
buildId: '123',
buildDetails: defaultBuild,
isLoading: false,
logs: [],
};
});

afterEach(() => {
sinon.restore();
});

// For when the containing .skip can be removed
it('should exist', () => {
assert.isDefined(SiteBuildLogs);
});

// Placeholder for when the containing .skip can be removed
it('requests build information once on load', () => {
// Verify that fetchBuild is called once
});

it('should render a button to download logs if builds exist', () => {
const wrapper = shallow(<SiteBuildLogs {...props} />);
expect(wrapper.find('SiteBuildLogTable')).to.have.length(1);
Expand All @@ -32,6 +62,21 @@ describe.skip('<SiteBuildLogs/>', () => {
expect(wrapper.find(LoadingIndicator)).to.have.length(1);
});

// Placeholder for when the containing .skip can be removed
it('should render an explanatory message if the build is over 180 days old', () => {
const stubBuild = { completedAt: new Date(Date.now() - (181 * 86400000)) }; // 181 days ago
const stub = sinon.stub(api, 'fetchBuild');
stub.resolves(stubBuild);

props.buildLogs.isLoading = false;
props.buildLogs.data = [];

const wrapper = shallow(<SiteBuildLogs {...props} />);
expect(wrapper.find('SiteBuildLogTable')).to.have.length(0);
expect(wrapper.find('p').contains('Builds more than 180 days old are deleted according to platform policy.')).to.be.true;
expect(wrapper.find('DownloadBuildLogsButton')).to.have.length(0);
});

it('should render an empty state if there are no builds', () => {
props.buildLogs.isLoading = false;
props.buildLogs.data = [];
Expand All @@ -42,7 +87,7 @@ describe.skip('<SiteBuildLogs/>', () => {
expect(wrapper.find('DownloadBuildLogsButton')).to.have.length(0);
});

it('should fetch the builds on mount', () => {
it('should fetch the build logs on mount', () => {
const spy = sinon.spy();

props.actions = { fetchBuildLogs: spy };
Expand Down
Loading