feat: alerts/reports add/edit modal #11770

riahk · 2020-11-21T00:30:38Z

SUMMARY

Add AlertReportModal
Add components for ant-d Select and Radio

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Add spec file for AlertReportModal

ADDITIONAL INFORMATION

codecov-io · 2020-11-21T00:43:21Z

Codecov Report

Merging #11770 (5b15176) into master (2769de3) will decrease coverage by 6.67%.
The diff coverage is 5.89%.

@@            Coverage Diff             @@
##           master   #11770      +/-   ##
==========================================
- Coverage   67.23%   60.56%   -6.68%     
==========================================
  Files         948      906      -42     
  Lines       46177    44641    -1536     
  Branches     4405     4038     -367     
==========================================
- Hits        31046    27035    -4011     
- Misses      15019    17606    +2587     
+ Partials      112        0     -112

Flag	Coverage Δ
cypress	`53.17% <5.42%> (+3.86%)`	⬆️
javascript	`?`
python	`64.54% <66.66%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
...set-frontend/src/common/components/Modal/Modal.tsx	`100.00% <ø> (ø)`
superset-frontend/src/common/components/index.tsx	`100.00% <ø> (ø)`
superset-frontend/src/middleware/asyncEvent.ts	`26.38% <ø> (-61.77%)`	⬇️
...perset-frontend/src/views/CRUD/alert/AlertList.tsx	`1.51% <0.00%> (-80.68%)`	⬇️
superset-frontend/src/views/CRUD/alert/types.ts	`100.00% <ø> (ø)`
superset-frontend/src/views/CRUD/hooks.ts	`59.60% <0.00%> (+2.72%)`	⬆️
superset/reports/api.py	`84.21% <ø> (ø)`
...frontend/src/views/CRUD/alert/AlertReportModal.tsx	`4.15% <4.15%> (ø)`
superset-frontend/src/common/components/Radio.tsx	`37.50% <37.50%> (ø)`
superset-frontend/src/common/components/Select.tsx	`60.00% <60.00%> (ø)`
... and 354 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 2769de3...f2b2556. Read the comment docs.

superset/config.py

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

nytai · 2020-12-09T00:47:34Z

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

+              }`,
+            })),
+            // @ts-ignore: Type not assignable
+            validator_config_json: JSON.parse(resource.validator_config_json),


if you assign this to a var with a type, you should be able to assign it. Eg,

const validatorConfig: AlertObject["validator_config_json"] = JSON.parse(resource.validator_config_json) ... validator_config_json: validatorConfig

or (JSON.parse(resource.validator_config_json) as AlertObject["validator_config_json"])

I think the issue here if more that resource is expecting validator_config_json to be an object, not a string. I considered adding string to the type definition but it ended up creating more errors/checks I would need to handle, so ignoring this one case with the stringified response felt like less work in the long run.

right, I don't think having to always serialize the validator config object to json before adding it to an AlertObject is desirable or practical. This suggestion is to get rid of the ts-ignore comment by casting the output of the json parse to the desired type or to store it in typed var.

https://stackoverflow.com/questions/38688822/how-to-parse-json-string-in-typescript

nytai · 2020-12-09T00:53:01Z

superset/reports/commands/update.py

+        if "validator_config_json" in self._properties:
+            self._properties["validator_config_json"] = json.dumps(
+                self._properties["validator_config_json"]
+            )
+


wondering if we should handle this client side before we make the request. It is confusing having json inside json. Or perhaps add a check to see if validator_config_json is already a json string before running this, that way the api would support both (json string or json object).

@dpgaspar added this logic to make it easier for the FE (you can see it in the create logic already), by not requiring us to stringify our json beforehand, which I'm fine with. Do you see it as being an issue in the long run?

My main concern is with the api spec (ie, /swagger/v1). If it specifies a json string for this field a user will likely make an api request with a json string and possibly encounter a opaque error message from the api server.

I think that the OpenAPI spec is clear on this, it's just a nested structure, this happens on other places also. I think it's more elegant then sending a JSON string for a specific field

nytai · 2020-12-09T01:57:47Z

We should probably set a min-width and overflow-x: auto on the modal.

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

nytai · 2020-12-10T07:31:52Z

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

