-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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(SQL Editor): names new query tabs correctly #18951
fix(SQL Editor): names new query tabs correctly #18951
Conversation
…SQL Lab tab names. All that is left is to add tests to make sure that the function works correctly
…le only if the character after the string 'Untitled Query ' is a number. This prevents any issues when trying to get the maximum value in the list.
…ntreCanada/superset into bug/Fix_SQL_Lab_Add_Query_Button
Codecov Report
@@ Coverage Diff @@
## master #18951 +/- ##
==========================================
+ Coverage 66.38% 66.56% +0.17%
==========================================
Files 1641 1641
Lines 63515 63500 -15
Branches 6418 6427 +9
==========================================
+ Hits 42165 42268 +103
+ Misses 19690 19550 -140
- Partials 1660 1682 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
thanks for taking a look at this logic and fixing something that's been broken for years!!!
One other request: would you mind looking into writing a test case for this change? If it's not possible then no worries, but i'm a bit nervous about making changes to this large, untyped file.
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx
Outdated
Show resolved
Hide resolved
…test to make sure that the newQueryEditor function in the TabbedSqlEditors component works as intended
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 iteration!
/testenv up |
@geido Ephemeral environment spinning up at http://54.190.58.79:8080. Credentials are |
Ephemeral environment shutdown and build artifacts deleted. |
* Added in code changes that now properly increment the Untitled Query SQL Lab tab names. All that is left is to add tests to make sure that the function works correctly * Updated the code so that it adds to the untitled_query_numbers variable only if the character after the string 'Untitled Query ' is a number. This prevents any issues when trying to get the maximum value in the list. * Refactored part of the mapping code, to make it shorter and easier to read/understand * Fixed issues in the code that were causing some of the CI tests to fail * Made code changes based on comments within the PR. Also added a unit test to make sure that the newQueryEditor function in the TabbedSqlEditors component works as intended * Fixed the failing cypress test in tabs.test.js (cherry picked from commit 5a5ff99)
SUMMARY
The reason why the bug (view attached issue) occurred in the first place was because there was a local variable within the TabbedSqlEditors component called "queryCount". Whenever the component was reloaded/re-rendered, it caused this local variable to reset to it's default integer value of: '1'. Since this variable is reset to 1, the naming of the new query tabs would not work as intended.
To fix this issue, I decided to use a more dynamic method of naming the new query tabs:
By using this more dynamic method of naming new query tabs, it allows us to avoid creating and maintaining unnecessary state. It also fixes the issue where there never used to be an "Untitled Query 1".
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
(the far right tab was created by selecting the "SQL query" button from the “+“ icon drop down on the top right of the web page beside the settings drop down)
After:
(The query tabs were created using a combination of the two add query buttons, this image shows that the naming now works correctly regardless of which way you decide to create a new query tab)
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION