Unit Test Fixes by acwhite211 · Pull Request #7663 · specify/specify7

acwhite211 · 2026-01-20T15:54:30Z

Fixes the various failing unit tests currently in main. The two groups the unit tests that are fixed in this PR are the tree synonymy and locality update tests.

Checklist

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

Testing instructions

See that all the unit tests are passing in the GitHub Actions.

melton-jason

I fixed a bug where the backend was completely ignoring the value of the sp7.allow_adding_child_to_synonymized_parent preferences.

The problem is that we were using a regular expression to fetch the value of the Remote Preference (now Collection Preference) prior to 61cc51b (introduced in #7557), and when we transitioned to a JSON dictionary approach to storing the preference, the regular expression was used as the key in the backend to fetch the preference value-- where it should have been using the string literal.

Below is the code from v7.11.3:

specify7/specifyweb/backend/trees/extras.py

Lines 401 to 402 in dcecec6

    
           pattern = r'^sp7\.allow_adding_child_to_synonymized_parent\.' + node.specify_model.name + '=(.+)' 
        
           override = re.search(pattern, get_remote_prefs(), re.MULTILINE)

Below is the code in main:

specify7/specifyweb/backend/trees/extras.py

Lines 420 to 423 in 6683008

    
           synonymized = treeManagement_pref.get('synonymized', {}) \ 
        
               if isinstance(treeManagement_pref, dict) else {} 
        
           add_synonym_enabled = synonymized.get(r'^sp7\.allow_adding_child_to_synonymized_parent\.' + node.specify_model.name + '=(.+)', False) if isinstance(synonymized, dict) else False

We should be using the literal sp7.allow_adding_child_to_synonymized_parent.TREE string rather than the regular expression as the key.

This fix also simplified some of the other changes in this PR, so feel free to give it a once over @acwhite211!

I'm not going to explicitly approve this PR due to the changes I've made (those should probably be reviewed first), but I'll be down to merge once the additional changes are reviewed further!

melton-jason · 2026-02-02T19:34:32Z

specifyweb/backend/trees/extras.py

-        if isinstance(treeManagement_pref, dict) else {}
-
-    add_synonym_enabled = synonymized.get(r'^sp7\.allow_adding_child_to_synonymized_parent\.' + node.specify_model.name + '=(.+)', False) if isinstance(synonymized, dict) else False
+    add_synonym_enabled = synonymized.get('sp7.allow_adding_child_to_synonymized_parent.' + node.specify_model.name, False) if isinstance(synonymized, dict) else False


(optional, just a question / request)

Is there a particular reason why we kept this preference name for the Collection Preferences (#7557)?
Since we're doing a total migration of this preference from Remote Preferences, I think we could move away from the verbose and confusing Java property names.

Something like this might be more readable for future maintainers and better leverage the new JSON structure of the preference (we could use an array instead of dict if you wanted to 🤷)

# other Collection Preferences ... "treeManagement": { "expand_synonymization_actions": { "taxon": true, "storage": true } }

I'd also be a fan to reconsider the default behavior of these preferences that are being redefined as Collection Preferences.
With this tree management preference in particular, perhaps we can invert its current behavior? That is, the user can opt-in to stricter tree checking rather than opt-in to more relaxed synonym checking behavior.
In other words, we provide the allow adding children to synonyms behavior by default and the managers can set this preference to disallow the adding synonymization of nodes with children and adding a child to a synonym.

@grantfitzsimmons
What do you think of this change to invert the synonymization preference (opt-in to strict checking rather than opt-in to looser checking)?
Form my experiences and understanding of how people interact with this preference thus far, the "looser" behavior is the more common and desired use case here.

I would prefer this to be opt-in. Users generally favor the added flexibility.

👍 I pushed a commit that switches both adding_node() and synonymize() to use the literal key sp7.allow_adding_child_to_synonymized_parent. instead of the old regex key, and centralizes CollectionPreferences parsing into a new function _get_collection_prefs_dict() so we handle None/empty/malformed JSON.

I added a new function that can handle both a new json structure, as well as being backwards compatible with the legacy one. Let me know if that structure looks good.
Ready for re-review.

These will be addressed in #7663

melton-jason

Love the new helper functions for reading preferences! 🥳

melton-jason · 2026-02-06T15:16:01Z

specifyweb/backend/trees/extras.py

+    except Exception:
+        return {}


I'm a little hesitant on using a general Exception catch-all like this 😅.
This could make it pretty hard to debug if there were a genuine bug with parsing the collection preferences as JSON.

Theoretically, the only times the JSON.loads should fail is when the Collection Prefs is either not valid JSON or contains some unparseable character.
In either case, I'm of the opinion we should fail fast (and not catch exceptions) because that would indicate something wrong with the construction or update of the Collection Prefs: this would hopefully be caught with automated tests and general testing.
If some bug were to occur without us catching it, the user would be silently using the default values for the preferences: which could result in data integrity issues.

If we must keep the exception catch here, can we log a warning?
For the best of both worlds, maybe we can include a flag parameter to this function (or otherwise monkey patch it during tests) we can use to indicate whether it should fail silently or raise any execption.

melton-jason · 2026-02-06T15:16:28Z

specifyweb/backend/trees/extras.py

+    syn = tm.get("synonymized")
+    if isinstance(syn, dict):
+        legacy_key = f"sp7.allow_adding_child_to_synonymized_parent.{tree_name}"
+        if legacy_key in syn:
+            return bool(syn.get(legacy_key))


I love the backwards compatibility sentiment here!
But we don't really need to be backwards compatible with the legacy preference.
This will be part of the new Collection Preferences that was introduced in #7557: which is not yet released!

As far as I know, we're not migrating any of the old legacy preferences or supporting the old Remote Preferences.
While people would need to reconfigure their preferences, this does come with the benefit of not needing to support any legacy customizations--so we can simplify our preference reading behavior and start with a clean slate.

Essentially, unless we decide to migrate people's preferences from Remote Preferences to Collection Preference or decide to implement a precedence in resolving these preferences, the only benefit of supporting the legacy preference is for testing databases that have already configured the preference

melton-jason · 2026-02-06T15:20:58Z

specifyweb/backend/trees/extras.py

+        collection_prefs_dict = _get_collection_prefs_dict(collection, user)

        treeManagement_pref = collection_prefs_dict.get('treeManagement', {})
+        treeManagement_pref = treeManagement_pref if isinstance(treeManagement_pref, dict) else {}

-        synonymized = treeManagement_pref.get('synonymized', {}) \
-            if isinstance(treeManagement_pref, dict) else {}
+        synonymized = treeManagement_pref.get('synonymized', {})
+        synonymized = synonymized if isinstance(synonymized, dict) else {}


Can we remove these lines?
They were only needed to read the expanded synonymization preference, but that's already handled as part of the _expand_synonymization_actions_enabled function call below

melton-jason · 2026-02-06T15:22:53Z

specifyweb/backend/trees/extras.py

+    collection_prefs_dict = _get_collection_prefs_dict(collection, user)

    treeManagement_pref = collection_prefs_dict.get('treeManagement', {})
+    treeManagement_pref = treeManagement_pref if isinstance(treeManagement_pref, dict) else {}

-    synonymized = treeManagement_pref.get('synonymized', {}) \
-        if isinstance(treeManagement_pref, dict) else {}
+    synonymized = treeManagement_pref.get('synonymized', {})
+    synonymized = synonymized if isinstance(synonymized, dict) else {}

-    add_synonym_enabled = synonymized.get(r'^sp7\.allow_adding_child_to_synonymized_parent\.' + node.specify_model.name + '=(.+)', False) if isinstance(synonymized, dict) else False
+    pref_key = f"sp7.allow_adding_child_to_synonymized_parent.{node.specify_model.name}"
+    add_synonym_enabled = bool(synonymized.get(pref_key, False))


Can we remove these lines and replace them with a call to the new _expand_synonymization_actions_enabled helper function?

melton-jason · 2026-02-06T15:24:42Z

specifyweb/backend/locality_update_tool/update_locality.py

-        parsed_locality_fields = [parse_field(
-            collection, 'Locality', dict['field'], dict['value'], locality_id, row_number) for dict in locality_values if dict['value'].strip() != ""]
+        parsed_locality_fields = [
+            parse_field(
+                collection, 'Locality', d['field'], d['value'], locality_id, row_number
+            )
+            for d in locality_values
+        ]

-        parsed_geocoorddetail_fields = [parse_field(
-            collection, 'Geocoorddetail', dict["field"], dict['value'], locality_id, row_number) for dict in geocoorddetail_values if dict['value'].strip() != ""]
+        parsed_geocoorddetail_fields = [
+            parse_field(
+                collection, 'Geocoorddetail', d['field'], d['value'], locality_id, row_number
+            )
+            for d in geocoorddetail_values
+        ]


Which failing tests were these fixing?
The if dict['value'].strip() != "" condition that was removed was put in place to handle the case when a field in the provided Data Set only contained whitespace (in which case the Locality Update Tool was to not parse the field and treat it like no value was provided).

It looks like tests are passing with the old condition as well.
For example a recent commit based on main, fbfcb52, had this function unchanged and backend tests passed: https://github.com/specify/specify7/actions/runs/21755196125/job/62763414702?pr=7643.

Although, I'm unsure if the Locality Update Tool tests even ran during that test suite. I couldn't find any of the tests in the output, and I also found a bunch of broken import paths for the locality update tests that I fixed in b87f4da

Was there some warning or error message in the test output that prompted this change, rather than a test explicitly failing?

melton-jason · 2026-02-06T16:00:13Z

specifyweb/backend/trees/extras.py

+    esa = tm.get("expand_synonymization_actions")
+    if isinstance(esa, dict) and tree_name in esa:
+        return bool(esa.get(tree_name))


Just a reminder before this is merged that we also need to support this new structure on the frontend!

melton-jason · 2026-02-06T16:25:20Z

specifyweb/backend/trees/extras.py

+def _expand_synonymization_actions_enabled(collection, user, tree_name: str) -> bool:
+    """
+    New CollectionPreferences shape:
+      treeManagement.expand_synonymization_actions.<tree_name> = true/false


(A continuation of #7663 (comment) and #7663 (comment))

Do we want to make the more expand synonymization behavior the default and instead make this preference be an opt-in for stricter tree checking?

We would just need to change the name of the preference and flip the conditions when reading the preference.

Or is the decision to tackle that in a different PR or keep the behavior as-is?

main unit test fixes

ecbdbf9

github-project-automation bot moved this to 📋Back Log in General Tester Board Jan 20, 2026

github-project-automation bot added this to General Tester Board Jan 20, 2026

acwhite211 marked this pull request as ready for review January 20, 2026 16:58

acwhite211 requested review from a team January 20, 2026 16:58

acwhite211 added this to the 7.12.0 milestone Jan 23, 2026

CarolineDenis requested a review from melton-jason January 27, 2026 14:06

melton-jason and others added 3 commits January 30, 2026 08:26

Merge branch 'main' into issue-7662

51382bd

fix: use correct key when accessing tree collection preference

45f6960

Merge branch 'main' into issue-7662

4806a8d

melton-jason reviewed Feb 2, 2026

View reviewed changes

acwhite211 added 3 commits February 2, 2026 15:56

add _get_collection_prefs_dict helper function

af74472

trigger test

c7ed248

add _expand_synonymization_actions_enabled

6b372ee

acwhite211 requested review from grantfitzsimmons and melton-jason February 3, 2026 16:40

melton-jason added a commit that referenced this pull request Feb 3, 2026

fix: remove code for unit test fix

d33be04

These will be addressed in #7663

acwhite211 and others added 2 commits February 4, 2026 09:11

Merge branch 'main' into issue-7662

7306127

fix: broken imports in locality update tests

b87f4da

melton-jason requested changes Feb 6, 2026

View reviewed changes

github-project-automation bot moved this from 📋Back Log to Dev Attention Needed in General Tester Board Feb 6, 2026

	pattern = r'^sp7\.allow_adding_child_to_synonymized_parent\.' + node.specify_model.name + '=(.+)'
	override = re.search(pattern, get_remote_prefs(), re.MULTILINE)

	synonymized = treeManagement_pref.get('synonymized', {}) \
	if isinstance(treeManagement_pref, dict) else {}

	add_synonym_enabled = synonymized.get(r'^sp7\.allow_adding_child_to_synonymized_parent\.' + node.specify_model.name + '=(.+)', False) if isinstance(synonymized, dict) else False

Conversation

acwhite211 commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Testing instructions

Uh oh!

melton-jason left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melton-jason left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melton-jason Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

acwhite211 commented Jan 20, 2026 •

edited

Loading

melton-jason Feb 6, 2026 •

edited

Loading