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

svenaas · 2024-03-04T18:28:13Z

Fixes #4379

Changes proposed in this pull request:

Adds a message to the build logs page which is displayed when a build is older than 180 days, after which build logs are no longer available for display
Adds a loading indicator on the build logs page
Refactors CommitSummary to take buildDetails instead of buildID, requiring parents to retrieve the build
Refactors build details fetch into hooks
Adds tests for CommitSummary

security considerations

None. Adds explanatory and status information to the user interface regarding details already present.

frontend/components/site/siteBuildLogs.jsx

frontend/components/site/CommitSummary.jsx

sknep

this looks awesome :)

svenaas · 2024-03-06T20:51:29Z

If you want to try this out locally the easiest route is probably to hardcode a completedAt on line 17 of siteBuildLogs.jsx. Or you could edit a build in the DB to prematurely age it.

frontend/components/site/siteBuildLogs.jsx

sknep · 2024-03-06T23:17:39Z

frontend/components/site/siteBuildLogs.jsx

+      {buildTooOld(buildDetails) && (
+      <div>
+        <SiteBuildLogTable buildLogs={['Builds more than 180 days old are deleted according to platform policy.']} />
+      </div>
+      )}
      {(!logs || logs?.length === 0) && (
      <div>
        <SiteBuildLogTable buildLogs={['This build does not have any build logs.']} />


This should work, but I haven't run it locally

Suggested change

{buildTooOld(buildDetails) && (

<div>

<SiteBuildLogTable buildLogs={['Builds more than 180 days old are deleted according to platform policy.']} />

</div>

)}

{(!logs || logs?.length === 0) && (

<div>

<SiteBuildLogTable buildLogs={['This build does not have any build logs.']} />

{(!logs || logs?.length === 0) && (

<div>

{buildTooOld(buildDetails) && (

<SiteBuildLogTable buildLogs={['Builds more than 180 days old are deleted according to platform policy.']} />

)}

{!buildTooOld(buildDetails) && (

<SiteBuildLogTable buildLogs={['This build does not have any build logs.']} />

)}

It just needs a little more to work:

Suggested change

{buildTooOld(buildDetails) && (

<div>

<SiteBuildLogTable buildLogs={['Builds more than 180 days old are deleted according to platform policy.']} />

</div>

)}

{(!logs || logs?.length === 0) && (

<div>

<SiteBuildLogTable buildLogs={['This build does not have any build logs.']} />

{(!logs || logs?.length === 0) && (

<div>

{buildTooOld(buildDetails) && (

<SiteBuildLogTable buildLogs={['Builds more than 180 days old are deleted according to platform policy.']} />

)}

{!buildTooOld(buildDetails) && (

<SiteBuildLogTable buildLogs={['This build does not have any build logs.']} />

)}

</div>

)}

Oh, I may have just snipped that in a different place from you. Never mind.

Here's the approach I came up with though:

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

Which do you think is more readable?

oh i like this one just fine! you don't have to wrap the <SiteBuildLogTable> in a <div> if you don't need to for layout purposes. It's just that the return () statement can only have one parent node, so if you have two or more elements, you wrap them in a div or <>. Each of these is in their own return, and only one element, so you could save yourself 4 lines if you like :)

Good point, thanks!

What about?

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

Do you prefer this to what I pushed, @apburnes?

I was wondering if a ternary operator would 1) work, 2) be easy to read. I could see a case for either approach; they're basically indistinguishable. I think ternaries are cool, but they're sometimes hard to read. I'm neutral on this :)

Giving this a gentle bump, @apburnes. I think I'd already pushed my approach to this when you suggested another way here, but I don't know whether you'd like me to switch to your version or were just in the process of suggesting it when I applied mine.

tl/dr: Am I waiting for you to re-review or are you waiting for me to switch to your suggested implementation?

apburnes · 2024-03-11T15:15:30Z

frontend/components/site/siteBuildLogs.jsx

-      </div>
-      )}
+      {(() => {
+        if (!logs || logs?.length === 0) {


Is there a scenario where there are build logs and the build is over 180 days?

Yes, the scenario you found on dev where the build log was still being shown on an old build that still had build logs: https://pages-dev.cloud.gov/sites/2/builds/5/logs

That's why we made this change.

The logic still shows both the 180 day message and the build logs.

It only checks buildTooOld if there are no build logs.

apburnes · 2024-03-11T15:19:16Z

frontend/components/site/siteBuildLogs.jsx

-        <SiteBuildLogTable buildLogs={['This build does not have any build logs.']} />
-      </div>
-      )}
+      {(() => {


This is the only place in the frontend we use the closure syntax to display components. We don't have solid conventions for writing UI components but It is introducing another pattern for logic control. Should we settle on a convention and document it?

I'm newest to React JSX, so I defer to y'all on which convention is wisest.

I'm not sure what the immediately invoked function does here so I'd remove if it isn't necessary

Refactored into a separate function at Andrew's suggestion.

apburnes · 2024-03-11T15:21:17Z

frontend/components/site/siteBuildLogs.jsx

+  });
+  const { isLoading, buildDetails } = build;
+
+  useEffect(() => {


Since we use this hook in two places, what do you think about creating a new build details hook in the hooks directory and importing it to the paces we use it?

OK, I'll see what I can do.

Refactored into hooks

apburnes · 2024-03-12T17:54:22Z

frontend/components/site/siteBuildTasks.jsx

@@ -55,8 +55,19 @@ const SiteBuildTasks = () => {
  const buildId = parseInt(buildIdStr, 10);
  const [buildTasks, setBuildTasks] = useState([]);

+  const [build, setBuild] = useState({


You can replace this useState and useEffect with the new buildDetails hook.

Aw shoot I forgot about build tasks. On it.

How 'bout now?

apburnes

LGTM. Thanks.

…s. (#4379)

sknep reviewed Mar 4, 2024

View reviewed changes

frontend/components/site/siteBuildLogs.jsx Outdated Show resolved Hide resolved

sknep reviewed Mar 4, 2024

View reviewed changes

frontend/components/site/CommitSummary.jsx Show resolved Hide resolved

sknep previously approved these changes Mar 6, 2024

View reviewed changes

svenaas dismissed sknep’s stale review via b09e7d2 March 6, 2024 20:19

sknep previously approved these changes Mar 6, 2024

View reviewed changes

svenaas requested a review from a team March 6, 2024 20:44

sknep reviewed Mar 6, 2024

View reviewed changes

frontend/components/site/siteBuildLogs.jsx Outdated Show resolved Hide resolved

sknep reviewed Mar 6, 2024

View reviewed changes

svenaas dismissed sknep’s stale review via 3834d8a March 7, 2024 15:32

svenaas requested review from sknep and apburnes March 7, 2024 15:53

apburnes reviewed Mar 11, 2024

View reviewed changes

svenaas force-pushed the 4379-explain-that-build-logs-are-gone-after-180-days branch from 962526d to f0a1847 Compare March 12, 2024 16:40

svenaas requested a review from apburnes March 12, 2024 17:23

apburnes reviewed Mar 12, 2024

View reviewed changes

svenaas requested a review from apburnes March 12, 2024 19:32

apburnes approved these changes Mar 12, 2024

View reviewed changes

fix: Add message explaining that build logs are deleted after 180 day…

8facede

…s. (#4379)

svenaas force-pushed the 4379-explain-that-build-logs-are-gone-after-180-days branch from 64c93be to 8facede Compare March 12, 2024 19:46

svenaas merged commit b3a1f6e into main Mar 12, 2024
7 of 8 checks passed

svenaas deleted the 4379-explain-that-build-logs-are-gone-after-180-days branch March 12, 2024 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

svenaas commented Mar 4, 2024 •

edited

Loading

sknep left a comment

svenaas commented Mar 6, 2024

sknep Mar 6, 2024 •

edited

Loading

svenaas Mar 7, 2024

svenaas Mar 7, 2024

sknep Mar 7, 2024 •

edited

Loading

svenaas Mar 7, 2024

apburnes Mar 7, 2024

svenaas Mar 7, 2024 •

edited

Loading

sknep Mar 8, 2024

svenaas Mar 11, 2024

apburnes Mar 11, 2024

svenaas Mar 11, 2024

svenaas Mar 11, 2024

apburnes Mar 11, 2024

svenaas Mar 11, 2024

apburnes Mar 11, 2024

svenaas Mar 11, 2024

drewbo Mar 12, 2024

svenaas Mar 12, 2024

apburnes Mar 11, 2024

svenaas Mar 11, 2024

svenaas Mar 12, 2024

apburnes Mar 12, 2024

svenaas Mar 12, 2024

svenaas Mar 12, 2024

apburnes left a comment

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

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

Conversation

svenaas commented Mar 4, 2024 • edited Loading

Changes proposed in this pull request:

security considerations

sknep left a comment

Choose a reason for hiding this comment

svenaas commented Mar 6, 2024

sknep Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sknep Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenaas Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apburnes left a comment

Choose a reason for hiding this comment

svenaas commented Mar 4, 2024 •

edited

Loading

sknep Mar 6, 2024 •

edited

Loading

sknep Mar 7, 2024 •

edited

Loading

svenaas Mar 7, 2024 •

edited

Loading