From 667f0bbc762967d72d9ca4f4cf6551ceb7da993b Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 20 Jun 2025 12:09:43 -0700 Subject: [PATCH 1/4] This will update the pgeditor to use the pgcritic to analyze a problem. --- htdocs/js/PGProblemEditor/pgproblemeditor.js | 26 ++++ lib/WebworkWebservice.pm | 1 + lib/WebworkWebservice/ProblemActions.pm | 22 +++- .../PGProblemEditor/format_code_form.html.ep | 14 ++ .../PGProblemEditor/pg_critic.html.ep | 121 ++++++++++++++++++ 5 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep diff --git a/htdocs/js/PGProblemEditor/pgproblemeditor.js b/htdocs/js/PGProblemEditor/pgproblemeditor.js index f48214574f..8e9fa78a54 100644 --- a/htdocs/js/PGProblemEditor/pgproblemeditor.js +++ b/htdocs/js/PGProblemEditor/pgproblemeditor.js @@ -231,6 +231,30 @@ .catch((err) => showMessage(`Error: ${err?.message ?? err}`)); }; + // Send a request to the server to run the PG critic in the CodeMirror editor. + const runPGCritic = () => { + const request_object = { courseID: document.getElementsByName('courseID')[0]?.value }; + + const user = document.getElementsByName('user')[0]; + if (user) request_object.user = user.value; + const sessionKey = document.getElementsByName('key')[0]; + if (sessionKey) request_object.key = sessionKey.value; + + request_object.rpc_command = 'runPGCritic'; + request_object.pgCode = + webworkConfig?.pgCodeMirror?.source ?? document.getElementById('problemContents')?.value ?? ''; + + fetch(webserviceURL, { method: 'post', mode: 'same-origin', body: new URLSearchParams(request_object) }) + .then((response) => response.json()) + .then((data) => { + renderArea.innerHTML = data.result_data.html; + }) + .catch((err) => { + console.log(err); + showMessage(`Error: ${err?.message ?? err}`); + }); + }; + document.getElementById('take_action')?.addEventListener('click', async (e) => { if (document.getElementById('current_action')?.value === 'format_code') { e.preventDefault(); @@ -240,6 +264,8 @@ document.querySelector('input[name="action.format_code"]:checked').value == 'convertCodeToPGML' ) { convertCodeToPGML(); + } else if (document.querySelector('input[name="action.format_code"]:checked').value == 'runPGCritic') { + runPGCritic(); } return; } diff --git a/lib/WebworkWebservice.pm b/lib/WebworkWebservice.pm index 1bd113b619..f44e4e5e5c 100644 --- a/lib/WebworkWebservice.pm +++ b/lib/WebworkWebservice.pm @@ -270,6 +270,7 @@ sub command_permission { putPastAnswer => 'problem_grader', tidyPGCode => 'access_instructor_tools', convertCodeToPGML => 'access_instructor_tools', + runPGCritic => 'access_instructor_tools', # WebworkWebservice::RenderProblem renderProblem => 'proctor_quiz_login', diff --git a/lib/WebworkWebservice/ProblemActions.pm b/lib/WebworkWebservice/ProblemActions.pm index 7684872367..9ec13c57ec 100644 --- a/lib/WebworkWebservice/ProblemActions.pm +++ b/lib/WebworkWebservice/ProblemActions.pm @@ -21,8 +21,9 @@ use warnings; use Data::Structure::Util qw(unbless); -use WeBWorK::PG::Tidy qw(pgtidy); -use WeBWorK::PG::ConvertToPGML qw(convertToPGML); +use WeBWorK::PG::Tidy qw(pgtidy); +use WeBWorK::PG::ConvertToPGML qw(convertToPGML); +use WeBWorK::PG::PGProblemCritic qw(analyzePGcode); sub getUserProblem { my ($invocant, $self, $params) = @_; @@ -180,4 +181,21 @@ sub convertCodeToPGML { } +sub runPGCritic { + my ($invocant, $self, $params) = @_; + my $pg_critic_results = analyzePGcode($params->{pgCode}); + + my $html_output = $self->c->render_to_string( + template => 'ContentGenerator/Instructor/PGProblemEditor/pg_critic', + results => $pg_critic_results + ); + + return { + ra_out => { + html => $html_output + }, + text => 'The script pg-critic has been run successfully.' + }; +} + 1; diff --git a/templates/ContentGenerator/Instructor/PGProblemEditor/format_code_form.html.ep b/templates/ContentGenerator/Instructor/PGProblemEditor/format_code_form.html.ep index 69da3d5a95..0ddab6d09f 100644 --- a/templates/ContentGenerator/Instructor/PGProblemEditor/format_code_form.html.ep +++ b/templates/ContentGenerator/Instructor/PGProblemEditor/format_code_form.html.ep @@ -30,4 +30,18 @@ +
+ <%= radio_button 'action.format_code' => 'runPGCritic', + id => 'action_format_code_run_pgcritic', class => 'form-check-input'=%> + <%= label_for 'action_format_code_run_pgcritic', class => 'form-check-label', begin =%> + <%== maketext('Run the PG Critic Analyzer') =%> + <% end =%> + + + + +
diff --git a/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep b/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep new file mode 100644 index 0000000000..278ce91bcf --- /dev/null +++ b/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep @@ -0,0 +1,121 @@ +
+

