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.

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

fix(event-flow-viz): handle null metadata selection #11885

Merged
merged 2 commits into from
Dec 8, 2020

Conversation

maloun96
Copy link
Contributor

@maloun96 maloun96 commented Dec 2, 2020

SUMMARY

Fix issue when removing additional metadata from the event flow chart. This was caused by the server that returns 500.
To fix that we need to send an empty array instead of null for multiselects.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before
before
After
after

TEST PLAN

  1. Log In
  2. Create a new event flow chart
  3. Add additional metadata
  4. Run Query
  5. Remove all additional metadata

ADDITIONAL INFORMATION

@maloun96 maloun96 changed the title Initialize object with empty array if is multiselect, fix 500 BE error Fix ##11746. Initialize with empty array if is multiselect, fix 500 BE error Dec 2, 2020
@codecov-io
Copy link

codecov-io commented Dec 2, 2020

Codecov Report

Merging #11885 (fb69383) into master (4666445) will decrease coverage by 12.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #11885       +/-   ##
===========================================
- Coverage   67.12%   54.88%   -12.25%     
===========================================
  Files         915      421      -494     
  Lines       44537    14809    -29728     
  Branches     4235     3822      -413     
===========================================
- Hits        29897     8128    -21769     
+ Misses      14526     6681     -7845     
+ Partials      114        0      -114     
Flag Coverage Δ
cypress 54.88% <100.00%> (+3.27%) ⬆️
javascript ?
python ?

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

Impacted Files Coverage Δ
.../src/explore/components/controls/SelectControl.jsx 87.50% <100.00%> (-5.90%) ⬇️
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/resizable/ResizableHandle.jsx 0.00% <0.00%> (-100.00%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.48%) ⬇️
...src/dashboard/components/gridComponents/Header.jsx 10.52% <0.00%> (-86.85%) ⬇️
...rc/dashboard/components/gridComponents/Divider.jsx 13.33% <0.00%> (-86.67%) ⬇️
...ontend/src/dashboard/util/getDashboardFilterKey.ts 14.28% <0.00%> (-85.72%) ⬇️
...nd/src/views/CRUD/data/query/QueryPreviewModal.tsx 14.70% <0.00%> (-82.97%) ⬇️
...set-frontend/src/views/CRUD/welcome/EmptyState.tsx 5.71% <0.00%> (-82.10%) ⬇️
... and 787 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 4666445...fb69383. Read the comment docs.

@villebro villebro changed the title Fix ##11746. Initialize with empty array if is multiselect, fix 500 BE error Fix(event-flow-viz): handle null metadata selection Dec 2, 2020
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

I think it might be simpler to make the backend more resilient to nulls by changing the below, something like for col in form_data["all_columns"] or []:
https://github.com/apache/incubator-superset/blob/07288789e2adce50939eeb564b1771120bfbc11d/superset/viz.py#L2701

@villebro villebro changed the title Fix(event-flow-viz): handle null metadata selection fix(event-flow-viz): handle null metadata selection Dec 2, 2020
@junlincc
Copy link
Member

junlincc commented Dec 2, 2020

Thanks for the quick fix!

unrelated to this PR

@rusackas can we change metadata back to additional metadata?

@maloun96
Copy link
Contributor Author

maloun96 commented Dec 3, 2020

@junlincc should we change as @villebro said or left from the front-end this fix?

@rusackas
Copy link
Member

rusackas commented Dec 3, 2020

Thanks for the quick fix!

unrelated to this PR

@rusackas can we change metadata back to additional metadata?

Already done.

@rusackas
Copy link
Member

rusackas commented Dec 3, 2020

@junlincc should we change as @villebro said or left from the front-end this fix?

I'd say go ahead and make the change as he suggested... might as well get it here and now :D Then it's fixed on F/E and B/E 🎉

@villebro
Copy link
Member

villebro commented Dec 3, 2020

I agree with @rusackas , better to make both ends more resilient.

@@ -102,10 +102,9 @@ export default class SelectControl extends React.PureComponent {
// Beware: This is acting like an on-click instead of an on-change
// (firing every time user chooses vs firing only if a new option is chosen).
onChange(opt) {
let optionValue = null;
let optionValue = this.props.multi ? [] : null;
Copy link
Member

Choose a reason for hiding this comment

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

One more thing - I might be missing something here, but I don't quite get why this can't always be initialized to []. @maloun96 can you check if that works? I'd much rather not have a ternary here that can be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@villebro in case that select has only 1 value it should be initialized with null, only if it can have multiple select that it should be []

@villebro villebro merged commit 05258eb into apache:master Dec 8, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants