superset icon indicating copy to clipboard operation
superset copied to clipboard

fix(alerts/reports): removing duplicate notification method options

Open fisjac opened this issue 1 year ago • 6 comments

SUMMARY

When adding notification methods to an alert or report, a user can select duplicate methods (select email twice or slack twice). This PR updates the behavior to exclude notification methods that are already being used.

Additionally, the delete notification method for additional methods only appears after a user has selected a notification method. This PR changes this so that the ability to delete additional notification methods is always available, even if the method has not yet been selected.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: https://www.loom.com/share/7bb024ba1d2a483eb6db44be6c137e32?sid=5620dc39-24aa-4165-8b12-39dddf6b48b5

After: https://www.loom.com/share/c4c9c68f4f0446f18f5e4cbf1064d052?sid=3caccb4a-e2ec-4cd4-989e-d34ec31e6b7e

TESTING INSTRUCTIONS

Ensure that within either your local superset_config.py or docker/pythonpath_dev/superset_config_docker.py files, you have a SLACK_API_TOKEN string filled in. It doesn't need to be an active api token, just any string.

Open superset and navigate to the alerts and reports section. Create an alert or report and open the Notification method collapse section. Observe that there are two options for notification method. Add a new notification method. Check the options of the newly added notification method which should exclude the previously added option.

Update the method of the first notification setting, and ensure that subsequent notification settings are being removed, and the add notification method button reappears.

ADDITIONAL INFORMATION

  • [ ] Has associated issue:
  • [x] Required feature flags: "ALERT_REPORTS": True
  • [x] 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

fisjac avatar Feb 23 '24 21:02 fisjac

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 69.91%. Comparing base (349e496) to head (59f9cdb). Report is 39 commits behind head on master.

Files Patch % Lines
...-frontend/src/features/alerts/AlertReportModal.tsx 58.33% 1 Missing and 4 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #27239      +/-   ##
==========================================
+ Coverage   69.77%   69.91%   +0.13%     
==========================================
  Files        1911     1913       +2     
  Lines       75056    75698     +642     
  Branches     8362     8638     +276     
==========================================
+ Hits        52374    52923     +549     
- Misses      20630    20684      +54     
- Partials     2052     2091      +39     
Flag Coverage Δ
javascript 57.93% <61.53%> (+0.44%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 23 '24 22:02 codecov[bot]

/testenv up FEATURE_ALERT_REPORTS=true

geido avatar Feb 28 '24 17:02 geido

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

github-actions[bot] avatar Feb 28 '24 17:02 github-actions[bot]

Let's rebase this with master to unstuck CI

geido avatar Mar 26 '24 18:03 geido

Ephemeral environment shutdown and build artifacts deleted.

github-actions[bot] avatar Mar 28 '24 15:03 github-actions[bot]

@fisjac I think you're going to need to rebase to bring in the other required CI checks.

eschutho avatar Apr 04 '24 23:04 eschutho