Enable "Run Query in New Tab" in SQL Lab #1343
Conversation
| dbId: (action.query.dbId) ? action.query.dbId : null, | ||
| schema: (action.query.schema) ? action.query.schema : null, | ||
| autorun: true, | ||
| sql: `${action.query.sql}`, |
There was a problem hiding this comment.
same as sql: action.query.sql,
|
Thanks for the quick feedback! Definitely should've caught that nit, cargo-culted a little too hard there. Will add tests tonight; since I was already in there I added a few for other reducer actions around QueryEditors - would you prefer those be separated out into another PR? |
Method #1; doesn't feel very clean. Going to attempt to reimplement using an action and changing state directly through the reducer rather than creating a new QueryEditor object directly from the QueryTable
Bug => Attempting to clone anything other than the most recent Query for a given TabbedQueryEditor would throw an exception, because we depended on lastQueryId to find the title of the QueryEditor to clone. Since you can only activate a clone from the currently active tab, we can instead fetch the ID of the Editor to copy the Title of from the tip of tabHistory.
| sql: action.query.sql, | ||
| }; | ||
|
|
||
| return sqlLabReducer(state, actions.addQueryEditor(qe)); |
There was a problem hiding this comment.
Wasn't super DRY essentially copying addQueryEditor into this action. This seems better, but not terribly clean either…suggestions for improvement welcome!
| import { describe, it } from 'mocha'; | ||
| import { expect } from 'chai'; | ||
|
|
||
| describe('sqlLabReducer', () => { |
There was a problem hiding this comment.
Couldn't find anything to model these tests off of, let me know how you'd prefer them to be structured.
| }); | ||
|
|
||
| it('should prefix the new tab title with "Copy of"', () => { | ||
| expect(newState.queryEditors[1].title).to.include("Copy of"); |
There was a problem hiding this comment.
Assuming that our new queryEditor will always be the last element is all well and good, but seems terribly hacky to me. Any suggestions or are you fine with it?
|
|
||
| it('should have at most one more tab', () => { | ||
| expect(newState.queryEditors).have.length(2); | ||
| //expect(newState.queryEditors.filter((qe) => qe.sql == testQuery.SQL).length).to.equal(2) |
There was a problem hiding this comment.
Woops. Will remove this when I clean up any other nits from code review.
|
Thanks for the PR, this looks good to me as is, I'll do a blitz soon to get some test coverage on SQL Lab, but for now this is good! |
nickbarnwell
deleted the
nickbarnwell/fix-1147-sqlab-run-query-in-new-tab
branch
* Enable "Clone to New Tab" btn in QueryHistoryTable Method apache#1; doesn't feel very clean. Going to attempt to reimplement using an action and changing state directly through the reducer rather than creating a new QueryEditor object directly from the QueryTable * Move Clone Logic to Action * Implement PR feedback * Clean up reducer action; fix bug Bug => Attempting to clone anything other than the most recent Query for a given TabbedQueryEditor would throw an exception, because we depended on lastQueryId to find the title of the QueryEditor to clone. Since you can only activate a clone from the currently active tab, we can instead fetch the ID of the Editor to copy the Title of from the tip of tabHistory. * Tests for Reducer Action * Fix CodeClimate feedback
mistercrunch
added
🏷️ bot
Fixes #1147 – saw this was on the roadmap but it's important for the workflow I personally use so worst case I'll abandon this patch if there's already a fix in mainline.
Commit 7d33c8f shows the first approach I took, before reading a bit more about Redux; I can rebase it away and add tests if the approach in f3326a3 looks good.
n.b. This was my first time working with React/Redux, apologies in advance if this isn't idiomatic; I tried my best to base it on the rest of the codebase.