From 6102c63d19f677d8726f36da35678057edab3345 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Sat, 5 Aug 2023 17:41:53 -0500 Subject: [PATCH] Rework problem data (or the persistence hash). This makes it so that persistence hash is only saved to the database when answers are submitted. In the case that answers can be checked by the user (via the "Check Answers" button), then the persistence hash is saved JSON encoded in a hidden form field. That case does not need the security of saving to the database, and in that case it shouldn't be saved to the database because answers aren't being recorded and the data in that case should temporary. So a hidden form field is just right for this. I objected to the previous implementation when #1940 was submitted, but allowed it at that time, but this is how the problem_data should have been implemented. Nothing should be written to the database for this when answers are not being recorded. If an instructor is acting as a student, the previous code was saving the persistent data to the student's problem even when the instructor just enters the problem. Of course it was also saving every time the instructor did anything with the problem including using the "Preview My Answers" button, the "Check Answers" button, the "Show Correct Answers" button, the "Show Problem Grader" button. Literally any form submission of the page. That just was not thought out. The render_rpc (and html2xml) routes use the hidden form field approach to also support saving the problem data from the persistence hash. This means that problems that use the persistence hash can be tested in the PG problem editor. Note that the PERSISTENCE_HASH_UPDATE flag is removed. That didn't improve efficiency at all. The processing that was done with that was too much. Even if this were saved when it was before it would have been more efficient to just save it. The handling of the persistence hash is also reworked for PG in a corresponding pull request. PG just sends the hash, and it can contain anything that can be JSON encoded. Webwork2 just JSON encodes it and stores it, but only when answers are submitted. --- lib/FormatRenderedProblem.pm | 2 + lib/WeBWorK/ContentGenerator/GatewayQuiz.pm | 74 +++---------------- lib/WeBWorK/ContentGenerator/Problem.pm | 9 ++- lib/WeBWorK/Utils/ProblemProcessing.pm | 33 ++------- lib/WeBWorK/Utils/Rendering.pm | 6 +- lib/WebworkWebservice/RenderProblem.pm | 4 +- .../ContentGenerator/GatewayQuiz.html.ep | 10 +++ templates/RPCRenderFormats/default.html.ep | 1 + 8 files changed, 46 insertions(+), 93 deletions(-) diff --git a/lib/FormatRenderedProblem.pm b/lib/FormatRenderedProblem.pm index 4ac56a80d6..722039d62b 100644 --- a/lib/FormatRenderedProblem.pm +++ b/lib/FormatRenderedProblem.pm @@ -27,6 +27,7 @@ use warnings; use JSON; use Digest::SHA qw(sha1_base64); use Mojo::Util qw(xml_escape); +use Mojo::JSON qw(encode_json); use Mojo::DOM; use WeBWorK::Utils qw(getAssetURL); @@ -280,6 +281,7 @@ sub formatRenderedProblem { showCorrectAnswersButton => $ws->{inputs_ref}{showCorrectAnswersButton} // '', showCorrectAnswersOnlyButton => $ws->{inputs_ref}{showCorrectAnswersOnlyButton} // 0, showFooter => $ws->{inputs_ref}{showFooter} // '', + problem_data => encode_json($rh_result->{PERSISTENCE_HASH}), pretty_print => \&pretty_print ); diff --git a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm index 36ae2cb148..427dfb5aa2 100644 --- a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm +++ b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm @@ -894,7 +894,6 @@ async sub pre_header_initialize ($c) { if ($c->{submitAnswers} || (($c->{previewAnswers} || $c->param('newPage')) && $can{recordAnswers})) { # If answers are being submitted, then save the problems to the database. If this is a preview or page change # and answers can be recorded, then save the last answer for future reference. - # Also save the persistent data to the database even when the last answer is not saved. # Deal with answers being submitted for a proctored exam. If there are no attempts left, then delete the # proctor session key so that it isn't possible to start another proctored test without being reauthorized. @@ -920,28 +919,6 @@ async sub pre_header_initialize ($c) { ($past_answers_string, $encoded_last_answer_string, $scores, $answer_types_string) = create_ans_str_from_responses($c->{formFields}, $pg_result, $pureProblem->flags =~ /:needs_grading/); - - # Transfer persistent problem data from the PERSISTENCE_HASH: - # - Get keys to update first, to avoid extra work when no updated ar - # are needed. When none, we avoid the need to decode/encode JSON, - # to save the pureProblem when it would not otherwise be saved. - # - We are assuming that there is no need to DELETE old - # persistent data if the hash is empty, even if in - # potential there may be some data already in the database. - my @persistent_data_keys = keys %{ $pg_result->{PERSISTENCE_HASH_UPDATED} }; - if (@persistent_data_keys) { - my $json_data = decode_json($pureProblem->{problem_data} || '{}'); - for my $key (@persistent_data_keys) { - $json_data->{$key} = $pg_result->{PERSISTENCE_HASH}{$key}; - } - $pureProblem->problem_data(encode_json($json_data)); - - # If the pureProblem will not be saved below, we should save the - # persistent data here before any other changes are made to it. - if (($c->{submitAnswers} && !$will{recordAnswers})) { - $c->db->putProblemVersion($pureProblem); - } - } } else { my $prefix = sprintf('Q%04d_', $problemNumbers[$i]); my @fields = sort grep {/^(?!previous).*$prefix/} (keys %{ $c->{formFields} }); @@ -974,6 +951,7 @@ async sub pre_header_initialize ($c) { $pureProblem->attempted(1); $pureProblem->num_correct($pg_result->{state}{num_of_correct_ans}); $pureProblem->num_incorrect($pg_result->{state}{num_of_incorrect_ans}); + $pureProblem->problem_data(encode_json($pg_result->{PERSISTENCE_HASH} || '{}')); # Add flags which are really a comma separated list of answer types. $pureProblem->flags($answer_types_string); @@ -1144,45 +1122,6 @@ async sub pre_header_initialize ($c) { # Reset start time $c->param('startTime', ''); } - } else { - # This 'else' case includes initial load of the first page of the - # quiz and checkAnswers calls, as well as when $can{recordAnswers} - # is false. - - # Save persistent data to database even in this case, when answers - # would not or cannot be recorded. - my @pureProblems = $db->getAllProblemVersions($effectiveUserID, $setID, $versionID); - for my $i (0 .. $#problems) { - # Process each problem. - my $pureProblem = $pureProblems[ $probOrder[$i] ]; - my $pg_result = $pg_results[ $probOrder[$i] ]; - - if (ref $pg_result) { - # Transfer persistent problem data from the PERSISTENCE_HASH: - # - Get keys to update first, to avoid extra work when no updates - # are needed. When none, we avoid the need to decode/encode JSON, - # or to save the pureProblem. - # - We are assuming that there is no need to DELETE old - # persistent data if the hash is empty, even if in - # potential there may be some data already in the database. - my @persistent_data_keys = keys %{ $pg_result->{PERSISTENCE_HASH_UPDATED} }; - next unless (@persistent_data_keys); # stop now if nothing to do - if ($isFakeSet) { - warn join("", - "This problem stores persistent data and this cannot be done in a fake set. ", - "Some functionality may not work properly when testing this problem in this setting."); - next; - } - - my $json_data = decode_json($pureProblem->{problem_data} || '{}'); - for my $key (@persistent_data_keys) { - $json_data->{$key} = $pg_result->{PERSISTENCE_HASH}{$key}; - } - $pureProblem->problem_data(encode_json($json_data)); - - $c->db->putProblemVersion($pureProblem); - } - } } debug('end answer processing'); @@ -1485,7 +1424,10 @@ async sub getProblemHTML ($c, $effectiveUser, $set, $formFields, $mergedProblem) : !$c->{previewAnswers} && $c->{will}{showCorrectAnswers} ? 1 : 0 ), - debuggingOptions => getTranslatorDebuggingOptions($c->authz, $c->{userID}) + debuggingOptions => getTranslatorDebuggingOptions($c->authz, $c->{userID}), + $c->{can}{checkAnswers} && defined $formFields->{ 'problem_data_' . $mergedProblem->problem_id } + ? (problemData => $formFields->{ 'problem_data_' . $mergedProblem->problem_id }) + : () }, ); @@ -1504,6 +1446,12 @@ async sub getProblemHTML ($c, $effectiveUser, $set, $formFields, $mergedProblem) $pg->{body_text} = undef; } + # If the user can check answers and either this is not an answer submission or the problem_data form + # parameter was previously set, then set or update the problem_data form parameter. + $c->param('problem_data_' . $mergedProblem->problem_id => encode_json($pg->{PERSISTENCE_HASH} || '{}')) + if $c->{can}{checkAnswers} + && (!$c->{submitAnswers} || defined $c->param('problem_data_' . $mergedProblem->problem_id)); + return $pg; } diff --git a/lib/WeBWorK/ContentGenerator/Problem.pm b/lib/WeBWorK/ContentGenerator/Problem.pm index b3e81975fb..1eb692e7f7 100644 --- a/lib/WeBWorK/ContentGenerator/Problem.pm +++ b/lib/WeBWorK/ContentGenerator/Problem.pm @@ -602,7 +602,9 @@ async sub pre_header_initialize ($c) { : !$c->{previewAnswers} && $will{showCorrectAnswers} ? 1 : 0 ), - debuggingOptions => getTranslatorDebuggingOptions($authz, $userID) + debuggingOptions => getTranslatorDebuggingOptions($authz, $userID), + $can{checkAnswers} + && defined $formFields->{problem_data} ? (problemData => $formFields->{problem_data}) : () } ); @@ -1412,6 +1414,11 @@ sub output_misc ($c) { push(@$output, $c->hidden_field(studentNavFilter => $c->param('studentNavFilter'))) if $c->param('studentNavFilter'); + # If the user can check answers and a problem_data form parameter for + # this problem has been set then add a hidden input with that data. + push(@$output, $c->hidden_field(problem_data => $c->param('problem_data'))) + if $c->{can}{checkAnswers} && defined $c->param('problem_data'); + return $output->join(''); } diff --git a/lib/WeBWorK/Utils/ProblemProcessing.pm b/lib/WeBWorK/Utils/ProblemProcessing.pm index 2459169556..0e9725b80f 100644 --- a/lib/WeBWorK/Utils/ProblemProcessing.pm +++ b/lib/WeBWorK/Utils/ProblemProcessing.pm @@ -23,7 +23,6 @@ for the problem pages, especially those generated by Problem.pm. =cut -use Mojo::JSON qw(encode_json); use Email::Stuffer; use Try::Tiny; use Mojo::JSON qw(encode_json decode_json); @@ -67,27 +66,6 @@ async sub process_and_log_answer ($c) { my $pureProblem = $db->getUserProblem($problem->user_id, $problem->set_id, $problem->problem_id); my $answer_log = $ce->{courseFiles}{logs}{answer_log}; - # Transfer persistent problem data from the PERSISTENCE_HASH: - # - Get keys to update first, to avoid extra work when no updates - # are needed. When none, we avoid the need to decode/encode JSON, - # or to save the pureProblem. - # - We are assuming that there is no need to DELETE old - # persistent data if the hash is empty, even if in - # potential there may be some data already in the database. - if (defined($pureProblem)) { - my @persistent_data_keys = keys %{ $pg->{PERSISTENCE_HASH_UPDATED} }; - if (@persistent_data_keys) { - my $json_data = decode_json($pureProblem->{problem_data} || '{}'); - for my $key (@persistent_data_keys) { - $json_data->{$key} = $pg->{PERSISTENCE_HASH}{$key}; - } - $pureProblem->problem_data(encode_json($json_data)); - if (!$submitAnswers) { # would not be saved below - $db->putUserProblem($pureProblem); - } - } - } - my ($encoded_last_answer_string, $scores2, $answer_types_string); my $scoreRecordedMessage = ''; @@ -134,13 +112,13 @@ async sub process_and_log_answer ($c) { # this stores previous answers to the problem to provide "sticky answers" if ($submitAnswers) { if (defined $pureProblem) { - # store answers in DB for sticky answers - my %answersToStore; - # store last answer to database for use in "sticky" answers $problem->last_answer($encoded_last_answer_string); $pureProblem->last_answer($encoded_last_answer_string); + # Also store persistent problem data. + $pureProblem->problem_data(encode_json($pg->{PERSISTENCE_HASH} || '{}')); + # store state in DB if it makes sense if ($will{recordAnswers}) { my $score = @@ -268,6 +246,11 @@ async sub process_and_log_answer ($c) { } } + # If the user can check answers and either this is not an answer submission or the problem_data form parameter was + # previously set, then set or update the problem_data form parameter. + $c->param(problem_data => encode_json($pg->{PERSISTENCE_HASH} || '{}')) + if $c->{can}{checkAnswers} && (!$submitAnswers || defined $c->param('problem_data')); + $c->{scoreRecordedMessage} = $scoreRecordedMessage; return $scoreRecordedMessage; } diff --git a/lib/WeBWorK/Utils/Rendering.pm b/lib/WeBWorK/Utils/Rendering.pm index 110c3972ba..c75d1e57ac 100644 --- a/lib/WeBWorK/Utils/Rendering.pm +++ b/lib/WeBWorK/Utils/Rendering.pm @@ -133,7 +133,8 @@ sub constructPGOptions ($ce, $user, $set, $problem, $psvn, $formFields, $transla $options{needs_grading} = ($problem->flags // '') =~ /:needs_grading$/; # Persistent problem data - $options{PERSISTENCE_HASH} = decode_json($problem->problem_data || '{}'); + $options{PERSISTENCE_HASH} = decode_json($translationOptions->{problemData} + // (defined $problem->problem_data && $problem->problem_data ne '' ? $problem->problem_data : '{}')); # Language $options{language} = $ce->{language}; @@ -284,8 +285,7 @@ sub renderPG ($c, $effectiveUser, $set, $problem, $psvn, $formFields, $translati map { $_ => $pg->{pgcore}{PG_alias}{resource_list}{$_}{uri} } keys %{ $pg->{pgcore}{PG_alias}{resource_list} } }; - $ret->{PERSISTENCE_HASH_UPDATED} = $pg->{pgcore}{PERSISTENCE_HASH_UPDATED}; - $ret->{PERSISTENCE_HASH} = $pg->{pgcore}{PERSISTENCE_HASH}; + $ret->{PERSISTENCE_HASH} = $pg->{pgcore}{PERSISTENCE_HASH}; } # Save the problem source. This is used by Caliper::Entity. Why? diff --git a/lib/WebworkWebservice/RenderProblem.pm b/lib/WebworkWebservice/RenderProblem.pm index 31d15ed169..5e848719a6 100644 --- a/lib/WebworkWebservice/RenderProblem.pm +++ b/lib/WebworkWebservice/RenderProblem.pm @@ -252,7 +252,8 @@ async sub renderProblem { show_pg_info => $rh->{show_pg_info} // 0, show_answer_hash_info => $rh->{show_answer_hash_info} // 0, show_answer_group_info => $rh->{show_answer_group_info} // 0 - } + }, + defined $rh->{problem_data} && $rh->{problem_data} ne '' ? (problemData => $rh->{problem_data}) : () }; $ce->{pg}{specialPGEnvironmentVars}{problemPreamble} = { TeX => '', HTML => '' } if $rh->{noprepostambles}; @@ -270,6 +271,7 @@ async sub renderProblem { errors => $pg->{errors}, pg_warnings => $pg->{warnings}, PG_ANSWERS_HASH => $pg->{PG_ANSWERS_HASH}, + PERSISTENCE_HASH => $pg->{PERSISTENCE_HASH}, problem_result => $pg->{result}, problem_state => $pg->{state}, flags => $pg->{flags}, diff --git a/templates/ContentGenerator/GatewayQuiz.html.ep b/templates/ContentGenerator/GatewayQuiz.html.ep index e6d77dcd54..3aef35d2db 100644 --- a/templates/ContentGenerator/GatewayQuiz.html.ep +++ b/templates/ContentGenerator/GatewayQuiz.html.ep @@ -644,6 +644,16 @@ <%= hidden_field 'probstatus' . $problems->[ $probOrder->[$i] ]{problem_id} => $c->{probStatus}[ $probOrder->[$i] ] %> % } + % + % # If the user can check answers and a problem_data form parameter for + % # this problem has been set then add a hidden input with that data. + % if ( + % $c->{can}{checkAnswers} + % && defined $c->param('problem_data_' . $problems->[ $probOrder->[$i] ]{problem_id}) + % ) { + <%= hidden_field 'problem_data_' . $problems->[ $probOrder->[$i] ]{problem_id} + => param('problem_data_' . $problems->[ $probOrder->[$i] ]{problem_id}) =%> + % } % } % <%= $jumpLinks->() =%> diff --git a/templates/RPCRenderFormats/default.html.ep b/templates/RPCRenderFormats/default.html.ep index b3fed74bbb..784f13416d 100644 --- a/templates/RPCRenderFormats/default.html.ep +++ b/templates/RPCRenderFormats/default.html.ep @@ -100,6 +100,7 @@ %= hidden_field showCorrectAnswersOnlyButton => $showCorrectAnswersOnlyButton %= hidden_field showFooter => $showFooter %= hidden_field extra_header_text => $extra_header_text + %= hidden_field problem_data => $problem_data % if ($formatName eq 'debug' && $ws->{inputs_ref}{clientDebug}) { %= hidden_field clientDebug => $ws->{inputs_ref}{clientDebug} % }