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

Add filters to Browse models #38560

Merged
merged 43 commits into from
Feb 9, 2024
Merged

Add filters to Browse models #38560

merged 43 commits into from
Feb 9, 2024

Conversation

rafpaf
Copy link
Contributor

@rafpaf rafpaf commented Feb 8, 2024 •
edited
Loading

This PR adds filters to Browse models

image

image

@rafpaf rafpaf self-assigned this Feb 8, 2024
@rafpaf rafpaf changed the base branch from master to models-in-browse-data-polish February 8, 2024 16:15
@metabase-bot metabase-bot bot added the .Team/AdminWebapp Admin and Webapp team label Feb 8, 2024
@rafpaf rafpaf force-pushed the models-in-browse-data-polish branch from d19a4ee to eea3b38 Compare February 8, 2024 16:16
@rafpaf rafpaf force-pushed the models-in-browse-data--filters branch from d144f2e to f6e8547 Compare February 8, 2024 19:23
Base automatically changed from models-in-browse-data-polish to master February 8, 2024 20:07
@rafpaf rafpaf added this to the 0.49 milestone Feb 8, 2024
@rafpaf rafpaf removed the request for review from npfitz February 8, 2024 21:36
@rafpaf rafpaf marked this pull request as ready for review February 8, 2024 21:36
@rafpaf rafpaf requested a review from camsaul as a code owner February 8, 2024 21:36
@rafpaf rafpaf force-pushed the models-in-browse-data--filters branch from 88b11e0 to 0bfe280 Compare February 9, 2024 15:26
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 9, 2024 •
edited
Loading

Codenotify: Notifying subscribers in CODENOTIFY files for diff a2d799f...8fd30a6.

Notify File(s)
@kdoh frontend/src/metabase/ui/components/icons/Icon/icons/index.ts
frontend/src/metabase/ui/components/icons/Icon/icons/model_with_badge.svg

@rafpaf rafpaf changed the base branch from master to models-in-browse-data-polish February 9, 2024 15:28
@replay-io replay.io
Copy link

replay-io bot commented Feb 9, 2024 •
edited
Loading

Status In Progress ↗︎ 51 / 52
Commit 8fd30a6
Results
❌ 4 Failed
⚠️ 2 Flaky
✅ 2295 Passed

@rafpaf rafpaf force-pushed the models-in-browse-data-polish branch from b744b3f to bccc6a8 Compare February 9, 2024 17:58
@rafpaf rafpaf force-pushed the models-in-browse-data--filters branch from d67ccf7 to 8c8ccf8 Compare February 9, 2024 18:34
@rafpaf rafpaf force-pushed the models-in-browse-data--filters branch from 8c8ccf8 to ed47524 Compare February 9, 2024 18:43
iethree
iethree requested changes Feb 9, 2024
View reviewed changes
Copy link
Contributor

@iethree iethree left a comment

Choose a reason for hiding this comment

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

  1. I think this branch needs a rebase, it has some divergent changes from what I saw in Browse models: Set default tab #38291
  2. we need to extract the enterprise specific logic and components from the OSS codebase. We do this by putting some placeholders in the OSS codebase that the the enterprise code can hook into.

enterprise/frontend/src/metabase-enterprise/moderation/index.js Show resolved Hide resolved
src/metabase/public_settings.clj Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

this can go away now that we're storing in localStorage, right?

