[#8918] Instructor: view results: questions for ranking: show rank within team #10895

AdithyaNarayan · 2020-12-25T21:23:05Z

Fixes #8918

Outline of Solution

Created new member fields in the rank-recipient-question-statistics-calculation class for team rank.
- In calculateRanks, all the teams and their team members are captured in the optionsPerTeam variable, which maps team names to its team members.
- For each team, the calculateRankPerOption is called with only the team members' responses, and the ranks within each team are calculated.
Likewise, the process is repeated for team rank excluding self.
Added test case for ranks within a team.

Screenshot

Points to Note
The solution assumes that team names are unique.

rrtheonlyone

Thanks for the PR 👍

We should only be showing the team rank when it makes sense (we should check against the recipientType)

For example, when ranking teams there is no need to show team rank:

AdithyaNarayan · 2021-01-14T08:26:39Z

Okay, thanks for the comment! I have made sure that the team ranking is shown only when the recipientType is OWN_TEAM_MEMBERS or OWN_TEAM_MEMBERS_INCLUDING_SELF.

rrtheonlyone

Looks good! Just some minor comments on the implementation/tests

Also do update the help section as well to explain what the team rank is all about

...nts/question-types/question-statistics/rank-recipients-question-statistics.component.spec.ts

...tatistics/question-statistics-calculation/rank-recipients-question-statistics-calculation.ts

AdithyaNarayan · 2021-01-30T05:17:41Z

Done👍

AdithyaNarayan · 2021-02-21T10:16:04Z

Is there anything more to do for this PR?

rrtheonlyone

LGTM 💯 Nice one

Apologies for delay in this @AdithyaNarayan .. should be good to go once CI passes

madanalogy · 2021-02-22T03:23:28Z

...nts/question-types/question-statistics/rank-recipients-question-statistics.component.spec.ts

+ * Loads data for testing.
+ */
+const loadTestData: (filename: string) => Response<FeedbackRankRecipientsResponseDetails>[] =
+(filename: string): Response<FeedbackRankRecipientsResponseDetails>[] => {


Shouldn't this be indented? But okay not a big deal since the lint check passes

madanalogy · 2021-02-22T03:26:11Z

...help-page/instructor-help-questions-section/instructor-help-questions-section.component.html

@@ -658,6 +658,9 @@ <h4 *ngIf="displaySubsection(0, 10)">Setting Up Questions</h4>
      <li>
        <b>Overall rank</b>: the recipient's rank relative to other recipients, as computed by TEAMMATES.
      </li>
+      <li>
+        <b>Team rank</b>: the recipient's rank relative to other recipients within their own team only. (Applicable only if students rank their teammates)


Brackets not needed

madanalogy · 2021-02-22T03:33:44Z

...nts/question-types/question-statistics/rank-recipients-question-statistics.component.spec.ts

+ */
+const loadTestData: (filename: string) => Response<FeedbackRankRecipientsResponseDetails>[] =
+(filename: string): Response<FeedbackRankRecipientsResponseDetails>[] => {
+  return require(`./test-data/${filename}`);


Any particular reason why test data is stored in a separate file? I'm not entirely against the approach don't get me wrong, just that our frontend tests so far all have their data embedded in the spec.ts file so I'm wondering if you had any particular reason for the extraction of data.

I can understand the extraction if the same data is being used by other test components but this doesn't seem to be the case. Would be better for now to just have the data as a constant variable as in other frontend tests for consistency (e.g. instructor-home-page.component.spec.ts)

Just felt that the code looked bad cosmetically with sample data and decided to put it in a JSON. Reverted

You could consider putting up an issue for this actually, I remember it was something I wanted to raise but didn't have the time to. That is to say, refactor frontend test data into a dedicated resource folder for reusability instead of having it nested within the spec.ts files. We can have a community discussion about this, and if the idea is adopted start taking the approach as an epic/task.

madanalogy · 2021-02-22T03:36:03Z

...help-page/instructor-help-questions-section/instructor-help-questions-section.component.html

@@ -672,6 +675,12 @@ <h4 *ngIf="displaySubsection(0, 10)">Setting Up Questions</h4>
          The <b>Overall Rank</b> ranks the average rank each recipient receives.
          For example, if recipient A received the ranks <code>(1, 2)</code> and recipient B received the ranks <code>(2, 4, 6)</code>, then recipient A and recipient B's average ranks are 1.5 and 4 respectively. By ranking these two averages, recipient A and B will get an <b>Overall Rank</b> of 1 and 2 respectively.
        </li>
+        <li>
+          The <b>Team Rank</b> works in a similar way to <b>Overall Rank</b> as it uses the average rank received to calculate the rank within the team. to the average rank each recipient receives. However, the average rank of the members are only compared to their own team members.


Typo in the sentence, I'm assuming the period at the end of "team" was unintended

madanalogy · 2021-02-22T03:36:24Z

...help-page/instructor-help-questions-section/instructor-help-questions-section.component.html

+        <li>
+          The <b>Team Rank</b> works in a similar way to <b>Overall Rank</b> as it uses the average rank received to calculate the rank within the team. to the average rank each recipient receives. However, the average rank of the members are only compared to their own team members.
+          For example, if recipient A (who is a part of team X) received the ranks <code>(1, 2)</code> and recipient B (who is a part of team X) received the ranks <code>(2, 4, 6)</code>, then recipient A and recipient B's average ranks are 1.5 and 4 respectively. By ranking these two averages, recipient A and B will get a <b>Team Rank</b> of 1 and 2 respectively.
+          And if recipient C (who is a part of team Y) received the ranks <code>(1, 1)</code> and recipient D (who is a part of team Y) received the ranks <code>(1, 2)</code>, then recipient C and recipient D's average ranks are 1 and 1.5 respectively. By ranking these two averages, recipient C and D will get a <b>Team Rank</b> of 1 and 2 respectively.


Unnecessary starting And, can be removed

AdithyaNarayan · 2021-02-22T11:19:17Z

Addressed the comments. Thanks for the feedback

madanalogy

LGTM! Will merge assuming no issues with the CI checks

Adithya Narayan Rangarajan Sreenivasan added 4 commits December 25, 2020 20:12

Add team rank and team rank excluding self

c681f97

Add test for team rank

2635a5e

Update snapshots

ca8146d

Fix lint errors

7268d9b

madanalogy added the s.ToReview The PR is waiting for review(s) label Dec 27, 2020

rrtheonlyone self-assigned this Dec 31, 2020

rrtheonlyone requested changes Dec 31, 2020

View reviewed changes

rrtheonlyone 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 Dec 31, 2020

Adithya Narayan Rangarajan Sreenivasan added 3 commits January 9, 2021 14:29

Merge branch 'master' into 8918-show-rank-within-team

a41dec9

Add recipient type check for team rank calculation

d1b9cc7

Trigger re-run of e2e tests

872a814

Merge branch 'master' into 8918-show-rank-within-team

7fdaeec

rrtheonlyone requested changes Jan 17, 2021

View reviewed changes

...nts/question-types/question-statistics/rank-recipients-question-statistics.component.spec.ts Outdated Show resolved Hide resolved

...tatistics/question-statistics-calculation/rank-recipients-question-statistics-calculation.ts Outdated Show resolved Hide resolved

Add help page and declerative methods

f63610b

Merge branch 'master' into 8918-show-rank-within-team

6681379

rrtheonlyone 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 Feb 7, 2021

Merge branch 'master' into 8918-show-rank-within-team

321239a

rrtheonlyone approved these changes Feb 21, 2021

View reviewed changes

rrtheonlyone added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Feb 21, 2021

rrtheonlyone requested a review from madanalogy February 21, 2021 16:12

madanalogy requested changes Feb 22, 2021

View reviewed changes

madanalogy self-assigned this Feb 22, 2021

madanalogy added s.Ongoing The PR is being worked on by the author(s) and removed s.FinalReview The PR is ready for final review labels Feb 22, 2021

Move test-data from json into spec.ts file

dbd59f8

madanalogy approved these changes Feb 22, 2021

View reviewed changes

madanalogy added 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 and removed s.Ongoing The PR is being worked on by the author(s) labels Feb 22, 2021

madanalogy added this to the V7.11.0 milestone Feb 22, 2021

Merge branch 'master' into 8918-show-rank-within-team

e9bc199

madanalogy merged commit 4e44320 into TEAMMATES:master Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#8918] Instructor: view results: questions for ranking: show rank within team #10895

[#8918] Instructor: view results: questions for ranking: show rank within team #10895

AdithyaNarayan commented Dec 25, 2020

rrtheonlyone left a comment

AdithyaNarayan commented Jan 14, 2021

rrtheonlyone left a comment •

edited

AdithyaNarayan commented Jan 30, 2021

AdithyaNarayan commented Feb 21, 2021

rrtheonlyone left a comment

madanalogy Feb 22, 2021

madanalogy Feb 22, 2021

madanalogy Feb 22, 2021

AdithyaNarayan Feb 22, 2021

madanalogy Feb 22, 2021

madanalogy Feb 22, 2021

madanalogy Feb 22, 2021

AdithyaNarayan commented Feb 22, 2021

madanalogy left a comment

[#8918] Instructor: view results: questions for ranking: show rank within team #10895

[#8918] Instructor: view results: questions for ranking: show rank within team #10895

Conversation

AdithyaNarayan commented Dec 25, 2020

rrtheonlyone left a comment

Choose a reason for hiding this comment

AdithyaNarayan commented Jan 14, 2021

rrtheonlyone left a comment • edited

Choose a reason for hiding this comment

AdithyaNarayan commented Jan 30, 2021

AdithyaNarayan commented Feb 21, 2021

rrtheonlyone left a comment

Choose a reason for hiding this comment

madanalogy Feb 22, 2021

Choose a reason for hiding this comment

madanalogy Feb 22, 2021

Choose a reason for hiding this comment

madanalogy Feb 22, 2021

Choose a reason for hiding this comment

AdithyaNarayan Feb 22, 2021

Choose a reason for hiding this comment

madanalogy Feb 22, 2021

Choose a reason for hiding this comment

madanalogy Feb 22, 2021

Choose a reason for hiding this comment

madanalogy Feb 22, 2021

Choose a reason for hiding this comment

AdithyaNarayan commented Feb 22, 2021

madanalogy left a comment

Choose a reason for hiding this comment

rrtheonlyone left a comment •

edited