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

Summarizing a model doesn't auto-visualize it unless you first view its query definition #32963

Closed
mazameli opened this issue Aug 4, 2023 · 0 comments · Fixed by #33047
Closed
Assignees
Labels
.Needs Triage Querying/Models aka Datasets .Reproduced Issues reproduced in test (usually Cypress) .Team/QueryProcessor :hammer_and_wrench: Type:Bug Product defects

Comments

@mazameli
Copy link
Contributor

mazameli commented Aug 4, 2023

Describe the bug

See this bug in action

Summarizing a model from the Summarize sidebar and breaking it out does not correctly exhibit the same auto-visualization-selection behavior that tables do. E.g., summarizing by count and breaking out by a date column should display a line chart, but it instead just displays a table.

The same behavior is seen when opening a model from a collection, then clicking the button to open the notebook editor, then summarizing and breaking out and clicking Visualize.

This behavior is not seen when taking the path of New -> Question -> use a model as data source -> summarize and visualize it.

Also, if you open the model, then open up its query definition view, then cancel, then summarize and break out from the sidebar, the auto visualization selection behavior works correctly.

To Reproduce

See this Loom: https://www.loom.com/share/697c1fee880b4dba87d0691c7a321922

  1. Create a model or open an existing one
  2. Click the Summarize button and pick a datetime column to break out by
  3. See a result visualized as a Table viz (when you should be seeing a line chart)

Or

  1. Open a model from a collection
  2. Click the notebook editor button
  3. Summarize by Count and a date column like created at
  4. Click Visualize
  5. See a result visualized as a Table viz (when you should be seeing a line chart)

Expected behavior

Summarizing a model should demonstrate the same auto-viz behavior that tables do.

Logs

No response

Information about your Metabase installation

{
  "browser-info": {
    "language": "en-US",
    "platform": "MacIntel",
    "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/115.0.0.0 Safari/537.36",
    "vendor": "Google Inc."
  },
  "system-info": {
    "file.encoding": "UTF-8",
    "java.runtime.name": "OpenJDK Runtime Environment",
    "java.runtime.version": "11.0.11+9",
    "java.vendor": "AdoptOpenJDK",
    "java.vendor.url": "https://adoptopenjdk.net/",
    "java.version": "11.0.11",
    "java.vm.name": "OpenJDK 64-Bit Server VM",
    "java.vm.version": "11.0.11+9",
    "os.name": "Mac OS X",
    "os.version": "11.5",
    "user.language": "en",
    "user.timezone": "America/Los_Angeles"
  },
  "metabase-info": {
    "databases": [
      "h2"
    ],
    "hosting-env": "unknown",
    "application-database": "h2",
    "application-database-details": {
      "database": {
        "name": "H2",
        "version": "2.1.214 (2022-06-13)"
      },
      "jdbc-driver": {
        "name": "H2 JDBC Driver",
        "version": "2.1.214 (2022-06-13)"
      }
    },
    "run-mode": "dev",
    "version": {
      "date": "2022-11-18",
      "src_hash": "09f4ec50c4b6096192e18c140068ce8a7e67434d",
      "tag": "v0.45.1-SNAPSHOT",
      "branch": "maz-make-saving-a-question-less-fear-inducing",
      "hash": "f694966"
    },
    "settings": {
      "report-timezone": null
    }
  }
}

Severity

P1 — it makes models worse than tables in this key way

Additional context

No response

@mazameli mazameli added Type:Bug Product defects .Needs Triage Querying/Models aka Datasets labels Aug 4, 2023
@darksciencebase darksciencebase added the .Team/QueryProcessor :hammer_and_wrench: label Aug 7, 2023
kulyk added a commit that referenced this issue Aug 9, 2023
kulyk added a commit that referenced this issue Aug 9, 2023
* Reproduce #32963

* Don't lock display for models
github-actions bot pushed a commit that referenced this issue Aug 9, 2023
* Reproduce #32963

* Don't lock display for models
@kulyk kulyk added the .Reproduced Issues reproduced in test (usually Cypress) label Aug 10, 2023
metabase-bot bot added a commit that referenced this issue Aug 10, 2023
* Reproduce #32963

* Don't lock display for models

Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com>
kamilmielnik added a commit that referenced this issue Oct 18, 2023
kamilmielnik added a commit that referenced this issue Oct 19, 2023
…chill mode only) (#34658)

* Update isNavigationAllowed logic to prevent new model creation

* Add simple model creation test

* Remove identifier

* Update comment

* Add a test case for saving a new model

* Avoid using fetchMock directly in QueryBuilder unit tests

* Remove setupCollectionsEndpointsByIds in favor of setupCollectionByIdEndpoint

* Add native question test base

* Update isNavigationAllowed tests

* Make isNavigationAllowed tests pass

* Update shouldShowUnsavedChangesWarningForSqlQuery logic to account for new questions

* Add a test case to run new native question

* Update mock to include individual collection GET endpoint

* Add DataSourceSelectors to MockNativeQueryEditor

* Update test beforeunload event to reflect new behavior

* Use setupCollectionByIdEndpoint instead of modifying setupCollectionsEndpoints

* Update failing test

* Use serializeCardForUrl

* Add a test for native query editing

* Do not trigger warning when leaving empty question

* Add beforeunload tests for creating new questions

* Remove redundant div

* Make assertions consistent

* Use single resetAllMocks()

* Fix post-rebase conflict

* Simplify getShouldShowUnsavedChangesWarning's logic

* Add isModifiedFromNotebook to store

* Show unsaved changes warning when structured question has been modified via notebook editor

* Use explicit return type, as it's not obvious that this function needs to be a Promise

* Remove unused selector dependency

* Update mocks with new attribute

* Add a test for editing notebook questions

* Extract waitForSaveNewQuestionToBeEnabled out of triggerNotebookQueryChange

* Introduce waitForSaveModelToBeEnabled & waitForSaveModelToBeDisabled

* Add a test case

* Remove redundant navigation

* Update isNavigationAllowed logic and tests

* Refactor tests and add more test cases

* Add more test cases

* Shorten code

* Add a test for saving the question

* Refactor isNavigationAllowed

* Add a test for not showing modal

* Add a test for non-notebook changes

* Organize code

* Use existing setUIControls

* Add extra assertions for post-save navigation to check if question is properly mark as non-dirty

* Add extra assertions for post-save navigation to check if question is properly mark as non-dirty

* Add a post-save nagivation test

* Add logic to reset UI controls isModifiedFromNotebook state after saving the question

* Rename to allowedPathnames

* Add explanatory comment

* Update failing e2e test

* Fix #32963 e2e repro failing

* Allow `/model/${slug}` to make models-query-editor e2e test pass

* Update isNavigationAllowed logic to make e2e tests pass

* Update isNavigationAllowed tests

* Add 1 more model location

* Revert identifier name change

* Use optional chaining operator for brevity

* Fix typo

* Rename notebook to structured

* Rename getQuestionLocations to getNativeQuestionLocations and don't use getNativeQuestionLocations in getStructuredQuestionLocations
This was referenced Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Needs Triage Querying/Models aka Datasets .Reproduced Issues reproduced in test (usually Cypress) .Team/QueryProcessor :hammer_and_wrench: Type:Bug Product defects
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants