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

Jump to bottom

[#9363] Replace questionMetaData with questionDetails in FeedbackQuestionAttributes #9410

Merged
merged 21 commits into from Feb 22, 2019
Merged

[#9363] Replace questionMetaData with questionDetails in FeedbackQuestionAttributes #9410

merged 21 commits into from Feb 22, 2019

Conversation

Adoby7
Copy link
Contributor

@Adoby7 Adoby7 commented Feb 9, 2019 •
edited

Fixes #9363

Outline of Solution

Replace the feedbackResponseMetadata to feedbackResponseDetails. Change the field in .json file as well. Customize the Json Deserialize Adaptor to deserialize FeedbackQuestionDetails to its corresponding subclass.

The challenge is FeedbackTextQuestionDetails. Due to the legacy data in data store before there were multiple question types, the json string of FeedbackTextQuestionDetails is the plain text of question text. However, as the recommendedLength was added, the plain text is not applicable any more. Therefore, in .json files and data store, there are both FeedbackTextQuestionDetails with and without recommendedLength information. We need to handle them separately

The script to change .json files can be found here

@xpdavid xpdavid self-assigned this Feb 9, 2019
@xpdavid xpdavid added the s.Ongoing The PR is being worked on by the author(s) label Feb 9, 2019
@xpdavid xpdavid self-requested a review February 12, 2019 13:13
@Adoby7 Adoby7 force-pushed the issue_9363_question_details branch 2 times, most recently from 2fbdf46 to e4c9c2a Compare February 12, 2019 15:09
@Adoby7 Adoby7 closed this Feb 12, 2019
@Adoby7 Adoby7 reopened this Feb 12, 2019
@Adoby7
Copy link
Contributor Author

Adoby7 commented Feb 12, 2019 •
edited

Ready for review

xpdavid
xpdavid reviewed Feb 16, 2019
View reviewed changes
Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

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

Looks good

src/main/java/teammates/common/datatransfer/attributes/FeedbackQuestionAttributes.java Outdated Show resolved Hide resolved
src/main/java/teammates/common/datatransfer/attributes/FeedbackQuestionAttributes.java Outdated Show resolved Hide resolved
src/main/java/teammates/common/util/JsonUtils.java Show resolved Hide resolved
src/test/java/teammates/test/cases/datatransfer/FeedbackQuestionAttributesTest.java Outdated Show resolved Hide resolved
src/test/java/teammates/test/cases/datatransfer/FeedbackQuestionAttributesTest.java Outdated Show resolved Hide resolved
src/test/java/teammates/test/cases/datatransfer/FeedbackQuestionAttributesTest.java Show resolved Hide resolved
src/test/java/teammates/test/cases/datatransfer/FeedbackQuestionAttributesTest.java Outdated Show resolved Hide resolved
src/test/java/teammates/test/cases/datatransfer/FeedbackQuestionAttributesTest.java Outdated Show resolved Hide resolved
@Adoby7
Merge branch 'master' into issue_9363_question_details
ac26a5f 
# Conflicts:
#	src/main/java/teammates/storage/api/FeedbackQuestionsDb.java
#	src/test/java/teammates/test/cases/logic/FeedbackQuestionsLogicTest.java
@Adoby7
fix compile error after merging
aab1219
@Adoby7
add tests for serialize adaptor
8ff4d56
@Adoby7
fix coding style problem
b5898ec
xpdavid
xpdavid reviewed Feb 17, 2019
View reviewed changes
src/test/java/teammates/test/cases/util/JsonUtilsTest.java Outdated
.withFeedbackSessionName("testFeedbackSession")
.withGiverType(FeedbackParticipantType.INSTRUCTORS)
.withRecipientType(FeedbackParticipantType.SELF)
.withNumOfEntitiesToGiveFeedbackTo(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should use Const.MAX_POSSIBLE_RECIPIENTS here.

src/test/java/teammates/test/cases/logic/FeedbackQuestionsLogicTest.java Outdated
@@ -417,7 +416,7 @@ public void testUpdateQuestionCascade() throws Exception {
FeedbackQuestionAttributes questionToUpdate = getQuestionFromDatastore("qn2InSession1InCourse2");

FeedbackQuestionDetails fqd = new FeedbackTextQuestionDetails("new question text");
questionToUpdate.questionMetaData = JsonUtils.toJson(fqd);
questionToUpdate.questionDetails = fqd;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use setQuestionDetail() here? Same as below.

src/test/java/teammates/test/cases/datatransfer/FeedbackQuestionDetailsTest.java
((FeedbackTextQuestionDetails) ftqd1).setRecommendedLength(50);
assertFalse(ftqd1.equals(ftqd2));

______TS("All attributes are same, should be same");
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Test case for two references to the same question details?
  • Test case for null input?

Basically, try to cover every statement for equal() you introduced.

xpdavid
xpdavid approved these changes Feb 21, 2019
View reviewed changes
Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

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

LGTM!

@xpdavid xpdavid added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Feb 21, 2019
@xpdavid xpdavid requested a review from damithc February 21, 2019 13:15
wkurniawan07
wkurniawan07 approved these changes Feb 21, 2019
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

Approving on behalf of @damithc

@xpdavid one remark for your attention

src/main/java/teammates/common/datatransfer/attributes/FeedbackQuestionAttributes.java
private static FeedbackQuestionDetails deserializeFeedbackTextQuestionDetails(String questionDetailsInJson) {
try {
// There are `FeedbackTextQuestion` with plain text, Json without `recommendedLength`, and complete Json
// in data store. Gson cannot parse the plain text case, so we need to handle it separately.
Copy link
Member

Choose a reason for hiding this comment

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

@xpdavid this sounds like something that needs data migration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. But it is not introduced in this PR. It a legacy pending data migration introduced long time ago :p

@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Feb 21, 2019
@xpdavid xpdavid merged commit 0c7fa8c into TEAMMATES:master Feb 22, 2019
@xpdavid xpdavid added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Feb 22, 2019
@Adoby7 Adoby7 deleted the issue_9363_question_details branch February 22, 2019 06:58
rrtheonlyone pushed a commit to rrtheonlyone/teammates that referenced this pull request Feb 24, 2019
@Adoby7 @rrtheonlyone
[TEAMMATES#9363] Replace questionMetaData with questionDetails in Fee…
a362562 
…dbackQuestionAttributes (TEAMMATES#9410)

[TEAMMATES#9363] Replace questionMetaData with questionDetails in FeedbackQuestionAttributes (TEAMMATES#9410)
@wkurniawan07 wkurniawan07 added this to the V7.0.0-beta.0 milestone Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xpdavid xpdavid xpdavid approved these changes

@wkurniawan07 wkurniawan07 wkurniawan07 approved these changes

@damithc damithc Awaiting requested review from damithc

Assignees

@xpdavid xpdavid

Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V7.0.0-beta.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Adoby7 @wkurniawan07 @xpdavid