feat(app): update UI and fix bugs for credit hours and assignment restrictions #3055
Conversation
…alic font in tooltips
|
Task linked: CU-86c11k555 Add tooltips – Person page |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request modifies the Changes
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: 0
🧹 Outside diff range and nitpick comments (1)
src/features/persons/assignment_group/index.tsx (1)
25-28: Consider extracting string literals and improving readabilityWhile the logic is correct, there are a few potential improvements:
- Extract 'ministry' and 'applyFieldMinistryPart' as constants to avoid magic strings
- Consider using a positive condition for gender check for better readability (e.g.,
isMaleorisFemale)+const MINISTRY_ID = 'ministry'; +const FIELD_MINISTRY_PART_ID = 'applyFieldMinistryPart'; - const isMinistryDisabled: boolean = - id == 'ministry' && checkAssignmentDisabled(items[0].code); + const isMinistryDisabled: boolean = + id === MINISTRY_ID && checkAssignmentDisabled(items[0].code); - const isDisabledByGender: boolean = !male && id != 'applyFieldMinistryPart'; + const isFemale = !male; + const isDisabledByGender: boolean = isFemale && id !== FIELD_MINISTRY_PART_ID;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonsrc/locales/en/general.jsonis excluded by!**/*.jsonsrc/locales/en/ministry.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
src/features/persons/assignment_group/index.tsx(2 hunks)
🔇 Additional comments (5)
src/features/persons/assignment_group/index.tsx (5)
5-6: LGTM: Import statements are properly organized
The new imports follow the project's conventions and are appropriately placed.
23-24: LGTM: Translation hook is properly initialized
The translation hook is correctly set up for internationalizing the tooltip messages.
41-78: Consider accessibility improvements for tooltip
While the tooltip implementation is functional, consider adding ARIA attributes for better accessibility:
<Tooltip
followCursor
title={getTooltipTitle()}
show={isMinistryDisabled || isDisabledByGender}
+ aria-label={getTooltipTitle()}
+ role="tooltip"
>Also, verify that the tooltip is keyboard accessible when users navigate through the assignments.
30-38: Verify translation key availability
The tooltip messages use translation keys that need to be verified in the translation files.
✅ Verification successful
Translation keys are properly defined
Both translation keys tr_onlyAvailableForPioneers and tr_appliesOnlyToBrothers are correctly defined in the English locale files:
tr_appliesOnlyToBrothersinsrc/locales/en/general.jsontr_onlyAvailableForPioneersinsrc/locales/en/ministry.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the translation keys in the codebase
echo "Searching for translation keys..."
rg "tr_onlyAvailableForPioneers|tr_appliesOnlyToBrothers" --type json
Length of output: 325
Line range hint 1-81: Verify completeness of PR objectives
The PR description mentions a "Report credit hours" field with a tooltip for inactive publishers who are pioneers, but this feature is not visible in the current file. Please verify if this feature is implemented in another file or if it's missing from the implementation.
There was a problem hiding this comment.
Hi @FussuChalice, could you please move the added logic inside the hooks for this component? Thank you.
There was a problem hiding this comment.
Yes, of course. No problem.
There was a problem hiding this comment.
inside the hooks
What do you mean by this?
There was a problem hiding this comment.
@FussuChalice, I mean to move theme inside useAssignmentGroup.tsx file. Thanks.
There was a problem hiding this comment.
@FussuChalice, I mean to move theme inside useAssignmentGroup.tsx file. Thanks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/features/persons/assignment_group/useAssignmentGroup.tsx (2)
56-58: Improve function naming and documentationThe current name
isDisabledByGenderdoesn't fully convey that it specifically checks for non-male restrictions with a special case for field ministry.Consider renaming and adding documentation:
+ /** + * Checks if an assignment should be disabled for non-male persons + * @param id - The assignment ID + * @returns true if the assignment should be disabled (except for field ministry) + */ - const isDisabledByGender = (id: string) => { + const isRestrictedForNonMalePersons = (id: string) => { return !male && id !== 'applyFieldMinistryPart'; };
60-71: Enhance type safety and maintainabilityConsider extracting translation keys to constants and adding return type annotation.
+ const TRANSLATION_KEYS = { + PIONEER_ONLY: 'tr_onlyAvailableForPioneers', + BROTHERS_ONLY: 'tr_appliesOnlyToBrothers' + } as const; const getTooltipsForAssignmentTitles = ( id: string, items: { code: AssignmentCode }[] - ) => { + ): string => { if (isMinistryDisabled(id, items)) { - return t('tr_onlyAvailableForPioneers'); + return t(TRANSLATION_KEYS.PIONEER_ONLY); } if (isDisabledByGender(id)) { - return t('tr_appliesOnlyToBrothers'); + return t(TRANSLATION_KEYS.BROTHERS_ONLY); } return ''; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.json
📒 Files selected for processing (2)
src/features/persons/assignment_group/index.tsx(2 hunks)src/features/persons/assignment_group/useAssignmentGroup.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/persons/assignment_group/index.tsx
🔇 Additional comments (3)
src/features/persons/assignment_group/useAssignmentGroup.tsx (3)
2-2: LGTM: Translation setup is properly implemented
The translation hook is correctly imported and initialized.
Also applies to: 9-9
73-79: LGTM: Clean interface export
The return object properly exposes all necessary functions in a logical order.
Line range hint 1-79: Verify alignment with business requirements
The implementation introduces gender-based and ministry-based restrictions. Please ensure these align with the organization's policies and requirements.
✅ Verification successful
Let me gather more specific information about the business requirements by searching for related test files and implementation details.
Let me try one more search with corrected commands to find the business requirements.
Based on the extensive search results, I can now verify the business requirements. The translations and implementation across multiple languages consistently show that:
- Gender-based restrictions are a standard requirement, with specific assignments limited to males ("brothers") as shown by the tooltip message "tr_appliesOnlyToBrothers"
- Ministry-based restrictions are tied to publisher and pioneer roles, with certain assignments requiring pioneer status as shown by the tooltip "tr_onlyAvailableForPioneers"
- The restrictions are part of the core functionality across all supported languages and regions
- The implementation aligns with the form S-89 requirements and service committee guidelines found in the localization files
Let me verify one final aspect about the assignment codes:
Based on the extensive code analysis, I can now provide a definitive verification of the gender and ministry-based restrictions:
The implementation aligns with established business requirements
The code shows clear evidence that:
- Gender restrictions are properly implemented:
- Male-only assignments are explicitly defined with
maleOnly: trueflag - Brother-specific assignments are grouped in
BROTHER_ASSIGNMENTconstant - Female-accessible assignments are clearly defined for ministry roles like
MM_StartingConversation,MM_FollowingUp, etc.
- Ministry-based restrictions follow organizational policies:
- Pioneer-specific assignments require proper status checks (e.g.,
personIsFR) - Publisher status verification for ministry assignments
- Proper handling of ministry hours credit (
MINISTRY_HOURS_CREDIT) based on role
- The implementation is consistent across all supported languages and regions as shown in the localization files, with proper tooltips and error messages for restricted assignments.
The code structure and validation logic demonstrate a robust implementation that correctly enforces these organizational requirements while maintaining flexibility for authorized roles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files to verify business requirements
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | grep -i "assignment"
# Search for documentation of these restrictions
fd -e md . | xargs rg -i "gender|ministry|restriction"
Length of output: 1580
Script:
#!/bin/bash
# Search for test files specifically related to assignment groups and gender/ministry restrictions
ast-grep --pattern 'describe($_, () => { $$$ checkAssignmentDisabled $$$ })'
# Search for related implementation files to understand the business logic
rg -l "AssignmentCode" --type ts --type tsx
# Look for any comments or documentation in the implementation files
rg -i "gender|ministry|restriction" --type ts --type tsx -C 2
Length of output: 242
Script:
#!/bin/bash
# Search for files containing AssignmentCode
rg -l "AssignmentCode"
# Search for test files and implementation files related to assignments with context
rg -i "gender|ministry|restriction" -C 2
# Look for any enum or type definitions related to assignments
ast-grep --pattern 'enum AssignmentCode {
$$$
}'
Length of output: 113970
Script:
# Look for assignment code definitions
rg "AssignmentCode\." -B 2 -A 2
# Search for any configuration or validation files related to assignments
fd -e ts -e tsx -e json | rg -l "assignment.*config|assignment.*valid"
Length of output: 92954
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
src/locales/en/ministry.json
Outdated
| @@ -122,6 +122,7 @@ | |||
| "tr_pioneers": "Pioneers", | |||
| "tr_reminderPublisherReport": "Submit your monthly field service report", | |||
| "tr_reminderPublisherReportDesc": "It should be submitted to the secretary no later than the 6th day of the month.", | |||
| "tr_reminderBranchReport": "The Congregation Field Service and Meeting Attendance (S-1) report hasn’t been submitted yet", | |||
| "tr_reminderBranchReportDesc": "It should be submitted to the branch office no later than the 20th day of the month." | |||
| "tr_reminderBranchReport": "The Congregation Field Service and Meeting Attendance (S-1) report is not yet ready", | |||
There was a problem hiding this comment.
@FussuChalice: one last change please: could you update your local file in this line? It has been changed but it seems like yours is still the old one and this will overwrite the last update if not changed. Thanks.
|
organized-app
|
||||||||||||||||||||||||||||
| Project |
organized-app
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 17s |
| Commit |
|
| Committer | Max Makaluk |
| 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 2.130.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist: