Skip to content

Fix unload protect being triggered in locality record sets - #4998

Merged
sharadsw merged 1 commit into
productionfrom
issue-4995
Jun 14, 2024
Merged

Fix unload protect being triggered in locality record sets#4998
sharadsw merged 1 commit into
productionfrom
issue-4995

Conversation

@sharadsw

@sharadsw sharadsw commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

Fixes #4995

Caused by #4939
The changes in that PR sets the validation blocker more times than it needs to which somehow causes an unload protect to trigger. My guess is it's a timing error.

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

  • Go to a locality record set
  • Navigate between records
  • Verify unload protect does not trigger

@sharadsw sharadsw added this to the 7.9.6 milestone Jun 12, 2024

@melton-jason melton-jason left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As noted in #4995 (comment), the issue only occurred when switching between records (most likely in a record set).
Unload protect would not be triggered on a record if the page was refreshed.

The unload protect is being triggered by the following line in the LatLongUI plugin:

resource.set(coordinateTextField, trimmedValue);
.

For example, navigate to https://lsumzmammals9823-edge.test.specifysystems.org/specify/view/locality/1/?recordsetid=478 which has the following values for LatLongUI Plugin related fields:

{
  "lat1text": "30",
  "latitude1": 30,
  "long1text": "-93",
  "longitude1": -93,
  "originallatlongunit": 0,
  "srclatlongunit": 0,
}

Navigate to the next record https://lsumzmammals9823-edge.test.specifysystems.org/specify/view/locality/2/?recordsetid=478
which has the following field values:

{
  "lat1text": "30.837",
  "latitude1": 30.837,
  "long1text": "-91.218",
  "longitude1": -91.218,
  "originallatlongunit": 0,
  "srclatlongunit": 0,
}

The unload protect on the second record is being triggered by in two instances:

  • lat1text is being set to 30
  • long1text is being set to -93

These values are coming from the previous resource's values for those fields.
(the field values are eventually 'reset' to their expected values).

This is happening because setValidation callback in the useEffect dependency array (linked below) is being updated far more frequently since it was changed in #4939.

Before #4939, the setValidation from useResourceValue was only being updated whenever the InFormEditorContext was updated.

Now, setValidation from useResourceValue is being updated whenever the resource or field changes.

This overlaps with:

/*
* Don't update this when resource changes, as that case is handled by the
* useEffect hooks above
*/

@sharadsw
sharadsw marked this pull request as ready for review June 12, 2024 18:32
@sharadsw
sharadsw requested a review from a team June 12, 2024 18:32

@emenslin emenslin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Testing instructions

  • Go to a locality record set
  • Navigate between records
  • Verify unload protect does not trigger

Unload protect is no longer triggered

jLmuA7maZD.mp4

This locality record set was uploaded through the workbench and it does trigger unload protect, however, it is also doing this in 7.9.5 whereas the other record sets are not. I am going to approve this pr but I wanted to mention it in case it can be fixed here

5BdNuzroUX.mp4

@emenslin
emenslin requested a review from a team June 12, 2024 20:20

@Areyes42 Areyes42 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Testing instructions

  • Go to a locality record set
  • Navigate between records
  • Verify unload protect does not trigger

Looks good, unload protect doesn't get triggered anymore! 👍

Screen.Recording.2024-06-12.at.4.47.38.PM.mov

@Areyes42
Areyes42 requested a review from a team June 12, 2024 21:53

@combs-a combs-a left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unsure what's going on with this case since it's also present in v7.9.5, but unload protect is being triggered on a few record sets I just made through queries:

12_unloadprotect1.mp4
12_unloadprotect2.mp4

Just commenting for now w/o requesting changes since it might be a separate issue from this since it follows Elizabeth's second case more. I can test more thoroughly when I get in tomorrow.

@maxpatiiuk

Copy link
Copy Markdown
Member

(optional) we could make setValidation here consumer a fieldRef rather than field state, thus not triggering a needless re-render

setValidation: React.useCallback(

@combs-a combs-a left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looking more into this, it seems like the issue I was having before was a separate one ( PR #3959 ) and was able to make a new record set that has the right bug (no unload protect in v7.9.5, but does have it in edge).

13_unloadprotect3.mp4

Testing instructions

  • Go to a locality record set
  • Navigate between records
  • Verify unload protect does not trigger

Looks good! 🎉

@sharadsw

Copy link
Copy Markdown
Contributor Author

Created some record sets with WB and queries but I'm not able to reproduce the bug. I think the underlying cause for those is definitely different than the one for this issue. I think you're correct about it being related to #3259

@sharadsw
sharadsw merged commit 9a8b1dd into production Jun 14, 2024
@sharadsw
sharadsw deleted the issue-4995 branch June 14, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅Done

Development

Successfully merging this pull request may close these issues.

Unload protect triggered when switching between records of a record set

6 participants