[Publisher][Bug] Remove additional context logic from FormStore and DataEntryFormComponents #1341

mxosman · 2024-05-22T18:31:57Z

Description of the change

Remove additional context logic from FormStore and DataEntryFormComponents as contexts are no longer handled by the record page.

There was a bug reported in staging for agency 11 where opening up a monthly record resulted in a crash. The crash was a result of the record page validation of previously saved inputs attempting to delete an error on the metricsValues object (in the FormStore - this object keeps track of the metric values and error state for each metric value) for a metric that did not exist in this object.

How is this happening?
I'm not entirely sure when this happened or if it's always been this way, but it appears that when you add a value to the additional context field in the Metric Settings definition for a top-level metric, save it, and then delete the value and save it (so an empty string is saved) -- the empty string slips through this check here, and gets unnecessarily validated through this.updateContextValue:

justice-counts/publisher/src/stores/FormStore.ts

Lines 87 to 88 in c7bea89

    
           if (context.value !== null && context.value !== undefined) { 
        
             this.updateContextValue(

It's still unclear whether this bug was here the entire time, or whether there was a relatively recent change in how empty string contexts are saved and later retrieved from the BE. But, I think the right move here would be to avoid running unnecessary logic for contexts in the FormStore and record page all together because they are not used in any way on the record page as they now live within the Metric Settings definitions and are handled by the MetricConfigStore.

Steps to repro:

Switch to the commit before the temporary fix via git switch e695ea5cda6c015e5982f86853cbd579b74e061c --detach
Choose any agency in staging
Go to Metric Settings and choose an available metric (or make one available)
Update the top-level metric definition by adding text in the additional context box, then save
Go back to the same metric definition and delete everything in the additional context box, then save
Find a record with matching frequency as the metric you just edited
Observe the unglamorous crash, with the following similar message in the console: TypeError: Cannot read properties of undefined (reading 'SUPERVISION_FUNDING') at updateFieldErrorMessage (FormStore.ts:341:1)

I tested this using the agencies in our playbook, going through the repro steps and ensuring there are no issues in the record page, going through and adding data to the records, reloading, and publishing, and really any areas that involve the FormStore.

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 Recidiviz/recidiviz-data#28666

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

… in Metric Settings by MetricConfigStore

mxosman · 2024-05-30T15:04:53Z

publisher/src/components/Reports/DataEntryFormComponents.tsx

@@ -158,68 +157,3 @@ export const DisaggregationDimensionTextInput = observer(
    );
  }
 );
-


This entire component is not being used anywhere.

mxosman · 2024-05-30T15:05:20Z

publisher/src/components/Reports/DataEntryForm.test.tsx

-  expect(
-    rootStore.formStore.metricsValues[0].PROSECUTION_STAFF.error?.message
-  ).toBe("You are also required to enter a value for this field.");
-});


Test no longer necessary as there are no context fields in the record pages.

mxosman · 2024-05-30T15:06:27Z

publisher/src/stores/FormStore.test.tsx

-  ).toEqual("100");
-
-  expect.hasAssertions();
-});


FormStore no longer needs to worry about the contexts, and as a result, updateContextValue will be deprecated.

mxosman · 2024-05-30T15:07:09Z

publisher/src/stores/FormStore.ts

-            metric.enabled
-          );
-        }
-      });


No contexts in the record page means we don't have to loop through them and update/validate their values.

mxosman · 2024-05-30T15:09:48Z

publisher/src/stores/FormStore.ts

-              ),
-              error: contextError,
-            };
-          }),


All of this logic is unnecessary since we are deprecating updateContextValue and have no interest in the values of the context in the record page.

mxosman · 2024-05-30T15:10:18Z

publisher/src/stores/FormStore.ts

-          key: context.key,
-          value: contextValue,
-        });
-      });


Cleaning up anywhere else that has context logic.

mxosman · 2024-05-30T15:10:41Z

publisher/src/stores/FormStore.ts

-        contextKey
-      );
-    }
-  };


And finally - since we have no interest in anything context-related in the record page, we can safely deprecate this method and all of its dependants.

helperbot-recidiviz · 2024-05-30T15:12:41Z

@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/

lilidworkin

Thank you so much for digging into this so deeply, figuring out the best fix, and testing so thoroughly!

mxosman · 2024-05-30T21:51:08Z

Thank you for the quick review, Lili!

Bug fix by removing all context logic from FormStore as it is handled…

4df2a75

… in Metric Settings by MetricConfigStore

mxosman added the Type: Bug label May 22, 2024

Fix tests

542d933

mxosman force-pushed the mahmoud/fix-remove-context-logic-from-formstore branch from 5c4dad1 to 542d933 Compare May 22, 2024 21:02

mxosman marked this pull request as ready for review May 30, 2024 15:03

mxosman commented May 30, 2024

View reviewed changes

mxosman requested review from morden35, lilidworkin and a team May 30, 2024 15:39

lilidworkin approved these changes May 30, 2024

View reviewed changes

mxosman merged commit b41431a into main May 30, 2024
8 checks passed

mxosman deleted the mahmoud/fix-remove-context-logic-from-formstore branch May 30, 2024 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Publisher][Bug] Remove additional context logic from FormStore and DataEntryFormComponents #1341

[Publisher][Bug] Remove additional context logic from FormStore and DataEntryFormComponents #1341

mxosman commented May 22, 2024 •

edited

Loading

mxosman May 30, 2024

mxosman May 30, 2024

mxosman May 30, 2024

mxosman May 30, 2024

mxosman May 30, 2024

mxosman May 30, 2024

mxosman May 30, 2024 •

edited

Loading

helperbot-recidiviz commented May 30, 2024

lilidworkin left a comment

mxosman commented May 30, 2024

	if (context.value !== null && context.value !== undefined) {
	this.updateContextValue(

@@ @@ -158,68 +157,3 @@ export const DisaggregationDimensionTextInput = observer( @@
                   );
                 }
               );

[Publisher][Bug] Remove additional context logic from FormStore and DataEntryFormComponents #1341

[Publisher][Bug] Remove additional context logic from FormStore and DataEntryFormComponents #1341

Conversation

mxosman commented May 22, 2024 • edited Loading

Description of the change

Type of change

Related issues

Checklists

Development

Code review

mxosman May 30, 2024

Choose a reason for hiding this comment

mxosman May 30, 2024

Choose a reason for hiding this comment

mxosman May 30, 2024

Choose a reason for hiding this comment

mxosman May 30, 2024

Choose a reason for hiding this comment

mxosman May 30, 2024

Choose a reason for hiding this comment

mxosman May 30, 2024

Choose a reason for hiding this comment

mxosman May 30, 2024 • edited Loading

Choose a reason for hiding this comment

helperbot-recidiviz commented May 30, 2024

lilidworkin left a comment

Choose a reason for hiding this comment

mxosman commented May 30, 2024

mxosman commented May 22, 2024 •

edited

Loading

mxosman May 30, 2024 •

edited

Loading