Skip to content

perf(AssessmentStatistics): add lazyloading for Statistics page data #7976

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions app/controllers/course/statistics/assessments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,19 @@
include Course::Statistics::SubmissionsConcern
include Course::Statistics::UsersConcern

def main_statistics
def assessment_statistics
@assessment = Course::Assessment.unscoped.

Check warning on line 8 in app/controllers/course/statistics/assessments_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/statistics/assessments_controller.rb#L8

Added line #L8 was not covered by tests
includes(programming_questions: [:language]).
calculated(:maximum_grade, :question_count).
find(assessment_params[:id])

load_ordered_questions
create_question_related_hash

Check warning on line 14 in app/controllers/course/statistics/assessments_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/statistics/assessments_controller.rb#L13-L14

Added lines #L13 - L14 were not covered by tests

@assessment_autograded = @question_hash.any? { |_, (_, _, auto_gradable)| auto_gradable }

Check warning on line 16 in app/controllers/course/statistics/assessments_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/statistics/assessments_controller.rb#L16

Added line #L16 was not covered by tests
end

def submission_statistics
@assessment = Course::Assessment.unscoped.
includes(programming_questions: [:language]).
calculated(:maximum_grade, :question_count).
Expand All @@ -16,10 +28,8 @@

load_course_user_students_info
load_ordered_questions
fetch_all_ancestor_assessments
create_question_related_hash

@assessment_autograded = @question_hash.any? { |_, (_, _, auto_gradable)| auto_gradable }
@student_submissions_hash = fetch_hash_for_main_assessment(submissions, @all_students)
end

Expand Down Expand Up @@ -47,10 +57,8 @@
where(assessment_id: assessment_params[:id])

create_submission_question_id_hash(@assessment.questions)

load_course_user_students_info
load_ordered_questions

create_student_live_feedback_hash
end

Expand All @@ -68,6 +76,10 @@
order(:created_at)
end

def ancestor_info
fetch_all_ancestor_assessments

Check warning on line 80 in app/controllers/course/statistics/assessments_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/statistics/assessments_controller.rb#L80

Added line #L80 was not covered by tests
end

private

def load_ordered_questions
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true
json.array! @ancestors do |ancestor|
json.id ancestor.id
json.title ancestor.title
json.courseTitle ancestor.course&.title
end
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ end
json.submissions @student_submissions_hash.each do |course_user, (submission, end_at)|
json.partial! 'course_user', course_user: course_user
json.partial! 'submission', submission: submission, end_at: end_at
json.maximumGrade @assessment.maximum_grade
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true
json.partial! 'assessment', assessment: @assessment, course: current_course
json.isAutograded @assessment_autograded
json.questionCount @assessment.question_count
json.questionIds @ordered_questions
json.liveFeedbackEnabled @assessment.programming_questions.any?(&:live_feedback_enabled)
Original file line number Diff line number Diff line change
@@ -1,16 +1,9 @@
# frozen_string_literal: true
json.assessment do
json.partial! 'assessment', assessment: @assessment, course: current_course
json.isAutograded @assessment_autograded
json.questionCount @assessment.question_count
json.questionIds @ordered_questions
json.liveFeedbackEnabled @assessment.programming_questions.any?(&:live_feedback_enabled)
end

json.submissions @student_submissions_hash.each do |course_user, (submission, answers, end_at)|
json.array! @student_submissions_hash.each do |course_user, (submission, answers, end_at)|
json.partial! 'course_user', course_user: course_user
json.partial! 'submission', submission: submission, end_at: end_at

