feat(trees): add biostrat filter to chronostrat#8257
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Triggered by 7d49186 on branch refs/heads/issue-8256
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx (1)
595-609: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the
isBioStratextraCondition mapping.
biostratConditions.test.tsonly testsgetQueryComboBoxConditions; the actualfieldName === 'isBioStrat'branch in this file's Search-button handler (which builds theSearchDialogextraFilters) is untested. Given this directly drives the BioStrat search-dialog filtering behavior called out in the PR objectives, a small unit/integration test would help lock in the mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx` around lines 595 - 609, Add test coverage for the `fieldName === 'isBioStrat'` branch in `QueryComboBox` so the Search-button handler’s `SearchDialog` extraFilters mapping is verified. Update or add a focused unit/integration test around `QueryComboBox` that exercises the handler and asserts the built extraCondition for `isBioStrat` matches the expected filter shape, similar to the existing `biostratConditions.test.ts` coverage for `getQueryComboBoxConditions`. Use the `QueryComboBox` Search-button path and `SearchDialog` extraFilters construction as the key symbols to locate the logic.specifyweb/frontend/js_src/lib/components/TreeView/__tests__/biostratFilter.test.ts (1)
14-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest duplicates filter logic instead of exercising the actual
Search.tsximplementation.
filterBio/filterChrono/filterAllare hand-written re-implementations of the predicates inline inSearch.tsx's.filter(...)callback (Lines 97-108 there), rather than imports of an exported function. If the production filtering logic inSearch.tsxchanges or regresses, this test suite won't detect it since it never calls the real code path.Consider extracting the predicate into a small exported helper (e.g.,
getBiostratPredicate(tableName, biostratFilter)) inSearch.tsx, then importing and testing that directly here for real regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/frontend/js_src/lib/components/TreeView/__tests__/biostratFilter.test.ts` around lines 14 - 36, The test file is re-implementing the biostrat filtering logic instead of validating the real `Search.tsx` behavior. Extract the `.filter(...)` predicate from `Search.tsx` into a small exported helper such as `getBiostratPredicate` (or equivalent), then update `biostratFilter.test.ts` to import and assert that helper directly so the test covers the production code path and catches regressions.specifyweb/backend/trees/views.py (1)
176-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding dedicated unit tests for the new biostrat filtering branch.
The only test coverage change (
test_trees.py) exercisesget_tree_rowswithbiostrat='all'; no test verifies the'bio'/'chrono'SQL filtering paths or the root-exception behavior discussed above. Given this logic directly implements the PR's core requirement, dedicated tests would guard against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/trees/views.py` around lines 176 - 260, Add focused unit tests for the biostrat branch in get_tree_rows so the new SQL filters are covered. Update test_trees.py to exercise geologictimeperiod with biostrat set to bio and chrono, and assert the generated rows respect the root exception plus isBioStrat true/false/null behavior. Use get_tree_rows and the existing tree query setup to verify the filtering paths directly, not just the biostrat='all' case.specifyweb/frontend/js_src/lib/components/TreeView/index.tsx (1)
427-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
aria-label/titleto the new biostratSelect, matching sibling selects.The
treePickerSelectabove (Line 306) sets botharia-labelandtitle; this new biostrat filterSelectsets neither, so screen-reader users get no context for the control's purpose beyond the currently selected option text.♻️ Proposed fix
{tableName === 'GeologicTimePeriod' && ( <Select + aria-label={treeText.biostratAll()} className="w-max" + title={treeText.biostratAll()} value={biostratFilter}(Swap in a more purpose-specific label string if one is added to the dictionary, e.g. a dedicated "Filter by" key.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/frontend/js_src/lib/components/TreeView/index.tsx` around lines 427 - 439, Add accessibility labeling to the new biostrat filter Select in TreeView so it matches the sibling treePicker control. Update the GeologicTimePeriod Select to include both aria-label and title attributes with a clear description of the filter’s purpose, using the same pattern as the existing Select above; if a dedicated copy string exists or is added to treeText, use that for the label.specifyweb/backend/trees/tests/test_trees.py (1)
340-460: 🎯 Functional Correctness | 🔵 TrivialMissing backend test coverage for actual bio/chrono filtering.
Both updated calls only pass the neutral
'all'value on aGeographytree, so the actualisBioStratSQL filtering logic (the core server-side objective of this PR) isn't exercised here. Consider adding a test against ageologictimeperiodtree asserting that'bio'returns onlyisBioStrat=TRUErows and'chrono'returnsFALSE/NULLrows.[medium_effort_and_high_reward]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/trees/tests/test_trees.py` around lines 340 - 460, The updated test coverage in get_tree_rows only exercises the neutral 'all' branch on a Geography tree, so the actual isBioStrat filtering paths are still untested. Add a test case using a geologictimeperiod tree and the get_tree_rows helper that verifies 'bio' returns only rows with isBioStrat enabled, while 'chrono' returns only rows with isBioStrat disabled or unset; use the existing get_tree_rows calls and assertions as the place to extend coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/backend/trees/views.py`:
- Around line 245-260: The child_count aggregate in the tree query is not
respecting the active biostrat filter, so expand chevrons can appear for rows
that will return no children. Update the query in the tree view logic around the
node/child joins and group_by/order_by so the same bio/chrono predicate applied
to node rows is also applied to the child join condition, ensuring the counted
children match what getRows will actually return for expand actions.
---
Nitpick comments:
In `@specifyweb/backend/trees/tests/test_trees.py`:
- Around line 340-460: The updated test coverage in get_tree_rows only exercises
the neutral 'all' branch on a Geography tree, so the actual isBioStrat filtering
paths are still untested. Add a test case using a geologictimeperiod tree and
the get_tree_rows helper that verifies 'bio' returns only rows with isBioStrat
enabled, while 'chrono' returns only rows with isBioStrat disabled or unset; use
the existing get_tree_rows calls and assertions as the place to extend coverage.
In `@specifyweb/backend/trees/views.py`:
- Around line 176-260: Add focused unit tests for the biostrat branch in
get_tree_rows so the new SQL filters are covered. Update test_trees.py to
exercise geologictimeperiod with biostrat set to bio and chrono, and assert the
generated rows respect the root exception plus isBioStrat true/false/null
behavior. Use get_tree_rows and the existing tree query setup to verify the
filtering paths directly, not just the biostrat='all' case.
In `@specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx`:
- Around line 595-609: Add test coverage for the `fieldName === 'isBioStrat'`
branch in `QueryComboBox` so the Search-button handler’s `SearchDialog`
extraFilters mapping is verified. Update or add a focused unit/integration test
around `QueryComboBox` that exercises the handler and asserts the built
extraCondition for `isBioStrat` matches the expected filter shape, similar to
the existing `biostratConditions.test.ts` coverage for
`getQueryComboBoxConditions`. Use the `QueryComboBox` Search-button path and
`SearchDialog` extraFilters construction as the key symbols to locate the logic.
In
`@specifyweb/frontend/js_src/lib/components/TreeView/__tests__/biostratFilter.test.ts`:
- Around line 14-36: The test file is re-implementing the biostrat filtering
logic instead of validating the real `Search.tsx` behavior. Extract the
`.filter(...)` predicate from `Search.tsx` into a small exported helper such as
`getBiostratPredicate` (or equivalent), then update `biostratFilter.test.ts` to
import and assert that helper directly so the test covers the production code
path and catches regressions.
In `@specifyweb/frontend/js_src/lib/components/TreeView/index.tsx`:
- Around line 427-439: Add accessibility labeling to the new biostrat filter
Select in TreeView so it matches the sibling treePicker control. Update the
GeologicTimePeriod Select to include both aria-label and title attributes with a
clear description of the filter’s purpose, using the same pattern as the
existing Select above; if a dedicated copy string exists or is added to
treeText, use that for the label.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bc6ed65-504e-4f8f-aa82-70783040873c
📒 Files selected for processing (15)
config/common/schema_localization_en.jsonspecifyweb/backend/context/data/schemalocalization.jsonspecifyweb/backend/trees/tests/test_trees.pyspecifyweb/backend/trees/views.pyspecifyweb/frontend/js_src/lib/components/QueryComboBox/__tests__/biostratConditions.test.tsspecifyweb/frontend/js_src/lib/components/QueryComboBox/helpers.tsspecifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsxspecifyweb/frontend/js_src/lib/components/TreeView/Row.tsxspecifyweb/frontend/js_src/lib/components/TreeView/Search.tsxspecifyweb/frontend/js_src/lib/components/TreeView/__tests__/biostratFilter.test.tsspecifyweb/frontend/js_src/lib/components/TreeView/index.tsxspecifyweb/frontend/js_src/lib/localization/tree.tsspecifyweb/frontend/js_src/lib/tests/ajax/static/context/schema_localization.json/lang=en.jsonspecifyweb/frontend/js_src/lib/utils/cache/definitions.tsspecifyweb/specify/fixtures/empty_db.json
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
1 participant
Fixes #8256
This PR introduces a new filter for Geologic Time Period tree views and searches, allowing users to display all nodes, only chronostratigraphic nodes, or only biostratigraphic nodes.
Building on the biostrat filter dropdown and backend support from the base branch (
issue-6049), this PR adds:isBioStrat = TRUE). Previously this combo box displayed all nodes because Specify 7 builds its own queries instead of using the custom SQL from the TypeSearch definition (which already contained theisBioStrat = 1filter since Specify 6). The ChronosStrat field continues to show all GeologicTimePeriod nodes.Checklist
Testing instructions
Open a PaleoContext form in any paleo/geo discipline (e.g., Invert Paleo, Vert Paleo, Geology).
In the BioStrat field (QueryComboBox, which will not be present by default), verify that only GeologicTimePeriod nodes with
isBioStrat = TRUEappear in the results.Click the magnifying glass (search) button on the BioStrat field. The search dialog should similarly only show results where
isBioStrat = TRUE.Verify the ChronosStrat field (same table, different typesearch) is unaffected and still shows all GeologicTimePeriod nodes.
Verify other QueryComboBox fields and tree search for other trees (Taxon, Geography, Storage, etc.) still work as expected.
Open the GeologicTimePeriod tree viewer. Toggle between
All Nodes/Biostratigraphy/Chronostratigraphy. Verify the tree search box respects the selection.Summary by CodeRabbit
New Features
Bug Fixes
Tests