-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(alert modal): set current user as default alert owner during new alert initialization #24070
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the improvement!
owners: [ | ||
{ | ||
value: currentUser.userId, | ||
label: `${currentUser.firstName} ${currentUser.lastName}`, | ||
}, | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Thanks for the PR, and looking forward to the next! |
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:
After:
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