[OSDEV-1374] SLC. Connect search & result page for name and address search with Backend API (integration)#483
Conversation
# Conflicts: # src/react/src/util/util.js
React App | Jest test suite - Code coverage reportTotal: 28.37%Your code coverage diff: 0.06% ▴ ✅ All code changes are covered |
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 56.14%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
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 |
Django App | Unittest test suite - Code coverage reportTotal: 80.16%Your code coverage diff: 0.07% ▴ ✅ All code changes are covered |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/django/api/tests/test_search_results.py (2)
11-16: Improve test data organization and clarity.Consider organizing test data more clearly:
- Move URL-encoded values to the URL helper method
- Use more descriptive names for test cases
- self.name = 'CHANG+KNITTING+FACTORY' - self.address = 'TONGHU+ECONOMIC+DEVELOPMENT+ZONE,HUIZHOU%2C+CHINA' - self.county_alpha_2_code = 'CN' - self.name1 = 'test name' - self.address1 = 'test address' - self.county_alpha_2_code1 = 'AX' + self.valid_search = { + 'name': 'CHANG KNITTING FACTORY', + 'address': 'TONGHU ECONOMIC DEVELOPMENT ZONE, HUIZHOU, CHINA', + 'country_code': 'CN' + } + self.no_results_search = { + 'name': 'test name', + 'address': 'test address', + 'country_code': 'AX' + }
20-62: Add test coverage for required fields validation.The mock response data is well-structured but doesn't test edge cases. Consider adding test cases for:
- Missing required fields (e.g., no coordinates)
- Empty or null values
- Different claim statuses
Example additional test case:
self.incomplete_response_mock = { "count": 1, "data": [{ "os_id": "CN2025016GP2VEJ", "name": "Test Factory", # Missing coordinates, address, etc. }] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/django/api/tests/test_search_results.py(1 hunks)
⏰ 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-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-dd-cov
🔇 Additional comments (1)
src/django/api/tests/test_search_results.py (1)
1-6: LGTM! Clean imports and good use of constants.The imports are appropriate and the mock path is properly defined as a constant.
mazursasha1990
left a comment
There was a problem hiding this comment.
Could you please check the unit tests for the Rect components that were changed?
|
@Innavin369 As for different buttons heights, pls, clarify this with designer (if you haven't do this already). |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/react/src/__tests__/components/SearchByNameAndAddressNotFoundResult.test.js (1)
39-46: Good test implementation, consider these enhancements.The test is well-structured and follows the established patterns. Consider these improvements for robustness:
- Add cleanup between tests
- Verify button existence before clicking
- Verify history.push call count
test("navigates to the contribute page when 'Add a new Location' is clicked", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + const { getByRole } = renderWithProviders(<SearchByNameAndAddressNotFoundResult />); const addNewLocation = getByRole('button', { name: 'Add a new Location' }); + expect(addNewLocation).toBeInTheDocument(); fireEvent.click(addNewLocation); expect(history.push).toHaveBeenCalledWith(productionLocationInfoRoute); + expect(history.push).toHaveBeenCalledTimes(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/react/src/__tests__/components/ConfirmNotFoundLocationDialog.test.js(2 hunks)src/react/src/__tests__/components/SearchByNameAndAddressNotFoundResult.test.js(2 hunks)src/react/src/__tests__/components/SearchByNameAndAddressSuccessResult.test.js(1 hunks)src/react/src/__tests__/components/SearchByOsIdSuccessResult.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/react/src/tests/components/SearchByOsIdSuccessResult.test.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-countries-cov
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (3)
src/react/src/__tests__/components/ConfirmNotFoundLocationDialog.test.js (2)
5-5: LGTM! Clean import addition.The addition of
productionLocationInfoRouteimport maintains consistency with existing imports and supports the new routing requirements.
75-77: LGTM! Test case properly updated.The test case correctly verifies the new routing behavior when adding a new production location. The assertion is clear and matches the expected navigation flow described in the PR objectives.
src/react/src/__tests__/components/SearchByNameAndAddressNotFoundResult.test.js (1)
4-4: LGTM! Clean import addition.The import of
productionLocationInfoRouteis properly added to the existing constants import.
src/react/src/__tests__/components/SearchByNameAndAddressSuccessResult.test.js
Show resolved
Hide resolved
|
@VadimKovalenkoSNF regarding the request for the style. I've checked and realized that all buttons are the same size in the design, 200*49, except Search by OS ID. I think it makes sense to leave the styles of this button the same as the rest. What do you think? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
doc/release/RELEASE-NOTES.md (1)
Line range hint
38-50: Improve list formatting for consistency.Consider updating the list formatting to match the rest of the document:
- Use asterisks (*) instead of dashes (-) for list items
- Use 2-space indentation instead of 4-space indentation
- - UI improvements: - - Added a toast component to display notifications during moderation event updates. - - Introduced a backdrop to prevent accidental clicks on other buttons during the update process. + * UI improvements: + * Added a toast component to display notifications during moderation event updates. + * Introduced a backdrop to prevent accidental clicks on other buttons during the update process. - - Connected GET `v1/production-locations`. - - Routing between pages `Production Location Search`,`Search returned no results`, `Production Location Information`, `Search results`, and `I don't see my Location` pop-up is configured. - - Max result limit set to 100. + * Connected GET `v1/production-locations`. + * Routing between pages `Production Location Search`,`Search returned no results`, `Production Location Information`, `Search results`, and `I don't see my Location` pop-up is configured. + * Max result limit set to 100.🧰 Tools
🪛 Markdownlint (0.37.0)
48-48: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
49-49: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
50-50: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
44-44: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
45-45: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
46-46: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
48-48: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
49-49: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
50-50: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
53-53: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
doc/release/RELEASE-NOTES.md(2 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
doc/release/RELEASE-NOTES.md
38-38: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
38-38: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
48-48: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
49-49: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
50-50: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
48-48: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
49-49: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
50-50: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (1)
doc/release/RELEASE-NOTES.md (1)
Line range hint
1-24: LGTM! Well-documented release notes with clear upgrade instructions.The release notes effectively document the significant PostgreSQL upgrade from version 12 to 13, along with clear instructions for deployment. The inclusion of links to detailed upgrade instructions and rollback procedures is particularly helpful.
🧰 Tools
🪛 Markdownlint (0.37.0)
35-35: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
36-36: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
37-37: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
38-38: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
39-39: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
40-40: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
41-41: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
35-35: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
36-36: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
37-37: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
38-38: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
39-39: Expected: 4; Actual: 8
Unordered list indentation(MD007, ul-indent)
40-40: Expected: 4; Actual: 8
Unordered list indentation(MD007, ul-indent)
41-41: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
|





OSDEV-1374 Implemented integration for the
Search resultspage to show results of searching by name and address (/contribute/production-location/search):Notes: I recommend avoiding loading 20 locations for each iteration of scrolling since the maximum limit is capped at 100 locations. On my end, the endpoint executes locally within 300-500 milliseconds, and the scroll works smoothly. Given this, I believe further complicating the component’s logic is unnecessary.