Assessments: Overview table - calculate not answered value #22156

nkiruka · 2018-05-02T17:02:41Z

This PR is to calculate the percent of students that did not answer a question and to display the value in the "not Answered column".

-In PR 22118, a dash is displayed in empty columns.

In this PR, the default value for empty columns is 0%
-The Not Answered column shows a teacher the percent of students that did not answer a question.

Implementation

Add a function to calculate the "not Answered" value
Add "notAnswered" prop to the multiple choice cell component

Next Steps:

Refactor code
Style table

nkiruka · 2018-05-02T18:43:00Z

@balderdash Please review

Erin007 · 2018-05-02T19:14:36Z

apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.jsx

-      />
-  );
-};
+const alphabetMapper =  [


What if this was named something like "multipleChoiceAnswerOptions"? Might be more descriptive.

Good idea! Thanks

Erin007 · 2018-05-02T19:22:59Z

apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.story.jsx

+               {percentAnswered: 20, isCorrectAnswer: false},
+               {percentAnswered: 20, isCorrectAnswer: false},
+               {percentAnswered: 20, isCorrectAnswer: false},
+    ],


maybe I'm confused, but I'm a little concerned that we're relying on order to match percentAnswered to specific answer options (eg. "A"). What if we had something like {multipleChoiceOption: "A", percentAnswered: 40, isCorrectAnswer: true}? Then all of the relevant data gets bundled together.

If you want the translatable string, I think you could do:
{multipleChoiceOption: {i18n.answerOptionA()}, percentAnswered: 40, isCorrectAnswer: true}

@Erin007 This is a valid point! After our in-person discussion, there is a tremendous payoff being able to identify which percentAnswered is associated with the answer choice. Additionally, debugging is easier. Thanks!

caleybrock · 2018-05-02T19:46:35Z

I changed the base on this PR to your original branch so that we only see the new changes :)

caleybrock · 2018-05-02T22:31:20Z

We chatted about component organization offline and @nkiruka will update.

Erin007 · 2018-05-04T00:30:24Z

apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.jsx

+  if (property === 'notAnswered') {
+     percentValue = calculateNotAnswered(rowData.answers);
+  } else {
+     percentValue = (cell && cell.percentAnswered) || '-';


If you set percentValue to "-" rather than to a number here, MultipleChoiceAnswerCell will be confused because it's expecting that propType to be a number.

You have a few choices:

1.) Instead of "-" you could set percentValue to 0, then have MultipleChoiceAnswerCell just render "0%"
2.) Instead of "-" you could set percentValue to 0. Then, you could introduce some logic in MultipleChoiceAnswerCell if percentValue === 0 render "-"
3.) Adjust the propTypes in MultipleChoiceAnswerCell to use oneOfType, explained here

Which option are you leaning toward? Other idea? If you want to chat through the pros/cons of each, let me know.

Erin007 · 2018-05-04T00:43:19Z

apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.jsx

+        }
+    });
+
+    return (100 - total);


I asked "Do we have any multiple choice questions where students can select multiple answers, like a "select all that apply..." ?" in Slack because, if that's the case, you could run into a situation where calculateNotAnswered returns a negative number. For example, let's say there are only two answer choices: "A" and "B" and students are allowed to select: only "A", only "B", none, or "A" AND "B". If 100% of the students selected "A" AND "B", total in this function is 200, and you'd return -100. Let's confirm the requirements with Poorva.

Good question. Since this is an overview table of all the students' assessments, I do not think this will be an issue. For the individual student tables, this will be applicable (a good test :-) to check). Thoughts!

If students can select multiple answer options, I'm still concerned about calculateNotAnswered for this overview table. For example we could have a scenario like:

In this case we would want the % not answered to be 25% because 1 out of 4 students did not answer the question; however the function as written would return 0%.

This looks like a great unit test case!

I concur!! Tests will be covered in the next PR.

nkiruka · 2018-05-04T16:00:51Z

@caleybrock @Erin007 Please see updated code for review

caleybrock · 2018-05-04T16:57:39Z

apps/src/templates/sectionAssessments/MultipleChoiceAnswerCell.jsx

