🚨 ⚠️ 💀 [NO TICKET]: Well inventory csv Feature Tests #247

jirhiker · 2025-11-14T22:03:32Z

🚨 ⚠️ 💀 DO NOT MERGE UNTIL READY TO RERUN TRANSFER

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

…arios

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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".

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

# Conflicts: # core/initializers.py

…ling for unsupported file types

codecov-commenter · 2025-11-14T22:57:23Z

Codecov Report

❌ Patch coverage is 43.57035% with 373 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
api/well_inventory.py	14.63%	210 Missing ⚠️
schemas/well_inventory.py	61.57%	83 Missing ⚠️
services/thing_helper.py	6.75%	69 Missing ⚠️
schemas/thing.py	65.00%	7 Missing ⚠️
api/lexicon.py	33.33%	2 Missing ⚠️
schemas/__init__.py	71.42%	2 Missing ⚠️

Files with missing lines	Coverage Δ
constants.py	`100.00% <100.00%> (ø)`
core/enums.py	`100.00% <100.00%> (ø)`
core/initializers.py	`88.23% <100.00%> (+0.28%)`	⬆️
db/__init__.py	`98.03% <100.00%> (ø)`
db/aquifer_system.py	`100.00% <100.00%> (ø)`
db/contact.py	`100.00% <100.00%> (ø)`
db/data_provenance.py	`93.33% <100.00%> (ø)`
db/geologic_formation.py	`100.00% <100.00%> (ø)`
db/group.py	`100.00% <100.00%> (ø)`
db/permission_history.py	`95.83% <100.00%> (ø)`
... and 17 more

…roved consistency

…d error handling

…or reporting

api/well_inventory.py

… error handling, and SRID support

jacob-a-brown

Because a number of updates/changed from BDMS-221 and BDMS-227 will influence the import of these data should we wait for those two to get merged before this PR? So that necessary updates/refactors can occur

api/well_inventory.py

…l details

…dation and error handling

chasetmartin

This is looking great. The updates to the feature file based on AMP feedback should be relatively straightforward: 2 possible contacts and a few additional notes fields.

…ved location handling

…alidations

…in well inventory processing

…in CSV import

jirhiker

api/well_inventory.py

+        content={
+            "validation_errors": validation_errors,
+            "summary": {
+                "total_rows_processed": rows_processed,
+                "total_rows_imported": rows_imported,
+                "validation_errors_or_warnings": rows_with_validation_errors_or_warnings,
+            },
+            "wells": wells,
+        },


To fix the problem, we should prevent potentially sensitive content from str(e) (the caught exception) from being returned to the end user as part of the API response. Internal details should only be logged on the server for diagnostics. Instead, return only a generic, non-sensitive error message in the API response. Concretely, in api/well_inventory.py, update the except DatabaseError as e: block (lines 223–233) to:

