fix(SQL Editor): names new query tabs correctly #18951
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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.
|
etr2460
reviewed
Mar 1, 2022
There was a problem hiding this comment.
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
3 tasks
etr2460
approved these changes
Mar 2, 2022
|
/testenv up |
|
@geido Ephemeral environment spinning up at http://54.190.58.79:8080. Credentials are |
geido
approved these changes
Mar 4, 2022
|
Ephemeral environment shutdown and build artifacts deleted. |
villebro
pushed a commit
that referenced
this pull request
Apr 3, 2022
* 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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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