Skip to content

Conversation

@melton-jason
Copy link
Contributor

Fixes #3259 (comment)

The issue occurred when setting the value for lat1text, lat2text, long1text, and long2text for the LatLonUI Plugin.

Which occured at this line:

resource.set(coordinateTextField, trimmedValue);

To highlight the issue, I will use an example from the sdnhmpaleo_3_31_23 database.
(https://sdnhmpaleo33123-v79-dev.test.specifysystems.org/specify/view/locality/24618/)

Specify tries to convert the value of in the database into a valid latitude/longitude value.

Here are the values in the database.

+-----------------+-------------------+
| lat1text        | long1text         |
+-----------------+-------------------+
| 32.810579° N   | 117.2229226° W   |
+-----------------+-------------------+

Specify trims lat1text and long1text to 32.810579 N and 117.2229226 W (respectively), and sets the value of the fields to the trimmed version, which triggers unload protect.
I have made the setting of these values "silent" which means they will not trigger unload protect.

TO TEST:
Make sure the resource's unload protect is not triggered when being initialized, but ensure that manually editing every component still properly triggers unload protect.

@melton-jason melton-jason requested review from a team August 30, 2023 21:52
Copy link
Contributor

@carlosmbe carlosmbe left a comment

Choose a reason for hiding this comment

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

Could not recreate issue with Lat Forms outside of sdnhmpaleo_3_31_23. PR Works great. Please advise on whether or not to approve

@carlosmbe carlosmbe requested a review from a team August 31, 2023 20:57
Copy link

@bronwyncombs bronwyncombs left a comment

Choose a reason for hiding this comment

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

Working as expected for me! Unload protect is still triggered if changes are made to the form.

Screen.Recording.2023-09-01.at.9.03.05.AM.mov

Base automatically changed from v7.9-dev to production September 28, 2023 01:25
Copy link
Contributor

@carlosmbe carlosmbe left a comment

Choose a reason for hiding this comment

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

Issue not fixed. Tested in NHMD_Test. Log in as petrifiedroadkill to NHMD Amber and open Jotaro Record Set.

#3259 (comment)

@CarolineDenis CarolineDenis added this to the 7.9.4 milestone Jan 17, 2024
@melton-jason melton-jason requested a review from a team March 14, 2024 14:44
@melton-jason
Copy link
Contributor Author

Issue not fixed. Tested in NHMD_Test. Log in as petrifiedroadkill to NHMD Amber and open Jotaro Record Set.

#3259 (comment)

#3959 (review)

@specify/ux-testing
Can this be confirmed?

Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

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

Could not recreate issue in edge, might've been fixed by something else?

@melton-jason melton-jason removed this from the 7.9.4 milestone Mar 15, 2024
@CarolineDenis CarolineDenis added this to the 7.9.6 milestone May 21, 2024
@CarolineDenis CarolineDenis requested a review from a team May 23, 2024 20:46
Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

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

Was able to recreate the issue this time but unload protect still triggered when it shouldn't be

2evXM0VPg8.mp4

@melton-jason
Copy link
Contributor Author

Was able to recreate the issue this time but unload protect still triggered when it shouldn't be

2evXM0VPg8.mp4

This is fixed.
I have reverted the state of this PR to be that of production.

Thus, the original issue should occur (that is, unload protect should only occur when the lat1text and/or long1text fields are changed by the LatLongUI Plugin).

However, I would potentially like to discuss potential impacts and propose a 'better' solution.

(Assuming unload protect is now only caused by the LatLongUI Plugin when lat1text and/or long1text are changed)
While silencing the automatic change would 'fix' the unload protect issue, it may cause unwanted side effects.

  • Consider the case from Don't trigger unload protect when setting LatLon plugin values #3959 (comment), where the change is because of some invalid lat1text and/or long1text (this time likely caused by some difference in encoding, as shown below)
    • In this specific case, Specify truncates the lat1text and long1text to 32.810579° N and 117.2229226° W respectively, which is probably what is intended, but will this always be the case?
    • Assume lat1text was instead 3Â.81Â579° N. Specify will silently parse this as 3.81579° N, a completely different value than what the user would expect.
+-----------------+-------------------+
| lat1text        | long1text         |
+-----------------+-------------------+
| 32.810579° N   | 117.2229226° W   |
+-----------------+-------------------+
  • The formatting of incorrect values to their 'correct' form will only be applied if the user makes a change to some other field on the Locality form and then saves.
    • While the values on the form will appear fine, any data integrity issues will not be caught until they are used in a Query, Report/Label, etc. There is a difference from what the user sees on the form, and what it actually present in the database.

Although, I do admit that only setting unload protect in this instance is a less-than optimal solution.
Ideally, we should show the user exactly what has changed so they can determine if they want to modify the field value further or accept the parsed value. i.e., a 'diff' view of the initial resource and all changes to the resource.

@CarolineDenis CarolineDenis modified the milestones: 7.9.6, 7.9.x May 28, 2024
@emenslin
Copy link
Collaborator

I agree with @melton-jason and that we should not silence the unload protect. Since Specify is changing the values the users should be made aware in some way that the values are no longer what they put in.

Maybe if they are putting in the values through workbench there could be some kind of error on any cells with values that Specify would change (e.g. 3Â.81Â579° N would change to 3.81579° N). Maybe an error like disambiguate where they could select how to change it, or just an error that tells the user that some of the values are not accepted or something?

I also think that Jasons idea of showing the user excactly what changed and give them the option to edit or accept the change would also work. I'm not sure visually what you were thinking but it sounds like it could also be a good solution.

@combs-a
Copy link
Collaborator

combs-a commented May 28, 2024

Yes, I also agree with the two above--if there is a change happening then unload protect should be triggered. The suggested change idea be good, though maybe I'd put it as a preference (but make it default) if we go that route--otherwise, I think flagging those cases as errors like Elizabeth said would be good.

@grantfitzsimmons
Copy link
Member

grantfitzsimmons commented Jul 15, 2024

@melton-jason #3259 (comment)

is this back? never left

@maxpatiiuk
Copy link
Member

Maybe if they are putting in the values through workbench there could be some kind of error on any cells with values that Specify would change (e.g. 3Â.81Â579° N would change to 3.81579° N). Maybe an error like disambiguate where they could select how to change it, or just an error that tells the user that some of the values are not accepted or something?

probably shouldn't be an error, but maybe could be a warning
there are valid cases where this would happen - i.e date format being changed when specify parses it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Dev Attention Needed

Development

Successfully merging this pull request may close these issues.

Going to a record set triggers a form change

9 participants