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

nasaownsky · 2024-11-22T15:06:26Z

Description of the change

Added new agency definition section for court and supervision sectors

Type of change

All pull requests must have at least one of the following labels applied (otherwise the PR will fail):

Label	Description
Type: Bug	non-breaking change that fixes an issue
Type: Feature	non-breaking change that adds functionality
Type: Breaking Change	fix or feature that would cause existing functionality to not work as expected
Type: Non-breaking refactor	change addresses some tech debt item or prepares for a later change, but does not change functionality
Type: Configuration Change	adjusts configuration to achieve some end related to functionality, development, performance, or security
Type: Dependency Upgrade	upgrades a project dependency - these changes are not included in release notes

Related issues

closes #1540

Checklists

Development

This box MUST be checked by the submitter prior to merging:

Double- and triple-checked that there is no Personally Identifiable Information (PII) being mistakenly added in this pull request

These boxes should be checked by the submitter prior to merging:

Tests have been written to cover the code changed/added as part of this pull request

Code review

These boxes should be checked by reviewers prior to merging:

This pull request has a descriptive title and information useful to a reviewer
This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
Potential security implications or infrastructural changes have been considered, if relevant

nasaownsky · 2024-11-22T15:20:54Z

Hi @nichelle-hall! Please test this feature and tell me your thoughts about my changes!

Also, I have two questions:

In figma designs there is statement:

If the supervision sector is combined, then we can exclude this entire includes/excludes section from the Agency Settings page.

but in issue description there is another statement:

If we have the unlikely & unusual case where an agency has both Court, and Supervision sectors, then we should show the all of the relevant sections when they open the modal

Could you please explain to me if my understanding based on my implementation is right or do I need to change something? If the latter, could you maybe tell me in more details about it?

There is comment in figma designs about free text box, but from endpoint structure description it is not clear where it should be written.

Thank you in advance!

nichelle-hall · 2024-11-25T21:21:41Z

Hi @nichelle-hall! Please test this feature and tell me your thoughts about my changes!

Also, I have two questions:

In figma designs there is statement:

If the supervision sector is combined, then we can exclude this entire includes/excludes section from the Agency Settings page.

but in issue description there is another statement:

If we have the unlikely & unusual case where an agency has both Court, and Supervision sectors, then we should show the all of the relevant sections when they open the modal

Could you please explain to me if my understanding based on my implementation is right or do I need to change something? If the latter, could you maybe tell me in more details about it?

There is comment in figma designs about free text box, but from endpoint structure description it is not clear where it should be written.

Thank you in advance!

Hi Ilya!

Sorry it took me a few days to get back to you - I was out on Friday. We actually had a meeting today and it looks like CSG is still ironing out what they want from this feature. We're gonna wait until Mahmoud comes back to iron it out some more. He'll be back on Wednesday. Do you have other tickets to work on in the meantime?

nasaownsky · 2024-11-26T12:14:06Z

Oh, okay, I see!

Do you have other tickets to work on in the meantime?

Sure, I have plenty!

mxosman · 2024-11-27T22:28:04Z

Hi @nasaownsky! I'll do a deeper review of the code next week, but just wanted to answer your questions:

Hi @nichelle-hall! Please test this feature and tell me your thoughts about my changes!

Also, I have two questions:

In figma designs there is statement:

If the supervision sector is combined, then we can exclude this entire includes/excludes section from the Agency Settings page.

but in issue description there is another statement:

If we have the unlikely & unusual case where an agency has both Court, and Supervision sectors, then we should show the all of the relevant sections when they open the modal

Could you please explain to me if my understanding based on my implementation is right or do I need to change something? If the latter, could you maybe tell me in more details about it?

Sorry, this was a bad explanation on my part. I think you implemented it how the ticket was described, but if I could go back in time, I'd remove that note in the ticket, and clarify that they should be separate sections and separate modals - meaning, if it's a Supervision + Courts agency, they should see two separate sections on the Agency Settings page (one for Supervision Agency Definition and another for Courts Agency Definition). Sorry about the horrible confusion here - 100% my bad - I don't know why I combined them as one in my mind when writing that note. Does this clarification make sense?

There is comment in figma designs about free text box, but from endpoint structure description it is not clear where it should be written.

Ah, very good question! For these, you can use the following structure:

{ 
  key: "ADDITIONAL_CONTEXT",
  value: "",
}

So, we'll have to also allow the setting object to have an optional value field and update the AgencyDefinitionSetting type. The BE is saving all of this as a JSON blob, so there shouldn't be any issues sending it with this new field as long as the FE is consistent.

One more thing to clarify - the Supervision Agency Definition section should not appear if the agency does not have any subpopulations included.

So it should appear for this configuration (they have two subpopulations included - Parole and Probation):

And should NOT appear at all for this configuration:

