feat(Alerts and Reports): Modal redesign #26202

rtexelm · 2023-12-07T00:34:02Z

SUMMARY

This combines changes from cobrafish (CorbinBullard, rtexelm, fisjac) into one branch allowing for tests of the full redesign.
No changes have been made to the functioning purpose of the modal and no additional features have been included.

Design Proposal
Contributing PR
Contributing PR

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

AFTER:

TESTING INSTRUCTIONS

Open the Alerts & Reports page from the Settings drop menu
Add a new report/alert or edit a report/alert

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

codecov · 2023-12-07T21:04:18Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (60fe581) 67.15% compared to head (077a979) 69.52%.
Report is 9 commits behind head on master.

Files	Patch %	Lines
...end/src/features/alerts/components/NumberInput.tsx	40.00%	2 Missing and 1 partial ⚠️
...res/alerts/components/AlertReportCronScheduler.tsx	71.42%	1 Missing and 1 partial ⚠️
...eatures/alerts/components/ValidatedPanelHeader.tsx	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #26202      +/-   ##
==========================================
+ Coverage   67.15%   69.52%   +2.36%     
==========================================
  Files        1902     1905       +3     
  Lines       74441    74500      +59     
  Branches     8306     8324      +18     
==========================================
+ Hits        49994    51795    +1801     
+ Misses      22393    20649    -1744     
- Partials     2054     2056       +2

