[OSDEV-1700] Hide redundand previous os ids from the search result#507
Merged
VadimKovalenkoSNF merged 12 commits intomainfrom Feb 13, 2025
Merged
[OSDEV-1700] Hide redundand previous os ids from the search result#507VadimKovalenkoSNF merged 12 commits intomainfrom
VadimKovalenkoSNF merged 12 commits intomainfrom
Conversation
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
📝 WalkthroughWalkthroughThis PR updates the release notes and the Changes
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
src/react/src/components/Contribute/ProductionLocationDetails.jsx (1)
21-25: Consider using optional chaining for cleaner code.The location pathname check can be simplified using optional chaining.
- const location = useLocation(); - let osIdSearchParameter = ''; - if (location && location.pathname) { - osIdSearchParameter = getLastPathParameter(location.pathname); - } + const location = useLocation(); + const osIdSearchParameter = getLastPathParameter(location?.pathname ?? '');🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
doc/release/RELEASE-NOTES.md (1)
54-54: Fix typo in bug fix entry.There is a grammatical error in the text.
-* [OSDEV-1700](https://opensupplyhub.atlassian.net/browse/OSDEV-1700) - SLC. Keep only one previous OS ID in the search result if it is matches search query. +* [OSDEV-1700](https://opensupplyhub.atlassian.net/browse/OSDEV-1700) - SLC. Keep only one previous OS ID in the search result if it matches search query.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
doc/release/RELEASE-NOTES.md(1 hunks)src/react/src/__tests__/components/ProductionLocationDetails.test.js(3 hunks)src/react/src/__tests__/utils.tests.js(2 hunks)src/react/src/components/Contribute/ProductionLocationDetails.jsx(3 hunks)src/react/src/util/util.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/react/src/components/Contribute/ProductionLocationDetails.jsx (1)
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#470
File: src/react/src/components/Contribute/ProductionLocationInfo.jsx:229-243
Timestamp: 2025-01-23T10:19:59.642Z
Learning: In React useEffect hooks that perform navigation, adding router-related dependencies (history, location) to the dependency array can cause unwanted re-renders and side effects. For navigation effects triggered by data changes, include only the data dependencies.
🪛 Biome (1.9.4)
src/react/src/components/Contribute/ProductionLocationDetails.jsx
[error] 23-23: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-django-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
🔇 Additional comments (5)
src/react/src/components/Contribute/ProductionLocationDetails.jsx (2)
2-2: LGTM! Clean import additions.The new imports for
useLocationandgetLastPathParameterare well-organized and necessary for the new functionality.Also applies to: 8-8
38-52: LGTM! Well-implemented filtering logic.The filtering of historical OS IDs is efficient and correctly implemented:
- Proper use of filter to match against the search parameter
- Correct key prop usage for list rendering
- Clean JSX structure
src/react/src/__tests__/components/ProductionLocationDetails.test.js (1)
6-109: LGTM! Excellent test coverage.The test suite is comprehensive and well-structured:
- Proper mocking of the useLocation hook
- Clear test cases for various scenarios
- Good cleanup with beforeEach
src/react/src/util/util.js (1)
7-8: LGTM! Clean utility function implementation.The getLastPathParameter function is:
- Well-implemented using lodash utilities
- Focused and reusable
- Efficiently handles path parameter extraction
Also applies to: 1482-1482
src/react/src/__tests__/utils.tests.js (1)
1877-1915: LGTM! Thorough test coverage for getLastPathParameter.The test suite is excellent:
- Covers all edge cases (empty string, slashes only)
- Tests various URL formats
- Clear and descriptive test cases
React App | Jest test suite - Code coverage reportTotal: 29.01%Your code coverage diff: 0.06% ▴ ✅ All code changes are covered |
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 56.14%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.91%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Django App | Unittest test suite - Code coverage reportTotal: 80.57%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/react/src/components/Contribute/ProductionLocationDetails.jsx (1)
38-52: Enhance the filtering logic and component performance.The current implementation has a few areas for improvement:
- The filter condition requires an exact match, which might be too strict.
- The filtered array could be memoized to prevent unnecessary re-renders.
- The code readability could be improved by extracting the filtered IDs to a separate variable.
Consider applying this refactor:
+ const filteredHistoricalOsIds = React.useMemo( + () => + historicalOsIdsNotEmpty + ? historicalOsIds.filter( + historicalOsId => + historicalOsId === osIdSearchParameter, + ) + : [], + [historicalOsIds, osIdSearchParameter], + ); + return ( <div> {/* ... */} - {historicalOsIdsNotEmpty && - historicalOsIds - .filter( - historicalOsId => - historicalOsId === osIdSearchParameter, - ) + {filteredHistoricalOsIds.length > 0 && + filteredHistoricalOsIds .map(historicalOsId => ( <Typography key={historicalOsId}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
doc/release/RELEASE-NOTES.md(1 hunks)src/react/src/components/Contribute/ProductionLocationDetails.jsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/release/RELEASE-NOTES.md
🧰 Additional context used
🧠 Learnings (1)
src/react/src/components/Contribute/ProductionLocationDetails.jsx (1)
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#470
File: src/react/src/components/Contribute/ProductionLocationInfo.jsx:229-243
Timestamp: 2025-01-23T10:19:59.642Z
Learning: In React useEffect hooks that perform navigation, adding router-related dependencies (history, location) to the dependency array can cause unwanted re-renders and side effects. For navigation effects triggered by data changes, include only the data dependencies.
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-django-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (1)
src/react/src/components/Contribute/ProductionLocationDetails.jsx (1)
2-2: LGTM!The new imports are correctly placed and necessary for implementing the OS ID filtering functionality.
Also applies to: 8-8
src/react/src/components/Contribute/ProductionLocationDetails.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/react/src/__tests__/utils.tests.js(2 hunks)src/react/src/util/util.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/src/util/util.js
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
🔇 Additional comments (1)
src/react/src/__tests__/utils.tests.js (1)
1878-1916: LGTM on edge case handling.The test suite thoroughly covers edge cases such as:
- Empty strings
- URLs with only slashes
- URLs without slashes
- URLs ending with "id/"
The handling of these edge cases looks correct and robust.
mazursasha1990
suggested changes
Feb 12, 2025
Contributor
mazursasha1990
left a comment
mazursasha1990
left a comment
There was a problem hiding this comment.
Left a suggestion on how the ProductionLocationDetails component could be improved.
src/react/src/components/Contribute/ProductionLocationDetails.jsx
Outdated
Show resolved
Hide resolved
mazursasha1990
suggested changes
Feb 12, 2025
Contributor
mazursasha1990
left a comment
mazursasha1990
left a comment
There was a problem hiding this comment.
Can you also correct the position of the info icon and make it in the center of the line?
It could be done by adding next styles to the locationHistoricalOsIdStyles classname:
display: 'flex',
alignItems: 'center',
gap: '5px',
Contributor
Author
|
@mazursasha1990 I've addressed your comments. |
|
This was referenced Feb 17, 2025
This was referenced Mar 5, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Fix OSDEV-1700
Hide redundand previous os ids from the search result.