Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SIP-4] replace SQL Lab ajax calls with SupersetClient #5896

Merged
merged 12 commits into from
Oct 18, 2018
Merged

[SIP-4] replace SQL Lab ajax calls with SupersetClient #5896

merged 12 commits into from
Oct 18, 2018

Conversation

williaster
Copy link
Contributor

This PR is one of a few PRs that implement the final step 4) discussed in #5772, to refactor just SQL Lab -specific ajax calls (not including charts) for easier review

Note that the new @superset-ui/core dep + setup for SupersetClient is duplicated across all ajax PRs:

@kristw @mistercrunch @graceguo-supercat @michellethomas @conglei

This was referenced Sep 14, 2018
Merged
Merged
Merged
williaster
williaster commented Sep 14, 2018
View reviewed changes
superset/assets/spec/javascripts/sqllab/ExploreResultsButton_spec.jsx Outdated
const visualizationPayload = { table_id: 107 };
fetchMock.post(visualizeEndpoint, visualizationPayload);

// let ajaxSpy;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will get rid of these

@kristw
Copy link
Contributor

kristw commented Sep 14, 2018

Seems like there are conflicts with master.

@williaster
Copy link
Contributor Author

williaster commented Sep 14, 2018 •
edited
Loading

yeah 😢 all of these now have conflicts, a lot from the no undef eslint rule. will rebase them all over the weekend or early next week. I'd like to test one of the client PRs in staging before merging to make sure it works with non-local hosts + urls, etc.

@codecov-io
Copy link

Codecov Report

Merging #5896 into master will decrease coverage by 0.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5896      +/-   ##
==========================================
- Coverage   77.23%   76.91%   -0.33%     
==========================================
  Files          47       47              
  Lines        9349     9362      +13     
==========================================
- Hits         7221     7201      -20     
- Misses       2128     2161      +33
Impacted Files Coverage Δ
superset/cli.py 36% <0%> (-13.2%) ⬇️
superset/cache_util.py
superset/dashboard_import_export_util.py
superset/import_util.py
superset/dict_import_export_util.py
superset/utils.py
superset/utils/cache.py 48.14% <0%> (ø)
superset/utils/dict_import_export.py 21.05% <0%> (ø)
superset/utils/dashboard_import_export.py 40.74% <0%> (ø)
superset/utils/core.py 88.3% <0%> (ø)
... and 11 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 af0ffa4...3fdd834. Read the comment docs.

@williaster
Copy link
Contributor Author

@kristw @michellethomas @graceguo-supercat this one should be good to go as well, sorry it's a bit longer than the others.

the short url explore cypress test surfaced a bug with refactoring getShortUrl to use promises that came from a rebase! 🚀

This was referenced Oct 17, 2018
Merged
Merged
Merged
graceguo-supercat
graceguo-supercat approved these changes Oct 18, 2018
View reviewed changes
Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@williaster williaster merged commit e163dfe into apache:master Oct 18, 2018
@williaster williaster deleted the chris--ajax-sqllab branch October 18, 2018 17:40
@williaster williaster mentioned this pull request Oct 24, 2018
Merged
bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018
@williaster @bipinsoniguavus
[SIP-4] replace SQL Lab ajax calls with SupersetClient (apache#5896)
2ac1f1e 
* [superset-client] replace sqllab ajax calls with SupersetClient

* [superset-client][sqllab] replace more misc ajax calls

* [superset-client][tests] call setupSupersetClient() in test shim

* [superset-client] replace more sqllab ajax calls and fix tests

* [superset-client][tests] remove commented lines

* [sqllab][superset-client] fix eslint and tests, add better error handling tests.

* [superset-client] fix tests from rebase

* [cypress][sqllab][superset-client] fix

* [superset-client] use Promises not callbacks in getShortUrl calls

* [superset-client][short-url] don't stringify POST

* [superset-client][short-url][cypress] add data-test attribute for more reliable test

* [cypress] remove .only() call
@graceguo-supercat graceguo-supercat mentioned this pull request May 16, 2019
Merged
12 tasks
@graceguo-supercat graceguo-supercat mentioned this pull request Sep 19, 2019
Merged
12 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graceguo-supercat graceguo-supercat graceguo-supercat approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@williaster @kristw @codecov-io @graceguo-supercat @mistercrunch