json.maximumGrade @assessment.maximum_grade
json.groups @group_names_hash[course_user.id] do |name|
json.name name
end
Expand All @@ -21,9 +14,3 @@ json.submissions @student_submissions_hash.each do |course_user, (submission, an
end
json.partial! 'attempt_status', answers: answers unless submission.nil?
end

json.ancestors @ancestors do |ancestor|
json.id ancestor.id
json.title ancestor.title
json.courseTitle ancestor.course&.title
end
26 changes: 21 additions & 5 deletions client/app/api/course/Statistics/AssessmentStatistics.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { LiveFeedbackHistoryState } from 'types/course/assessment/submission/liveFeedback';
import {
AncestorAssessmentStats,
AncestorInfo,
AssessmentLiveFeedbackStatistics,
MainAssessmentStats,
MainAssessmentInfo,
MainSubmissionInfo,
} from 'types/course/statistics/assessmentStatistics';

import { APIResponse } from 'api/types';
Expand All @@ -24,15 +26,23 @@ export default class AssessmentStatisticsAPI extends BaseCourseAPI {
ancestorId: string | number,
): APIResponse<AncestorAssessmentStats> {
return this.client.get(
`${this.#urlPrefix}/${ancestorId}/ancestor_statistics/`,
`${this.#urlPrefix}/${ancestorId}/ancestor_statistics`,
);
}

fetchMainStatistics(
fetchAssessmentStatistics(
assessmentId: string | number,
): APIResponse<MainAssessmentStats> {
): APIResponse<MainAssessmentInfo | null> {
return this.client.get(
`${this.#urlPrefix}/${assessmentId}/main_statistics`,
`${this.#urlPrefix}/${assessmentId}/assessment_statistics`,
);
}

fetchSubmissionStatistics(
assessmentId: string | number,
): APIResponse<MainSubmissionInfo[]> {
return this.client.get(
`${this.#urlPrefix}/${assessmentId}/submission_statistics`,
);
}

Expand All @@ -54,4 +64,10 @@ export default class AssessmentStatisticsAPI extends BaseCourseAPI {
{ params: { question_id: questionId, course_user_id: courseUserId } },
);
}

fetchAncestorInfo(
assessmentId: number,
): Promise<APIResponse<AncestorInfo[]>> {
return this.client.get(`${this.#urlPrefix}/${assessmentId}/ancestor_info`);
}
}
64 changes: 44 additions & 20 deletions client/app/bundles/course/assessment/operations/statistics.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import { AxiosError } from 'axios';
import { dispatch } from 'store';
import {
AncestorAssessmentStats,
AssessmentLiveFeedbackStatistics,
} from 'types/course/statistics/assessmentStatistics';
import { AncestorAssessmentStats } from 'types/course/statistics/assessmentStatistics';

import CourseAPI from 'api/course';

Expand All @@ -13,17 +10,39 @@ export const fetchAssessmentStatistics = async (
assessmentId: number,
): Promise<void> => {
try {
dispatch(actions.reset());
const response =
await CourseAPI.statistics.assessment.fetchMainStatistics(assessmentId);
const data = response.data;
dispatch(
actions.initialize({
assessment: data.assessment,
submissions: data.submissions,
ancestors: data.ancestors,
}),
);
await CourseAPI.statistics.assessment.fetchAssessmentStatistics(
assessmentId,
);
dispatch(actions.setAssessmentStatistics(response.data));
} catch (error) {
if (error instanceof AxiosError) throw error.response?.data?.errors;
throw error;
}
};

export const fetchSubmissionStatistics = async (
assessmentId: number,
): Promise<void> => {
try {
const response =
await CourseAPI.statistics.assessment.fetchSubmissionStatistics(
assessmentId,
);
dispatch(actions.setSubmissionStatistics(response.data));
} catch (error) {
if (error instanceof AxiosError) throw error.response?.data?.errors;
throw error;
}
};

