Skip to content

Conversation

@acwhite211
Copy link
Member

@acwhite211 acwhite211 commented Jan 20, 2026

Fixes #7662

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.
image

@github-project-automation github-project-automation bot moved this to 📋Back Log in General Tester Board Jan 20, 2026
@acwhite211 acwhite211 marked this pull request as ready for review January 20, 2026 16:58
@acwhite211 acwhite211 requested review from a team January 20, 2026 16:58
@acwhite211 acwhite211 added this to the 7.12.0 milestone Jan 23, 2026
Copy link
Contributor

@melton-jason melton-jason left a comment

Choose a reason for hiding this comment

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

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:

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:

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!

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
Copy link
Contributor

Choose a reason for hiding this comment

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

(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
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

melton-jason added a commit that referenced this pull request Feb 3, 2026
These will be addressed in #7663
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 📋Back Log

Development

Successfully merging this pull request may close these issues.

Fix Failing Unit Tests

4 participants