Let me know if all of this helps clarify or if there are other areas I can help clear up! Thank you for your patience, @nasaownsky.

nasaownsky · 2024-12-02T13:10:00Z

Hi @mxosman, thank you for clarification info!

if it's a Supervision + Courts agency, they should see two separate sections on the Agency Settings page (one for Supervision Agency Definition and another for Courts Agency Definition)

That is hell of an engineering problem! I've tried to came up with the solution for this request, but after several trial and error I realized that this would be not as easy as I initially thought. The main issue is that we have this setting modal as a unified setting SECTOR_INCLUDES_EXCLUDES, and all of the actions for opening this modal and so on are happening under this unified name, that represents actual BE endpoint. So even if we split this modal into two separate modals, the edit button would be controlling one SECTOR_INCLUDES_EXCLUDES setting, which would be just the latest modal in FE. And even if create some sophisticated separation mechanism, we will just rewrite BE with single set of options (Court or Supervision) every time we will save our Court or Supervision setting in combined agencies. And so while I think it is possible to made some custom solution for this problem, it would be not very elegant and much time consuming.

So then I came up with another solution: why don't we just display single modal with combined options for combined agencies like this:

I tested this solution in multiple scenarios and it works just great! I tweaked it so that if it is combined agency and if the agency does not have any subpopulations included -- it will be just Court Agency Definition modal, and if it does have some subpopulations -- it will be Combined Agency Definition modal with setting options for courts and pretrial system and for respective Supervision systems.

Anyway, please check out my solution and tell me what you think about it! Thank you!

mxosman · 2024-12-02T18:02:54Z

Hi @mxosman, thank you for clarification info!

if it's a Supervision + Courts agency, they should see two separate sections on the Agency Settings page (one for Supervision Agency Definition and another for Courts Agency Definition)

That is hell of an engineering problem! I've tried to came up with the solution for this request, but after several trial and error I realized that this would be not as easy as I initially thought. The main issue is that we have this setting modal as a unified setting SECTOR_INCLUDES_EXCLUDES, and all of the actions for opening this modal and so on are happening under this unified name, that represents actual BE endpoint. So even if we split this modal into two separate modals, the edit button would be controlling one SECTOR_INCLUDES_EXCLUDES setting, which would be just the latest modal in FE. And even if create some sophisticated separation mechanism, we will just rewrite BE with single set of options (Court or Supervision) every time we will save our Court or Supervision setting in combined agencies. And so while I think it is possible to made some custom solution for this problem, it would be not very elegant and much time consuming.

Oh that's interesting - I actually thought it would be much easier this way. I do see that both settings are unified under the SECTOR_INCLUDES_EXCLUDES setting - but further in the value property - it's separated by sector. Wouldn't it be possible to achieve the separation and read/write from the SECTOR_INCLUDES_EXCLUDES > value > sector property?

So then I came up with another solution: why don't we just display single modal with combined options for combined agencies like this:

Ah, that was the goal of the original ticket write up - totally fine this solution if the above is not feasible - especially since this wouldn't be a common case (having both Court + Supervision sectors).

I'm still looking through the code - but, noticed when I was playing with it that:

it still shows for a Supervision-only agency
the "Other" text input field does not seem to persist consistently yet (maybe you're still working on this one)

LMK if I should hold off on reviewing based on this conversation!

nasaownsky · 2024-12-02T18:24:19Z

it still shows for a Supervision-only agency

You mean it shows up when the agency does not have any subpopulations included? It actually working correct, I think you just stumble upon bug with Supervision Populations setting section -- it does not updates UI correctly when switching agencies. Here, I made a loom demonstrating it.

https://www.loom.com/share/4c36b54f83514162834d15519a8fa2d9?sid=5863bcba-4691-465b-875c-02b72ee0d82c

the "Other" text input field does not seem to persist consistently yet (maybe you're still working on this one)

The field itself showing up for me correctly everytime... Do you mean it is not recording value to BE? That's because I wanted to choose our path forward -- with combined modal or not -- so I won't need to rewrite that functionality if we decide something else!

mxosman · 2024-12-02T17:45:25Z

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

+        >
+          <Modal
+            title={`${agencyTitle} Agency Definition`}
+            description={


Can the modal accept what's currently in the description property as its children instead of feeding this all in the description property?

It seems like the modal can accept children, and this used in Admin panel for agency and user provisioning modals, but for this exact purpose it lacks some styling. I think I was looking through code of all settings related components and made it with use of description for the sake of uniformity. To sum up: I think we either refactor all settings components to use it as children or keep it as description.

Gotcha! Thank you for clarifying - let's keep it as is!

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

mxosman · 2024-12-02T18:19:37Z

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

+    );
+  }, [currentAgencySettings]);
+
+  const isSettingConfigured = agencyDefinitionSetting.length > 0;


