eschutho commented on a change in pull request #13102:
URL: https://github.com/apache/superset/pull/13102#discussion_r580718566
##########
File path: superset-frontend/src/SqlLab/components/QuerySearch.jsx
##########
@@ -135,196 +91,183 @@ class QuerySearch extends React.PureComponent {
default:
return null;
}
- }
-
- changeFrom(user) {
- const val = user ? user.value : null;
- this.setState({ from: val });
- }
-
- changeTo(status) {
- const val = status ? status.value : null;
- this.setState({ to: val });
- }
-
- changeUser(user) {
- const val = user ? user.value : null;
- this.setState({ userId: val });
- }
+ };
- insertParams(baseUrl, params) {
+ const insertParams = (baseUrl, params) => {
const validParams = params.filter(function (p) {
return p !== '';
});
return `${baseUrl}?${validParams.join('&')}`;
- }
+ };
- changeStatus(status) {
- const val = status ? status.value : null;
- this.setState({ status: val });
- }
+ const refreshQueries = () => {
+ setQueriesLoading(true);
+ const params = [
+ userId ? `user_id=${userId}` : '',
+ databaseId ? `database_id=${databaseId}` : '',
+ searchText ? `search_text=${searchText}` : '',
+ status ? `status=${status}` : '',
+ from ? `from=${getTimeFromSelection(from)}` : '',
+ to ? `to=${getTimeFromSelection(to)}` : '',
+ ];
- changeSearch(event) {
- this.setState({ searchText: event.target.value });
- }
+ SupersetClient.get({
+ endpoint: insertParams('/superset/search_queries', params),
+ })
+ .then(({ json }) => {
+ setQueriesArray(json);
+ setQueriesLoading(false);
+ })
+ .catch(() => {
+ actions.addDangerToast(t('An error occurred when refreshing queries'));
+ });
+ };
+ // functionally this is closest to componentDidMount, though Abramov
recommends that it is better if you have a more robust dependency array, though
I am afraid that will go into a useCallback loop like before. Though, I could
remove some of the refreshQueries in the other methods, and instead have the
dependency array focus on when values are changed? That would maybe not play
nicely with onChange though.
+ useEffect(() => {
+ refreshQueries();
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, []);
+
+ const onUserClicked = userId => {
+ setUserId(userId);
+ refreshQueries();
+ };
+ // add these to dependency array instead of having their own functions.
- userLabel(user) {
- if (user.first_name && user.last_name) {
- return `${user.first_name} ${user.last_name}`;
+ const onDbClicked = dbId => {
+ // By my reading of this, it is setting state, and then running the
refreshQueries function. Is that correct? I ended up keeping these two, because
I couldn't think of a good way of enabling these in useEffect without also
having it refreshQueries when you change the DB or example in the DB (which was
not part of original functionality)
+ setDatabaseId(dbId);
Review comment:
I see the issue here. You're saying that `setDatabaseId` and `setUserId`
are called elsewhere and it would trigger the refreshQueries function as well.
Yeah, looks like what you have is the best bet. good catch on that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
