feat(app): add first month of report in person details #3127
Conversation
WalkthroughThe changes in this pull request include updates to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
✅ Deploy Preview for preview-organized ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@semantic-release/[email protected] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (15)
src/features/meetings/midweek_export/S140TemplateSelector/index.tsx (4)
Line range hint
35-45: Add meaningful alt text for accessibilityThe image element is missing a meaningful alt text description. Empty alt attributes should only be used for decorative images.
<img src={item.small} - alt="" + alt={`${item.name} template preview`} style={{
Line range hint
19-47: Consider keyboard accessibility for fullscreen featureThe fullscreen button is only visible on hover, making it inaccessible to keyboard users. Consider:
- Making the button always visible for keyboard users
- Adding keyboard shortcuts
- Ensuring proper focus management
<Box onMouseEnter={() => setS140TemplateItemHovered(true)} onMouseLeave={() => setS140TemplateItemHovered(false)} + onFocus={() => setS140TemplateItemHovered(true)} + onBlur={() => setS140TemplateItemHovered(false)} sx={{ position: 'relative', marginBottom: '-5px' }} >
Line range hint
35-47: Move inline styles to sx prop for consistencyThe component uses both inline styles and the sx prop. For consistency and better maintainability, consider moving all styles to the sx prop.
<img src={item.small} alt={`${item.name} template preview`} - style={{ - width: 'auto', - height: '240px', - borderRadius: '2.97px', - cursor: 'pointer', - border: selected === item.id - ? '2px solid var(--accent-main)' - : '2px solid var(--accent-200)', - boxShadow: '0px 1.48px 5.94px 0px #1C1C1C1F', - }} + sx={{ + width: 'auto', + height: '240px', + borderRadius: '2.97px', + cursor: 'pointer', + border: (theme) => selected === item.id + ? `2px solid ${theme.vars.accent.main}` + : `2px solid ${theme.vars.accent[200]}`, + boxShadow: '0px 1.48px 5.94px 0px #1C1C1C1F', + }} />
Line range hint
14-89: Consider memoizing S140TemplateItem for performanceThe template item component could benefit from memoization to prevent unnecessary re-renders when parent state changes.
-const S140TemplateItem = ({ +const S140TemplateItem = React.memo(({ item, onChange, selected, -}: S140TemplateItemType) => { +}: S140TemplateItemType) => { // ... component implementation -}; +});src/features/meetings/midweek_export/S89TemplateSelector/index.tsx (1)
Line range hint
1-89: Significant code duplication with S140TemplateSelectorThe S89TemplateSelector and S140TemplateSelector components are nearly identical, which violates the DRY principle and makes maintenance harder.
Consider creating a shared base component:
// src/features/meetings/midweek_export/components/BaseTemplateSelector.tsx import { ReactNode } from 'react'; import { PhotoProvider, PhotoView } from 'react-photo-view'; import { Box } from '@mui/material'; import { useBreakpoints } from '@hooks/index'; interface BaseTemplateItemProps { id: string; name: string; desc: string; src: string; small: string; selected: boolean; onChange: (id: string) => void; } export const BaseTemplateSelector = ({ items, selected, onChange, renderItem, }: { items: BaseTemplateItemProps[]; selected: string; onChange: (id: string) => void; renderItem?: (props: BaseTemplateItemProps) => ReactNode; }) => { const { tabletDown } = useBreakpoints(); return ( <PhotoProvider maskClosable={false} maskOpacity={0.5}> <Box sx={{ display: 'flex', flexWrap: 'wrap', gap: '10px', justifyContent: tabletDown ? 'center' : 'flex-start', }} > {items.map((item) => renderItem?.({ ...item, selected, onChange }))} </Box> </PhotoProvider> ); };Then create specific implementations:
// S89TemplateSelector.tsx const S89TemplateSelector = (props: S89TemplateSelectorType) => { const { images, handleChange } = useS89TemplateSelector(props); return ( <BaseTemplateSelector items={images} selected={props.selected} onChange={handleChange} renderItem={(itemProps) => ( <S89TemplateItem key={itemProps.id} {...itemProps} /> )} /> ); };Would you like me to create a GitHub issue to track this refactoring task?
src/features/persons/spiritual_status/first_report/useFirstReport.tsx (2)
15-15: Remove unnecessaryasynckeyword fromhandleChangeFirstReportfunctionThe
handleChangeFirstReportfunction does not contain anyawaitexpressions, so it does not need to be declared asasync. Removing theasynckeyword can prevent confusion and improve readability.Apply this diff to remove the unnecessary
asynckeyword:-const handleChangeFirstReport = async (value: Date) => { +const handleChangeFirstReport = (value: Date) => {
23-24: Simplify date manipulation for settingfinalValueCurrently, the code formats the date to a string and then parses it back to a date object. This can be simplified by directly creating a new date representing the first day of the month.
Apply this diff to simplify the date handling:
-const startMonth = formatDate(value, 'yyyy/MM/01'); -finalValue = new Date(startMonth).toISOString(); +const firstDayOfMonth = new Date(value.getFullYear(), value.getMonth(), 1); +finalValue = firstDayOfMonth.toISOString();src/services/worker/backupAction.ts (1)
32-32: Consider using an enum for backup status.The string literals for backup status ('started', 'completed', 'failed') are used throughout the code. Consider defining an enum to prevent typos and improve maintainability.
+enum BackupStatus { + STARTED = 'started', + COMPLETED = 'completed', + FAILED = 'failed' +} + -let backup = ''; +let backup = BackupStatus.STARTED;Also applies to: 40-42
src/features/whats_new/useWhatsNew.tsx (2)
25-32: Good use of memoization for performance optimization.The useMemo hooks for releases and version calculation prevent unnecessary recalculations. However, consider extracting the version sorting logic into a separate utility function for better testability.
+const getLatestVersion = (releases: ReleaseNoteType): string => { + const releasesDates = Object.keys(releases); + return releasesDates.sort().reverse()[0] ?? 'next'; +}; const version = useMemo(() => { - const releasesDates = Object.keys(releases); - return releasesDates.sort().reverse().at(0); + return getLatestVersion(releases); }, [releases]);
96-96: Verify dependency array completeness.The useEffect dependency array includes
appLangandi18n, but these aren't directly used in the effect. Consider removing unused dependencies.-}, [appLang, i18n, version, releases]); +}, [version, releases]);src/features/congregation/app_access/user_details/user_additional_rights/useUserAdditionalRights.tsx (1)
20-20: Consider refactoring repeated role initialization logicThe initialization of
cong_roleis repeated across all toggle handlers. Consider extracting this into a utility function to improve maintainability and reduce duplication.+ const ensureUserRoles = (user) => { + const newUser = structuredClone(user); + newUser.profile.cong_role = newUser.profile.cong_role || []; + return newUser; + }; const handleToggleMidweek = async (value: boolean) => { try { setIsMidweek(value); - const newUser = structuredClone(user); - newUser.profile.cong_role = newUser.profile.cong_role || []; + const newUser = ensureUserRoles(user);Also applies to: 50-50, 80-80, 110-110
src/features/congregation/app_access/user_details/user_main_roles/useUserMainRoles.tsx (1)
151-152: Align variable naming with state variableThe variable
isFieldOverseeris used to setisServiceOverseerstate. Consider using consistent naming to improve code clarity.- const isFieldOverseer = + const isServiceOverseer = user.profile.cong_role?.includes('service_overseer') ?? false; setIsServiceOverseer(isFieldOverseer);src/components/date_picker/index.tsx (1)
104-114: Consider keyboard accessibility improvementsThe Enter key handling is good, but consider adding:
- Escape key handling to close the picker
- Tab key handling for better keyboard navigation
const handleKeyDown = (e: KeyboardEvent<Element>) => { - if (e.key !== 'Enter') return; + switch (e.key) { + case 'Enter': + if (!isValid(innerValue)) return; + setValueTmp(innerValue); + setOpen(false); + onChange?.(innerValue); + break; + case 'Escape': + setOpen(false); + break; + default: + break; + } - - const isValidDate = isValid(innerValue); - - if (!isValidDate) return; - - setValueTmp(innerValue); - setOpen(false); - onChange?.(innerValue); };src/utils/date.ts (1)
354-356: Add JSDoc documentation for better code maintainabilityConsider adding JSDoc documentation to describe the function's purpose, parameters, and return value.
+/** + * Validates if the given value is a valid date + * @param value - Value to validate + * @returns true if the value is a valid date, false otherwise + */ export const isValidDate = (value: unknown) => { return isValid(value); };CHANGELOG.md (1)
3-3: Fix heading increment in changelog.The heading level should only increment by one level at a time.
Change the heading format:
-### Bug Fixes +## Bug Fixes🧰 Tools
🪛 Markdownlint (0.35.0)
3-3: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.jsonsrc/locales/en/ministry.jsonis excluded by!**/*.json
📒 Files selected for processing (23)
CHANGELOG.md(1 hunks)release.config.cjs(0 hunks)release.config.mjs(1 hunks)src/App.tsx(2 hunks)src/components/date_picker/index.tsx(3 hunks)src/definition/person.ts(1 hunks)src/features/congregation/app_access/congregation_admin/useCongregationAdmin.tsx(1 hunks)src/features/congregation/app_access/user_details/profile_settings/useProfileSettings.tsx(1 hunks)src/features/congregation/app_access/user_details/user_additional_rights/useUserAdditionalRights.tsx(5 hunks)src/features/congregation/app_access/user_details/user_main_roles/useUserMainRoles.tsx(5 hunks)src/features/meetings/midweek_export/S140TemplateSelector/index.tsx(1 hunks)src/features/meetings/midweek_export/S89TemplateSelector/index.tsx(1 hunks)src/features/persons/spiritual_status/baptized_publisher/index.tsx(2 hunks)src/features/persons/spiritual_status/baptized_publisher/useBaptizedPublisher.tsx(1 hunks)src/features/persons/spiritual_status/first_report/index.tsx(1 hunks)src/features/persons/spiritual_status/first_report/useFirstReport.tsx(1 hunks)src/features/persons/spiritual_status/unbaptized_publisher/index.tsx(2 hunks)src/features/persons/spiritual_status/useSpiritualStatus.tsx(0 hunks)src/features/whats_new/useWhatsNew.tsx(3 hunks)src/services/dexie/schema.ts(1 hunks)src/services/worker/backupAction.ts(2 hunks)src/services/worker/backupUtils.ts(1 hunks)src/utils/date.ts(2 hunks)
💤 Files with no reviewable changes (2)
- src/features/persons/spiritual_status/useSpiritualStatus.tsx
- release.config.cjs
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGELOG.md
3-3: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🔇 Additional comments (17)
release.config.mjs (1)
1-37: Configuration for semantic-release is correctly set up
The release.config.mjs file is properly configured with the necessary branches and plugins for semantic-release. The settings align with best practices for automated releases.
src/features/persons/spiritual_status/first_report/index.tsx (1)
7-33: FirstReport component is well implemented
The FirstReport component correctly utilizes hooks for translation and user context. The integration of DatePicker and Typography components is appropriate and follows good React practices.
src/features/congregation/app_access/congregation_admin/useCongregationAdmin.tsx (1)
51-51: Ensure cong_role defaults to an empty array if undefined
Using user.profile?.cong_role || [] safely handles cases where cong_role might be undefined, preventing potential runtime errors when accessing user roles.
src/features/persons/spiritual_status/unbaptized_publisher/index.tsx (2)
6-6: Importing FirstReport component correctly
The FirstReport component is properly imported from the relative path, ensuring it can be used within this module.
49-49: Integrate FirstReport component into the JSX
Adding the FirstReport component enhances the functionality of the UnbaptizedPublisher component by displaying the first report information.
src/definition/person.ts (1)
73-73: LGTM! Verify usage in related components.
The first_report type definition follows the established pattern for date-related fields and correctly marks it as optional. This aligns with the PR objective of adding first month of report tracking.
Let's verify the consistent usage of this new field:
✅ Verification successful
The first_report field is consistently used across the codebase
The new field is properly integrated and used in relevant components:
- Schema initialization in
src/services/dexie/schema.ts - User context management in
src/hooks/useCurrentUser.tsx - Spiritual status features in baptized/unbaptized publisher components
- Monthly and yearly statistics calculations in service year components
- Monthly report handling in publisher report feature
The implementation follows a consistent pattern across all usages, properly handling the optional nature of the field.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components and hooks using first_report
rg -l "first_report" --type tsx --type ts
Length of output: 69
Script:
#!/bin/bash
# Search for components and hooks using first_report without file type restrictions
rg "first_report"
Length of output: 1909
src/features/congregation/app_access/user_details/user_additional_rights/useUserAdditionalRights.tsx (1)
135-149: LGTM: Improved null safety with optional chaining
The use of optional chaining (?.) and nullish coalescing (??) operators improves the robustness of role checks.
src/features/congregation/app_access/user_details/profile_settings/useProfileSettings.tsx (2)
54-54: LGTM: Improved role initialization with optional chaining
The use of optional chaining for cong_role initialization and Array/Set combination for unique role assignment is a robust approach.
Line range hint 20-149: Verify consistent role handling across codebase
Let's verify that all role-related code follows the same pattern of safe initialization and access.
✅ Verification successful
Based on the search results, I can now provide a final response regarding the role handling across the codebase:
Role handling is consistent and follows safe initialization patterns
The codebase demonstrates consistent role handling patterns:
- All role modifications properly initialize
cong_rolewith a fallback empty array:newUser.profile.cong_role = newUser.profile.cong_role || [] - Role updates use immutable operations (filter, push) and maintain array uniqueness (Set)
- Optional chaining is used when accessing roles:
user.profile.cong_role?.includes() - Role assignments follow a clear pattern of adding/removing specific roles based on user privileges
- The code under review in
useProfileSettings.tsxfollows these same patterns with proper initialization and Set usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential unsafe access to cong_role across the codebase
# Find potential unsafe access to cong_role without optional chaining
echo "Checking for unsafe cong_role access..."
rg "profile\.cong_role\." --type ts --type tsx
# Find inconsistent role initialization patterns
echo "Checking for role initialization patterns..."
rg "cong_role\s*=\s*" --type ts --type tsx
# Find all role-related code to verify consistency
echo "Listing all role-related code..."
rg "cong_role" --type ts --type tsx -C 2
Length of output: 481
Script:
#!/bin/bash
# Let's try without file type restrictions since tsx is not recognized
echo "Checking for unsafe cong_role access..."
rg "profile\.cong_role\."
echo "Checking for role initialization patterns..."
rg "cong_role\s*=\s*"
echo "Listing all role-related code..."
rg "cong_role" -C 2
Length of output: 57278
src/features/persons/spiritual_status/baptized_publisher/index.tsx (1)
149-149: Verify FirstReport component integration
The FirstReport component is rendered without any props. Please verify if this is intentional or if it needs to receive props for proper functionality (e.g., readOnly state which is passed to other components).
✅ Verification successful
Let me check the FirstReport component's implementation to verify if it requires any props.
FirstReport component is correctly integrated
The FirstReport component is properly implemented as a self-contained component that doesn't require any external props. It internally:
- Uses hooks for translations and current user state
- Manages its own state through useFirstReport custom hook
- Handles readonly state internally using isPersonEditor from useCurrentUser
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check FirstReport component's props and usage
ast-grep --pattern 'FirstReport'
Length of output: 637
Script:
#!/bin/bash
# Check FirstReport component implementation
cat src/features/persons/spiritual_status/first_report/index.tsx
Length of output: 960
src/features/persons/spiritual_status/baptized_publisher/useBaptizedPublisher.tsx (2)
167-169: LGTM: Proper filtering of history records
The filtering logic correctly excludes deleted records and ensures start_date is not null.
171-183: Consider edge cases in history creation logic
The automatic creation of history entry when setting baptism date is good, but consider these edge cases:
- What happens if the baptism date is changed multiple times?
- Should we validate that the new history entry doesn't overlap with existing ones?
Also, consider extracting the date formatting logic to a separate function for better maintainability.
+const createHistoryEntry = (date: Date) => ({
+ _deleted: false,
+ end_date: null,
+ id: crypto.randomUUID(),
+ start_date: new Date(formatDate(date, 'yyyy/MM/01')).toISOString(),
+ updatedAt: new Date().toISOString(),
+});
if (histories.length === 0 && value) {
- const startMonth = new Date(
- formatDate(value, 'yyyy/MM/01')
- ).toISOString();
-
- newPerson.person_data.publisher_baptized.history.push({
- _deleted: false,
- end_date: null,
- id: crypto.randomUUID(),
- start_date: startMonth,
- updatedAt: new Date().toISOString(),
- });
+ newPerson.person_data.publisher_baptized.history.push(
+ createHistoryEntry(value)
+ );
}src/components/date_picker/index.tsx (2)
90-96: LGTM: Proper date validation before updates
The addition of isValid check before updating the date is a good improvement for data integrity.
219-219: LGTM: Proper keyboard event handling integration
The keyboard event handler is correctly integrated with the field props.
src/App.tsx (1)
69-69: LGTM: Role-based route protection is properly implemented
The changes correctly implement role-based access control for the field service groups route, allowing access to publishers, appointed members, and service committee members.
Also applies to: 109-122
src/services/dexie/schema.ts (1)
205-205: LGTM: Schema addition follows existing patterns
The first_report property is correctly structured and initialized, following the established patterns in the schema.
src/services/worker/backupUtils.ts (1)
1459-1480: LGTM! The person data handling looks secure and well-implemented.
The implementation correctly:
- Filters out deleted records before backup
- Encrypts sensitive person data
- Maintains data privacy by only including necessary fields
Let's verify the encryption implementation:
✅ Verification successful
The encryption implementation for person data is correctly and consistently applied
The verification confirms that all assignments to obj.persons are properly secured:
- When handling multiple persons: Data is encrypted via
encryptObjectin thebackupPersonsmapping before assignment - When handling single person records: Encryption is performed directly before the assignment
- All cases use the correct encryption parameters (table, masterKey, accessCode)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify encryption usage consistency
# Test: Check if all sensitive data is properly encrypted
# Look for any direct assignments of person data without encryption
rg -A 5 "obj\.persons\s*=" | grep -v "encryptObject"
# Verify encryption is used consistently for person data
ast-grep --pattern 'encryptObject({
data: $_,
table: "persons",
$$$
})'
Length of output: 1270
Script:
#!/bin/bash
# Let's check the context around these assignments to understand if encryption is properly handled
rg -B 10 -A 10 "obj\.persons\s*=" src/services/worker/backupUtils.ts
# Also check for any encryption of persons data that might happen before the assignment
ast-grep --pattern 'const backupPersons = $$$'
Length of output: 2261
src/features/congregation/app_access/user_details/user_main_roles/useUserMainRoles.tsx
Outdated
Show resolved
Hide resolved
|
organized-app
|
||||||||||||||||||||||||||||
| Project |
organized-app
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 11s |
| Commit |
|
| Committer | Rasamoelina, Haja Onjatiana |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
1
|
| View all changes introduced in this branch ↗︎ | |
|
🎉 This PR is included in version 3.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
No description provided.