fix: move measuring_point_height_is_assumed from thing to measuring_p…

[jirhiker]

…oint_history

Why

This PR addresses the following problem / context:

• Use bullet points here

How

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

• Use bullet points here

Notes

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

• Use bullet points here

[Copilot]

Pull request overview

This PR adds a measuring_point_height_is_assumed boolean field to the MeasuringPointHistory table to track whether a measuring point height value was explicitly provided or assumed/defaulted to 0. The changes ensure that water wells always have at least one measuring point history record, with a default value of 0 when no explicit height is provided.

Changes:

• Added measuring_point_height_is_assumed boolean column to MeasuringPointHistory table with database migration
• Modified well creation logic to always create a MeasuringPointHistory record, defaulting to 0 when height is not provided and marking it as assumed
• Updated schemas to include the new field in both request and response models
• Enhanced transfer logic to handle missing measuring point heights and track whether values are assumed

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
alembic/versions/c4d5e6f7a8b9_add_measuring_point_height_is_assumed.py	Database migration to add the new boolean column to measuring_point_history table
db/measuring_point_history.py	Added measuring_point_height_is_assumed boolean field to the model
db/thing.py	Added guard to return 0 when measuring point history is empty; removed blank line
schemas/thing.py	Added measuring_point_height_is_assumed field to CreateWell and WellResponse schemas
services/thing_helper.py	Modified to always create MeasuringPointHistory, defaulting to 0 with is_assumed flag; added NaN handling
services/well_inventory_csv.py	Simplified to pass Pydantic model directly to add_thing
transfers/well_transfer.py	Added logic to handle missing measuring points, track assumed status, and added debugging output; included new field in exclusions list
transfers/util.py	Added helper method to retrieve valid values for a lexicon type

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3e00163070

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

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

[jirhiker]

@marissafichera @ksmuczynski

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

[ksmuczynski]

@jirhiker There are two tables that capture measuring point height: Observation and MeasuringPointHistory.

The MeasuringPointHistory table is designed to be the source of truth. It stores official, surveyed heights from construction or re-survey events. Since updates to this table only happen during significant events (like a new wellhead installation), we want to force the user entering that data to provide the surveyed height immediately to ensure the record is complete. A non-nullable measuring_point_height should be enforced here.

The Observation table is designed to capture routine measurements taken during field events. This table is often messy and much more prone to human error. The measuring_point_height field here SHOULD allow NULLs because it allows the database to honestly reflect that a measurement was missed or overlooked.

A NULL in the Observation table isn't a dead end and does not mean depth to water (below ground surface) can't be calculated. We can use the Observation date to look up the authoritative height from the MeasuringPointHistory table and fill in the blanks during reporting.

My recommendation is to keep the MeasuringPointHistory table as-is and add an observation_reason field to the Observation table to document the reason for a missing value/measurement. I would make observation_reason a nullable, constrained lexicon term (the Notes field would be used to provide any additional context). Some example lexicon terms include: Dry, Obstructed, Destroyed, Equipment Failure, Not Sampled/Measured.

[jirhiker]

@ksmuczynski I agree with everything except that any measuring_point_height should be nullable. defaulting the measuring_point_height to 0 and adding flags in both the Observation and MeasuringPointHistory reduces the complexity of the logic to calculate depth to water bgs. Will you open a jira ticket for adding the observation_reason flag to Observation.

[jirhiker]

@ksmuczynski I think for non legacy data and when implementing the Well Inventory/Water Level forms we should very much consider

" we want to force the user entering that data to provide the surveyed height immediately to ensure the record is complete. A non-nullable measuring_point_height should be enforced here."

[jirhiker]

@ksmuczynski upon further consideration Im going to pass this off to you and let you move forward with your recommendation. I will close this PR as Ive started to introduce scope creep

[ksmuczynski]

Sure thing, I'll get a Jira ticket opened for the observation_reason field and lexicon terms today.

Regarding the 0 default for the measuring point height: I’m hesitant to go that route because, in the field, a height of 0 actually describes a specific well type (flush mount). If we default to 0 during a bulk CSV upload, we'll lose the ability to distinguish between a flush mount well and a forgotten measurement. This risks introducing a small elevation error that would be invisible to anyone reviewing the data later.

To keep the logic simple for reporting without losing data integrity, what if we keep the database field (Observation.measuring_point_height) Nullable but handle the 'fallback' logic in a Calculated View or a SQLAlchemy hybrid property? This creates a 'virtual' non-nullable column for the app to query, and we don't have to deal with NULL math or complex joins in the code.

For our current CLI tool, we could add a Pydantic validator. The script will expect a height, but if that's missing, it will require an observation_reason to proceed. This fails fast during the import if the data is incomplete, which is safer than letting a default 0 slip through and silently corrupt our elevation models.

When we eventually move to web forms, we can have the UI auto-populate the height from the MeasuringPointHistory table. This keeps the raw data honest for the scientists, but the API and CLI stay 'easy' for developers. What do you think?