Skip to content

Commit

Permalink
Fix for #21984: Viewing tables in Data Model is registered in "Recent…
Browse files Browse the repository at this point in the history
…ly viewed" (#23780) (#24188)

* Add explicit ignore_true parameter to Tables.load requests in admin panel to avoid counting the requests as a table view

* Remove .skip for repro

* Fix formatting

* Adapt repro to the fix

Repro was written before I know what the fix would look like.

We're conditionally choosing whether to display `HomePopularSection` or `HomeRecentSection`.
With this fix applied, data model table visit is not registered any more so there is nothing else really
that the test user visited. Hence, "Pick where you left off" is never displayed on the page and we
display `HomePopularSection` instead.

This adaption reflects those changes.

* Revert "Add explicit ignore_true parameter to Tables.load requests in admin panel to avoid counting the requests as a table view"

This reverts commit f887d0d.

* Remove :table-read event from GET api/table/:id endpoint

* Remove unused ns

Co-authored-by: Nemanja <31325167+nemanjaglumac@users.noreply.github.com>

Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com>
Co-authored-by: Nemanja <31325167+nemanjaglumac@users.noreply.github.com>
  • Loading branch information
3 people committed Aug 1, 2022
1 parent a29e84e commit e85e261
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const { REVIEWS_ID } = SAMPLE_DATABASE;

const reviewsDataModelPage = `/admin/datamodel/database/${SAMPLE_DB_ID}/table/${REVIEWS_ID}`;

describe.skip("issue 21984", () => {
describe("issue 21984", () => {
beforeEach(() => {
cy.intercept("GET", "/api/table/*/query_metadata?**").as("tableMetadata");

Expand All @@ -23,14 +23,11 @@ describe.skip("issue 21984", () => {
it('should not show data model visited tables in search or in "Pick up where you left off" items on homepage (metabase#21984)', () => {
cy.visit("/");

cy.findByText("Pick up where you left off")
.parent()
.within(() => {
cy.findByText("Reviews").should("not.exist");
});
cy.findByText("Metabase tips");
cy.findByText("Pick up where you left off").should("not.exist");

cy.findByPlaceholderText("Search…").click();
cy.findByText("Recently viewed");
cy.findAllByTestId("recently-viewed-item").should("not.contain", "Reviews");
cy.findByText("Nothing here");
});
});
6 changes: 2 additions & 4 deletions src/metabase/api/table.clj
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
[metabase.api.common :as api]
[metabase.driver :as driver]
[metabase.driver.util :as driver.u]
[metabase.events :as events]
[metabase.models.card :refer [Card]]
[metabase.models.field :refer [Field]]
[metabase.models.field-values :as field-values :refer [FieldValues]]
Expand Down Expand Up @@ -46,9 +45,8 @@
(let [api-perm-check-fn (if (Boolean/parseBoolean include_editable_data_model)
api/write-check
api/read-check)]
(u/prog1 (-> (api-perm-check-fn Table id)
(hydrate :db :pk_field))
(events/publish-event! :table-read (assoc <> :actor_id api/*current-user-id*)))))
(-> (api-perm-check-fn Table id)
(hydrate :db :pk_field))))

(defn- update-table!*
"Takes an existing table and the changes, updates in the database and optionally calls `table/update-field-positions!`
Expand Down

0 comments on commit e85e261

Please sign in to comment.