Batch Edit: Date fields show incorrect heading

[grantfitzsimmons]

Describe the bug

Batch editing date fields erroneously shows (Year) for all date field components in the headings for the Batch Edit data set.

To Reproduce
Steps to reproduce the behavior:

1. Add a precision date field to a query
2. Add the combined field (e.g. catalogedDate) and the individual date components (catalogedDate day, month, year)
3. Save the query
4. Batch edit the results

See that all column headings end with (Year) and a number.

Expected behavior
It should use the same headings as in the query results.

Reported By
RBGE during global testing for v7.11.0 (v711-prerelease)

[melton-jason]

Ah, the Issue here is that Batch Edit currently expects there to only be a single field per table in a query.
Specifically, when BatchEdit is constructing the captions for visible fields in the Query, it uses a dictionary with field names as keys:

specify7/specifyweb/stored_queries/batch_edit.py

Lines 1097 to 1103 in 03229ab

	localization_dump = {}
	if captions:
	for (field, caption) in zip(visible_fields, captions):
	table_name, field_name = _get_table_and_field(field)
	field_labels = localization_dump.get(table_name, {})
	field_labels[field_name] = caption
	localization_dump[table_name] = field_labels

Essentially, what is happening here is that if you have fields such as: Cat Date, Cat Date (Day), Cat Date (Month), Cat Date (Year) in your query, for each of the fields Specify is seeing the field name is catalogedDate and storing the caption for catalogedDate as what is displayed in the query (and overwriting any previously stored caption for the field).

When constructing the Upload Plan, Specify is checking this mapping of fields to captions for each of the queries visible fields and getting the captions from the previously constructed dictionary:

specify7/specifyweb/stored_queries/batch_edit.py

Lines 898 to 905 in 03229ab

	table_name, field_name = _get_table_and_field(field)
	field_labels = localization_dump.get(table_name, {})
	# It could happen that the field we saw doesn't exist.
	# Plus, the default options get chosen in the cases of
	if field_name not in field_labels or field.fieldspec.contains_tree_rank():
	localized_label = naive_field_format(field.fieldspec)
	else:
	localized_label = field_labels[field_name]

Since only one of the captions can be mapped to the catalogedDate field (the last one processed), each catalogedDate field will use its caption as its column header in the upload plan.

This should be a relatively straightforward fix.
For example, we could do any of:

• Modify the dictionary to not be a direct mapping of field name to caption, and include auxiliary information such as date precision. This solution is simple, but would make retrieving values more difficult
  • Alternatively, we could make QueryFields hashable (QueryField and QueryFieldSpec are derived from NamedTuples which are immutable, so hashing would be allowed) and use those as keys for the dictionary. This would allow us a more direct one->one mapping of query fields to captions, but may sensitive to any future QueryField changes (if we need to override the class' hash function)
  • Or we could use something like a List instead of a Dictionary. The drawback of doing so is that we would be sacrificing a little performance when retrieving the field caption (O(n) instead of O(1))
• Keep the dictionary as-is and instead when retrieving the caption check whether the QueryField contains a DatePart and modify the caption if needed. A drawback of doing so is this may complicate the retrieval code

[realVinayak]

Nice finds so far. This issue is actually a regression, due to some refactoring I did for the multiple tree support. So, actually, it is present in 7.11 but not in 7.10.2.3.

Jason, in 7.10.2.3, it simply uses stringids, one level dict. I was not satisfied with that implementation, so I added the nested dict logic. stringids actually already contain the date part in them. So, maybe just revert to the original implementation? Will be O(1) too. It should be quite simple to revert to original. multiple trees do not depend on the implementation of localization, but I'll recommend some UX testing for them too (especially in cases where ranks from lots of trees get added).

The rationale for using the newer dict logic was that stringids can be unstable on the backend (they were implemented hackily, and didn't have that much confidence in stringid logic). Plus, debugging became easier