You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/02/28 01:31:49 UTC
[GitHub] [incubator-superset] etr2460 opened a new pull request #9220: [SQL
Lab] Implement refetch results button properly
etr2460 opened a new pull request #9220: [SQL Lab] Implement refetch results button properly
URL: https://github.com/apache/incubator-superset/pull/9220
### CATEGORY
Choose one
- [x] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
Previously, we were rendering the Fetch data preview button when you switched away from a running query tab and switched back after the query finished. Clicking on this button would just rerun the query again. Now we check to see if a results key is set, and if so we fetch the results from the results cache
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
Run a query in a tab, switch to another tab, come back and click the fetch results button, see no new query run.
Ensure refetching data previews still works.
Ensure running normal queries still work
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
to: @graceguo-supercat @betodealmeida @rusackas
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 merged pull request #9220: [SQL Lab]
Implement refetch results button properly
Posted by GitBox <gi...@apache.org>.
etr2460 merged pull request #9220: [SQL Lab] Implement refetch results button properly
URL: https://github.com/apache/incubator-superset/pull/9220
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9220: [SQL Lab]
Implement refetch results button properly
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9220: [SQL Lab] Implement refetch results button properly
URL: https://github.com/apache/incubator-superset/pull/9220#issuecomment-593644045
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9220?src=pr&el=h1) Report
> Merging [#9220](https://codecov.io/gh/apache/incubator-superset/pull/9220?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/cd4605e4c1f1716dc182a6686991e971df924920?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `60%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9220?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9220 +/- ##
==========================================
- Coverage 58.92% 58.92% -0.01%
==========================================
Files 372 372
Lines 11999 12008 +9
Branches 2940 2942 +2
==========================================
+ Hits 7071 7076 +5
- Misses 4750 4754 +4
Partials 178 178
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9220?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...erset-frontend/src/SqlLab/components/ResultSet.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC5qc3g=) | `75.7% <60%> (-1.86%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9220?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9220?src=pr&el=footer). Last update [cd4605e...cf88080](https://codecov.io/gh/apache/incubator-superset/pull/9220?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on a change in
pull request #9220: [SQL Lab] Implement refetch results button properly
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9220: [SQL Lab] Implement refetch results button properly
URL: https://github.com/apache/incubator-superset/pull/9220#discussion_r386606137
##########
File path: superset-frontend/src/SqlLab/components/ResultSet.jsx
##########
@@ -258,19 +258,32 @@ export default class ResultSet extends React.PureComponent {
}
}
if (query.cached || (query.state === 'success' && !query.results)) {
- return (
- <Button
- bsSize="sm"
- className="fetch"
- bsStyle="primary"
- onClick={this.reFetchQueryResults.bind(this, {
- ...query,
- isDataPreview: true,
- })}
- >
- {t('Fetch data preview')}
- </Button>
- );
+ if (query.isDataPreview) {
+ return (
+ <Button
+ bsSize="sm"
+ className="fetch"
+ bsStyle="primary"
+ onClick={this.reFetchQueryResults.bind(this, {
Review comment:
https://www.freecodecamp.org/news/the-best-way-to-bind-event-handlers-in-react-282db2cf1530/
In Superset code base, usually we bind in constructor.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9220: [SQL Lab] Implement refetch results button properly
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9220: [SQL Lab] Implement refetch results button properly
URL: https://github.com/apache/incubator-superset/pull/9220#discussion_r386653408
##########
File path: superset-frontend/src/SqlLab/components/ResultSet.jsx
##########
@@ -258,19 +258,32 @@ export default class ResultSet extends React.PureComponent {
}
}
if (query.cached || (query.state === 'success' && !query.results)) {
- return (
- <Button
- bsSize="sm"
- className="fetch"
- bsStyle="primary"
- onClick={this.reFetchQueryResults.bind(this, {
- ...query,
- isDataPreview: true,
- })}
- >
- {t('Fetch data preview')}
- </Button>
- );
+ if (query.isDataPreview) {
+ return (
+ <Button
+ bsSize="sm"
+ className="fetch"
+ bsStyle="primary"
+ onClick={this.reFetchQueryResults.bind(this, {
Review comment:
got it. It was bound like this before, but I'll move the binding in both places to the constructor
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9220: [SQL Lab]
Implement refetch results button properly
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9220: [SQL Lab] Implement refetch results button properly
URL: https://github.com/apache/incubator-superset/pull/9220#issuecomment-593640854
@graceguo-supercat I updated all the instances of bind in the file to happen in the constructor (and removed the `bind` on the renderControls function since it wasn't needed)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org