fixed provider filter in resources #4443
Merged
+117
−78
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Walkthrough
The changes update mock data for components and documents by splitting a "jupyter" entry into two with distinct metadata, adding a new mock document for "test-provider," and modifying test assertions to reflect increased resource counts and the new provider. The document filter logic is refined for more precise property checks. Additionally, multiple components and hooks replace the custom `useQueryParams` hook with the standard `useSearchParams` from `react-router-dom` for URL query parameter handling, and the deprecated `useQueryParams` hook is removed.
## Changes
| File(s) | Change Summary |
|------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------|
| frontend/src/__mocks__/mockComponents.ts | Split "jupyter" mock entry into two: one renamed "Start basic workbench" with empty provider, one as "test-provider" with updated fields and icons. |
| frontend/src/__mocks__/mockDocs.ts | Added a new mock document for "test-provider" with unique metadata and spec. |
| frontend/src/__tests__/cypress/cypress/tests/mocked/resources/resources.cy.ts | Updated test assertions: increased expected counts, replaced "Jupyter" provider filter with "test-provider". |
| frontend/src/pages/learningCenter/useDocFilterer.ts | Refined filtering logic: replaced custom hook with `useSearchParams`, simplified property checks, improved null/undefined handling. |
| frontend/src/__tests__/cypress/cypress/pages/components/JupyterCard.ts | Updated drawer panel heading text from "Jupyter" to "Start basic workbench" in test. |
| frontend/src/__tests__/cypress/cypress/tests/mocked/applications/enabled.cy.ts | Changed expected Jupyter card title text to "Start basic workbench" in test. |
| frontend/src/__tests__/cypress/cypress/tests/mocked/applications/explore.cy.ts | Renamed test case and updated assertions from "Jupyter" to "Start basic workbench"; removed provider text check. |
| frontend/src/pages/exploreApplication/ExploreApplications.tsx | Replaced custom `useQueryParams` hook with `useSearchParams` for reading `selectId` query parameter. |
| frontend/src/pages/learningCenter/CategoryFilters.tsx | Replaced `useQueryParams` with `useSearchParams` for reading category query parameter. |
| frontend/src/pages/learningCenter/LearningCenter.tsx | Replaced `useQueryParams` with `useSearchParams` for reading sorting parameters. |
| frontend/src/pages/learningCenter/LearningCenterFilters.tsx | Replaced `useQueryParams` with `useSearchParams` for reading category filter parameter. |
| frontend/src/pages/learningCenter/LearningCenterListHeader.tsx | Replaced `useQueryParams` with `useSearchParams` for reading sorting parameters. |
| frontend/src/pages/learningCenter/LearningCenterToolbar.tsx | Replaced `useQueryParams` with `useSearchParams` for reading URL query parameters. |
| frontend/src/pages/learningCenter/useQueryFilters.ts | Replaced `useQueryParams` with `useSearchParams` for reading filter query parameters. |
| frontend/src/utilities/useQueryParams.tsx | Deleted deprecated custom `useQueryParams` hook. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant UI
participant useDocFilterer
participant mockDocs
User->>UI: Applies filters (type, application, provider)
UI->>useDocFilterer: Calls filter function with query params
useDocFilterer->>mockDocs: Retrieves documents
useDocFilterer->>useDocFilterer: Applies type, application, provider filters
useDocFilterer-->>UI: Returns filtered docs
UI-->>User: Displays filtered resultsSuggested labels
Suggested reviewers
Poem
Learnt from: Gkrumbach07 |
|
@DaoDaoNoCode this was caused by us removing the "Jupyter" name from the provider during the manifest changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/__mocks__/mockComponents.ts (1)
33-61: Consider extracting the duplicated getStartedMarkDown content.The new "test-provider" entry is well-structured for testing provider filtering. However, the extensive getStartedMarkDown content is duplicated between both entries.
Consider extracting the common markdown content to reduce duplication:
+const JUPYTER_GETTING_STARTED_MARKDOWN = '# Jupyter\nLaunch Jupyter and start a notebook server...'; export const mockComponents = (): OdhApplication[] => [ { // ... first entry spec: { // ... other fields - getStartedMarkDown: '# Jupyter\nLaunch Jupyter and start a notebook server...', + getStartedMarkDown: JUPYTER_GETTING_STARTED_MARKDOWN, }, }, { // ... second entry spec: { // ... other fields - getStartedMarkDown: '# Jupyter\nLaunch Jupyter and start a notebook server...', + getStartedMarkDown: JUPYTER_GETTING_STARTED_MARKDOWN, }, }, ];frontend/src/__tests__/cypress/cypress/pages/components/JupyterCard.ts (1)
19-19: Correctly updated selector text to match new display name.The text selector properly reflects the updated
displayNamefrom the mock data.Consider whether the class name
JupyterCardshould be updated to reflect the new display name, or add a comment explaining that it still represents the same component despite the name change:+// Note: Class name retained as JupyterCard for backwards compatibility, +// even though display name changed to "Start basic workbench" export class JupyterCard extends Card {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/__mocks__/mockComponents.ts(1 hunks)frontend/src/__tests__/cypress/cypress/pages/components/JupyterCard.ts(1 hunks)frontend/src/__tests__/cypress/cypress/tests/mocked/applications/enabled.cy.ts(1 hunks)frontend/src/__tests__/cypress/cypress/tests/mocked/applications/explore.cy.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
frontend/src/__tests__/cypress/cypress/tests/mocked/applications/enabled.cy.ts (1)
Learnt from: christianvogt
PR: opendatahub-io/odh-dashboard#4381
File: frontend/src/__tests__/cypress/tsconfig.json:9-9
Timestamp: 2025-06-19T20:38:32.485Z
Learning: In the ODH Dashboard project, the `frontend/src/__tests__/cypress/tsconfig.json` file intentionally has an empty `files` array to disable type checking for Cypress test files. This is part of the monorepo structure where Cypress was separated into its own package but type checking is deliberately disabled for it.
frontend/src/__tests__/cypress/cypress/pages/components/JupyterCard.ts (1)
Learnt from: christianvogt
PR: opendatahub-io/odh-dashboard#4381
File: frontend/src/__tests__/cypress/tsconfig.json:9-9
Timestamp: 2025-06-19T20:38:32.485Z
Learning: In the ODH Dashboard project, the `frontend/src/__tests__/cypress/tsconfig.json` file intentionally has an empty `files` array to disable type checking for Cypress test files. This is part of the monorepo structure where Cypress was separated into its own package but type checking is deliberately disabled for it.
frontend/src/__tests__/cypress/cypress/tests/mocked/applications/explore.cy.ts (1)
Learnt from: christianvogt
PR: opendatahub-io/odh-dashboard#4381
File: frontend/src/__tests__/cypress/tsconfig.json:9-9
Timestamp: 2025-06-19T20:38:32.485Z
Learning: In the ODH Dashboard project, the `frontend/src/__tests__/cypress/tsconfig.json` file intentionally has an empty `files` array to disable type checking for Cypress test files. This is part of the monorepo structure where Cypress was separated into its own package but type checking is deliberately disabled for it.
frontend/src/__mocks__/mockComponents.ts (1)
Learnt from: christianvogt
PR: opendatahub-io/odh-dashboard#4381
File: frontend/src/__tests__/cypress/tsconfig.json:9-9
Timestamp: 2025-06-19T20:38:32.485Z
Learning: In the ODH Dashboard project, the `frontend/src/__tests__/cypress/tsconfig.json` file intentionally has an empty `files` array to disable type checking for Cypress test files. This is part of the monorepo structure where Cypress was separated into its own package but type checking is deliberately disabled for it.
🧬 Code Graph Analysis (1)
frontend/src/__tests__/cypress/cypress/tests/mocked/applications/explore.cy.ts (1)
frontend/src/__tests__/cypress/cypress/pages/components/JupyterCard.ts (1)
jupyterCard(24-24)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cypress-Setup
- GitHub Check: Lint
🔇 Additional comments (3)
frontend/src/__mocks__/mockComponents.ts (1)
18-31: Good approach for provider filter testing.The changes to split the jupyter entry and clear the provider field support the stated objective of fixing provider filter functionality. The empty provider field for "Start basic workbench" creates a good test case for components without providers.
frontend/src/__tests__/cypress/cypress/tests/mocked/applications/enabled.cy.ts (1)
16-16: Correctly updated to match mock data changes.The assertion update properly reflects the new
displayNamevalue from the mock data, maintaining test accuracy.frontend/src/__tests__/cypress/cypress/tests/mocked/applications/explore.cy.ts (1)
11-13: Test updates consistently reflect the mock data changes.Both the test name and assertion have been properly updated to match the new
displayNamevalue, maintaining test accuracy and readability.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4443 +/- ##
==========================================
- Coverage 82.61% 82.61% -0.01%
==========================================
Files 1756 1756
Lines 36696 36697 +1
Branches 10859 10860 +1
==========================================
- Hits 30318 30316 -2
- Misses 6378 6381 +3
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
rsun19
changed the title
rsun19
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DaoDaoNoCode The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.redhat.com/browse/RHOAIENG-28106
Description
How Has This Been Tested?
Go to the providers checkbox in resources, and click on a provider. It should correctly filter now.
Actually, since I refactored a lot of the deprecated
useQueryParams()method, you should also test by clicking on all of the possible filters in the resources page. And ensure the enabled and explore pages work properly.Test Impact
Updated Cypress tests
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
useSearchParamsfromreact-router-domacross multiple components and hooks.