You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/07/30 07:26:39 UTC

[GitHub] [incubator-superset] etr2460 opened a new pull request #10473: feat: refactor error components and add database issue code

etr2460 opened a new pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473


   ### SUMMARY
   - Refactors the `TimeoutErrorMessage` into a generic `ErrorAlert` component
   - Adds the `DatabaseErrorMessage` component for handling all generic database errors
   - Uses the `ErrorAlert` component to handle all other errors, removing the need for the Bootstrap Alert
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   New UI:
   <img width="497" alt="Screen Shot 2020-07-29 at 10 52 07 AM" src="https://user-images.githubusercontent.com/7409244/88836624-d4425e00-d18b-11ea-9765-dc361a545bff.png">
   <img width="657" alt="Screen Shot 2020-07-29 at 10 52 14 AM" src="https://user-images.githubusercontent.com/7409244/88836625-d4425e00-d18b-11ea-8222-286b3843d806.png">
   <img width="1273" alt="Screen Shot 2020-07-29 at 10 52 42 AM" src="https://user-images.githubusercontent.com/7409244/88836630-d5738b00-d18b-11ea-8ba7-26d319e2efe8.png">
   <img width="1588" alt="Screen Shot 2020-07-29 at 11 07 55 AM" src="https://user-images.githubusercontent.com/7409244/88836632-d60c2180-d18b-11ea-9b40-22c1101fd4c3.png">
   <img width="486" alt="Screen Shot 2020-07-29 at 10 51 38 AM" src="https://user-images.githubusercontent.com/7409244/88836620-d3113100-d18b-11ea-8bd6-b01a2d9d3e72.png">
   
   ### TEST PLAN
   CI, force errors throughout dashboards, charts, and sql lab and see correct rendering, typecheck everything
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [x] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   to: @john-bodley @ktmud @nytai


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#discussion_r465450943



##########
File path: superset/db_engine_specs/base.py
##########
@@ -137,6 +137,7 @@ class BaseEngineSpec:  # pylint: disable=too-many-public-methods
     """Abstract class for database engine specific configurations"""
 
     engine = "base"  # str as defined in sqlalchemy.engine.engine
+    engine_name = "Base Database"

