-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Sort schema columns alphabetically #4595
Conversation
…ted` raises an Exception it returns the column names unaltered from the query runner.
I'd like to add tests for this behavior. What's the best way to do so? I have a few ideas:
|
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.
Re. tests, there are three options I can think of:
- Mock
get_schema
to return some data. - Extract sorting logic into its own function and test it with dummy data.
- Test this at the handler level with more of an integration test, and use the test Postgres database as the target.
I used @arikfr's second testing option. Extracted the sort into its own function and tested with dummy data. |
Pending further feedback I think this is ready to merge. |
Not sure what's causing CircleCI e2e tests to fail. I didn't change any front-end files. |
😪, looks like it kept flaky after #4545, for now it's a matter of re-trigger the build. I guess Cypress indeed keeps the chained element and we will need to make sure the results are fresh before asserting (cc @kravets-levko). |
@gabrieldutra I found another thing that makes pivot test flaky: cypress input is too fast. When test adds a new line to query - it immediately clicks Execute button. But query editor is debounced, so 50/50 it will execute query with old text - therefore test fails. Here is my attempt to fix this - ec2ede4 I have no other idea for how to detect when query object received a new text from editor. |
I'm continuing to test local. This isn't ready to merge yet. When connected to Redash's internal metadata database the schema sort doesn't work at all. |
…ever returned! ____ ____ ____ _____ / __ \/ __ \/ __ \/ ___/ / /_/ / /_/ / /_/ (__ ) \____/\____/ .___/____/ /_/
Found the issue. When I refactored to
|
… sorter. Related to a31f901
Alrighty. I added a second test case that would have caught this issue addressed in a31f901. |
On my second unit test, would it be better to use The first test verifies the sort behavior. The second test verifies that Redash uses that sort behavior. Seems like the |
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.
👍
We're using Redash Version: 24.04.0-dev and the sorting functionality is not working, I saw there is a new MR from @susodapop |
What type of PR is this? (check all applicable)
Description
This PR sorts the column names shown in the schema viewer alphabetically at the API level. It also moves table name sorting from the Model code into the
get_schema()
method of the schema resource. If the sort raises an exception then the API returns the columns in the order sent by the query runner.Users will need to refresh their schemas before the change is visible in the front-end.
Related Tickets & Documents
https://github.com/redashlabs/product/issues/15.
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Before
After