[#12048] Migrate CreateFeedbackQuestionAction #12217
Conversation
| "questionType": "TEXT", | ||
| "questionText": "What is the best selling point of your product?" | ||
| }, | ||
| "description": "This is a text question.", | ||
| "questionNumber": 1, | ||
| "questionType": "TEXT", |
There was a problem hiding this comment.
questionType seems to be repeated here. possible to remove this redundancy?
There was a problem hiding this comment.
Yes! Thanks for the catch!
| if (instructorDetailForCourse != null) { | ||
| gateKeeper.verifyAccessible(instructorDetailForCourse, | ||
| getNonNullFeedbackSession(feedbackSessionName, courseId), | ||
| Const.InstructorPermissions.CAN_MODIFY_SESSION); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I believe we need to use the isCourseMigrated method here.
There's a case that's not considered here. After migration, datastore entity is still non-null. However, source of truth is now the sql entity, not the datastore one.
| getNonNullSqlFeedbackSession(feedbackSessionName, courseId), | ||
| request.getQuestionNumber(), | ||
| request.getQuestionDescription(), | ||
| request.getQuestionDetails().getQuestionType(), | ||
| request.getGiverType(), | ||
| request.getRecipientType(), | ||
| request.getNumberOfEntitiesToGiveFeedbackTo(), | ||
| request.getShowResponsesTo(), | ||
| request.getShowGiverNameTo(), | ||
| request.getShowRecipientNameTo(), | ||
| request.getQuestionDetails() |
There was a problem hiding this comment.
Maybe it's time we consider a factory method for questions and responses. Can have a static method (maybe makeQuestion or similar?) in FeedbackQuestions that takes in the params and the question type and returns the subclass.
There was a problem hiding this comment.
Sure! Thanks for the advice!
| @@ -41,6 +41,6 @@ public String toString() { | |||
| */ | |||
| @Converter | |||
| public static class FeedbackConstantSumResponseDetailsConverter | |||
| extends JsonConverter<FeedbackConstantSumResponseDetails> { | |||
| extends FeedbackQuestionDetailsConverter { | |||
There was a problem hiding this comment.
if i'm not wrong, these are response details, and not question details and hence the converter here is incorrect (need to create a FeedbackResponseDetailsConverter)
There was a problem hiding this comment.
Yes! Thanks for the catch!
wkurniawan07
added
s.ToMerge
Part of #12048
questionTextinFeedbackQuestionbecause initially question details are stored as a json string inquestionText, and now we have all the different types of question inheritingFeedbackQuestionand the question details are stored in each subclass. ThequestionTextin the parent class is redundant.JsonConverterforquestionDetailsis changed to one not using generic as the one with generic is not working currently, and it will be updated in the future.testArchitecture_uiShouldNotTouchStorageis updated.GetFeedbackQuestionActionis migrated.