Student: view results: rank questions: show average #8701 #8784

nidhi98gupta · 2018-04-14T17:22:19Z

Statistics to student is only shown when instructor sets required visibility settings i.e. visibility setting is permissive enough (i.e., if the ranks for all team members are in the results bundle).

Screenshot of changes

nidhi98gupta · 2018-04-16T16:05:23Z

I have end semester examination next week so I'll resume work on this PR after a few days.
Apologies for the delay.

nidhi98gupta · 2018-05-07T07:11:15Z

@joanneong Rank recipient review is ready. Would you prefer to review this first or after I make changes in rank (option) question too?

joanneong · 2018-05-07T18:57:32Z

@nidhi98gupta If you don't mind waiting a couple more days, I should be able to take a look at this by the end of this week :)

joanneong · 2018-05-13T16:13:56Z

@nidhi98gupta My apologies for the delay - the end of the school semester is always chaotic. I will try to give you a full review by tomorrow, but in the meantime, could you upload some screenshots/ gif of the changes that you have made so that it's easier for other reviewers as well? :)

nidhi98gupta · 2018-05-18T09:31:40Z

Hi @joanneong, I have added screenshot in PR description.

joanneong

Overall the current code that is here is okay. However, while testing, I realised a problem:

Create a feedback form that asks students in the course to give feedback to instructors in the course
Make the visibility setting "Shown anonymously to recipient and team members, visible to instructor

There is an error when a student tries to view his/her responses for such a question. More specifically, the error is a NullPointerException, so there are probably logic holes in your FeedbackRankRecipientsQuestionDetails.java.

Try to resolve it first, otherwise, I can try to look for the root cause as well. Sorry for the late review! :(

joanneong · 2018-05-27T07:03:20Z

src/test/resources/pages/instructorFeedbackResultsEditCommentByDifferentInstructor.html

@@ -995,7 +995,7 @@ <h4 class="modal-title">
                                    </table>
                                  </div>
                                  <div class="form-group">
-                                    <div class="panel panel-default panel-body content-editor" id="responsecommenttext-1-0-1-1-1" spellcheck="false" style="position: relative;">
+                                    <div class="panel panel-default panel-body content-editor empty" id="responsecommenttext-1-0-1-1-1" spellcheck="false" style="position: relative;">


Why is this change necessary?

I don't know why this file got changed in God Mode. Reverting it.

It's happening quite often and is an unrelated GodMode change. I am investigating it.

I am guessing the test is unstable and hence fails and corrects the HTML files at times (which shouldn't happen)

nidhi98gupta · 2018-05-28T07:45:10Z

@joanneong Ready for review.

nidhi98gupta · 2018-06-04T03:15:00Z

@joanneong A reminder.

xpdavid

Again It's me :D

xpdavid · 2018-06-11T16:49:17Z

...main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

@@ -238,6 +239,79 @@ public String getQuestionAdditionalInfoHtml(int questionNumber,
                        Slots.QUESTION_ADDITIONAL_INFO, additionalInfo);
    }

+    private List<FeedbackResponseAttributes> getActualResponses(


response param in getQuestionResultStatisticsHtml already contains the response of the question?

response param contains only limited number of responses sufficient to show responses received by student but not sufficient to build statistics.

Then please give a better name. I would make more sense to add this method in FeedbackSessionResultsBundle.java. In that case, please also add some unit tests.
getAllResponsesForQuestion(question)

I am not sure where to add unit tests. Please help.

xpdavid · 2018-06-11T16:49:58Z

...main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

+        return responses;
+    }
+
+    private String getStudentQuestionResultsStatisticsHtml(


Add a Javadoc although it is a private method.
State the behaviour of the method.

xpdavid · 2018-06-11T16:54:33Z

...main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

+        Map<String, Integer> recipientSelfRanks = generateSelfRankForEachRecipient(actualResponses);
+
+        String fragmentTemplateToUse = FormTemplates.RANK_RESULT_STATS_RECIPIENTFRAGMENT;
+        String templateToUse = FormTemplates.RANK_RESULT_RECIPIENT_STATS;


Use the constant directly.

xpdavid · 2018-06-11T16:55:04Z

...main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

+
+        String statsTitle = isRecipientTypeTeam ? "Summary of responses received by your team"
+                : "Summary of responses received by you";
+        return Templates.populateTemplate(templateToUse,


FormTemplates.RANK_RESULT_RECIPIENT_STATS

xpdavid · 2018-06-11T16:55:12Z

...main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

+        String selfRank = recipientSelfRanks.containsKey(currentUserIdentifier)
+                ? Integer.toString(recipientSelfRanks.get(currentUserIdentifier)) : "-";
+
+        fragments.append(Templates.populateTemplate(fragmentTemplateToUse,


FormTemplates.RANK_RESULT_STATS_RECIPIENTFRAGMENT;

xpdavid · 2018-06-11T16:58:02Z

...main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

+                Slots.RANK_EXCLUDING_SELF_OVERALL, overallRankExceptSelf));
+
+        String statsTitle = isRecipientTypeTeam ? "Summary of responses received by your team"
+                : "Summary of responses received by you";


Summary of responses you received？

Numerical questions statistics has Summary of responses received by you. Title should be consistent?

xpdavid · 2018-06-11T16:59:31Z

src/test/resources/pages/studentFeedbackResultsPageRank.html

+        <div class="row">
+          <div class="col-sm-4 text-color-gray">
+            <strong>
+              Summary of responses received by you


Testing situation for team?

Added test for team.

xpdavid · 2018-06-12T12:13:49Z

...main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

@@ -238,6 +239,79 @@ public String getQuestionAdditionalInfoHtml(int questionNumber,
                        Slots.QUESTION_ADDITIONAL_INFO, additionalInfo);
    }

+    private List<FeedbackResponseAttributes> getActualResponses(


Then please give a better name. I would make more sense to add this method in FeedbackSessionResultsBundle.java. In that case, please also add some unit tests.
getAllResponsesForQuestion(question)

xpdavid · 2018-06-12T12:14:20Z

...main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

+        String questionId = question.getId();
+        //Get all actual responses for this question.
+        responses = new ArrayList<>();
+        for (FeedbackResponseAttributes response : bundle.actualResponses) {


Can make use of .stream().filter.... in Java8

nidhi98gupta · 2018-07-14T10:29:32Z

@xpdavid Reminder to review.

xpdavid · 2018-07-15T13:30:45Z

src/main/java/teammates/common/datatransfer/FeedbackSessionResultsBundle.java

+    /**
+     * Returns list of responses sorted by giverName > recipientName > qnNumber with identities
+     * of giver/recipients NOT hidden which is used for anonymous result calculation.
+     * @param question question whose responses are required


empty line between javadoc and param description

xpdavid · 2018-07-15T13:31:08Z

src/main/java/teammates/common/datatransfer/FeedbackSessionResultsBundle.java

+    /**
+     * Returns list of unsorted responses with identities of giver/recipients NOT hidden which is used for
+     * anonymous result calculation.
+     * @param question question whose responses are required


xpdavid · 2018-07-15T13:33:52Z

...main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

+            return "";
+        }
+
+        StringBuilder fragments = new StringBuilder();


Put variable declaration to L296

xpdavid · 2018-07-15T13:37:06Z

src/test/java/teammates/test/cases/datatransfer/FeedbackSessionResultsBundleTest.java

+        List<String> allResponsesString = new ArrayList<>();
+        allResponsesString.add(allResponses.get(0).toString());
+        allResponsesString.add(allResponses.get(1).toString());
+        assertTrue("Responses are missing", allResponsesString.containsAll(allExpectedResponses));


Can we use assertEquals()

xpdavid · 2018-07-15T13:46:05Z

src/test/resources/data/FeedbackSessionResultsBundleTest.json

+        "INSTRUCTORS"
+      ]
+    },
+    "qn4InSession1InCourse1": {


Put test data here only if they are necessary. If you are only going to question 1, 3 (why not 2?) and their corresponding responses, there is no need to have question 2,4,5.

xpdavid · 2018-07-15T13:46:25Z

src/test/resources/data/FeedbackSessionResultsBundleTest.json

+      "isClosingEmailEnabled": true,
+      "isPublishedEmailEnabled": true
+    },
+    "session2InCourse1": {


Same for session

xpdavid · 2018-07-15T13:46:32Z

src/test/resources/data/FeedbackSessionResultsBundleTest.json

+        "sessionLevel": {}
+      }
+    },
+    "instructor2OfCourse1": {


And instructor

xpdavid · 2018-07-15T13:46:39Z

src/test/resources/data/FeedbackSessionResultsBundleTest.json

+      "email": "helper@course1.tmt",
+      "institute": "TEAMMATES Test Institute 1"
+    },
+    "student1InCourse1": {


nidhi98gupta · 2018-07-16T21:24:53Z

@xpdavid Ready for review.

xpdavid

Good work.

@joanneong anything to add on?

joanneong

Looks pretty ready to me. I checked both the functionality and the code quality, and I've only two comments below :)

joanneong · 2018-07-22T11:30:12Z

...main/java/teammates/common/datatransfer/questions/FeedbackRankRecipientsQuestionDetails.java

+     */
+    private String getStudentQuestionResultsStatisticsHtml(String studentEmail, FeedbackQuestionAttributes question,
+            FeedbackSessionResultsBundle bundle) {
+        // at least should be able to viewed by other students for team recipient


Minor nit: "at least should be able to be viewed by..."

joanneong · 2018-07-22T11:59:54Z

src/test/resources/data/FeedbackSessionResultsBundleTest.json

+        "nationality": "American",
+        "gender": "male",
+        "moreInfo": "I am just a student :P",
+        "pictureKey": "asdf34&hfn3!@"


Can I just check why there are two different "institute"s for student1InCourse1?

Yes, that's a mistake. I'll fix it.

nidhi98gupta · 2018-07-22T23:09:45Z

@joanneong Ready for review.

joanneong

LGTM :)

@damithc , @wkurniawan07 , a final review?

damithc

Code quality seem fine 👍

joanneong · 2018-07-24T08:19:23Z

@damithc We still have to wait for wilson's review right :P

damithc · 2018-07-24T08:45:39Z

@damithc We still have to wait for wilson's review right :P

@wkurniawan07 have a look if you have time (optional).

@joanneong as @xpdavid has approved the PR, you can merge tonight if there are no change requests from @wkurniawan07 by then.

xpdavid · 2018-07-25T04:03:54Z

Merge now. If there are any comments by @wkurniawan07, can discuss here.

…EAMMATES#8784) [TEAMMATES#8701] Student: view results: rank questions: show average (TEAMMATES#8784)

Add getStudentQuestionResultsStatisticsHtml

613b211

whipermr5 added the s.Ongoing The PR is being worked on by the author(s) label Apr 15, 2018

joanneong self-assigned this Apr 16, 2018

nidhi98gupta added 7 commits May 6, 2018 11:03

Merge branch 'master' into 8701-show-average-in-rank-questions

6643a3c

Add required conditions for rank summary

83a3981

Remove trailing spaces

10d4b59

Remove repeated condition

cf02f57

Update Html pages

62f206c

fix pmd error

577bdde

Update Html file

08067e4

joanneong self-requested a review May 13, 2018 16:14

joanneong added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels May 23, 2018

joanneong requested changes May 27, 2018

View reviewed changes

joanneong 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 27, 2018

nidhi98gupta added 4 commits May 27, 2018 14:17

Merge branch 'master' into 8701-show-average-in-rank-questions

e0279cc

Fix null pointer exception

f80a560

Unrelated changes

a648b36

fix pmd errors

33e7bdc

xpdavid requested changes Jun 11, 2018

View reviewed changes

xpdavid self-assigned this Jun 11, 2018

xpdavid reviewed Jun 12, 2018

View reviewed changes

Remove not needed variable

5917b95

xpdavid reviewed Jul 15, 2018

View reviewed changes

nidhi98gupta added 2 commits July 16, 2018 23:25

Remove unnecessary data

c815cd9

Undo unintended changes

5cbedc6

xpdavid approved these changes Jul 17, 2018

View reviewed changes

xpdavid added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Jul 21, 2018

joanneong reviewed Jul 22, 2018

View reviewed changes

Minor changes

c4324a9

joanneong approved these changes Jul 23, 2018

View reviewed changes

joanneong added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Jul 23, 2018

damithc approved these changes Jul 23, 2018

View reviewed changes

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 Jul 25, 2018

Merge branch 'master' into 8701-show-average-in-rank-questions

99b575b

xpdavid merged commit b3f4581 into TEAMMATES:master Jul 25, 2018

xpdavid added s.FinalReview The PR is ready for final review c.Feature User-facing feature; can be new feature or enhancement to existing feature and removed s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging labels Jul 25, 2018

jacoblipech pushed a commit to jacoblipech/teammates that referenced this pull request Jul 25, 2018

[TEAMMATES#8701] Student: view results: rank questions: show average (T…

6af526c

…EAMMATES#8784) [TEAMMATES#8701] Student: view results: rank questions: show average (TEAMMATES#8784)

xpdavid added this to the V6.8.0 milestone Jul 29, 2018

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 Jul 29, 2018

Student: view results: rank questions: show average #8701 #8784

Student: view results: rank questions: show average #8701 #8784

Conversation

nidhi98gupta commented Apr 14, 2018 • edited Loading

nidhi98gupta commented Apr 16, 2018

nidhi98gupta commented May 7, 2018

joanneong commented May 7, 2018

joanneong commented May 13, 2018

nidhi98gupta commented May 18, 2018

joanneong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nidhi98gupta commented May 28, 2018

nidhi98gupta commented Jun 4, 2018

xpdavid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nidhi98gupta commented Jul 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nidhi98gupta commented Jul 16, 2018

xpdavid left a comment

Choose a reason for hiding this comment

joanneong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nidhi98gupta commented Jul 22, 2018

joanneong left a comment

Choose a reason for hiding this comment

damithc left a comment

Choose a reason for hiding this comment

joanneong commented Jul 24, 2018

damithc commented Jul 24, 2018

xpdavid commented Jul 25, 2018

nidhi98gupta commented Apr 14, 2018 •

edited

Loading