Log the error details (using a logger, e.g. Python's logging module).

Append a generic error message to validation_errors, e.g. "A database error occurred while importing this row."
Add the required logging setup at the top of the file if not already present (import and basic configuration for the logging module).

the feature file only tests that the function runs without error, this commit adds tests to verify that the data is actually saved correctly in the database.

Copilot

Pull request overview

This PR adds comprehensive feature tests for the well inventory CSV upload functionality, focusing on database content verification after import. The tests validate that the API correctly processes well inventory uploads and creates appropriate database records.

Changes:

Added new well inventory CSV feature test suite with database content validation
Removed is_suitable_for_datalogger field from Thing model, replaced with status history approach
Updated contact transfer logic to support notes and improved data exclusion patterns

Reviewed changes

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

Show a summary per file

File	Description
tests/test_well_inventory.py	New comprehensive test suite validating database contents after well inventory CSV upload
tests/features/well-inventory-csv.feature	New Gherkin feature file defining well inventory upload scenarios and validation rules
tests/features/steps/well-inventory-csv.py	Step definitions implementing well inventory CSV upload test scenarios
tests/features/steps/well-inventory-csv-given.py	Given step definitions for setting up well inventory CSV test data files
tests/features/steps/well-inventory-csv-validation-error.py	Step definitions for validation error assertions in well inventory tests
tests/features/data/well-inventory-*.csv	Test data files for various well inventory upload scenarios
transfers/well_transfer.py	Removed `is_suitable_for_datalogger` field handling and cleaned up field list
transfers/contact_transfer.py	Updated to use `model_dump(exclude=...)` pattern instead of manual pop operations
db/thing.py	Removed `is_suitable_for_datalogger` column, added `open_status` and `datalogger_suitability_status` properties
schemas/thing.py	Added `is_open` field, updated well creation/response schemas
services/thing_helper.py	Enhanced to handle status history, monitoring frequencies, alternate IDs, and notes
api/well_inventory.py	New endpoint implementation for well inventory CSV upload with validation
schemas/well_inventory.py	New schema defining well inventory row structure with field validators

transfers/well_transfer.py

tests/transfers/test_contact_with_multiple_wells.py

schemas/well_inventory.py

api/well_inventory.py

services/thing_helper.py

These fields are in the status history table, not the thing table. the same fields should be excluded from both sequence and paralell transfers

Copilot

Pull request overview

Copilot reviewed 80 out of 84 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

transfers/well_transfer.py:1

Duplicate entries for "well_depth_source", "well_completion_date_source", and "well_construction_method_source" in the exclusion list. Remove the duplicate entries on lines 342-344 to avoid confusion.

# ===============================================================================

transfers/well_transfer.py:1

Duplicate entries for "well_depth_source", "well_completion_date_source", and "well_construction_method_source" in the exclusion list. Remove the duplicate entries on lines 724-726 to avoid confusion.

# ===============================================================================

tests/features/environment.py

core/lexicon.json

tests/features/well-inventory-csv.feature

jacob-a-brown · 2026-02-03T23:18:34Z

@jirhiker @TylerAdamMartinez @chasetmartin @ksmuczynski

I just completed a fair bit of work to resolve merge conflicts from staging; a lot of work was done in staging since we last worked on the well inventory csv. In doing so I found a few errors and added some new features. Almost all of the features/work in this PR was vetted previously, but with these merge conflicts (and some fixes - see below) I think that a final/an additional review is warranted.

@jirhiker I updated the well transfer script to put the datalogger installation and open statuses into the status_history table. I also realized that the same fields need to be excluded in both _step and step_parallel when dumping CreateWell so I put them into a shared list at the top of the script.

Fixed Errors

Monitoring frequencies were not persisted as expected. They are now added when a well is added.
Some incorrect headers/field names were used in the endpoint (just the notes). Those have now been corrected.
Historic depth to water notes were not written as expected.
MG-043 should be used in the test tests/transfers/test_contact_with_multiple_wells::test_owner_comment_creates_notes_for_primary_only since that well has two contacts

New Work

I added tests/test_well_inventory.py to ensure that the data is being persisted as expected. The test tests/features/well-inventory-csv.feature only tested that the correct data was accepted and that the responses were correct, but did not test if the data was being written to the DB as expected.
Now that we are using alembic I added a script to remove is _suitable_for_datalogger from the thing model since that is now persisted in the status_history table.

Areas of Particular Interest

Most of this work was done before our pivot to the admin view and 1:1 imports. I tried to resolve merge conflicts and address these new features in a judicious manner, but I may have missed something.

Outstanding Questions

Should the contact notes being added to the first and second contact(s), or just the first?

Add 33 new tests to improve coverage of well inventory CSV functionality: Error Handling Tests (17): - Invalid file type, empty file, headers only - Duplicate columns/well IDs, missing required fields - Invalid date/numeric/email/phone formats - Invalid UTM coordinates, lexicon values, boolean values - Missing contact type/role, partial water level fields - Non-UTF8 encoding Unit Tests for Helper Functions (11): - _make_location() with UTM zones 13N and 12N - _make_contact() with full info and empty name - _make_well_permission() success and error cases - generate_autogen_well_id() various scenarios - AUTOGEN_REGEX pattern matching API Edge Case Tests (6): - Too many rows (>2000) - Semicolon and tab delimiters - Duplicate header row in data - Valid CSV with comma in quoted fields - Non-numeric well ID suffix handling Coverage improvement: - api/well_inventory.py: 14% → 68% - schemas/well_inventory.py: 61% → 90% - Total: 43% → 79% Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

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

Copilot · 2026-02-04T18:20:20Z

transfers/tester.py

+df = read_csv("WellData")
+wells = get_transferable_wells(df)
+print(len(wells))
+
+mp = wells[wells["MPHeight"].notna()]
+print(len(mp))
+


This appears to be a test/debug script that should not be committed to the repository. Consider removing it or moving it to a dedicated scripts/debugging directory.

Suggested change

df = read_csv("WellData")

wells = get_transferable_wells(df)

print(len(wells))

mp = wells[wells["MPHeight"].notna()]

print(len(mp))

def analyze_transferable_wells(csv_name: str = "WellData") -> tuple[int, int]:

"""

Analyze transferable wells from the given CSV source.

Parameters

----------

csv_name : str, optional

The name or path of the CSV data source to read. Defaults to "WellData".

Returns

-------

tuple[int, int]

A tuple containing:

- the total number of transferable wells

- the number of transferable wells with a non-null MPHeight value

"""

df = read_csv(csv_name)

wells = get_transferable_wells(df)

mp = wells[wells["MPHeight"].notna()]

return len(wells), len(mp)

def main() -> None:

"""

Entry point for manual execution.

Reads the default well data source, computes transferable wells and those

with MPHeight defined, and prints their counts.

"""

total_wells, mp_wells = analyze_transferable_wells()

print(total_wells)

print(mp_wells)

if __name__ == "__main__":

main()

Copilot · 2026-02-04T18:20:20Z

api/well_inventory.py

+                row["well_name_point_id"] = well_id
+
+            if well_id in seen_ids:
+                print(seen_ids)


Debug print statement should be removed or replaced with proper logging.

Add missing Request and HasOne imports that were causing NameError during test collection. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

- Add test for validation error structure consistency (row, field, error keys) - Add test for SQLAlchemy and_() query pattern correctness Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

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

Copilot · 2026-02-04T18:32:29Z

services/util.py

+from zoneinfo import ZoneInfo
+from datetime import datetime


Import statements should be organized according to PEP 8: standard library imports first, then third-party libraries, then local imports. The datetime import should come before zoneinfo alphabetically within the standard library group.

Suggested change

from zoneinfo import ZoneInfo

from datetime import datetime

from datetime import datetime

from zoneinfo import ZoneInfo

Copilot · 2026-02-04T18:32:29Z

db/group.py

    )

