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

[Justice Counts] Fix data entry form bug when navigating back from publish confirm page #148

Merged
merged 2 commits into from Nov 11, 2022

Conversation

terryttsai
Copy link
Contributor

@terryttsai terryttsai commented Nov 10, 2022

Description of the change

See video for reproducing the bug here: https://app.zenhub.com/workspaces/justice-counts--622109ea3bb6e0001d2d06ad/issues/recidiviz/recidiviz-data/16570

This bug is caused by formStore.validatePreviouslySavedInputs being called when the user is navigating from the Publish Confirm page back to the data entry page, which replaces all user inputs with what was originally fetched from the server when the data entry page was first loaded.

The DataEntryForm component gets unmounted and remounted when navigating to and away from the Publish Confirm page, which causes formStore.validatePreviouslySavedInputs to get triggered. I believe formStore.validatePreviouslySavedInputs is intended to be triggered once on loading the data entry page and not when navigating from Publish Confirm back to the Data Entry Form.

My solution is to move the formStore.validatePreviouslySavedInputs trigger out of the DataEntryForm component and into the parent ReportDataEntry component. The parent component is loaded once when the data entry page is loaded, and calls formStore.validatePreviouslySavedInputs. It doesn't get called again until the user navigates away from the data entry section entirely.

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

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

@terryttsai terryttsai requested a review from a team November 10, 2022 21:29
useEffect(() => {
/** Runs validation of previously saved inputs on load */
if (reportMetrics) {
formStore.validatePreviouslySavedInputs(reportID);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, we could also call this whenever a report is fetched? It could just be part of the process of fetching a report, storing it in the report store, and also storing inputs + validation in the form store?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes a lot of sense actually! I don't have strong feelings though -- if you feel that this solution is robust enough for now @terryttsai , I'm fine to merge this in as-is. Up to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong feelings either - both make a lot of sense! I'm good with your solution here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go with this for now!

Copy link
Collaborator

@lilidworkin lilidworkin left a comment

Choose a reason for hiding this comment

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

@terryttsai thank you so much for investigating this and for writing such a clear and well-explained PR description! This solution makes perfect sense to me. Curious what @mxosman thinks about your suggestion of moving this validation logic up to happen right after a report is fetched -- I'll leave it up to you both if you think that's worth doing now. I'm happy with this as-is though :)

useEffect(() => {
/** Runs validation of previously saved inputs on load */
if (reportMetrics) {
formStore.validatePreviouslySavedInputs(reportID);
Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes a lot of sense actually! I don't have strong feelings though -- if you feel that this solution is robust enough for now @terryttsai , I'm fine to merge this in as-is. Up to you.

Copy link
Contributor

@mxosman mxosman left a comment

Choose a reason for hiding this comment

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

Nice debugging and solution, Terry!!

@terryttsai terryttsai merged commit 851c79c into main Nov 11, 2022
@terryttsai terryttsai deleted the terry/fix-data-entry-form-bug branch November 11, 2022 21:27
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.

[Frontend][Bug] Edit metric -> review and publish -> go back to data entry -> changes lost
3 participants