fix(alerts/reports): removing duplicate notification method options #27239
Conversation
fisjac
force-pushed
the
ar-notification-settings-duplicate
branch
from
fd19896
to
642b9d3
Compare
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| settings.pop(); | ||
| setNotificationAddState('active'); | ||
| } | ||
| } |
There was a problem hiding this comment.
as much as possible, I would recommend using array.map and filter for most of this logic instead of making copies of arrays and then adding and removing elements with slice, pop, etc.. It's ok to rewrite the existing patterns here.
There was a problem hiding this comment.
Changed the pattern to use filter rather than slice and pop
| @@ -498,9 +493,16 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({ | |||
| >([]); | |||
| const onNotificationAdd = () => { | |||
| const settings: NotificationSetting[] = notificationSettings.slice(); | |||
There was a problem hiding this comment.
I know this is existing code, but since we're reusing this pattern, if we need to make a copy for immutability, I would suggest using destructuring over slice. But if you can use map and filter on notificationSettings, that would be all the better. And those methods also make a copy, so would still work as a pattern for keeping notificationSettings immutable if you needed it.
There was a problem hiding this comment.
changed the pattern to use filter and reduce instead of slice
|
/testenv up FEATURE_ALERT_REPORTS=true |
|
@geido Ephemeral environment spinning up at http://34.220.95.81:8080. Credentials are |
|
Let's rebase this with master to unstuck CI |
fisjac
force-pushed
the
ar-notification-settings-duplicate
branch
from
642b9d3
to
59f9cdb
Compare
|
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. |
lint editing pattern to filter reduce
fisjac
force-pushed
the
ar-notification-settings-duplicate
branch
from
59f9cdb
to
215e957
Compare
michael-s-molina
added
the
v4.0
mistercrunch
added
🍒 4.0.1
🍒 4.0.2
🏷️ bot
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