+            <span>{`${percentValue}%`}</span>
+          }
+          {(percentValue < 0 ) &&
+            <span>{'-'}</span>


I thought you said Poorva likes it without -?

Poorva also likes it with the dash and she is open to either option. I ran a mini survey with Maddie and Erin and the 'dash' option is preferred.

caleybrock · 2018-05-04T16:59:27Z

apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.jsx

+const NOT_ANSWERED = 'notAnswered';
+
+const calculateNotAnswered = (multipleChoiceDataArr) => {
+    let total = 0;


nit: indentation in this function

Fixed, Thanks

caleybrock · 2018-05-04T17:09:42Z

apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.jsx

+
+  let percentValue = 0;
+
+  if (property === 'notAnswered') {


You could use your constant here instead of this string.

Fixed, Thanks

caleybrock · 2018-05-04T17:12:05Z

apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.jsx

-    const maxAnswerChoicesLength = this.props.questionAnswerData.reduce((answersTotal, currentAnswerCount) => {
-      return Math.max(answersTotal, currentAnswerCount.answers.length);
-    }, 0);
-
    let dataColumns = [];


I'm not sure I totally understand the reason for the re-factoring in this function but I do have some suggestions to make it easier to read.
Your new code creates an empty dataColumns, pushes one element to it, and then deconstructs it to add it to an array. Would it be simpler to just add this.getQuestionColumn(sortable) to your array like this?

return [ this.getQuestionColumn(sortable), ...columnLabelNames, this.getNotAnsweredColumn()}, ];

I think it could make your final constructed array easier to read.
Note: you could make this.getNotAnsweredColumn() return an object that already has the property NOT_ANSWERED so you don't have to reconstruct it here.

Thanks, it does make it easier to read. Fixed.

nkiruka · 2018-05-08T16:57:09Z

Please review. Next PR will contain the refactored code with the new data structures. Thanks.

nkiruka · 2018-05-08T17:34:38Z

@caleybrock Please see updated changes. Thanks

caleybrock · 2018-05-08T17:39:33Z

apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.jsx

    let dataColumns = [];
+
    dataColumns.push(this.getQuestionColumn(sortable));


dataColumns is not longer used - so you can delete it.

nkiruka · 2018-05-08T17:45:31Z

@caleybrock Please review. Thanks

caleybrock

Looks good given we address Erin's concerns in a follow up PR

nkiruka added 5 commits May 1, 2018 22:29

add function to calculate notAnswered value

230227c

add notAnswered prop

4fb8dc9

update multiple choice data

d231ca4

add notAnswered prop

f7369ce

Add cell formatter to notAnswered

549baec

nkiruka added access report teacher-dashboard and removed access report labels May 2, 2018

nkiruka requested review from caleybrock and Erin007 May 2, 2018 17:03

nkiruka assigned caleybrock and joshlory May 2, 2018

Erin007 reviewed May 2, 2018

View reviewed changes

caleybrock changed the base branch from staging to assessments-answer-columns May 2, 2018 19:45

nkiruka added 2 commits May 3, 2018 09:02

remove not Answered prop

f35273f

update code per PR review

3ec3594

Erin007 reviewed May 4, 2018

View reviewed changes

nkiruka added 6 commits May 3, 2018 18:58

update code per Pr

948b52f

add multiple choice option

e6c4526

update code per PR review

3260af7

Add default props

47b70ee

refactor code

c547f58

merge conflict

fc904b1

nkiruka requested a review from maddiedierker May 4, 2018 16:19

code clean

9fae7d1

nkiruka changed the base branch from assessments-answer-columns to staging May 4, 2018 16:50

caleybrock reviewed May 4, 2018

View reviewed changes

nkiruka added 2 commits May 4, 2018 11:43

code clean

e7d313f

update code per PR review

7374363

update code per PR review

afc67e8

caleybrock reviewed May 8, 2018

View reviewed changes

remove unused data columns array

a48213f

caleybrock approved these changes May 8, 2018

View reviewed changes

nkiruka added 3 commits May 8, 2018 13:41

change percentAnswered proptype

49374e1

Add percentValue to props

fd61e59

fixed storybook error

3899573

nkiruka merged commit ed81c60 into staging May 14, 2018

This was referenced May 14, 2018

Assessments: Assessment data format #22400

Merged

Assessments: Multiple choice table test #22411

Merged

nkiruka deleted the calculate-notAnswered branch September 17, 2018 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assessments: Overview table - calculate not answered value #22156

Assessments: Overview table - calculate not answered value #22156

nkiruka commented May 2, 2018 •

edited

nkiruka commented May 2, 2018

Erin007 May 2, 2018

nkiruka May 2, 2018

Erin007 May 2, 2018 •

edited

Erin007 May 2, 2018 •

edited

nkiruka May 2, 2018

caleybrock commented May 2, 2018

caleybrock commented May 2, 2018

Erin007 May 4, 2018 •

edited

Erin007 May 4, 2018

nkiruka May 4, 2018

Erin007 May 4, 2018

joshlory May 7, 2018

nkiruka May 8, 2018

nkiruka commented May 4, 2018

caleybrock May 4, 2018

nkiruka May 4, 2018

caleybrock May 4, 2018

caleybrock May 4, 2018

nkiruka May 4, 2018

caleybrock May 4, 2018

nkiruka May 4, 2018

caleybrock May 4, 2018 •

edited

nkiruka May 8, 2018

nkiruka commented May 8, 2018

nkiruka commented May 8, 2018

caleybrock May 8, 2018

nkiruka commented May 8, 2018

caleybrock left a comment

		let dataColumns = [];

		dataColumns.push(this.getQuestionColumn(sortable));

Assessments: Overview table - calculate not answered value #22156

Assessments: Overview table - calculate not answered value #22156

Conversation

nkiruka commented May 2, 2018 • edited

nkiruka commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Erin007 May 2, 2018 • edited

Choose a reason for hiding this comment

Erin007 May 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleybrock commented May 2, 2018

caleybrock commented May 2, 2018

Erin007 May 4, 2018 • edited

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

nkiruka commented May 4, 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

caleybrock May 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkiruka commented May 8, 2018

nkiruka commented May 8, 2018

Choose a reason for hiding this comment

nkiruka commented May 8, 2018

caleybrock left a comment

Choose a reason for hiding this comment

nkiruka commented May 2, 2018 •

edited

Erin007 May 2, 2018 •

edited

Erin007 May 2, 2018 •

edited

Erin007 May 4, 2018 •

edited

caleybrock May 4, 2018 •

edited