+    __table_args__ = (
+        UniqueConstraint("name", "group_type", name="uq_group_name_group_type"),


Changing from a unique constraint on name alone to a composite unique constraint on (name, group_type) is a breaking change. This means groups with the same name can now exist if they have different types. Ensure this is intentional and that any existing code expecting unique names across all groups is updated accordingly.

Suggested change

UniqueConstraint("name", "group_type", name="uq_group_name_group_type"),

UniqueConstraint("name", name="uq_group_name"),

kbighorse · 2026-02-04T18:36:14Z

Triggering CI re-run after import fix

kbighorse

I added more test coverage.
If the new merge conflict is resolved, tests pass, and it survives a test transfer, I approve.

kbighorse · 2026-02-04T18:38:44Z

transfers/permissions_transfer.py

@@ -1,5 +1,6 @@
 from datetime import datetime

+


question: this should violate linting, no?

jirhiker added 3 commits November 11, 2025 20:40

feat: implement BDD steps for well inventory CSV upload and validation

c0f77a6

feat: add BDD steps and validation for well inventory CSV upload

168dda0

feat: add additional BDD steps for well inventory CSV validation scen…

f3b39d9

…arios

jirhiker marked this pull request as draft November 14, 2025 22:03

chatgpt-codex-connector bot reviewed Nov 14, 2025

View reviewed changes

tests/features/steps/well-inventory-csv.py Outdated Show resolved Hide resolved

jirhiker added 2 commits November 14, 2025 15:08

Merge branch 'refs/heads/staging' into well-inventory-csv

85ec7fd

# Conflicts: # core/initializers.py

fix: refactor CSV handling in well-inventory steps and add error hand…

bd2dc36

…ling for unsupported file types

jirhiker requested review from TylerAdamMartinez, chasetmartin, jacob-a-brown and ksmuczynski November 14, 2025 22:56

jirhiker added 3 commits November 14, 2025 16:48

fix: update well inventory CSV validation to use context.rows for imp…

69c2b03

…roved consistency

feat: implement well inventory CSV upload endpoint with validation an…

6d1e55c

…d error handling

fix: update well inventory CSV handling to improve validation and err…

77d6283

…or reporting

github-advanced-security bot found potential problems Nov 15, 2025

View reviewed changes

api/well_inventory.py Fixed Show fixed Hide fixed

feat: enhance well inventory CSV processing with improved validation,…

897286c

… error handling, and SRID support

jacob-a-brown reviewed Nov 17, 2025

View reviewed changes

api/well_inventory.py Outdated Show resolved Hide resolved

api/well_inventory.py Outdated Show resolved Hide resolved

api/well_inventory.py Outdated Show resolved Hide resolved

api/well_inventory.py Outdated Show resolved Hide resolved

api/well_inventory.py Outdated Show resolved Hide resolved

jirhiker and others added 3 commits November 17, 2025 14:22

feat: expand well inventory CSV model with additional contact and wel…

96220b4

…l details

Merge branch 'staging' into well-inventory-csv

a9bd8ed

feat: refactor well inventory CSV processing with improved model vali…

94feba4

…dation and error handling

jirhiker changed the title ~~Well inventory csv Feature Tests~~ NO TICKET: Well inventory csv Feature Tests Nov 18, 2025

chasetmartin reviewed Nov 18, 2025

View reviewed changes

jirhiker added 6 commits November 18, 2025 20:05

Merge branch 'refs/heads/staging' into well-inventory-csv

a26217c

refactor: enhance well inventory processing with new models and impro…

c9a9cba

…ved location handling

refactor: streamline well inventory data handling and enhance model v…

594888a

…alidations

refactor: enhance contact role validation and improve error handling …

e5ef68d

…in well inventory processing

refactor: enhance contact role validation and improve error handling …

75f57ac

…in well inventory processing

refactor: improve error handling and streamline well data processing …

ae06bff

…in CSV import

github-advanced-security bot found potential problems Nov 20, 2025

View reviewed changes

jacob-a-brown added 4 commits February 2, 2026 16:20

feat: add more detailed notes about well inventory

4d4c02a

fix: remove outdated note

1f6676b

fix: create monitoring frequencies for things

50970db

feat: test that data is persisted as expected for well inventory

da7a88c

the feature file only tests that the function runs without error, this commit adds tests to verify that the data is actually saved correctly in the database.

jacob-a-brown requested review from chasetmartin, Copilot and jacob-a-brown February 3, 2026 22:06

Copilot AI reviewed Feb 3, 2026

View reviewed changes

jacob-a-brown added 3 commits February 3, 2026 15:17

fix: cleanup well inventory pytest

60fc69e

feat: test contact notes

8633977

feat: add datalogger/open status to well transfer & fix exclusions

bbfa939

These fields are in the status history table, not the thing table. the same fields should be excluded from both sequence and paralell transfers

Copilot AI review requested due to automatic review settings February 3, 2026 23:11

Copilot AI reviewed Feb 3, 2026

View reviewed changes

tests/features/environment.py Show resolved Hide resolved

core/lexicon.json Show resolved Hide resolved

tests/features/well-inventory-csv.feature Outdated Show resolved Hide resolved

jacob-a-brown added 4 commits February 3, 2026 16:36

fix: remove duplicate lexicon values

e237672

fix: fix typo

311917f

fix: fix spelling typo in note

2d4dfd7

fix: remove duplicate excluded fields from well transfer

4f751a3

jacob-a-brown requested a review from kbighorse February 4, 2026 17:11

Copilot AI review requested due to automatic review settings February 4, 2026 18:19

kbighorse and others added 2 commits February 4, 2026 10:19

Formatting changes

e5be6af

Copilot AI reviewed Feb 4, 2026

View reviewed changes

fix: add missing imports to ChemistrySampleInfoAdmin

6794d81

Add missing Request and HasOne imports that were causing NameError during test collection. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings February 4, 2026 18:31

Add regression tests for well inventory validation and query patterns

ec956ea

- Add test for validation error structure consistency (row, field, error keys) - Add test for SQLAlchemy and_() query pattern correctness Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Copilot AI reviewed Feb 4, 2026

View reviewed changes

kbighorse approved these changes Feb 4, 2026

View reviewed changes

transfers/permissions_transfer.py

@@ -1,5 +1,6 @@

from datetime import datetime

Copy link

Contributor

kbighorse Feb 4, 2026

Choose a reason for hiding this comment

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

question: this should violate linting, no?

Merge branch 'staging' into well-inventory-csv

43c53e7

@@ -17,6 +17,7 @@
             from io import StringIO
             from itertools import groupby
             from typing import Set
+            import logging
             from fastapi import APIRouter, UploadFile, File
             from fastapi.responses import JSONResponse
@@ -221,13 +222,12 @@
                             if added:
                                 session.commit()
                         except DatabaseError as e:
+                            logging.error(f"Database error while importing row '{model.well_name_point_id}': {e}")
                             validation_errors.append(
                                 {
-                                    {
-                                        "row": model.well_name_point_id,
-                                        "field": "Database error",
-                                        "error": str(e),
-                                    }
+                                    "row": model.well_name_point_id,
+                                    "field": "Database error",
+                                    "error": "A database error occurred while importing this row.",
                                 }
                             )
                             continue

-df = read_csv("WellData")
-wells = get_transferable_wells(df)
-print(len(wells))
-mp = wells[wells["MPHeight"].notna()]
-print(len(mp))
+def analyze_transferable_wells(csv_name: str = "WellData") -> tuple[int, int]:
+    """
+    Analyze transferable wells from the given CSV source.
+    Parameters
+    ----------
+    csv_name : str, optional
+        The name or path of the CSV data source to read. Defaults to "WellData".
+    Returns
+    -------
+    tuple[int, int]
+        A tuple containing:
+        - the total number of transferable wells
+        - the number of transferable wells with a non-null MPHeight value
+    """
+    df = read_csv(csv_name)
+    wells = get_transferable_wells(df)
+    mp = wells[wells["MPHeight"].notna()]
+    return len(wells), len(mp)
+def main() -> None:
+    """
+    Entry point for manual execution.
+    Reads the default well data source, computes transferable wells and those
+    with MPHeight defined, and prints their counts.
+    """
+    total_wells, mp_wells = analyze_transferable_wells()
+    print(total_wells)
+    print(mp_wells)
+if __name__ == "__main__":
+    main()

	UniqueConstraint("name", "group_type", name="uq_group_name_group_type"),
	UniqueConstraint("name", name="uq_group_name"),

🚨 ⚠️ 💀 [NO TICKET]: Well inventory csv Feature Tests #247

Are you sure you want to change the base?

🚨 ⚠️ 💀 [NO TICKET]: Well inventory csv Feature Tests #247

Uh oh!

Conversation

jirhiker commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

How

Notes

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

codecov-commenter commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

jacob-a-brown left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chasetmartin left a comment

Choose a reason for hiding this comment

Uh oh!

Check warning

Uh oh!

Copilot Autofix

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacob-a-brown commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed Errors

New Work

Areas of Particular Interest

Outstanding Questions

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

jirhiker commented Nov 14, 2025 •

edited

Loading

codecov-commenter commented Nov 14, 2025 •

edited

Loading

jacob-a-brown commented Feb 3, 2026 •

edited

Loading