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.

Sign up for GitHub

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

Jump to bottom

Remove redundant (and wrong) hasData check from ViewHeader component #37566

Merged
merged 5 commits into from
Jan 18, 2024
Merged

Remove redundant (and wrong) hasData check from ViewHeader component #37566

merged 5 commits into from
Jan 18, 2024

Conversation

nemanjaglumac
Copy link
Member

@nemanjaglumac nemanjaglumac commented Jan 11, 2024 •
edited

This PR resolves #37516 by migrating the hasData check to MLv2 for cases when the query is the instance of a StructuredQuery removing the redundant check altogether.

We might soon be able to do this for the native query check as well, but for now it has to use the old version of the lib.
Once we migrate the second (native) part to MLv2, we can mark #37227 as done.

As it turned out that hasData() is not even needed in this file, it removes the need to handle structured and native queries separately.

This PR also marks hasData() methods on StructuredQuery and NativeQuery as deprecated.

@nemanjaglumac nemanjaglumac requested a review from a team January 11, 2024 12:25
@nemanjaglumac nemanjaglumac self-assigned this Jan 11, 2024
@nemanjaglumac nemanjaglumac added the no-backport Do not backport this PR to any branch label Jan 11, 2024
@nemanjaglumac

This comment was marked as outdated.

Sign in to view
@ranquild

This comment was marked as outdated.

Sign in to view
@replay-io replay.io
Copy link

replay-io bot commented Jan 11, 2024 •
edited

StatusComplete ↗︎
Commit0a714a1
Results
❌ 1 Failed
  • should not display query editing controls and 'Browse Data' link 
      AssertionError: Timed out retrying after 4000ms: Unable to find an element with the text: Save. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.
Ignored nodes: comments, <script />, <style />
<div
  class="eb9z3dt4 eanyi7817 emotion-9e76f1 e11094na0"
  data-testid="qb-header"
  style="opacity: 1;"
