[OSDEV-1747] Check auth for SLC pages.#523
Conversation
…arch by OS id res pages
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
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 |
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.91%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 |
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Django App | Unittest test suite - Code coverage reportTotal: 80.44%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
![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/__tests__/components/SearchByOsIdResult.test.js (2)
51-60: Fix double space afterconstkeyword.The test logic is well-structured and comprehensive, but there's a minor style issue.
- const expectedTitle = 'Production Location Search' + const expectedTitle = 'Production Location Search'
62-75: Fix style issues.The test logic is well-structured and comprehensive, but there are minor style issues.
- const expectedTitle = 'Production Location Search' + const expectedTitle = 'Production Location Search' - - expect(title).toBeInTheDocument(); + expect(title).toBeInTheDocument();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/react/src/__tests__/components/SearchByOsIdResult.test.js(2 hunks)
🔇 Additional comments (3)
src/react/src/__tests__/components/SearchByOsIdResult.test.js (3)
9-14: LGTM! Well-structured mock state for unauthorized users.The mock state correctly simulates an unauthorized user state with appropriate nesting and properties.
16-39: LGTM! Well-designed test utility function.The updated
renderComponentfunction provides good flexibility for testing both authorized and unauthorized states. The default authorized state with override capability is a clean approach.
8-109: LGTM! Comprehensive test coverage for authentication states.The test suite effectively validates the component's behavior for both authorized and unauthorized users, supporting the PR's goal of securing SLC pages. The use of testing-library best practices and consistent patterns throughout the suite is commendable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/react/src/components/Contribute/ContributeProductionLocation.jsx (2)
20-24: Add accessibility attributes to the loading indicator.The loading state is handled correctly, but the CircularProgress component should have accessibility attributes to improve user experience for screen readers.
<div className={classes.circularProgressContainerStyles}> - <CircularProgress /> + <CircularProgress aria-label="Loading authentication status" /> </div>Also applies to: 49-55
110-123: Remove unused state mappings.The
dataandfetchingproperties are mapped from state but not used in the component.const mapStateToProps = ({ - contributeProductionLocation: { - singleProductionLocation: { data, fetching }, - }, auth: { user: { user }, session: { fetching: fetchingSessionSignIn }, }, }) => ({ - data, - fetching, userHasSignedIn: !user.isAnon, fetchingSessionSignIn, });
📜 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/__tests__/components/ContributeProductionLocation.test.js(2 hunks)src/react/src/components/Contribute/ContributeProductionLocation.jsx(3 hunks)src/react/src/components/Contribute/ProductionLocationInfo.jsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- doc/release/RELEASE-NOTES.md
- src/react/src/components/Contribute/ProductionLocationInfo.jsx
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: run-flake8-linter
- 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-dd-cov
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (3)
src/react/src/components/Contribute/ContributeProductionLocation.jsx (2)
4-6: LGTM! Imports and constants are well-organized.The new imports and constants support the authentication functionality and improve code maintainability.
Also applies to: 12-12, 18-18
57-59: LGTM! Authentication check is properly implemented.The component correctly handles unauthenticated users by showing the login form, aligning with the PR objectives to ensure SLC pages are accessible only to authorized users.
src/react/src/__tests__/components/ContributeProductionLocation.test.js (1)
11-24: LGTM! Mock states are well-defined.The mock states for authorized and unauthorized users are clear and reusable across tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/react/src/components/Contribute/ContributeProductionLocation.jsx (1)
117-130: Simplify Redux state mapping and remove unused props.The state mapping uses deep nested destructuring and maps unused props (data, fetching).
Consider simplifying the state mapping:
const mapStateToProps = ({ - contributeProductionLocation: { - singleProductionLocation: { data, fetching }, - }, auth: { user: { user }, session: { fetching: fetchingSessionSignIn }, }, }) => ({ - data, - fetching, userHasSignedIn: !user.isAnon, fetchingSessionSignIn, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/react/src/__tests__/components/AuthLogInFromRoute.test.js(1 hunks)src/react/src/components/Contribute/ContributeProductionLocation.jsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/src/tests/components/AuthLogInFromRoute.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-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
🔇 Additional comments (3)
src/react/src/components/Contribute/ContributeProductionLocation.jsx (3)
4-6: LGTM! Well-organized imports and constants.The new imports are properly organized and the TITLE constant improves maintainability.
Also applies to: 12-12, 18-18
20-24: LGTM! Well-structured authentication flow.The component now properly handles authentication states with clear loading feedback and consistent title usage.
Also applies to: 49-60, 64-64
102-115: LGTM! Complete PropTypes definition.All required props and class properties are now properly defined in PropTypes, addressing the previous review comment.
VadimKovalenkoSNF
left a comment
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Replace your change to release 1.31
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/react/src/components/Contribute/ContributeProductionLocation.jsx (1)
117-130: Properly mapped Redux state.The Redux state mapping correctly includes both the production location data and authentication state. However,
dataandfetchingare mapped but not used in the component.- data, - fetching,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
doc/release/RELEASE-NOTES.md(2 hunks)src/react/src/__tests__/components/AuthLogInFromRoute.test.js(1 hunks)src/react/src/__tests__/components/ContributeProductionLocation.test.js(2 hunks)src/react/src/components/AddLocationData.jsx(2 hunks)src/react/src/components/Contribute.jsx(2 hunks)src/react/src/components/Contribute/ContributeProductionLocation.jsx(3 hunks)src/react/src/components/Contribute/ProductionLocationInfo.jsx(7 hunks)src/react/src/components/Contribute/SearchByNameAndAddressResult.jsx(4 hunks)src/react/src/components/Contribute/SearchByOsIdResult.jsx(4 hunks)src/react/src/components/Dashboard.jsx(7 hunks)src/react/src/components/RequireAuthNotice.jsx(1 hunks)src/react/src/components/Settings/Settings.jsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- doc/release/RELEASE-NOTES.md
- src/react/src/components/Contribute.jsx
- src/react/src/components/Settings/Settings.jsx
- src/react/src/components/Contribute/SearchByNameAndAddressResult.jsx
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- 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-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 (28)
src/react/src/components/RequireAuthNotice.jsx (1)
1-31: LGTM! Good implementation of a reusable authentication notice component.The component provides a clean, modular way to display authentication notices across the application. Props are properly defined with appropriate defaults, and the implementation is straightforward.
src/react/src/components/AddLocationData.jsx (2)
13-13: Great modular approach with RequireAuthNotice component.Adding this import supports the PR's goal of better modularity in authentication handling.
46-46: Good refactoring to use RequireAuthNotice.Replacing the previous implementation with RequireAuthNotice simplifies the code and improves maintainability.
src/react/src/__tests__/components/ContributeProductionLocation.test.js (6)
11-23: Good test setup with mock states.Creating separate mock states for authorized and unauthorized users improves test organization and readability.
25-31: Improved test utility function.Updating the renderComponent function to accept a preloadedState parameter makes the tests more flexible.
33-35: Good test hygiene with beforeEach.Adding a beforeEach hook to clear mocks ensures tests don't affect each other.
37-46: Good test coverage for unauthorized users.This test properly verifies that unauthorized users see the login link with the correct attributes.
48-58: Good test coverage for authorized users.This test ensures authorized users see the appropriate UI elements.
1-108: Add test for loading state.The current tests don't cover the component's loading state behavior.
it('renders loading indicator when fetching session', () => { const loadingState = { auth: { user: { user: { isAnon: true } }, session: { fetching: true }, }, }; const { getByRole } = renderComponent(loadingState); expect(getByRole('progressbar')).toBeInTheDocument(); });src/react/src/components/Contribute/SearchByOsIdResult.jsx (9)
11-11: Good import of RequireAuthNotice component.Importing the RequireAuthNotice component is consistent with the PR's goal of centralizing authentication UI.
31-32: Good addition of authentication props.Adding userHasSignedIn and fetchingSessionSignIn as props ensures the component can handle authentication states.
34-34: Good use of constant for title.Extracting the title to a constant improves maintainability and reduces duplication.
56-56: Good enhancement of loading state check.Including fetchingSessionSignIn in the loading check ensures a consistent user experience while authentication state is being determined.
64-66: Good handling of unauthorized users.Adding a check for unauthorized users and rendering the RequireAuthNotice component ensures proper authentication enforcement.
75-75: Good use of constant.Using the TITLE constant instead of a hardcoded string improves maintainability.
105-106: Good prop type validation.Adding propType validation for the new authentication props ensures proper usage of the component.
113-116: Good state mapping.Updating mapStateToProps to include auth state ensures the component has access to authentication information.
120-121: Good prop mapping.Mapping auth state to component props ensures the component can react to authentication changes.
src/react/src/components/Contribute/ProductionLocationInfo.jsx (3)
86-86: Good improvement: Using a constant for the title.Creating a TITLE constant and using it consistently improves maintainability and reduces the chance of typos.
394-404: Proper authentication flow implementation.The authentication flow is correctly implemented with appropriate states:
- Loading indicator when fetching session data
- Authentication notice when user is not signed in
- Only showing content to authenticated users
This ensures SLC pages are properly protected as required in the PR objective.
947-948: Redux state mapped correctly for authentication.The Redux state mapping properly derives the user authentication status and fetching state, enabling the component to make authentication-based rendering decisions.
src/react/src/components/Dashboard.jsx (3)
59-59: Good improvement: Extracted title to a constant.Extracting the title to a TITLE constant improves consistency and maintainability throughout the component.
93-100: Enhanced security with proper authentication check.Adding a dedicated authentication check before the dashboard access check improves security by ensuring only authenticated users can access any dashboard features. This is a key improvement that aligns with the PR objective of checking authentication for SLC pages.
366-366: Consistent authentication state derivation.The user authentication state is consistently derived from
!user.isAnon, matching the pattern used in other components. This ensures uniform authentication checks across the application.src/react/src/__tests__/components/AuthLogInFromRoute.test.js (2)
27-38: Good test for navigation functionality.The test properly verifies both component rendering and navigation functionality when the link is clicked, which ensures the component behaves correctly.
23-25:Details
✅ Verification successful
Verify default values match the component defaults.
The test hardcodes default title and text values. Ensure these match the actual default values in the RequireAuthNotice component to make the test more resilient to changes in the component's defaults.
🏁 Script executed:
#!/bin/bash # Check the default props in the RequireAuthNotice component grep -A 10 "defaultProps" $(find src -name "RequireAuthNotice.jsx")Length of output: 310
Default values are correctly aligned
The default title and text used in the test match the default values defined in the
RequireAuthNoticecomponent. No changes are needed.src/react/src/components/Contribute/ContributeProductionLocation.jsx (2)
49-59: Proper authentication flow implementation.The authentication flow follows the same pattern as in other components:
- First checking if session data is being fetched
- Then checking if the user is authenticated
- Only showing content to authenticated users
This implementation ensures consistent user experience and security across the application.
102-115: Complete PropTypes definition.The PropTypes definition is comprehensive, including all the class properties used in the component. This ensures proper type checking and documentation of the component's props.
|
5 participants
OSDEV-1747 SLC. all pages for SLC should be available for authorized users only.