Add status tabs and summary card shells to images single page. #5073

dvail · 2023-03-02T15:06:42Z

Description

Implements the CVE status tab section of the image single page and bare-bones summary cards.

Follow ups needed for ROX-15384 to be complete

Implement correct icons and styling for summary cards
Integrate query with URL search parameters applied by the filter bar
Integrate summary card "hidden" rendering with URL search parameters

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

Error state:

Loading state (the blue speck is a spinner):

Rendering at various sizes when the request completes successfully:

openshift-ci · 2023-03-02T15:06:49Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

dvail · 2023-03-02T15:06:55Z

Current dependencies on/for this PR:

master
- PR Add status tabs and summary card shells to images single page. #5073 👈
  - PR Add CVE severity and status icons to image page summary cards #5116
    - PR Add images table for Workload CVE Image single page #5175
      - PR Add expandable section for image vulnerability components table #5218
  - PR Add URL string union hook to better integrate url params with Tab components #5111

This comment was auto-generated by Graphite.

roxbot · 2023-03-02T15:24:20Z

Images are ready for the commit at 16bf636.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.74.x-313-g16bf636f53.

dvail · 2023-03-02T15:12:34Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/WorkloadCvesImageSinglePage.tsx

@@ -109,7 +110,7 @@ export type ImageDetailsResponse = {
        } | null;

        scan: {
-            dataSource: { name: string };
+            dataSource: { name: string } | null;


I saw that this field was incorrectly typed when I was experimenting with code generation in #5065, but I fixed it as part of this PR since it is unlikely that the former is going to get merged soon (if at all).

dvail · 2023-03-02T15:22:16Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/types.ts

+
+const cveStatusTabValues = ['Observed', 'Deferred', 'False Positive'] as const;
+
+export type CveStatusTab = typeof cveStatusTabValues[number];
+
+export function isValidCveStatusTab(value: unknown): value is CveStatusTab {
+    return cveStatusTabValues.some((tab) => tab === value);
+}


I moved this over since we are now using the CveStatusTab type in a handful of places, but if you think it fits better in searchUtils LMK. (EntityTab will probably follow in the future too if we keep this here.)

dvail · 2023-03-02T15:24:47Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/ImageSingleVulnerabilities.tsx

+                    isBox
+                >
+                    <Tab eventKey="Observed" title={<TabTitleText>Observed CVEs</TabTitleText>}>
+                        <PageSection variant="light" component="div">


At one of the upcoming syncs we will have to see if the plan is for "Deferrals" and "False positives" to be almost identical to the "Observed" tab. If so, we probably don't need to render a separate component tree in each tab here and can just have the tabs control gql query params for the underlying data.

dvail · 2023-03-03T15:13:27Z

...latform/src/Containers/Vulnerabilities/WorkloadCves/SummaryCards/CvesByStatusSummaryCard.tsx

@@ -0,0 +1,36 @@
+import React from 'react';


These summary card components are barely reusable, if at all. Do you think it is cleaner to keep them in separate files like this, or just inline them in the main ImageSingleVulnerabilities component?

i think keeping them separate makes sense since the ImageSingleVulnerabilities component is already 200+ lines long.

dvail · 2023-03-06T18:49:47Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/WorkloadCvesImageSinglePage.tsx

-            dataSource: { name: string };
-            scanTime: Date | null;
-        };
+        dataSource: { name: string } | null;


Note: un-nesting these fields to avoid extra work on the backend as pointed out by Mandar.

alwayshooin · 2023-03-08T03:40:56Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/ImageSingleVulnerabilities.tsx

+    });
+
+    return statusCounts;
+}


can we pull these under the type defs so it's easier to scan? i personally like having the types be defined first, but if this makes sense to you i'm open to that too

Sure that works for me. How about types for the React component props? I've been trying to keep the prop type definitions directly above the component, but below other functions and type definitions.

alwayshooin · 2023-03-08T03:43:10Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/ImageSingleVulnerabilities.tsx

