BDMS-626: Improve validation, schema alignment, and well inventory handling

[ksmuczynski]

Why

This PR addresses the following problem / context:

• Database mapping was incomplete: public_availability_acknowledgement, monitoring_status, well/water notes, and water level observations were not being persisted correctly.
• Several schema fields were overly strict, requiring values that should be optional, using plain strings where lexicon enums were expected, and rejecting valid CSVs too aggressively.
• WellInventoryRow schema now integrates directly with lexicon-based enums to improve consistency between CSV ingestion and database persistence.
• Real-world imports exposed database failures that were hard to diagnose, especially around contact organization foreign keys and empty-string values being written into lexicon-backed columns.
• Real-world source files also included monitoring_frequency values like Complete that do not map cleanly to the current lexicon and needed importer-side normalization.
• Valid CSVs saved with a UTF-8 BOM could be misread at the header level, causing required fields like project to appear missing.
• BDD tests suffered from primary key conflicts and outdated test data that no longer aligned with the importer’s validation and best-effort behavior.
• Previously, a single malformed row in a CSV would abort the entire import process. Users need "best-effort" logic where valid data is saved while invalid rows are flagged.

How

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

• Database mapping:

  • public_availability_acknowledgement now maps to Location.release_status (True → public, False → private, unset → draft).
  • monitoring_status is now written to the StatusHistory table.
  • well_notes and water_notes are now stored as polymorphic notes on the Thing.
  • Providing water-level data now creates the expected Sample and Observation records and attaches them to the correct field activity.
  • Water-level observation field names were aligned with the actual database model.

• WellInventoryRow schema:

  • Made site_name, elevation_ft, elevation_method, monitoring_point_height_ft, and depth_to_water_ft optional.
  • Replaced plain str types with lexicon-based enums for elevation_method, depth_source, well_pump_type, monitoring_status, sample_method, and data_quality.
  • Added a flexible_lexicon_validator for case-insensitive, whitespace-tolerant matching.
  • Relaxed contact validation to require only one of contact_name or contact_organization.
  • Made water_level_date_time required only when depth_to_water_ft is provided.
  • Normalized blank values like depth to water, contact organization, and well status to None instead of persisting invalid empty strings.
  • Added regression coverage for blank depth_to_water_ft and blank lexicon-backed contact/status fields.

• Best-effort logic:

  • Implemented row-level atomic savepoints so individual row failures no longer abort the full import.
  • Failed rows are logged with 1-based row numbers and well IDs in a validation_errors list.
  • Fixed an UnboundLocalError caused by auto-generated well IDs.
  • Updated error reporting to surface actual database-level failures rather than collapsing them to a generic database error.
  • Added commit=False support so services/thing_helper.py and services/contact_helper.py participate correctly in the outer best-effort transaction without prematurely committing.
  • Made reruns idempotent by detecting previously imported rows and skipping duplicate record creation instead of failing on unique constraints.

• Lexicon and import compatibility:

  • Added missing organization terms required by real-user imports so contact organization foreign keys can resolve correctly.
  • Updated monitoring_status enum construction to use the correct lexicon category.
  • Added BOM-safe CSV decoding so UTF-8 BOM headers are parsed correctly during CLI import.
  • Normalized monitoring_frequency = Complete source values so they do not create a monitoring frequency record and instead set monitoring_status to Not currently monitored.
  • Added unit and BDD coverage for the Complete monitoring frequency normalization behavior.

• BDD test suite:

  • Updated test CSVs in tests/features/data/ to use valid lexicon terms and properly quoted comma-containing values.
  • Added scenario-based unique ID suffixing in Given steps to isolate tests and prevent primary key conflicts.
  • Updated well-inventory-csv.feature to assert partial success (e.g., "1 well is imported") in negative scenarios where best-effort behavior is expected.
  • Aligned validation expectations with the importer’s current detailed database error reporting.
  • All 44 well inventory BDD scenarios now pass when the test database is rebuilt against the current branch state.

Notes

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

• Further enhancements may be required for complete schema validation coverage.
• Additional real-user CSV validation coverage may be scoped in a follow-up PR focused on ingestion of real user-entered data.
• Lexicon matching is intentionally lenient at ingestion time (case-insensitive, whitespace-stripped), but values must still resolve to a known lexicon term before persistence.
• Because contact organizations remain lexicon-backed, new real-world organizations still require lexicon seeding in the target database before imports will succeed.
• Test isolation is achieved via per-scenario ID suffixing in the BDD setup; if the shared test database strategy changes in the future, this workaround may no longer be necessary.
• BDD runs on this branch may require rebuilding ocotilloapi_test after schema or lexicon changes to avoid stale test database state.

[Copilot]

Pull request overview

This PR updates the well-inventory CSV ingestion pipeline to better align schema validation with lexicon-backed enums, persist previously-missed fields (notes, monitoring/public availability, water level observations), and change the import behavior to “best-effort” so individual row failures don’t abort the entire upload.

Changes:

• Added row-level savepoints and improved error reporting so invalid rows are skipped while valid rows are persisted.
• Updated WellInventoryRow schema to relax/adjust requirements and introduce lexicon-backed enum parsing plus CSV field aliases.
• Updated BDD/unit tests and test CSV fixtures to reflect new validation rules and partial-success behavior.

Reviewed changes

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

Show a summary per file

