Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
rusackas merged PR #27239: URL: https://github.com/apache/superset/pull/27239 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
eschutho commented on PR #27239: URL: https://github.com/apache/superset/pull/27239#issuecomment-2038449555 @fisjac I think you're going to need to rebase to bring in the other required CI checks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[PR] fix(alerts/reports): removing duplicate notification method options [superset]
fisjac opened a new pull request, #27239: URL: https://github.com/apache/superset/pull/27239 ### 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](https://github.com/apache/superset/issues/13351)) - [ ] 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
github-actions[bot] commented on PR #27239: URL: https://github.com/apache/superset/pull/27239#issuecomment-2025439867 Ephemeral environment shutdown and build artifacts deleted. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
fisjac closed pull request #27239: fix(alerts/reports): removing duplicate notification method options URL: https://github.com/apache/superset/pull/27239 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
fisjac commented on code in PR #27239:
URL: https://github.com/apache/superset/pull/27239#discussion_r1540441179
##
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##
@@ -498,9 +493,16 @@ const AlertReportModal:
FunctionComponent = ({
>([]);
const onNotificationAdd = () => {
const settings: NotificationSetting[] = notificationSettings.slice();
Review Comment:
changed the pattern to use filter and reduce instead of slice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
fisjac commented on code in PR #27239:
URL: https://github.com/apache/superset/pull/27239#discussion_r1540440994
##
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##
@@ -548,8 +550,16 @@ const AlertReportModal:
FunctionComponent = ({
setting: NotificationSetting,
) => {
const settings = notificationSettings.slice();
-
+// if you've changed notification method
+if (settings[index].method !== setting.method) {
+ // remove subsequent notification methods
+ while (settings.length - 1 > index) {
+settings.pop();
+setNotificationAddState('active');
+ }
+}
Review Comment:
Changed the pattern to use filter rather than slice and pop
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
geido commented on PR #27239: URL: https://github.com/apache/superset/pull/27239#issuecomment-2021153660 Let's rebase this with master to unstuck CI -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
github-actions[bot] commented on PR #27239: URL: https://github.com/apache/superset/pull/27239#issuecomment-1969499874 @geido Ephemeral environment spinning up at http://34.220.95.81:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
geido commented on PR #27239: URL: https://github.com/apache/superset/pull/27239#issuecomment-1969484134 /testenv up FEATURE_ALERT_REPORTS=true -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
eschutho commented on code in PR #27239:
URL: https://github.com/apache/superset/pull/27239#discussion_r1503489044
##
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##
@@ -498,9 +493,16 @@ const AlertReportModal:
FunctionComponent = ({
>([]);
const onNotificationAdd = () => {
const settings: NotificationSetting[] = notificationSettings.slice();
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
eschutho commented on code in PR #27239:
URL: https://github.com/apache/superset/pull/27239#discussion_r1503487125
##
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##
@@ -548,8 +550,16 @@ const AlertReportModal:
FunctionComponent = ({
setting: NotificationSetting,
) => {
const settings = notificationSettings.slice();
-
+// if you've changed notification method
+if (settings[index].method !== setting.method) {
+ // remove subsequent notification methods
+ while (settings.length - 1 > index) {
+settings.pop();
+setNotificationAddState('active');
+ }
+}
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] fix(alerts/reports): removing duplicate notification method options [superset]
codecov[bot] commented on PR #27239: URL: https://github.com/apache/superset/pull/27239#issuecomment-1962070217 ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/27239?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `55.6%` with `4 lines` in your changes are missing coverage. Please review. > Project coverage is 67.33%. Comparing base [(`744f68d`)](https://app.codecov.io/gh/apache/superset/commit/744f68d63784cf90a200db134655147641cef12f?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`642b9d3`)](https://app.codecov.io/gh/apache/superset/pull/27239?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 15 commits behind head on master. | [Files](https://app.codecov.io/gh/apache/superset/pull/27239?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Patch % | Lines | |---|---|---| | [...-frontend/src/features/alerts/AlertReportModal.tsx](https://app.codecov.io/gh/apache/superset/pull/27239?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZlYXR1cmVzL2FsZXJ0cy9BbGVydFJlcG9ydE1vZGFsLnRzeA==) | 50.00% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/27239?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Additional details and impacted files ```diff @@Coverage Diff @@ ## master #27239 +/- ## == + Coverage 67.21% 67.33% +0.11% == Files1905 1908 +3 Lines 7452874452 -76 Branches 8337 8311 -26 == + Hits5009650130 +34 + Misses 2238322267 -116 - Partials 2049 2055 +6 ``` | [Flag](https://app.codecov.io/gh/apache/superset/pull/27239/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [javascript](https://app.codecov.io/gh/apache/superset/pull/27239/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `57.18% <55.55%> (+0.20%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/27239?src=pr=continue_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[PR] fix(alerts/reports): removing duplicate notification method options [superset]
fisjac opened a new pull request, #27239: URL: https://github.com/apache/superset/pull/27239 ### 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](https://github.com/apache/superset/issues/13351)) - [ ] 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
