Define models needed for HPC-7904 #30
Conversation
Pl217
added
the
pending prior merge
Pl217
force-pushed
the
HPC-7904-models
branch
from
1c8157c
to
2d53c80
Compare
Pl217
added
ready for review
|
@s0 I would also like you to take a look, since I've made some changes to models defined in #29. |
planId column is marked as NULL-able, but there are zero records in DB withouth this column set
This column is marked as NULL-able, and there are records already which have NULL set for this column.
This column is NULL-able with a DEFAULT value specified to be empty array. But, seems this DEFAULT value wasn't always set, since there are records with NULL value for this column.
This column is defined with `overriding boolean DEFAULT false NOT NULL` which clearly qualifies it to be marked as nonNullWithDefault
These two columns are defined as NULL-able and already have records with NULL values for these columns
Pl217
force-pushed
the
HPC-7904-models
branch
from
2d53c80
to
9bd6e33
Compare
src/db/models/location.ts
Outdated
| status: { | ||
| kind: 'enum', | ||
| values: LOCATION_STATUS, | ||
| }, |
There was a problem hiding this comment.
enum should only be used when the property uses an enum defined in sql, this particular field is a string, so instead we should use a checked string with t.keyof to validate upon read, and restrict the type when writing.
The reason for this is that at some point I'd like to extend the validation function / unit tests to check that enum types are defined correctly, and allow the same values.
There was a problem hiding this comment.
Makes sense even now, without enum definition checks
src/db/models/planVersion.ts
Outdated
| // Even though this column isn't defined using DB enum only two values are used | ||
| clusterSelectionType: { | ||
| kind: 'enum', | ||
| values: PLAN_VERSION_CLUSTER_SELECTION_TYPE, | ||
| }, |
There was a problem hiding this comment.
should use a checked t.keyof type instead
There was a problem hiding this comment.
Should have removed this comment above too. Since it "justifies" why 'enum' was used
There are a few changes I would request in #29, but it got already merged, so I made them here. Didn't want to open a new PR.
Took the opportunity to update Typescript, since I also updated it in UN-OCHA/hpc-api#21.
Version bump is also done, because we'll need to do a release after this is merged, so that separate PR for version bump is not needed.