export const fetchAncestorInfo = async (
assessmentId: number,
): Promise<void> => {
try {
const response =
await CourseAPI.statistics.assessment.fetchAncestorInfo(assessmentId);
dispatch(actions.setAncestorInfo(response.data));
} catch (error) {
if (error instanceof AxiosError) throw error.response?.data?.errors;
throw error;
Expand All @@ -41,10 +60,15 @@ export const fetchAncestorStatistics = async (

export const fetchLiveFeedbackStatistics = async (
assessmentId: number,
): Promise<AssessmentLiveFeedbackStatistics[]> => {
const response =
await CourseAPI.statistics.assessment.fetchLiveFeedbackStatistics(
assessmentId,
);
return response.data;
): Promise<void> => {
try {
const response =
await CourseAPI.statistics.assessment.fetchLiveFeedbackStatistics(
assessmentId,
);
dispatch(actions.setLiveFeedbackStatistics(response.data));
} catch (error) {
if (error instanceof AxiosError) throw error.response?.data?.errors;
throw error;
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,16 @@ const AssessmentStatisticsPage: FC = () => {
const [tabValue, setTabValue] = useState('gradesPerQuestion');
const [includePhantom, setIncludePhantom] = useState(false);

const statistics = useAppSelector(getAssessmentStatistics);
const assessmentStatistics = useAppSelector(getAssessmentStatistics);

const tabComponentMapping = tabMapping(includePhantom);

return (
<Page
backTo={statistics.assessment?.url}
backTo={assessmentStatistics?.url}
className="space-y-5"
title={t(translations.header, {
title: statistics.assessment?.title ?? '',
title: assessmentStatistics?.title ?? '',
})}
>
<>
Expand Down Expand Up @@ -160,7 +160,7 @@ const AssessmentStatisticsPage: FC = () => {
label={t(translations.duplicationHistory)}
value="duplicationHistory"
/>
{statistics.assessment?.liveFeedbackEnabled && (
{assessmentStatistics?.liveFeedbackEnabled && (
<Tab
className="min-h-12"
id="liveFeedback"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,36 +1,60 @@
import { FC, useState } from 'react';
import { Dispatch, FC, SetStateAction, useState } from 'react';
import { useParams } from 'react-router-dom';
import { AncestorInfo } from 'types/course/statistics/assessmentStatistics';

import LoadingIndicator from 'lib/components/core/LoadingIndicator';
import Preload from 'lib/components/wrappers/Preload';
import { useAppSelector } from 'lib/hooks/store';

import { fetchAncestorInfo } from '../../operations/statistics';

import AncestorOptions from './AncestorOptions';
import AncestorStatistics from './AncestorStatistics';
import { getAssessmentStatistics } from './selectors';
import { getAncestorInfo } from './selectors';

const DuplicationHistoryStatistics: FC = () => {
const ancestors = useAppSelector(getAssessmentStatistics).ancestors;
const renderDuplicationHistoryStatistics = (
ancestorInfo: AncestorInfo[],
parsedAssessmentId: number,
selectedAncestorId: number,
setSelectedAncestorId: Dispatch<SetStateAction<number>>,
): JSX.Element => (
<>
<AncestorOptions
ancestors={ancestorInfo}
parsedAssessmentId={parsedAssessmentId}
selectedAncestorId={selectedAncestorId}
setSelectedAncestorId={setSelectedAncestorId}
/>
<div className="mb-8">
<AncestorStatistics
currentAssessmentSelected={selectedAncestorId === parsedAssessmentId}
selectedAssessmentId={selectedAncestorId}
/>
</div>
</>
);

const DuplicationHistoryStatistics: FC = () => {
const { assessmentId } = useParams();
const parsedAssessmentId = parseInt(assessmentId!, 10);

const ancestorInfo = useAppSelector(getAncestorInfo);
const [selectedAncestorId, setSelectedAncestorId] =
useState(parsedAssessmentId);

const fetchAndSetAncestorInfo = async (): Promise<void> => {
if (ancestorInfo.length > 0) return;
await fetchAncestorInfo(parsedAssessmentId);
};

return (
<>
<AncestorOptions
ancestors={ancestors}
parsedAssessmentId={parsedAssessmentId}
selectedAncestorId={selectedAncestorId}
setSelectedAncestorId={setSelectedAncestorId}
/>
<div className="mb-8">
<AncestorStatistics
currentAssessmentSelected={selectedAncestorId === parsedAssessmentId}
selectedAssessmentId={selectedAncestorId}
/>
</div>
</>
<Preload render={<LoadingIndicator />} while={fetchAndSetAncestorInfo}>
{renderDuplicationHistoryStatistics(
ancestorInfo,
parsedAssessmentId,
selectedAncestorId,
setSelectedAncestorId,
)}
</Preload>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ interface Props {
const AncestorGradesChart: FC<Props> = (props) => {
const { ancestorSubmissions } = props;

const totalGrades =
ancestorSubmissions
?.filter((s) => s.totalGrade)
?.map((s) => s.totalGrade) ?? [];
const gradedSubmissions =
ancestorSubmissions?.filter((s) => s.totalGrade) ?? [];
const totalGrades = gradedSubmissions.map((s) =>
parseFloat(s.totalGrade as unknown as string),
);
const maximumGrade = gradedSubmissions[0]?.maximumGrade ?? undefined;

return <GradesChart totalGrades={totalGrades} />;
return <GradesChart maximumGrade={maximumGrade} totalGrades={totalGrades} />;
};

export default AncestorGradesChart;
Loading