+        mainContent = (
+            <Bullseye>
+                <Spinner isSVG />
+            </Bullseye>


just curious, what's the difference in logic here between using a spinner vs the skeleton?

I went with the spinner since we are waiting on a large chunk of UI with variable shape. (Summary cards + a table with an unknown number of rows.) That and the spinner is what we use in other parts of the app for these types of table sections, and it matches with the PF example.

I try to use the Skeleton for small, precise UI elements that are loading that we will definitely know the shape of, like the header/label section at the top of this page, or the result preview items on the collection form.

alwayshooin · 2023-03-08T03:45:49Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/ImageSingleVulnerabilities.tsx

+        const severityCounts = severityCountsFromImageVulnerabilities(vulnerabilities);
+        const cveStatusCounts = statusCountsFromImageVulnerabilities(vulnerabilities);
+        // TODO Integrate these with page search filters
+        const hiddenSeverities = new Set<VulnerabilitySeverity>(['LOW_VULNERABILITY_SEVERITY']);


oh why are we hiding the low severities? is there a possibility of adding new severities to hide in the widget?

Oops, no this was just for testing and I forgot to delete it. The hidden severities/statuses are going to be calculated based on search filters once the toolbar is merged. I'll fix this here.

alwayshooin · 2023-03-08T03:46:16Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/ImageSingleVulnerabilities.tsx

+                        />
+                    </GridItem>
+                </Grid>
+            </>


i dont think you need the fragments here if it's already wrapped by a single component (Grid)

alwayshooin · 2023-03-08T03:47:54Z

...latform/src/Containers/Vulnerabilities/WorkloadCves/SummaryCards/CvesByStatusSummaryCard.tsx

@@ -0,0 +1,36 @@
+import React from 'react';


i think keeping them separate makes sense since the ImageSingleVulnerabilities component is already 200+ lines long.

openshift-ci bot added the do-not-merge/work-in-progress label Mar 2, 2023

github-actions bot added the area/ui label Mar 2, 2023

dvail force-pushed the dv/ROX-15384-implement-image-single-page-summary branch 2 times, most recently from 939248b to 1a45852 Compare March 3, 2023 15:46

dvail added the epic/vm2.0/workload-cves label Mar 3, 2023

dvail commented Mar 3, 2023

View reviewed changes

dvail requested a review from alwayshooin March 3, 2023 15:54

dvail marked this pull request as ready for review March 3, 2023 15:54

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 3, 2023

dvail mentioned this pull request Mar 6, 2023

Add URL string union hook to better integrate url params with Tab components #5111

Merged

5 tasks

dvail commented Mar 6, 2023

View reviewed changes

dvail mentioned this pull request Mar 6, 2023

Add CVE severity and status icons to image page summary cards #5116

Merged

5 tasks

alwayshooin approved these changes Mar 8, 2023

View reviewed changes

dvail added 3 commits March 8, 2023 13:26

Add status tabs and summary card shells to images single page.

bca7adb

Update query

0dd7ef8

Address PR comments

16bf636

dvail force-pushed the dv/ROX-15384-implement-image-single-page-summary branch from 81b074b to 16bf636 Compare March 8, 2023 18:28

dvail merged commit bb8d847 into master Mar 8, 2023

dvail deleted the dv/ROX-15384-implement-image-single-page-summary branch March 8, 2023 20:19

This was referenced Mar 9, 2023

Add images table for Workload CVE Image single page #5175

Merged

Add expandable section for image vulnerability components table #5218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add status tabs and summary card shells to images single page. #5073

Add status tabs and summary card shells to images single page. #5073

dvail commented Mar 2, 2023 •

edited

openshift-ci bot commented Mar 2, 2023

dvail commented Mar 2, 2023 •

edited

roxbot commented Mar 2, 2023 •

edited

dvail Mar 2, 2023

dvail Mar 2, 2023

dvail Mar 2, 2023

dvail Mar 3, 2023

alwayshooin Mar 8, 2023

dvail Mar 6, 2023 •

edited

alwayshooin Mar 8, 2023

dvail Mar 8, 2023

alwayshooin Mar 8, 2023

dvail Mar 8, 2023

alwayshooin Mar 8, 2023

dvail Mar 8, 2023

alwayshooin Mar 8, 2023

alwayshooin Mar 8, 2023

Add status tabs and summary card shells to images single page. #5073

Add status tabs and summary card shells to images single page. #5073

Conversation

dvail commented Mar 2, 2023 • edited

Description

Follow ups needed for ROX-15384 to be complete

Checklist

Testing Performed

openshift-ci bot commented Mar 2, 2023

dvail commented Mar 2, 2023 • edited

roxbot commented Mar 2, 2023 • edited

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

dvail Mar 6, 2023 • edited

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

dvail commented Mar 2, 2023 •

edited

dvail commented Mar 2, 2023 •

edited

roxbot commented Mar 2, 2023 •

edited

dvail Mar 6, 2023 •

edited