-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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 bug fix #22688
fix: alert modal bug fix #22688
Conversation
df8941b
to
abbdd78
Compare
Codecov Report
@@ Coverage Diff @@
## master #22688 +/- ##
==========================================
- Coverage 67.07% 66.92% -0.16%
==========================================
Files 1869 1870 +1
Lines 71579 72501 +922
Branches 7806 8162 +356
==========================================
+ Hits 48015 48519 +504
- Misses 21536 21910 +374
- Partials 2028 2072 +44
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 |
@@ -442,7 +442,7 @@ const AsyncSelect = forwardRef( | |||
fetchedQueries.current.clear(); | |||
setAllValuesLoaded(false); | |||
setSelectOptions(initialOptions); | |||
}, [initialOptions]); | |||
}, [initialOptions, options]); |
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.
@AAfghahi can you add more context to the PR description? I'm trying to understand the problem because initialOptions
is calculated from options
:
const initialOptions = useMemo(
() => options && Array.isArray(options) ? options.slice() : EMPTY_OPTIONS,
[options],
);
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.
Yeah! So, in AlertReportModal, what we are sending over to options is not an array, it returns this:
https://github.com/apache/superset/blob/master/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx#L811
so this fails the Array.isArray(options)
and does not update initialOptions, and leaves initialOptions as EMPTY_OPTIONS
. This is what was causing the behavior from before.
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.
I understood. I think we had a problem during the refactoring. For the async Select, we don't have initial options so we can remove the references to it. I'm sending you a patch with the changes. Let me know if AlertReportModal
works after applying it with git apply patch.txt
/testenv up |
@AAfghahi Ephemeral environment spinning up at http://54.184.216.139:8080. Credentials are |
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
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Due to some refactoring, we were no longer updating the dropdown in the alert/report modal when a user switched from one alert option to another.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
ADDITIONAL INFORMATION