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.
openwebwork · Dec 17, 2024 · 6102c63 · 6102c63
1 parent 44e76a4
commit 6102c63
Show file tree

Hide file tree

Showing 8 changed files with 46 additions and 93 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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}
 					% }