fix: fetch all owners for dashboard and chart listview filters and properties modal #9784
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #9784 +/- ##
==========================================
+ Coverage 70.79% 70.91% +0.12%
==========================================
Files 587 183 -404
Lines 30435 17826 -12609
Branches 3152 0 -3152
==========================================
- Hits 21545 12641 -8904
+ Misses 8776 5185 -3591
+ Partials 114 0 -114
Continue to review full report at Codecov.
|
nytai
force-pushed
the
tai/request-all-records
branch
from
4f84c97
to
b0828c9
Compare
|
This most likely will conflict with #9628 |
nytai
changed the title
|
It's not ready for merge yet. Just a heads up how they might interfere with each other. Here are the related changes: d2050b5 Feel free to merge your changes first if you are ready. |
nytai
force-pushed
the
tai/request-all-records
branch
from
2f68649
to
d77bb4f
Compare
Codecov Report
@@ Coverage Diff @@
## master #9784 +/- ##
=======================================
Coverage 71.35% 71.36%
=======================================
Files 585 585
Lines 30917 30932 +15
Branches 3246 3252 +6
=======================================
+ Hits 22061 22074 +13
Misses 8747 8747
- Partials 109 111 +2
Continue to review full report at Codecov.
|
nytai
force-pushed
the
tai/request-all-records
branch
from
be8efb7
to
1565405
Compare
nytai
added a commit
to preset-io/superset
that referenced
this pull request
Jun 4, 2020
…nd properties modal (apache#9784)" This reverts commit d187d28.
auxten
pushed a commit
to auxten/incubator-superset
that referenced
this pull request
Nov 20, 2020
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Superset api limits records to 20 by default. @bkyryliuk recently opened a PR(#9667) to fix some of the owners dropdowns so that all records are returned by the api. The pr received some push back since changing the defaults probably isn't the best approach. This PR requests all records for the owners endpoints to fix the issue flagged in #9667.
Originally we intended to add pagination (via infinite scroll) and backend filtering. While trying to implement that approach I hit some limitations in react-select's async mode that caused me to reimplement fetching using react-select in non async mode, code is here: 90f7f79
Also since there is currently an upgrade for react-select in progress (#9628) I opted for holding off on adding more complexity until the upgrade is complete. React select is fairly fast at searching and I don't think we should expect the owners list to be that large, so I'm not convinced that adding pagination will result in a more performant end user experience than fetching all records after mount and searching locally.
TEST PLAN
ADDITIONAL INFORMATION
REVIEWERS