@@ -0,0 +1,1317 @@
+/**


this is a very large file, it's kind of hard to parse out the various pieces. Moving the styled components into a seperate file would help. Separating out some of the stateful logic from the jsx markup would also help.

We're planning another round of changes for this feature so we can wait to address this then.

nytai

LGTM

superset-github-bot bot added the preset-io label Nov 21, 2020

pull-request-size bot added the size/XXL label Nov 21, 2020

riahk force-pushed the moriah/alert-report-modal branch from 76f8d03 to 426d4bd Compare December 3, 2020 18:29

riahk marked this pull request as ready for review December 7, 2020 22:19

riahk force-pushed the moriah/alert-report-modal branch from 8f0c15c to ecc5506 Compare December 7, 2020 22:43

nytai reviewed Dec 9, 2020

View reviewed changes

superset/config.py Outdated Show resolved Hide resolved

nytai reviewed Dec 9, 2020

View reviewed changes

riahk force-pushed the moriah/alert-report-modal branch from f333a08 to 9d95e36 Compare December 9, 2020 22:23

riahk requested a review from nytai December 9, 2020 22:28

nytai reviewed Dec 10, 2020

View reviewed changes

riahk force-pushed the moriah/alert-report-modal branch from e5175ee to c22de64 Compare December 11, 2020 00:31

riahk added 17 commits December 11, 2020 11:34

alert list modal

671ee22

basic layout w/ placeholder inputs

5ba092d

add autocomplete input for owners

cd1e5e3

add radio and select components

826a117

add dashboard/chart select

73e92a7

notification settings

2eb340f

fix typescript errors, update alert format

4fc8ed2

fix edit mode display

0e9c150

fix duplicate import

bccd9a9

add spec file, rm python log

e5dca70

recipients logic

01e3b48

address comments, fix json parse errors

8b37517

add better error handling, fix styling for small screen widths

7999c7c

fix AlertList

e67b1e1

update operation -> op change

e836752

add working_timeout field

8dcd4cb

fix alert list tests

38f19ef

riahk force-pushed the moriah/alert-report-modal branch from c22de64 to 38f19ef Compare December 11, 2020 20:10

riahk added 2 commits December 11, 2020 13:49

fix tests, fix option translation

5b15176

fix liint

f2b2556

nytai approved these changes Dec 11, 2020

View reviewed changes

nytai merged commit 6963087 into apache:master Dec 11, 2020

nytai deleted the moriah/alert-report-modal branch December 11, 2020 23:50

This was referenced Jan 13, 2021

[explore]chart save modal scroll and dropdown #12469

Closed

fix: Jumpiness of datasource modal #12502

Closed

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: alerts/reports add/edit modal #11770

feat: alerts/reports add/edit modal #11770

riahk commented Nov 21, 2020 •

edited

Loading

codecov-io commented Nov 21, 2020 •

edited

Loading

nytai Dec 9, 2020

riahk Dec 9, 2020

nytai Dec 9, 2020

nytai Dec 9, 2020

riahk Dec 9, 2020

nytai Dec 9, 2020

dpgaspar Dec 9, 2020 •

edited

Loading

nytai commented Dec 9, 2020

nytai Dec 10, 2020

nytai left a comment

feat: alerts/reports add/edit modal #11770

feat: alerts/reports add/edit modal #11770

Conversation

riahk commented Nov 21, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Nov 21, 2020 • edited Loading

Codecov Report

nytai Dec 9, 2020

Choose a reason for hiding this comment

riahk Dec 9, 2020

Choose a reason for hiding this comment

nytai Dec 9, 2020

Choose a reason for hiding this comment

nytai Dec 9, 2020

Choose a reason for hiding this comment

riahk Dec 9, 2020

Choose a reason for hiding this comment

nytai Dec 9, 2020

Choose a reason for hiding this comment

dpgaspar Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

nytai commented Dec 9, 2020

nytai Dec 10, 2020

Choose a reason for hiding this comment

nytai left a comment

Choose a reason for hiding this comment

riahk commented Nov 21, 2020 •

edited

Loading

codecov-io commented Nov 21, 2020 •

edited

Loading

dpgaspar Dec 9, 2020 •

edited

Loading