>
  <div
    class="emotion-1o97tm2 eanyi784"
    data-testid="qb-header-left-side"
  >
    <div
      class="emotion-1o7d3sk eanyi7816"
    >
      <div
        class="emotion-2y363g eanyi7810"
      >
        <span
          class="emotion-rq1m63 e1grb1gu1"
          data-testid="head-crumbs-container"
        >
          <div
            class="emotion-1npab0 egv21s31"
          >
            <div
              class="egv21s30 emotion-1hali18 eoa5drl1"
              data-testid="editable-text"
              data-value="test question "
            >
              <textarea
                class="emotion-pojurp eoa5drl0"
                data-testid="saved-question-header-title"
                placeholder="Add title"
              >
                test question
              </textarea>
            </div>
          </div>
        </span>
      </div>
    </div>
    <div
      class="emotion-1kv07fc eanyi7815"
    >
      <button
        class="Button eanyi787 e13bobqq0 emotion-1j2zjzk emiw9oj2"
        data-testid="revision-history-button"
      >
        <div
          class="emotion-zxs4gr emiw9oj1"
        >
          <div
            class=" emotion-1w35l9a emiw9oj0"
          >
            Edited
            a few seconds ago by Bobby Tables
          </div>
        </div>
      </button>
    </div>
  </div>
  <div
    class="emotion-1mjo75b eanyi782"
    data-testid="qb-header-action-panel"
  >
    <div
      class="emotion-1x0k1ds eanyi781"
    >
      <button
        aria-label="Refresh"
        class="Button eanyi780 RunButton eyw0xx60 Button--medium Button--onlyIcon p1 emotion-18xm8tu emiw9oj2"
        result="[object Object]"
      >
        <div
          class="emotion-zxs4gr emiw9oj1"
        >
          <svg
            aria-label="refresh icon"
            class="Icon Icon-refresh"
            clip-rule="evenodd"
            fill="currentcolor"
            fill-rule="evenodd"
            height="16"
            role="img"
            viewBox="0 0 16 16"
            width="16"
            xmlns="http://www.w3.org/2000/svg"
          >
            <path
              d="M5.264 3.52a5.25 5.25 0 0 1 6.41.73H10.5a.75.75 0 0 0 0 1.5h3a.75.75 0 0 0 .75-.75V2a.75.75 0 0 0-1.5 0v1.204a6.75 6.75 0 1 0 2 4.796.75.75 0 0 0-1.5 0 5.25 5.25 0 1 1-7.986-4.48z"
            />
          </svg>
        </div>
      </button>
    </div>
    <div
      class="emotion-1wlwghu e190m98b1"
    />
    <div
      class="emotion-1x0k1ds eanyi781"
    >
      <button
        class="Button e1j0gl1h0 Button--onlyIcon emotion-htlzfv emiw9oj2"
      >
        <div
          class="emotion-zxs4gr emiw9oj1"
        >
          <div
            class=" emotion-1w35l9a emiw9oj0"
          >
            <svg
              aria-label="bookmark icon"
              class="Icon Icon-bookmark emotion-19207x7 e1j0gl1h1"
              fill="currentcolor"
              height="16"
              role="img"
              viewBox="0 0 16 16"
              width="16"
              xmlns="http://www.w3.org/2000/svg"
            >
              <path
                d="M3.25 3c0-.966.784-1.75 1.75-1.75h6c.966 0 1.75.784 1.75 1.75v11a.75.75 0 0 1-1.28.53L8 11.06l-3.47 3.47A.75.75 0 0 1 3.25 14V3zM5 2.75a.25.25 0 0 0-.25.25v9.19l2.72-2.72a.75.75 0 0 1 1.06 0l2.72 2.72V3a.25.25 0 0 0-.25-.25H5z"
              />
            </svg>
          </div>
        </div>
      </button>
    </div>
    <div
      class="emotion-1x0k1ds eanyi781"
    >
      <button
        class="Button eyw0xx60 Button--onlyIcon p1 emotion-1h09fm5 emiw9oj2"
        data-testid="qb-header-info-button"
      >
        <div
          class="emotion-zxs4gr emiw9oj1"
        >
          <svg
            aria-label="info icon"
            class="Icon Icon-info"
            fill="currentcolor"
            height="16"
            role="img"
            viewBox="0 0 16 16"
            width="16"
            xmlns="http://www.w3.org/2000/svg"
          >
            <path
              d="M7.75 4.5a1.25 1.25 0 1 0 0-2.5 1.25 1.25 0 0 0 0 2.5zm-2 2.5a.75.75 0 0 1 .75-.75H8a.75.75 0 0 1 .75.75v5.25H10a.75.75 0 1 1 0 1.5H6a.75.75 0 0 1 0-1.5h1.25v-4.5H6.5A.75.75 0 0 1 5.75 7z"
            />
          </svg>
        </div>
      </button>
    </div>
    <div
      class=" emotion-16y0tgk ehi2edu0"
    >
      <button
        aria-label="Move, archive, and more..."
        class="Button e1gdjdn70 Button--onlyIcon p1 emotion-5elapw emiw9oj2"
      >
        <div
          class="emotion-zxs4gr emiw9oj1"
        >
          <svg
            aria-label="ellipsis icon"
            class="Icon Icon-ellipsis"
            fill="currentcolor"
            height="16"
            role="img"
            viewBox="0 0 16 16"
            width="16"
            xmlns="http://www.w3.org/2000/svg"
          >
            <path
              d="M4.5 8A1.25 1.25 0 1 1 2 8a1.25 1.25 0 0 1 2.5 0zm4.75 0a1.25 1.25 0 1 1-2.5 0 1.25 1.25 0 0 1 2.5 0zm3.5 1.25a1.25 1.25 0 1 0 0-2.5 1.25 1.25 0 0 0 0 2.5z"
            />
          </svg>
        </div>
      </button>
      <span
        class="hide"
      />
    </div>
  </div>
</div>
    at assertIsNotAdHoc (http://localhost:4000/__cypress/tests?p=e2e/test/scenarios/question/reproductions/18978-18977-nested-question-nodata-user.cy.spec.js:44871:53)
    at Context.eval (http://localhost:4000/__cypress/tests?p=e2e/test/scenarios/question/reproductions/18978-18977-nested-question-nodata-user.cy.spec.js:44835:9)
      AssertionError: Timed out retrying after 4000ms: Unable to find an element with the text: Save. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.
Ignored nodes: comments, <script />, <style />
<div
  class="eb9z3dt4 eanyi7817 emotion-9e76f1 e11094na0"
  data-testid="qb-header"
  style="opacity: 1;"
>
  <div
    class="emotion-1o97tm2 eanyi784"
    data-testid="qb-header-left-side"
  >
    <div
      class="emotion-1o7d3sk eanyi7816"
    >
      <div
        class="emotion-2y363g eanyi7810"
      >
        <span
          class="emotion-rq1m63 e1grb1gu1"
          data-testid="head-crumbs-container"
        >
          <div
            class="emotion-1npab0 egv21s31"
          >
            <div
              class="egv21s30 emotion-1hali18 eoa5drl1"
              data-testid="editable-text"
              data-value="test question "
            >
              <textarea
                class="emotion-pojurp eoa5drl0"
                data-testid="saved-question-header-title"
                placeholder="Add title"
              >
                test question
              </textarea>
            </div>
          </div>
        </span>
      </div>
    </div>
    <div
      class="emotion-1kv07fc eanyi7815"
    >
      <button
        class="Button eanyi787 e13bobqq0 emotion-1j2zjzk emiw9oj2"
        data-testid="revision-history-button"
      >
        <div
          class="emotion-zxs4gr emiw9oj1"
        >
          <div
            class=" emotion-1w35l9a emiw9oj0"
          >
            Edited
            a few seconds ago by Bobby Tables
          </div>
        </div>
      </button>
    </div>
  </div>
  <div
    class="emotion-1mjo75b eanyi782"
    data-testid="qb-header-action-panel"
  >
    <div
      class="emotion-1x0k1ds eanyi781"
    >
      <button
        aria-label="Refresh"
        class="Button eanyi780 RunButton eyw0xx60 Button--medium Button--onlyIcon p1 emotion-18xm8tu emiw9oj2"
        result="[object Object]"
      >
        <div
          class="emotion-zxs4gr emiw9oj1"
        >
          <svg
            aria-label="refresh icon"
            class="Icon Icon-refresh"
            clip-rule="evenodd"
            fill="currentcolor"
            fill-rule="evenodd"
            height="16"
            role="img"
            viewBox="0 0 16 16"
            width="16"
            xmlns="http://www.w3.org/2000/svg"
          >
            <path
              d="M5.264 3.52a5.25 5.25 0 0 1 6.41.73H10.5a.75.75 0 0 0 0 1.5h3a.75.75 0 0 0 .75-.75V2a.75.75 0 0 0-1.5 0v1.204a6.75 6.75 0 1 0 2 4.796.75.75 0 0 0-1.5 0 5.25 5.25 0 1 1-7.986-4.48z"
            />
          </svg>
        </div>
      </button>
    </div>
    <div
      class="emotion-1wlwghu e190m98b1"
    />
    <div
      class="emotion-1x0k1ds eanyi781"
    >
      <button
        class="Button e1j0gl1h0 Button--onlyIcon emotion-htlzfv emiw9oj2"
      >
        <div
          class="emotion-zxs4gr emiw9oj1"
        >
          <div
            class=" emotion-1w35l9a emiw9oj0"
          >
            <svg
              aria-label="bookmark icon"
              class="Icon Icon-bookmark emotion-19207x7 e1j0gl1h1"
              fill="currentcolor"
              height="16"
              role="img"
              viewBox="0 0 16 16"
              width="16"
              xmlns="http://www.w3.org/2000/svg"
            >
              <path
                d="M3.25 3c0-.966.784-1.75 1.75-1.75h6c.966 0 1.75.784 1.75 1.75v11a.75.75 0 0 1-1.28.53L8 11.06l-3.47 3.47A.75.75 0 0 1 3.25 14V3zM5 2.75a.25.25 0 0 0-.25.25v9.19l2.72-2.72a.75.75 0 0 1 1.06 0l2.72 2.72V3a.25.25 0 0 0-.25-.25H5z"
              />
            </svg>
          </div>
        </div>
      </button>
    </div>
    <div
      class="emotion-1x0k1ds eanyi781"
    >
      <button
        class="Button eyw0xx60 Button--onlyIcon p1 emotion-1h09fm5 emiw9oj2"
        data-testid="qb-header-info-button"
      >
        <div
          class="emotion-zxs4gr emiw9oj1"
        >
          <svg
            aria-label="info icon"
            class="Icon Icon-info"
            fill="currentcolor"
            height="16"
            role="img"
            viewBox="0 0 16 16"
            width="16"
            xmlns="http://www.w3.org/2000/svg"
          >
            <path
              d="M7.75 4.5a1.25 1.25 0 1 0 0-2.5 1.25 1.25 0 0 0 0 2.5zm-2 2.5a.75.75 0 0 1 .75-.75H8a.75.75 0 0 1 .75.75v5.25H10a.75.75 0 1 1 0 1.5H6a.75.75 0 0 1 0-1.5h1.25v-4.5H6.5A.75.75 0 0 1 5.75 7z"
            />
          </svg>
        </div>
      </button>
    </div>
    <div
      class=" emotion-16y0tgk ehi2edu0"
    >
      <button
        aria-label="Move, archive, and more..."
        class="Button e1gdjdn70 Button--onlyIcon p1 emotion-5elapw emiw9oj2"
      >
        <div
          class="emotion-zxs4gr emiw9oj1"
        >
          <svg
            aria-label="ellipsis icon"
            class="Icon Icon-ellipsis"
            fill="currentcolor"
            height="16"
            role="img"
            viewBox="0 0 16 16"
            width="16"
            xmlns="http://www.w3.org/2000/svg"
          >
            <path
              d="M4.5 8A1.25 1.25 0 1 1 2 8a1.25 1.25 0 0 1 2.5 0zm4.75 0a1.25 1.25 0 1 1-2.5 0 1.25 1.25 0 0 1 2.5 0zm3.5 1.25a1.25 1.25 0 1 0 0-2.5 1.25 1.25 0 0 0 0 2.5z"
            />
          </svg>
        </div>
      </button>
      <span
        class="hide"
      />
    </div>
  </div>
</div>
    at assertIsNotAdHoc (http://localhost:4000/__cypress/tests?p=e2e/test/scenarios/question/reproductions/18978-18977-nested-question-nodata-user.cy.spec.js:44871:53)
    at Context.eval (http://localhost:4000/__cypress/tests?p=e2e/test/scenarios/question/reproductions/18978-18977-nested-question-nodata-user.cy.spec.js:44835:9)
⚠️ 1 Flaky
  • should convert 'is empty' on a text column to a custom expression using IsEmpty() 
      Timed out retrying after 4000ms: Expected to find content: 'Showing 1,112 rows' but never did.
✅ 2219 Passed

@nemanjaglumac nemanjaglumac force-pushed the qc-mlv2-37516 branch 2 times, most recently from 661496a to 97bdc11 Compare January 18, 2024 13:50
@nemanjaglumac
Use MLv2 for structured query hasData check
edfc56c 
Resolves #37516
@nemanjaglumac
Remove redundant checks
1cf5379
@nemanjaglumac
Update a related E2E test
e339c0e 
- This commit makes the test more explicit and more understandable.

- Fix the test assumption
In all other cases, we show the Save button to the nodata user,
but we disable it. It was only in this test that we wrongly asserted
that the button doesn't exist at all.
@nemanjaglumac
Deprecate hasData() methods
9504f8d
nemanjaglumac
nemanjaglumac commented Jan 18, 2024
View reviewed changes
frontend/src/metabase/query_builder/components/view/ViewHeader.jsx
Comment on lines -420 to -422
const isNewQuery = !question
.legacyQuery({ useStructuredQuery: true })
.hasData();
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this condition was wrong all along. Or to be more precise, negating the hasData check was wrong. It only worked because of a fallback condition.
https://github.com/metabase/metabase/pull/37566/files#diff-7eae47ba62c31da65c448230eacac88be86470a204d450011acb631d7bb74144L426

But even the fallback was wrong.
canEditQuery shouldn't affect whether or not the button is shown. It only affects whether or not the button is disabled. This caused a bug where users without data permissions wouldn't even see the "Save" button.

The existing E2E test was originally written with that assumption in mind. I've updated the test.
"Nodata" user should see a disabled Save button.

@nemanjaglumac nemanjaglumac changed the title Use MLv2 for structured query hasData check Remove redundant (and wrong) hasData check from ViewHeader component Jan 18, 2024
@nemanjaglumac nemanjaglumac merged commit 1aa7ae3 into master Jan 18, 2024
106 checks passed
@nemanjaglumac nemanjaglumac deleted the qc-mlv2-37516 branch January 18, 2024 15:50
@github-actions GitHub Actions
Copy link

@nemanjaglumac 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
Reviewers

@ranquild ranquild ranquild approved these changes

Assignees

@nemanjaglumac nemanjaglumac

Labels
no-backport Do not backport this PR to any branch .Team/QueryingComponents
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[MLv2] [FE] ViewHeader - hasData check
2 participants
@nemanjaglumac @ranquild