Skip to content

BDMS 626: mp height#610

Merged
ksmuczynski merged 7 commits intokas-well-BDMS-626-inventory-ingestion-updates_v2from
jab-bdms-626-mp-height
Mar 18, 2026
Merged

BDMS 626: mp height#610
ksmuczynski merged 7 commits intokas-well-BDMS-626-inventory-ingestion-updates_v2from
jab-bdms-626-mp-height

Conversation

@jacob-a-brown
Copy link
Contributor

…ft for non-null observations

Either of these should be required when a non-null observation is being added to the DB so that dtw bgs can be calculated

Why

This PR addresses the following problem / context:

  • either measuring_point_height_ft or mp_height should be required if there is a non-null observation
  • if bother are provided they should correspond

How

Implementation summary - the following was changed / added / removed:

  • raise errors if mp_height != measuring_point_height_ft
  • default to measuring_point_height_ft, otherwise use mp_height, for Observation and MeasuringPointHistory
  • add numerous unit tests

Notes

Any special considerations, workarounds, or follow-up work to note?

  • Use bullet points here

…ft for non-null observations

Either of these should be required when a non-null observation is being added to the DB
so that dtw bgs can be calculated
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds validation and normalization around measuring point height fields during well inventory CSV import so groundwater-level observations can reliably compute DTW/BGS, and extends unit test coverage for the new rules.

Changes:

  • Validate that mp_height and measuring_point_height_ft do not conflict when both are provided.
  • Normalize a single universal_mp_height value (prefer measuring_point_height_ft, fallback to mp_height) for well creation and observations.
  • Add tests covering precedence, blank values, conflict errors, and required-height behavior when depth-to-water is provided.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
services/well_inventory_csv.py Adds mp-height conflict validation, derives a unified height value, enforces requirement when depth_to_water_ft is present, and uses unified height for CreateWell + Observation.
tests/test_well_inventory.py Updates existing fixtures to avoid new conflicts and adds targeted tests for mp-height selection and validation failures.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

@ksmuczynski ksmuczynski left a comment

Choose a reason for hiding this comment

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

Very minor comments, none of which are required to be addressed. One thought I had was about the BDD tests and feature files - should they be updated to reflect these changes, too? This is the only reason I am requesting changes and not approving.

Copy link
Contributor

@ksmuczynski ksmuczynski left a comment

Choose a reason for hiding this comment

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

APproved!

@ksmuczynski ksmuczynski merged commit 6fe2bc1 into kas-well-BDMS-626-inventory-ingestion-updates_v2 Mar 18, 2026
5 checks passed
@ksmuczynski ksmuczynski deleted the jab-bdms-626-mp-height branch March 18, 2026 22:15
ksmuczynski added a commit that referenced this pull request Mar 19, 2026
…6-mp-height"

- This revert removes the conflict check between mp_height and measuring_point_height_ft
- Removes the requirement that one of those heights be present for non-null DTW observations
- Removes the importer’s use of the merged universal_mp_height logic
- Removes  all of the new mp-height-specific tests
- This reverts commit 6fe2bc1, reversing
changes made to 6fb61cf.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants