Browse Source

fix(new-widget-builder-experience): Edit after add to dashboard duplicates (#32865)

Nar Saynorath 3 years ago
parent
commit
6affc0c923

+ 1 - 0
static/app/views/dashboardsV2/create.tsx

@@ -54,6 +54,7 @@ function CreateDashboard(props: Props) {
         dashboard={dashboard}
         dashboards={[]}
         newWidget={newWidget}
+        onSetNewWidget={() => setNewWidget(undefined)}
       />
     </Feature>
   );

+ 9 - 4
static/app/views/dashboardsV2/dashboard.tsx

@@ -80,6 +80,7 @@ type Props = {
   widgetLimitReached: boolean;
   isPreview?: boolean;
   newWidget?: Widget;
+  onSetNewWidget?: () => void;
   paramDashboardId?: string;
   paramTemplateId?: string;
 };
@@ -137,7 +138,7 @@ class Dashboard extends Component<Props, State> {
   }
 
   async componentDidMount() {
-    const {isEditing, organization, api, selection} = this.props;
+    const {isEditing, organization, api, selection, newWidget} = this.props;
     if (organization.features.includes('dashboard-grid-layout')) {
       window.addEventListener('resize', this.debouncedHandleResize);
     }
@@ -151,7 +152,9 @@ class Dashboard extends Component<Props, State> {
       this.fetchTags();
     }
 
-    this.addNewWidget();
+    if (newWidget) {
+      this.addNewWidget();
+    }
 
     // Get member list data for issue widgets
     this.fetchMemberList();
@@ -165,7 +168,7 @@ class Dashboard extends Component<Props, State> {
     if (prevProps.isEditing !== isEditing && isEditing) {
       this.fetchTags();
     }
-    if (newWidget !== prevProps.newWidget) {
+    if (newWidget && newWidget !== prevProps.newWidget) {
       this.addNewWidget();
     }
     if (!isEqual(prevProps.selection.projects, selection.projects)) {
@@ -199,11 +202,13 @@ class Dashboard extends Component<Props, State> {
   }
 
   async addNewWidget() {
-    const {api, organization, newWidget, handleAddCustomWidget} = this.props;
+    const {api, organization, newWidget, handleAddCustomWidget, onSetNewWidget} =
+      this.props;
     if (newWidget) {
       try {
         await validateWidget(api, organization.slug, newWidget);
         handleAddCustomWidget(newWidget);
+        onSetNewWidget?.();
       } catch (error) {
         // Don't do anything, widget isn't valid
         addErrorMessage(error);

+ 12 - 2
static/app/views/dashboardsV2/detail.tsx

@@ -76,6 +76,7 @@ type Props = RouteComponentProps<RouteParams, {}> & {
   route: PlainRoute;
   newWidget?: Widget;
   onDashboardUpdate?: (updatedDashboard: DashboardDetails) => void;
+  onSetNewWidget?: () => void;
 };
 
 type State = {
@@ -601,8 +602,16 @@ class DashboardDetail extends Component<Props, State> {
   }
 
   renderDashboardDetail() {
-    const {organization, dashboard, dashboards, params, router, location, newWidget} =
-      this.props;
+    const {
+      organization,
+      dashboard,
+      dashboards,
+      params,
+      router,
+      location,
+      newWidget,
+      onSetNewWidget,
+    } = this.props;
     const {modifiedDashboard, dashboardState, widgetLimitReached} = this.state;
     const {dashboardId} = params;
 
@@ -670,6 +679,7 @@ class DashboardDetail extends Component<Props, State> {
                     router={router}
                     location={location}
                     newWidget={newWidget}
+                    onSetNewWidget={onSetNewWidget}
                     isPreview={this.isPreview}
                   />
                 </Layout.Main>

+ 1 - 0
static/app/views/dashboardsV2/view.tsx

@@ -71,6 +71,7 @@ function ViewEditDashboard(props: Props) {
               dashboards={dashboards}
               onDashboardUpdate={onDashboardUpdate}
               newWidget={newWidget}
+              onSetNewWidget={() => setNewWidget(undefined)}
             />
           ) : (
             <LoadingIndicator />

+ 32 - 0
tests/js/spec/views/dashboardsV2/dashboard.spec.tsx

@@ -48,6 +48,7 @@ describe('Dashboards > Dashboard', () => {
   });
   it('dashboard adds new widget if component is mounted with newWidget prop', async () => {
     const mockHandleAddCustomWidget = jest.fn();
+    const mockCallbackToUnsetNewWidget = jest.fn();
     const wrapper = mountWithTheme(
       <Dashboard
         paramDashboardId="1"
@@ -61,16 +62,19 @@ describe('Dashboards > Dashboard', () => {
         location={initialData.location}
         newWidget={newWidget}
         widgetLimitReached={false}
+        onSetNewWidget={mockCallbackToUnsetNewWidget}
       />,
       initialData.routerContext
     );
     await tick();
     wrapper.update();
     expect(mockHandleAddCustomWidget).toHaveBeenCalled();
+    expect(mockCallbackToUnsetNewWidget).toHaveBeenCalled();
   });
 
   it('dashboard adds new widget if component updated with newWidget prop', async () => {
     const mockHandleAddCustomWidget = jest.fn();
+    const mockCallbackToUnsetNewWidget = jest.fn();
     const wrapper = mountWithTheme(
       <Dashboard
         paramDashboardId="1"
@@ -83,14 +87,42 @@ describe('Dashboards > Dashboard', () => {
         router={initialData.router}
         location={initialData.location}
         widgetLimitReached={false}
+        onSetNewWidget={mockCallbackToUnsetNewWidget}
       />,
       initialData.routerContext
     );
     expect(mockHandleAddCustomWidget).not.toHaveBeenCalled();
+    expect(mockCallbackToUnsetNewWidget).not.toHaveBeenCalled();
     wrapper.setProps({newWidget});
     await tick();
     wrapper.update();
     expect(mockHandleAddCustomWidget).toHaveBeenCalled();
+    expect(mockCallbackToUnsetNewWidget).toHaveBeenCalled();
+  });
+
+  it('dashboard does not try to add new widget if no newWidget', async () => {
+    const mockHandleAddCustomWidget = jest.fn();
+    const mockCallbackToUnsetNewWidget = jest.fn();
+    const wrapper = mountWithTheme(
+      <Dashboard
+        paramDashboardId="1"
+        dashboard={mockDashboard}
+        organization={initialData.organization}
+        isEditing={false}
+        onUpdate={() => undefined}
+        handleUpdateWidgetList={() => undefined}
+        handleAddCustomWidget={mockHandleAddCustomWidget}
+        router={initialData.router}
+        location={initialData.location}
+        widgetLimitReached={false}
+        onSetNewWidget={mockCallbackToUnsetNewWidget}
+      />,
+      initialData.routerContext
+    );
+    await tick();
+    wrapper.update();
+    expect(mockHandleAddCustomWidget).not.toHaveBeenCalled();
+    expect(mockCallbackToUnsetNewWidget).not.toHaveBeenCalled();
   });
 
   it('displays widgets with drag handle when in edit mode', () => {

+ 47 - 3
tests/js/spec/views/dashboardsV2/detail.spec.jsx

@@ -621,7 +621,7 @@ describe('Dashboards > Detail', function () {
       expect(breadcrumbs.find('BreadcrumbItem').last().text()).toEqual('Custom Errors');
     });
 
-    it('enters edit mode when given a new widget in location query', async function () {
+    it('unsets newWidget after rendering', async function () {
       initialData.router.location = {
         query: {
           displayType: 'line',
@@ -642,11 +642,55 @@ describe('Dashboards > Detail', function () {
         />,
         initialData.routerContext
       );
-      await tick();
+      expect(wrapper.find('DashboardDetail').props().initialState).toEqual(
+        DashboardState.EDIT
+      );
+      expect(wrapper.find('DashboardDetail').props().newWidget).toBeDefined();
+      await act(async () => {
+        // Wrap await tick in act because componentDidMount in Dashboard triggers
+        // state change when parsing widget from location
+        await tick();
+      });
       wrapper.update();
+
+      // The newWidget state was cleared after adding the widget
+      expect(wrapper.find('DashboardDetail').props().newWidget).toBeUndefined();
+
+      await act(async () => {
+        await tick();
+      });
+    });
+
+    it('enters edit mode when given a new widget in location query', async function () {
+      initialData.router.location = {
+        query: {
+          displayType: 'line',
+          interval: '5m',
+          queryConditions: ['title:test', 'event.type:test'],
+          queryFields: ['count()', 'failure_count()'],
+          queryNames: ['1', '2'],
+          queryOrderby: '',
+          title: 'Widget Title',
+        },
+      };
+      wrapper = mountWithTheme(
+        <ViewEditDashboard
+          organization={initialData.organization}
+          params={{orgId: 'org-slug', dashboardId: '1'}}
+          router={initialData.router}
+          location={{...initialData.router.location, pathname: '/mockpath'}}
+        />,
+        initialData.routerContext
+      );
+
       expect(wrapper.find('DashboardDetail').props().initialState).toEqual(
         DashboardState.EDIT
       );
+      await act(async () => {
+        // Wrap await tick in act because componentDidMount in Dashboard triggers
+        // state change when parsing widget from location
+        await tick();
+      });
     });
 
     it('enters view mode when not given a new widget in location query', async function () {
@@ -666,7 +710,7 @@ describe('Dashboards > Detail', function () {
       );
     });
 
-    it('opens add widget to custom  modal', async function () {
+    it('opens add widget to custom modal', async function () {
       types.MAX_WIDGETS = 10;
 
       initialData = initializeOrg({

+ 32 - 0
tests/js/spec/views/dashboardsV2/gridLayout/dashboard.spec.tsx

@@ -114,6 +114,7 @@ describe('Dashboards > Dashboard', () => {
 
   it('dashboard adds new widget if component is mounted with newWidget prop', async () => {
     const mockHandleAddCustomWidget = jest.fn();
+    const mockCallbackToUnsetNewWidget = jest.fn();
     const wrapper = mountWithTheme(
       <Dashboard
         paramDashboardId="1"
@@ -127,16 +128,19 @@ describe('Dashboards > Dashboard', () => {
         location={initialData.location}
         newWidget={newWidget}
         widgetLimitReached={false}
+        onSetNewWidget={mockCallbackToUnsetNewWidget}
       />,
       initialData.routerContext
     );
     await tick();
     wrapper.update();
     expect(mockHandleAddCustomWidget).toHaveBeenCalled();
+    expect(mockCallbackToUnsetNewWidget).toHaveBeenCalled();
   });
 
   it('dashboard adds new widget if component updated with newWidget prop', async () => {
     const mockHandleAddCustomWidget = jest.fn();
+    const mockCallbackToUnsetNewWidget = jest.fn();
     const wrapper = mountWithTheme(
       <Dashboard
         paramDashboardId="1"
@@ -149,14 +153,42 @@ describe('Dashboards > Dashboard', () => {
         router={initialData.router}
         location={initialData.location}
         widgetLimitReached={false}
+        onSetNewWidget={mockCallbackToUnsetNewWidget}
       />,
       initialData.routerContext
     );
     expect(mockHandleAddCustomWidget).not.toHaveBeenCalled();
+    expect(mockCallbackToUnsetNewWidget).not.toHaveBeenCalled();
     wrapper.setProps({newWidget});
     await tick();
     wrapper.update();
     expect(mockHandleAddCustomWidget).toHaveBeenCalled();
+    expect(mockCallbackToUnsetNewWidget).toHaveBeenCalled();
+  });
+
+  it('dashboard does not try to add new widget if no newWidget', async () => {
+    const mockHandleAddCustomWidget = jest.fn();
+    const mockCallbackToUnsetNewWidget = jest.fn();
+    const wrapper = mountWithTheme(
+      <Dashboard
+        paramDashboardId="1"
+        dashboard={mockDashboard}
+        organization={initialData.organization}
+        isEditing={false}
+        onUpdate={() => undefined}
+        handleUpdateWidgetList={() => undefined}
+        handleAddCustomWidget={mockHandleAddCustomWidget}
+        router={initialData.router}
+        location={initialData.location}
+        widgetLimitReached={false}
+        onSetNewWidget={mockCallbackToUnsetNewWidget}
+      />,
+      initialData.routerContext
+    );
+    await tick();
+    wrapper.update();
+    expect(mockHandleAddCustomWidget).not.toHaveBeenCalled();
+    expect(mockCallbackToUnsetNewWidget).not.toHaveBeenCalled();
   });
 
   describe('Issue Widgets', () => {