You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "curious86 (via GitHub)" <gi...@apache.org> on 2023/05/16 05:35:57 UTC
[GitHub] [superset] curious86 opened a new pull request, #24070: fix(alert modal): set current user as default alert owner during new alert initialization
curious86 opened a new pull request, #24070:
URL: https://github.com/apache/superset/pull/24070
### SUMMARY
Currently, when the modal window opens, the "owners" field is left unpopulated. This is slightly annoying as it would be more user friendly and efficient to automatically pre-populate the field with the currently logged-in user.
To address this, I propose retrieving the current user from Redux state and setting it as the default owner when the alert modal is opened.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
<img width="1166" alt="Screenshot 2023-05-15 at 1 43 35 AM" src="https://github.com/apache/superset/assets/97945303/bf0a361b-c4d5-47fd-a391-4dd95643f13a">
After:
<img width="1167" alt="Screenshot 2023-05-15 at 1 43 57 AM" src="https://github.com/apache/superset/assets/97945303/258662d3-a135-4098-889e-a4f8dde0e5f3">
### TESTING INSTRUCTIONS
Open Superset in browser > Click on the "Settings" menu on top right > Click on "Alerts & Reports" > on the "Alerts & Reports" page, click on the "+ ALERT" button to create a new alert & notice the behavior.
### ADDITIONAL INFORMATION
NA
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #24070: fix(alert modal): set current user as default alert owner during new alert initialization
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24070:
URL: https://github.com/apache/superset/pull/24070#issuecomment-1549212959
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24070?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24070](https://app.codecov.io/gh/apache/superset/pull/24070?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f0433c2) into [master](https://app.codecov.io/gh/apache/superset/commit/78bc0693d47538f3bda549ea92893f52e60ab1c8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (78bc069) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head f0433c2 differs from pull request most recent head 694a506. Consider uploading reports for the commit 694a506 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24070 +/- ##
=======================================
Coverage 68.22% 68.22%
=======================================
Files 1942 1942
Lines 75215 75218 +3
Branches 8145 8146 +1
=======================================
+ Hits 51318 51321 +3
Misses 21812 21812
Partials 2085 2085
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `54.52% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24070?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...-frontend/src/features/alerts/AlertReportModal.tsx](https://app.codecov.io/gh/apache/superset/pull/24070?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZlYXR1cmVzL2FsZXJ0cy9BbGVydFJlcG9ydE1vZGFsLnRzeA==) | `54.44% <100.00%> (+0.37%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas merged pull request #24070: fix(alert modal): set current user as default alert owner during new alert initialization
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #24070:
URL: https://github.com/apache/superset/pull/24070
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #24070: fix(alert modal): set current user as default alert owner during new alert initialization
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24070:
URL: https://github.com/apache/superset/pull/24070#issuecomment-1550405335
Thanks for the PR, and looking forward to the next!
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24070: fix(alert modal): set current user as default alert owner during new alert initialization
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24070:
URL: https://github.com/apache/superset/pull/24070#discussion_r1194689903
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -1011,7 +1015,15 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
!isEditMode &&
(!currentAlert || currentAlert.id || (isHidden && show))
) {
- setCurrentAlert({ ...defaultAlert });
+ setCurrentAlert({
+ ...defaultAlert,
+ owners: [
+ {
+ value: currentUser.userId,
+ label: `${currentUser.firstName} ${currentUser.lastName}`,
+ },
+ ],
Review Comment:
Note: I was going to propose breaking this out into a util, but it turns out we have a lot of different `User` types that are very different (sometimes `id` us used, other times it's `userId`, and sometimes it's `firstName`, other times `first_name` etc). So best leave this as-is and do a separate PR to clean up all user types in one go.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] curious86 commented on a diff in pull request #24070: fix(alert modal): set current user as default alert owner during new alert initialization
Posted by "curious86 (via GitHub)" <gi...@apache.org>.
curious86 commented on code in PR #24070:
URL: https://github.com/apache/superset/pull/24070#discussion_r1194691398
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -1011,7 +1015,15 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
!isEditMode &&
(!currentAlert || currentAlert.id || (isHidden && show))
) {
- setCurrentAlert({ ...defaultAlert });
+ setCurrentAlert({
+ ...defaultAlert,
+ owners: [
+ {
+ value: currentUser.userId,
+ label: `${currentUser.firstName} ${currentUser.lastName}`,
+ },
+ ],
Review Comment:
Ack.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org