-
Notifications
You must be signed in to change notification settings - Fork 40
Unit Test Fixes #7663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Unit Test Fixes #7663
Conversation
melton-jason
left a comment
There was a problem hiding this 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:
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!
These will be addressed in #7663
melton-jason
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the new helper functions for reading preferences! 🥳
Triggered by d9c793e on branch refs/heads/issue-7662
|
There's a front-end issue in main causing lots of failures in the front-end unit testing. It has to do with ajax mocking. I've been looking into a solution, but nothing working yet. |
|
I simplified the tree management structure and synonymy changes as to avoid putting too much complexity into this PR. The scope of this PR was starting to get too far away from fixing unit tests related to trees, so these changes are now better scoped to this PR. We can look into doing more structural changes to the tree management data in a future issue. I was able to fix the failing front-end unit tests related to ajax mocking by making adjustments in the ajax utils index.js file. @melton-jason ready for re-review. |
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-explanatory (or properly documented)
Testing instructions