[OSDEV-1129] Implement results page for name address search UI part.#465
Conversation
… search by os id results
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
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 |
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
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 |
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Django App | Unittest test suite - Code coverage reportTotal: 80.18%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
…ddress-search-front-end-part
📝 WalkthroughWalkthroughThis pull request introduces significant updates to the Open Supply Hub project, particularly for version 1.28.0. It includes the addition of new components for handling search results by name and address, updates to routing and action creators, and enhancements to the reducer for managing production locations. The release notes have been updated to document these changes, including a new feature related to the user interface for search results and modifications to existing components. Changes
Sequence DiagramsequenceDiagram
participant User
participant SearchPage
participant SearchByNameAndAddressResult
participant ProductionLocationDetails
participant ConfirmNotFoundLocationDialog
User->>SearchPage: Enter name and address
SearchPage->>SearchByNameAndAddressResult: Fetch locations
SearchByNameAndAddressResult-->>SearchByNameAndAddressResult: Process results
alt Locations found
SearchByNameAndAddressResult->>ProductionLocationDetails: Display location details
User->>ProductionLocationDetails: Select or indicate not found
alt Location not found
User->>ConfirmNotFoundLocationDialog: Open dialog
ConfirmNotFoundLocationDialog->>User: Confirm action
User->>SearchPage: Search again or add new location
end
else No locations found
SearchByNameAndAddressResult->>User: Display "No results" message
end
Possibly related PRs
Suggested reviewers
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: 2
🧹 Nitpick comments (26)
src/react/src/components/Contribute/SearchByOsIdSuccessResult.jsx (1)
11-21: Consolidated prop destructuring
By destructuringproductionLocation, you keep the component concise and easier to maintain. This helps unify related data into a single object, reducing the need for multiple individual props. Ensure your back-end or parent component always supplies the necessary data shape to avoid runtime errors.src/react/src/components/Contribute/PreviousOsIdTooltip.jsx (1)
41-44: Enhanced testability
Thedata-testid="previous-os-id-tooltip"attribute simplifies locating this element in tests, improving reliability of UI tests.src/react/src/__tests__/components/SearchByNameAndAddressNotFoundResult.test.js (1)
1-2: Correct imports for testing
Including React and relevant testing-library methods sets up a clear testing environment. Consider grouping test-related imports together to keep them organized.src/react/src/components/Contribute/ProductionLocationDetails.jsx (2)
8-49: Use a data-testid or aria-label for improved accessibility.
It's helpful to attach adata-testidor anaria-labelto certain elements, such as thePrevious OS ID:labels or addresses, to enhance accessibility and facilitate robust automated testing. This would make it easier to identify these elements in both user-assistive tools and tests.
36-36: Include a test identifier for the tooltip.
Since you're rendering<PreviousOsIdTooltip />for each historical OS ID, consider specifying adata-testidor similar identifier within the tooltip component itself if you aren’t already. This will make it easier to verify its content in tests.src/react/src/__tests__/components/ProductionLocationDetails.test.js (1)
46-51: Tooltip count test is a nice touch.
Checking the number of tooltips matches the number of historical OS IDs is a practical validation step. Consider verifying each tooltip’s content or label for even more confidence.src/react/src/components/Contribute/SearchByNameAndAddressNotFoundResult.jsx (1)
15-15: Implement or track the "Add New Location" handler.
Currently,handleAddNewLocationis empty. If this is intentional, consider adding a TODO comment or a relevant placeholder for clarity. This will help future contributors understand the planned functionality.Are you planning to open a new feature ticket for this?
src/react/src/components/Contribute/SearchByNameAndAddressResult.jsx (2)
30-33: Consider preserving user input for back navigation
When navigating back to the search screen viahandleBackToSearchByNameAddress, the current functionality clears all location data. Depending on your UX, users might want their previous search criteria preserved. Consider selectively preserving state or confirming with product requirements.
51-60: Structure the ternary for readability
The ternary operator for displaying the success vs. not found results is fine. However, if the logic expands, consider splitting it into separate functions or early returns for clarity.src/react/src/__tests__/components/SearchByOsIdSuccessResult.test.js (1)
31-43: Check field naming consistency
TheproductionLocationobject uses keys likeos_idandhistorical_os_id, while in your mocks and components you useosIdandhistoricalOsIds. Ensure consistent naming across the codebase to avoid confusion. Consider normalizing property names at the Redux or API boundary.src/react/src/components/Contribute/SearchByOsIdTab.jsx (1)
34-34: Adopt route constants for clarity
Using a hardcoded path inhistory.pushis acceptable if you don’t anticipate changes. However, to enhance maintainability and avoid magic strings, consider referencing a route constant for the final URL structure, e.g.searchByOsIdResultRoute.src/react/src/components/Contribute/SearchByOsIdResult.jsx (2)
31-31: Validate parameter name consistency.
Ensure theosIDparameter name inuseParams()exactly matches the URL segment param name in yourRoutedefinition.
72-72: Pass data with descriptive prop name.
Currently passingproductionLocation={data}is okay, but consider renamingdatatoproductionLocationDatafor clarity.- productionLocation={data} + productionLocation={productionLocationData}src/react/src/components/Contribute/SearchByNameAndAddressSuccessResult.jsx (3)
11-16: Consider renamingclearLocationsfor clarity.
The prop nameclearLocationsmight be vague to future maintainers. A more explicit name likeresetSearchResultsorclearSearchResultscould help.
34-34: Remove or implement the empty stub.
handleSelectLocationis currently empty. Consider either implementing it or removing it to avoid confusion.Would you like help drafting the functionality or opening a tracking issue for this?
44-123: Good structure, minor usability suggestions.
- Consider letting users sort their results by different criteria if needed (e.g., name, address, etc.), rather than always “Best match.”
- Ensure no performance bottlenecks if the number of production locations grows significantly.
src/react/src/util/styles.js (2)
986-1081: Success result styles are clean and consistent.
The structure, naming, and usage of theming variables reflect best practice, though you could consider refactoring repeated font settings if they appear often in other style blocks.
1170-1199: Production location details styles
Everything is consistent with the existing style approach, making the UI cohesive.src/react/src/actions/contributeProductionLocation.js (2)
55-57: TODO note for real API
The comment acknowledges that a real endpoint is forthcoming. Just ensure the mock is replaced before merging to production.Do you want help drafting an issue or PR for hooking up the real API?
58-134: Mock data approach
The large mocked data is fine for development, but watch out for performance in test or staging if it grows.src/react/src/util/constants.jsx (1)
355-356: Consider omitting the trailing slash.
Depending on your router setup, ending the path with a slash ('/contribute/production-location/search/') can sometimes cause unexpected route matching behavior or duplication. You might want to confirm whether the trailing slash is intentional.- export const searchByNameAndAddressResultRoute = - '/contribute/production-location/search/'; + export const searchByNameAndAddressResultRoute = + '/contribute/production-location/search';doc/release/RELEASE-NOTES.md (5)
12-15: Check punctuation in headings.
Markdownlint flags trailing punctuation in headings (line 13: “#### Migrations:”). Consider removing the colon.- #### Migrations: + #### Migrations🧰 Tools
🪛 Markdownlint (0.37.0)
13-13: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
19-21: Good practice for code changes section.
It’s recommended to add a short bullet or summary, especially if future maintainers might read these notes.
25-27: Bugfix section is consistent.
Same feedback as the code/API changes section: a quick bullet helps clarity.
28-33: Markdown list indentation.
Lines 30-32 have a four-space indentation instead of two, triggering MD007 warnings. You can fix it by reducing indentation to two spaces:- * Successful Search: ... - * Confirmation Dialog Window: ... - * Unsuccessful Search: ... + * Successful Search: ... + * Confirmation Dialog Window: ... + * Unsuccessful Search: ...🧰 Tools
🪛 Markdownlint (0.37.0)
30-30: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
31-31: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
32-32: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
34-36: Check punctuation in heading.
Markdownlint also flags trailing punctuation on line 34 (“### Release instructions:”).- ### Release instructions: + ### Release instructions🧰 Tools
🪛 Markdownlint (0.37.0)
34-34: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
doc/release/RELEASE-NOTES.md(1 hunks)src/react/src/Routes.jsx(4 hunks)src/react/src/__tests__/components/ConfirmNotFoundLocationDialog.test.js(1 hunks)src/react/src/__tests__/components/ProductionLocationDetails.test.js(1 hunks)src/react/src/__tests__/components/SearchByNameAndAddressNotFoundResult.test.js(1 hunks)src/react/src/__tests__/components/SearchByNameAndAddressSuccessResult.test.js(1 hunks)src/react/src/__tests__/components/SearchByOsIdSuccessResult.test.js(1 hunks)src/react/src/__tests__/components/SearchByOsIdTab.test.js(1 hunks)src/react/src/actions/contributeProductionLocation.js(1 hunks)src/react/src/components/Contribute/ConfirmNotFoundLocationDialog.jsx(1 hunks)src/react/src/components/Contribute/PreviousOsIdTooltip.jsx(2 hunks)src/react/src/components/Contribute/ProductionLocationDetails.jsx(1 hunks)src/react/src/components/Contribute/SearchByNameAndAddressNotFoundResult.jsx(1 hunks)src/react/src/components/Contribute/SearchByNameAndAddressResult.jsx(1 hunks)src/react/src/components/Contribute/SearchByNameAndAddressSuccessResult.jsx(1 hunks)src/react/src/components/Contribute/SearchByOsIdNotFoundResult.jsx(3 hunks)src/react/src/components/Contribute/SearchByOsIdResult.jsx(4 hunks)src/react/src/components/Contribute/SearchByOsIdSuccessResult.jsx(2 hunks)src/react/src/components/Contribute/SearchByOsIdTab.jsx(1 hunks)src/react/src/reducers/ContributeProductionLocationReducer.js(2 hunks)src/react/src/util/constants.jsx(1 hunks)src/react/src/util/styles.js(3 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
doc/release/RELEASE-NOTES.md
30-30: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
31-31: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
32-32: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
40-40: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
13-13: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
34-34: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
🔇 Additional comments (66)
src/react/src/components/Contribute/SearchByOsIdNotFoundResult.jsx (3)
6-6: Rename style function for clarity
Renaming the style function tomakeSearchByOsIdNotFoundResultStyleshelps differentiate styling for the "not found" scenario. This is consistent with the component's purpose and improves maintainability.
17-17: Renamed class for improved semantics
ChangingresultSubTitleStylestoresultDescriptionStylesclarifies the function of this text, making the code more self-explanatory.
36-36: Export default alignment
Using the renamed style function inwithStylesensures styles align with the component's "not found" use-case. This export approach is suitable for consistent theming.src/react/src/components/Contribute/SearchByOsIdSuccessResult.jsx (3)
2-2: New imports for improved structure
Importingobject,func, and especiallyproductionLocationPropTypeneatly modularizes type checks. Also, having a dedicated componentProductionLocationDetailsfosters better separation of concerns.Also applies to: 7-8
28-34: Delegation to ProductionLocationDetails
Passing props toProductionLocationDetailscentralizes the UI for location details. This fosters reusability. Confirm that any fallback or error states are handled gracefully if one of these props (likecountryName) is missing orundefined.
46-46: Updated prop types
RequiringproductionLocationas a single prop clarifies the expected data structure and enforces a stronger contract for this component. This aligns well with the new approach of passing a consolidated object.src/react/src/components/Contribute/PreviousOsIdTooltip.jsx (2)
2-2: PropTypes import ensures stricter type checking
Importingobjectfromprop-typeshelps validate the structure ofclasses, preventing errors due to unexpected data shapes.
49-51: Explicit prop type definition
Declaringclasses: object.isRequiredclarifies that this component depends on injected styles. This ensures any parent code or tests must include the right classes.src/react/src/__tests__/components/SearchByNameAndAddressNotFoundResult.test.js (3)
8-10: Proper mock handling
Mockinghistory.pushensures the test can verify navigation calls without side effects. This is a clean solution that makes unit tests more predictable.
12-28: Not found message test coverage
Verifying that the UI displays messages and buttons for "no results" states ensures clarity and consistency. Good detail checking text content and button presence.
30-38: Navigation flow test
Firingclickon the "Search again" button and confirminghistory.pushis called with the correct route thoroughly covers your routing logic.src/react/src/components/Contribute/ProductionLocationDetails.jsx (3)
15-17: Confirm usage ofArray.isArray(historicalOsIds).
You're already validatinghistoricalOsIdsas an array inpropTypes. Therefore, the additionalArray.isArraycheck here is a safe fallback, but it might be redundant. You can keep it as a guard or remove it for brevity.
51-53: Default props usage recognized.
ProvidinghistoricalOsIds: []by default helps avoid runtime errors when the prop is absent. Good practice!
55-62: Prop Types check is solid.
The prop definitions look comprehensive and accurate, ensuring the component receives the necessary data.src/react/src/__tests__/components/ProductionLocationDetails.test.js (3)
5-29: Excellent coverage for rendering states.
The tests cover a wide range of usage scenarios, including rendering with non-empty historical IDs, verifying displayed text, and ensuring all details are shown. This provides strong confidence in the component’s correctness.
31-37: Proper handling of an emptyhistoricalOsIdsarray.
Great job verifying that the component does not render irrelevant elements when no previous OS IDs exist. This ensures the UI remains clean and avoids confusion for end users.
39-44: Fallback test for undefinedhistoricalOsIds.
Testing the case wherehistoricalOsIdsis undefined ensures defensive coding practices, preventing unexpected runtime errors.src/react/src/components/Contribute/SearchByNameAndAddressNotFoundResult.jsx (1)
10-53: Straightforward user guidance.
This component clearly guides the user when no search results are found and provides two intuitive next steps. The layout is well-structured.src/react/src/__tests__/components/SearchByOsIdTab.test.js (1)
67-67: URL path-based searching confirmed.
Switching from a query parameter to a path segment clarifies the route for OS ID searches. Make sure all references in the codebase are updated to this new route pattern, and confirm no downstream reliance on the old query param approach.✅ Verification successful
Based on the search results and code analysis, I can now provide a final response:
Path-based OS ID search implementation is consistent across the codebase
The codebase shows a consistent implementation of path-based OS ID searching:
- The route is defined in constants as
/contribute/production-location/search/id/:osID- The React components and tests consistently use this path pattern
- No references to the old query parameter approach (
?osId=) were found- The change appears to be properly implemented across all relevant components:
- SearchByOsIdTab
- SearchByOsIdResult
- SearchByOsIdNotFoundResult
- SearchByOsIdSuccessResult
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify there are no leftover references to the old query parameter approach rg "search\?.*osId=" -A 3Length of output: 27
Script:
#!/bin/bash # Search for any URL patterns containing 'search' and 'id' to find both old and new patterns rg -i "search.*id.*" -A 3 # Also check for route definitions that might handle these patterns ast-grep --pattern 'path: $PATH'Length of output: 68138
Script:
#!/bin/bash # Look for test files that might contain URL-related tests fd "test" -e js -x rg -l "search.*id" # Check for any API client or service files fd "api|service|client" -e js -x rg -l "search.*id"Length of output: 216
Script:
#!/bin/bash # Search for any route definitions or URL patterns in the codebase rg -i "path.*search.*id|route.*search.*id" -A 2 # Look for API calls or fetch operations that might still use query parameters rg -i "fetch.*search.*\?.*osId|get.*search.*\?.*osId" -A 2Length of output: 54433
src/react/src/components/Contribute/SearchByNameAndAddressResult.jsx (2)
26-28: Validate dependency array in useEffect
SincefetchLocationsis a function provided by Redux, it shouldn't change identity frequently. Specifying[fetchLocations]as a dependency is fine, but if this function depends on other variables or might change across renders, consider adding them to the dependency array to avoid potential mis-fires or missed calls.
64-71: Prop types alignment
ThepropTypesspecification looks consistent with the mapped Redux store. Ensure thatproductionLocationPropTypeindatafully matches the shape used in the success result component. This helps maintain type consistency throughout the codebase.src/react/src/__tests__/components/SearchByOsIdSuccessResult.test.js (1)
6-14: Mock implementations are well-organized
Defining clear mock components for child elements is a good practice, ensuring your tests isolate only the relevant logic.src/react/src/components/Contribute/ConfirmNotFoundLocationDialog.jsx (1)
21-24: Revisit add-new-location workflow
CallingclearLocations()immediately on adding a new location might remove key info that could be used to prefill the new location form. Ensure that clearing is truly intended or confirm with product requirements if partial state needs to be retained.src/react/src/components/Contribute/SearchByOsIdResult.jsx (3)
2-2: Confirm usage of react-router hooks.
Importing bothuseHistoryanduseParamsis correct for this scenario, but ensure the route definition forosIDis aligned with how it's parsed in the component.Could you confirm that the route using
osIDis correctly defined in your routing configuration (e.g.,path="/some-route/:osID")? If not, users might encounter unexpected behavior.
39-39: Ensure correct data check.
!isEmpty(data)is a valid approach. Do note that for large data objects, a direct property check may be more efficient (e.g.,data && !isEmpty(data)).
91-92: PropTypes usage looks good.
EnforcingisRequiredondataandfetchinghelps validate component usage early, preventing runtime errors.src/react/src/__tests__/components/ConfirmNotFoundLocationDialog.test.js (7)
1-5: Overall test structure is clear.
The test suite is well-organized, ensuring the critical user flows (reviewing, searching again, adding a new location, etc.) are covered.
6-9: Mocking history for push is appropriate.
Mocking outhistory.pushensures you avoid unintended navigation or side effects during tests. Great choice!
25-46: Good coverage of dialog UI elements.
You verify the presence of title and buttons thoroughly, ensuring clarity in user expectations.
48-62: Verifying "search again" logic.
The test ensures the dialog close, location clearing, and redirect are each triggered. Checks are well-structured.
64-75: Validating addition of a new location flow.
Thoroughly ensures the same logic applies to the "Yes, add a new location" flow.
77-85: Checking close dialog behavior.
Ensuring the close button triggers the callback is an essential user flow.
87-100: Confirming invisible dialog state.
Testing that the dialog does not render whenconfirmDialogIsOpenisfalseproperly covers the dialog’s conditional rendering.src/react/src/reducers/ContributeProductionLocationReducer.js (4)
4-5: Action imports renamed for clarity.
Transitioning fromfetching-based naming tofetchis consistent and precise. This improves readability across the codebase.Also applies to: 8-11
16-19: State initialization is well-structured.
UsingObject.freezeto prevent accidental mutations is good practice for Redux states.Also applies to: 20-24
30-38: Single production location flow.
The logic clearly resets error and data on start, sets them on fail, and populates them on success. This ensures predictable state transitions.Also applies to: 41-46, 50-58
69-78: Multiple production locations flow.
The newly introduced actions handle the loading, error, and data states consistently. Perfect for scaling use cases that require multiple location data sets.Also applies to: 79-87, 88-98, 99-104
src/react/src/__tests__/components/SearchByNameAndAddressSuccessResult.test.js (6)
1-5: Good test file structure.
All core functionalities (rendering data, interactions, etc.) are covered in separate tests for clarity.
6-9: Mocking child components effectively.
This isolation keeps tests focused on the parent component’s logic while ensuring subcomponents don't introduce side effects or complexity.
23-51: Props offer good coverage.
TheproductionLocationsarray andproductionLocationsCountproperty ensure thorough test coverage. Testing multiple results is especially valuable.
53-83: Ensuring correct render of search results.
The test checks text presence and quantity of results. This is a robust approach to verifying correct UI rendering.
85-94: Dialog handling on “I don't see my Location.”
Perfectly tests the chain of events to confirm that the confirmation dialog appears as expected.
96-108: Dialog close behavior verification.
Testing the visibility after closing ensures a consistent user experience.src/react/src/components/Contribute/SearchByNameAndAddressSuccessResult.jsx (3)
1-9: Imports and prop-types usage look solid.
All imports, including custom prop types, are well-structured. No issues found.
20-32: Guard against missing DOM element in scroll event listener.
Ifdocument.getElementById('mainPanel')does not exist, this code could throw an error. Ensure that#mainPanelis always rendered or handle it gracefully with a null check before accessingoffsetHeight.
126-135: PropTypes correctly enforced.
Requiring these props is a good practice and keeps the component robust.src/react/src/Routes.jsx (4)
34-34: Import statement is valid.
The import forSearchByNameAndAddressResultcorrectly references the new component.
61-61: Route constant usage is consistent with existing conventions.
No issues spotted.
89-93: Assigningid="mainPanel"is a good approach for DOM references.
Ensures that other components can reliably target this element for scrolling or measurements.
178-182: New route entry for search-by-name-and-address.
This route is well-integrated into the existingSwitch. Consider testing navigation to ensure no conflicts with other routes.src/react/src/util/styles.js (5)
437-439: Minor margin addition looks correct.
This margin aligns with the existing style conventions in the file.
441-455: New “Not Found” result styles for OS ID
The style definitions are consistent and follow the existing naming/structuring patterns.
973-985: Shared result styles
Provides a neat separation for the loader container and back-to-search button. Appropriately named and easy to locate.
1082-1127: Not found result styles
Good approach for centering content in a “No Results” scenario. No issues found.
1127-1169: “Confirm Not Found Location” dialog styles
Ensures consistent theming for modals. Looks good.src/react/src/actions/contributeProductionLocation.js (5)
9-32: New action creators for fetching single vs. multiple locations
Renaming tostartFetchSingleProductionLocation(etc.) clarifies usage. The newly addedstartFetchProductionLocationsand related actions align well with Redux best practices.
36-52:fetchProductionLocationByOsId: confirm fallback handling
The API request and error handling logic are straightforward. Ensure errors are caught downstream as well, in case the component expects certain data.
136-163: Initial fetch multiple locations
This async function dispatches the correct start action and calls a mock. Looks good.
144-146:setTimeoutusage
Delays are fine for simulating latency. Confirm that automated tests handle timing gracefully.
149-152: Error flow
The error dispatch pattern is consistent and ensures logging is captured. No issues here.src/react/src/util/constants.jsx (1)
353-354: Route parameter usage looks good.
This change from a static route to a dynamic parameter route (:osID) allows more flexible navigation. Ensure that any consuming components or pages handle theosIDparameter correctly (e.g., by matching the route in the router configuration).doc/release/RELEASE-NOTES.md (5)
6-7: Release heading is properly introduced.
The new version header “## Release 1.28.0” is concise and follows the existing format.
8-11: Good introduction section.
It’s helpful to keep the product name and release date at the top. No issues found.
16-18: Scheme changes placeholder is fine.
No content issues. This placeholder is consistent with the existing style.
22-24: Architecture/environment placeholder is fine.
No concerns.
42-42: Release date update.
Make sure to keep older release notes consistent. Having two different release dates for 1.27.0 might be confusing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/react/src/components/Contribute/ConfirmNotFoundLocationDialog.jsx (3)
30-34: Consider optimizing state updates and adding error handlingThe
handleSearchAgainfunction triggers multiple state changes which could cause unnecessary re-renders. Additionally, there's no error handling for the navigation.Consider batching the state updates and adding error handling:
const handleSearchAgain = () => { + try { + // Batch state updates + Promise.all([ handleConfirmDialogClose(), - navigateToNameAddressTab(), - clearLocations() + clearLocations(), + navigateToNameAddressTab() + ]).catch(console.error); + } catch (error) { + console.error('Failed to handle search again:', error); + // Consider showing an error message to the user + } };
36-81: Enhance accessibility for screen readersWhile basic accessibility attributes are present, the dialog content could be more accessible.
Consider these improvements:
<Dialog classes={{ paper: classes.dialogPaperStyles, }} open={confirmDialogIsOpen} onClose={handleConfirmDialogClose} aria-labelledby="confirm-not-found-location-dialog-title" + aria-describedby="confirm-not-found-location-dialog-description" > {/* ... */} <DialogTitle id="confirm-not-found-location-dialog-title"> <Typography className={classes.dialogTitleStyles}> Are you sure you have reviewed the entire list and could not find the production location? </Typography> </DialogTitle> + <DialogContent> + <Typography id="confirm-not-found-location-dialog-description"> + Choose whether you want to search again or add a new production location. + </Typography> + </DialogContent>
85-90: Document the expected shape of classes propWhile PropTypes are defined, the
classesprop type could be more specific.Consider adding documentation about the expected shape:
ConfirmNotFoundLocationDialog.propTypes = { confirmDialogIsOpen: bool.isRequired, handleConfirmDialogClose: func.isRequired, clearLocations: func.isRequired, - classes: object.isRequired, + classes: shape({ + dialogPaperStyles: string.isRequired, + closeButtonStyles: string.isRequired, + dialogTitleStyles: string.isRequired, + dialogActionsStyles: string.isRequired, + buttonBaseStyles: string.isRequired, + buttonLabelStyles: string.isRequired, + addLocationButtonStyles: string.isRequired, + }).isRequired, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/react/src/components/Contribute/ConfirmNotFoundLocationDialog.jsx(1 hunks)src/react/src/components/Contribute/SearchByOsIdResult.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/src/components/Contribute/SearchByOsIdResult.jsx
🔇 Additional comments (2)
src/react/src/components/Contribute/ConfirmNotFoundLocationDialog.jsx (2)
11-11: Decouple routing logic from the componentThe direct usage of the
historyutility makes the component tightly coupled to the routing implementation. This makes the component harder to test and less reusable.Consider injecting the navigation logic as a prop:
const ConfirmNotFoundLocationDialog = ({ confirmDialogIsOpen, handleConfirmDialogClose, clearLocations, + onNavigateToSearch, classes, }) => { - const navigateToNameAddressTab = () => { - history.push(`${contributeProductionLocationRoute}?tab=name-address`); - };Also applies to: 26-28
15-20: Well-structured component definition!The component is well-named and the props are descriptively named, making the code self-documenting.
roman-stolar
left a comment
roman-stolar
left a comment
There was a problem hiding this comment.
LGTM, the structure and design looks well.
|
@mazursasha1990 Please fix merge conflicts |
…ddress-search-front-end-part
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/react/src/util/styles.js (4)
441-454: Consider consistent line spacing.While the implementation follows best practices, the line spacing between style objects could be more consistent.
Apply this diff to improve readability:
export const makeSearchByOsIdNotFoundResultStyles = theme => Object.freeze({ resultTitleStyles: Object.freeze({ fontSize: '36px', fontWeight: theme.typography.fontWeightSemiBoldPlus, lineHeight: '44px', }), + resultDescriptionStyles: Object.freeze({ fontSize: '18px', lineHeight: '21px', fontWeight: theme.typography.fontWeightSemiBold, marginTop: '8px', }), });
972-1066: Consider extracting common button styles.The implementation duplicates button styles across multiple objects. Consider extracting common button styles to reduce duplication.
Apply this refactoring to improve maintainability:
export const makeSearchByNameAndAddressSuccessResultStyles = theme => Object.freeze({ + commonButtonStyles: Object.freeze({ + textTransform: 'none', + backgroundColor: theme.palette.action.main, + color: theme.palette.common.black, + '&:hover': { + backgroundColor: theme.palette.action.dark, + }, + }), // ... other styles ... buttonBaseStyles: Object.freeze({ height: '48px', - textTransform: 'none', - backgroundColor: theme.palette.action.main, - color: theme.palette.common.black, - '&:hover': { - backgroundColor: theme.palette.action.dark, - }, + ...commonButtonStyles, }), // ... use commonButtonStyles in other button style objects });
1113-1154: Use theme colors instead of hardcoded values.Consider using theme colors for consistency and maintainability.
Apply this diff to use theme colors:
export const makeConfirmNotFoundLocationDialogStyles = theme => Object.freeze({ closeButtonStyles: { position: 'absolute', right: '16px', top: '16px', - color: '#000', + color: theme.palette.common.black, }, // ... other styles });
1156-1185: Use consistent function syntax.The function uses arrow syntax with parentheses instead of the
Object.freezepattern used in other style functions.Apply this diff for consistency:
-export const makeProductionLocationDetailsStyles = theme => ({ +export const makeProductionLocationDetailsStyles = theme => + Object.freeze({
📜 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/Routes.jsx(4 hunks)src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx(2 hunks)src/react/src/util/constants.jsx(1 hunks)src/react/src/util/styles.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/src/util/constants.jsx
🧰 Additional context used
🪛 Markdownlint (0.37.0)
doc/release/RELEASE-NOTES.md
30-30: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
31-31: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
32-32: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
13-13: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
34-34: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (11)
src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx (2)
13-13: LGTM! Import change aligns with new routing structure.The change from
productionLocationInfoRoutetosearchByNameAndAddressResultRoutecorrectly aligns with the new UI implementation for the search results page.
Line range hint
75-82: LGTM! URL construction looks correct.The URL construction logic properly handles the search parameters and uses the new route constant. The implementation correctly:
- Uses the new base route
- Constructs search params with name, address and country
- Handles potential null/undefined country value with nullish coalescing
doc/release/RELEASE-NOTES.md (1)
6-36: LGTM! Release notes are well-structured and comprehensive.The release notes effectively document:
- Clear version number and release date
- Placeholders for DB/architecture changes
- Detailed description of the new UI implementation for search results
🧰 Tools
🪛 Markdownlint (0.37.0)
30-30: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
31-31: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
32-32: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
13-13: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
34-34: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
src/react/src/Routes.jsx (4)
34-34: LGTM!The import statement follows the established pattern and is correctly placed with other component imports.
62-62: LGTM!The route constant import follows the established pattern and is correctly placed with other route constant imports.
180-184: LGTM!The route definition follows the established pattern and is correctly placed with other routes.
91-95: Verify the need for duplicate panel identifiers.The
mainelement has bothclassName="mainPanel"andid="mainPanel". This seems redundant unless there's a specific requirement for both.Run the following script to check if both identifiers are necessary:
✅ Verification successful
Both className and id are necessary for mainPanel
The duplicate identifiers serve different purposes:
id="mainPanel"is used for programmatic access inSearchByNameAndAddressSuccessResult.jsxto calculate scroll positionclassName="mainPanel"is used for styling as referenced inApp.css🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of mainPanel id and class # Check for mainPanel id usage echo "Checking mainPanel id usage:" rg -l "#mainPanel" --type js --type jsx --type css # Check for mainPanel class usage echo "Checking mainPanel class usage:" rg -l "\.mainPanel" --type js --type jsx --type cssLength of output: 302
Script:
#!/bin/bash # Search for mainPanel usages with correct file types and patterns echo "Checking static mainPanel id/class usage:" rg -l --type-add 'react:*.{jsx,tsx}' --type react --type css --type js '(#mainPanel|\.mainPanel|className="mainPanel"|id="mainPanel")' echo -e "\nChecking dynamic mainPanel id usage:" rg -A 2 --type-add 'react:*.{jsx,tsx}' --type react --type js '(getElementById|querySelector).*(["'\''])mainPanel' echo -e "\nChecking className assignments with context:" rg -A 2 --type-add 'react:*.{jsx,tsx}' --type react --type js 'className.*mainPanel'Length of output: 1221
src/react/src/util/styles.js (4)
437-437: LGTM!The addition of marginBottom follows the existing styling pattern.
692-692: LGTM!The modification to marginTop follows the existing styling pattern.
959-970: LGTM!The implementation is well-structured and follows best practices for style definitions.
1068-1111: Button styles duplication noted.Similar button styles duplication as noted in the previous comment.
…ddress-search-front-end-part
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
doc/release/RELEASE-NOTES.md (1)
25-28: Improve markdown formatting for better readability.The bullet points indentation should be 2 spaces instead of 4 spaces to follow markdown standards.
Apply this diff to fix the indentation:
* [OSDEV-1129](https://opensupplyhub.atlassian.net/browse/OSDEV-1129) - The UI for the results page for name and address search was implemented. It includes the following screens: - * Successful Search: If the search is successful, the results screen displays a list of production locations. Each item includes the following information about the production location: name, OS ID, address, and country name. Users can either select a specific production location or press the "I don't see my Location" button, which triggers a confirmation dialog window. - * Confirmation Dialog Window: In this window, users can confirm that no correct location was found using the provided search parameters. They can either proceed to create a new production location or return to the search. - * Unsuccessful Search: If the search is unsuccessful, an explanation is provided along with two options: return to the search or add a new production location. + * Successful Search: If the search is successful, the results screen displays a list of production locations. Each item includes the following information about the production location: name, OS ID, address, and country name. Users can either select a specific production location or press the "I don't see my Location" button, which triggers a confirmation dialog window. + * Confirmation Dialog Window: In this window, users can confirm that no correct location was found using the provided search parameters. They can either proceed to create a new production location or return to the search. + * Unsuccessful Search: If the search is unsuccessful, an explanation is provided along with two options: return to the search or add a new production location.🧰 Tools
🪛 Markdownlint (0.37.0)
26-26: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
27-27: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
28-28: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
src/react/src/util/styles.js (2)
959-1066: Consider extracting common button styles to a shared style object.The button styles (width, height, textTransform, etc.) are duplicated across multiple style functions. Consider creating a shared base button style object to improve maintainability.
Example refactor:
+const baseButtonStyles = theme => Object.freeze({ + textTransform: 'none', + height: '48px', + backgroundColor: theme.palette.action.main, + color: theme.palette.common.black, + '&:hover': { + backgroundColor: theme.palette.action.dark, + }, +}); export const makeSearchByNameAndAddressSuccessResultStyles = theme => Object.freeze({ // ... other styles ... buttonBaseStyles: Object.freeze({ - height: '48px', - textTransform: 'none', - backgroundColor: theme.palette.action.main, - color: theme.palette.common.black, - '&:hover': { - backgroundColor: theme.palette.action.dark, - }, + ...baseButtonStyles(theme), }), });
1113-1185: Use theme colors instead of hardcoded values.Replace hardcoded color values with theme colors for better maintainability and consistency:
export const makeConfirmNotFoundLocationDialogStyles = theme => Object.freeze({ closeButtonStyles: { position: 'absolute', right: '16px', top: '16px', - color: '#000', + color: theme.palette.common.black, }, // ... other styles });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
doc/release/RELEASE-NOTES.md(1 hunks)src/react/src/Routes.jsx(4 hunks)src/react/src/util/constants.jsx(1 hunks)src/react/src/util/styles.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/react/src/Routes.jsx
- src/react/src/util/constants.jsx
🧰 Additional context used
🪛 Markdownlint (0.37.0)
doc/release/RELEASE-NOTES.md
26-26: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
27-27: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
28-28: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
⏰ 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-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
🔇 Additional comments (3)
doc/release/RELEASE-NOTES.md (1)
Line range hint
1-24: LGTM!The release notes are well-structured and provide clear documentation of the changes in version 1.28.0, including proper references to Jira tickets and detailed descriptions of the implemented features.
Also applies to: 29-1000
🧰 Tools
🪛 Markdownlint (0.37.0)
26-26: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
27-27: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
28-28: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
30-30: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
src/react/src/util/styles.js (2)
437-439: LGTM! Consistent spacing value.The added margin maintains consistency with the spacing system used throughout the styles.
1068-1111: LGTM! Well-structured styles with good responsive design considerations.The styles are well-organized and make proper use of theme variables for consistency. The responsive design approach is appropriate for the use case.
|
3 participants
OSDEV-1129 - SLC. Implement results page for name & address search (FE).
The UI for the results page for name and address search was implemented. It includes the following screens:
Successful Search: If the search is successful, the results screen displays a list of production locations. Each item includes the following information about the production location: name, OS ID, address, and country name. Users can either select a specific production location or press the "I don’t see my Location" button, which triggers a confirmation dialog window.
Confirmation Dialog Window: In this window, users can confirm that no correct location was found using the provided search parameters. They can either proceed to create a new production location or return to the search.
Unsuccessful Search: If the search is unsuccessful, an explanation is provided along with two options: return to the search or add a new production location.