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.

Sign up for GitHub

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

Jump to bottom

Assessments: Overview table - calculate not answered value #22156

Merged
merged 21 commits into from
May 14, 2018
Merged

Assessments: Overview table - calculate not answered value #22156

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
230227c
add function to calculate notAnswered value
nkiruka May 2, 2018
4fb8dc9
add notAnswered prop
nkiruka May 2, 2018
d231ca4
update multiple choice data
nkiruka May 2, 2018
f7369ce
add notAnswered prop
nkiruka May 2, 2018
549baec
Add cell formatter to notAnswered
nkiruka May 2, 2018
f35273f
remove not Answered prop
nkiruka May 3, 2018
3ec3594
update code per PR review
nkiruka May 3, 2018
948b52f
update code per Pr
nkiruka May 4, 2018
e6c4526
add multiple choice option
nkiruka May 4, 2018
3260af7
update code per PR review
nkiruka May 4, 2018
47b70ee
Add default props
nkiruka May 4, 2018
c547f58
refactor code
nkiruka May 4, 2018
fc904b1
merge conflict
nkiruka May 4, 2018
9fae7d1
code clean
nkiruka May 4, 2018
e7d313f
code clean
nkiruka May 4, 2018
7374363
update code per PR review
nkiruka May 8, 2018
afc67e8
update code per PR review
nkiruka May 8, 2018
a48213f
remove unused data columns array
nkiruka May 8, 2018
49374e1
change percentAnswered proptype
nkiruka May 8, 2018
fd61e59
Add percentValue to props
nkiruka May 11, 2018
3899573
fixed storybook error
nkiruka May 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions apps/src/templates/sectionAssessments/MultipleChoiceAnswerCell.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ const styles = {
color: color.level_perfect,
float: 'left',
width: 20,
fontSize: 25,
},
text: {
color: color.charcoal,
Expand All @@ -25,12 +24,13 @@ const styles = {
float: 'left',
paddingRight: 40,
width: 20,
textAlign: 'center',
},
};

class MultipleChoiceAnswerCell extends Component {
static propTypes = {
percentValue: PropTypes.string.isRequired,
percentValue: PropTypes.number.isRequired,
isCorrectAnswer: PropTypes.bool,
};

Expand All @@ -39,7 +39,7 @@ class MultipleChoiceAnswerCell extends Component {
return (
<div>
<div>
{percentValue}
{`${percentValue}%`}
</div>
<div>
{isCorrectAnswer &&
Expand Down
25 changes: 23 additions & 2 deletions apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,34 @@ const alphabetMapper = [
commonMsg.answerOptionG(),
];

const calculateNotAnswered = (multipleChoiceDataArr) => {
let total = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation in this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, Thanks

multipleChoiceDataArr.forEach (studentsAnswersObj => {
if (studentsAnswersObj.percentAnswered) {
total += studentsAnswersObj.percentAnswered;
}
});

return (100 - total);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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:
img_0564
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%.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a great unit test case!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

};

const answerColumnsFormatter = (percentAnswered, {rowData, columnIndex, rowIndex, property}) => {
const cell = rowData.answers[columnIndex - 1];

let percentValue = 0;

if (property === 'notAnswered') {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use your constant here instead of this string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, Thanks

percentValue = calculateNotAnswered(rowData.answers);
} else {
percentValue = (cell && cell.percentAnswered) || '-';
Copy link
Contributor

@Erin007 Erin007 May 4, 2018 •
edited

Choose a reason for hiding this comment

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

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.

}

return (
<MultipleChoiceAnswerCell
id={rowData.id}
percentValue={(cell && `${cell.percentAnswered}%`) || '-'}
percentValue={percentValue}
isCorrectAnswer={cell && cell.isCorrectAnswer}

/>
);
};
Expand All @@ -36,7 +57,6 @@ const questionAnswerDataPropType = PropTypes.shape({
question: PropTypes.string,
percentAnswered: PropTypes.string,
isCorrectAnswer: PropTypes.bool,
notAnswered: PropTypes.string,
});

class MultipleChoiceOverviewTable extends Component {
Expand Down Expand Up @@ -77,6 +97,7 @@ class MultipleChoiceOverviewTable extends Component {
props: {style: tableLayoutStyles.headerCell},
},
cell: {
format: answerColumnsFormatter,
props: {style: tableLayoutStyles.cell},
}
}
Expand Down
2 changes: 1 addition & 1 deletion apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.story.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const multipleChoiceData = [
{percentAnswered: 10, isCorrectAnswer: false},
{percentAnswered: 9, isCorrectAnswer: false},
{percentAnswered: 5, isCorrectAnswer: false},
{percentAnswered: 45, isCorrectAnswer: true},
{percentAnswered: 32, isCorrectAnswer: true},
{percentAnswered: 5, isCorrectAnswer: false},
],
},
Expand Down