Skip to content
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

feat(admin): Send SQL queries from the UI to system tables #2311

Merged
merged 2 commits into from
Dec 20, 2021

Conversation

lynnagara
Copy link
Member

@lynnagara lynnagara commented Dec 18, 2021

Adds UI components to send freeform SQL queries from the UI to
any system table. Removes UI components related to the canned queries.

Looks like this now:
Screen Shot 2021-12-17 at 4 14 44 pm

Adds UI components to send freeform SQL queries from the UI to
any system table. Removes UI components related to the canned queries.
@lynnagara lynnagara requested a review from a team as a code owner December 18, 2021 00:19
Comment on lines +101 to +107
}).then((resp) => {
if (resp.ok) {
return resp.json();
} else {
return resp.json().then(Promise.reject.bind(Promise));
}
});
Copy link
Member Author

@lynnagara lynnagara Dec 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks terrible but it seems to work. Should probably all be rewritten as async/await

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add some UI container for displaying errors in general, and then make HTTP request logic more generic so every API call writes to the same error div or whatever.

This is perfectly good for what we already have, though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is intentionally free of UI elements - all of the methods here should just return or reject a promise. We can definitely add some system to display errors in a consistent manner across all pages but I don't know that it should be here. Currently this just uses window.alert to display the error message in as I didn't want to deal with styling + managing the state for those yet.

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2021

Codecov Report

Merging #2311 (1c194d8) into master (a85dd91) will not change coverage.
The diff coverage is n/a.

❗ Current head 1c194d8 differs from pull request most recent head 85b7dca. Consider uploading reports for the commit 85b7dca to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2311   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files         557      557           
  Lines       25600    25600           
=======================================
  Hits        23753    23753           
  Misses       1847     1847           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a85dd91...85b7dca. Read the comment docs.

value={value}
onChange={(evt) => onChange(evt.target.value)}
style={{ width: "100%", height: 100 }}
placeholder={"Write your query here"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this placeholder could be a valid query? like the one in your screenshot

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just going to merge this as-is. Let's build canned queries soon 🙂

@lynnagara lynnagara merged commit e438c76 into master Dec 20, 2021
@lynnagara lynnagara deleted the support-sql-queries branch December 20, 2021 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants