fix: SQL Lab show "Refetch Results" button while fetching new query results

[graceguo-supercat]

SUMMARY

when user run a query in SQL Lab, after query is done from engine, and then results are fetched into browser, SQL Lab should show results table. But when the results data is big, it may take a few seconds download. Many users saw the "Refetch results" button show up after query is succeed. Here is a screenshot:
I run a simple query against Presto engine:

select * from superset_production_logs_v01
where ds > '2019-01-01'

Because it has a lot of data, it takes long time to download results. SQL Lab shows "Refetch results" for a few seconds, then after results are fetched, it shows results table.

This "Refetch Results" button is very confusing. Many users will just click the button, which cause SQL Lab even longer time to fetch results.

The root cause is pretty simple: race condition. SQL Lab polling query status every second, and once resultKey is returned, it will start another request to fetch result. Sometimes the fetching request sends out before the last poll request returned. So the last poll response reset query state from fetching to success, and this will trigger "Refetch Results" button. After fetch response returned, this strange state will be reset to correct state, results table will show up.

This PR is trying to fix this race condition. Currently SQL Lab's query states are bad designed. It seems we are re-use same "success" state twice:
pending -> running -> success -> fetching -> success
there are a lot of logic depends on the current states. If i introduce another state to replace the final "success", it will cause many trouble :( So my fix here is hacky but simple: if query is in "fetching" state, polling should not change its state back to "success".

TESTING INSTRUCTIONS

CI and manual test.

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

Merging #15109 (75db31d) into master (f8b270d) will decrease coverage by 0.28%.
The diff coverage is 67.39%.

❗ Current head 75db31d differs from pull request most recent head 94dcb9a. Consider uploading reports for the commit 94dcb9a to get more accurate results

@@            Coverage Diff             @@
##           master   #15109      +/-   ##
==========================================
- Coverage   77.53%   77.24%   -0.29%     
==========================================
  Files         967      969       +2     
  Lines       49740    50023     +283     
  Branches     6351     6436      +85     
==========================================
+ Hits        38565    38640      +75     
- Misses      10973    11178     +205     
- Partials      202      205       +3     

Flag	Coverage Δ
hive	?
javascript	72.20% <63.37%> (-0.24%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/SqlLab/reducers/sqlLab.js	34.12% <0.00%> (-0.84%)	⬇️
superset-frontend/src/dashboard/actions/hydrate.js	1.72% <ø> (ø)
...ard/components/FiltersBadge/DetailsPanel/index.tsx	86.20% <ø> (ø)
.../FilterBar/CascadeFilters/CascadePopover/index.tsx	61.76% <ø> (ø)
...Filters/FilterBar/FilterControls/FilterControl.tsx	100.00% <ø> (ø)
...mponents/nativeFilters/FiltersConfigModal/utils.ts	66.25% <ø> (ø)
...nd/src/dashboard/components/nativeFilters/utils.ts	56.25% <ø> (-30.00%)	⬇️
...perset-frontend/src/dashboard/containers/Chart.jsx	100.00% <ø> (ø)
...nd/src/dashboard/containers/DashboardComponent.jsx	92.30% <ø> (ø)
...-frontend/src/dashboard/reducers/dashboardState.js	73.33% <0.00%> (ø)
... and 75 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8b270d...94dcb9a. Read the comment docs.

[etr2460]

sp nit: async

[etr2460]

perhaps something like this is cleaner?

Suggested change

	let updatedState = {
	...changedQuery,
	};
	if (currentState === 'success') {
	// race condition:
	// because of asyc behavior, sql lab may still poll a couple of seconds
	// when it started fetching or finished rendering results
	if (['fetching', 'success'].includes(prevState)) {
	updatedState = {
	...updatedState,
	state: prevState,
	};
	}
	}
	newQueries[id] = { ...state.queries[id], ...updatedState };
	newQueries[id] = {
	...state.queries[id],
	...changedQuery,
	// race condition:
	// because of async behavior, sql lab may still poll a couple of seconds
	// when it started fetching or finished rendering results
	state: currentState === 'success' && ['fetching', 'success'].includes(prevState) ? prevState : currentState
	};

[graceguo-supercat]

yes much cleaner!

[etr2460]

lgtm, thanks for addressing comments