File	Description
services/well_inventory_csv.py	Best-effort import via nested savepoints; mapping updates for release status, notes, monitoring status, and water level observation persistence
schemas/well_inventory.py	Schema optionality updates, lexicon enum coercion, contact + water level validation adjustments, and alias handling
services/thing_helper.py	Adds monitoring status history writes; adjusts commit/rollback behavior for outer-transaction support
services/contact_helper.py	Adjusts commit/rollback behavior for outer-transaction support
cli/service_adapter.py	Improves exit code and stderr reporting for partial success and validation failures
tests/test_well_inventory.py	Adds schema alias tests and helper row builder
tests/features/well-inventory-csv.feature	Updates expected partial-success outcomes in negative scenarios
tests/features/steps/*.py	Updates step definitions for partial success and loosens validation-error matching
tests/features/data/*.csv	Refreshes fixture data to match new lexicon/validation expectations
core/lexicon.json	Adds a new note_type term

[ksmuczynski]

@jirhiker Recent updates to the CLI command tests are failing on my branch. Since these changes don't seem to impact the well inventory ingestion, should I resolve the test failures within this PR, or would you prefer I handle them in a separate one before re-opening for review?

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

The idempotency shortcut returns existing_well.name, which gets appended to wells and counted in total_rows_imported. On reruns this reports rows as “imported” even though nothing new was created, which is misleading for CLI summary/metrics. Consider returning None (and optionally adding a warning entry) when a row is detected as already imported so total_rows_imported reflects newly-created records only.

Suggested change

	return existing_well.name
	logging.info(
	"Well '%s' already imported; skipping creation for idempotent CSV row",
	existing_well.name,
	)
	return None

[Copilot]

This step expects the error text "contact_1_role is required when contact fields are provided", but the WellInventoryRow validator raises "... when contact data is provided". Because _handle_validation_error matches by substring, this wording mismatch will prevent the step from matching the actual validation error. Update the expected string to match the current validator message (or use a shorter substring that’s stable).

[Copilot]

The assertion message here only echoes the numeric exit code, which doesn’t help diagnose failures. Consider including context.cli_result.stderr (and/or .stdout) so a failing non-zero exit assertion shows the underlying CLI error output.

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

services/well_inventory_csv.py:796

• model.completion_source is now an OriginType enum (see WellInventoryRow), but CreateWell.well_completion_date_source is typed as str | None. Passing the enum object here can serialize to something like OriginType.<name> instead of the underlying lexicon term, causing incorrect persistence or FK failures. Convert to the enum .value (or change CreateWell.well_completion_date_source to OriginType) before calling CreateWell(...) for consistency with the other enum-backed fields.

        well_completion_date=model.date_drilled,
        well_completion_date_source=model.completion_source,
        well_pump_type=model.well_pump_type,

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

services/well_inventory_csv.py:803

• WellInventoryRow.completion_source is now an OriginType enum, but CreateWell.well_completion_date_source is typed as str | None and is later written to DataProvenance.origin_type in add_thing. Passing the enum through here risks persisting the enum repr (e.g., OriginType.<member>) instead of the lexicon term. Convert to the enum's .value (or None) before building CreateWell, similar to how other lexicon-backed fields are normalized above (e.g., elevation_method, depth_source).

        well_completion_date=model.date_drilled,
        well_completion_date_source=model.completion_source,
        well_pump_type=model.well_pump_type,

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

tests/features/steps/well-inventory-csv.py:1

• select is used but not imported in this file (at least in the provided diff context). Add from sqlalchemy import select (or equivalent) near the top so this step doesn’t raise NameError.

[Copilot]

When elevation_ft is missing, this writes an elevation of 0.0 meters, which is a real value and can be indistinguishable from valid near-sea-level elevations. Prefer persisting None (or omitting the field) when elevation is absent; if the database requires a non-null elevation, consider enforcing it as required at the schema level instead of silently defaulting to 0.

Suggested change

	convert_ft_to_m(float(elevation_ft)) if elevation_ft is not None else 0.0
	convert_ft_to_m(float(elevation_ft)) if elevation_ft is not None else None

[Copilot]

This changes add_thing(commit=False) behavior: previously the function refreshed thing (and its notes) even when only flushing. If any callers rely on refreshed relationships/attributes after commit=False, they may now observe stale/incomplete state. Consider restoring session.refresh(thing) (and any required refreshes) after session.flush() as well, while keeping the conditional rollback behavior.

[Copilot]

WellStatusField typed as MonitoringStatus is semantically confusing and makes it hard to reason about which lexicon category is intended for well status vs monitoring status. Consider introducing a dedicated enum/type alias (e.g., WellStatus or StatusValue) and use that for well_status, leaving MonitoringStatus for monitoring-specific status.

Suggested change

	WellStatusField: TypeAlias = Annotated[
	Optional[MonitoringStatus],
	BeforeValidator(flexible_lexicon_validator(MonitoringStatus)),
	WellStatus: TypeAlias = MonitoringStatus
	WellStatusField: TypeAlias = Annotated[
	Optional[WellStatus],
	BeforeValidator(flexible_lexicon_validator(WellStatus)),

[Copilot]

On unexpected exceptions this returns early without explicitly rolling back the session. Since the function is operating on an open SQLAlchemy session (and may have pending/failed nested transactions), explicitly calling session.rollback() before returning helps ensure the connection is returned to the pool cleanly and avoids “transaction is aborted” issues in subsequent operations within the same context.

Suggested change

	logging.exception("Unexpected error in _import_well_inventory_csv")
	logging.exception("Unexpected error in _import_well_inventory_csv")
	session.rollback()

[Copilot]

Returning a truthy value for already-imported rows makes the importer treat “skipped” rows as “imported” (e.g., imported_count += 1 and total_rows_imported can include duplicates on reruns). Consider returning a sentinel (e.g., None) or a structured result that distinguishes created vs skipped, so summaries and progress reporting remain accurate while still surfacing which well IDs were detected as duplicates.

Suggested change

	return existing_well.name
	logging.info(
	"Skipping import for existing well '%s' (id=%s)", existing_well.name, getattr(existing_well, "id", None)
	)
	return None