Batch Edit: Locality's `lat1text` and `lat2text` fields get updated internally and marks the record as updated

[sharadsw]

From @combs-a:

LatLon has been kind of fixed, but there are also potentially integers; ints are being converted to floats and that marks them as updated cells when they shouldn't be.

The fields lat1text and lat2text are updated internally through the workbench which means Locality records can get marked as updated even when there are no changes visible to the user. This could potentially cause issues with matching records in batch edit when relationships are enabled in #6126

More info here: #5417 (comment)

[CarolineDenis]

NOTES:

• Workbench has the same behavior
• Keep it as known behavior for now
  Relevant code is here:

specify7/specifyweb/specify/parse.py

This was added for CoGe tool, we might break it if we fix this.

[lexiclevenger]

As of 7.11, records affected by this are no longer highlighted as updated. Now, they're marked as new cells and a new record is created after committing the data set, resulting in duplicate locality records despite no changes actually being made by the user.

In this data set, I only edited row 5, but batch edit would create new records for localities I made no changes to.

[realVinayak]

melton-jason, Thanks for the research on this issue, and the #6655. I like the below solution you proposed:

I think it would be beneficial to replace just the values of latitude1 with the lat1text and longitude1 with long1text in the Query Results (regardless of whether or not the text fields were explicitly mapped) and hide the text fields from the resulting BatchEdit Data Set if the text fields were mapped. Then the normal WorkBench behavior of coordinate information can take place.

So, from what I understand, the latitude1 field needs to show lat1text, correct, and same with long? The cleanest way to implement that will be to add a query-rewrite (see below), which I had to add for multiple tree support:

specify7/specifyweb/stored_queries/batch_edit.py

Lines 582 to 589 in a46c114

	def rewrite(
	self, table: Table, all_tree_info: TREE_INFORMATION, running_path=[]
	) -> "RowPlanMap":
	from .batch_edit_query_rewrites import _batch_edit_rewrite # ugh, fix this
	# NOTE: This is written in a very generic way, and makes future rewrites also not too hard.
	# However, tree rank rewrites was probably the hardest that needed to be done.
	return _batch_edit_rewrite(self, table, all_tree_info, running_path)

There are below issues:

1. If not careful, it'll also do the adjustment for to-one relationships (rather than just locality). Fixable, just add a prop denoting is_origin. Basically, replace latitiude field in the join path here with the lat1text, and ditto for longitude.
2. If you do the first implementation, then ordering of fields will get broken. This is because it checks using stringids to compare against order selected by the user (from front-end), which will now have a mismatch (since lat1text will be the field, instead of latitude).
3. An issue with the implementation is that lat1text can be different from latitude. This means that we can run into the issue where user makes a query, doesn't make any change, but batch edit still performs changes. That needs to be minimized. Further, if locality is in a to-one relationship, then it can be matched to a different record OR new locality record gets created (actually, this will be a uniqueness rule violation).

How about this:

1. During parsing fields, when the bounduploadtable has an id, don't add lat1text and long1text (I saw your previous commit, this is the same)
2. During update in BoundUpdateTable, we can know if latitude and longitude has changed. So, just some logic here to add the lat1text if latitude has changed (same for long)

specify7/specifyweb/workbench/upload/upload_table.py

Lines 1072 to 1082 in a46c114

	def _do_update(self, reference_obj, dirty_fields, **attrs):
	# TODO: Try handling parent_obj. Quite complicated and ugly.
	self.auditor.update(reference_obj, None, dirty_fields)
	for key, value in attrs.items():
	setattr(reference_obj, key, value)
	if hasattr(reference_obj, "version"):
	# Consider using bump_version here.
	# I'm not doing it for performance reasons -- we already checked our version at this point, and have a lock, so can just increment the version.
	setattr(reference_obj, "version", getattr(reference_obj, "version") + 1)
	reference_obj.save()
	return reference_obj

3. Doing clone in BoundUploadTable, don't bother checking anything (if latitude has changed), just directly set the lat1text based on latitude.
   Implementation can be done kinda cleanly, by making a member function in BoundUploadTable called _get_extras or something.

That should cover the edge cases that have been run into. Do you if there are more to consider? By the way, the extra field originalLatLongUnit can also be handled this way (just add it at the update or clone time). Ofc, if the user has mapped these fields (technically, if they are in dirty fields), don't touch them (so, if lat1text has been mapped, then don't set it in the clone or update function).

The initial core idea is to make it very hard to accidentally change data, which is why some of the other bugs also needed fix (which accidentally change data).
#6529
#6528