Will this still be a valid way to check this if the user checks and unchecks everything? I think there will still be fields here if the user unchecks everything.

For the purpose of this component this would be enough since I just need to know if BE is returning something, and if it's not -- configure default setting object with the correct structure.

mxosman · 2024-12-02T18:21:08Z

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

+  const isSettingConfigured = agencyDefinitionSetting.length > 0;
+
+  useEffect(() => {
+    setCurrentSystems([]);


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

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.

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

mxosman · 2024-12-02T18:22:20Z

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

+        ...((currentAgencySystems?.filter((system) =>
+          SupervisionSubsystems.includes(system)
+        ) as AgencySystemKeys[]) || []),


We could just have one variable that we set outside of this effect that gives us the list of subsystems and use that variable throughout.

This includes supervision systems only, and we using them only in this effect to set currentSystems, which could be combined.

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

mxosman · 2024-12-02T18:25:12Z

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

+        ([key, obj]) => {
+          return {
+            key,
+            included: defaultIncluded ?? obj.default,


Since we're getting the default settings, why are we also passing in a defaultIncluded override param?

When BE returns nothing we need to configure initial object, and since included is not an optional field we need to pass "NO" as initial value on the line 159. Maybe initialIncluded instead?

Ohh I see - thank you for explaining! Hmm - how about unconfiguredDefault?

mxosman · 2024-12-02T18:25:42Z

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

+    setUpdatedSetting(defaultSetting);
+    setDescriptionValue("");
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [agencyDefinitionSetting, currentSystems]);


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

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

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

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

mxosman · 2024-12-02T18:30:52Z

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

+                              if (sector.sector === system) {
+                                return {
+                                  ...sector,
+                                  settings: sector.settings.map((setting) =>
+                                    setting.key === key
+                                      ? {
+                                          ...setting,
+                                          included: boolToYesNoEnum(!checked),
+                                        }
+                                      : setting
+                                  ),
+                                };
+                              }
+                              return sector;
+                            });
+
+                            return updates;


Can we move this to a helper function just for readability that takes in the prevSettings and spits out the update?

mxosman · 2024-12-02T19:17:31Z

it still shows for a Supervision-only agency

You mean it shows up when the agency does not have any subpopulations included? It actually working correct, I think you just stumble upon bug with Supervision Populations setting section -- it does not updates UI correctly when switching agencies. Here, I made a loom demonstrating it.

https://www.loom.com/share/4c36b54f83514162834d15519a8fa2d9?sid=5863bcba-4691-465b-875c-02b72ee0d82c

Ahh - interesting... weird that it's not updating when switching over. Thank you for pointing out that it's an unrelated bug. Hopefully it's not too gnarly to fix - thankfully the underlying logic is working and it's just the UI not updating.

Side note: wIthin that same Supervision-only agency in your video - when I turn uncheck all of the subpopulations, it reverts to the Court Agency Definition instead of not showing that section at all.

the "Other" text input field does not seem to persist consistently yet (maybe you're still working on this one)

The field itself showing up for me correctly everytime... Do you mean it is not recording value to BE? That's because I wanted to choose our path forward -- with combined modal or not -- so I won't need to rewrite that functionality if we decide something else!

Sorry for not being clear - yes, I meant it doesn't save the value. Gotcha - please disregard then - my apologies as I thought that part was ready!

nasaownsky · 2024-12-04T14:01:53Z

@mxosman I resolved visual issues and added other fixes, please check my changes and thank you for your thoughtful comments and questions!

As for textbox implementation you suggested following structure:

{ 
  key: "ADDITIONAL_CONTEXT",
  value: "",
}

but our setting is an object that could have multiple sectors, and above structure belongs to only one sector. So, in case of multiple sectors in setting object -- should we write textbox input to each sector or only to first one?

mxosman · 2024-12-04T16:03:42Z

@mxosman I resolved visual issues and added other fixes, please check my changes and thank you for your thoughtful comments and questions!

Taking a look now! Thank you so much for your thoughtful responses and clarifications!

As for textbox implementation you suggested following structure:
{ 
  key: "ADDITIONAL_CONTEXT",
  value: "",
}
but our setting is an object that could have multiple sectors, and above structure belongs to only one sector. So, in case of multiple sectors in setting object -- should we write textbox input to each sector or only to first one?

Good question! There should be an additional context box for each sector. So if it's a Supervision (w/ enabled subpopulations) agency + Courts agency - there should be multiple context text boxes - one for each sector/sub-sector. If the agency is just a Courts agency (for example), then the additional context belongs just to the courts sector. If it's a supervision agency with Parole and Probation, then two additional context boxes - one for Parole and another for Probation.

Oh, and one quick thing - can we update the placeholder for these context boxes and replace "breakdown" with "agency"? So it reads:

If the listed categories do not adequately describe your agency, please describe additional data elements included in your agency’s definition.

Current placeholder for reference:

mxosman · 2024-12-04T16:05:40Z

I really appreciate you working through the complexity here - I know these new requests have been super challenging. Just know that all your efforts, creativity and patience are deeply appreciated. 🙏

mxosman · 2024-12-04T16:17:13Z

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

+    removeEditMode();
+  };
+
+  const handleCheckboxChange = (


Can you take another look at this? I'm unable to select anything in the checkboxes.

It looks like it's just the supervision agency options that are affected.

Fixed that!

mxosman · 2024-12-04T16:52:20Z

publisher/src/components/AgencySettings/AgencySettingsSupervisions.tsx

@@ -72,6 +72,10 @@ export const AgencySettingsSupervisions: React.FC<{
    userStore.isAgencyAdmin(agencyId) ||
    userStore.isJusticeCountsAdmin(agencyId);

+  useEffect(() => {
+    setSupervisionSystemsToSave(currentAgencySystems);
+  }, [currentAgencySystems]);


Great call on this!

nasaownsky · 2024-12-06T13:13:59Z

@mxosman Thank you for thoughtful comments and observations! This one is really tricky, but I think we are close to an end. I fixed major issue with selection that you pointed out and also added textbox functionality. Please, check my changes and let me know what you think about them. Thank you so much!

mxosman

Ah, this looks and feels great! Thank you for all of your work on this, @nasaownsky! This was all tricky indeed - hopefully we'll get a break soon from all the trickiness in these requests. Left a few tiny nits and question about what ended up fixing the checkbox issue - but otherwise looks great and is ready to move on to staging - the end for this is here! Thanks again!

mxosman · 2024-12-06T15:14:47Z

publisher/src/components/AgencySettings/AgencySettings.styles.tsx

@@ -444,6 +444,13 @@ export const SupervisionSystemRow = styled(AgencySettingsInfoRow)<{
  border: none;
 `;

+// Agency Defifnition


tiniest nit: typo in Definition

mxosman · 2024-12-06T15:16:03Z

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

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

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


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

Oh, this actually was resolved in this commit

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

mxosman · 2024-12-06T15:24:16Z

publisher/src/components/AgencySettings/AgencySettingsDefinition.tsx

+
+  const initialSetting = [
+    ...agencyDefinitionSetting,
+    ...getDefaultSetting(unconfiguredSystems, IncludesExcludesEnum.NO),


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

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

Gotcha! Great catch and sleuthing!

nasaownsky · 2024-12-06T15:48:17Z

Yay! Thank you so much for all your help on this @mxosman!

Add agency definition setting UI and logic

81c8e2d

nasaownsky added the Type: Feature label Nov 22, 2024

nasaownsky requested review from nichelle-hall, mxosman and lilidworkin November 22, 2024 15:06

Handle combined agency scenarios

a418227

mxosman reviewed Dec 2, 2024

View reviewed changes

nasaownsky added 3 commits December 3, 2024 16:17

Fix court setting appearing in supervision agencies

f40abce

Fix populations UI bug

0d91a80

Fixes

de7ffec

mxosman reviewed Dec 4, 2024

View reviewed changes

nasaownsky added 2 commits December 6, 2024 16:52

Add textbox functionality & fix checkbox select issue

90d11de

Fixes

36f5dc2

mxosman approved these changes Dec 6, 2024

View reviewed changes

Fix typo

93008cd

nasaownsky merged commit d197d62 into main Dec 6, 2024
7 checks passed

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

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

Conversation

nasaownsky commented Nov 22, 2024

Description of the change

Type of change

Related issues

Checklists

Development

Code review

nasaownsky commented Nov 22, 2024

nichelle-hall commented Nov 25, 2024

nasaownsky commented Nov 26, 2024

mxosman commented Nov 27, 2024 • edited Loading

nasaownsky commented Dec 2, 2024 • edited Loading

mxosman commented Dec 2, 2024 • edited Loading

nasaownsky commented Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxosman commented Dec 2, 2024

nasaownsky commented Dec 4, 2024 • edited Loading

mxosman commented Dec 4, 2024 • edited Loading

mxosman commented Dec 4, 2024

Choose a reason for hiding this comment

mxosman Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasaownsky commented Dec 6, 2024

mxosman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasaownsky Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasaownsky commented Dec 6, 2024

mxosman commented Nov 27, 2024 •

edited

Loading

nasaownsky commented Dec 2, 2024 •

edited

Loading

mxosman commented Dec 2, 2024 •

edited

Loading

nasaownsky commented Dec 2, 2024 •

edited

Loading

nasaownsky commented Dec 4, 2024 •

edited

Loading

mxosman commented Dec 4, 2024 •

edited

Loading

mxosman Dec 4, 2024 •

edited

Loading

nasaownsky Dec 6, 2024 •

edited

Loading