Skip to content

Commit

Permalink
[dagit] AssetView: Don't show historical view message if it's not (#7639
Browse files Browse the repository at this point in the history
)

## Summary

Resolves #7636.

We shouldn't show the "historical view" alert on the asset materialization list if the most recent materialization is at the top of the list.

Added a "Loading…" title to the Spinner since it's good to have, and since it gives us something removable to target for testing.

## Test Plan

Jest
  • Loading branch information
hellendag committed Apr 28, 2022
1 parent 3630d97 commit cc07c17
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 4 deletions.
57 changes: 57 additions & 0 deletions js_modules/dagit/packages/core/src/assets/AssetView.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import {render, screen, waitForElementToBeRemoved} from '@testing-library/react';
import * as React from 'react';
import {MemoryRouter} from 'react-router-dom';

import {TestProvider} from '../testing/TestProvider';

import {AssetView} from './AssetView';

describe('AssetView', () => {
const defaultMocks = {
MaterializationEvent: () => ({
timestamp: 100,
}),
};

const Test = ({path}: {path: string}) => {
return (
<TestProvider apolloProps={{mocks: defaultMocks}}>
<MemoryRouter initialEntries={[path]}>
<AssetView assetKey={{path: ['foo']}} />
</MemoryRouter>
</TestProvider>
);
};

const MESSAGE = /this is a historical view of materializations as of \./i;

describe('Historical view alert', () => {
it('shows historical view alert if `asOf` is old', async () => {
render(<Test path="/foo?asOf=10" />);
const spinner = screen.queryByTitle(/loading…/i);
await waitForElementToBeRemoved(spinner);
expect(screen.queryByText(MESSAGE)).toBeVisible();
});

it('does not show historical view alert if `asOf` is past latest materialization', async () => {
render(<Test path="/foo?asOf=200" />);
const spinner = screen.queryByTitle(/loading…/i);
await waitForElementToBeRemoved(spinner);
expect(screen.queryByText(MESSAGE)).toBeNull();
});

it('does not show historical view alert if `asOf` is equal to latest materialization', async () => {
render(<Test path="/foo?asOf=100" />);
const spinner = screen.queryByTitle(/loading…/i);
await waitForElementToBeRemoved(spinner);
expect(screen.queryByText(MESSAGE)).toBeNull();
});

it('does not show historical view alert if no `asOf` is specified', async () => {
render(<Test path="/foo" />);
const spinner = screen.queryByTitle(/loading…/i);
await waitForElementToBeRemoved(spinner);
expect(screen.queryByText(MESSAGE)).toBeNull();
});
});
});
6 changes: 4 additions & 2 deletions js_modules/dagit/packages/core/src/assets/AssetView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ export const AssetView: React.FC<Props> = ({assetKey}) => {
const {assetOrError} = queryResult.data || queryResult.previousData || {};
const asset = assetOrError && assetOrError.__typename === 'Asset' ? assetOrError : null;
const lastMaterializedAt = asset?.assetMaterializations[0]?.timestamp;
const viewingMostRecent = !params.asOf || Number(lastMaterializedAt) <= Number(params.asOf);

const definition = asset?.definition;

const repoAddress = definition
Expand Down Expand Up @@ -180,7 +182,7 @@ export const AssetView: React.FC<Props> = ({assetKey}) => {
>
<Spinner purpose="page" />
</Box>
) : params.asOf ? (
) : viewingMostRecent ? null : (
<Box
padding={{vertical: 16, horizontal: 24}}
border={{side: 'bottom', width: 1, color: Colors.KeylineGray}}
Expand All @@ -191,7 +193,7 @@ export const AssetView: React.FC<Props> = ({assetKey}) => {
hasDefinition={!!definition}
/>
</Box>
) : undefined}
)}
</div>
{isDefinitionLoaded &&
(params.view === 'definition' ? (
Expand Down
5 changes: 3 additions & 2 deletions js_modules/dagit/packages/ui/src/components/Spinner.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ export const Spinner: React.FC<{
value?: number;
fillColor?: string;
stopped?: boolean;
}> = ({purpose, value, fillColor = Colors.Gray600, stopped}) => {
title?: string;
}> = ({purpose, value, fillColor = Colors.Gray600, stopped, title = 'Loading…'}) => {
const size = () => {
switch (purpose) {
case 'page':
Expand All @@ -39,7 +40,7 @@ export const Spinner: React.FC<{
};

return (
<SpinnerWrapper $padding={padding()}>
<SpinnerWrapper $padding={padding()} title={title}>
<SlowSpinner size={size()} value={value} $fillColor={fillColor} $stopped={stopped} />
</SpinnerWrapper>
);
Expand Down

1 comment on commit cc07c17

@vercel
Copy link

@vercel vercel bot commented on cc07c17 Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Successfully deployed to the following URLs:

dagit-storybook – ./js_modules/dagit/packages/ui

dagit-storybook-elementl.vercel.app
dagit-storybook.vercel.app
dagit-storybook-git-master-elementl.vercel.app

Please sign in to comment.