PG Critic Results

+ +

Metadata

+ +

The following lists required metadata. If any is missing, the given tag must be filled in. + However, make sure that the categories are correct, especially if the problem has been + copied.

+ +% sub showIcon { my $show = shift; +% return $show ? q! +% +% +% ! : q! +% +% +% !; +%} + + + + + + + +
DBsubject <%== showIcon($results->{metadata}{DBsection}) %>
DBchapter <%== showIcon($results->{metadata}{DBchapter}) %>
DBsection <%== showIcon($results->{metadata}{DBsection}) %>
Keywords <%== showIcon($results->{metadata}{KEYWORDS}) %>
+ +

You done good with the following

+ + + +% if ($results->{good}{PGML}) { + +%} +% if ($results->{good}{solution}) { + +%} +% if ($results->{good}{hint}) { + +
PGMLThis problem uses PGML, the current preferred way to write problem (text), solution and hint + blocks.
SolutionsThis problem has a solution block. Every problem should have solutions that the + student can view after the answer data.
HintsThis problem has a hint. This can be helpful for students after attempting the problem + a few times (this can be set by the instructor). +%} +
+ + +% if( scalar(@{$results->{deprecated_macros}}) > 0) { +

Deprecated Macros

+

This problem has the following deprecated macros: <%= join(', ',@{$results->{deprecated_macros}} ) %>

+ +

These should be removed from the problem in that these macros will be deleted from PG in a future + version. The functions from these macros may be listed below to help aid in transitioning away from + these macros.

+% } + + +% my $has_bad_features = 0; +% $has_bad_features += $results->{bad}{$_} for (keys %{$results->{bad}}); + +% # <%== dumper $results->{bad} %> + +% if ($has_bad_features) { +

You can improve on the following:

+

There are features in this problem that contain old or deprecated features. The following +list gives feedback of how the problem can be improved.

+%} + + + + +
From 3fa727c6f69412e78f53d761cd390e69d6bb2c87 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 20 Jun 2025 13:53:15 -0700 Subject: [PATCH 2/4] Improvements part of the pgcritic report now shows up if the only problem is that solutions are not shown. --- .../Instructor/PGProblemEditor/pg_critic.html.ep | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep b/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep index 278ce91bcf..b18b1cf167 100644 --- a/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep +++ b/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep @@ -54,13 +54,10 @@ these macros.

