🚨 ⚠️ 💀 [NO TICKET]: Well inventory csv Feature Tests by jirhiker · Pull Request #247 · DataIntegrationGroup/OcotilloAPI

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

🚨 ⚠️ 💀 DO NOT MERGE UNTIL VERIFYING DB SCHEMA IS COMPATIBLE WITH THIS PR

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

# 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

… 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

…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

…odels

…ing and validation

Copilot

Pull request overview

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

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 82 out of 86 changed files in this pull request and generated 6 comments.

Copilot · 2026-02-07T21:15:46Z

+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ===============================================================================
+from transfers.util import get_transferable_wells, read_csv


This appears to be an ad-hoc analysis script committed under the production transfers/ package. If it’s intended for one-off/interactive use, consider moving it to a scripts/ (or similar) directory to avoid it being treated as library code, and to reduce long-term maintenance surface in the transfers module.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…ing and validation

Copilot

Pull request overview

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

Copilot · 2026-02-07T21:29:39Z

+    assert "datalogger_installation_status" in context.water_well_data
+    assert "open_status" in context.water_well_data
    assert (
-        context.water_well_data["is_suitable_for_datalogger"]
-        == context.objects["wells"][0].is_suitable_for_datalogger
+        context.water_well_data["datalogger_installation_status"]
+        == context.objects["wells"][0].datalogger_installation_status


This step asserts datalogger_installation_status, but the rest of the PR (e.g. Thing.datalogger_suitability_status property and OGC feature mapping) uses datalogger_suitability_status. Unless the API/ORM actually exposes datalogger_installation_status, this will fail. Align the test to the implemented field/property name.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Copilot · 2026-02-07T23:00:24Z

-                    "well_construction_method_source",
-                ]
-            )
+            well_data = data.model_dump(exclude=EXCLUDED_FIELDS)


pydantic.BaseModel.model_dump(exclude=...) expects a set/dict include-exclude structure (Pydantic v2 IncEx), not a list. Passing a list can raise a type error depending on the runtime path. Consider defining EXCLUDED_FIELDS as a set[str] (or using exclude=set(EXCLUDED_FIELDS)) to ensure compatibility.

Suggested change

well_data = data.model_dump(exclude=EXCLUDED_FIELDS)

well_data = data.model_dump(exclude=set(EXCLUDED_FIELDS))

Copilot · 2026-02-07T23:00:25Z

+# router = APIRouter(prefix="/well-inventory-csv")
+
+
+# @router.post("")


The entire API implementation is commented out, so /well-inventory-csv will not be registered and the Behave tests that POST to that endpoint will fail. Implement and export the FastAPI router (or delete the feature/steps that assume it exists) so the endpoint is actually available.

Copilot · 2026-02-07T23:00:25Z

 class Group(Base, AutoBaseMixin, ReleaseMixin):
    # --- Column Definitions ---
-    name: Mapped[str] = mapped_column(String(100), nullable=False, unique=True)
+    name: Mapped[str] = mapped_column(String(100), nullable=False)


This changes the DB uniqueness rules for group (from unique name to composite unique (name, group_type)), but this PR doesn’t include an Alembic migration to drop the old constraint and add the new one. Without a migration the runtime schema will drift and deployments/tests that rely on migrations will fail. Add an Alembic migration that (1) drops the previous unique constraint on name (if present) and (2) creates uq_group_name_type, accounting for existing data.

…_id in well_inventory_csv.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 82 out of 86 changed files in this pull request and generated 3 comments.

Copilot · 2026-02-07T23:22:47Z

+# router = APIRouter(prefix="/well-inventory-csv")
+
+
+# @router.post("")
+# async def well_inventory_csv(


This module is entirely commented out, so the /well-inventory-csv endpoint won’t exist at runtime. The Behave steps added in this PR post to /well-inventory-csv, so those feature tests will fail unless the router and handler are restored (and the router is included in the main FastAPI app).

Copilot · 2026-02-07T23:22:48Z

