Skip to content

Commit 54de808

Browse files
author
adi-herwana-nus
committed
feat(question-generation): improved error handling
- properly forward package creation errors to user
1 parent a0499c8 commit 54de808

File tree

15 files changed

+215
-90
lines changed

15 files changed

+215
-90
lines changed

‎app/controllers/course/assessment/question/programming_controller.rb

+6
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,12 @@ def update
6060
end
6161
end
6262

63+
def import_result
64+
head :not_found and return if @programming_question&.import_job.nil?
65+
66+
render partial: 'import_result', locals: { import_job: @programming_question.import_job }
67+
end
68+
6369
def codaveri_languages
6470
languages = Coursemology::Polyglot::Language.
6571
where(enabled: true, question_generation_whitelisted: true).

‎app/helpers/course/assessment/question/programming_helper.rb

+16-40
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,23 @@
11
# frozen_string_literal: true
22
module Course::Assessment::Question::ProgrammingHelper
3-
# Displays the result alert for an import job.
3+
# Displays a specific error type for an import job, for frontend to map to an appropriate error message.
44
#
5-
# @return [String] If there is an import job for the question.
6-
# @return [nil] If there is no import job for the question.
7-
def import_result_message
8-
import_job = @programming_question.import_job
9-
return nil unless import_job
5+
# @return [String] If the import job for the question exists and raised an error.
6+
# @return [nil] If the import job for the question succeded, or does not exist.
7+
def import_result_error
8+
return nil unless import_errored?
109

11-
if import_job.completed?
12-
successful_import_message
13-
elsif import_job.errored?
14-
errored_import_message
10+
case @programming_question.import_job.error['class']
11+
when InvalidDataError.name
12+
:invalid_package
13+
when Timeout::Error.name
14+
:evaluation_timeout
15+
when Course::Assessment::ProgrammingEvaluationService::TimeLimitExceededError.name
16+
:time_limit_exceeded
17+
when Course::Assessment::ProgrammingEvaluationService::Error.name
18+
:evaluation_error
19+
else
20+
:generic_error
1521
end
1622
end
1723

@@ -50,34 +56,4 @@ def can_edit_online?
5056

5157
@meta.present?
5258
end
53-
54-
private
55-
56-
def successful_import_message
57-
t('course.assessment.question.programming.form.import_result.success')
58-
end
59-
60-
def errored_import_message
61-
t('course.assessment.question.programming.form.import_result.error',
62-
error: import_error_message(@programming_question.import_job.error))
63-
end
64-
65-
# Translates an error object serialised in the +TrackableJobs+ table to a user-readable message.
66-
#
67-
# @param [Hash] error The error object in the +TrackableJobs+ table.
68-
# @return [String]
69-
def import_error_message(error)
70-
case error['class']
71-
when InvalidDataError.name
72-
t('course.assessment.question.programming.form.import_result.errors.invalid_package')
73-
when Timeout::Error.name
74-
t('course.assessment.question.programming.form.import_result.errors.evaluation_timeout')
75-
when Course::Assessment::ProgrammingEvaluationService::TimeLimitExceededError.name
76-
t('course.assessment.question.programming.form.import_result.errors.time_limit_exceeded')
77-
when Course::Assessment::ProgrammingEvaluationService::Error.name
78-
t('course.assessment.question.programming.form.import_result.errors.evaluation_error')
79-
else
80-
error['message']
81-
end
82-
end
8359
end

‎app/models/course/assessment/assessment_ability.rb

+1
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,7 @@ def allow_manage_questions
190190
can :manage, question_class, question: question_assessments_current_course
191191
end
192192
can :duplicate, Course::Assessment::Question, question_assessments_current_course
193+
can :import_result, Course::Assessment::Question::Programming
193194
can :codaveri_languages, Course::Assessment::Question::Programming
194195
can :generate, Course::Assessment::Question::Programming
195196
end

‎app/views/course/assessment/question/programming/_import_result.json.jbuilder

+4-1
Original file line numberDiff line numberDiff line change
@@ -16,5 +16,8 @@ json.importResult do
1616
end
1717
end
1818

19-
json.importResultMessage import_result_message if import_job
19+
if import_errored?
20+
json.error import_result_error
21+
json.message import_job.error['message']
22+
end
2023
end

‎client/app/api/course/Assessment/Question/Programming.ts

+7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import {
22
LanguageData,
3+
PackageImportResultData,
34
ProgrammingFormData,
45
ProgrammingPostStatusData,
56
} from 'types/course/assessment/question/programming';
@@ -22,6 +23,12 @@ export default class ProgrammingAPI extends BaseAPI {
2223
return this.client.get(`${this.#urlPrefix}/${id}/edit`);
2324
}
2425

26+
fetchImportResult(
27+
id: number,
28+
): APIResponse<{ importResult: PackageImportResultData }> {
29+
return this.client.get(`${this.#urlPrefix}/${id}/import_result`);
30+
}
31+
2532
create(data: FormData): APIResponse<ProgrammingPostStatusData> {
2633
return this.client.post(`${this.#urlPrefix}`, data);
2734
}

