Feature/js updates to accommodate question types with options #179
Conversation
There was a problem hiding this comment.
Looks good overall @jupiter007. I have a few change requests with regard to the DB and then some minor cleanup.
| @@ -81,7 +84,7 @@ export class Question extends MySqlModel { | |||
| this.cleanup(); | |||
|
|
|||
| // Save the record and then fetch it | |||
| const newId = await Question.insert(context, this.tableName, this, 'Question.create'); | |||
| const newId = await Question.insert(context, this.tableName, this, 'Question.create', ['questionOptions']); | |||
There was a problem hiding this comment.
I think you'll want to add the questionOptions to the list of props to skip on the update as well
There was a problem hiding this comment.
Thanks. I missed that one. It has been added.
|
|
||
| if (!this.questionId) { | ||
| this.errors.push('Question ID can\'t be blank'); | ||
| } |
There was a problem hiding this comment.
I think this should also check for the existence of text and orderNumber. They are required in the DB, so we should verify that here
There was a problem hiding this comment.
Yes, thanks so much for catching that. I made the update.
| `createdById` int, | ||
| `modified` TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| `modifiedById` int, | ||
| FOREIGN KEY (questionId) REFERENCES questions(id) ON DELETE CASCADE |
There was a problem hiding this comment.
Switch text to VARCHAR(255) I can't see a scenario where we'd want the user including more text than that.
Then, I would add the following (below the FOREIGN KEY constraint) to ensure uniqueness of the options and that there are no dupes for the order:
CONSTRAINT unique_question_option UNIQUE (`questionId`, `text`),
CONSTRAINT unique_question_option UNIQUE (`questionId`, `orderNumber`)`
There was a problem hiding this comment.
Cool. Thanks. I added the constraints
src/resolvers/question.ts
Outdated
|
|
||
| // Get Question based on provided questionId | ||
| const questionData = await Question.findById('updateQuestionOptions resolver', context, questionId); | ||
| const existingQuestionOptions = await getExistingQuestionOptions(context, questionId); |
There was a problem hiding this comment.
You don't need this. The cascade delete on the DB will remove them automatically.
Just add a comment here noting that the DB will delete them.
There was a problem hiding this comment.
Thanks. I keep forgetting about the cascade functionality. I removed the code to delete questionOptions and added a comment
src/schemas/question.ts
Outdated
| } | ||
|
|
||
| "QuestionOption always belongs to a Question" | ||
| type QuestionOption { |
There was a problem hiding this comment.
You've already defined QuestionOption within the QuestionOption schema above. I would say leave the type for it and theQuestionOptionInput in the other file
There was a problem hiding this comment.
Good catch. I removed QuestionOption from schema and moved QuestionOptionInput to the questionOption schema
src/services/questionService.ts
Outdated
| export const getExistingQuestionOptions = async (context: MyContext, questionId: number): Promise<QuestionOption[]> => { | ||
|
|
||
| //Get all the existing question options already associated with this question | ||
| return await QuestionOption.findByQuestionId('questionService', context, questionId); |
There was a problem hiding this comment.
Why extract this one liner out into the service?
There was a problem hiding this comment.
I meant to remove this since we already have a findByQuestionId method in QuestionOption model. I removed from service and updated resolver to use the existing method instead.
… duplicated QuestionOption schema and updated data-migration script to use VARCHAR(255) for text
|
Thanks so much for reviewing my PR @briri . I've responded to your comments. Please take another look when you get the chance. |
Description
Needed to make updated to accommodate question options for question types like radio buttons, checkboxes, and select drop-down
questionOptionstable and seed dataquestionOptionsschema, resolver andQuestionOptionmodel, which will be used foroptionquestion typesquestionsresolver to add or updatequestionOptionswhen passed in the mutationquestionOptionsto schema and updatedquestionquery and mutations to includequestionOptionsFixes # (172)
Type of change
How Has This Been Tested?
This has been manually tested and tested with unit tests.
Checklist: