Conversation
| "use strict"; | ||
| module.exports = { | ||
| up: async (queryInterface, Sequelize) => { | ||
| await queryInterface.createTable("IntakeForms", { |
There was a problem hiding this comment.
I wonder if we'll need a way to distinguish between internal intake forms and external intake forms, or other kinds of intake forms when there are multiple for the same noun. Not sure if we need to do anything for that now, or not.
| fieldId: { | ||
| type: Sequelize.INTEGER, | ||
| references: { |
There was a problem hiding this comment.
I don't know enough about sequelize nullability, but this should be nullable in the database since not all intake items are fields. Is it nullable?
| key: "id", | ||
| }, | ||
| }, | ||
| textContent: { |
There was a problem hiding this comment.
What is textContent for? Which kinds of intake items use it?
| key: "id", | ||
| }, | ||
| }, | ||
| sectionId: { |
There was a problem hiding this comment.
So sections are not stored in the database, they are just an integer on the intake items? How is nesting represented?
| router.get("/api/intake-forms", async (req, res) => { | ||
| const intakeForms = await sequelize.models.IntakeForm.findAll(); | ||
| res.send({ | ||
| intakeForms, |
There was a problem hiding this comment.
I honestly am not a huge fan of how sequelize includes capital case object keys in JSON. Also, I might have named pageChildren to be intakeItems so that other intake items that contain children intake items (like sections) have the same key as pages for their children. Additionally, it seems unnecessary to me for each intake item to send intakeFormId since the requester already asked only for intake items for that intake form.
I'm not sure how much of that could be customized in the sequelize up migrations, or if addressing it would require implementing a transformer to convert the internal sequelize representation to an external json api representation. I don't want to cause you a ton of work needlessly by insisting on having such a transformation layer - what are your thoughts?
| // Only include sections or items with no sections | ||
| [s.Op.or]: [{ sectionId: null }, { type: "section" }], |
There was a problem hiding this comment.
Is the idea here that we're omitting intake items with type: "page" since pages can't contain pages? Or what are we trying to omit? I'm not sure I fully understand the purpose of this.
| order: ["orderIndex"], | ||
| }); | ||
|
|
||
| res.send(intakeFormItems); |
There was a problem hiding this comment.
Is the http response header content-type set to application/json for this endpoint? I know that sometimes res.send() assumes content-type: text/html but that res.json() always sets the json content type. So calling res.json() might be safer, although perhaps express is configured in such a way where that doesn't matter in this project, I'm not sure.
| res.send(intakeFormItems); | |
| res.json(intakeFormItems); |
|
|
||
| router.get("/api/intake-forms", async (req, res) => { | ||
| const intakeForms = await sequelize.models.IntakeForm.findAll(); | ||
| res.send({ |
There was a problem hiding this comment.
Same here about maybe switching to res.json()
| import "./Fields/BatchPostFields.js"; | ||
| import "./Fields/GetNounFields.js"; | ||
| import "./IntakeForms/GetIntakeForms"; | ||
| import "./IntakeForms/PutIntakeForms"; |
There was a problem hiding this comment.
A little unusual to include an empty file in a PR - maybe move it to another PR? Or is this PR incomplete and will include the Put endpoint, too?
| export interface IntakePageItem { | ||
| type: IntakeItemType.Page; | ||
| id: number; | ||
| intakeItems: IntakeItem[]; |
There was a problem hiding this comment.
This does not match the api response json that has pageChildren rather than intakeItems
Models for intake form stuff, and GET api endpoints
The naming of some of the fields in the response could use some more work (i.e. couldn't figure out how to get
sectionChildrenandpageChildrento just both be namedchildren) but this has gotten big enough and I think it's a good startSome SQL to seed your DB to test this. This will create an intake form with three fields in it, two of them within a section
Here is the API response for
GET /api/intake-forms/{id}