‎client/app/bundles/course/assessment/pages/AssessmentGenerate/GenerateExportDialog.tsx

+45-18
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,17 @@ import {
2020
Typography,
2121
} from '@mui/material';
2222
import { red } from '@mui/material/colors';
23-
import { LanguageData } from 'types/course/assessment/question/programming';
23+
import {
24+
LanguageData,
25+
PackageImportResultError,
26+
} from 'types/course/assessment/question/programming';
2427

2528
import GlobalAPI from 'api';
2629
import buildFormData from 'course/assessment/question/programming/commons/builder';
30+
import { ImportResultErrorMapper } from 'course/assessment/question/programming/components/common/ImportResult';
2731
import {
2832
create,
33+
fetchImportResult,
2934
update,
3035
} from 'course/assessment/question/programming/operations';
3136
import { generationActions as actions } from 'course/assessment/reducers/generation';
@@ -60,7 +65,7 @@ const translations = defineMessages({
6065
},
6166
exportError: {
6267
id: 'course.assessment.generation.exportError',
63-
defaultMessage: 'An error occured in exporting this question.',
68+
defaultMessage: 'An error occured in exporting this question: {error}',
6469
},
6570
});
6671

@@ -84,6 +89,26 @@ const GenerateExportDialog: FC<Props> = (props) => {
8489
);
8590
};
8691

92+
const handleExportError = async (
93+
conversation: ConversationState,
94+
exportErrorMessage?: string,
95+
): Promise<void> => {
96+
let exportError = PackageImportResultError.GENERIC_ERROR;
97+
if (conversation.questionId) {
98+
const importResult = await fetchImportResult(conversation.questionId);
99+
exportError = importResult.error ?? exportError;
100+
// exportErrorMessage in arguments will take precedence, in case a new error happens somewhere other than the import job.
101+
exportErrorMessage = exportErrorMessage ?? importResult.message;
102+
}
103+
dispatch(
104+
actions.exportConversationError({
105+
conversationId: conversation.id,
106+
exportError,
107+
exportErrorMessage,
108+
}),
109+
);
110+
};
111+
87112
const pollQuestionExportJobs = (): void => {
88113
Object.values(generatePageData.conversations)
89114
.filter(
@@ -102,19 +127,11 @@ const GenerateExportDialog: FC<Props> = (props) => {
102127
}),
103128
);
104129
} else if (response.data.status === 'errored') {
105-
dispatch(
106-
actions.exportConversationError({
107-
conversationId: conversation.id,
108-
}),
109-
);
130+
handleExportError(conversation);
110131
}
111132
})
112133
.catch((error) => {
113-
dispatch(
114-
actions.exportConversationError({
115-
conversationId: conversation.id,
116-
}),
117-
);
134+
handleExportError(conversation, error.message);
118135
});
119136
});
120137
};
@@ -128,6 +145,20 @@ const GenerateExportDialog: FC<Props> = (props) => {
128145
};
129146
});
130147

148+
const exportErrorMessage = (conversation: ConversationState): string => {
149+
if (
150+
!conversation.exportError ||
151+
conversation.exportError === PackageImportResultError.GENERIC_ERROR
152+
) {
153+
return t(translations.exportError, {
154+
error: conversation.exportErrorMessage ?? '',
155+
});
156+
}
157+
// We reuse the same error messages as the main programming question page,
158+
// though the user should never see INVALID_PACKAGE error because it's entirely managed by us.
159+
return t(ImportResultErrorMapper[conversation.exportError]);
160+
};
161+
131162
return (
132163
<Dialog
133164
className="top-10"
@@ -203,7 +234,7 @@ const GenerateExportDialog: FC<Props> = (props) => {
203234
/>
204235
{conversation.exportStatus === 'error' && (
205236
<Typography color={red[700]} variant="caption">
206-
{t(translations.exportError)}
237+
{exportErrorMessage(conversation)}
207238
</Typography>
208239
)}
209240
</section>
@@ -286,11 +317,7 @@ const GenerateExportDialog: FC<Props> = (props) => {
286317
}
287318
})
288319
.catch((error) => {
289-
dispatch(
290-
actions.exportConversationError({
291-
conversationId: conversation.id,
292-
}),
293-
);
320+
handleExportError(conversation, error.message);
294321
});
295322
});
296323
}}

‎client/app/bundles/course/assessment/pages/AssessmentGenerate/types.ts

+3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {
22
LanguageData,
33
MetadataTestCase,
4+
PackageImportResultError,
45
} from 'types/course/assessment/question/programming';
56

