[dagit] Make the warning icon optional in AppTopNav (#8294)

### Summary & Motivation In Cloud, we don't need the warning icon next to the "Status" top nav item, since in that context, daemon errors are our problem, not the user's. By not rendering the icon, we can skip performing our most common GraphQL query, `InstanceWarningQuery`. ### How I Tested These Changes Jest specs. Set `showStatusWarningIcon` to false in dagit-app, verify that the icon doesn't appear even though I have no daemons running.
dagster-io · Jun 10, 2022 · b3d063e · b3d063e
1 parent c881f12
commit b3d063e
Show file tree

Hide file tree

Showing 2 changed files with 116 additions and 32 deletions.
diff --git a/js_modules/dagit/packages/core/src/app/AppTopNav.test.tsx b/js_modules/dagit/packages/core/src/app/AppTopNav.test.tsx
@@ -1,7 +1,8 @@
-import {render, screen, waitFor} from '@testing-library/react';
+import {act, render, screen, waitFor, within} from '@testing-library/react';
 import * as React from 'react';
 
 import {TestProvider} from '../testing/TestProvider';
+import {InstigationStatus} from '../types/globalTypes';
 
 import {AppTopNav} from './AppTopNav';
 
@@ -64,45 +65,122 @@ describe('AppTopNav', () => {
 
   describe('Repo location errors', () => {
     it('does not show warning icon when no errors', async () => {
-      render(
-        <TestProvider apolloProps={{mocks: [defaultMocks]}}>
-          <AppTopNav searchPlaceholder="Test..." />
-        </TestProvider>,
-      );
-
-      await waitFor(() => {
-        expect(screen.getByText(/workspace/i)).toBeVisible();
-        expect(
-          screen.queryByRole('img', {
-            name: /warning/i,
-          }),
-        ).toBeNull();
+      await act(async () => {
+        render(
+          <TestProvider apolloProps={{mocks: [defaultMocks]}}>
+            <AppTopNav searchPlaceholder="Test..." />
+          </TestProvider>,
+        );
       });
+
+      expect(screen.getByText(/workspace/i)).toBeVisible();
+      expect(
+        screen.queryByRole('img', {
+          name: /warning/i,
+        }),
+      ).toBeNull();
     });
 
-    // todo dish: Figure out what graphql-tools is doing with this mock. 🤪
-    // eslint-disable-next-line jest/no-disabled-tests
-    it.skip('shows the error message when repo location errors are found', async () => {
+    it('shows the error message when repo location errors are found', async () => {
       const mocks = {
         RepositoryLocationOrLoadError: () => ({
           __typename: 'PythonError',
         }),
       };
 
-      render(
-        <TestProvider apolloProps={{mocks: [defaultMocks, mocks]}}>
-          <AppTopNav searchPlaceholder="Test..." />
-        </TestProvider>,
-      );
-
-      await waitFor(() => {
-        expect(screen.getByText(/workspace/i)).toBeVisible();
-        expect(
-          screen.getByRole('img', {
-            name: /warning/i,
-          }),
-        ).toBeVisible();
+      await act(async () => {
+        render(
+          <TestProvider apolloProps={{mocks: [defaultMocks, mocks]}}>
+            <AppTopNav searchPlaceholder="Test..." />
+          </TestProvider>,
+        );
+      });
+
+      expect(screen.getByText(/workspace/i)).toBeVisible();
+      expect(
+        screen.getByRole('img', {
+          name: /warning/i,
+        }),
+      ).toBeVisible();
+    });
+  });
+
+  describe('Daemon status errors', () => {
+    const mocksWithDaemonError = {
+      DaemonHealth: () => ({
+        allDaemonStatuses: () => [...new Array(1)],
+      }),
+      DaemonStatus: () => ({
+        id: 'SENSOR',
+        daemonType: 'SENSOR',
+        required: true,
+        healthy: false,
+      }),
+    };
+
+    const mocksWithSensor = {
+      Repository: () => ({
+        sensors: () => [...new Array(1)],
+      }),
+      InstigationState: () => ({
+        status: () => InstigationStatus.RUNNING,
+      }),
+    };
+
+    it('does not show status warning icon if there are sensor daemon errors but no sensors', async () => {
+      const mocksWithoutSensor = {
+        Repository: () => ({
+          sensors: () => [],
+        }),
+      };
+
+      await act(async () => {
+        render(
+          <TestProvider
+            apolloProps={{mocks: [defaultMocks, mocksWithDaemonError, mocksWithoutSensor]}}
+          >
+            <AppTopNav searchPlaceholder="Test..." showStatusWarningIcon={false} />
+          </TestProvider>,
+        );
+      });
+
+      expect(screen.getByText(/workspace/i)).toBeVisible();
+      const link = screen.getByRole('link', {name: /status/i});
+      expect(within(link).queryByText(/warning/i)).toBeNull();
+    });
+
+    it('shows status warning icon by default, if there are errors', async () => {
+      await act(async () => {
+        render(
+          <TestProvider
+            apolloProps={{mocks: [defaultMocks, mocksWithDaemonError, mocksWithSensor]}}
+          >
+            <AppTopNav searchPlaceholder="Test..." />
+          </TestProvider>,
+        );
       });
+
+      const link = screen.getByRole('link', {
+        name: /status warning/i,
+      });
+
+      expect(within(link).getByText(/status/i)).toBeVisible();
+    });
+
+    it('does not show status warning icon if `showStatusWarningIcon` is false, even with errors', async () => {
+      await act(async () => {
+        render(
+          <TestProvider
+            apolloProps={{mocks: [defaultMocks, mocksWithDaemonError, mocksWithSensor]}}
+          >
+            <AppTopNav searchPlaceholder="Test..." showStatusWarningIcon={false} />
+          </TestProvider>,
+        );
+      });
+
+      expect(screen.getByText(/workspace/i)).toBeVisible();
+      const link = screen.getByRole('link', {name: /status/i});
+      expect(within(link).queryByText(/warning/i)).toBeNull();
     });
   });
 });
diff --git a/js_modules/dagit/packages/core/src/app/AppTopNav.tsx b/js_modules/dagit/packages/core/src/app/AppTopNav.tsx
@@ -15,9 +15,15 @@ import {WebSocketStatus} from './WebSocketProvider';
 interface Props {
   searchPlaceholder: string;
   rightOfSearchBar?: React.ReactNode;
+  showStatusWarningIcon?: boolean;
 }
 
-export const AppTopNav: React.FC<Props> = ({children, rightOfSearchBar, searchPlaceholder}) => {
+export const AppTopNav: React.FC<Props> = ({
+  children,
+  rightOfSearchBar,
+  searchPlaceholder,
+  showStatusWarningIcon = true,
+}) => {
   const history = useHistory();
 
   return (
@@ -51,7 +57,7 @@ export const AppTopNav: React.FC<Props> = ({children, rightOfSearchBar, searchPl
             <TopNavLink to="/instance">
               <Box flex={{direction: 'row', alignItems: 'center', gap: 6}}>
                 Status
-                <InstanceWarningIcon />
+                {showStatusWarningIcon ? <InstanceWarningIcon /> : null}
               </Box>
             </TopNavLink>
           </ShortcutHandler>