-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
feat: report/alert list CRUD view #11802
Conversation
aa16e16
to
549f736
Compare
}, | ||
{ | ||
Header: t('Schedule'), | ||
accessor: 'crontab', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This column will be updated later and keeping it as plain text column for now
b4c85ed
to
429cf67
Compare
Codecov Report
@@ Coverage Diff @@
## master #11802 +/- ##
==========================================
- Coverage 67.61% 60.76% -6.85%
==========================================
Files 924 880 -44
Lines 44833 43135 -1698
Branches 4251 3785 -466
==========================================
- Hits 30313 26213 -4100
- Misses 14417 16922 +2505
+ Partials 103 0 -103
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
0e07dcb
to
ead0c44
Compare
4beb1b3
to
87d21aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one small typing proposal.
color: ${({ status, theme }) => { | ||
if (status === 'alerting') return '#FBC700'; | ||
if (status === 'failed') return theme.colors.error.base; | ||
if (status === 'ok') return theme.colors.success.base; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use enum
for status here, potentially with a switch?
@@ -40,6 +41,7 @@ export function useListViewResource<D extends object = any>( | |||
handleErrorMsg: (errorMsg: string) => void, | |||
infoEnable = true, | |||
defaultCollectionValue: D[] = [], | |||
baseFilters: FilterValue[] = [], // must memolize otherwise infinite loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baseFilters: FilterValue[] = [], // must memolize otherwise infinite loop | |
baseFilters: FilterValue[] = [], // must be memoized |
superset/app.py
Outdated
if feature_flag_manager.is_feature_enabled("SIP_34_QUERY_SEARCH_UI"): | ||
appbuilder.add_view_no_menu(AlertReportModelView) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the correct feature flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ops! nice catch
87d21aa
to
c2908ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. needs a rebase before merge though
99ae01d
to
fca0fb6
Compare
f9c3349
to
009f0ea
Compare
SUMMARY
/api/v1/report
type
Switch
and it to storybookSIP_34_ALERTS_UI
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION