Skip to content

Commit bee704d

Browse files
drgrice1drgrice1
committed
Rework PG error/warning/debug message handling.
The PG messages are now dealt with completely separately from the webwork2 general warning handling. Furthermore PG warnings and debug messages are separately dealt with. The PG `internal_debug_messages` are no longer dealt with at all. They are unnecessary and inappropriately named, and will be removed from PG. Also, handling of the warnings is now much more consistent in all of the different places that problems are rendered. In fact, they all use the new `templates/ContentGenerator/Base/problem_warning_and_debug_output.html.ep` template for this. All of this output is no longer wrapped in a `code` tag. That is the wrong thing for this. None of it is code. It also causes problems with openwebwork/pg#1384.
1 parent cc358e3 commit bee704d

File tree

15 files changed

+140
-207
lines changed

15 files changed

+140
-207
lines changed

‎htdocs/js/System/system.scss‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -374,13 +374,6 @@ h1.page-title {
374374
line-height: 1.4;
375375
}
376376

377-
.Warnings {
378-
code {
379-
white-space: normal;
380-
color: inherit;
381-
}
382-
}
383-
384377
.error-output {
385378
word-wrap: break-word;
386379
color: #d63384;

‎lib/WeBWorK/ContentGenerator.pm‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -813,7 +813,7 @@ there are pg errors.
813813
=cut
814814

815815
sub have_warnings ($c) {
816-
return $c->stash('warnings') || $c->{pgerrors};
816+
return $c->stash('warnings');
817817
}
818818

819819
=item exists_theme_file
@@ -1220,8 +1220,8 @@ Used to display a generic warning message at the top of the page
12201220
=cut
12211221

12221222
sub warningMessage ($c) {
1223-
return $c->maketext('<strong>Warning</strong>: There may be something wrong with this question. '
1224-
. 'Please inform your instructor including the warning messages below.');
1223+
return $c->maketext('<strong>Warning</strong>: WeBWorK has encountered warnings while processing your request. '
1224+
. 'See the warning messages below for details.');
12251225
}
12261226

12271227
=item $string = formatDateTime($date_time, $format_string, $timezone, $locale)

‎lib/WeBWorK/ContentGenerator/GatewayQuiz.pm‎

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -274,14 +274,15 @@ sub get_instructor_comment ($c, $problem) {
274274

275275
async sub pre_header_initialize ($c) {
276276
# Make sure these are defined for the templates.
277-
$c->stash->{problems} = [];
278-
$c->stash->{pg_results} = [];
279-
$c->stash->{startProb} = 0;
280-
$c->stash->{endProb} = 0;
281-
$c->stash->{numPages} = 0;
282-
$c->stash->{pageNumber} = 0;
283-
$c->stash->{problem_numbers} = [];
284-
$c->stash->{probOrder} = [];
277+
$c->stash->{problems} = [];
278+
$c->stash->{pg_results} = [];
279+
$c->stash->{startProb} = 0;
280+
$c->stash->{endProb} = 0;
281+
$c->stash->{numPages} = 0;
282+
$c->stash->{pageNumber} = 0;
283+
$c->stash->{problem_numbers} = [];
284+
$c->stash->{probOrder} = [];
285+
$c->stash->{haveProblemWarnings} = 0;
285286

286287
# If authz->checkSet has failed, then this set is invalid. No need to proceeded.
287288
return if $c->{invalidSet};
@@ -1447,11 +1448,6 @@ sub nav ($c, $args) {
14471448
return '';
14481449
}
14491450

1450-
sub warningMessage ($c) {
1451-
return $c->maketext('<strong>Warning</strong>: There may be something wrong with a question in this test. '
1452-
. 'Please inform your instructor including the warning messages below.');
1453-
}
1454-
14551451
# Evaluation utility
14561452
# $effectiveUser is the current effective user, $set is the merged set version, $formFields is a reference to the
14571453
# hash of parameters from the input form that need to be passed to the translator, and $mergedProblem
@@ -1510,16 +1506,14 @@ async sub getProblemHTML ($c, $effectiveUser, $set, $formFields, $mergedProblem)
15101506
},
15111507
);
15121508

1513-
# Warnings in the renderPG subprocess will not be caught by the global warning handler of this process.
1514-
# So rewarn them and let the global warning handler take care of it.
1515-
warn $pg->{warnings} if $pg->{warnings};
1516-
15171509
# If the user can check answers and either this is not an answer submission or the problem_data form
15181510
# parameter was previously set, then set or update the problem_data form parameter.
15191511
$c->param('problem_data_' . $mergedProblem->problem_id => encode_json($pg->{PERSISTENCE_HASH} || '{}'))
15201512
if $c->{can}{checkAnswers}
15211513
&& (!$c->{submitAnswers} || defined $c->param('problem_data_' . $mergedProblem->problem_id));
15221514

1515+
$c->stash->{haveProblemWarnings} = 1 if $pg->{warnings} || @{ $pg->{pgwarning} // [] };
1516+
15231517
return $pg;
15241518
}
15251519

‎lib/WeBWorK/ContentGenerator/Problem.pm‎

Lines changed: 3 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -594,10 +594,6 @@ async sub pre_header_initialize ($c) {
594594
}
595595
);
596596

