feat: Aggregating Alerts Feature (#2230)

* Aggregating Alerts Feature Signed-off-by: Josh Slaughter <[email protected]> * Fixing lint issue Signed-off-by: Josh Slaughter <[email protected]> * - Deeper grouping of notices, now by message then severity - Sorting of notices by severity - Added a number next to show details if # of notices > 1 - Adjusted css for info-svg-icon to align it with others - Turned off param-reassign rule Signed-off-by: Josh Slaughter <[email protected]> * Adjusting betterer to account for eslint rule change Signed-off-by: Josh Slaughter <[email protected]> --------- Signed-off-by: Josh Slaughter <[email protected]>
amundsen-io · Feb 29, 2024 · 73313b8 · 73313b8
1 parent 890bfaf
commit 73313b8
Show file tree

Hide file tree

Showing 11 changed files with 238 additions and 69 deletions.
diff --git a/frontend/amundsen_application/static/.betterer.results b/frontend/amundsen_application/static/.betterer.results
@@ -9,9 +9,9 @@ exports[`eslint`] = {
       [71, 6, 118, "Visible, non-interactive elements with click handlers must have at least one keyboard listener.", "3051403928"],
       [71, 6, 118, "Static HTML elements with event handlers require a role.", "3051403928"]
     ],
-    "js/components/EditableText/index.tsx:175217081": [
-      [82, 6, 13, "Do not use setState in componentDidUpdate", "57229240"],
-      [100, 6, 13, "Do not use setState in componentDidUpdate", "57229240"]
+    "js/components/EditableText/index.tsx:2590685581": [
+      [83, 6, 13, "Do not use setState in componentDidUpdate", "57229240"],
+      [101, 6, 13, "Do not use setState in componentDidUpdate", "57229240"]
     ],
     "js/components/OwnerEditor/index.tsx:2385247435": [
       [85, 6, 13, "Do not use setState in componentDidUpdate", "57229240"]
@@ -35,12 +35,8 @@ exports[`eslint`] = {
       [79, 19, 21, "Must use destructuring state assignment", "3239125001"]
     ],
     "js/ducks/dashboard/api/v0.ts:1572663041": [
-      [20, 2, 4, "Assignment to property of function parameter \'data\'.", "2087377941"],
       [43, 28, 104, "Do not nest ternary expressions.", "1163212497"]
     ],
-    "js/ducks/issue/api/v0.ts:638633038": [
-      [45, 8, 19, "Assignment to property of function parameter \'notificationPayload\'.", "3904454342"]
-    ],
     "js/ducks/issue/reducer.ts:891877399": [
       [141, 6, 56, "Unexpected lexical declaration in case block.", "2031834906"]
     ],
@@ -56,20 +52,14 @@ exports[`eslint`] = {
       [417, 6, 61, "Unexpected lexical declaration in case block.", "2053236172"]
     ],
     "js/ducks/search/utils.ts:923002554": [
-      [19, 2, 8, "Assignment to function parameter \'resource\'.", "2131237679"],
       [20, 2, 248, "Expected a default case.", "1034339850"]
     ],
     "js/ducks/tableMetadata/api/v0.ts:3333048528": [
-      [92, 6, 9, "Assignment to property of function parameter \'tableData\'.", "1466754955"],
       [141, 23, 2, "Expected to return a value at the end of arrow function.", "5859494"]
     ],
     "js/ducks/tableMetadata/reducer.ts:3935312006": [
       [482, 6, 84, "Unexpected lexical declaration in case block.", "114266473"]
     ],
-    "js/ducks/utilMethods.ts:1762524729": [
-      [21, 6, 3, "Assignment to property of function parameter \'obj\'.", "193420290"],
-      [34, 6, 3, "Assignment to property of function parameter \'obj\'.", "193420290"]
-    ],
     "js/features/Breadcrumb/index.tsx:1046428150": [
       [69, 6, 141, "Visible, non-interactive elements with click handlers must have at least one keyboard listener.", "1742605206"],
       [69, 6, 141, "Static HTML elements with event handlers require a role.", "1742605206"]
@@ -79,16 +69,6 @@ exports[`eslint`] = {
       [116, 6, 36, "Visible, non-interactive elements with click handlers must have at least one keyboard listener.", "3801508926"],
       [116, 6, 36, "Static HTML elements with event handlers require a role.", "3801508926"]
     ],
-    "js/features/ColumnList/ColumnType/parser.ts:2297011907": [
-      [59, 6, 10, "Assignment to function parameter \'startIndex\'.", "3807744539"],
-      [60, 6, 12, "Assignment to function parameter \'currentIndex\'.", "2078922066"],
-      [84, 10, 12, "Assignment to function parameter \'currentIndex\'.", "2078922066"],
-      [86, 8, 12, "Assignment to function parameter \'currentIndex\'.", "2078922066"],
-      [131, 8, 10, "Assignment to function parameter \'startIndex\'.", "3807744539"],
-      [132, 8, 12, "Assignment to function parameter \'currentIndex\'.", "2078922066"],
-      [135, 6, 12, "Assignment to function parameter \'currentIndex\'.", "2078922066"],
-      [178, 4, 10, "Assignment to function parameter \'columnType\'.", "460876587"]
-    ],
     "js/features/Footer/index.tsx:3658564998": [
       [45, 8, 22, "Must use destructuring props assignment", "1925601400"],
       [49, 12, 22, "Must use destructuring props assignment", "1925601400"]
@@ -141,14 +121,8 @@ exports[`eslint`] = {
       [13, 0, 455, "Component should be written as a pure function", "2334877134"],
       [15, 35, 20, "Must use destructuring props assignment", "2510284131"]
     ],
-    "js/pages/TableDetailPage/ReportTableIssue/index.tsx:1210646415": [
-      [170, 15, 20, "Script URL is a form of eval.", "3959800777"]
-    ],
-    "js/pages/TableDetailPage/TableOwnerEditor/index.tsx:2069554136": [
-      [30, 4, 3, "Assignment to property of function parameter \'obj\'.", "193420290"]
-    ],
-    "js/utils/owner.ts:2186084439": [
-      [10, 4, 9, "Assignment to property of function parameter \'resultObj\'.", "1686251499"]
+    "js/pages/TableDetailPage/ReportTableIssue/index.tsx:2299677182": [
+      [168, 15, 20, "Script URL is a form of eval.", "3959800777"]
     ]
   }`
 };
diff --git a/frontend/amundsen_application/static/.betterer.ts b/frontend/amundsen_application/static/.betterer.ts
@@ -15,7 +15,7 @@ export default {
       'no-extra-boolean-cast': 'error',
       'no-multi-str': 'error',
       'no-nested-ternary': 'error',
-      'no-param-reassign': 'error',
+      'no-param-reassign': 'off',
       'no-restricted-globals': 'error',
       'no-script-url': 'error',
       'no-unneeded-ternary': 'error',

diff --git a/frontend/amundsen_application/static/js/components/Alert/Alert.tsx b/frontend/amundsen_application/static/js/components/Alert/Alert.tsx
@@ -82,6 +82,9 @@ export const Alert: React.FC<AlertProps> = ({
         onClick={handleSeeDetails}
       >
         {OPEN_PAYLOAD_CTA}
+        {(payload?.descriptions || []).length > 1
+          ? ` (${(payload.descriptions || []).length})`
+          : ''}
       </button>
     );
   }

diff --git a/frontend/amundsen_application/static/js/components/Alert/AlertList.tsx b/frontend/amundsen_application/static/js/components/Alert/AlertList.tsx
@@ -3,28 +3,67 @@
 
 import * as React from 'react';
 
-import { NoticeType } from 'config/config-types';
+import { NoticeSeverity, NoticeType } from 'config/config-types';
 import { Alert } from './Alert';
 
 export interface AlertListProps {
   notices: NoticeType[];
 }
 
+export interface AggregatedAlertListProps {
+  notices: {
+    [key: string]: NoticeType;
+  };
+}
+
+const aggregateNotices = (notices) =>
+  notices.reduce((accum, notice: NoticeType) => {
+    if (notice) {
+      const { messageHtml, severity, payload } = notice;
+
+      if (typeof messageHtml !== 'function') {
+        if (payload) {
+          accum[messageHtml] ??= {};
+          accum[messageHtml][severity] ??= {
+            payload: { descriptions: [] },
+          };
+          accum[messageHtml][severity].payload.descriptions.push(payload);
+        } else {
+          accum[messageHtml] = {
+            [severity]: { ...notice },
+          };
+        }
+      }
+    }
+
+    return accum;
+  }, {});
+
 export const AlertList: React.FC<AlertListProps> = ({ notices }) => {
   if (!notices.length) {
     return null;
   }
 
+  const aggregated = aggregateNotices(notices);
+  const NoticeSeverityValues = Object.values(NoticeSeverity);
+
   return (
     <div className="alert-list">
-      {notices.map((notice, idx) => (
-        <Alert
-          key={idx}
-          message={notice.messageHtml}
-          severity={notice.severity}
-          payload={notice.payload}
-        />
-      ))}
+      {Object.keys(aggregated).map((notice, idx) =>
+        Object.keys(aggregated[notice])
+          .sort(
+            (a: NoticeSeverity, b: NoticeSeverity) =>
+              NoticeSeverityValues.indexOf(a) - NoticeSeverityValues.indexOf(b)
+          )
+          .map((severity) => (
+            <Alert
+              key={idx}
+              message={notice}
+              severity={severity as NoticeSeverity}
+              payload={aggregated[notice][severity].payload}
+            />
+          ))
+      )}
     </div>
   );
 };
diff --git a/frontend/amundsen_application/static/js/components/Alert/alert.story.tsx b/frontend/amundsen_application/static/js/components/Alert/alert.story.tsx
@@ -138,6 +138,22 @@ const list = [
     messageHtml: 'Second alert of the stack',
   },
   { severity: NoticeSeverity.ALERT, messageHtml: 'Third alert of the stack' },
+  {
+    severity: NoticeSeverity.ALERT,
+    messageHtml: 'Aggregated alert of the stack',
+    payload: {
+      term: 'Test term 1',
+      description: 'Test description 1',
+    },
+  },
+  {
+    severity: NoticeSeverity.ALERT,
+    messageHtml: 'Aggregated alert of the stack',
+    payload: {
+      term: 'Test term 2',
+      description: 'Test description 2',
+    },
+  },
 ];
 
 export const AlertListStory = (): React.ReactNode => (

diff --git a/frontend/amundsen_application/static/js/components/Alert/styles.scss b/frontend/amundsen_application/static/js/components/Alert/styles.scss
@@ -51,6 +51,11 @@ $alert-alert-background: #ffe4dd;
     margin-right: $spacer-1;
   }
 
+  .info-svg-icon {
+    margin-right: $spacer-half;
+    margin-left: -$spacer-half;
+  }
+
   .alert-action {
     margin: auto 0 auto auto;
   }

diff --git a/frontend/amundsen_application/static/js/components/DefinitionList/definitionList.story.tsx b/frontend/amundsen_application/static/js/components/DefinitionList/definitionList.story.tsx
@@ -34,12 +34,12 @@ export const DefinitionListStory = (): React.ReactNode => (
           {
             term: 'Verity checks',
             description:
-              '1 out of 4 checks failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Ownser</a>)',
+              '1 out of 4 checks failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Owner</a>)',
           },
           {
             term: 'Failed DAGs',
             description:
-              '1 out of 4 DAGs failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Ownser</a>)',
+              '1 out of 4 DAGs failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Owner</a>)',
           },
         ]}
       />
@@ -55,7 +55,46 @@ export const DefinitionListStory = (): React.ReactNode => (
           {
             term: 'Failed DAGs',
             description:
-              '1 out of 4 DAGs failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Ownser</a>)',
+              '1 out of 4 DAGs failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Owner</a>)',
+          },
+        ]}
+      />
+    </StorySection>
+    <StorySection title="DefinitionList with aggregated descriptions">
+      <DefinitionList
+        definitions={[
+          {
+            term: 'Table name',
+            description: [
+              {
+                'Failed Check': 'coco.fact_rides',
+                Owner: '<a href="http://lyft.com">Owner</a>',
+              },
+              {
+                'Failed Check 2': 'coco.fact_rides',
+                Owner: 'Just a normal string',
+              },
+            ],
+          },
+        ]}
+      />
+    </StorySection>
+    <StorySection title="DefinitionList with aggregated descriptions and fixed term width">
+      <DefinitionList
+        termWidth={200}
+        definitions={[
+          {
+            term: 'Table name',
+            description: [
+              {
+                'Failed Check': 'coco.fact_rides',
+                Owner: '<a href="http://lyft.com">Owner</a>',
+              },
+              {
+                'Failed Check 2': 'coco.fact_rides',
+                Owner: 'Just a normal string',
+              },
+            ],
           },
         ]}
       />

diff --git a/frontend/amundsen_application/static/js/components/DefinitionList/index.spec.tsx b/frontend/amundsen_application/static/js/components/DefinitionList/index.spec.tsx
@@ -149,5 +149,32 @@ describe('DefinitionList', () => {
         expect(actual).toEqual(expected);
       });
     });
+
+    describe('when passing aggregated descriptions', () => {
+      it('should render them', () => {
+        const { wrapper } = setup({
+          definitions: [
+            {
+              term: 'Table name',
+              description: [
+                {
+                  'Failed Check': 'coco.fact_rides',
+                  Owner: '<a href="http://lyft.com">Owner</a>',
+                },
+                {
+                  'Failed Check 2': 'coco.fact_rides',
+                  Owner: 'Just a normal string',
+                },
+              ],
+            },
+          ],
+        });
+
+        const itemGroup = wrapper.find('.definition-list-items-group');
+
+        expect(itemGroup.length).toEqual(2);
+        expect(itemGroup.find('.definition-list-term').length).toEqual(4);
+      });
+    });
   });
 });
diff --git a/frontend/amundsen_application/static/js/components/DefinitionList/index.tsx b/frontend/amundsen_application/static/js/components/DefinitionList/index.tsx
@@ -23,26 +23,69 @@ export interface DefinitionListProps {
 export const DefinitionList: React.FC<DefinitionListProps> = ({
   definitions,
   termWidth,
-}) => (
-  <dl className="definition-list">
-    {definitions.map(({ term, description }) => (
-      <div className="definition-list-container" key={term}>
-        <dt
-          className="definition-list-term"
-          style={{ minWidth: termWidth ? `${termWidth}px` : 'auto' }}
-        >
-          {term}
-        </dt>
-        <dd className="definition-list-definition">
-          {typeof description === 'string' ? (
-            <SanitizedHTML html={description} className="definition-text" />
+}) => {
+  const parseDescription = (description) => {
+    switch (typeof description) {
+      case 'object':
+        return (
+          <>
+            {Array.isArray(description)
+              ? description.map((item) => {
+                  const items = Object.keys(item).map((key) => (
+                    <div key={key}>
+                      <div
+                        className="definition-list-term"
+                        style={{
+                          minWidth: termWidth ? `${termWidth}px` : 'auto',
+                        }}
+                      >
+                        {key}:
+                      </div>
+                      <div className="definition-list-definition">
+                        {parseDescription(item[key])}
+                      </div>
+                    </div>
+                  ));
+
+                  return (
+                    <div className="definition-list-items-group">{items}</div>
+                  );
+                })
+              : description}
+          </>
+        );
+      case 'string':
+        return <SanitizedHTML html={description} className="definition-text" />;
+      default:
+        return description;
+    }
+  };
+
+  return (
+    <dl className="definition-list">
+      {definitions.map(({ term, description }) => (
+        <div className="definition-list-container" key={term}>
+          {Array.isArray(description) ? (
+            <div className="definition-list-inner-container">
+              {parseDescription(description)}
+            </div>
           ) : (
-            description
+            <>
+              <dt
+                className="definition-list-term"
+                style={{ minWidth: termWidth ? `${termWidth}px` : 'auto' }}
+              >
+                {term}
+              </dt>
+              <dd className="definition-list-definition">
+                {parseDescription(description)}
+              </dd>
+            </>
           )}
-        </dd>
-      </div>
-    ))}
-  </dl>
-);
+        </div>
+      ))}
+    </dl>
+  );
+};
 
 export default DefinitionList;