superset
superset copied to clipboard
fix(alerts/reports): removing duplicate notification method options
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
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.
/testenv up FEATURE_ALERT_REPORTS=true
@geido Ephemeral environment spinning up at http://34.220.95.81:8080. Credentials are admin
/admin
. Please allow several minutes for bootstrapping and startup.
Let's rebase this with master to unstuck CI
Ephemeral environment shutdown and build artifacts deleted.
@fisjac I think you're going to need to rebase to bring in the other required CI checks.