Flag	Coverage Δ
hive	`53.79% <100.00%> (?)`
javascript	`56.94% <96.00%> (+0.09%)`	⬆️
mysql	`78.00% <100.00%> (-0.02%)`	⬇️
postgres	`78.10% <100.00%> (-0.02%)`	⬇️
presto	`53.74% <100.00%> (?)`
python	`83.07% <100.00%> (+4.82%)`	⬆️
sqlite	`77.62% <100.00%> (-0.02%)`	⬇️
unit	`56.42% <100.00%> (?)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lilykuang · 2023-12-08T17:40:52Z

/testenv up FEATURE_ALERT_REPORTS=true

github-actions · 2023-12-08T17:42:49Z

@lilykuang Ephemeral environment spinning up at http://35.93.128.126:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

yousoph · 2024-01-10T17:58:05Z

/testenv up

github-actions · 2024-01-10T18:02:25Z

@yousoph Ephemeral environment spinning up at http://34.219.202.1:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

yousoph · 2024-01-10T18:18:02Z

/testenv up FEATURE_ALERT_REPORTS=true

github-actions · 2024-01-10T18:22:30Z

@yousoph Ephemeral environment spinning up at http://35.92.61.181:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

kgabryje · 2024-01-30T12:00:38Z

/testenv up FEATURE_ALERT_REPORTS=true

github-actions · 2024-01-30T12:05:13Z

@kgabryje Ephemeral environment spinning up at http://35.88.169.129:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Pipfile.lock

package-lock.json

superset-frontend/src/components/Button/index.tsx

superset-frontend/src/components/CronPicker/CronPicker.tsx

superset-frontend/src/features/alerts/components/NotificationMethod.tsx

superset-frontend/src/features/alerts/components/ValidatedPanelHeader.tsx

superset-frontend/src/features/alerts/types.ts

superset-frontend/src/features/alerts/AlertReportModal.tsx

superset-frontend/src/components/Button/index.tsx

superset-frontend/src/components/CronPicker/CronPicker.tsx

superset-frontend/src/features/alerts/AlertReportModal.test.tsx

geido · 2024-02-01T16:41:11Z

superset-frontend/src/features/alerts/AlertReportModal.test.tsx

+) => {
+  expect(element).toBeInTheDocument();
+  userEvent.type(element, `${value}{enter}`);
+  await waitFor(() => {


Let's try with findBy rather than waitFor

This works for instances where we are awaiting a unique element to be rendered; however, it doesn't work for tests in which we are awaiting the creation of a duplicate item such as when adding a second notification method.

await comboboxSelect( screen.getAllByRole('combobox', { name: /delivery method/i, })[1], 'Slack', () => screen.getAllByRole('textbox')[1], );

geido · 2024-02-01T16:41:56Z

superset-frontend/src/features/alerts/AlertReportModal.test.tsx

+describe('properly renders the modal', () => {
+  it('properly renders add alert text', () => {
+    const addAlertProps = generateMockedProps();
+    render(<AlertReportModal {...addAlertProps} />, { useRedux: true });


This is repeated in a bunch of places. Let's create a setup function that renders the modal and let's refer to that in every test

For each of the tests in this section, the AlertReportModal is rerendered with different mocked props. Does it still make sense to use a setup function in this scenario?

superset-frontend/src/features/alerts/AlertReportModal.tsx

superset-frontend/src/features/alerts/AlertReportModal.test.tsx

superset-frontend/src/features/alerts/AlertReportModal.tsx

geido · 2024-02-08T10:34:51Z

superset-frontend/src/features/alerts/components/NumberInput.tsx

+  onChange: (event: ChangeEvent<HTMLInputElement>) => void;
+}
+
+export default function NumberInput({


Wondering why are we introducing a component for a number input rather than using the one provided by Antdesign? Is there any particular case that cannot be handled?

@fisjac you might want to look at this one

superset-frontend/src/features/alerts/components/NotificationMethod.tsx

kgabryje · 2024-02-15T13:40:16Z

Scheduling section looks a bit different than on the designs

EDIT: looks like react-js-cron was bumped from version 1.2 to 2.1 a week ago and now we should import its stylesheet explicitly

superset-frontend/src/features/alerts/AlertReportModal.tsx

superset-frontend/src/features/alerts/buildErrorTooltipMessage.tsx

geido · 2024-02-19T13:59:42Z

/testenv up FEATURE_ALERT_REPORTS=true

github-actions · 2024-02-19T14:10:44Z

@geido Ephemeral environment spinning up at http://35.93.119.155:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

kgabryje · 2024-02-19T14:24:54Z

superset-frontend/src/features/alerts/AlertReportModal.tsx

+          </StyledInputContainer>
+          {isScreenshot && (
+            <StyledInputContainer
+              css={!isReport && contentType === 'chart' && noMarginBottom}


Nit: instead of all that logic for adding noMarginBottom styles, could we maybe just use something like :last-child in StyledInputContainer? That would simplify the logic and we could get rid of noMarginBottom everywhere.

If you agree, I'm ok with doing that as a cleanup follow up

kgabryje

LGTM! Thank you for your great work @fisjac @rtexelm and @CorbinBullard and addressing many rounds of reviews

geido · 2024-02-19T14:45:21Z

(Reproduced on Master) I think it would be great if the input would catch this before submitting to the backend

(Reproduced on Master) We should probably erase the value when setting the "Not null" option

(Reproduced on Master) There is an issue with setting up a custom width for the screenshot and then choosing the CSV option. The custom width param will still be sent to the backend and will generate a validation error if it is below 600px. That should be cleared.
(Reproduced on Master) Also, not sure why the input for the custom width does not behave like a standard numeric input (up and down arrows on focus).
- I am still not clear about the reason why we chose to use a custom component rather than the one offered by Antd? CC @fisjac
(Reproduced on Master) The grace period shows as optional. When editing the alert I found a value being set, even though I did not set it. When trying to erase it and save again, I will get the following error

(Reproduced on Master) When setting a log retention of value None, I am getting an error that it should not be none :)

(Reproduced on Master) When choosing a notification method, if I click by mistake on the same notification method, "Email" for instance, it will clear the email recipients. I believe it should clear them only if the notification method is different from the one selected.
Probably not in the scope of this PR, but it would be nice to have some validation around the email field

github-actions · 2024-02-19T15:28:44Z

Ephemeral environment shutdown and build artifacts deleted.

…-to-the-embedded-dashboard * master: (1182 commits) fix(ci): mypy pre-commit issues (apache#27161) feat(Alerts and Reports): Modal redesign (apache#26202) refactor: Migrate ErrorBoundary to typescript (apache#27143) chore(tests): Remove unnecessary explicit Flask-SQLAlchemy session expunges (apache#27136) fix(plugins): Apply dashboard filters to comparison query in BigNumber with Time Comparison chart (apache#27138) fix: Duplicated toast messages (apache#27135) docs: add Geotab to users list (apache#27134) fix: Plain error message when visiting a dashboard via permalink without permissions (apache#27132) fix: ID param for DELETE ssh_tunnel endpoint (apache#27130) chore(hail mary): Update package-lock.json via npm-audit-fix (apache#26693) chore: lower cryptography min version to 41.0.2 (apache#27129) docs(miscellaneous): Export Datasoruces: export datasources exports to ZIP (apache#27120) fix(pivot-table-v2): Added forgotten translation pivot table v2 (apache#22840) fix: RLS modal overflow (apache#27128) refactor: Updates some database columns to MediumText (apache#27119) fix: gevent upgrade to 23.9.1 (apache#27112) fix: removes old deprecated sqllab endpoints (apache#27117) feat(storybook): Co-habitating/Upgrading Storybooks to v7 (dependency madness ensues) (apache#26907) fix: bump grpcio, urllib3 and paramiko (apache#27124) chore(internet_port): added new ports and removed unnecessary string class (apache#27078) ...

Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: fisjac <jfisher9882@gmail.com> Co-authored-by: Corbin <corbindbullard@gmail.com> Co-authored-by: Lily Kuang <lily@preset.io> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

pull-request-size bot added the size/XXL label Dec 7, 2023

lilykuang closed this Dec 7, 2023

lilykuang reopened this Dec 7, 2023

lilykuang marked this pull request as ready for review December 7, 2023 18:07

lilykuang marked this pull request as draft December 7, 2023 18:07

lilykuang closed this Dec 7, 2023

lilykuang reopened this Dec 7, 2023

eschutho marked this pull request as ready for review December 7, 2023 18:11

eschutho marked this pull request as draft December 7, 2023 18:11

lilykuang changed the title ~~redesign(Alerts and Reports modal): Combine collective work into branch for testing~~ feat(Alerts and Reports modal): Combine collective work into branch for testing Dec 7, 2023

lilykuang changed the title ~~feat(Alerts and Reports modal): Combine collective work into branch for testing~~ feat(Alerts and Reports modal): Combine collective work into branch for testing [DO NOT MERGE] Dec 7, 2023

fisjac force-pushed the redesign/ar-modal/test branch from c5ea629 to eb69c23 Compare January 5, 2024 23:03

rtexelm force-pushed the redesign/ar-modal/test branch 2 times, most recently from 89281a6 to 1d031d8 Compare January 9, 2024 03:09

fisjac force-pushed the redesign/ar-modal/test branch from 3478a6a to 2c76c37 Compare January 25, 2024 19:38

apache deleted a comment from github-actions bot Jan 30, 2024

kgabryje requested changes Jan 31, 2024

View reviewed changes

geido reviewed Feb 1, 2024

View reviewed changes

superset-frontend/src/features/alerts/AlertReportModal.tsx Outdated Show resolved Hide resolved

superset-frontend/src/features/alerts/AlertReportModal.tsx Show resolved Hide resolved

superset-frontend/src/features/alerts/AlertReportModal.tsx Outdated Show resolved Hide resolved

fisjac force-pushed the redesign/ar-modal/test branch from 9b2db6c to 266ad29 Compare February 6, 2024 00:29

geido reviewed Feb 8, 2024

View reviewed changes

kgabryje reviewed Feb 15, 2024

View reviewed changes

superset-frontend/src/features/alerts/components/NotificationMethod.tsx Outdated Show resolved Hide resolved

kgabryje reviewed Feb 15, 2024

View reviewed changes

superset-frontend/src/features/alerts/AlertReportModal.tsx Outdated Show resolved Hide resolved

kgabryje reviewed Feb 15, 2024

View reviewed changes

superset-frontend/src/features/alerts/buildErrorTooltipMessage.tsx Show resolved Hide resolved

rtexelm added 2 commits February 15, 2024 13:03

Fix syntax, edit translastion, clean

80a603c

Merge to resolve conflicts with 'master'

b8b897c

rtexelm changed the title ~~feat(Alerts and Reports modal): Combine collective work into branch for testing [DO NOT MERGE]~~ feat(Alerts and Reports modal redesign): Combine collective work into branch for review Feb 15, 2024

rtexelm marked this pull request as ready for review February 15, 2024 18:45

fisjac and others added 8 commits February 15, 2024 14:31

fixing tooltip to show sections with errors only

d7b13f6

Fix default panel and tests, fix panel borders

d22236d

adding builErrorTooltipMessage unit tests

1ee8969

adding license

07801bf

test fix and lint

1ea30b1

Add cronpicker global styles, fix title, axe skipped tests

13ac2ef

Fix return after else

47f85c7

Refactor getTitleText

077a979

rtexelm requested a review from kgabryje February 16, 2024 19:48

eschutho changed the title ~~feat(Alerts and Reports modal redesign): Combine collective work into branch for review~~ feat(Alerts and Reports): Modal redesign Feb 16, 2024

kgabryje reviewed Feb 19, 2024

View reviewed changes

kgabryje approved these changes Feb 19, 2024

View reviewed changes

geido merged commit 601e62a into apache:master Feb 19, 2024
43 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Alerts and Reports): Modal redesign #26202

feat(Alerts and Reports): Modal redesign #26202

rtexelm commented Dec 7, 2023

codecov bot commented Dec 7, 2023 •

edited

Loading

lilykuang commented Dec 8, 2023

github-actions bot commented Dec 8, 2023

yousoph commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

yousoph commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

kgabryje commented Jan 30, 2024

github-actions bot commented Jan 30, 2024

geido Feb 1, 2024

fisjac Feb 6, 2024

geido Feb 1, 2024

fisjac Feb 5, 2024

geido Feb 8, 2024

rtexelm Feb 15, 2024

kgabryje commented Feb 15, 2024 •

edited

Loading

geido commented Feb 19, 2024

github-actions bot commented Feb 19, 2024

kgabryje Feb 19, 2024

kgabryje left a comment

geido commented Feb 19, 2024 •

edited

Loading

github-actions bot commented Feb 19, 2024

feat(Alerts and Reports): Modal redesign #26202

feat(Alerts and Reports): Modal redesign #26202

Conversation

rtexelm commented Dec 7, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Dec 7, 2023 • edited Loading

Codecov Report

lilykuang commented Dec 8, 2023

github-actions bot commented Dec 8, 2023

yousoph commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

yousoph commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

kgabryje commented Jan 30, 2024

github-actions bot commented Jan 30, 2024

geido Feb 1, 2024

Choose a reason for hiding this comment

fisjac Feb 6, 2024

Choose a reason for hiding this comment

geido Feb 1, 2024

Choose a reason for hiding this comment

fisjac Feb 5, 2024

Choose a reason for hiding this comment

geido Feb 8, 2024

Choose a reason for hiding this comment

rtexelm Feb 15, 2024

Choose a reason for hiding this comment

kgabryje commented Feb 15, 2024 • edited Loading

geido commented Feb 19, 2024

github-actions bot commented Feb 19, 2024

kgabryje Feb 19, 2024

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

geido commented Feb 19, 2024 • edited Loading

github-actions bot commented Feb 19, 2024

codecov bot commented Dec 7, 2023 •

edited

Loading

kgabryje commented Feb 15, 2024 •

edited

Loading

geido commented Feb 19, 2024 •

edited

Loading