frontend/src/metabase-types/api/mocks/settings.ts Outdated
@@ -149,6 +149,7 @@ export const createMockSettings = (
"custom-homepage-dashboard": null,
"help-link": "metabase",
"help-link-custom-destination": "",
"default-browse-tab": null,
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this now that we're storing this in localStorage, right?

frontend/src/metabase-types/api/settings.ts Outdated
@@ -203,6 +203,7 @@ export interface Settings {
"custom-formatting": FormattingSettings;
"custom-homepage": boolean;
"custom-homepage-dashboard": number | null;
"default-browse-tab": string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this now that we're storing this in localStorage, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remnant of some rebasing chaos, now resolved

frontend/src/metabase/browse/components/BrowseApp.tsx
Comment on lines +43 to +53
const [onlyShowVerifiedModels, setOnlyShowVerifiedModels] = useState(
localStorage.getItem("onlyShowVerifiedModelsInBrowseData") !== "false",
);

const changeOnlyShowVerifiedModels = (newValue: boolean) => {
localStorage.setItem(
"onlyShowVerifiedModelsInBrowseData",
newValue ? "true" : "false",
);
setOnlyShowVerifiedModels(newValue);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

we can't have this enterprise-only logic in an OSS component

frontend/src/metabase/browse/components/BrowseApp.tsx Outdated
Comment on lines 59 to 72
// If no children specified, use the tab id to determine what to show inside the tab
if (!children) {
if (tab === "models") {
children = (
<BrowseModels
modelsResult={modelsResult}
onlyShowVerifiedModels={onlyShowVerifiedModels}
/>
);
}
if (tab === "databases") {
children = <BrowseDatabases databasesResult={databasesResult} />;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this logic changed in another branch that merged recently, right? IIRC you extracted it to a component.

frontend/src/metabase/browse/components/BrowseApp.tsx
return (
<BrowseAppRoot data-testid="browse-app">
<BrowseContainer>
<BrowseDataHeader>
<BrowseSectionContainer>
<Flex maw="1014px" m="0 auto" w="100%" align="center">
Copy link
Contributor

Choose a reason for hiding this comment

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

can we avoid this pixel measure? maybe 64rem (1024px) ?

Suggested change
<Flex maw="1014px" m="0 auto" w="100%" align="center">
<Flex maw="1014px" m="0 auto" w="100%" align="center">

frontend/src/metabase/browse/components/BrowseApp.tsx
Comment on lines +99 to +107
<Switch
ml="auto"
size="sm"
labelPosition="left"
checked={onlyShowVerifiedModels}
label={<strong>{t`Only show verified models`}</strong>}
onChange={e => {
changeOnlyShowVerifiedModels(e.target.checked);
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

this switch component can't be in the OSS codebase, we need to import a component from a plugin like ExtraModelsFilters or something. check out what @npfitz did in a similar case here:

https://github.com/metabase/metabase/pull/38573/files#diff-d45380b47cb0156cc91e1f8d02bd70a25c6feb1f1de33db8dbf5cfa3ba8ce4cbR593

frontend/src/metabase/browse/components/BrowseApp.tsx
return (
<BrowseModels
modelsResult={modelsResult}
onlyShowVerifiedModels={onlyShowVerifiedModels}
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 passing down an enterprise-specifc property like this, I would maybe make state a generic filterProperties or something like that.

frontend/src/metabase/browse/components/BrowseModels.tsx
Comment on lines +52 to +63
const modelsFiltered = onlyShowVerifiedModels
? models.filter(
(model: SearchResult) => model.moderated_status === "verified",
)
: models;
const modelsWithoutInstanceAnalyticsCollection = modelsFiltered.filter(
(model: SearchResult) => !isInstanceAnalyticsCollection(model.collection),
);

const groupsOfModels = groupModels(
modelsWithoutInstanceAnalyticsCollection,
localeCode,
Copy link
Contributor

Choose a reason for hiding this comment

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

this logic must be in the enterprise folder

@rafpaf rafpaf merged commit b521141 into models-in-browse-data-polish Feb 9, 2024
101 of 103 checks passed
@rafpaf rafpaf deleted the models-in-browse-data--filters branch February 9, 2024 20:15
@rafpaf rafpaf restored the models-in-browse-data--filters branch February 12, 2024 03:53
@rafpaf rafpaf mentioned this pull request Feb 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iethree iethree iethree requested changes

@naatz-co naatz-co naatz-co approved these changes

@camsaul camsaul Awaiting requested review from camsaul camsaul is a code owner

Assignees

@rafpaf rafpaf

Labels
.Team/AdminWebapp Admin and Webapp team
Projects
None yet
Milestone
0.49
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rafpaf @naatz-co @iethree @kdoh @luizarakaki