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

feat(datasets): Allow swap dataset after deletion #30364

Merged

Conversation

Antonio-RiveroMartnez
Copy link
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez commented Sep 23, 2024

SUMMARY

Currently, if you delete a dataset, the charts that were using it don't keep the controls config when swapping to a dataset that have similar structure.
This is because the state gets overwritten with form data that doesn't have the controls configs so the sync method is not able to fetch the correct values for them.

This PR keeps the values and other configs when the API response doesn't include dataset id (deleted) and fallback to read from the datasource when getting the controls values in case there's nothing in the state.

One thing to notice is that before, when you first loaded a chart that have no dataset because it was deleted, the explore data was a fallback and didn't trigger the API requests, now, because we're not using this fallback explore but keeping the form data info from the API response, the UI will show a Not Found error until you swap the dataset and update the chart.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

before.mov

Now:

test_2.mov

TESTING INSTRUCTIONS

Create two datasets with the same schema (columns and metrics).
Create a chart using one of these and save it.
Delete this dataset.
Access the chart -- you'll see an option to swap the dataset.
Remap the chart to use the other dataset.
Your controls config must be kept

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • 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

@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 23, 2024
@Antonio-RiveroMartnez
Copy link
Member Author

/testenv up

Copy link
Contributor

@Antonio-RiveroMartnez Ephemeral environment spinning up at http://35.162.103.7:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@Antonio-RiveroMartnez Antonio-RiveroMartnez marked this pull request as ready for review September 24, 2024 09:42
@dosubot dosubot bot added data:dataset Related to dataset configurations explore:dataset Related to the dataset of Explore labels Sep 24, 2024
@Antonio-RiveroMartnez Antonio-RiveroMartnez marked this pull request as draft September 24, 2024 14:02
@Antonio-RiveroMartnez Antonio-RiveroMartnez marked this pull request as ready for review September 24, 2024 14:32
@dosubot dosubot bot added change:frontend Requires changing the frontend explore Namespace | Anything related to Explore labels Sep 24, 2024
@Antonio-RiveroMartnez
Copy link
Member Author

/testenv up

Copy link
Contributor

@Antonio-RiveroMartnez Ephemeral environment spinning up at http://54.218.73.54:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@sfirke
Copy link
Member

sfirke commented Sep 24, 2024

Wow, that is a nice quality of life improvement for users! Huge potential time saver.

@rusackas
Copy link
Member

I think this PR is a fantastic step in the right direction ❤️

If there's interest/bandwidth to take things a step further, it's worth noting that deleting a dataset might break HUNDREDS of charts. While this PR allows you to restore them one at a time, it might be a great idea to (also) allow this swap/substitution in the Dataset deletion modal. You could give the user any/all of these choices:

  • Replace {deleted dataset} with [Select dataset ˅]
  • Delete dataset (charts will be broken)
  • Delete dataset and affected charts

Of course, taking it even further over the horizon, we probably shouldn't be deleting anything but rather doing "soft deletes" where rows are tagged as deleted, so that they can be restored by a DBA. (Note, this is not the same as "Archiving" which requires UI changes and un-archiving capabilities.

Love the PR, but just saying this is the entry of a longer tunnel ;)

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Looks great! Left some non-blocking comments.

Unrelated to this PR, but I'd love to see a more verbose error than "Not found" in the chart area

@Antonio-RiveroMartnez Antonio-RiveroMartnez merged commit 18c2376 into apache:master Sep 25, 2024
34 checks passed
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend data:dataset Related to dataset configurations explore:dataset Related to the dataset of Explore explore Namespace | Anything related to Explore size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants