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

[#12421] Comments for Responses for Essay Question #12426

Conversation

undermyumbrella1
Copy link
Contributor

@undermyumbrella1 undermyumbrella1 commented May 12, 2023

Fixes #12421

Approach 1:

  • make a pr to enable comments for each question type

  • involves changing the flag to allow comment for a question type

  • involves expanding set of question types that allows comment

  • update test to ensure flag returns true

  • when all question type allows comment, the 3 points above will be obsolete

  • aka everything will be deleted

  • checks will no longer be done to check if question type allows for comment
    (as all question allows comment)

Approach 2:

  • 1 pr to enable comments for all question type
  • removes flag, checker methods to check if a question type allows comment
  • the comment logic is done outside the question, so it is universal regardless of qn
  • tests is done in one place to check comment logic

I chose approach 2 as it was easier, do let me know if you prefer breaking it down by question type.

  • For now, submit responses for ALL questions button, will not save comments
  • only submit responses for question 1 button will save comment for qn 1
  • this is the same for existing logic that allows comment for mcq/msq question
  • This pr is to ensure all question types works the same way as existing logic for mcq/msq
  • I will make a seperate pr to address this
  • Or would you prefer I do it in this pr?

image
image
image
image
image

@github-actions
Copy link

Hi @ellaella12, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

  • Description must reference the issue number the PR is fixing, e.g. Fixes #<issue-number> (or Part of #<issue-number> if the PR does not address the issue fully)

Please address the above before we proceed to review your PR.

@undermyumbrella1 undermyumbrella1 changed the title [#12421] Feature/response comment essay question [#12421] Comments for Responses for Essay Question May 13, 2023
@cedricongjh cedricongjh self-requested a review May 13, 2023 17:37
@cedricongjh cedricongjh added the s.ToReview The PR is waiting for review(s) label May 13, 2023
Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

However, there appears to be errors when I try to submit a feedback response comment on non-MCQ and MRQ question types:

image

Do fix this and address the other comment regarding testing, thank you!

@jasonqiu212 jasonqiu212 added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels May 16, 2023
@undermyumbrella1
Copy link
Contributor Author

Hi, thank you for the review!

May I check how do i replicate the error message above?
I tried submitting for different qn types and got this
image

@cedricongjh
Copy link
Contributor

Hi, thank you for the review!

May I check how do i replicate the error message above? I tried submitting for different qn types and got this image

HI @ellaella12, I tried again but did not run into the error, so no worries about this!

@cedricongjh cedricongjh self-requested a review May 18, 2023 13:38
Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

Hi @ellaella12, there's another thing to consider that I missed out, we need to add in the student's comment in the downloaded result as well:

Click download result:
image

The csv has Giver comment column for MCQ and MRQ type questions
Uploading image.png…

Do add that in and re-request a review, thank you!

@undermyumbrella1
Copy link
Contributor Author

undermyumbrella1 commented Jun 23, 2023

Hi sorry about that, I have made a fix, it's working now. Thank you!

TEAMMATES.-.Online.Peer.Feedback_Evaluation.System.for.Student.Team.Projects.1.webm

@zhaojj2209
Copy link
Contributor

zhaojj2209 commented Jun 24, 2023

Thanks for the fix, great work so far!

One last issue I've managed to uncover: after submitting a feedback response to a distribute points (recipients) question, reloading the page and attempting to submit it again without making any changes will give the following error:

Screenshot 2023-06-24 at 22 17 14

The issue doesn't appear if you make any changes to the response before submitting the response again, and the issue is not present for any other question type.

Let me know if you need more clarifications on when the bug occurs.

This should be the last bug left (crosses fingers), after this the PR should be good to go 💪

Edit: My bad, found one more bug 🙇‍♀️ The response comments are not being submitted when clicking the "Submit Responses to All Questions" button, do look into it.

@undermyumbrella1
Copy link
Contributor Author

undermyumbrella1 commented Jun 28, 2023

Hi, thank you for the review! Sorry for the numerous bugs. have made a fix for submission without changing answer, by changing QuestionConstraintComponent to check is valid only in ngOnInit() when both the recipientSubmissionForms and questionDetails has initialized.
Originally, the check is valid is called right after recipientSubmissionForms is initialised, so default questionDetails are used = wrong validity check. Is this fix ok?
TEAMMATES - Online Peer Feedback_Evaluation System for Student Team Projects (2).webm

For this Edit: My bad, found one more bug 🙇‍♀️ The response comments are not being submitted when clicking the "Submit Responses to All Questions" button, do look into it.
The comment dated May 28 As for your question regarding the saving of comments when saving all questions, I agree that it should be in a separate PR, with a separate issue to track it.
Or should the change be implemented in this pr?

@zhaojj2209
Copy link
Contributor

zhaojj2209 commented Jun 28, 2023

For this Edit: My bad, found one more bug 🙇‍♀️ The response comments are not being submitted when clicking the "Submit Responses to All Questions" button, do look into it. The comment dated May 28 As for your question regarding the saving of comments when saving all questions, I agree that it should be in a separate PR, with a separate issue to track it. Or should the change be implemented in this pr?

My bad! This totally slipped my mind. Let's work on it in a separate PR as we discussed previously.

Will review the bug fix this weekend.

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

Thank you so much for the work done!

@wkurniawan07 Now that we are allowing feedback response participants to add comments for every question type, should we evaluate whether we should do the same for instructor comments as well? Currently instructors are allowed to add comments on all question types except team contribution.

@zhaojj2209 zhaojj2209 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 Jul 2, 2023
@damithc
Copy link
Contributor

damithc commented Jul 2, 2023

@wkurniawan07 Now that we are allowing feedback response participants to add comments for every question type, should we evaluate whether we should do the same for instructor comments as well? Currently instructors are allowed to add comments on all question types except team contribution.

@zhaojj2209 Perhaps it is better to leave out team contribution questions from this feature. The values shown to instructors could be different from those shown to students (due to normalizing etc.), so the comments instructors enter could be misleading to students.

Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

LGTM, great work on this!

@weiquu weiquu 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 Jul 2, 2023
@zhaojj2209 zhaojj2209 merged commit f029bb2 into TEAMMATES:master Jul 22, 2023
8 checks passed
@samuelfangjw samuelfangjw added the c.Feature User-facing feature; can be new feature or enhancement to existing feature label Aug 21, 2023
@samuelfangjw samuelfangjw added this to the V8.29.0 milestone Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature 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.

Comments for Responses for Essay Question
10 participants