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

Make Bookmarks Normal #38443

Closed
wants to merge 4 commits into from
Closed

Make Bookmarks Normal #38443

wants to merge 4 commits into from

Conversation

iethree
Copy link
Contributor

@iethree iethree commented Feb 5, 2024

Description

Instead of having a special, confusing, data format for bookmarks, make them follow something similar to the search payload that will play nicer with other data in the app.

Fixes #36273
Also fixes bookmarked verified collections icon (no issue for that one)

Screen Shot 2024-02-05 at 9 56 34 AM

Checklist

  • Tests have been added/updated to cover changes in this PR

@iethree iethree requested a review from camsaul as a code owner February 5, 2024 16:57
@metabase-bot metabase-bot bot added the .Team/AdminWebapp Admin and Webapp team label Feb 5, 2024
@iethree iethree added the no-backport Do not backport this PR to any branch label Feb 5, 2024
Copy link

replay-io bot commented Feb 5, 2024

StatusComplete ↗︎
Commit21eeb89
Results
3 Failed
  • can add, update bookmark name when collection name is updated, and remove bookmarks from collection from its page
      AssertionError: Timed out retrying after 4000ms: Unable to find an element with the text: First collection 2. 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 />
      <aside
        aria-hidden="false"
        class="Nav emotion-uhlg79 exp4uno13"
        data-testid="main-navbar-root"
      >
        <nav
          class="emotion-11066ww exp4uno12"
        >
          <div
            class="emot...
  • should add, update bookmark name when dashboard name is updated, and then remove bookmark
      AssertionError: Timed out retrying after 4000ms: Unable to find an element with the text: Orders in a dashboard 2. 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 />
      <aside
        aria-hidden="false"
        class="Nav emotion-uhlg79 exp4uno13"
        data-testid="main-navbar-root"
      >
        <nav
          class="emotion-11066ww exp4uno12"
        >
          <div
            class=...
  • should add, update bookmark name when question name is updated, then remove bookmark from question page
      AssertionError: Timed out retrying after 4000ms: Unable to find an element with the text: Orders 2. 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 />
      <aside
        aria-hidden="true"
        class="Nav emotion-1xyrsax exp4uno13"
        data-testid="main-navbar-root"
      >
        <nav
          class="emotion-oop1v8 exp4uno12"
        >
          <div
            class="emotion-1agdudp...
⚠️ 3 Flaky
2263 Passed

@iethree
Copy link
Contributor Author

iethree commented Feb 5, 2024

The failed tests here revealed a hiccup: by making bookmarks normal, we now have possible id collisions because our entity store always works off of the id property - so we can't bookmark both collection 37 and dashboard 37.

I need to dig into whether I can teach the entity system to use bookmark_id 🤔

PS. hooray for e2e tests that caught this!

@iethree iethree marked this pull request as draft February 8, 2024 23:27
@iethree iethree closed this Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/AdminWebapp Admin and Webapp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Audit V2] bookmarking Metabase analytics has a different icon
2 participants