fix: adding additional configs and colors for queryHistory #14995
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14995 +/- ##
==========================================
- Coverage 77.68% 77.68% -0.01%
==========================================
Files 965 965
Lines 49535 49538 +3
Branches 6268 6268
==========================================
Hits 38482 38482
- Misses 10852 10855 +3
Partials 201 201
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Thanks for the quick fix, I'll test the |
|
ok! Let me know how it looks, and we can get it merged afterwards. |
There was a problem hiding this comment.
LGTM, will wait for @michellethomas to test so we can merge.
AAfghahi
force-pushed
the
fixingQueryHistory
branch
from
afff3ed
to
8092f5d
Compare
|
@betodealmeida michelle and I went through this on zoom, and it is now working for her. |
| name={statusAttributes[q.state].config.name} | ||
| status={statusAttributes[q.state].config.status} | ||
| /> | ||
| {q.state && ( |
There was a problem hiding this comment.
Is it possible for q.state to be undefined? You added this when we were testing, is this still needed?
There was a problem hiding this comment.
no, I changed it back, ty.
AAfghahi
force-pushed
the
fixingQueryHistory
branch
from
48890cb
to
c4b82a6
Compare
) * added additional configs and colors for queryHistory * added condition to status icon * Update superset-frontend/src/SqlLab/components/QueryTable/index.jsx * Update superset-frontend/src/SqlLab/components/QueryTable/index.jsx (cherry picked from commit 7f4e036)
|
Hi @AAfghahi thanks so much for getting this fix in. I think there's some cleanup todo from this issue to make sure it doesn't happen again. How do you want me to keep track of the suggestions gh issue? It would be nice to either make sure that any possible sqllab query states that are not in statusAttributes will be flagged in CI or if the code in QueryTable can handle unknown states (states not listed in statusAttributes). It could be good to add tests for different query states, and it looks like the |
|
@michellethomas Hello, sorry for the late reply. Thank you so much for these thoughts and suggestions, I agree with all of them. I am comfortable keeping this PR open as a tracking mechanism, we can also create an issue as a collection. I will change the code tomorrow to handle unknown states, and will also add tests. Though, the tests may have to wait till next week, because the rest of this sprint I need to focus on a feature that we are rolling out (if that is ok). I'll add fetching into QueryState tomorrow (one of the reasons it wasn't even including earlier was because we didn't know it existed!). |
* upstream/master: fix(explore): Datepicker glitch on hover outside the modal (apache#15033) Add ming-height to empty tab (apache#14878) Remove nowrap (apache#14954) display all metric results in editor (apache#15031) feat: Add "is_select_query" method to base engine spec to make it possible to override it (apache#15013) fix(dashboard): custom css should be removed on unmount (apache#15025) feat(filter-box): hide druid options if druid not enabled (apache#14921) fix: adding additional configs and colors for queryHistory (apache#14995) chore: rename 'Source' to 'Database' for consistency (apache#15021) chore(ci): fix ci conflict (apache#15016) fix(native-filters): avoid double load on initialization (apache#15012) feat(native-filters): Support default to first value in select filter (apache#14869) docs: required information for OAuth2 configuration (apache#15010) Update index.mdx (apache#14990)
mistercrunch
added
the
🏷️ bot
SUMMARY
Some of the statuses in Query History did not have the proper configs and colors, this PR fixes that. Changes were suggested in:
#14885
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Should be like:
TESTING INSTRUCTIONS
Go to the QueryHistory tab in sql lab, make sure that
ADDITIONAL INFORMATION