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

gracefully handle null assessment id #24525

Merged
merged 2 commits into from Aug 29, 2018
Merged

Conversation

davidsbailey
Copy link
Member

https://codeorg.zendesk.com/agent/tickets/206936 provides a reliable repro of assessment tab failing to load. the js error console shows the following:

screen shot 2018-08-28 at 2 57 05 pm

which points to the following minified code:

screen shot 2018-08-28 at 2 57 31 pm

I don't have an exact repro, but I'm pretty sure this PR will fix the immediate problem. We do a similar thing here, which fixed a similar problem that i could repro, which worked in that situation:

export const isCurrentAssessmentSurvey = (state) => {
const scriptId = state.scriptSelection.scriptId;
const surveysStructure = state.sectionAssessments.surveysByScript[scriptId] || {};
const currentAssessmentId = state.sectionAssessments.assessmentId;
return Object.keys(surveysStructure).includes(currentAssessmentId + '');

So I'm proposing fixing the immediate problem, and then testing it out on the zendesk ticket requester's account to see if the issue is resolved.

@@ -637,7 +637,7 @@ export const countSubmissionsForCurrentAssessment = (state) => {
const studentResponses = getAssessmentResponsesForCurrentScript(state);
let totalSubmissions = 0;
Object.values(studentResponses).forEach((student) => {
if (Object.keys(student.responses_by_assessment).includes(currentAssessmentId.toString())) {
if (Object.keys(student.responses_by_assessment).includes(currentAssessmentId + '')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be the safest change, but it seems reasonable to step back a layer and make countSubmissionsForCurrentAssessment early-out and return 0 if currentAssessmentId is undefined. It'd be an easy case to add a unit test for, too.

@davidsbailey
Copy link
Member Author

Great idea @islemaster . Please take another look

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

👍 Looks great!

@davidsbailey davidsbailey merged commit 50db5b8 into staging Aug 29, 2018
@davidsbailey davidsbailey deleted the unassigned-assessments-fail branch August 29, 2018 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants