Skip to content

BDMS 57: updated contact model, schema, tests, and transfer#119

Merged
jirhiker merged 9 commits into
pre-productionfrom
jab-contact-updates
Sep 3, 2025
Merged

BDMS 57: updated contact model, schema, tests, and transfer#119
jirhiker merged 9 commits into
pre-productionfrom
jab-contact-updates

Conversation

@jacob-a-brown

Copy link
Copy Markdown
Contributor

Why

This PR addresses the following problem / context:

  • The fields organization and nma_pk_owners were missing from Ocotillo

How

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

  • Added organization as a nullable string field
  • Made name nullable so that organization can be the primary name of a contact if that's how it's been recorded
  • An error is raised if either organization or name are null when creating or updating a contact
  • Added Company column to ownersdata.csv to more closely mimic NM_Aquifer
  • Added nma_pk_owners field to contact to tie records back to NM_Aquifer. nma_pk_owners is only to be used when transferring data. It is not usable by the API.
  • When transferring data from the tables to Ocotillo DB any person without (first name or last name) and company are skipped because there's no identifying information.
    • Should this be the case? Or should the OwnerKey become the name?

Notes

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

  • Added clear print statements to transfers/transfer.py to help the user know when a transfer function is being called
  • Added transfers to the beginning of paths so that the transfer script can be called from the root directory as python -m transfers.transfer
  • Renamed owners_transfer to contacts_transfer to be consistent in style and naming
  • ownersdata.csv has PointID as a column. In NM_Aquifer there is an OwnerLink table to connect owners to locations. When creating the tables for the transfer will we include PointIDs in the owners data table, or should I add an OwnerLink table to mimic NM_Aquifer more closely? (and update the transfer script appropriately)

@codecov-commenter

codecov-commenter commented Sep 3, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
api/contact.py 95.96% <100.00%> (+0.16%) ⬆️
db/contact.py 100.00% <100.00%> (ø)
schemas/contact.py 100.00% <100.00%> (ø)
tests/conftest.py 100.00% <100.00%> (ø)
tests/test_contact.py 100.00% <100.00%> (ø)

Comment thread api/contact.py
contact.organization is None
and contact_data.organization is None
and contact_data.name is None
):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be

if contact_data.name is None and contact_data.organization is None:
   if contact.name is None:
      ....
   elif contact.organization is None:
      .... 

Comment thread db/contact.py
search_vector = Column(TSVectorType("name", "role"))
search_vector = Column(
TSVectorType("name", "role", "organization", "nma_pk_owners")
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should think about whether or not we should make nma_pk_owners searchable

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That data is encoded in either the name or organization, so that may be redundant and lead to confusion... It's also not returned in any responses. I'll remove this in a PR. Should role be searchable too? That's not really unique to a particular contact (different contacts can have the same roles).

@jirhiker jirhiker Sep 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lets leave role it in for now, but i get your point. Someone probably won't search by the "role" keyword

@jirhiker jirhiker merged commit 99e71ec into pre-production Sep 3, 2025
3 checks passed
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