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.

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-wide Courts + Supervision Subpopulation TIG elements #1541

Merged
merged 5 commits into from
Oct 11, 2024

Conversation

mxosman
Copy link
Contributor

@mxosman mxosman commented Oct 8, 2024

Description of the change

Add new agency-wide Courts + Supervision Subpopulation TIG elements in preparation for the implementation of this feature outlined in #1540.

Reference TIG:

Also, I changed the folder name from Settings -> AgencySettings for more clarity (and b/c it was bugging me a lil' bit).

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 #1538

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

@mxosman mxosman force-pushed the mahmoud/add-new-court-supervision-tig-elements branch from 2184fbd to c6c4cb9 Compare October 8, 2024 17:44
@morden35
Copy link
Contributor

morden35 commented Oct 9, 2024

Looks good Mahmoud! Obviously this feature is still in progress, but is there any way to test the structure of this file? Also, is there any way we can add some sort of parity test between the two repos?

@mxosman
Copy link
Contributor Author

mxosman commented Oct 9, 2024

Looks good Mahmoud! Obviously this feature is still in progress, but is there any way to test the structure of this file?

Ah, hmm... I could strongly type the constants in this file and that will enforce each one's structure is consistent - which is a great call - but, let me know if I'm misunderstanding what you mean by testing the structure of this file or if you were thinking of a test for the entire file itself? Here's the strongly typed version: 432806c

Also, is there any way we can add some sort of parity test between the two repos?

There isn't a way that I can think of off the top of my head without having some sort of linkage between the two repos - which I'm not entirely sure how we would do without publishing one as a yarn or npm package and importing it in the other repo. LMK if you have any ideas/suggestions!

Comment on lines 63 to 67
PROBATION_IN_LIEU_INCARCERATION: {
label:
"People sentenced to a period of probation in lieu of incarceration (including to electronic monitoring, home confinement, traditional supervision, etc.)",
default: IncludesExcludesEnum.YES,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you link to where I can find these definitions on the JC websites?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find it on the link that is in the PR description.

Copy link
Contributor Author

@mxosman mxosman Oct 9, 2024

Choose a reason for hiding this comment

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

Oh strange - the link in the PR description takes me right to the supervision elements - I forgot to add the courts, which should be updated now.

Here is the a link to the file that Matt posted in Slack which includes both TIG elements along with the respective links to the JC websites that I used as reference: https://recidiviz.slack.com/archives/C04JL8A2ADB/p1727808639123169

Courts: https://jc-tig.netlify.app/courts#criminal-courts
Screenshot 2024-10-09 at 3 09 39 PM

Supervision subpopulations: https://jc-tig.netlify.app/community_supervision#supervision-types
Screenshot 2024-10-09 at 3 09 11 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!!

@morden35
Copy link
Contributor

Ah, hmm... I could strongly type the constants in this file and that will enforce each one's structure is consistent - which is a great call - but, let me know if I'm misunderstanding what you mean by testing the structure of this file or if you were thinking of a test for the entire file itself? Here's the strongly typed version: 432806c

thank you for this! I think this looks good!

My original comment was referring to / asking how can we test that each given IncludesExcludes object is in the structure that the FE expects. However I'm not sure we can test this yet since it's not being used yet (if my understanding is correct)

Copy link
Contributor

@morden35 morden35 left a comment

Choose a reason for hiding this comment

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

@mxosman
Copy link
Contributor Author

mxosman commented Oct 10, 2024

Ah, hmm... I could strongly type the constants in this file and that will enforce each one's structure is consistent - which is a great call - but, let me know if I'm misunderstanding what you mean by testing the structure of this file or if you were thinking of a test for the entire file itself? Here's the strongly typed version: 432806c

thank you for this! I think this looks good!

My original comment was referring to / asking how can we test that each given IncludesExcludes object is in the structure that the FE expects. However I'm not sure we can test this yet since it's not being used yet (if my understanding is correct)

Oh, gotcha!! In that case, I think the best way to enforce this/test it is by leveraging TypeScript and strongly typing it. That way, if you try to deviate from the structure you'll get hit with a TypeScript error immediately (directly in the IDE, via a pop-up in the browser when you run the app, and lint test failures). It would accomplish the same thing as if we wrote some quick smoke tests to make sure that each object has all the right fields and how I would go about checking & enforcing the structure.

Later on, useful tests for us to include would be - based on the agency's sector, does it show this field in the Agency Settings page - and, when they click the edit button, does it show all of the relevant TIG fields for that sector, and does it show all of the right defaults when they click on the JC definitions, etc.? (Sorry if I'm stating something that might be obvious to you!) But, you gave me another idea to make it even more safe and strict - especially if we want to add more TIG elements in the future - I'll consolidate everything into one object keyed by sector and strictly type that, which will help continue to enforce the structure when we make new additions and leave no room for deviations in structure without also explicitly updating the type.

@mxosman
Copy link
Contributor Author

mxosman commented Oct 10, 2024

OK - this commit bf11169 should be more secure and will error if you don't adhere to the structure:

Removing an expected field:
Screenshot 2024-10-10 at 12 28 03 PM

Adding an unexpected field:
Screenshot 2024-10-10 at 12 27 37 PM

Adding an unexpected sector:
Screenshot 2024-10-10 at 12 28 44 PM

Using an unexpected value for default:
Screenshot 2024-10-10 at 1 07 52 PM

Using a non-string for the label:
Screenshot 2024-10-10 at 1 07 40 PM

@mxosman mxosman requested a review from morden35 October 10, 2024 18:09
@mxosman
Copy link
Contributor Author

mxosman commented Oct 10, 2024

Also, is there any way we can add some sort of parity test between the two repos?

Oh, I thought about this a little more - I'm not sure how feasible this would be, but we might actually be able to write something on the recidiviz-data repo to fetch/access a file here given the file's URL and read it since this is a public repo (which probably means no permissions hurdles to cross). Even though it's no longer necessary, if we ever have a need in the future - it might be possible to do, but not sure how cumbersome it would be.

@mxosman mxosman merged commit 00f6e4b into main Oct 11, 2024
7 checks passed
@mxosman mxosman deleted the mahmoud/add-new-court-supervision-tig-elements branch October 11, 2024 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Publisher] Add new agency-wide TIG elements to the frontend
3 participants