[ENG-580] feat: allow orgs to be cleared in Role org selector#16464
[ENG-580] feat: allow orgs to be cleared in Role org selector#16464Jacobjeevan wants to merge 2 commits into
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Walkthrough
ChangesTagConfig nullable org fields and organization selector fixes
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR enables organizations to be explicitly cleared in the role org selector used for tag configuration, by changing the
Confidence Score: 4/5Safe to merge with awareness of the outstanding issues in RoleOrgSelector flagged in prior review rounds. The core null/undefined and payload changes are correct and well-scoped. The duplicate-detection fix was applied to FacilityOrganizationSelector but the matching includes-vs-find inconsistency in RoleOrgSelector.handleConfirmSelection (line 86) remains unaddressed, and the stale-initial-state pattern is still present. The org-clearing and archive-preservation logic itself is sound. src/components/Common/RoleOrgSelector.tsx — reference-equality duplicate check and stale initial state from previous review rounds are still unresolved. Important Files Changed
Reviews (2): Last reviewed commit: "Merge branch 'develop' into ENG-580-tag-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const [selectedOrganizations, setSelectedOrganizations] = useState< | ||
| Organization[] | ||
| >([]); | ||
| >(currentOrganizations || []); |
There was a problem hiding this comment.
useState only consumes its initializer on the very first render. In TagConfigForm, existingConfig is fetched asynchronously, so currentOrganizations is [] at the time RoleOrgSelector mounts. By the time the query resolves and currentOrganizations changes to [existingConfig.organization], selectedOrganizations is already frozen at [].
FacilityOrganizationSelector already solves this with a dedicated useEffect (lines 292–305) that calls setSelectedOrganizations(resolvedOrganizations) whenever value or currentOrganizations changes. RoleOrgSelector needs the same pattern; without it, the organization selector appears empty when opening the edit drawer for an existing tag config, even though the underlying form value is correctly set.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/Admin/TagConfig/TagConfigForm.tsx (2)
363-363:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInternationalize user-facing label.
The hardcoded string "Facility Organisation" violates the project guideline requiring all user-facing strings to use i18next for multi-language support in healthcare interfaces.
🌍 Proposed fix
- <FormLabel>Facility Organisation</FormLabel> + <FormLabel>{t("facility_organisation")}</FormLabel>Then add to
public/locale/en.json:"facility_organisation": "Facility Organisation"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Admin/TagConfig/TagConfigForm.tsx` at line 363, Replace the hardcoded string "Facility Organisation" in the FormLabel component with an i18next translation key. Import and use the useTranslation hook (or appropriate i18n translation function for the component context) to access translations, then replace the label text with a call to the translation function using the key "facility_organisation". Finally, add the corresponding translation entry to public/locale/en.json with the value "Facility Organisation" to complete the internationalization setup.Source: Coding guidelines
390-390:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInternationalize user-facing label.
The hardcoded string "Organization" violates the project guideline requiring all user-facing strings to use i18next for multi-language support in healthcare interfaces.
🌍 Proposed fix
- <FormLabel>Organization</FormLabel> + <FormLabel>{t("organization")}</FormLabel>Then add to
public/locale/en.json:"organization": "Organization"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Admin/TagConfig/TagConfigForm.tsx` at line 390, The FormLabel component in TagConfigForm contains a hardcoded string "Organization" that violates the internationalization guideline. Replace the hardcoded "Organization" string with an i18next translation key using the useTranslation hook (ensure it is already imported). Add the corresponding translation entry for the key to the public/locale/en.json file to maintain the English language default. This ensures the label is properly internationalized for multi-language support in healthcare interfaces.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Common/RoleOrgSelector.tsx`:
- Around line 45-47: The selectedOrganizations state in RoleOrgSelector is
initialized with currentOrganizations only once at component mount, so it
becomes stale when currentOrganizations or value props change after the initial
render (such as when async data loads). Add a separate useEffect hook that
watches for changes to currentOrganizations and value props and updates
selectedOrganizations state accordingly. This sync effect should be distinct
from the existing useEffect at line 114 that handles auto-selection, with
dependencies on [value, currentOrganizations] to ensure the state stays
synchronized whenever these props change.
In `@src/types/emr/tagConfig/tagConfig.ts`:
- Around line 78-80: The tag config type definition now requires `organization`
and `facility_organization` to always be present in API payloads as nullable
fields (rather than optional). Verify that the backend endpoints POST
/api/v1/tag_config/ and PUT /api/v1/tag_config/{external_id}/ correctly accept
and handle null values for both the organization and facility_organization
fields when they are sent in request payloads. Update backend validation logic
if needed to allow null values for these required fields instead of treating
them as missing fields.
---
Outside diff comments:
In `@src/pages/Admin/TagConfig/TagConfigForm.tsx`:
- Line 363: Replace the hardcoded string "Facility Organisation" in the
FormLabel component with an i18next translation key. Import and use the
useTranslation hook (or appropriate i18n translation function for the component
context) to access translations, then replace the label text with a call to the
translation function using the key "facility_organisation". Finally, add the
corresponding translation entry to public/locale/en.json with the value
"Facility Organisation" to complete the internationalization setup.
- Line 390: The FormLabel component in TagConfigForm contains a hardcoded string
"Organization" that violates the internationalization guideline. Replace the
hardcoded "Organization" string with an i18next translation key using the
useTranslation hook (ensure it is already imported). Add the corresponding
translation entry for the key to the public/locale/en.json file to maintain the
English language default. This ensures the label is properly internationalized
for multi-language support in healthcare interfaces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab92e8f4-17b8-4393-9e16-b8f34d410b03
📒 Files selected for processing (5)
src/components/Common/RoleOrgSelector.tsxsrc/pages/Admin/TagConfig/TagConfigForm.tsxsrc/pages/Admin/TagConfig/TagConfigView.tsxsrc/pages/Facility/settings/organizations/components/FacilityOrganizationSelector.tsxsrc/types/emr/tagConfig/tagConfig.ts
| const [selectedOrganizations, setSelectedOrganizations] = useState< | ||
| Organization[] | ||
| >([]); | ||
| >(currentOrganizations || []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether selectedOrganizations is synchronized after mount.
rg -n -C3 'useState<\s*Organization\[\]\s*>\(|setSelectedOrganizations\(|useEffect\(' src/components/Common/RoleOrgSelector.tsx
rg -n -C3 'RoleOrgSelector|currentOrganizations|value=' src/pages/Admin/TagConfig/TagConfigForm.tsxRepository: ohcnetwork/care_fe
Length of output: 4171
🏁 Script executed:
cat -n src/components/Common/RoleOrgSelector.tsxRepository: ohcnetwork/care_fe
Length of output: 14712
Line 45–47: useState(currentOrganizations || []) doesn't sync when async props update post-mount.
The initial state from currentOrganizations is used only on first render. When currentOrganizations or value changes after mount (e.g., edit form data loading), selectedOrganizations becomes stale. The existing useEffect at line 114 only handles auto-selection; it doesn't synchronize state with prop changes. Add a sync effect keyed to value and currentOrganizations:
Suggested fix
const [selectedOrganizations, setSelectedOrganizations] = useState<
Organization[]
>(currentOrganizations || []);
+
+ useEffect(() => {
+ if (value && value.length > 0) {
+ const resolvedOrganizations = value
+ .map((id) => currentOrganizations?.find((org) => org.id === id))
+ .filter((org): org is Organization => Boolean(org));
+ setSelectedOrganizations(resolvedOrganizations);
+ return;
+ }
+ setSelectedOrganizations(currentOrganizations || []);
+ }, [value, currentOrganizations]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Common/RoleOrgSelector.tsx` around lines 45 - 47, The
selectedOrganizations state in RoleOrgSelector is initialized with
currentOrganizations only once at component mount, so it becomes stale when
currentOrganizations or value props change after the initial render (such as
when async data loads). Add a separate useEffect hook that watches for changes
to currentOrganizations and value props and updates selectedOrganizations state
accordingly. This sync effect should be distinct from the existing useEffect at
line 114 that handles auto-selection, with dependencies on [value,
currentOrganizations] to ensure the state stays synchronized whenever these
props change.
| organization: string | null; | ||
| facility?: string; | ||
| facility_organization?: string; | ||
| facility_organization: string | null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for backend TagConfig serializer/model validation that may need to handle the new required-nullable contract
# Search for TagConfig model/serializer definitions
rg -n -C5 -i 'class.*tagconfig.*(serializer|model|viewset)' --type py
# Search for organization field validation
rg -n -C3 'organization.*field|facility_organization.*field' --type py -g '*tag*config*'Repository: ohcnetwork/care_fe
Length of output: 211
🏁 Script executed:
# First, locate and examine the tagConfig.ts file
fd -n "tagConfig.ts" -t fRepository: ohcnetwork/care_fe
Length of output: 293
🏁 Script executed:
# Check the context around lines 78-80 in the tagConfig file
if [ -f "src/types/emr/tagConfig/tagConfig.ts" ]; then
cat -n "src/types/emr/tagConfig/tagConfig.ts" | sed -n '70,90p'
fiRepository: ohcnetwork/care_fe
Length of output: 753
🏁 Script executed:
# Search for TagConfigRequest usage in mutations and queries
rg -n "TagConfigRequest" -t ts -t tsx -C 3Repository: ohcnetwork/care_fe
Length of output: 90
🏁 Script executed:
# Look for API endpoint definitions for tag config
rg -n "tag_config" -t ts -t tsx -A 2 -B 2 -g "*Api.ts"Repository: ohcnetwork/care_fe
Length of output: 90
🏁 Script executed:
# Check git history for changes to this interface
git log -p --follow -S "organization: string | null" -- "src/types/emr/tagConfig/tagConfig.ts" | head -100Repository: ohcnetwork/care_fe
Length of output: 44
🏁 Script executed:
# Search for TagConfigRequest usage in mutations and queries
rg "TagConfigRequest" -t ts -A 3 -B 3Repository: ohcnetwork/care_fe
Length of output: 2651
🏁 Script executed:
# Look for form usage or mutation definitions related to tag config
rg -i "tagconfig" -t ts -l | head -20Repository: ohcnetwork/care_fe
Length of output: 1059
🏁 Script executed:
# Check API route definitions for tag config
rg "tag_config" -t ts --max-count 20Repository: ohcnetwork/care_fe
Length of output: 3707
🏁 Script executed:
# View TagConfigForm to see how the payload is constructed, particularly organization and facility_organization fields
cat -n "src/pages/Admin/TagConfig/TagConfigForm.tsx" | head -150Repository: ohcnetwork/care_fe
Length of output: 5816
🏁 Script executed:
# See more of the TagConfigForm to understand the form structure and how organization/facility_organization are handled
cat -n "src/pages/Admin/TagConfig/TagConfigForm.tsx" | sed -n '1,250p'Repository: ohcnetwork/care_fe
Length of output: 9717
🏁 Script executed:
# Continue viewing the form to see organization and facility_organization field rendering
cat -n "src/pages/Admin/TagConfig/TagConfigForm.tsx" | sed -n '250,400p'Repository: ohcnetwork/care_fe
Length of output: 6074
🏁 Script executed:
# Check the git history to see if organization/facility_organization were changed from optional to required-nullable
git log --oneline -20 -- "src/types/emr/tagConfig/tagConfig.ts"Repository: ohcnetwork/care_fe
Length of output: 134
🏁 Script executed:
# Check the specific change in the git commit to see what changed from previous state
git show f826b8cf -- "src/types/emr/tagConfig/tagConfig.ts" | head -100Repository: ohcnetwork/care_fe
Length of output: 2738
🏁 Script executed:
# Also check the full diff of that commit
git show f826b8cf --statRepository: ohcnetwork/care_fe
Length of output: 50374
🏁 Script executed:
# Let's verify if this file existed before this commit or if it's truly new
git log --all --full-history -- "src/types/emr/tagConfig/tagConfig.ts" | head -50Repository: ohcnetwork/care_fe
Length of output: 1751
🏁 Script executed:
# Check the parent commit to see if the file existed before
git show f826b8cf^:src/types/emr/tagConfig/tagConfig.ts 2>&1 | head -20Repository: ohcnetwork/care_fe
Length of output: 102
🏁 Script executed:
# Check if this is truly a new file or if it was modified
git log --diff-filter=A -- "src/types/emr/tagConfig/tagConfig.ts"Repository: ohcnetwork/care_fe
Length of output: 260
🏁 Script executed:
# Let's look at the parent commit to see what existed before
git show 01dad4c040aca11befba547f2e4a0a9ebffbaa41:src/types/emr/tagConfig/tagConfig.ts 2>&1 | head -100Repository: ohcnetwork/care_fe
Length of output: 2478
🏁 Script executed:
# Let's verify the form data handling one more time to ensure null serialization is correct
cat -n "src/pages/Admin/TagConfig/TagConfigForm.tsx" | sed -n '177,196p'Repository: ohcnetwork/care_fe
Length of output: 807
Verify backend API compatibility with required-nullable organization fields.
This change alters the API contract: organization and facility_organization are now required fields that can be null, rather than optional fields. Previously, these fields could be omitted entirely from the request payload; now they must always be present (set to null when cleared).
The form data construction confirms the payload will always include both fields:
- Default values are explicitly set to
null(lines 102-103) - Zod schema requires both fields as nullable strings (lines 77-78)
- The mutation payload includes both fields unconditionally (lines 187-188)
Ensure the backend endpoints (POST /api/v1/tag_config/ and PUT /api/v1/tag_config/{external_id}/) correctly accept and handle null values for both fields in request payloads.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types/emr/tagConfig/tagConfig.ts` around lines 78 - 80, The tag config
type definition now requires `organization` and `facility_organization` to
always be present in API payloads as nullable fields (rather than optional).
Verify that the backend endpoints POST /api/v1/tag_config/ and PUT
/api/v1/tag_config/{external_id}/ correctly accept and handle null values for
both the organization and facility_organization fields when they are sent in
request payloads. Update backend validation logic if needed to allow null values
for these required fields instead of treating them as missing fields.
There was a problem hiding this comment.
Pull request overview
Enables clearing the organization selection when managing Tag Configs (admin and facility contexts) by moving org fields to explicit null and ensuring selectors can represent “no org selected”.
Changes:
- Updated
TagConfigRequestand form validation to treatorganization/facility_organizationas nullable and submitnullto clear values. - Adjusted Tag Config archiving payload to include nullable org IDs.
- Improved selector behavior by de-duping selections by
id(facility org selector) and attempting to initialize role org selection from provided orgs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/types/emr/tagConfig/tagConfig.ts |
Makes org fields nullable (and required) in the request type to support explicit clearing. |
src/pages/Facility/settings/organizations/components/FacilityOrganizationSelector.tsx |
Prevents duplicate selections by comparing org IDs; trims an unused effect dependency. |
src/pages/Admin/TagConfig/TagConfigView.tsx |
Ensures archive updates include nullable org IDs in the update payload. |
src/pages/Admin/TagConfig/TagConfigForm.tsx |
Updates zod schema/defaults and payload construction to allow and submit null org values. |
src/components/Common/RoleOrgSelector.tsx |
Attempts to hydrate initial selection from currentOrganizations (but still needs sync logic to work reliably). |
🎭 Playwright Test ResultsStatus: ✅ Passed
📊 Detailed results are available in the playwright-final-report artifact. Run: #9631 |
Deploying care-preview with
|
| Latest commit: |
ded55b9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://44c1ac9b.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-580-tag-managing-org.care-preview-a7w.pages.dev |
ohc-project-manager
Bot
added
Changes Required
and removed
needs review
needs testing
labels
Proposed Changes
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit