BA-2364: change password #239
Conversation
pt-tsl
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis pull request refactors the password change functionality within the authentication module by renaming the Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as useChangePassword Hook
participant API as AuthApi
Hook->>Hook: Check for token presence
alt Token present
Hook->>API: call changeExpiredPassword(payload)
else Token absent
Hook->>API: call changePassword(payload)
end
API-->>Hook: Return response
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/authentication/.eslintrc.js
packages/authentication/modules/access/index.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/authentication/.eslintrc.js
packages/authentication/modules/access/useChangePassword/index.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/authentication/.eslintrc.js
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (22)
🚧 Files skipped from review as they are similar to previous changes (20)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (9)
✨ Finishing Touches
🪧 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 (
|
pt-tsl
force-pushed
the
BA-2364-change-password
branch
3 times, most recently
from
7198523 to
af76b46
Compare
![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 (1)
packages/design-system/components/native/inputs/TextInput/index.tsx (1)
42-44: Consider avoiding hard-coded padding values.While the comments explain the rationale for subtracting 12px, hard-coding this value creates a potential maintenance issue if the padding changes in the future.
- // Had to do this adjustment to the error container width because the error text was overflowing the container - // The 12px subtraction is to account for the padding of the error container - <View style={[styles.errorContainer, { width: errorContainerWidth - 12 }]}> + // Adjust width to account for the error container's horizontal padding + <View style={[styles.errorContainer, { width: errorContainerWidth - (styles.errorContainer.paddingHorizontal * 2 || 12) }]}>Alternatively, if the padding is defined elsewhere and not accessible here:
+ // Define padding constant at the top of the file + const ERROR_CONTAINER_PADDING = 12; ... - // Had to do this adjustment to the error container width because the error text was overflowing the container - // The 12px subtraction is to account for the padding of the error container - <View style={[styles.errorContainer, { width: errorContainerWidth - 12 }]}> + // Adjust width to account for the error container's horizontal padding + <View style={[styles.errorContainer, { width: errorContainerWidth - ERROR_CONTAINER_PADDING }]}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/design-system/components/native/inputs/TextInput/index.tsx(3 hunks)packages/design-system/components/native/inputs/TextInput/styles.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/design-system/components/native/inputs/TextInput/styles.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Component Test Packages
🔇 Additional comments (4)
packages/design-system/components/native/inputs/TextInput/index.tsx (4)
4-4: Good addition for handling layout events.The import of
LayoutChangeEventfrom 'react-native' is necessary for the new layout measurement functionality. This is a clean approach to enable dynamic width calculations.
18-18: LGTM: State for dynamic width management.Adding a state variable to track the error container width helps ensure that error messages display properly without overflowing. The initial value of 0 is appropriate as it will be updated after the first layout measurement.
21-24: Clean implementation of layout handling.The
onLayoutfunction follows React Native best practices for capturing and responding to component dimensions. This is a good approach to dynamically measure the container width.
33-33: Good use of onLayout event.Adding the
onLayoutprop to the container View ensures that width measurements are captured whenever the layout changes, which supports responsive design.
pt-tsl
force-pushed
the
BA-2364-change-password
branch
2 times, most recently
from
de369b0 to
4b91e05
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsx (3)
166-215: Good addition of token-based test suite.The new test suite for token-based password changes properly verifies that the refactored hook maintains backward compatibility with the expired password flow, which is an important part of the refactoring strategy.
Consider adding additional test cases to ensure all edge cases are covered for the token flow, such as error scenarios and validation issues, similar to the main test suite.
176-177: Consider enhancing the test comment for clarity.While the comment explains that this test ensures the token-based flow has the same behavior, it would be helpful to be more specific about what aspects of the behavior should remain the same.
- // This is just to ensure that running with token has the same behavior as running without token + // This test ensures that the success callback is properly executed when using a token for expired passwords, + // maintaining backward compatibility with the previous implementation
166-215: Consider reducing duplication between test suites.There's significant duplication in setup and assertions between the two test suites. Consider extracting common testing utilities or using beforeEach hooks to reduce repetition.
For example, you could extract a common test function:
const testHookBehavior = ( options: { url: string; password: string; token?: string; mockResponse?: Record<string, any>; } ) => { const { url, password, token, mockResponse = {} } = options; const currentPassword = '1234'; test('should run onSuccess', async () => { mockFetch(url, { method: 'POST', status: 200, response: { currentPassword, newPassword: password, ...(token && { token }), ...mockResponse, }, }); let hasOnSuccessRan = false; const { result } = renderHook( () => useChangePassword({ ...(token && { token }), defaultValues: { currentPassword, newPassword: password, confirmNewPassword: password, }, options: { onSuccess: () => { hasOnSuccessRan = true; }, }, }), { wrapper: ComponentWithProviders, }, ); await result.current.form.handleSubmit(); expect(hasOnSuccessRan).toBe(true); }); };Then use it in each describe block:
describe('useChangePassword', () => { // ... setup code ... testHookBehavior({ url: changePasswordUrl, password, }); // ... other tests ... }); describe('useChangePassword with token for expired passwords', () => { // ... setup code ... testHookBehavior({ url: changePasswordUrl, password, token, }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/authentication/modules/access/index.ts(1 hunks)packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsx(6 hunks)packages/authentication/modules/access/useChangePassword/constants.ts(2 hunks)packages/authentication/modules/access/useChangePassword/index.ts(4 hunks)packages/authentication/modules/access/useChangePassword/types.ts(1 hunks)packages/authentication/services/auth.ts(2 hunks)packages/authentication/types/auth.ts(1 hunks)packages/design-system/components/native/icons/AlertTriangleIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/index.ts(1 hunks)packages/design-system/components/native/inputs/TextInput/index.tsx(3 hunks)packages/design-system/components/native/inputs/TextInput/styles.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/design-system/components/native/inputs/TextInput/styles.ts
- packages/design-system/components/native/icons/index.ts
- packages/design-system/components/native/icons/AlertTriangleIcon/index.tsx
- packages/authentication/modules/access/index.ts
- packages/authentication/modules/access/useChangePassword/constants.ts
- packages/design-system/components/native/inputs/TextInput/index.tsx
- packages/authentication/modules/access/useChangePassword/index.ts
- packages/authentication/types/auth.ts
- packages/authentication/services/auth.ts
- packages/authentication/modules/access/useChangePassword/types.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Component Test Packages
🔇 Additional comments (5)
packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsx (5)
10-15: Import and test suite renamed successfully.The renaming from
useChangeExpiredPasswordtouseChangePasswordand updating the API endpoint reflect the move to a more general password change implementation, which aligns with the PR objectives for implementing the password change feature within Login & Security settings.
36-47: Hook implementation updated correctly.The hook usage has been successfully updated in this test case to use the new
useChangePasswordhook with the appropriate parameters, maintaining the expected behavior while making it more flexible for different scenarios.
66-77: Error case testing maintained.The error handling test has been properly updated to use the new hook name while preserving the same functionality, ensuring that error conditions are still properly handled after the refactor.
100-111: Password mismatch validation test preserved.The test for validation when passwords don't match has been correctly updated to use the new hook name while maintaining the same functionality, which is crucial for the password change feature.
146-154: Custom validation support preserved.The test verifying support for custom validation schemas has been properly updated to use the new hook name, ensuring that this flexibility is maintained in the refactored implementation.
pt-tsl
force-pushed
the
BA-2364-change-password
branch
from
4b91e05 to
56094f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/design-system/components/native/views/ScrollView/index.tsx (3)
14-14: Consider platform-specific keyboard avoidance behavior.The
behavior="padding"works well on iOS but might not be optimal for Android, which often works better with"height". Consider making this configurable or platform-specific.- <KeyboardAvoidingView behavior="padding" style={{ flex: 1 }}> + <KeyboardAvoidingView + behavior={Platform.OS === 'ios' ? 'padding' : 'height'} + style={{ flex: 1 }} + keyboardVerticalOffset={Platform.OS === 'ios' ? 64 : 0}>Don't forget to add the import:
-import { KeyboardAvoidingView } from 'react-native' +import { KeyboardAvoidingView, Platform } from 'react-native'
10-10: Consider memoizing styles for performance.The
createStylesfunction is called on every render. Consider memoizing it usinguseMemoor moving it outside the component to avoid unnecessary style recalculations.const ScrollView: FC<ScrollViewProps> = ({ style, avoidKeyboard = true, children, ...props }) => { - const styles = createStyles() + const styles = useMemo(() => createStyles(), [])Don't forget to add the import:
-import { FC } from 'react' +import { FC, useMemo } from 'react'
1-8: Consider adding a documentation comment.Adding a JSDoc comment to describe the component's purpose and usage would improve developer experience, especially noting the keyboard avoiding behavior.
import { FC } from 'react' import { KeyboardAvoidingView } from 'react-native' import { ScrollView as NativeScrollView } from 'react-native-gesture-handler' import { createStyles } from './styles' import type { ScrollViewProps } from './types' +/** + * A scrollable view component with optional keyboard avoiding behavior. + * When `avoidKeyboard` is true, it wraps the content in a KeyboardAvoidingView + * to ensure the content remains visible when the keyboard is shown. + */ const ScrollView: FC<ScrollViewProps> = ({ style, avoidKeyboard = true, children, ...props }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/authentication/modules/access/index.ts(1 hunks)packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsx(6 hunks)packages/authentication/modules/access/useChangePassword/constants.ts(2 hunks)packages/authentication/modules/access/useChangePassword/index.ts(4 hunks)packages/authentication/modules/access/useChangePassword/types.ts(1 hunks)packages/authentication/services/auth.ts(2 hunks)packages/authentication/types/auth.ts(1 hunks)packages/design-system/components/native/buttons/Button/styles.ts(2 hunks)packages/design-system/components/native/buttons/Button/types.ts(1 hunks)packages/design-system/components/native/icons/AlertTriangleIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/index.ts(1 hunks)packages/design-system/components/native/inputs/TextInput/index.tsx(3 hunks)packages/design-system/components/native/inputs/TextInput/styles.ts(1 hunks)packages/design-system/components/native/views/ScrollView/index.tsx(1 hunks)packages/design-system/components/native/views/ScrollView/styles.ts(1 hunks)packages/design-system/components/native/views/ScrollView/types.ts(1 hunks)packages/design-system/components/native/views/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/design-system/components/native/icons/index.ts
- packages/design-system/components/native/inputs/TextInput/styles.ts
- packages/authentication/services/auth.ts
- packages/authentication/types/auth.ts
- packages/design-system/components/native/icons/AlertTriangleIcon/index.tsx
- packages/authentication/modules/access/index.ts
- packages/authentication/modules/access/useChangePassword/constants.ts
- packages/authentication/modules/access/useChangePassword/tests/useChangePassword.test.tsx
- packages/design-system/components/native/inputs/TextInput/index.tsx
- packages/authentication/modules/access/useChangePassword/index.ts
- packages/authentication/modules/access/useChangePassword/types.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/design-system/components/native/views/ScrollView/index.tsx (2)
packages/design-system/components/native/views/ScrollView/types.ts (1)
ScrollViewProps(5-7)packages/design-system/components/native/views/ScrollView/styles.ts (1)
createStyles(3-9)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Component Test Packages
🔇 Additional comments (7)
packages/design-system/components/native/views/ScrollView/styles.ts (1)
1-9: Clean implementation of ScrollView container stylesThe
createStylesfunction provides a clean implementation of basic styles for a ScrollView container. Settingflex: 1allows the component to fill available space, whilepadding: 0gives control over spacing to child elements instead of the container itself.packages/design-system/components/native/views/ScrollView/types.ts (1)
1-7: Well-structured type definition for ScrollViewThe
ScrollViewPropsinterface correctly extends React Native's ScrollView component props usingComponentProps<typeof ScrollView>. The addition of the optionalavoidKeyboardproperty suggests this component will provide keyboard-aware scrolling functionality, which is a valuable enhancement.packages/design-system/components/native/buttons/Button/types.ts (1)
10-14: Appropriate extension of ButtonStylesOptionsAdding the optional
modeproperty toButtonStylesOptionsis a logical extension that increases the flexibility of button styling. This change aligns well with the existingButtonModetype defined on line 5 and follows the established pattern for optional properties.packages/design-system/components/native/buttons/Button/styles.ts (2)
49-55: Visual update to outlined button text colorThe change from using
colors.object.contrasttocolors.object.highfor the text color when the variant is 'inherit' likely represents a design adjustment to improve visual hierarchy or readability.
69-73: Fixed typographical error in commentThe comment has been corrected from "inherid" to "inherit", which improves code clarity.
packages/design-system/components/native/views/index.ts (1)
7-8: LGTM! Consistent export pattern followed.The export statements for the new ScrollView component and its types follow the existing pattern and maintain alphabetical ordering in the file.
packages/design-system/components/native/views/ScrollView/index.tsx (1)
7-7:✅ Verification successful
Check for circular type references in ScrollViewProps.
Based on the relevant code snippets,
ScrollViewPropsextendsComponentProps<typeof ScrollView>, which could create a circular reference sinceScrollViewis being defined in this file. Verify this doesn't cause TypeScript issues.
🏁 Script executed:
#!/bin/bash # Verify the type definition in ScrollView/types.ts cat packages/design-system/components/native/views/ScrollView/types.tsLength of output: 260
Verification Complete: No Circular Reference Detected
The
ScrollViewPropsinterface inpackages/design-system/components/native/views/ScrollView/types.tsextendsComponentProps<typeof ScrollView>, but sinceScrollViewis imported from react-native (and not defined locally within this file), it does not introduce a circular type reference with the locally defined component inindex.tsx.
- File:
packages/design-system/components/native/views/ScrollView/types.ts
import { ScrollView } from 'react-native'ensures thatScrollViewPropsuses the external component type.Based on the verification, there are no TypeScript issues related to circular type references here.
packages/design-system/components/native/views/ScrollView/index.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/components/native/views/ScrollView/index.tsx
Outdated
Show resolved
Hide resolved
anicioalexandre
added
the
approved
pt-tsl
force-pushed
the
BA-2364-change-password
branch
from
56094f3 to
acabe20
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/design-system/components/native/views/ScrollView/styles.ts (1)
5-5: Consider fixing naming inconsistency in style property.The style property
scrollViewcontainerhas a naming inconsistency. To maintain proper camelCase convention, consider renaming it toscrollViewContainer(capitalize the 'c' in "container").- scrollViewcontainer: { + scrollViewContainer: {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/authentication/CHANGELOG.md(1 hunks)packages/authentication/package.json(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/native/buttons/Button/styles.ts(2 hunks)packages/design-system/components/native/views/ScrollView/index.tsx(1 hunks)packages/design-system/components/native/views/ScrollView/styles.ts(1 hunks)packages/design-system/components/native/views/ScrollView/types.ts(1 hunks)packages/design-system/components/native/views/index.ts(1 hunks)packages/design-system/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/authentication/package.json
- packages/wagtail/package.json
- packages/design-system/package.json
- packages/components/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/design-system/components/native/views/ScrollView/types.ts
- packages/design-system/components/native/buttons/Button/styles.ts
- packages/design-system/components/native/views/index.ts
- packages/design-system/components/native/views/ScrollView/index.tsx
🔇 Additional comments (5)
packages/design-system/components/native/views/ScrollView/styles.ts (1)
3-12: LGTM! Clean styling implementation.The style implementation is clean and follows React Native best practices. The use of
flex: 1in both styles appropriately ensures the containers will expand to fill available space, which works well with keyboard handling and scrollable content.packages/wagtail/CHANGELOG.md (1)
3-9: Version Update for Wagtail Changelog
The changelog update for version 1.0.27 clearly documents the patch changes and notes the dependency update of@baseapp-frontend/design-systemto 1.0.10. This entry is concise and aligns well with the overall dependency updates across packages.packages/components/CHANGELOG.md (1)
3-10: Components Changelog Version Update
The new version 1.0.27 entry reflects the appropriate dependency updates—namely, updating@baseapp-frontend/authenticationto 4.2.1 and@baseapp-frontend/design-systemto 1.0.10. The update is clearly structured and consistent with the broader versioning strategy.packages/authentication/CHANGELOG.md (1)
3-9: Authentication Package Update for Password Change Feature
The changelog for version 4.2.1 appropriately captures the renaming of theuseChangeExpiredPasswordhook touseChangePasswordas well as its enhancement to include the/change-passwordendpoint. This change is critical to the new password change functionality described in the PR objectives.Please ensure that all related tests and documentation in the authentication module have been updated accordingly.
packages/design-system/CHANGELOG.md (1)
3-11: Design System Changelog Update for New Components and Fixes
The version 1.0.10 update documents several important changes:
- The addition of the
ScrollViewcomponent with integratedKeyboardAvoidingViewsupport.- Modifications to the native
TextInputto prevent the error message view from overflowing.- The introduction of the
AlertTriangleIconto the native icons set.- A fix for the text color issue in the
Buttoncomponent'soutlinedvariant.These changes are clearly stated and consistent with updates noted across related packages.
pt-tsl
force-pushed
the
BA-2364-change-password
branch
from
acabe20 to
28f3be0
Compare
|
Acceptance Criteria
Guardrails
Although the flow in Figma shows a modal asking the user if they want to end all current sessions, we have to check if that functionality was already built by PORG.
So for now just update the password without showing the modal.
Design Link: https://www.figma.com/design/z5ZdVlKEMmGIX4GVnjwfxA/BaseApp---NATIVE?node-id=6046-17914&t=ZYZocuoVLM11VrtX-0
Approvd
https://app.approvd.io/silverlogic/BA/stories/39018
Summary by CodeRabbit