597-
# Warnings in the renderPG subprocess will not be caught by the global warning handler of this process.
598-
# So rewarn them and let the global warning handler take care of it.
599-
warn $pg->{warnings} if $pg->{warnings};
600-
601597
debug('end pg processing');
602598

603599
$pg->{body_text} .= $c->hidden_field(
@@ -609,20 +605,6 @@ async sub pre_header_initialize ($c) {
609605
$can{showHints} &&= $pg->{flags}{hintExists};
610606
$can{showSolutions} &&= $pg->{flags}{solutionExists};
611607

612-
# Record errors
613-
$c->{pgdebug} = $pg->{debug_messages} if ref $pg->{debug_messages} eq 'ARRAY';
614-
$c->{pgwarning} = $pg->{warning_messages} if ref $pg->{warning_messages} eq 'ARRAY';
615-
$c->{pginternalerrors} = $pg->{internal_debug_messages} if ref $pg->{internal_debug_messages} eq 'ARRAY';
616-
# $c->{pgerrors} is defined if any of the above are defined, and is nonzero if any are non-empty.
617-
$c->{pgerrors} = @{ $c->{pgdebug} // [] } || @{ $c->{pgwarning} // [] } || @{ $c->{pginternalerrors} // [] }
618-
if defined $c->{pgdebug} || defined $c->{pgwarning} || defined $c->{pginternalerrors};
619-
620-
# If $c->{pgerrors} is not defined, then the PG messages arrays were not defined,
621-
# which means $pg->{pgcore} was not defined and the translator died.
622-
warn 'Processing of this PG problem was not completed. Probably because of a syntax error. '
623-
. 'The translator died prematurely and no PG warning messages were transmitted.'
624-
unless defined $c->{pgerrors};
625-
626608
# Store fields
627609
$c->{want} = \%want;
628610
$c->{can} = \%can;
@@ -635,53 +617,6 @@ async sub pre_header_initialize ($c) {
635617
return;
636618
}
637619

638-
sub warnings ($c) {
639-
my $output = $c->c;
640-
641-
# Display warning messages
642-
if (!defined $c->{pgerrors}) {
643-
push(
644-
@$output,
645-
$c->tag(
646-
'div',
647-
$c->c(
648-
$c->tag('h3', style => 'color:red;', $c->maketext('PG question failed to render')),
649-
$c->tag('p', $c->maketext('Unable to obtain error messages from within the PG question.'))
650-
)->join('')
651-
)
652-
);
653-
} elsif ($c->{pgerrors} > 0) {
654-
my @pgdebug = @{ $c->{pgdebug} // [] };
655-
my @pgwarning = @{ $c->{pgwarning} // [] };
656-
my @pginternalerrors = @{ $c->{pginternalerrors} // [] };
657-
push(
658-
@$output,
659-
$c->tag(
660-
'div',
661-
$c->c(
662-
$c->tag('h2', $c->maketext('PG question processing error messages')),
663-
@pgdebug ? $c->c(
664-
$c->tag('h3', $c->maketext('PG debug messages')),
665-
$c->tag('p', $c->c(@pgdebug)->join($c->tag('br')))
666-
)->join('') : '',
667-
@pgwarning ? $c->c(
668-
$c->tag('h3', $c->maketext('PG warning messages')),
669-
$c->tag('p', $c->c(@pgwarning)->join($c->tag('br')))
670-
)->join('') : '',
671-
@pginternalerrors ? $c->c(
672-
$c->tag('h3', $c->maketext('PG internal errors')),
673-
$c->tag('p', $c->c(@pginternalerrors)->join($c->tag('br')))
674-
)->join('') : ''
675-
)->join('')
676-
)
677-
);
678-
}
679-
680-
push(@$output, $c->SUPER::warnings());
681-
682-
return $output->join('');
683-
}
684-
685620
sub head ($c) {
686621
return '' if ($c->{invalidSet});
687622
return $c->{pg}{head_text} if $c->{pg}{head_text};
@@ -1042,14 +977,7 @@ sub output_problem_body ($c) {
1042977
} else {
1043978
# For students render the body text of the problem with a message about error details.
1044979
return $c->c(
1045-
$c->tag(
1046-
'div',
1047-
id => 'output_problem_body',
1048-
class => 'text-dark',
1049-
style => 'color-scheme: light',
1050-
data => { bs_theme => 'light' },
1051-
$c->b($c->{pg}{body_text})
1052-
),
980+
$c->tag('div', $c->b($c->{pg}{body_text})),
1053981
$c->include(
1054982
'ContentGenerator/Base/error_output',
1055983
error => $c->{pg}{errors},
@@ -1534,7 +1462,8 @@ sub output_past_answer_button ($c) {
15341462
$c->hidden_field(selected_sets => $c->{problem}->set_id),
15351463
$c->hidden_field(selected_users => $c->{problem}->user_id),
15361464
$c->tag(
1537-
'p',
1465+
'div',
1466+
class => 'mb-3',
15381467
$c->submit_button(
15391468
$c->maketext('Show Past Answers'),
15401469
name => 'action',

‎lib/WeBWorK/ContentGenerator/ShowMeAnother.pm‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -212,30 +212,12 @@ async sub pre_header_initialize ($c) {
212212
}
213213
);
214214

215-
# Warnings in the renderPG subprocess will not be caught by the global warning handler of this process.
216-
# So rewarn them and let the global warning handler take care of it.
217-
warn $pg->{warnings} if $pg->{warnings};
218-
219215
debug('end pg processing');
220216

221217
# Update and fix hint/solution options after PG processing
222218
$c->{can}{showHints} &&= $pg->{flags}{hintExists};
223219
$c->{can}{showSolutions} &&= $pg->{flags}{solutionExists};
224220

225-
# Record errors
226-
$c->{pgdebug} = $pg->{debug_messages} if ref $pg->{debug_messages} eq 'ARRAY';
227-
$c->{pgwarning} = $pg->{warning_messages} if ref $pg->{warning_messages} eq 'ARRAY';
228-
$c->{pginternalerrors} = $pg->{internal_debug_messages} if ref $pg->{internal_debug_messages} eq 'ARRAY';
229-
# $c->{pgerrors} is defined if any of the above are defined, and is nonzero if any are non-empty.
230-
$c->{pgerrors} = @{ $c->{pgdebug} // [] } || @{ $c->{pgwarning} // [] } || @{ $c->{pginternalerrors} // [] }
231-
if defined $c->{pgdebug} || defined $c->{pgwarning} || defined $c->{pginternalerrors};
232-
233-
# If $c->{pgerrors} is not defined, then the PG messages arrays were not defined,
234-
# which means $pg->{pgcore} was not defined and the translator died.
235-
warn 'Processing of this PG problem was not completed. Probably because of a syntax error. '
236-
. 'The translator died prematurely and no PG warning messages were transmitted.'
237-
unless defined $c->{pgerrors};
238-
239221
$c->{pg} = $pg;
240222

241223
return;

‎lib/WeBWorK/Utils/Rendering.pm‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -252,10 +252,9 @@ sub renderPG ($c, $effectiveUser, $set, $problem, $psvn, $formFields, $translati
252252
};
253253

254254
if (ref($pg->{pgcore}) eq 'PGcore') {
255-
$ret->{internal_debug_messages} = $pg->{pgcore}->get_internal_debug_messages;
256-
$ret->{warning_messages} = $pg->{pgcore}->get_warning_messages();
257-
$ret->{debug_messages} = $pg->{pgcore}->get_debug_messages();
258-
$ret->{PG_ANSWERS_HASH} = {
255+
$ret->{warning_messages} = $pg->{pgcore}->get_warning_messages();
256+
$ret->{debug_messages} = $pg->{pgcore}->get_debug_messages();
257+
$ret->{PG_ANSWERS_HASH} = {
259258
map {
260259
$_ => {
261260
response_obj => unbless($pg->{pgcore}{PG_ANSWERS_HASH}{$_}->response_obj),
@@ -269,6 +268,8 @@ sub renderPG ($c, $effectiveUser, $set, $problem, $psvn, $formFields, $translati
269268
keys %{ $pg->{pgcore}{PG_alias}{resource_list} }
270269
};
271270
$ret->{PERSISTENCE_HASH} = $pg->{pgcore}{PERSISTENCE_HASH};
271+
} else {
272+
$ret->{render_fail} = 1;
272273
}
273274

274275
# Save the problem source. This is used by Caliper::Entity. Why?
@@ -277,7 +278,7 @@ sub renderPG ($c, $effectiveUser, $set, $problem, $psvn, $formFields, $translati
277278
$pg->free;
278279
return $ret;
279280
})->catch(sub ($err) {
280-
return { body_text => '', answers => {}, flags => { error_flag => 1 }, errors => $err };
281+
return { body_text => '', answers => {}, render_fail => 1, flags => { error_flag => 1 }, errors => $err };
281282
});
282283
}
283284

‎lib/WebworkWebservice/RenderProblem.pm‎

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -258,26 +258,23 @@ async sub renderProblem {
258258

259259
# New version of output:
260260
return {
261-
text => $pg->{body_text},
262-
header_text => $pg->{head_text},
263-
post_header_text => $pg->{post_header_text},
264-
answers => $pg->{answers},
265-
errors => $pg->{errors},
266-
pg_warnings => $pg->{warnings},
267-
PG_ANSWERS_HASH => $pg->{PG_ANSWERS_HASH},
268-
PERSISTENCE_HASH => $pg->{PERSISTENCE_HASH},
269-
problem_result => $pg->{result},
270-
problem_state => $pg->{state},
271-
flags => $pg->{flags},
272-
psvn => $psvn,
273-
problem_seed => $problemSeed,
274-
resource_list => $pg->{resource_list},
275-
warning_messages => ref $pg->{warning_messages} eq 'ARRAY' ? $pg->{warning_messages} : [],
276-
debug_messages => ref $pg->{debug_messages} eq 'ARRAY' ? $pg->{debug_messages} : [],
277-
internal_debug_messages => ref $pg->{internal_debug_messages} eq 'ARRAY'
278-
? $pg->{internal_debug_messages}
279-
: [],
280-
compute_time => logTimingInfo($beginTime, Benchmark->new),
261+
text => $pg->{body_text},
262+
header_text => $pg->{head_text},
263+
post_header_text => $pg->{post_header_text},
264+
answers => $pg->{answers},
265+
errors => $pg->{errors},
266+
pg_warnings => $pg->{warnings},
267+
PG_ANSWERS_HASH => $pg->{PG_ANSWERS_HASH},
268+
PERSISTENCE_HASH => $pg->{PERSISTENCE_HASH},
269+
problem_result => $pg->{result},
270+
problem_state => $pg->{state},
271+
flags => $pg->{flags},
272+
psvn => $psvn,
273+
problem_seed => $problemSeed,
274+
resource_list => $pg->{resource_list},
275+
warning_messages => ref $pg->{warning_messages} eq 'ARRAY' ? $pg->{warning_messages} : [],
276+
debug_messages => ref $pg->{debug_messages} eq 'ARRAY' ? $pg->{debug_messages} : [],
277+
compute_time => logTimingInfo($beginTime, Benchmark->new),
281278
};
282279
}
283280

‎templates/ContentGenerator/Base/error_output.html.ep‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,8 @@
55
% }
66
<p>
77
<%= maketext(
8-
'WeBWorK has encountered a software error while attempting to process this problem. It is likely that '
9-
. 'there is an error in the problem itself. If you are a student, report this error message to your '
10-
. 'professor to have it corrected. If you are a professor, please consult the error output below for '
8+
'WeBWorK has encountered a software error. If you are a student, report this error message to your '
9+
. 'instructor to have it corrected. If you are a instructor, please consult the error output below for '
1110
. 'more information.'
1211
) %>
1312
</p>

‎templates/ContentGenerator/Base/feedback_macro_email.html.ep‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
% next if $key eq 'pg_object'; # Not used in internal feedback mechanism
77
<%= hidden_field $key => $value =%>
88
% }
9-
<%= submit_button maketext($ce->{feedback_button_name}) || maketext('Email instructor'),
10-
name => 'feedbackForm', class => 'btn btn-primary' =%>
9+
<div class="mb-3">
10+
<%= submit_button maketext($ce->{feedback_button_name}) || maketext('Email instructor'),
11+
name => 'feedbackForm', class => 'btn btn-primary' =%>
12+
</div>
1113
% end

‎templates/ContentGenerator/Base/problem_warning_and_debug_output.html.ep‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
% if ($warnings) {
2+
<div class="row g-0 mb-3">
3+
<div class="col-12 alert alert-danger m-0">
4+
<h2><%= maketext('PG warning messages') %></h2>
5+
<ul class="m-0">
6+
% for (split m/\n+/, $warnings) {
7+
<li><div><%== $_ %></div></li>
8+
% }
9+
</ul>
10+
</div>
11+
</div>
12+
% }
13+
% if (@$warning_messages) {
14+
<div class="row g-0 mb-3">
15+
<div class="col-12 alert alert-danger m-0">
16+
<h2><%= maketext('PG processing warning messages') %></h2>
17+
<ul class="m-0">
18+
% for (@$warning_messages) {
19+
<li><div><%== $_ %></div></li>
20+
% }
21+
</ul>
22+
</div>
23+
</div>
24+
% }
25+
% if (@$debug_messages) {
26+
<div class="row g-0 mb-3">
27+
<div class="col-12 alert alert-info m-0 overflow-x-auto">
28+
<h2><%= maketext('PG debug messages') %></h2>
29+
% for (@$debug_messages) {
30+
<div class="my-3"><%== $_ %></div>
31+
% }
32+
</div>
33+
</div>
34+
% }

0 commit comments

Comments
 (0)