67
const CODAVERI_DIFFICULTIES = ['easy', 'medium', 'hard'] as const;
@@ -64,6 +65,8 @@ export interface ConversationState {
6465
duplicateFromId?: string;
6566
toExport: boolean;
6667
exportStatus: ExportStatus;
68+
exportError?: PackageImportResultError;
69+
exportErrorMessage?: string;
6770
redirectEditUrl?: string;
6871
importJobUrl?: string;
6972
questionId?: number;

‎client/app/bundles/course/assessment/pages/AssessmentShow/AssessmentShowPage.tsx

+16-13
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
ListItemIcon,
1010
ListItemText,
1111
Paper,
12+
Tooltip,
1213
Typography,
1314
} from '@mui/material';
1415
import { AssessmentData } from 'types/course/assessment/assessments';
@@ -182,20 +183,22 @@ const AssessmentShowPage = (props: AssessmentShowPageProps): JSX.Element => {
182183
<NewQuestionMenu with={assessment.newQuestionUrls} />
183184
)}
184185
{assessment.generateQuestionUrl && (
185-
<Link
186-
opensInNewTab
187-
to={assessment.generateQuestionUrl}
188-
underline="none"
189-
>
190-
<Button
191-
size="small"
192-
startIcon={<AutoFixHigh />}
193-
sx={{ marginTop: '0 !important', marginLeft: 1 }}
194-
variant="outlined"
186+
<Tooltip title={t(translations.generateTooltip)}>
187+
<Link
188+
opensInNewTab
189+
to={assessment.generateQuestionUrl}
190+
underline="none"
195191
>
196-
{t(translations.generate)}
197-
</Button>
198-
</Link>
192+
<Button
193+
size="small"
194+
startIcon={<AutoFixHigh />}
195+
sx={{ marginTop: '0 !important', marginLeft: 1 }}
196+
variant="outlined"
197+
>
198+
{t(translations.generate)}
199+
</Button>
200+
</Link>
201+
</Tooltip>
199202
)}
200203

201204
{assessment.hasUnautogradableQuestions && (

‎client/app/bundles/course/assessment/question/programming/components/common/ImportResult.tsx

+34-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import { Alert, Typography } from '@mui/material';
2-
import { PackageImportResultData } from 'types/course/assessment/question/programming';
2+
import {
3+
PackageImportResultData,
4+
PackageImportResultError,
5+
} from 'types/course/assessment/question/programming';
36

47
import Link from 'lib/components/core/Link';
58
import useTranslation from 'lib/hooks/useTranslation';
@@ -12,11 +15,40 @@ interface ImportResultProps {
1215
disabled?: boolean;
1316
}
1417

18+
export const ImportResultErrorMapper = {
19+
[PackageImportResultError.INVALID_PACKAGE]:
20+
translations.packageImportInvalidPackage,
21+
[PackageImportResultError.EVALUATION_TIMEOUT]:
22+
translations.packageImportEvaluationTimeout,
23+
[PackageImportResultError.EVALUATION_TIME_LIMIT_EXCEEDED]:
24+
translations.packageImportTimeLimitExceeded,
25+
[PackageImportResultError.EVALUATION_ERROR]:
26+
translations.packageImportEvaluationError,
27+
};
28+
1529
const ImportResult = (props: ImportResultProps): JSX.Element => {
1630
const { of: result, disabled } = props;
1731

1832
const { t } = useTranslation();
1933

34+
const importResultMessage = (): string => {
35+
if (!result.status) {
36+
return t(translations.packagePending);
37+
}
38+
if (result.status === 'success') {
39+
return t(translations.packageImportSuccess);
40+
}
41+
if (
42+
!result.error ||
43+
result.error === PackageImportResultError.GENERIC_ERROR
44+
) {
45+
return t(translations.packageImportGenericError, {
46+
error: result.message ?? '',
47+
});
48+
}
49+
return t(ImportResultErrorMapper[result.error]);
50+
};
51+
2052
return (
2153
<Alert
2254
classes={
@@ -26,9 +58,7 @@ const ImportResult = (props: ImportResultProps): JSX.Element => {
2658
}
2759
severity={result.status ?? 'info'}
2860
>
29-
<Typography variant="body2">
30-
{result.importResultMessage ?? t(translations.packagePending)}
31-
</Typography>
61+
<Typography variant="body2">{importResultMessage()}</Typography>
3262

3363
{result.buildLog && (
3464
<Link href={`#${BUILD_LOG_ID}`}>{t(translations.seeBuildLog)}</Link>

‎client/app/bundles/course/assessment/question/programming/operations.ts

+8
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
LanguageData,
77
MetadataTestCase,
88
MetadataTestCases,
9+
PackageImportResultData,
910
ProgrammingFormData,
1011
ProgrammingPostStatusData,
1112
} from 'types/course/assessment/question/programming';
@@ -33,6 +34,13 @@ export const fetchEdit = async (id: number): Promise<ProgrammingFormData> => {
3334
return response.data;
3435
};
3536

37+
export const fetchImportResult = async (
38+
id: number,
39+
): Promise<PackageImportResultData> => {
40+
const response = await ProgrammingAPI.fetchImportResult(id);
41+
return response.data.importResult;
42+
};
43+
3644
export const create = async (
3745
data: FormData,
3846
): Promise<ProgrammingPostStatusData> => {

0 commit comments

Comments
 (0)