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

Disable saving models without up-to-date result_metadata #40087

Merged
merged 9 commits into from Mar 18, 2024

Conversation

ranquild
Copy link
Contributor

@ranquild ranquild commented Mar 13, 2024

Fixes #37009

Disables creating new and updating existing models without result_metadata. That means that if the query was changed, the user would have to run the query first and wait for results.

How to verify:

  • New -> Modal -> Native
  • SELECT * FROM PRODUCTS
  • Try to save the model without running the query. The button should be disabled and there should be a tooltip.
  • Run the query
  • Now the save button should become enabled
  • Save the model
  • ... menu -> Edit query definition
  • Try changing the model query. The button should become disabled
  • Run the updated query. The button should become enabled
Screenshot 2024-03-15 at 16 23 04

@ranquild ranquild added the backport Automatically create PR on current release branch on merge label Mar 13, 2024
@ranquild ranquild changed the title Disable saving models without up-to-date result_metadata [WIP] Disable saving models without up-to-date result_metadata Mar 13, 2024
@ranquild ranquild marked this pull request as ready for review March 13, 2024 15:39
Copy link

replay-io bot commented Mar 13, 2024

Status Complete ↗︎
Commit e20008f
Results
⚠️ 2 Flaky
2363 Passed

@ranquild ranquild changed the title [WIP] Disable saving models without up-to-date result_metadata Disable saving models without up-to-date result_metadata Mar 15, 2024
@ranquild ranquild requested a review from a team March 15, 2024 14:46
@@ -82,13 +81,6 @@ const Tables = createEntity({
// loads `query_metadata` for a single table
fetchMetadata: compose(
withAction(FETCH_METADATA),
withForceReload((state, { id }) => {
Copy link
Contributor Author

@ranquild ranquild Mar 15, 2024

Choose a reason for hiding this comment

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

This hack is no longer needed. Introduced here #39311

Copy link
Contributor

@luizarakaki luizarakaki left a comment

Choose a reason for hiding this comment

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

lgtm

ButtonsS.ButtonSmall,
)}
/>,
saveButtonTooltipLabel ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of conditionally rendering the Tooltip we could pass disabled={!saveButtonTooltipLabel} prop to it.
This way we could inline the ActionButton back here and remove key prop from it.

Copy link
Contributor Author

@ranquild ranquild Mar 18, 2024

Choose a reason for hiding this comment

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

Didn't know about about disabled, thanks!

fields.every(field => field.display_name);

const saveButtonTooltipLabel =
!isEmpty && isDirty && isNative && isResultDirty
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only native models?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because for MBQL queries result_metadata can be computed by the BE without running the query

@kamilmielnik kamilmielnik requested a review from a team March 18, 2024 08:49
disabled={!saveButtonTooltipLabel}
>
<ActionButton
key="save"
Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant now

Suggested change
key="save"

@ranquild ranquild merged commit c98419c into master Mar 18, 2024
108 checks passed
@ranquild ranquild deleted the disable-model-saving-without-metadata branch March 18, 2024 11:55
Copy link

@ranquild Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge .Team/QueryingComponents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updated native models may lose their result set metadata
3 participants