Metric Settings: Add Configuration Status button

mxosman commented 3 months ago

Description of the change

Add configuration status buttons to metric breakdowns and includes/excludes (top-level and dimension). This will help agencies see and indicate whether the breakdowns and includes/excludes have been configured to their satisfaction.

You can play with it here: https://mahmoudtest---publisher-web-b47yvyxs3q-uc.a.run.app/

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

Checklists

Development

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

[x] 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

helperbot-recidiviz commented 3 months ago

@mxosman successfully triggered a playtest deployment. Full deployment usually takes 5 minutes. Your playtest link is https://mahmoudtest---publisher-web-b47yvyxs3q-uc.a.run.app/

morden35 commented 3 months ago

I think this is looking great Mahmoud!

On a higher level, I noticed that the user is still able to change the metric and breakdown settings even after they have marked configuration as complete. Is that okay, or is that something we want to gate? Aka do we want to user to mark "Undo Complete Configuration" in order to change the metric configuration? lmk if that makes sense

lilidworkin commented 3 months ago

@mxosman sorry for the delay on review of this! I just played around on staging and noticed that in some cases the save isn't persisting (I think in cases where this configuration hasn't been saved before, because if you try a second time, then it goes through) but I'm 99% sure it's a backend issue and I have a hunch of what might be going on, so I will look into it soon. I'll also do an actual review of the code tomorrow!

lilidworkin commented 3 months ago

I noticed that the user is still able to change the metric and breakdown settings even after they have marked configuration as complete. Is that okay, or is that something we want to gate? Aka do we want to user to mark "Undo Complete Configuration" in order to change the metric configuration? lmk if that makes sense

This is a really good callout @morden35 ! I would say for v0 it's fine to keep as is. If CSG points this out though, it's an easy thing to taskify for Ilya :)

morden35 commented 3 months ago

This is a really good callout @morden35 ! I would say for v0 it's fine to keep as is. If CSG points this out though, it's an easy thing to taskify for Ilya :)

makes sense!

mxosman commented 3 months ago

Thank you all so much for the feedback so far! I'm going to do some catching up in PSI-land and follow up on the feedback later today. Please feel no rush, @lilidworkin - will look at it again too incase it's FE!

mxosman commented 3 months ago

Looks like later today === Monday. Have a great weekend!

lilidworkin commented 3 months ago

And @mxosman obviously no rush at all on implementing the change above! We can also have Ilya take over this PR and do it too :)

mxosman commented 3 months ago

I noticed that the user is still able to change the metric and breakdown settings even after they have marked configuration as complete. Is that okay, or is that something we want to gate? Aka do we want to user to mark "Undo Complete Configuration" in order to change the metric configuration? lmk if that makes sense

This is a really good callout @morden35 ! I would say for v0 it's fine to keep as is. If CSG points this out though, it's an easy thing to taskify for Ilya :)

+1 - great call out and agreed to keep it to what they asked for to start! Thank you for suggesting!

... LMK if that makes sense. I think the fix is to combine these requests into a single one. Is that possible or tricky? If that's hard, we can also think about possible backend solutions.

!!! Thank you so so much for catching and digging into this! This is all super helpful, and makes perfect sense! I think you're absolutely right. It shouldn't be too hard, let me play around with it and see!

THANK YOU BOTH SO MUCH!

mxosman commented 3 months ago

OK, I promise tomorrow morning this has my full attention! Taking care of some higher-pri stuff, but am at a good place to mentally pivot back to this! Should not take much longer.

Also, I'm thinking once the fixes are in, I'll redeploy to playtesting and ping the CSG thread to play with it via the playtesting URL (I remember Katie wanted a heads up to check-in with folks on her end before we roll it out) -- and I can keep this PR open to address any of their feedback before merging it in to be deployed to staging + prod. But, LMK if it makes more sense to merge in and gate to staging env!

lilidworkin commented 3 months ago

That makes a lot of sense to me @mxosman ! And really no rush at all!!

helperbot-recidiviz commented 3 months ago

@mxosman successfully triggered a playtest deployment. Full deployment usually takes 5 minutes. Your playtest link is https://mahmoudtest---publisher-web-b47yvyxs3q-uc.a.run.app/

mxosman commented 3 months ago

Hi @lilidworkin! I just redeployed to playtesting and it looks like it's working -- whenever you have a free moment, could I ask for another quick sanity check before I ping CSG?

lilidworkin commented 3 months ago

@mxosman just played with it and wasn't able to repro! Looks awesome! Definitely feel good about you posting and letting CSG know. I would tag Amir and Katie in particular. Thank you so much for your work on this!

mxosman commented 3 months ago

@mxosman just played with it and wasn't able to repro! Looks awesome! Definitely feel good about you posting and letting CSG know. I would tag Amir and Katie in particular. Thank you so much for your work on this!

Thank you so so much for catching this and your time testing and all your work on it too!

mxosman commented 3 months ago

Hi @lilidworkin & @morden35! I just got the green light from Amir and Katie to launch this. 🚀 Would love an approval whenever either of you have a free moment today!

morden35 commented 3 months ago

woo congrats Mahmoud!

Recidiviz / justice-counts