fix: Data table z index in sql Editor (#13972)

(cherry picked from commit 7fb138381f)

superset-frontend/spec/javascripts/datasource/DatasourceEditor_spec.jsx

@@ -61,7 +61,7 @@ describe('DatasourceEditor', () => {
   });
 
   it('renders Tabs', () => {
-    expect(wrapper.find(Tabs)).toExist();
+    expect(wrapper.find('#table-tabs')).toExist();
   });
 
   it('makes an async request', () =>

superset-frontend/spec/javascripts/sqllab/SqlEditorLeftBar_spec.jsx

@@ -20,7 +20,8 @@ import React from 'react';
 import configureStore from 'redux-mock-store';
 import fetchMock from 'fetch-mock';
 import { shallow } from 'enzyme';
-import { render, screen, act } from '@testing-library/react';
+import { render, screen } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
 import { Provider } from 'react-redux';
 import '@testing-library/jest-dom/extend-expect';
 import thunk from 'redux-thunk';
@@ -56,6 +57,10 @@ describe('SqlEditorLeftBar', () => {
     });
   });
 
+  afterEach(() => {
+    wrapper.unmount();
+  });
+
   it('is valid', () => {
     expect(React.isValidElement(<SqlEditorLeftBar {...mockedProps} />)).toBe(
       true,
@@ -68,19 +73,14 @@ describe('SqlEditorLeftBar', () => {
 });
 
 describe('Left Panel Expansion', () => {
-  beforeEach(async () => {
-    await act(async () => {
-      render(
-        <ThemeProvider theme={supersetTheme}>
-          <Provider store={store}>
-            <SqlEditorLeftBar {...mockedProps} />
-          </Provider>
-        </ThemeProvider>,
-      );
-    });
-  });
-
-  it('table should be visible when expanded is true', async () => {
+  it('table should be visible when expanded is true', () => {
+    const { container } = render(
+      <ThemeProvider theme={supersetTheme}>
+        <Provider store={store}>
+          <SqlEditorLeftBar {...mockedProps} />
+        </Provider>
+      </ThemeProvider>,
+    );
     const dbSelect = screen.getByText(/select a database/i);
     const schemaSelect = screen.getByText(/select a schema \(0\)/i);
     const dropdown = screen.getByText(/Select table/i);
@@ -89,5 +89,28 @@ describe('Left Panel Expansion', () => {
     expect(schemaSelect).toBeInTheDocument();
     expect(dropdown).toBeInTheDocument();
     expect(abUser).toBeInTheDocument();
+    expect(
+      container.querySelector('.ant-collapse-content-active'),
+    ).toBeInTheDocument();
+  });
+
+  it('should toggle the table when the header is clicked', async () => {
+    const collapseMock = jest.fn();
+    render(
+      <ThemeProvider theme={supersetTheme}>
+        <Provider store={store}>
+          <SqlEditorLeftBar
+            actions={{ ...mockedActions, collapseTable: collapseMock }}
+            tables={[table]}
+            queryEditor={defaultQueryEditor}
+            database={databases}
+            height={0}
+          />
+        </Provider>
+      </ThemeProvider>,
+    );
+    const header = screen.getByText(/ab_user/);
+    userEvent.click(header);
+    expect(collapseMock).toHaveBeenCalled();
   });
 });

superset-frontend/spec/javascripts/sqllab/TableElement_spec.jsx

@@ -118,22 +118,6 @@ describe('TableElement', () => {
       'active',
     );
   });
-  it('calls the collapseTable action', () => {
-    const wrapper = mount(
-      <Provider store={store}>
-        <TableElement {...mockedProps} />
-      </Provider>,
-      {
-        wrappingComponent: ThemeProvider,
-        wrappingComponentProps: {
-          theme: supersetTheme,
-        },
-      },
-    );
-    expect(mockedActions.collapseTable.called).toBe(false);
-    wrapper.find('[data-test="collapse"]').hostNodes().simulate('click');
-    expect(mockedActions.collapseTable.called).toBe(true);
-  });
   it('removes the table', () => {
     const wrapper = mount(
       <Provider store={store}>

superset-frontend/src/SqlLab/components/SouthPane/SouthPane.tsx

@@ -33,7 +33,7 @@ import {
   LOCALSTORAGE_MAX_QUERY_AGE_MS,
 } from '../../constants';
 
-const TAB_HEIGHT = 64;
+const TAB_HEIGHT = 90;
 
 /*
     editorQueries are queries executed by users passed from SqlEditor component
@@ -63,7 +63,6 @@ const StyledPane = styled.div`
     flex-direction: column;
   }
   .tab-content {
-    overflow: hidden;
     .alert {
       margin-top: ${({ theme }) => theme.gridUnit * 2}px;
     }

superset-frontend/src/SqlLab/components/SqlEditorLeftBar.jsx

@@ -59,6 +59,7 @@ export default class SqlEditorLeftBar extends React.PureComponent {
     this.onDbChange = this.onDbChange.bind(this);
     this.getDbList = this.getDbList.bind(this);
     this.onTableChange = this.onTableChange.bind(this);
+    this.onToggleTable = this.onToggleTable.bind(this);
   }
 
   onSchemaChange(schema) {
@@ -91,6 +92,16 @@ export default class SqlEditorLeftBar extends React.PureComponent {
     this.props.actions.addTable(this.props.queryEditor, tableName, schemaName);
   }
 
+  onToggleTable(tables) {
+    this.props.tables.forEach(table => {
+      if (!tables.includes(table.id.toString()) && table.expanded) {
+        this.props.actions.collapseTable(table);
+      } else if (tables.includes(table.id.toString()) && !table.expanded) {
+        this.props.actions.expandTable(table);
+      }
+    });
+  }
+
   getDbList(dbs) {
     this.props.actions.setDatabases(dbs);
   }
@@ -172,13 +183,13 @@ export default class SqlEditorLeftBar extends React.PureComponent {
               `}
               expandIconPosition="right"
               ghost
+              onChange={this.onToggleTable}
             >
               {this.props.tables.map(table => (
                 <TableElement
                   table={table}
                   key={table.id}
                   actions={this.props.actions}
-                  onClick={this.toggleTable}
                 />
               ))}
             </Collapse>

superset-frontend/src/SqlLab/components/TableElement.jsx

@@ -79,15 +79,6 @@ class TableElement extends React.PureComponent {
     this.props.actions.addQueryEditor(qe);
   }
 
-  toggleTable(e) {
-    e.preventDefault();
-    if (this.props.table.expanded) {
-      this.props.actions.collapseTable(this.props.table);
-    } else {
-      this.props.actions.expandTable(this.props.table);
-    }
-  }
-
   removeTable() {
     this.props.actions.removeDataPreview(this.props.table);
     this.props.actions.removeTable(this.props.table);
@@ -214,13 +205,7 @@ class TableElement extends React.PureComponent {
           title={table.name}
           trigger={['hover']}
         >
-          <StyledSpan
-            data-test="collapse"
-            className="table-name"
-            onClick={e => {
-              this.toggleTable(e);
-            }}
-          >
+          <StyledSpan data-test="collapse" className="table-name">
             <strong>{table.name}</strong>
           </StyledSpan>
         </Tooltip>

superset-frontend/src/SqlLab/main.less

@@ -56,7 +56,7 @@ body {
   position: relative;
   background-color: @lightest;
   overflow-x: auto;
-  overflow-y: hidden;
+  overflow-y: auto;
 
   > .ant-tabs-tabpane {
     position: absolute;
@@ -300,6 +300,7 @@ div.Workspace {
   .schemaPane-enter-done,
   .schemaPane-exit {
     transform: translateX(0);
+    z-index: 7;
   }
 
   .schemaPane-exit-active {

superset-frontend/src/components/ProgressBar/index.tsx

@@ -30,6 +30,10 @@ const ProgressBar = styled(({ striped, ...props }: ProgressBarProps) => (
   <AntdProgress {...props} />
 ))`
   line-height: 0;
+  position: static;
+  .ant-progress-inner {
+    position: static;
+  }
   .ant-progress-outer {
     ${({ percent }) => !percent && `display: none;`}
   }
@@ -37,6 +41,7 @@ const ProgressBar = styled(({ striped, ...props }: ProgressBarProps) => (
     font-size: ${({ theme }) => theme.typography.sizes.s}px;
   }
   .ant-progress-bg {
+    position: static;
     ${({ striped }) =>
       striped &&
       `

superset-frontend/src/datasource/DatasourceEditor.jsx

@@ -79,6 +79,13 @@ const FlexRowContainer = styled.div`
   }
 `;
 
+const StyledTableTabs = styled(Tabs)`
+  overflow: visible;
+  .ant-tabs-content-holder {
+    overflow: visible;
+  }
+`;
+
 const EditLockContainer = styled.div`
   font-size: ${supersetTheme.typography.sizes.s}px;
   display: flex;
@@ -995,7 +1002,7 @@ class DatasourceEditor extends React.PureComponent {
             </>
           }
         />
-        <Tabs
+        <StyledTableTabs
           fullWidth={false}
           id="table-tabs"
           data-test="edit-dataset-tabs"
@@ -1086,7 +1093,7 @@ class DatasourceEditor extends React.PureComponent {
               </Col>
             </div>
           </Tabs.TabPane>
-        </Tabs>
+        </StyledTableTabs>
       </DatasourceContainer>
     );
   }

superset-frontend/src/datasource/DatasourceModal.tsx

@@ -49,6 +49,10 @@ const StyledDatasourceModal = styled(Modal)`
   .modal-footer {
     flex: 0 1 auto;
   }
+
+  .ant-modal-body {
+    overflow: visible;
+  }
 `;
 
 interface DatasourceModalProps {

superset/config.py

@@ -359,6 +359,13 @@ DEFAULT_FEATURE_FLAGS: Dict[str, bool] = {
     "OMNIBAR": False,
     "DASHBOARD_RBAC": False,
     "ENABLE_EXPLORE_DRAG_AND_DROP": False,
+    # Enabling ALERTS_ATTACH_REPORTS, the system sends email and slack message
+    # with screenshot and link
+    # Disables ALERTS_ATTACH_REPORTS, the system DOES NOT generate screenshot
+    # for report with type 'alert' and sends email and slack message with only link;
+    # for report with type 'report' still send with email and slack message with
+    # screenshot and link
+    "ALERTS_ATTACH_REPORTS": True,
 }
 
 # Set the default view to card/grid view if thumbnail support is enabled.

superset/datasets/api.py

@@ -314,6 +314,8 @@ class DatasetRestApi(BaseSupersetModelRestApi):
             changed_model = UpdateDatasetCommand(
                 g.user, pk, item, override_columns
             ).run()
+            if override_columns:
+                RefreshDatasetCommand(g.user, pk).run()
             response = self.response(200, id=changed_model.id, result=item)
         except DatasetNotFoundError:
             response = self.response_404()

superset/reports/commands/execute.py

@@ -26,6 +26,7 @@ from sqlalchemy.orm import Session
 from superset import app
 from superset.commands.base import BaseCommand
 from superset.commands.exceptions import CommandException
+from superset.extensions import feature_flag_manager
 from superset.models.reports import (
     ReportExecutionLog,
     ReportSchedule,
@@ -52,7 +53,7 @@ from superset.reports.dao import (
     ReportScheduleDAO,
 )
 from superset.reports.notifications import create_notification
-from superset.reports.notifications.base import NotificationContent, ScreenshotData
+from superset.reports.notifications.base import NotificationContent
 from superset.reports.notifications.exceptions import NotificationError
 from superset.utils.celery import session_scope
 from superset.utils.screenshots import (
@@ -153,7 +154,7 @@ class BaseReportState:
             raise ReportScheduleSelleniumUserNotFoundError()
         return user
 
-    def _get_screenshot(self) -> ScreenshotData:
+    def _get_screenshot(self) -> bytes:
         """
         Get a chart or dashboard screenshot
 
@@ -176,7 +177,6 @@ class BaseReportState:
                 window_size=app.config["WEBDRIVER_WINDOW"]["dashboard"],
                 thumb_size=app.config["WEBDRIVER_WINDOW"]["dashboard"],
             )
-        image_url = self._get_url(user_friendly=True)
         user = self._get_screenshot_user()
         try:
             image_data = screenshot.get_screenshot(user=user)
@@ -188,7 +188,7 @@ class BaseReportState:
             )
         if not image_data:
             raise ReportScheduleScreenshotFailedError()
-        return ScreenshotData(url=image_url, image=image_data)
+        return image_data
 
     def _get_notification_content(self) -> NotificationContent:
         """
@@ -196,7 +196,19 @@ class BaseReportState:
 
         :raises: ReportScheduleScreenshotFailedError
         """
-        screenshot_data = self._get_screenshot()
+        screenshot_data = None
+        url = self._get_url(user_friendly=True)
+        if (
+            feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS")
+            or self._report_schedule.type == ReportScheduleType.REPORT
+        ):
+            screenshot_data = self._get_screenshot()
+            if not screenshot_data:
+                return NotificationContent(
+                    name=self._report_schedule.name,
+                    text="Unexpected missing screenshot",
+                )
+
         if self._report_schedule.chart:
             name = (
                 f"{self._report_schedule.name}: "
@@ -207,7 +219,7 @@ class BaseReportState:
                 f"{self._report_schedule.name}: "
                 f"{self._report_schedule.dashboard.dashboard_title}"
             )
-        return NotificationContent(name=name, screenshot=screenshot_data)
+        return NotificationContent(name=name, url=url, screenshot=screenshot_data)
 
     def _send(self, notification_content: NotificationContent) -> None:
         """

superset/reports/notifications/base.py

@@ -21,16 +21,11 @@ from typing import Any, List, Optional, Type
 from superset.models.reports import ReportRecipients, ReportRecipientType
 
 
-@dataclass
-class ScreenshotData:
-    url: str  # url to chart/dashboard for this screenshot
-    image: bytes  # bytes for the screenshot
-
-
 @dataclass
 class NotificationContent:
     name: str
-    screenshot: Optional[ScreenshotData] = None
+    url: Optional[str] = None  # url to chart/dashboard for this screenshot
+    screenshot: Optional[bytes] = None  # bytes for the screenshot
     text: Optional[str] = None
 
 

superset/reports/notifications/email.py

@@ -63,21 +63,21 @@ class EmailNotification(BaseNotification):  # pylint: disable=too-few-public-met
             return EmailContent(body=self._error_template(self._content.text))
         # Get the domain from the 'From' address ..
         # and make a message id without the < > in the end
+        image = None
+        domain = self._get_smtp_domain()
+        msgid = make_msgid(domain)[1:-1]
+        body = __(
+            """
+            <b><a href="%(url)s">Explore in Superset</a></b><p></p>
+            <img src="cid:%(msgid)s">
+            """,
+            url=self._content.url,
+            msgid=msgid,
+        )
         if self._content.screenshot:
-            domain = self._get_smtp_domain()
-            msgid = make_msgid(domain)[1:-1]
+            image = {msgid: self._content.screenshot}
 
-            image = {msgid: self._content.screenshot.image}
-            body = __(
-                """
-                <b><a href="%(url)s">Explore in Superset</a></b><p></p>
-                <img src="cid:%(msgid)s">
-                """,
-                url=self._content.screenshot.url,
-                msgid=msgid,
-            )
-            return EmailContent(body=body, images=image)
-        return EmailContent(body=self._error_template("Unexpected missing screenshot"))
+        return EmailContent(body=body, images=image)
 
     def _get_subject(self) -> str:
         return __(

superset/reports/notifications/slack.py

@@ -57,20 +57,18 @@ class SlackNotification(BaseNotification):  # pylint: disable=too-few-public-met
     def _get_body(self) -> str:
         if self._content.text:
             return self._error_template(self._content.name, self._content.text)
-        if self._content.screenshot:
-            return __(
-                """
-                *%(name)s*\n
-                <%(url)s|Explore in Superset>
-                """,
-                name=self._content.name,
-                url=self._content.screenshot.url,
-            )
-        return self._error_template(self._content.name, "Unexpected missing screenshot")
+        return __(
+            """
+            *%(name)s*\n
+            <%(url)s|Explore in Superset>
+            """,
+            name=self._content.name,
+            url=self._content.url,
+        )
 
     def _get_inline_screenshot(self) -> Optional[Union[str, IOBase, bytes]]:
         if self._content.screenshot:
-            return self._content.screenshot.image
+            return self._content.screenshot
         return None
 
     @retry(SlackApiError, delay=10, backoff=2, tries=5)

tests/datasets/api_tests.py

@@ -598,12 +598,7 @@ class TestDatasetApi(SupersetTestCase):
         rv = self.put_assert_metric(uri, dataset_data, "put")
         assert rv.status_code == 200
 
-        columns = (
-            db.session.query(TableColumn)
-            .filter_by(table_id=dataset.id)
-            .order_by("column_name")
-            .all()
-        )
+        columns = db.session.query(TableColumn).filter_by(table_id=dataset.id).all()
 
         assert columns[0].column_name == dataset_data["columns"][0]["column_name"]
         assert columns[0].description == dataset_data["columns"][0]["description"]

tests/reports/commands_tests.py

@@ -750,6 +750,10 @@ def test_email_dashboard_report_fails(
 )
 @patch("superset.reports.notifications.email.send_email_smtp")
 @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+@patch.dict(
+    "superset.extensions.feature_flag_manager._feature_flags",
+    ALERTS_ATTACH_REPORTS=True,
+)
 def test_slack_chart_alert(screenshot_mock, email_mock, create_alert_email_chart):
     """
     ExecuteReport Command: Test chart slack alert
@@ -773,6 +777,34 @@ def test_slack_chart_alert(screenshot_mock, email_mock, create_alert_email_chart
         assert_log(ReportState.SUCCESS)
 
 
+@pytest.mark.usefixtures(
+    "load_birth_names_dashboard_with_slices", "create_alert_email_chart"
+)
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch.dict(
+    "superset.extensions.feature_flag_manager._feature_flags",
+    ALERTS_ATTACH_REPORTS=False,
+)
+def test_slack_chart_alert_no_attachment(email_mock, create_alert_email_chart):
+    """
+    ExecuteReport Command: Test chart slack alert
+    """
+    # setup screenshot mock
+
+    with freeze_time("2020-01-01T00:00:00Z"):
+        AsyncExecuteReportScheduleCommand(
+            test_id, create_alert_email_chart.id, datetime.utcnow()
+        ).run()
+
+        notification_targets = get_target_from_report_schedule(create_alert_email_chart)
+        # Assert the email smtp address
+        assert email_mock.call_args[0][0] == notification_targets[0]
+        # Assert the there is no attached image
+        assert email_mock.call_args[1]["images"] is None
+        # Assert logs are correct
+        assert_log(ReportState.SUCCESS)
+
+
 @pytest.mark.usefixtures(
     "load_birth_names_dashboard_with_slices", "create_report_slack_chart"
 )
@@ -859,6 +891,10 @@ def test_soft_timeout_alert(email_mock, create_alert_email_chart):
 )
 @patch("superset.reports.notifications.email.send_email_smtp")
 @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+@patch.dict(
+    "superset.extensions.feature_flag_manager._feature_flags",
+    ALERTS_ATTACH_REPORTS=True,
+)
 def test_soft_timeout_screenshot(screenshot_mock, email_mock, create_alert_email_chart):
     """
     ExecuteReport Command: Test soft timeout on screenshot
@@ -882,11 +918,11 @@ def test_soft_timeout_screenshot(screenshot_mock, email_mock, create_alert_email
 
 
 @pytest.mark.usefixtures(
-    "load_birth_names_dashboard_with_slices", "create_alert_email_chart"
+    "load_birth_names_dashboard_with_slices", "create_report_email_chart"
 )
 @patch("superset.reports.notifications.email.send_email_smtp")
 @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
-def test_fail_screenshot(screenshot_mock, email_mock, create_alert_email_chart):
+def test_fail_screenshot(screenshot_mock, email_mock, create_report_email_chart):
     """
     ExecuteReport Command: Test soft timeout on screenshot
     """
@@ -896,10 +932,10 @@ def test_fail_screenshot(screenshot_mock, email_mock, create_alert_email_chart):
     screenshot_mock.side_effect = Exception("Unexpected error")
     with pytest.raises(ReportScheduleScreenshotFailedError):
         AsyncExecuteReportScheduleCommand(
-            test_id, create_alert_email_chart.id, datetime.utcnow()
+            test_id, create_report_email_chart.id, datetime.utcnow()
         ).run()
 
-    notification_targets = get_target_from_report_schedule(create_alert_email_chart)
+    notification_targets = get_target_from_report_schedule(create_report_email_chart)
     # Assert the email smtp address, asserts a notification was sent with the error
     assert email_mock.call_args[0][0] == notification_targets[0]
 
@@ -908,6 +944,32 @@ def test_fail_screenshot(screenshot_mock, email_mock, create_alert_email_chart):
     )
 
 
+@pytest.mark.usefixtures(
+    "load_birth_names_dashboard_with_slices", "create_alert_email_chart"
+)
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch.dict(
+    "superset.extensions.feature_flag_manager._feature_flags",
+    ALERTS_ATTACH_REPORTS=False,
+)
+def test_email_disable_screenshot(email_mock, create_alert_email_chart):
+    """
+    ExecuteReport Command: Test soft timeout on screenshot
+    """
+
+    AsyncExecuteReportScheduleCommand(
+        test_id, create_alert_email_chart.id, datetime.utcnow()
+    ).run()
+
+    notification_targets = get_target_from_report_schedule(create_alert_email_chart)
+    # Assert the email smtp address, asserts a notification was sent with the error
+    assert email_mock.call_args[0][0] == notification_targets[0]
+    # Assert the there is no attached image
+    assert email_mock.call_args[1]["images"] is None
+
+    assert_log(ReportState.SUCCESS)
+
+
 @pytest.mark.usefixtures("create_invalid_sql_alert_email_chart")
 @patch("superset.reports.notifications.email.send_email_smtp")
 def test_invalid_sql_alert(email_mock, create_invalid_sql_alert_email_chart):