Review comment:
       sgtm, simply removing this line should be fine then right?
   
   ```suggestion
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] ktmud commented on a change in pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#discussion_r466556727



##########
File path: superset-frontend/src/components/ErrorMessage/ErrorAlert.tsx
##########
@@ -0,0 +1,203 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, ReactNode } from 'react';
+import { Modal } from 'react-bootstrap';
+import { styled, supersetTheme } from '@superset-ui/style';
+import { t } from '@superset-ui/translation';
+import { noOp } from 'src/utils/common';
+import Button from 'src/views/CRUD/dataset/Button';
+
+import Icon from '../Icon';
+import { ErrorLevel, ErrorSource } from './types';
+import CopyToClipboard from '../CopyToClipboard';
+
+const ErrorAlertDiv = styled.div<{ level: ErrorLevel }>`
+  align-items: center;
+  background-color: ${({ level, theme }) => theme.colors[level].light2};
+  border-radius: ${({ theme }) => theme.borderRadius}px;
+  border: 1px solid ${({ level, theme }) => theme.colors[level].base};
+  color: ${({ level, theme }) => theme.colors[level].dark2};
+  padding: ${({ theme }) => 2 * theme.gridUnit}px;
+  width: 100%;
+
+  .top-row {
+    display: flex;
+    justify-content: space-between;
+  }
+
+  .error-body {
+    padding-top: ${({ theme }) => theme.gridUnit}px;
+    padding-left: ${({ theme }) => 8 * theme.gridUnit}px;
+  }
+
+  .icon {
+    margin-right: ${({ theme }) => 2 * theme.gridUnit}px;
+  }
+
+  .link {
+    color: ${({ level, theme }) => theme.colors[level].dark2};
+    text-decoration: underline;
+  }
+`;
+
+const ErrorModal = styled(Modal)<{ level: ErrorLevel }>`
+  color: ${({ level, theme }) => theme.colors[level].dark2};
+
+  .icon {
+    margin-right: ${({ theme }) => 2 * theme.gridUnit}px;
+  }
+
+  .header {
+    align-items: center;
+    background-color: ${({ level, theme }) => theme.colors[level].light2};
+    display: flex;
+    justify-content: space-between;
+    font-size: ${({ theme }) => theme.typography.sizes.l}px;
+
+    // Remove clearfix hack as Superset is only used on modern browsers
+    ::before,
+    ::after {
+      content: unset;
+    }
+  }
+`;
+
+const LeftSideContent = styled.div`
+  align-items: center;
+  display: flex;
+`;
+
+interface ErrorAlertProps {
+  body: ReactNode;
+  copyText?: string;
+  level: ErrorLevel;
+  source?: ErrorSource;
+  subtitle: ReactNode;
+  title: ReactNode;
+}
+
+export default function ErrorAlert({
+  body,
+  copyText,
+  level,
+  source,
+  subtitle,
+  title,
+}: ErrorAlertProps) {
+  const [isModalOpen, setIsModalOpen] = useState(false);
+  const [isBodyExpanded, setIsBodyExpanded] = useState(false);
+
+  const isExpandable = (['explore', 'sqllab'] as (
+    | string
+    | undefined
+  )[]).includes(source);

Review comment:
       Same here

##########
File path: superset-frontend/src/components/ErrorMessage/DatabaseErrorMessage.tsx
##########
@@ -0,0 +1,94 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { t, tn } from '@superset-ui/translation';
+
+import { ErrorMessageComponentProps } from './types';
+import IssueCode from './IssueCode';
+import ErrorAlert from './ErrorAlert';
+
+interface DatabaseErrorExtra {
+  owners?: string[];
+  issue_codes: {
+    code: number;
+    message: string;
+  }[];
+  engine_name: string | null;
+}
+
+function DatabaseErrorMessage({
+  error,
+  source,
+}: ErrorMessageComponentProps<DatabaseErrorExtra>) {
+  const { extra, level, message } = error;
+
+  const isVisualization = (['dashboard', 'explore'] as (
+    | string
+    | undefined
+  )[]).includes(source);

Review comment:
       Typing here seems not needed. If source is optional, you can also give it a default value, otherwise maybe make it non-optional. There is actually a [eslint rule](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/require-default-props.md) in the PropTypes world.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#discussion_r466571699



##########
File path: superset-frontend/src/components/ErrorMessage/DatabaseErrorMessage.tsx
##########
@@ -0,0 +1,94 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { t, tn } from '@superset-ui/translation';
+
+import { ErrorMessageComponentProps } from './types';
+import IssueCode from './IssueCode';
+import ErrorAlert from './ErrorAlert';
+
+interface DatabaseErrorExtra {
+  owners?: string[];
+  issue_codes: {
+    code: number;
+    message: string;
+  }[];
+  engine_name: string | null;
+}
+
+function DatabaseErrorMessage({
+  error,
+  source,
+}: ErrorMessageComponentProps<DatabaseErrorExtra>) {
+  const { extra, level, message } = error;
+
+  const isVisualization = (['dashboard', 'explore'] as (
+    | string
+    | undefined
+  )[]).includes(source);

Review comment:
       Ah fair enough. Giving it a default value is a good call and will make it so we don't need to override the type




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#issuecomment-670143385


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=h1) Report
   > Merging [#10473](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9914ae1b527d176851f5720e5789ef30288bb41e&el=desc) will **decrease** coverage by `5.01%`.
   > The diff coverage is `36.28%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10473/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10473      +/-   ##
   ==========================================
   - Coverage   64.29%   59.27%   -5.02%     
   ==========================================
     Files         544      765     +221     
     Lines       30586    36189    +5603     
     Branches     2885     3424     +539     
   ==========================================
   + Hits        19665    21452    +1787     
   - Misses      10743    14544    +3801     
   - Partials      178      193      +15     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `59.80% <15.66%> (?)` | |
   | #python | `58.96% <93.33%> (-10.87%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...c/components/ErrorMessage/DatabaseErrorMessage.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRXJyb3JNZXNzYWdlL0RhdGFiYXNlRXJyb3JNZXNzYWdlLnRzeA==) | `0.00% <0.00%> (ø)` | |
   | [...rc/components/ErrorMessage/TimeoutErrorMessage.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRXJyb3JNZXNzYWdlL1RpbWVvdXRFcnJvck1lc3NhZ2UudHN4) | `0.00% <0.00%> (-6.67%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupErrorMessages.ts](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRXJyb3JNZXNzYWdlcy50cw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset/errors.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXJyb3JzLnB5) | `100.00% <ø> (+6.25%)` | :arrow_up: |
   | [...rontend/src/components/ErrorMessage/ErrorAlert.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRXJyb3JNZXNzYWdlL0Vycm9yQWxlcnQudHN4) | `22.44% <22.44%> (ø)` | |
   | [...onents/ErrorMessage/ErrorMessageWithStackTrace.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRXJyb3JNZXNzYWdlL0Vycm9yTWVzc2FnZVdpdGhTdGFja1RyYWNlLnRzeA==) | `29.41% <25.00%> (+29.41%)` | :arrow_up: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.56% <50.00%> (+0.31%)` | :arrow_up: |
   | [superset/db\_engine\_specs/athena.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2F0aGVuYS5weQ==) | `86.36% <100.00%> (+0.64%)` | :arrow_up: |
   | [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `87.35% <100.00%> (+2.80%)` | :arrow_up: |
   | [superset/db\_engine\_specs/bigquery.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2JpZ3F1ZXJ5LnB5) | `95.23% <100.00%> (+0.05%)` | :arrow_up: |
   | ... and [566 more](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=footer). Last update [9914ae1...929dcd3](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#issuecomment-666133571


   @etr2460 is the plan to migrate lesser error messages, like the "No Results" error to this component?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#issuecomment-666503032


   No results is a bit of an edge case, because I think we want to have individual chart plugins control the rendering of "No results". A simple use case is if you consider a table with 0 rows. The table viz still shows all the column headers with a message that states no rows exist, and I personally think that's a better UX than a generic message


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] john-bodley commented on a change in pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#discussion_r464633078



##########
File path: superset/db_engine_specs/athena.py
##########
@@ -23,6 +23,7 @@
 
 class AthenaEngineSpec(BaseEngineSpec):
     engine = "awsathena"
+    engine_name = "AWS Athena"

Review comment:
       ```suggestion
       engine_name = "Amazon Athena"
   ```

##########
File path: superset/db_engine_specs/cockroachdb.py
##########
@@ -19,3 +19,4 @@
 
 class CockroachDbEngineSpec(PostgresEngineSpec):
     engine = "cockroachdb"
+    engine_name = "Cockroach DB"

Review comment:
       ```suggestion
       engine_name = "CockroachDB"
   ```

##########
File path: superset/db_engine_specs/hana.py
##########
@@ -24,6 +24,7 @@
 
 class HanaEngineSpec(PostgresBaseEngineSpec):
     engine = "hana"
+    engine_name = "Hana"

Review comment:
       ```suggestion
       engine_name = "SAP HANA"
   ```

##########
File path: superset/db_engine_specs/teradata.py
##########
@@ -21,6 +21,7 @@ class TeradataEngineSpec(BaseEngineSpec):
     """Dialect for Teradata DB."""
 
     engine = "teradata"
+    engine_name = "Teradata DB"

Review comment:
       ```suggestion
       engine_name = "Teradata"
   ```

##########
File path: superset/db_engine_specs/postgres.py
##########
@@ -38,6 +38,7 @@ class PostgresBaseEngineSpec(BaseEngineSpec):
     """ Abstract class for Postgres 'like' databases """
 
     engine = ""
+    engine_name = "Postgres"

Review comment:
       ```suggestion
       engine_name = "PostgreSQL"
   ```

##########
File path: superset/db_engine_specs/redshift.py
##########
@@ -19,6 +19,7 @@
 
 class RedshiftEngineSpec(PostgresBaseEngineSpec):
     engine = "redshift"
+    engine_name = "Redshift"

Review comment:
       ```suggestion
       engine_name = "Amazon Redshift"
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#issuecomment-670143385


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=h1) Report
   > Merging [#10473](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9914ae1b527d176851f5720e5789ef30288bb41e&el=desc) will **decrease** coverage by `4.95%`.
   > The diff coverage is `36.28%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10473/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10473      +/-   ##
   ==========================================
   - Coverage   64.29%   59.34%   -4.96%     
   ==========================================
     Files         544      765     +221     
     Lines       30586    36198    +5612     
     Branches     2885     3424     +539     
   ==========================================
   + Hits        19665    21480    +1815     
   - Misses      10743    14525    +3782     
   - Partials      178      193      +15     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `59.80% <15.66%> (?)` | |
   | #python | `59.06% <93.33%> (-10.77%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...c/components/ErrorMessage/DatabaseErrorMessage.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRXJyb3JNZXNzYWdlL0RhdGFiYXNlRXJyb3JNZXNzYWdlLnRzeA==) | `0.00% <0.00%> (ø)` | |
   | [...rc/components/ErrorMessage/TimeoutErrorMessage.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRXJyb3JNZXNzYWdlL1RpbWVvdXRFcnJvck1lc3NhZ2UudHN4) | `0.00% <0.00%> (-6.67%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupErrorMessages.ts](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRXJyb3JNZXNzYWdlcy50cw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset/errors.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXJyb3JzLnB5) | `100.00% <ø> (+6.25%)` | :arrow_up: |
   | [...rontend/src/components/ErrorMessage/ErrorAlert.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRXJyb3JNZXNzYWdlL0Vycm9yQWxlcnQudHN4) | `22.44% <22.44%> (ø)` | |
   | [...onents/ErrorMessage/ErrorMessageWithStackTrace.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRXJyb3JNZXNzYWdlL0Vycm9yTWVzc2FnZVdpdGhTdGFja1RyYWNlLnRzeA==) | `29.41% <25.00%> (+29.41%)` | :arrow_up: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.56% <50.00%> (+0.31%)` | :arrow_up: |
   | [superset/db\_engine\_specs/athena.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2F0aGVuYS5weQ==) | `86.36% <100.00%> (+0.64%)` | :arrow_up: |
   | [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `87.64% <100.00%> (+3.08%)` | :arrow_up: |
   | [superset/db\_engine\_specs/bigquery.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2JpZ3F1ZXJ5LnB5) | `95.23% <100.00%> (+0.05%)` | :arrow_up: |
   | ... and [561 more](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=footer). Last update [9914ae1...929dcd3](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter commented on pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#issuecomment-670143385


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=h1) Report
   > Merging [#10473](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9914ae1b527d176851f5720e5789ef30288bb41e&el=desc) will **decrease** coverage by `5.32%`.
   > The diff coverage is `93.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10473/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10473      +/-   ##
   ==========================================
   - Coverage   64.29%   58.96%   -5.33%     
   ==========================================
     Files         544      354     -190     
     Lines       30586    22784    -7802     
     Branches     2885        0    -2885     
   ==========================================
   - Hits        19665    13435    -6230     
   + Misses      10743     9349    -1394     
   + Partials      178        0     -178     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #python | `58.96% <93.33%> (-10.87%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/errors.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXJyb3JzLnB5) | `100.00% <ø> (+6.25%)` | :arrow_up: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.56% <50.00%> (+0.31%)` | :arrow_up: |
   | [superset/db\_engine\_specs/athena.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2F0aGVuYS5weQ==) | `86.36% <100.00%> (+0.64%)` | :arrow_up: |
   | [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `87.35% <100.00%> (+2.80%)` | :arrow_up: |
   | [superset/db\_engine\_specs/bigquery.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2JpZ3F1ZXJ5LnB5) | `95.23% <100.00%> (+0.05%)` | :arrow_up: |
   | [superset/db\_engine\_specs/clickhouse.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2NsaWNraG91c2UucHk=) | `94.44% <100.00%> (+0.32%)` | :arrow_up: |
   | [superset/db\_engine\_specs/cockroachdb.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2NvY2tyb2FjaGRiLnB5) | `100.00% <100.00%> (ø)` | |
   | [superset/db\_engine\_specs/db2.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RiMi5weQ==) | `90.90% <100.00%> (+0.90%)` | :arrow_up: |
   | [superset/db\_engine\_specs/dremio.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RyZW1pby5weQ==) | `87.50% <100.00%> (+1.78%)` | :arrow_up: |
   | [superset/db\_engine\_specs/drill.py](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RyaWxsLnB5) | `82.14% <100.00%> (+0.66%)` | :arrow_up: |
   | ... and [578 more](https://codecov.io/gh/apache/incubator-superset/pull/10473/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=footer). Last update [9914ae1...929dcd3](https://codecov.io/gh/apache/incubator-superset/pull/10473?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 merged pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
etr2460 merged pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] ktmud commented on a change in pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#discussion_r466556166



##########
File path: superset-frontend/src/components/ErrorMessage/DatabaseErrorMessage.tsx
##########
@@ -0,0 +1,94 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { t, tn } from '@superset-ui/translation';
+
+import { ErrorMessageComponentProps } from './types';
+import IssueCode from './IssueCode';
+import ErrorAlert from './ErrorAlert';
+
+interface DatabaseErrorExtra {
+  owners?: string[];
+  issue_codes: {
+    code: number;
+    message: string;
+  }[];
+  engine_name: string | null;
+}
+
+function DatabaseErrorMessage({
+  error,
+  source,
+}: ErrorMessageComponentProps<DatabaseErrorExtra>) {
+  const { extra, level, message } = error;
+
+  const isVisualization = (['dashboard', 'explore'] as (
+    | string
+    | undefined
+  )[]).includes(source);

Review comment:
       Typing here seems not needed. If source is optional, you can also give it a default value, otherwise maybe make it non-optional. There is actually an [eslint rule](https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/require-default-props.md) for this in the PropTypes world.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] john-bodley commented on a change in pull request #10473: feat: refactor error components and add database issue code

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #10473:
URL: https://github.com/apache/incubator-superset/pull/10473#discussion_r464557230



##########
File path: superset/db_engine_specs/druid.py
##########
@@ -35,6 +35,7 @@ class DruidEngineSpec(BaseEngineSpec):  # pylint: disable=abstract-method
     """Engine spec for Druid.io"""
 
     engine = "druid"
+    engine_name = "Druid"

Review comment:
       ```suggestion
       engine_name = "Apache Druid"
   ```

##########
File path: superset/db_engine_specs/base.py
##########
@@ -137,6 +137,7 @@ class BaseEngineSpec:  # pylint: disable=too-many-public-methods
     """Abstract class for database engine specific configurations"""
 
     engine = "base"  # str as defined in sqlalchemy.engine.engine
+    engine_name = "Base Database"

Review comment:
       Can this be an optional `str`? No one should be instantiating the base class.

##########
File path: superset/db_engine_specs/pinot.py
##########
@@ -24,6 +24,7 @@
 
 class PinotEngineSpec(BaseEngineSpec):  # pylint: disable=abstract-method
     engine = "pinot"
+    engine_name = "Pinot"

Review comment:
       ```suggestion
       engine_name = "Apache Pinot"
   ```

##########
File path: superset/db_engine_specs/kylin.py
##########
@@ -25,6 +25,7 @@ class KylinEngineSpec(BaseEngineSpec):  # pylint: disable=abstract-method
     """Dialect for Apache Kylin"""
 
     engine = "kylin"
+    engine_name = "Kylin"

Review comment:
       ```suggestion
       engine_name = "Apache Kylin"
   ```

##########
File path: superset/db_engine_specs/hive.py
##########
@@ -55,6 +55,7 @@ class HiveEngineSpec(PrestoEngineSpec):
     """Reuses PrestoEngineSpec functionality."""
 
     engine = "hive"
+    engine_name = "Hive"

Review comment:
       ```suggestion
       engine_name = "Apache Hive"
   ```

##########
File path: superset/db_engine_specs/impala.py
##########
@@ -27,6 +27,7 @@ class ImpalaEngineSpec(BaseEngineSpec):
     """Engine spec for Cloudera's Impala"""
 
     engine = "impala"
+    engine_name = "Impala"

Review comment:
       ```suggestion
       engine_name = "Apache Impala"
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org