% } - % my $has_bad_features = 0; % $has_bad_features += $results->{bad}{$_} for (keys %{$results->{bad}}); -% # <%== dumper $results->{bad} %> - -% if ($has_bad_features) { +% if ($has_bad_features || !$results->{good}{solution}) {

You can improve on the following:

There are features in this problem that contain old or deprecated features. The following list gives feedback of how the problem can be improved.

From ef458598ebd4bfce1524d9656c7d404f03ecdf4b Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Fri, 27 Jun 2025 16:59:53 -0400 Subject: [PATCH 3/4] Improved work on the PG critic interface to the PGEditor. --- .../PGProblemEditor/pg_critic.html.ep | 75 ++++++++++++------- 1 file changed, 49 insertions(+), 26 deletions(-) diff --git a/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep b/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep index b18b1cf167..f75fabe741 100644 --- a/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep +++ b/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep @@ -25,22 +25,40 @@ Keywords <%== showIcon($results->{metadata}{KEYWORDS}) %> -

You done good with the following

- +% my $pos = $results->{positive}; +% if ($pos->{PGML} || $pos->{solution} || $pos->{hint}) { +

Good aspects of this problems are the following

+% } -% if ($results->{good}{PGML}) { +% if ($pos->{PGML}) { -%} -% if ($results->{good}{solution}) { +% } +% if ($pos->{solution}) { -%} -% if ($results->{good}{hint}) { +% } +% if ($pos->{hint}) { +% } +% my @good_parsers = grep { $pos->{parsers}->{$_} } keys %{$pos->{parsers}}; +% if (@good_parsers) { + +% } +% my @good_macros = grep { $pos->{macros}->{$_} } keys %{$pos->{macros}}; +% if (@good_macros) { + +% }
PGMLThis problem uses PGML, the current preferred way to write problem (text), solution and hint blocks.
SolutionsThis problem has a solution block. Every problem should have solutions that the student can view after the answer data.
HintsThis problem has a hint. This can be helpful for students after attempting the problem a few times (this can be set by the instructor). -%} +% } +% # list of the positive contexts: +% my @good_contexts = grep { $pos->{contexts}{$_} } keys %{$pos->{parsers}}; +% if (@good_contexts) { +
Modern ContextsThis problem uses the following modern contexts: + <%= join(', ', @good_contexts) %>
Modern ParsersThis problem uses features of the following modern parsers: + <%= join(', ', @good_parsers) %>
Modern MacrosThis problem uses functionality from the following modern macros: + <%= join(', ', @good_macros) %>
@@ -55,59 +73,64 @@ % } % my $has_bad_features = 0; -% $has_bad_features += $results->{bad}{$_} for (keys %{$results->{bad}}); +% $has_bad_features += $results->{negative}{$_} for (keys %{$results->{negative}}); -% if ($has_bad_features || !$results->{good}{solution}) { +% if ($has_bad_features || !$pos->{solution}) {

You can improve on the following:

There are features in this problem that contain old or deprecated features. The following list gives feedback of how the problem can be improved.

%}
    -% if ($results->{bad}{BEGIN_TEXT}) { +% if ($results->{negative}{BEGIN_TEXT}) {
  • This problem contains older formatting blocks like BEGIN_TEXT. Consider use PGML. In the Format Code section of the PG Editor, the "Convert to PGML" should be used as a start to get the problem switched.
    • %} -% if ($results->{bad}{beginproblem}) { +% if ($results->{negative}{beginproblem}) {
  • This problem contains the line TEXT(beginproblem()). This is no longer necessary and should be removed.
    • %} -% if ($results->{bad}{context_texstrings}) { +% if ($results->{negative}{context_texstrings}) {
  • This problem contains the line Context()->texStrings;. This is no longer necessary and should be removed.
    • %} -% if ($results->{bad}{oldtable}) { +% if ($results->{negative}{oldtable}) {
  • This problem contains the deprecated begintable command. This is not assessible and often cannot be converted to hardcopy. This table should be written using nicetables or a PGML table.
    • %} -% if ($results->{bad}{showPartialCorrect}) { +% if ($results->{negative}{showPartialCorrect}) {
  • This problem contains the line $showPartialCorrectAnswers = 1. This is enabled by default and needed only if set to 0.
    • % } -% if (!$results->{good}{solution}) { +% if (!$pos->{solution}) {
  • This problem does not have a solution. Consider adding one.
    • % } -% if ($results->{bad}{fun_cmp} || $results->{bad}{str_cmp} || $results->{bad}{num_cmp}) { +% if ($results->{negative}{fun_cmp} || $results->{negative}{str_cmp} || $results->{negative}{num_cmp}) {
  • This problem contains the functioins num_cmp, str_cmp or fun_cmp. These are old ways of checking answers. These should be converted to MathObjects. % } -% if ($results->{bad}{multiple_loadmacros}) { +% if ($results->{negative}{multiple_loadmacros}) {
  • This problem contains two loadMacros function call. Combine the function calls and make sure that all macros are needed for your problem.
    • % } -% if ($results->{bad}{old_multiple_choice}) { +% if ($results->{negative}{macros}{PGchoicemacros}) {
  • This problem contains old versions of multiple choice. The sample problems - - Multiple Choice with Checkbox, - Multiple Choice with Popup and + + Multiple Choice with Checkbox, + Multiple Choice with Popup and Multiple Choice with Radio Buttons should be examined as well the macros: - parserPopUp.pl, - parserCheckboxList.pl and - parserRadioButtons.pl. + parserPopUp.pl, + parserCheckboxList.pl and + parserRadioButtons.pl.
    • % } -% if ($results->{bad}{lines_below_enddocument}) { +% if ($results->{negative}{macros}{PGgraphmacros}) { +
  • This problem uses PGgraphmacros a old plotting library. Consider using + Plots.pl + or PGtikz.pl +%} +% if ($results->{negative}{lines_below_enddocument}) {
  • There is content (code or other text), below the ENDDOCUMENT() line. Although this is ignored, there shouldn't be content in this area.
    • % } From bfc499de9c5e9910889618b783e9a1a17f51b343 Mon Sep 17 00:00:00 2001 From: Peter Staab Date: Tue, 8 Jul 2025 14:05:25 -0400 Subject: [PATCH 4/4] Add randomness to the list of criteria to be checked. --- .../PGProblemEditor/pg_critic.html.ep | 62 ++++++++++--------- 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep b/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep index f75fabe741..d5a4e77f1d 100644 --- a/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep +++ b/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep @@ -33,31 +33,35 @@ % if ($pos->{PGML}) { PGMLThis problem uses PGML, the current preferred way to write problem (text), solution and hint - blocks. + blocks. % } % if ($pos->{solution}) { SolutionsThis problem has a solution block. Every problem should have solutions that the - student can view after the answer data. + student can view after the answer data. % } % if ($pos->{hint}) { HintsThis problem has a hint. This can be helpful for students after attempting the problem - a few times (this can be set by the instructor). + a few times (this can be set by the instructor). +% } +% if ($pos->{randomness}) { + RandomnessThis problem uses randomness. This is desired to give to a class of students, each + of whom may have a different problem. % } % # list of the positive contexts: % my @good_contexts = grep { $pos->{contexts}{$_} } keys %{$pos->{parsers}}; % if (@good_contexts) { Modern ContextsThis problem uses the following modern contexts: - <%= join(', ', @good_contexts) %> + <%= join(', ', @good_contexts) %> % } % my @good_parsers = grep { $pos->{parsers}->{$_} } keys %{$pos->{parsers}}; % if (@good_parsers) { Modern ParsersThis problem uses features of the following modern parsers: - <%= join(', ', @good_parsers) %> + <%= join(', ', @good_parsers) %> % } % my @good_macros = grep { $pos->{macros}->{$_} } keys %{$pos->{macros}}; % if (@good_macros) { Modern MacrosThis problem uses functionality from the following modern macros: - <%= join(', ', @good_macros) %> + <%= join(', ', @good_macros) %> % } @@ -68,25 +72,24 @@

    This problem has the following deprecated macros: <%= join(', ',@{$results->{deprecated_macros}} ) %>

    These should be removed from the problem in that these macros will be deleted from PG in a future - version. The functions from these macros may be listed below to help aid in transitioning away from - these macros.

    + version. The functions from these macros may be listed below to help aid in transitioning away from + these macros.

    % } % my $has_bad_features = 0; % $has_bad_features += $results->{negative}{$_} for (keys %{$results->{negative}}); % if ($has_bad_features || !$pos->{solution}) { -

    You can improve on the following:

    -

    There are features in this problem that contain old or deprecated features. The following -list gives feedback of how the problem can be improved.

    +

    You can improve on the following:

    +

    There are features in this problem that contain old or deprecated features. The following + list gives feedback of how the problem can be improved.

    %}
      % if ($results->{negative}{BEGIN_TEXT}) {
    • This problem contains older formatting blocks like BEGIN_TEXT. Consider use PGML. - In the Format Code section of the PG Editor, the "Convert to PGML" should be used - as a start to get the problem switched. -
      • + In the Format Code section of the PG Editor, the "Convert to PGML" should be used + as a start to get the problem switched. %} % if ($results->{negative}{beginproblem}) {
    • This problem contains the line TEXT(beginproblem()). This is no longer necessary and should be removed.
      • @@ -96,43 +99,46 @@ list gives feedback of how the problem can be improved.

      %} % if ($results->{negative}{oldtable}) {
    • This problem contains the deprecated begintable command. This is not assessible and often cannot be - converted to hardcopy. This table should be written using nicetables or a PGML table.
      • + converted to hardcopy. This table should be written using nicetables or a PGML table. %} % if ($results->{negative}{showPartialCorrect}) {
    • This problem contains the line $showPartialCorrectAnswers = 1. This is enabled by default and needed only - if set to 0.
      • + if set to 0. % } % if (!$pos->{solution}) {
    • This problem does not have a solution. Consider adding one.
      • % } +% if (!$pos->{randomness}) { +
    • This problem does not have randomness. Consider adding variables that take on random values.
      • +% } % if ($results->{negative}{fun_cmp} || $results->{negative}{str_cmp} || $results->{negative}{num_cmp}) {
    • This problem contains the functioins num_cmp, str_cmp or fun_cmp. - These are old ways of checking answers. These should be converted to MathObjects. + These are old ways of checking answers. These should be converted to MathObjects. % } % if ($results->{negative}{multiple_loadmacros}) {
    • This problem contains two loadMacros function call. Combine the function - calls and make sure that all macros are needed for your problem.
      • + calls and make sure that all macros are needed for your problem. % } % if ($results->{negative}{macros}{PGchoicemacros}) {
    • This problem contains old versions of multiple choice. The sample problems - - Multiple Choice with Checkbox, - Multiple Choice with Popup and - Multiple Choice with Radio Buttons should be examined as well the macros: - parserPopUp.pl, - parserCheckboxList.pl and - parserRadioButtons.pl. + + Multiple Choice with Checkbox, + Multiple Choice with Popup and + Multiple Choice with Radio Buttons should be examined as well the macros: + parserPopUp.pl, + parserCheckboxList.pl and + parserRadioButtons.pl.
      • % } % if ($results->{negative}{macros}{PGgraphmacros}) {
    • This problem uses PGgraphmacros a old plotting library. Consider using - Plots.pl - or PGtikz.pl + Plots.pl + or PGtikz.pl
      • %} % if ($results->{negative}{lines_below_enddocument}) {
    • There is content (code or other text), below the ENDDOCUMENT() line. Although this - is ignored, there shouldn't be content in this area.
      • + is ignored, there shouldn't be content in this area. % }