885: fix, adds autocomplete operator edit in AgentOrganisationDetails - #912
885: fix, adds autocomplete operator edit in AgentOrganisationDetails#912DarrellRoberts wants to merge 13 commits into
Conversation
|
|
||
| export type StatusValue = AgentEngagementStatusType | AgentVolunteerSearchType | AgentTrustType; | ||
|
|
||
| // @ts-expect-error - TODO: Add INCONTACT and TRIED_TO_CONTACT types |
|
|
||
| type IconComponent = React.ComponentType<{ size?: number; color?: string }>; | ||
|
|
||
| // @ts-expect-error - TODO: Add INCONTACT and TRIED_TO_CONTACT types |
| [AgentEngagementStatus.INACTIVE]: t("dashboard.agentProfile.status.engagement.inactive"), | ||
| }); | ||
| export const createEngagementStatusLabelMap = (t: TFunction): Record<AgentEngagementStatus, string> => | ||
| // @ts-expect-error TODO - Add INCONTACT and TRIED_TO_CONTACT types |
ReviewOverview: adds a new Potential regression: strict operator validation could block unrelated edits on existing agentsoperator: z
.string()
.min(1, required)
.refine((val) => validOperators.includes(val), {
message: t(`${i18nPrefix}.operatorInvalid`),
}),This requires the operator to exactly match an entry in the current That's compounded by this change: - operator: details?.operator || agent.operator || "",
+ operator: details?.operator,This drops both the UX: autocomplete updates the real value on every keystroke, not just on selectiononChange={(e) => {
const v = e.target.value as T;
setLocalValue(v);
setValue(v); // fires react-hook-form's onChange immediately
setOpen(true);
}}Since Scope: two unrelated
|
|
@DarrellRoberts does this bot review make sense? |
…eed4deed-org/fe into darrell/fix/operator-agent-org-details
|
thanks @nadavosa , I've pushed my changes. @need4deed in response to the bot comments: Exact match
This is intentional. It's so a user is forced to submit a confirmed Operator value rather than a random string like "aus" as opposed to
Error showing whilst user typing
I added a bit more UX along with a hint so that if the value is less than 3 characters, we flag it (otherwise the list won't show), and then if it's 3 or more characters but the user hasn't selected from the list, it will still show an error.
Uncontrolled component
This is a fair point and have reverted the change
Another fair point, have made check lowercase
The updated sdk causes linting errors. I'm actually working in parallel on this issue hence why I commented above. |
Follow-upThanks for the detailed responses — the exact-match and typing-UX decisions make sense given the "aus" vs "ausbildung-ota.de" example, and the uncontrolled-component fix looks correct now. One thing still not quite right in the case-insensitivity fix: return operatorOptions.filter((op) => op.includes(value.toLowerCase()));Only Saw your note about #923 for the |
arturasmckwcz
left a comment
There was a problem hiding this comment.
Automated review findings (4 confirmed issues).
| operator: z | ||
| .string() | ||
| .min(3, t(`${i18nPrefix}.operatorRequired`)) | ||
| .refine((val) => validOperators.includes(val), { |
There was a problem hiding this comment.
Correctness: Operator field validation requires an exact match against the currently-fetched organization title list, but this gates the Save button for the entire Organisation Details form, not just the operator field.
An existing agent's stored operator (free-text pre-PR, per SDK operator: string) isn't guaranteed to be an exact-case match to any current organization title (renamed org, trailing whitespace, or never drawn from this list). isValid becomes false for the whole shared zod schema, disabling Save even for unrelated edits (e.g. about). This also triggers transiently on every load, since organizations defaults to [] while useGetOrganization() is still fetching, so validOperators is empty and any operator value fails until the request resolves.
There was a problem hiding this comment.
do we currently have NGOs in production with stored operators which don't fall into the list? If so, wouldn't we want to phase this out? Or is the autocomplete list just a suggestion?
this could be a confusion on my part
| const displayOperators = useCallback( | ||
| (value: string) => { | ||
| if (value?.length >= 3) { | ||
| return operatorOptions.filter((op) => op.includes(value.toLowerCase())); |
There was a problem hiding this comment.
Correctness: The operator autocomplete filter lowercases the typed search term but not the option being searched, so it never matches capitalized organization names.
operatorOptions.filter((op) => op.includes(value.toLowerCase())) — organization titles like "Diakonie Deutschland" are capitalized; typing "dia" produces value.toLowerCase() === "dia", and "Diakonie Deutschland".includes("dia") is false because of the capital D. The autocomplete dropdown never shows matches for realistic (capitalized) organization names, making the PR's core feature effectively non-functional.
There was a problem hiding this comment.
good point and relates to @nadavosa comment. I've added an extra toLowerCase() for the operator (or op).
|
|
||
| {type === "autocomplete" && ( | ||
| <DropdownWrapper ref={wrapperRef}> | ||
| <DropdownButton $hasError={!!errorMessage} onClick={() => setOpen((o) => !o)}> |
There was a problem hiding this comment.
Correctness: The autocomplete's text input and clear button are nested inside the dropdown-toggle button's onClick handler with no stopPropagation, so clicking into the input while the list is open closes it.
DropdownButton has onClick={() => setOpen((o) => !o)} and wraps the <input>/<ClearButton>; clicks bubble. With the dropdown open after typing 3+ characters, a plain click into the input to reposition the cursor (not a keystroke) bubbles up and toggles open to false, hiding the list unexpectedly while the input stays focused.
There was a problem hiding this comment.
good point and I found it was easier to just always set the setOpen to true whenever the user clicks on the input field. This means when the user clicks outside of the input, it closes the autocomplete but whenever they click on the input, e.g. when inserting a letter anywhere within the value, it opens the autocomplete.
Or in other words, the autocomplete will always be open so long as there are matches & the user-focus is on the text-input
| </DropdownWrapper> | ||
| )} | ||
|
|
||
| {type === "autocomplete" && ( |
There was a problem hiding this comment.
Simplification: This new autocomplete block duplicates the existing checkbox-list/radio-list dropdown structure (DropdownWrapper/DropdownButton/DropdownList/OptionRow) almost line-for-line instead of reusing it.
~55 lines at 506-561 and ~55 lines at 563-620 both implement the same wrapper/button/list/option-row pattern, differing only in the button's inner content and single- vs multi-select. Any future change to dropdown behavior (styling, keyboard nav, outside-click handling) now has to be made twice and can drift out of sync.
There was a problem hiding this comment.
originally I wanted to reuse the checkbox-list/radio-list but I found it was easier to create a new type, as otherwise you are complicating this checkbox/radio-lists with extra props and conditions (also risks accidentally mutating their existing behaviour).
Also now they have different behaviours and slightly different appearences (now no chevron arrow for autocomplete as it's no longer needed)
I think also if there is a future change to the checkbox-list/radio-list it shouldn't neccessarily mean that the same change should apply to the autocomplete?
…eed4deed-org/fe into darrell/fix/operator-agent-org-details
|
thanks @nadavosa and @arturasmckwcz , I've implemented some changes.
@arturasmckwcz for your points I've replied directly |
arturasmckwcz
left a comment
There was a problem hiding this comment.
Re-review findings (automated).
| // back to an id at submit time via AgentType/Service option mappings. | ||
| organizationType: z.string().min(1, required), | ||
| operator: z.string().min(1, required), | ||
| operator: z |
There was a problem hiding this comment.
Correctness: The operator field's Zod validation requires an exact match against validOperators, sourced from a separately-cached useGetOrganization() query, while the pre-filled operator value comes from the independently-cached agent query — the two caches can diverge.
Failure scenario: The organizations list is fetched once (5 min staleTime) per tab. Another coordinator renames the linked organization. The agent profile then loads a fresh operator reflecting the new title, but the stale cached organizations list still has the old title, so validOperators.includes(operator) is false, isValid stays false, and the whole Organisation Details save button is blocked — even for edits to unrelated fields — with only a cryptic error under the untouched operator field.
| </DropdownWrapper> | ||
| )} | ||
|
|
||
| {type === "autocomplete" && ( |
There was a problem hiding this comment.
Simplification: The new type === "autocomplete" branch fully duplicates the DropdownWrapper/DropdownButton/DropdownList/OptionRow scaffold already used by the checkbox-list/radio-list branch instead of extending it, and has already silently diverged from it in two ways (see follow-up comment below).
Failure scenario: radio-list's button toggles open/closed, autocomplete's only opens and never closes on click; radio-list's list renders on open alone while autocomplete additionally gates on options.length > 0. Any future fix to shared dropdown behavior (keyboard nav, a11y, styling) has to be applied to both blocks or they drift further, as already happened here.
| <DropdownButton | ||
| $hasError={!!errorMessage} | ||
| onClick={() => { | ||
| if (value) setOpen(true); |
There was a problem hiding this comment.
Correctness: DropdownButton's onClick reads the external value prop instead of localValue, and the adjacent ClearButton doesn't call e.stopPropagation(), so clicking Clear also bubbles into the still-truthy value closure and forces open back to true right after clearing.
Failure scenario: User clicks the X to clear the operator field: ClearButton's handler sets value to "", then the same click event bubbles to DropdownButton, whose closure still has the pre-clear (truthy) value, so setOpen(true) fires. Currently masked because options.length > 0 also gates the list and becomes 0 once cleared, but it leaves open stuck true — a latent bug that surfaces if empty-value gating on options is ever relaxed.
| import { useGetQuery } from "./useGetQuery"; | ||
| import { apiPathOrganization, cacheTTL } from "@/config/constants"; | ||
|
|
||
| export const useGetOrganization = () => { |
There was a problem hiding this comment.
Efficiency: useGetOrganization omits addLang: false, unlike every other comparable hook fetching non-translated reference data, so the UI language becomes part of the request and the react-query cache key for data that doesn't depend on it.
Failure scenario: Backend GET /organization/ never reads request.query.language, and organization titles are explicitly documented as untranslated. Toggling UI language between en/de causes react-query to treat the two languages as separate cache entries, triggering a redundant refetch and duplicate cached copies of identical data.
| }} | ||
| > | ||
| <InputWrapper> | ||
| <input |
There was a problem hiding this comment.
Correctness: The new autocomplete <input> wires neither onBlur nor onKeyDown, unlike every other edit-mode input type (text/textarea/number) in this shared component, which all support submit-on-blur and Enter-to-submit.
Failure scenario: Dormant today since no current EditableField caller passes a submit prop with type="autocomplete", but a future caller adding it following the existing text-input pattern will find blur-to-save and Enter-to-save silently do nothing, and Enter won't even preventDefault() if the field is ever placed inside a real <form>.



Description
On edit for
agentprofile forOrganisationDetailsthis allows a new autocompleteEditableFieldto fieldoperator.Related Issues
Closes #885
Changes
EditableFieldtype:autocompleteScreenshots / Demos
Aufzeichnung.2026-08-07.232740.mp4
Checklist