Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Publisher][Agency Settings] Add new agency definition section for court and supervision sectors #1594

Merged
merged 8 commits into from
Dec 6, 2024
Merged

[Publisher][Agency Settings] Add new agency definition section for court and supervision sectors #1594

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
81c8e2d
Add agency definition setting UI and logic
nasaownsky Nov 22, 2024
a418227
Handle combined agency scenarios
nasaownsky Dec 2, 2024
f40abce
Fix court setting appearing in supervision agencies
nasaownsky Dec 3, 2024
0d91a80
Fix populations UI bug
nasaownsky Dec 4, 2024
de7ffec
Fixes
nasaownsky Dec 4, 2024
90d11de
Add textbox functionality & fix checkbox select issue
nasaownsky Dec 6, 2024
36f5dc2
Fixes
nasaownsky Dec 6, 2024
93008cd
Fix typo
nasaownsky Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions publisher/src/components/AgencySettings/AgencySettings.styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,13 @@ export const SupervisionSystemRow = styled(AgencySettingsInfoRow)<{
border: none;
`;

// Agency Defifnition
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiniest nit: typo in Definition

export const DefinitionDescriptionInputWrapper = styled.div`
:not(:last-child) {
padding-bottom: 24px;
}
`;

// Data Source
export const DataSourceContainer = styled.div`
border-top: 1px solid ${palette.highlight.grey5};
Expand Down
81 changes: 58 additions & 23 deletions publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {
DataSourceContainer,
DataSourceQuestionWrapper,
DataSourceTitle,
DefinitionDescriptionInputWrapper,
EditButtonContainer,
} from "./AgencySettings.styles";
import { AgencySettingsEditModeModal } from "./AgencySettingsEditModeModal";
Expand All @@ -56,14 +57,14 @@ type AgencyDefinitionSetting = {
sector: AgencySystemKeys;
settings: {
key: string;
included: IncludedValue;
included?: IncludedValue;
value?: string;
}[];
};

const getDefaultSetting = (
systems: AgencySystemKeys[],
defaultIncluded?: IncludedValue
unconfiguredDefault?: IncludedValue
): AgencyDefinitionSetting[] => {
if (!systems.length) return [];

Expand All @@ -74,7 +75,7 @@ const getDefaultSetting = (
([key, obj]) => {
return {
key,
included: defaultIncluded ?? obj.default,
included: unconfiguredDefault ?? obj.default,
};
}
),
Expand Down Expand Up @@ -115,7 +116,6 @@ const AgencySettingsDefinition: React.FC<{

const [isConfirmModalOpen, setIsConfirmModalOpen] = useState(false);
const [currentSystems, setCurrentSystems] = useState<AgencySystemKeys[]>([]);
const [descriptionValue, setDescriptionValue] = useState("");

const agencyDefinitionSetting = useMemo(() => {
return (
Expand All @@ -125,8 +125,6 @@ const AgencySettingsDefinition: React.FC<{
);
}, [currentAgencySettings]);

const isSettingConfigured = agencyDefinitionSetting.length > 0;

useEffect(() => {
setCurrentSystems([]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to reset this every time the effect runs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, unfortunately we need, because when we select agency without applicable sectors we need to reset currentSystems, otherwise it will have previous values in it. And I can't do it in reset effect here, because it depends on currentSystems and it'll cause infinite loop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it - can we add a small comment here to clarify?


Expand All @@ -152,15 +150,22 @@ const AgencySettingsDefinition: React.FC<{
isCombinedAgency,
]);

const initialSetting = isSettingConfigured
? agencyDefinitionSetting
: getDefaultSetting(currentSystems, IncludesExcludesEnum.NO);
const configuredSystems = agencyDefinitionSetting.map(
(setting) => setting.sector
);
const unconfiguredSystems = currentSystems.filter(
(sector) => !configuredSystems.includes(sector)
);

const initialSetting = [
...agencyDefinitionSetting,
...getDefaultSetting(unconfiguredSystems, IncludesExcludesEnum.NO),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this what ended up fixing the checkbox issue (using the unconfiguredSystems here)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the problem was that once systems were written to BE, there was no way for unconfigured (or unwritten) systems to get into initialSetting

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Great catch and sleuthing!

];

const [updatedSetting, setUpdatedSetting] = useState(initialSetting);

useEffect(() => {
setUpdatedSetting(initialSetting);
setDescriptionValue("");
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [agencyDefinitionSetting, currentSystems]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify the purpose of this effect? We already initialize it with defaultSetting and update it via the component, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reset effect used for resetting setting for UI to work properly. The absence of this effect causes visual bug with incorrect working UI when switching agencies like here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying. I'd add a small comment here too for clarity.


Expand Down Expand Up @@ -215,6 +220,28 @@ const AgencySettingsDefinition: React.FC<{
});
};

const handleDescriptionChange = (system: AgencySystemKeys, value: string) => {
setUpdatedSetting((prevSettings) => {
return prevSettings.map((sector) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nit: setting instead of sector for the mapped param.

Copy link
Collaborator Author

@nasaownsky nasaownsky Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this actually was resolved in this commit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry - I was looking at the wrong commit! Thank you so much!

if (sector.sector === system) {
return {
...sector,
settings: [
...sector.settings.filter(
(setting) => setting.key !== "ADDITIONAL_CONTEXT"
), // Ensure no duplicate keys
{
key: "ADDITIONAL_CONTEXT",
value,
},
],
};
}
return sector;
});
});
};

if (!currentSystems.length) return null;

return (
Expand Down Expand Up @@ -243,6 +270,12 @@ const AgencySettingsDefinition: React.FC<{
</DataSourceQuestionWrapper>

{currentSystems?.map((system) => {
const defaultDescription = initialSetting
.find((setting) => setting.sector === system)
?.settings.find(
(setting) => setting.key === "ADDITIONAL_CONTEXT"
)?.value;

return (
<React.Fragment key={system}>
{isSupervisionAgencyWithEnabledSubpopulations && (
Expand Down Expand Up @@ -272,21 +305,23 @@ const AgencySettingsDefinition: React.FC<{
handleCheckboxChange(key, checked, system)
}
/>
<DefinitionDescriptionInputWrapper>
<NewInput
name={`${system.toLocaleLowerCase()}_description`}
id={`${system.toLocaleLowerCase()}_description`}
type="text"
multiline
placeholder="If the listed categories do not adequately describe your agency, please describe additional data elements included in your agency’s definition."
defaultValue={defaultDescription}
onChange={(e) =>
handleDescriptionChange(system, e.target.value)
}
fullWidth
/>
</DefinitionDescriptionInputWrapper>
</React.Fragment>
);
})}
<NewInput
name="agency_definition_description"
id="agency_definition_description"
type="text"
multiline
placeholder="If the listed categories do not adequately describe your breakdown, please describe additional data elements included in your agency’s definition."
value={descriptionValue}
onChange={(e) => {
setDescriptionValue(e.target.value);
}}
fullWidth
/>
</DataSourceContainer>
}
buttons={[
Expand All @@ -309,7 +344,7 @@ const AgencySettingsDefinition: React.FC<{
)}

<AgencySettingsBlock withBorder id="agency_definition">
<BasicInfoBlockTitle configured={isSettingConfigured}>
<BasicInfoBlockTitle>
{agencyTitle} Agency Definition
{allowEdit && (
<EditButtonContainer>
Expand Down