[M1962] inverts submission/validation#688
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds end-to-end macroinvertebrate belt-invert support: CSV ingest serializer and choices, observation/transect validators, submission writer/parsers, model migration/choice cleanup, sample-unit wiring, and tests/fixtures. ChangesMacroinvertebrate Belt-Invert Protocol Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/api/ingest/macroinvertebrate_serializer.py`:
- Around line 24-26: Annotate the mutable class attributes with typing.ClassVar
to avoid shared-state mutation: change the declarations for
additional_group_fields and composite_fields (and the other mutable class
attribute in this module) to use ClassVar with the correct types (e.g.,
ClassVar[List[str]] for additional_group_fields and ClassVar[Dict[str,
List[str]]] for composite_fields), import ClassVar from typing if needed, and
keep the existing copy() usage when deriving from
CollectRecordCSVSerializer.additional_group_fields to preserve instance-safe
mutation.
In `@src/api/resources/sampleunitmethods/beltinvertmethod.py`:
- Line 14: The Meta class uses mutable defaults causing RUF012: replace mutable
values in Meta (exclude = [] and extra_kwargs = {...}) with immutable defaults
(e.g., exclude = None or exclude = () and extra_kwargs = None or use a
frozendict/tuple) and ensure the serializer or class that consumes these values
handles None by assigning a new list/dict at runtime; update
beltinvertmethod.py's Meta (attributes exclude and extra_kwargs) to use
immutable defaults and add logic where the Meta values are read (e.g., in the
serializer/class initializer or a classmethod) to coerce None into a fresh
list/dict to preserve previous behavior.
In `@src/api/submission/validations/validators/invert_belt_transect.py`:
- Around line 40-72: The code reads self.relative_depth_path but never uses it,
so update _get_query_args to fetch relative_depth =
self.get_value(collect_record, self.relative_depth_path) or None, validate it
alongside depth (e.g., attempt float(relative_depth) inside the existing try
block and raise on failure), and include "relative_depth": relative_depth in the
returned query dict so duplicate-match queries consider relative depth;
reference symbols: _get_query_args, self.relative_depth_path, relative_depth.
In `@src/api/submission/validations/validators/invert_obs.py`:
- Around line 119-125: The try/except in the density bootstrap masks all
errors—replace the blanket except with targeted handling: catch
InvertBeltTransectWidth.DoesNotExist when the DB lookup for
InvertBeltTransectWidth (used by width_id/width_val) fails, and also catch
ValueError/TypeError for invalid numeric casts of len_surveyed/width_val; for
unexpected exceptions let them propagate (or log and raise) instead of returning
OK so real runtime/DB errors aren’t silently ignored—update the block around
transect.get("width"), float(transect.get("len_surveyed")...), and
InvertBeltTransectWidth.objects.get(...) accordingly.
In `@src/api/tests/test_ingest.py`:
- Around line 456-468: The test is unpacking two values from utils.ingest but
never uses the second (output); in src/api/tests/test_ingest.py update the call
to utils.ingest so you only capture the value you need (new_records) — either
assign new_records = utils.ingest(...)[0] or use a throwaway name (e.g.,
new_records, _ = utils.ingest(...)) and remove any reference to output; keep the
same arguments and call to utils.ingest and only change the left-hand
assignment.
In `@src/api/tests/test_validations.py`:
- Line 398: Replace the ambiguous multiplication sign characters used in the
comments (e.g., the comment showing "1m wide × 50m long = 50 m²" and the other
similar comment) with the ASCII letter "x" so Ruff RUF003 is satisfied; update
the comment text in test_validations.py (the density/area calculation comment
and the other comment around the same area) to use "1m wide x 50m long = 50 m²"
(and the equivalent replacement for the other occurrence).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a8a2574-b6d1-4fd5-9098-88d24953d564
⛔ Files ignored due to path filters (1)
src/api/tests/ingest_data/macroinvertebrate.csvis excluded by!**/*.csv
📒 Files selected for processing (23)
src/api/ingest/__init__.pysrc/api/ingest/choices.pysrc/api/ingest/macroinvertebrate_serializer.pysrc/api/ingest/utils.pysrc/api/migrations/0116_invertgroupofinterest_density_upper_bound.pysrc/api/models/core.pysrc/api/models/protocols/macroinvertebrate.pysrc/api/resources/sampleunitmethods/beltinvertmethod.pysrc/api/submission/parser.pysrc/api/submission/utils.pysrc/api/submission/validations/belt_invert.pysrc/api/submission/validations/utils.pysrc/api/submission/validations/validators/__init__.pysrc/api/submission/validations/validators/invert_belt_transect.pysrc/api/submission/validations/validators/invert_obs.pysrc/api/submission/validations/validators/sample_unit_consistency.pysrc/api/submission/writer.pysrc/api/tests/fixtures/collect_records.pysrc/api/tests/fixtures/macroinvertebrate.pysrc/api/tests/test_ingest.pysrc/api/tests/test_validations.pysrc/api/utils/invert_goi.pysrc/api/utils/sample_units.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
gridcell
left a comment
gridcell
left a comment
There was a problem hiding this comment.
AI Code Review
Assessment: The macroinvertebrate belt-transect protocol is well-implemented: all dispatch registration sites are correctly wired, validators are cleanly organised, and the batched InvertSizeValidator query avoids the N+1 anti-pattern. The most important item to address before merge is a UUID serialization bug in DifferentInvertTransectWidthValidator that will crash result persistence at runtime. The destructive migration 0116 should be verified against production row counts before applying, and test coverage for the new writer and two record-level validators is missing.
| Severe | Warning | Question | Info | Praise |
|---|---|---|---|---|
| 0 | 8 | 4 | 4 | 5 |
Files reviewed: 24 / 24
Cross-Cutting Concerns
- The new BeltInvertProtocolWriter submission path has no end-to-end test (CR-005), and the new DifferentInvertTransectWidthValidator with the UUID-serialization bug (CR-001) has no test (CR-006). A submission test would have surfaced CR-001 immediately. Adding the writer test should be the highest-priority follow-up.
- Migration 0116 and the choices endpoint removal are coupled — confirming the table is empty in production and confirming no live client reads invertharvesttypes (CR-003, CR-010) must be done together before deploy, not independently.
- sample_unit_consistency.py and sample_units.py are shared utilities used by all protocols. The UUID-not-stringified bug (CR-001) was introduced into the shared file; reviewing similar context-dict constructions in the other (non-invert) width validators in that file is advisable to catch any inherited defects.
- Mode B (PR URL) was used for this review — local .gitignore/.dockerignore filtering was skipped because the working tree is not the target repo. All 24 changed files were reviewed.
Findings (cross-file / praise)
CR-005 · Warning · test_gap — BeltInvertProtocolWriter.write() has no test
The new submission writer introduces four cooperating methods: get_or_create_invertbelt_transect, get_or_create_beltinvert, create_obsbeltinvert, and write(). None of these are exercised by any test in the diff. Other protocols (fish belt, benthic LIT) have end-to-end submission tests that call the writer and assert resulting DB rows. The macroinvertebrate protocol is missing a parallel test.
Suggestion: Add a submission test analogous to the existing fish belt one. Use
valid_belt_invert_collect_record, call the submission endpoint or invokeBeltInvertProtocolWriter(collect_record, context).write()directly, then assert thatInvertBeltTransect,BeltInvert,ObsBeltInvert, andObserverrecords were created with the expected field values.
Why it matters: The writer integrates four new DB models and two new parser functions. Without an integration test, regressions in any of those layers — wrong field mapping, missing transect, missing observations — will go undetected.
Applies to: src/api/submission/writer.py, src/api/tests/test_validations.py
Found by: reviewer-tests
CR-006 · Warning · test_gap — UniqueInvertBeltTransectValidator and DifferentInvertTransectWidthValidator are untested
Two new record-level validators — UniqueInvertBeltTransectValidator (detects duplicate transect submissions against existing DB records) and DifferentInvertTransectWidthValidator (warns when belt widths differ across the same sample event) — have no test coverage. These validators require pre-existing DB rows to trigger their error or warning path, which the current test suite never sets up for macroinvertebrate.
Suggestion: For
UniqueInvertBeltTransectValidator: submit a valid collect record to create a BeltInvert row, then validate a second collect record with matching site/management/date/number/depth/width/observers and assert ERROR with codeduplicate_invert_belt_transect. ForDifferentInvertTransectWidthValidator: create anInvertBeltTransectwith one width, then validate a collect record targeting the same sample event with a different width ID and assert WARN with codedifferent_transect_width_se.
Why it matters: The duplicate-transect check is a critical data-quality guard. Leaving it untested means a regression in the observer-profile filter could allow duplicate submissions silently. Note the DifferentInvertTransectWidth path also has the UUID serialization bug (CR-001) which a test would have surfaced.
Applies to: src/api/submission/validations/validators/invert_belt_transect.py, src/api/submission/validations/validators/sample_unit_consistency.py, src/api/tests/test_validations.py
Found by: reviewer-tests
CR-010 · Question · api_contract — Was invertharvesttypes ever used by any active API client?
The choices endpoint previously exposed invertharvesttypes to API consumers. Were any mobile or third-party clients relying on that key? If genuinely unused, the cleanup is fully safe. If clients are still reading the key, they will silently receive a response missing that field after deploy. Older client versions in the wild may break without a deprecation window.
Applies to: src/api/resources/choices.py
Found by: reviewer-database
CR-011 · Question · test_gap — invert_group_of_interest_2 fixture is added but never referenced in any test
macroinvertebrate.py gains a new invert_group_of_interest_2 fixture (Crown-of-thorns starfish / COTS), but no test in the diff uses it as a parameter. Is this intended for a follow-up test that was intentionally deferred, or did the test that needs it get accidentally dropped?
Applies to: src/api/tests/fixtures/macroinvertebrate.py
Found by: reviewer-tests
CR-012 · Question · test_gap — InvertSizeValidator boundary value (exactly at 1.5x max_length) is not tested
The validator warns when size > max_length * 1.5. The existing test uses size=50, well above the threshold. There is no test for size == max_length * 1.5 (which should pass) or size == max_length * 1.5 + epsilon (which should warn). The exact boundary is a common location for off-by-one mistakes.
Applies to: src/api/tests/test_validations.py
Found by: reviewer-tests
CR-015 · Info · maintainability — Three collect record fixtures share nearly identical boilerplate
valid_belt_invert_collect_record, valid_belt_invert_collect_record_goi, and valid_belt_invert_collect_record_family are approximately 50 lines each and differ only in the invert_attribute fixture and observation list. The repeated beltinvert_transect/sample_event/observers construction could be extracted into a shared factory helper to reduce maintenance surface.
Applies to: src/api/tests/fixtures/collect_records.py
Found by: reviewer-tests
CR-016 · Info · maintainability — macroinvertebrate_file fixture defined in test module rather than conftest
All other protocol file fixtures (fishbelt_file, benthicpit_file, bleaching_file, etc.) live in a conftest.py, making them available project-wide. The new macroinvertebrate_file is defined inline in test_ingest.py and cannot be shared with any other test module that might need it in the future.
Applies to: src/api/tests/test_ingest.py
Found by: reviewer-tests
CR-017 · Praise · performance — InvertSizeValidator batches all InvertAttribute lookups into a single DB query — avoids N+1
InvertSizeValidator.__call__ collects all unique invert_attribute IDs from the observation list in a set comprehension, then issues one InvertAttribute.objects.select_related(...).filter(pk__in=...) query for all of them. The resulting max_length_lookup dict is built once and consulted in O(1) per check_invert_size call.
Applies to: src/api/submission/validations/validators/invert_obs.py
Found by: reviewer-backend-python, reviewer-database
CR-018 · Praise · maintainability — All protocol dispatch tables correctly updated — no missed registration sites
The PR correctly wires MACROINVERTEBRATE_PROTOCOL into every dispatch point that existing protocols use: ingest/utils.py (ingest()), submission/utils.py (get_writer() and _validate_collect_record()), submission/validations/utils.py (PROTOCOL_SAMPLE_UNIT_MAP and PROTOCOL_SAMPLE_EVENT_PATH), utils/sample_units.py (migrate_collect_record_sample_event()), and models/core.py (CollectRecord.sample_unit property). These registration sites are easy to miss in a protocol addition and the author has covered all of them.
Applies to: src/api/ingest/utils.py, src/api/submission/utils.py, src/api/submission/validations/utils.py, src/api/utils/sample_units.py, src/api/models/core.py
Found by: reviewer-backend-python
CR-019 · Praise · maintainability — Well-structured validation module with clear separation of concerns
The belt_invert.py validation module is comprehensively organised with clear section comments (field-level, row-level, record-level) and appropriate validator composition. The structure mirrors established patterns from other protocols and will be easy to maintain and extend.
Found by: reviewer-python-quality
CR-020 · Praise · maintainability — Robust invert_obs validators with proper null-handling and polymorphic attribute resolution
InvertSizeValidator and InvertSizeBinRequiredValidator properly handle optional/nullable size fields and apply sensible defaults. The polymorphic attribute type resolution (species → genus → family → order → class) is cleanly implemented and handles cases where an attribute might not have a max_length, preventing false-positive size warnings for GOI and family observations.
Found by: reviewer-python-quality
CR-021 · Praise · test_gap — Attribute-rank coverage for InvertSizeValidator is precisely targeted
Adding separate fixtures and tests for species-level, GOI-level, and family-level observations is a precisely targeted set of edge cases. The InvertSizeValidator iterates a fixed priority list of subattr names to find max_length — testing that GOI and family observations do not produce false-positive size warnings directly exercises the two paths where the lookup returns None and the validator must silently pass.
Applies to: src/api/tests/test_validations.py, src/api/tests/fixtures/collect_records.py
Found by: reviewer-tests
Agent Coverage
| Agent | Role | Files | Findings | Blocking |
|---|---|---|---|---|
| reviewer-backend-python | backend | 21 | 8 | 0 |
| reviewer-python-quality | code_quality | 21 | 2 | 0 |
| reviewer-database | database | 3 | 7 | 0 |
| reviewer-tests | testing | 5 | 9 | 0 |
| reviewer-security | security | 24 | 1 | 0 |
Generated by AI Review Team
cdk-nag report✅ No unsuppressed errors. See |
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Summary by CodeRabbit
New Features
Changes
Tests