+    elevation_ft = float(model.elevation_ft)
+    elevation_m = convert_ft_to_m(elevation_ft)
+
+    loc = Location(


The CSV row contains elevation_method, and downstream tests in this PR assert location.elevation_method equals the CSV value. _make_location currently sets only point and elevation. If Location has an elevation_method column (as the tests imply), it needs to be persisted on the Location record (and/or the tests should be updated to validate DataProvenance instead).

Suggested change

loc = Location(

elevation=elevation_m,

elevation_method=model.elevation_method,

Copilot · 2026-02-07T23:22:48Z

@@ -221,7 +221,7 @@
      "description": null
    },
    {


Renaming the lexicon category from origin_source to origin_type is a breaking change for any existing DB-seeded lexicon data and any code expecting the old category name. This needs an accompanying migration/seed-update path (or backward-compatible aliasing) to ensure environments with an existing lexicon table don’t fail when building enums or validating lexicon terms.

Suggested change

{

{

"name": "origin_source",

"description": null

},

{

Copilot

Pull request overview

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

Copilot · 2026-02-08T00:44:29Z

+# router = APIRouter(prefix="/well-inventory-csv")
+
+
+# @router.post("")
+# async def well_inventory_csv(
+#     user: amp_editor_dependency,
+#     session: session_dependency,
+#     file: UploadFile = File(...),
+# ):


The entire /well-inventory-csv API router and handler are commented out, but the Behave steps post to /well-inventory-csv. As-is, this will return 404 and break feature tests; please implement the actual APIRouter route (or remove the feature steps) and ensure the router is included in the app.

Copilot · 2026-02-08T00:44:30Z

+    validate_email,
+    AfterValidator,
+    field_validator,
+)


Importing validate_email directly from pydantic is not reliable (and commonly fails in Pydantic v2 where it’s not exported from the top-level). Use EmailStr for the field type or import the validator from the supported module (e.g., pydantic.networks) to avoid runtime ImportError.

Suggested change

validate_email,

AfterValidator,

field_validator,

)

AfterValidator,

field_validator,

)

from pydantic.networks import validate_email

Copilot · 2026-02-08T00:44:30Z

+
+    loc = Location(
+        point=transformed_point.wkt,
+        elevation=elevation_m,


The import creates Location records without setting elevation_method, but tests/test_well_inventory.py asserts location.elevation_method == file_content['elevation_method']. Either populate Location.elevation_method here (if it’s a column), or adjust the test to validate the intended provenance field instead.

Suggested change

elevation=elevation_m,

elevation=elevation_m,

elevation_method=model.elevation_method,

Copilot · 2026-02-08T00:44:30Z

+    assert "datalogger_installation_status" in context.water_well_data
+    assert "open_status" in context.water_well_data
    assert (
-        context.water_well_data["is_suitable_for_datalogger"]
-        == context.objects["wells"][0].is_suitable_for_datalogger
+        context.water_well_data["datalogger_installation_status"]
+        == context.objects["wells"][0].datalogger_installation_status
+    )


The codebase surfaces the computed field as datalogger_suitability_status (see db/thing.py and schemas/thing.py), but this step asserts datalogger_installation_status and accesses Thing.datalogger_installation_status (which doesn’t appear in the model changes). Align the step to the actual API/property name (datalogger_suitability_status) to prevent failing feature tests.

Copilot · 2026-02-08T00:44:30Z

+EXCLUDED_FIELDS = [
+    "location_id",
+    "group_id",
+    "well_purposes",
+    "well_casing_materials",
+    "measuring_point_height",
+    "measuring_point_description",
+    "well_completion_date_source",
+    "well_construction_method_source",
+    "well_depth_source",
+    "alternate_ids",
+    "monitoring_frequencies",
+    "notes",
+    "is_suitable_for_datalogger",
+    "is_open",
+    "well_status",
+]


Pydantic v2 model_dump(exclude=...) expects a set/dict-style include/exclude structure; passing a plain list can raise a type error depending on the exact Pydantic version. To make this robust, define EXCLUDED_FIELDS as a set[str] (or a dict) before passing it to model_dump.

Copilot · 2026-02-08T00:44:31Z

    rebuild = False
-    # rebuild = True
-    erase_data = True
+    erase_data = False
    if rebuild:
-        erase_and_rebuild_db()
+        _drop_and_rebuild_db()


Hard-coding rebuild = True makes every Behave run drop and rebuild the database, which is slow and potentially destructive for local workflows. Since .env.example introduces REBUILD_DB, this should be controlled via environment variables (e.g., default to no rebuild, enable in CI when needed).

Copilot · 2026-02-08T00:44:31Z

+    def test_upload_headers_only(self):
+        """Upload fails when CSV has headers but no data rows."""
+        file_path = Path("tests/features/data/well-inventory-no-data-headers.csv")
+        if file_path.exists():
+            result = well_inventory_csv(file_path)
+            assert result.exit_code == 1
+            assert "No data rows found" in result.stderr


Several tests are effectively skipped when fixture files are missing (if file_path.exists(): ...). This can silently reduce coverage and hide regressions. Prefer asserting the fixture exists (fail fast) or generate the CSV content in the test using tmp_path so the test always executes.

Copilot · 2026-02-08T00:44:31Z

+            permission = _make_well_permission(
+                well=well,
+                contact=contact_for_permissions,
+                permission_type=permission_type,
+                permission_allowed=permission_allowed,
+                start_date=model.date_time.date(),
+            )


_make_well_permission raises PydanticStyleException when permissions are provided but no contact exists. _import_well_inventory_csv only catches ValueError and DatabaseError, so this exception would bubble out and abort the whole import unexpectedly. Handle PydanticStyleException (or convert this case into a row-level validation error) so the import returns consistent validation_errors output.

Suggested change

permission = _make_well_permission(

well=well,

contact=contact_for_permissions,

permission_type=permission_type,

permission_allowed=permission_allowed,

start_date=model.date_time.date(),

)

try:

permission = _make_well_permission(

well=well,

contact=contact_for_permissions,

permission_type=permission_type,

permission_allowed=permission_allowed,

start_date=model.date_time.date(),

)

except PydanticStyleException as exc:

# Convert to a ValueError so the import logic can treat this as

# a row-level validation error instead of aborting the import.

raise ValueError(str(exc)) from exc

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

Comment thread tests/features/steps/well-inventory-csv.py Outdated

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 AI found potential problems Nov 15, 2025

View reviewed changes

Comment thread api/well_inventory.py 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

Comment thread api/well_inventory.py Outdated

Comment thread api/well_inventory.py Outdated

Comment thread api/well_inventory.py Outdated

Comment thread api/well_inventory.py Outdated

Comment thread api/well_inventory.py Outdated

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 AI found potential problems Nov 20, 2025

View reviewed changes

Comment thread api/well_inventory.py Fixed

jirhiker added 2 commits February 8, 2026 01:01

feat: implement well inventory CSV import functionality and related m…

ef49776

…odels

feat: enhance well inventory CSV processing with improved error handl…

eb06ca4

…ing and validation

Copilot AI review requested due to automatic review settings February 7, 2026 14:28

Formatting changes

0add256

Copilot AI reviewed Feb 7, 2026

View reviewed changes

Comment thread tests/test_well_inventory.py Outdated

Comment thread schemas/thing.py Outdated

Comment thread api/ogc/features.py

Comment thread services/well_inventory_csv.py Outdated

Comment thread api/well_inventory.py

Update tests/test_well_inventory.py

9a901bc

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings February 7, 2026 21:13

Update services/well_inventory_csv.py

b075aa5

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI reviewed Feb 7, 2026

View reviewed changes

Update schemas/thing.py

294fcb7

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings February 7, 2026 21:26

jirhiker and others added 2 commits February 8, 2026 08:29

feat: refactor well inventory CSV processing for improved error handl…

ef4a3f1

…ing and validation

Formatting changes

a53ec1a

Copilot AI reviewed Feb 7, 2026

View reviewed changes

Update schemas/well_inventory.py

231eff4

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings February 7, 2026 22:57

Copilot AI reviewed Feb 7, 2026

View reviewed changes

jirhiker and others added 3 commits February 8, 2026 10:09

fix: clean up EOF comments in well_inventory.py and add default thing…

65a1e9e

…_id in well_inventory_csv.py

Update services/well_inventory_csv.py

3f180f1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update tests/features/well-inventory-csv.feature

1c37e01

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings February 7, 2026 23:19

Formatting changes

7c09529

Copilot AI reviewed Feb 7, 2026

View reviewed changes

jirhiker added 2 commits February 8, 2026 10:23

Merge branch 'staging' into well-inventory-csv

335ae97

feat: update well inventory fields and improve validation error handling

60614ea

Copilot AI review requested due to automatic review settings February 8, 2026 00:40

Formatting changes

4d0a796

jirhiker merged commit 96ecb5b into staging Feb 8, 2026
3 checks passed

Copilot AI reviewed Feb 8, 2026

View reviewed changes

jacob-a-brown mentioned this pull request Mar 16, 2026

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

Merged

	well_data = data.model_dump(exclude=EXCLUDED_FIELDS)
	well_data = data.model_dump(exclude=set(EXCLUDED_FIELDS))

		# router = APIRouter(prefix="/well-inventory-csv")


		# @router.post("")

	loc = Location(
	elevation=elevation_m,
	elevation_method=model.elevation_method,

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!

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!

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!

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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!

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

jirhiker commented Nov 14, 2025 •

edited

Loading

codecov-commenter commented Nov 14, 2025 •

edited

Loading