SW-6282 Add is_ad_hoc and observation_type to observations table #2627
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
src/main/resources/db/migration/0300/V320__AdHocPlotObservations.sql
Outdated
Show resolved
Hide resolved
|
@sgrimm as I've started working on the actual creation of the plot and observation, I am starting to think that I also need to modify the |
| ALTER TABLE tracking.observations ADD CONSTRAINT ad_hoc_observation_type CHECK ( | ||
| (is_ad_hoc = FALSE AND observation_type_id IS NULL) | ||
| OR (is_ad_hoc = TRUE AND observation_type_id IS NOT NULL) | ||
| ); |
There was a problem hiding this comment.
What would you think about making observation type non-nullable and setting it to "Monitoring" for existing (and new non-ad-hoc) observations?
My sense is that observation type and ad-hocness are independent variables. We've already talked about possibly having assigned biomass observations, for example.
| name TEXT NOT NULL UNIQUE | ||
| ); | ||
|
|
||
| ALTER TABLE tracking.observations ADD COLUMN is_ad_hoc BOOLEAN DEFAULT FALSE NOT NULL; |
There was a problem hiding this comment.
Opinions differ on this, but I prefer not using DEFAULT for this kind of thing because it eliminates a little bit of automated bug detection: if, somewhere in the server code, we accidentally forget to set the is_ad_hoc value when we're inserting a new row, the insertion will succeed and use the default, rather than failing because we didn't specify a value for a mandatory column.
That's why a lot of my migrations look like "add nullable column, populate existing rows, set it to non-nullable."
That said, DEFAULT is part of SQL for a reason. If we want "forgot to include the column when inserting" cases to make a best-effort attempt to insert the row rather than abort the whole transaction, this is how to do it.
There was a problem hiding this comment.
Makes sense. I was doing this as a means to save on modification of the pre-existing observations code, but I see the benefit in explicitly migrating the field.
Yes, I think that's right. |
| ALTER TABLE tracking.monitoring_plots ALTER COLUMN planting_subzone_id DROP NOT NULL; | ||
| ALTER TABLE tracking.observation_plots ALTER COLUMN monitoring_plot_history_id DROP NOT NULL; |
There was a problem hiding this comment.
I had to add these and plumb their nullability through the pre-existing code. There isn't any mention of subzones in the PRD, and it seems like these plots need to be able to be created before any subzones exist. I also opted to allow the monitoring plot history to be optional, since creation of ad hoc observations will be a single event.
nickgraz
force-pushed
the
ng/SW-6282-db-2
branch
from
9130483
to
5558394
Compare
| ALTER TABLE tracking.observations ALTER COLUMN observation_type_id SET NOT NULL; | ||
|
|
||
| ALTER TABLE tracking.monitoring_plots ALTER COLUMN planting_subzone_id DROP NOT NULL; | ||
| ALTER TABLE tracking.observation_plots ALTER COLUMN monitoring_plot_history_id DROP NOT NULL; |
There was a problem hiding this comment.
Dropping the not-null from monitoring plot subzone IDs is definitely needed both for this and for the flexible site editing changes.
I'm nervous about making plot history IDs optional. I think it's likely to force us to write more complicated queries for things like viewing historical ad-hoc observations on a site map since we can no longer count on observation plots being tied to particular map versions via history IDs (meaning we could get the correct map geometry with simple joins on the history IDs) and we'll instead need to calculate the right map to show by, I guess, comparing the observation times against ranges of modification times on the site history.
Also, I think it's plausible we'll allow users to rename or otherwise edit ad-hoc plots after the fact since we're letting users name them initially, which means they would have more than one history row.
Was this just about avoiding storing data that didn't seem necessary, or will it cause problems to keep history for these plots?
In order to support ad hoc observations, add
is_ad_hocandobservation_typetoobservationstable.