Skip to content

Conversation

@yardz
Copy link
Contributor

@yardz yardz commented Mar 4, 2021

SUMMARY

When creating a Graph Chart with the feature flag ENABLE_EXPLORE_DRAG_AND_DROP activated, the application freezes on a white screen.

Scope

The purpose of this PR is to resolve the bug with as little impact as possible on the application

Debuging

When a chart (Graph) is created several error messages are displayed on the console. One of the messages is

Warning: Failed prop type: Invalid prop `options` of type `object` supplied to `SelectControl`, expected `array`.

Captura de Tela 2021-03-04 às 3 37 44 PM

Proposed solution

As the interface is not very clear, and for some reason the contract was changed, the idea of the solution is to change this prop only in the case where the error occurred and not impact any other part of the application that may be using this data.

For this, only the src/explore/components/Control.tsx component was changed, transforming the options props into an array only for the use of SelectControl.

AFTER

Captura de Tela 2021-03-04 às 4 22 03 PM

TEST PLAN

No behavior has been changed, so the application should work normally. In addition, all tests already implemented should pass correctly.

To test if this fix is working correctly, it is necessary to activate the feature flag:ENABLE_EXPLORE_DRAG_AND_DROP
(The activation is not in this PR because it is not intended to change any application's behavior.)

ADDITIONAL INFORMATION

The bug was reported in this comment: #13210 (comment)

@junlincc @ktmud

@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #13466 (e8fa31a) into master (0318b6d) will decrease coverage by 3.88%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13466      +/-   ##
==========================================
- Coverage   77.36%   73.47%   -3.89%     
==========================================
  Files         906      605     -301     
  Lines       45699    21266   -24433     
  Branches     5522     5523       +1     
==========================================
- Hits        35356    15626   -19730     
+ Misses      10216     5513    -4703     
  Partials      127      127              
Flag Coverage Δ
cypress 57.94% <66.66%> (+<0.01%) ⬆️
hive ?
javascript 63.40% <66.66%> (+<0.01%) ⬆️
mysql ?
postgres ?
python ?
sqlite ?

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

Impacted Files Coverage Δ
...perset-frontend/src/explore/components/Control.tsx 91.66% <66.66%> (-3.58%) ⬇️
superset/utils/celery.py
superset/models/sql_lab.py
superset/examples/world_bank.py
superset/queries/saved_queries/dao.py
superset/models/reports.py
superset/annotation_layers/commands/bulk_delete.py
...otation_layers/annotations/commands/bulk_delete.py
...t/annotation_layers/annotations/commands/create.py
superset/db_engine_specs/cockroachdb.py
... and 292 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 0318b6d...e8fa31a. Read the comment docs.

@rusackas
Copy link
Member

rusackas commented Mar 5, 2021

@kgabryje @zhaoyongjie in case you have time to review :)

@zhaoyongjie
Copy link
Member

@rusackas @yardz

I guess we need to fix controlPanel for GraphViz in the superset-ui repo. the columnSelectControl no adaptation DND in explore.

https://github.com/apache-superset/superset-ui/blob/54cee3aa95af344c83f73fb786966c778071a495/plugins/plugin-chart-echarts/src/Graph/controlPanel.tsx#L31-L47

@zhaoyongjie
Copy link
Member

I fixed the issue in the superset-ui repo
apache-superset/superset-ui#991

@ktmud
Copy link
Member

ktmud commented Mar 5, 2021

I think we should try to avoid such specific prop manipulation logics in the general Control renderer. These type of errors should be fixed in each control itself instead.

@yardz
Copy link
Contributor Author

yardz commented Mar 8, 2021

I think we should try to avoid such specific prop manipulation logics in the general Control renderer. These type of errors should be fixed in each control itself instead.

As I don't understand the data flow of the application very well, I made the fix with the least possible impact but with the awareness that it is possibly not the correct way. As the fix was created in another PR, I will close this one because it is neither necessary nor correct.

@yardz yardz closed this Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants