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.

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

Student: Comment on responses: Fix put document error #9034 #9035

Merged
merged 6 commits into from
Aug 10, 2018

Conversation

nidhi98gupta
Copy link
Contributor

Fixes #9034

@nidhi98gupta
Copy link
Contributor Author

@xpdavid Put comment document was failing silently because comment id was null. Please review.

@@ -307,7 +307,7 @@ private void saveNewCommentsByFeedbackParticipant(List<FeedbackResponseCommentAt
throws EntityDoesNotExistException {
for (FeedbackResponseCommentAttributes frc : commentsToSave) {
try {
logic.createFeedbackResponseComment(frc);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a test for this.

@xpdavid xpdavid added c.Bug Bug/defect report s.Ongoing The PR is being worked on by the author(s) labels Aug 10, 2018
@xpdavid xpdavid self-assigned this Aug 10, 2018
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.

Will the test fail before the fix?

@nidhi98gupta
Copy link
Contributor Author

Will the test fail before the fix?

Yes

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
Copy link
Contributor

xpdavid commented Aug 10, 2018

Should not have exception now.

image

@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 Aug 10, 2018
@xpdavid xpdavid requested a review from damithc August 10, 2018 07:11
Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

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

Just a nit. Can merge after that.

* @param expected the expected results for the search query.
*/
protected static void verifySearchResults(FeedbackResponseCommentSearchResultBundle actual,
FeedbackResponseCommentAttributes... expected) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be 8-space indentation?

@xpdavid xpdavid 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 Aug 10, 2018
@xpdavid xpdavid merged commit c5e80a9 into TEAMMATES:master Aug 10, 2018
@xpdavid xpdavid added this to the V6.9.0 milestone Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants