Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(ReportModal): simplify state reducer and improve error handling #19942

Merged
merged 2 commits into from
May 5, 2022

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented May 4, 2022

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
  5. Create TS types based on backend enums

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

before.mp4

After

after.mp4

TESTING INSTRUCTIONS

Go to dashboard or chart page to create or edit a report

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@ktmud ktmud changed the title refactor(ReportModal): simplify state reducer refactor(ReportModal): simplify state reducer and improve error handling May 4, 2022
/**
* Types mirroring enums in `superset/reports/models.py`:
*/
export type ReportScheduleType = 'Alert' | 'Report';
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: update views/CURD/alert/types.ts to use these types

},
level: 'error',
message: 'Request timed out',
return;
Copy link
Member Author

Choose a reason for hiding this comment

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

Early return to reduce indentation.

{
code: 1000,
message: t('Issue 1000 - The dataset is too large to query.'),
},
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: timeout can happen in other API endpoints as well. We should probably move these error codes out of this generic helper function.

@@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

Just rename the variable to make it easier to understand...

@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #19942 (a6db08e) into master (902ac05) will decrease coverage by 0.17%.
The diff coverage is 76.19%.

❗ Current head a6db08e differs from pull request most recent head 00f574f. Consider uploading reports for the commit 00f574f to get more accurate results

@@            Coverage Diff             @@
##           master   #19942      +/-   ##
==========================================
- Coverage   66.27%   66.10%   -0.18%     
==========================================
  Files        1712     1712              
  Lines       63957    63962       +5     
  Branches     6720     6726       +6     
==========================================
- Hits        42390    42284     -106     
- Misses      19859    19966     +107     
- Partials     1708     1712       +4     
Flag Coverage Δ
hive ?
javascript 51.25% <70.58%> (-0.01%) ⬇️
mysql 81.98% <100.00%> (+<0.01%) ⬆️
postgres 82.03% <100.00%> (+<0.01%) ⬆️
presto ?
python 82.12% <100.00%> (-0.35%) ⬇️
sqlite 81.77% <100.00%> (+<0.01%) ⬆️
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...set-frontend/src/components/ReportModal/styles.tsx 87.87% <ø> (ø)
...frontend/src/dashboard/components/Header/index.jsx 60.92% <ø> (ø)
superset/reports/schemas.py 98.83% <ø> (ø)
...nts/ExploreAdditionalActionsMenu/ExploreReport.tsx 50.00% <50.00%> (-7.90%) ⬇️
...uperset-frontend/src/utils/getClientErrorObject.ts 63.15% <64.00%> (-8.28%) ⬇️
superset-frontend/src/reports/actions/reports.js 36.36% <75.00%> (-3.04%) ⬇️
...rset-frontend/src/components/ReportModal/index.tsx 82.53% <75.67%> (+4.07%) ⬆️
superset/models/reports.py 100.00% <100.00%> (ø)
superset/reports/commands/base.py 92.30% <100.00%> (ø)
superset/reports/commands/create.py 89.18% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 902ac05...00f574f. Read the comment docs.

@ktmud ktmud force-pushed the report-modal-refactor branch 3 times, most recently from 06ecb41 to 9354f12 Compare May 4, 2022 18:02
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

a couple comments, but this makes sense to me.

/**
* Types mirroring enums in `superset/reports/models.py`:
*/
export type ReportScheduleType = 'Alert' | 'Report';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using the same variables as in the backend but maybe we can change the backend as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Hello, I think that I do add these to the backend in this refactor of the report modal.

#19130

// 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] ||
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Object.values(error.message as Record<string, string[]>)[0]?.[0] ||
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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@ktmud ktmud merged commit 7b88ec7 into apache:master May 5, 2022
hughhhh pushed a commit to hve-labs/superset that referenced this pull request May 11, 2022
@john-bodley john-bodley deleted the report-modal-refactor branch June 8, 2022 05:33
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XL 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants