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 2022/05/04 06:17:35 UTC
[GitHub] [superset] ktmud opened a new pull request, #19942: refactor(ReportModal): simplify state reducer
ktmud opened a new pull request, #19942:
URL: https://github.com/apache/superset/pull/19942
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Simplify the state reducer for `ReportModal` and improve error handling:
1. Reduce indirectness in the state reducer
2. Refactor `getClientError` to more consistently handle validation `message` from Marshmallow (set the default `"error"` message to be the first error of the first validated field).
3. Update the error message for name uniqueness to include the referred duplicate name, to make it sound more friendly
4. Editing a report will also display errors in an inline Alert instead of in a toast
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
#### Before
https://user-images.githubusercontent.com/335541/166630300-41eb948b-3b72-4d79-a238-22da7908731a.mp4
#### After
https://user-images.githubusercontent.com/335541/166630843-305bd2fa-4f5f-4c93-886e-db57387cb098.mp4
### TESTING INSTRUCTIONS
Go to dashboard or chart page to create or edit a report
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] codecov[bot] commented on pull request #19942: refactor(ReportModal): simplify state reducer and improve error handling
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19942:
URL: https://github.com/apache/superset/pull/19942#issuecomment-1116968636
# [Codecov](https://codecov.io/gh/apache/superset/pull/19942?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19942](https://codecov.io/gh/apache/superset/pull/19942?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (078c737) into [master](https://codecov.io/gh/apache/superset/commit/24e4ab6a1fb8f3e2a17e355a4cbeea6969e72728?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (24e4ab6) will **decrease** coverage by `12.74%`.
> The diff coverage is `67.21%`.
```diff
@@ Coverage Diff @@
## master #19942 +/- ##
===========================================
- Coverage 66.52% 53.78% -12.75%
===========================================
Files 1714 1714
Lines 65079 65081 +2
Branches 6725 6725
===========================================
- Hits 43294 35003 -8291
- Misses 20073 28366 +8293
Partials 1712 1712
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.76% <27.27%> (-0.01%)` | :arrow_down: |
| python | `56.41% <27.27%> (-26.00%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `48.03% <27.27%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19942?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/components/ReportModal/styles.tsx](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvUmVwb3J0TW9kYWwvc3R5bGVzLnRzeA==) | `87.87% <ø> (ø)` | |
| [...frontend/src/dashboard/components/Header/index.jsx](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0hlYWRlci9pbmRleC5qc3g=) | `60.92% <ø> (ø)` | |
| [superset/reports/commands/create.py](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9jcmVhdGUucHk=) | `25.67% <0.00%> (-63.52%)` | :arrow_down: |
| [superset/reports/commands/update.py](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy91cGRhdGUucHk=) | `28.98% <0.00%> (-60.87%)` | :arrow_down: |
| [superset/reports/dao.py](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9kYW8ucHk=) | `37.60% <0.00%> (-45.87%)` | :arrow_down: |
| [superset/reports/commands/exceptions.py](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGNlcHRpb25zLnB5) | `87.50% <42.85%> (-11.42%)` | :arrow_down: |
| [...nts/ExploreAdditionalActionsMenu/ExploreReport.tsx](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQWRkaXRpb25hbEFjdGlvbnNNZW51L0V4cGxvcmVSZXBvcnQudHN4) | `57.89% <50.00%> (ø)` | |
| [...uperset-frontend/src/utils/getClientErrorObject.ts](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL2dldENsaWVudEVycm9yT2JqZWN0LnRz) | `71.42% <66.66%> (ø)` | |
| [...rset-frontend/src/components/ReportModal/index.tsx](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvUmVwb3J0TW9kYWwvaW5kZXgudHN4) | `78.46% <84.00%> (ø)` | |
| [superset-frontend/src/reports/actions/reports.js](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3JlcG9ydHMvYWN0aW9ucy9yZXBvcnRzLmpz) | `39.39% <100.00%> (ø)` | |
| ... and [284 more](https://codecov.io/gh/apache/superset/pull/19942/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19942?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19942?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [24e4ab6...078c737](https://codecov.io/gh/apache/superset/pull/19942?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] etr2460 commented on a diff in pull request #19942: refactor(ReportModal): simplify state reducer and improve error handling
Posted by GitBox <gi...@apache.org>.
etr2460 commented on code in PR #19942:
URL: https://github.com/apache/superset/pull/19942#discussion_r865402255
##########
superset-frontend/src/reports/types.ts:
##########
@@ -0,0 +1,29 @@
+/**
+ * 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.
+ */
+
+/**
+ * Types mirroring enums in `superset/reports/models.py`:
+ */
+export type ReportScheduleType = 'Alert' | 'Report';
Review Comment:
is appending `Type` to the end of all these types standard? I could see it for `ReportScheduleType` (since it's defining what type of report it is), but seems like we could just use `ReportCreationMethod` and `ReportRecipient` for the other 2
##########
superset-frontend/src/utils/getClientErrorObject.ts:
##########
@@ -48,7 +48,13 @@ export function parseErrorJson(responseObject: JsonObject): ClientErrorObject {
error.error = error.description = error.errors[0].message;
error.link = error.errors[0]?.extra?.link;
}
-
+ // Marshmallow field validation returns the error mssage in the format
+ // of { message: { field1: [msg1, msg2], field2: [msg], } }
+ if (typeof error.message === 'object' && !error.error) {
+ error.error =
+ Object.values(error.message as Record<string, string[]>)[0]?.[0] ||
Review Comment:
```suggestion
Object.values(error.message as Record<string, string[]>)?.[0]?.[0] ||
```
Maybe it'll never happen, but i've found being cautious about parsing errors to be a good idea (since the backend hasn't standardized on it yet)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] ktmud commented on a diff in pull request #19942: refactor(ReportModal): simplify state reducer and improve error handling
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #19942:
URL: https://github.com/apache/superset/pull/19942#discussion_r866043380
##########
superset-frontend/src/reports/types.ts:
##########
@@ -0,0 +1,29 @@
+/**
+ * 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.
+ */
+
+/**
+ * Types mirroring enums in `superset/reports/models.py`:
+ */
+export type ReportScheduleType = 'Alert' | 'Report';
Review Comment:
I think I'll change `ReportCreationMethodType` but keep `ReportRecipientType`, as `ReportRecipient` sounds like _one_ recipient where both a recipient type and relevant metadata have been configured.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] ktmud commented on a diff in pull request #19942: refactor(ReportModal): simplify state reducer and improve error handling
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #19942:
URL: https://github.com/apache/superset/pull/19942#discussion_r865424527
##########
superset-frontend/src/reports/types.ts:
##########
@@ -0,0 +1,29 @@
+/**
+ * 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.
+ */
+
+/**
+ * Types mirroring enums in `superset/reports/models.py`:
+ */
+export type ReportScheduleType = 'Alert' | 'Report';
Review Comment:
I'm using the same variables as in the backend but maybe we can change the backend as well.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] ktmud commented on a diff in pull request #19942: refactor(ReportModal): simplify state reducer and improve error handling
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #19942:
URL: https://github.com/apache/superset/pull/19942#discussion_r864486705
##########
superset-frontend/src/reports/types.ts:
##########
@@ -0,0 +1,29 @@
+/**
+ * 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.
+ */
+
+/**
+ * Types mirroring enums in `superset/reports/models.py`:
+ */
+export type ReportScheduleType = 'Alert' | 'Report';
Review Comment:
TODO: update `views/CURD/alert/types.ts` to use these types
##########
superset-frontend/src/utils/getClientErrorObject.ts:
##########
@@ -68,78 +74,95 @@ export function parseErrorJson(responseObject: JsonObject): ClientErrorObject {
}
export function getClientErrorObject(
- response: SupersetClientResponse | ResponseWithTimeout | string,
+ response:
+ | SupersetClientResponse
+ | TimeoutError
+ | { response: Response }
+ | string,
): Promise<ClientErrorObject> {
// takes a SupersetClientResponse as input, attempts to read response as Json if possible,
// and returns a Promise that resolves to a plain object with error key and text value.
return new Promise(resolve => {
if (typeof response === 'string') {
resolve({ error: response });
- } else {
- const responseObject =
- response instanceof Response ? response : response.response;
- if (responseObject && !responseObject.bodyUsed) {
- // attempt to read the body as json, and fallback to text. we must clone the
- // response in order to fallback to .text() because Response is single-read
- responseObject
- .clone()
- .json()
- .then(errorJson => {
- const error = { ...responseObject, ...errorJson };
- resolve(parseErrorJson(error));
- })
- .catch(() => {
- // fall back to reading as text
- responseObject.text().then((errorText: any) => {
- resolve({ ...responseObject, error: errorText });
- });
- });
- } else if (
- 'statusText' in response &&
- response.statusText === 'timeout' &&
- 'timeout' in response
- ) {
- resolve({
- ...responseObject,
- error: 'Request timed out',
- errors: [
- {
- error_type: ErrorTypeEnum.FRONTEND_TIMEOUT_ERROR,
- extra: {
- timeout: response.timeout / 1000,
- issue_codes: [
- {
- code: 1000,
- message: t(
- 'Issue 1000 - The dataset is too large to query.',
- ),
- },
- {
- code: 1001,
- message: t(
- 'Issue 1001 - The database is under an unusual load.',
- ),
- },
- ],
- },
- level: 'error',
- message: 'Request timed out',
+ return;
Review Comment:
Early return to reduce indentation.
##########
superset/reports/dao.py:
##########
@@ -133,23 +134,24 @@ def validate_unique_creation_method(
@staticmethod
def validate_update_uniqueness(
- name: str, report_type: str, report_schedule_id: Optional[int] = None
+ name: str, report_type: ReportScheduleType, expect_id: Optional[int] = None
Review Comment:
Just rename the variable to make it easier to understand...
##########
superset-frontend/src/utils/getClientErrorObject.ts:
##########
@@ -68,78 +74,95 @@ export function parseErrorJson(responseObject: JsonObject): ClientErrorObject {
}
export function getClientErrorObject(
- response: SupersetClientResponse | ResponseWithTimeout | string,
+ response:
+ | SupersetClientResponse
+ | TimeoutError
+ | { response: Response }
+ | string,
): Promise<ClientErrorObject> {
// takes a SupersetClientResponse as input, attempts to read response as Json if possible,
// and returns a Promise that resolves to a plain object with error key and text value.
return new Promise(resolve => {
if (typeof response === 'string') {
resolve({ error: response });
- } else {
- const responseObject =
- response instanceof Response ? response : response.response;
- if (responseObject && !responseObject.bodyUsed) {
- // attempt to read the body as json, and fallback to text. we must clone the
- // response in order to fallback to .text() because Response is single-read
- responseObject
- .clone()
- .json()
- .then(errorJson => {
- const error = { ...responseObject, ...errorJson };
- resolve(parseErrorJson(error));
- })
- .catch(() => {
- // fall back to reading as text
- responseObject.text().then((errorText: any) => {
- resolve({ ...responseObject, error: errorText });
- });
- });
- } else if (
- 'statusText' in response &&
- response.statusText === 'timeout' &&
- 'timeout' in response
- ) {
- resolve({
- ...responseObject,
- error: 'Request timed out',
- errors: [
- {
- error_type: ErrorTypeEnum.FRONTEND_TIMEOUT_ERROR,
- extra: {
- timeout: response.timeout / 1000,
- issue_codes: [
- {
- code: 1000,
- message: t(
- 'Issue 1000 - The dataset is too large to query.',
- ),
- },
- {
- code: 1001,
- message: t(
- 'Issue 1001 - The database is under an unusual load.',
- ),
- },
- ],
- },
- level: 'error',
- message: 'Request timed out',
+ return;
+ }
+
+ if (
+ response instanceof TypeError &&
+ response.message === 'Failed to fetch'
+ ) {
+ resolve({
+ error: t('Network error'),
+ });
+ return;
+ }
+
+ if (
+ 'timeout' in response &&
+ 'statusText' in response &&
+ response.statusText === 'timeout'
+ ) {
+ resolve({
+ ...response,
+ error: t('Request timed out'),
+ errors: [
+ {
+ error_type: ErrorTypeEnum.FRONTEND_TIMEOUT_ERROR,
+ extra: {
+ timeout: response.timeout / 1000,
+ issue_codes: [
+ {
+ code: 1000,
+ message: t('Issue 1000 - The dataset is too large to query.'),
+ },
Review Comment:
TODO: timeout can happen in other API endpoints as well. We should probably move these error codes out of this generic helper function.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] ktmud merged pull request #19942: refactor(ReportModal): simplify state reducer and improve error handling
Posted by GitBox <gi...@apache.org>.
ktmud merged PR #19942:
URL: https://github.com/apache/superset/pull/19942
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] AAfghahi commented on a diff in pull request #19942: refactor(ReportModal): simplify state reducer and improve error handling
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #19942:
URL: https://github.com/apache/superset/pull/19942#discussion_r866139042
##########
superset-frontend/src/reports/types.ts:
##########
@@ -0,0 +1,29 @@
+/**
+ * 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.
+ */
+
+/**
+ * Types mirroring enums in `superset/reports/models.py`:
+ */
+export type ReportScheduleType = 'Alert' | 'Report';
Review Comment:
Hello, I think that I do add these to the backend in this refactor of the report modal.
https://github.com/apache/superset/pull/19130
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] ktmud commented on a diff in pull request #19942: refactor(ReportModal): simplify state reducer and improve error handling
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #19942:
URL: https://github.com/apache/superset/pull/19942#discussion_r866046264
##########
superset-frontend/src/utils/getClientErrorObject.ts:
##########
@@ -48,7 +48,13 @@ export function parseErrorJson(responseObject: JsonObject): ClientErrorObject {
error.error = error.description = error.errors[0].message;
error.link = error.errors[0]?.extra?.link;
}
-
+ // Marshmallow field validation returns the error mssage in the format
+ // of { message: { field1: [msg1, msg2], field2: [msg], } }
+ if (typeof error.message === 'object' && !error.error) {
+ error.error =
+ Object.values(error.message as Record<string, string[]>)[0]?.[0] ||
Review Comment:
`Object.values()` should always return an array as long as the input object exists. I can add another check in L53 to make sure `error.message` is not `null`.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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