Updates required for the https://github.com/CDLUC3/dmsp_frontend_prototype/issues/188 frontend Edit Question ticket #194
Conversation
| @@ -8,7 +8,5 @@ CREATE TABLE `questionOptions` ( | |||
| `createdById` int, | |||
| `modified` TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, | |||
| `modifiedById` int, | |||
| CONSTRAINT unique_question_option_text UNIQUE (`questionId`, `text`), | |||
There was a problem hiding this comment.
I had to remove these constraints because they were preventing me from updating existing question options on the Edit Question page
| @@ -3,7 +3,7 @@ import { MyContext } from "../context"; | |||
| import { QuestionOption } from "../models/QuestionOption"; | |||
| import { Question } from "../models/Question"; | |||
| import { Section } from "../models/Section"; | |||
| import { hasPermissionOnQuestion } from "../services/questionService"; | |||
| import { getQuestionOptionsToRemove, hasPermissionOnQuestion } from "../services/questionService"; | |||
There was a problem hiding this comment.
Needed to add getQuestionOptionsToRemove function for when a user removes options from an existing questions.
There was a problem hiding this comment.
I have a list of new tickets I am going to create and consolidating how associations like this are handled is one of them.
Let's merge this fix for now and we can come back afterward to make sure we're handling them all in the same way.
| // Separate incoming options into "to update" and "to create" | ||
| const optionsToUpdate = []; | ||
| const optionsToCreate = []; | ||
|
|
||
| questionOptions.forEach(option => { | ||
| if (existingOptionsMap.has(option.questionOptionId)) { | ||
| if (existingOptionsMap.has(option.id)) { |
There was a problem hiding this comment.
Switched to using id instead of questionOptionId so that I didn't need to extend the existing QuestionOption class
There was a problem hiding this comment.
yeah that makes sense. You're working within the context of the QuestionOption itself here.
| @@ -3,7 +3,7 @@ import { MyContext } from "../context"; | |||
| import { QuestionOption } from "../models/QuestionOption"; | |||
| import { Question } from "../models/Question"; | |||
| import { Section } from "../models/Section"; | |||
| import { hasPermissionOnQuestion } from "../services/questionService"; | |||
| import { getQuestionOptionsToRemove, hasPermissionOnQuestion } from "../services/questionService"; | |||
There was a problem hiding this comment.
I have a list of new tickets I am going to create and consolidating how associations like this are handled is one of them.
Let's merge this fix for now and we can come back afterward to make sure we're handling them all in the same way.
| // Separate incoming options into "to update" and "to create" | ||
| const optionsToUpdate = []; | ||
| const optionsToCreate = []; | ||
|
|
||
| questionOptions.forEach(option => { | ||
| if (existingOptionsMap.has(option.questionOptionId)) { | ||
| if (existingOptionsMap.has(option.id)) { |
There was a problem hiding this comment.
yeah that makes sense. You're working within the context of the QuestionOption itself here.
| @@ -74,22 +74,5 @@ export const resolvers: Resolvers = { | |||
| return await QuestionOption.findByQuestionOptionId('updateQuestion resolver', context, updatedQuestionOption.id); | |||
| } | |||
| }, | |||
| removeQuestionOption: async (_, { questionOptionId }, context: MyContext): Promise<QuestionOption> => { | |||
There was a problem hiding this comment.
I was also thinking about situations like this. There are a few cases where we exposed individual removeThing resolvers, but the front end sends the list of its associations when the parent object gets created/updated.
As part of the ticket I mentioned above, we should probably figure out which approaches we are using for each scenario and delete any unused ones
Description
uniqconstraints ontextandorderNumberfor questionOptions table, because it was restricting the ability to update any text on an existing record.getQuestionOptionsToRemoveto remove questionOptions that are no longer in the updated setquestionOptionsresolver to useidinstead ofquestionOptionIdbecause otherwise I had to extend existing classFixes # (193)
Type of change
How Has This Been Tested?
Manual testing and also checked that it didn't break any existing unit tests
Checklist: