Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates required for the https://github.com/CDLUC3/dmsp_frontend_prototype/issues/188 frontend Edit Question ticket #194

Merged
merged 5 commits into from
Feb 7, 2025
Merged

Updates required for the https://github.com/CDLUC3/dmsp_frontend_prototype/issues/188 frontend Edit Question ticket #194

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ff4574d
made changes to accommodate edit question updates
jupiter007 Feb 1, 2025
ab3f3e9
Changed questionOptionsId to just id
jupiter007 Feb 1, 2025
2a569cb
removed reference to questionOptionId
jupiter007 Feb 3, 2025
1cb33df
Removed unique constraints on questionOptions text and orderNumber be…
jupiter007 Feb 6, 2025
8e049c4
merged development into branch
jupiter007 Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions data-migrations/2025-01-24-0318-create-question-options-table.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to remove these constraints because they were preventing me from updating existing question options on the Edit Question page

CONSTRAINT unique_question_option_orderNumber UNIQUE (`questionId`, `orderNumber`),
FOREIGN KEY (questionId) REFERENCES questions(id) ON DELETE CASCADE
) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8mb3;
25 changes: 22 additions & 3 deletions src/resolvers/question.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to add getQuestionOptionsToRemove function for when a user removes options from an existing questions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import { BadUserInputError, ForbiddenError, NotFoundError } from "../utils/graphQLErrors";
import { QuestionCondition } from "../models/QuestionCondition";

Expand Down Expand Up @@ -140,15 +140,33 @@ export const resolvers: Resolvers = {
existingQuestionOptions.map(option => [option.id, option])
);


// Get list of options that need to be removed
const optionsToRemove = await getQuestionOptionsToRemove(questionOptions as QuestionOption[], context, questionId);

// Remove question options that are no longer in the updated questionOptions array
if (optionsToRemove.length > 0) {
await Promise.all(
optionsToRemove.map(async (option) => {
const questionOption = new QuestionOption({
questionId: option.questionId,
id: option.id
});

await questionOption.delete(context);
})
);
}

// 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)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to using id instead of questionOptionId so that I didn't need to extend the existing QuestionOption class

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that makes sense. You're working within the context of the QuestionOption itself here.

// Add to update list, merging the new data with the existing data
optionsToUpdate.push({
...existingOptionsMap.get(option.questionOptionId), // existing option data
...existingOptionsMap.get(option.id), // existing option data
...option // updated fields from input
});
} else {
Expand Down Expand Up @@ -180,6 +198,7 @@ export const resolvers: Resolvers = {
);
}


// Return newly updated question
return await Question.findById('updateQuestion resolver', context, updatedQuestion.id);
}
Expand Down
27 changes: 5 additions & 22 deletions src/resolvers/questionOption.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ export const resolvers: Resolvers = {
questionOptions: async (_, { questionId }, context: MyContext): Promise<QuestionOption[]> => {
return await QuestionOption.findByQuestionId('questionOption resolver', context, questionId);
},
questionOption: async (_, { questionOptionId }, context: MyContext): Promise<QuestionOption> => {
return await QuestionOption.findByQuestionOptionId('questionOption resolver', context, questionOptionId);
questionOption: async (_, { id }, context: MyContext): Promise<QuestionOption> => {
return await QuestionOption.findByQuestionOptionId('questionOption resolver', context, id);
},
},
Mutation: {
Expand Down Expand Up @@ -42,21 +42,21 @@ export const resolvers: Resolvers = {
}
},
updateQuestionOption: async (_, { input: {
questionOptionId,
id,
text,
orderNumber,
isDefault } }, context: MyContext): Promise<QuestionOption> => {

// Get QuestionOption based on provided questionOptionId
const questionOptionData = await QuestionOption.findByQuestionOptionId('questionOption resolver', context, questionOptionId);
const questionOptionData = await QuestionOption.findByQuestionOptionId('questionOption resolver', context, id);

// Throw Not Found error if QuestionOption data is not found
if (!questionOptionData) {
throw NotFoundError('QuestionOption not found')
}

const questionOption = new QuestionOption({
id: questionOptionId,
id: questionOptionData.id,
questionId: questionOptionData.questionId,
text: text || questionOptionData.text,
orderNumber: orderNumber || questionOptionData.orderNumber,
Expand All @@ -74,22 +74,5 @@ export const resolvers: Resolvers = {
return await QuestionOption.findByQuestionOptionId('updateQuestion resolver', context, updatedQuestionOption.id);
}
},
removeQuestionOption: async (_, { questionOptionId }, context: MyContext): Promise<QuestionOption> => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Retrieve existing questionOption
const questionOptionData = await QuestionOption.findByQuestionOptionId('removeQuestion resolver', context, questionOptionId);

// Throw Not Found error if QuestionOption is not found
if (!questionOptionData) {
throw NotFoundError('QuestionOption not found')
}

//Need to create a new instance of QuestionOption so that it recognizes the 'delete' function of that instance
const questionOption = new QuestionOption({
...questionOptionData,
id: questionOptionId
});

return await questionOption.delete(context);
},
},
};
10 changes: 6 additions & 4 deletions src/schemas/questionOption.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ export const typeDefs = gql`
extend type Query {
"Get the Question Options that belong to the associated questionId"
questionOptions(questionId: Int!): [QuestionOption]
"Get the specific Question Option based on questionOptionId"
questionOption(questionOptionId: Int!): QuestionOption
"Get the specific Question Option based on question option id"
questionOption(id: Int!): QuestionOption
}

extend type Mutation {
Expand All @@ -14,7 +14,7 @@ extend type Mutation {
"Update a QuestionOption"
updateQuestionOption(input: UpdateQuestionOptionInput!): QuestionOption!
"Delete a QuestionOption"
removeQuestionOption(questionOptionId: Int!): QuestionOption
removeQuestionOption(id: Int!): QuestionOption
}

"QuestionOption always belongs to a Question"
Expand Down Expand Up @@ -66,12 +66,14 @@ input AddQuestionOptionInput {

input UpdateQuestionOptionInput {
"The id of the QuestionOption"
questionOptionId: Int
id: Int
"The option text"
text: String!
"The option order number"
orderNumber: Int!
"Whether the option is the default selected one"
isDefault: Boolean
"id of parent question"
questionId: Int
}
`
17 changes: 17 additions & 0 deletions src/services/questionService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Question } from "../models/Question";
import { VersionedQuestion } from "../models/VersionedQuestion";
import { NotFoundError } from "../utils/graphQLErrors";
import { QuestionCondition } from "../models/QuestionCondition";
import { QuestionOption } from "../models/QuestionOption";
import { VersionedQuestionCondition } from "../models/VersionedQuestionCondition";
import { formatLogMessage, logger } from "../logger";

Expand Down Expand Up @@ -169,3 +170,19 @@ export const generateQuestionConditionVersion = async (
}
}


export const getQuestionOptionsToRemove = async (questionOptions: QuestionOption[], context: MyContext, questionId: number): Promise<QuestionOption[]> => {
//Get all the existing question options associated with this question
const existingOptions = await QuestionOption.findByQuestionId('questionService', context, questionId);

// Create a Set of question option ids
const questionOptionIds = new Set(
questionOptions.map(option => option.id)
);

// Get options that exist in questionOptions table, but are not included in updated questionOptions
const optionsToRemove = existingOptions.filter(existing => !questionOptionIds.has(existing.id))

return Array.isArray(optionsToRemove) ? optionsToRemove : [];
}

16 changes: 9 additions & 7 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,7 +986,7 @@ export type MutationRemoveQuestionConditionArgs = {


export type MutationRemoveQuestionOptionArgs = {
questionOptionId: Scalars['Int']['input'];
id: Scalars['Int']['input'];
};


Expand Down Expand Up @@ -1598,7 +1598,7 @@ export type Query = {
question?: Maybe<Question>;
/** Get the QuestionConditions that belong to a specific question */
questionConditions?: Maybe<Array<Maybe<QuestionCondition>>>;
/** Get the specific Question Option based on questionOptionId */
/** Get the specific Question Option based on question option id */
questionOption?: Maybe<QuestionOption>;
/** Get the Question Options that belong to the associated questionId */
questionOptions?: Maybe<Array<Maybe<QuestionOption>>>;
Expand Down Expand Up @@ -1816,7 +1816,7 @@ export type QueryQuestionConditionsArgs = {


export type QueryQuestionOptionArgs = {
questionOptionId: Scalars['Int']['input'];
id: Scalars['Int']['input'];
};


Expand Down Expand Up @@ -2356,12 +2356,14 @@ export type UpdateQuestionInput = {
};

export type UpdateQuestionOptionInput = {
/** The id of the QuestionOption */
id?: InputMaybe<Scalars['Int']['input']>;
/** Whether the option is the default selected one */
isDefault?: InputMaybe<Scalars['Boolean']['input']>;
/** The option order number */
orderNumber: Scalars['Int']['input'];
/** The id of the QuestionOption */
questionOptionId?: InputMaybe<Scalars['Int']['input']>;
/** id of parent question */
questionId?: InputMaybe<Scalars['Int']['input']>;
/** The option text */
text: Scalars['String']['input'];
};
Expand Down Expand Up @@ -3166,7 +3168,7 @@ export type MutationResolvers<ContextType = MyContext, ParentType extends Resolv
removeProjectOutputFromPlan?: Resolver<Maybe<ResolversTypes['ProjectOutput']>, ParentType, ContextType, RequireFields<MutationRemoveProjectOutputFromPlanArgs, 'planId' | 'projectOutputId'>>;
removeQuestion?: Resolver<Maybe<ResolversTypes['Question']>, ParentType, ContextType, RequireFields<MutationRemoveQuestionArgs, 'questionId'>>;
removeQuestionCondition?: Resolver<Maybe<ResolversTypes['QuestionCondition']>, ParentType, ContextType, RequireFields<MutationRemoveQuestionConditionArgs, 'questionConditionId'>>;
removeQuestionOption?: Resolver<Maybe<ResolversTypes['QuestionOption']>, ParentType, ContextType, RequireFields<MutationRemoveQuestionOptionArgs, 'questionOptionId'>>;
removeQuestionOption?: Resolver<Maybe<ResolversTypes['QuestionOption']>, ParentType, ContextType, RequireFields<MutationRemoveQuestionOptionArgs, 'id'>>;
removeRepository?: Resolver<Maybe<ResolversTypes['Repository']>, ParentType, ContextType, RequireFields<MutationRemoveRepositoryArgs, 'repositoryId'>>;
removeSection?: Resolver<ResolversTypes['Section'], ParentType, ContextType, RequireFields<MutationRemoveSectionArgs, 'sectionId'>>;
removeTag?: Resolver<Maybe<ResolversTypes['Tag']>, ParentType, ContextType, RequireFields<MutationRemoveTagArgs, 'tagId'>>;
Expand Down Expand Up @@ -3416,7 +3418,7 @@ export type QueryResolvers<ContextType = MyContext, ParentType extends Resolvers
publishedTemplates?: Resolver<Maybe<Array<Maybe<ResolversTypes['VersionedTemplate']>>>, ParentType, ContextType, Partial<QueryPublishedTemplatesArgs>>;
question?: Resolver<Maybe<ResolversTypes['Question']>, ParentType, ContextType, RequireFields<QueryQuestionArgs, 'questionId'>>;
questionConditions?: Resolver<Maybe<Array<Maybe<ResolversTypes['QuestionCondition']>>>, ParentType, ContextType, RequireFields<QueryQuestionConditionsArgs, 'questionId'>>;
questionOption?: Resolver<Maybe<ResolversTypes['QuestionOption']>, ParentType, ContextType, RequireFields<QueryQuestionOptionArgs, 'questionOptionId'>>;
questionOption?: Resolver<Maybe<ResolversTypes['QuestionOption']>, ParentType, ContextType, RequireFields<QueryQuestionOptionArgs, 'id'>>;
questionOptions?: Resolver<Maybe<Array<Maybe<ResolversTypes['QuestionOption']>>>, ParentType, ContextType, RequireFields<QueryQuestionOptionsArgs, 'questionId'>>;
questionTypes?: Resolver<Maybe<Array<Maybe<ResolversTypes['QuestionType']>>>, ParentType, ContextType>;
questions?: Resolver<Maybe<Array<Maybe<ResolversTypes['Question']>>>, ParentType, ContextType, RequireFields<QueryQuestionsArgs, 'sectionId'>>;
Expand Down