Skip to content

Commit

Permalink
Rework problem data (or the persistence hash).
Browse files Browse the repository at this point in the history
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 openwebwork#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.
  • Loading branch information
drgrice1 committed Dec 19, 2024
1 parent f36d1c5 commit 3e471c0
Show file tree
Hide file tree
Showing 8 changed files with 46 additions and 93 deletions.
2 changes: 2 additions & 0 deletions lib/FormatRenderedProblem.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
);

Expand Down
74 changes: 11 additions & 63 deletions lib/WeBWorK/ContentGenerator/GatewayQuiz.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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} });
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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');

Expand Down Expand Up @@ -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 })
: ()
},
);

Expand All @@ -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;
}

Expand Down
9 changes: 8 additions & 1 deletion lib/WeBWorK/ContentGenerator/Problem.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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}) : ()
}
);

Expand Down Expand Up @@ -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('');
}

Expand Down
33 changes: 8 additions & 25 deletions lib/WeBWorK/Utils/ProblemProcessing.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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 = '';

Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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;
}
Expand Down
6 changes: 3 additions & 3 deletions lib/WeBWorK/Utils/Rendering.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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?
Expand Down
4 changes: 3 additions & 1 deletion lib/WebworkWebservice/RenderProblem.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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},
Expand Down
10 changes: 10 additions & 0 deletions templates/ContentGenerator/GatewayQuiz.html.ep
Original file line number Diff line number Diff line change
Expand Up @@ -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->() =%>
Expand Down
1 change: 1 addition & 0 deletions templates/RPCRenderFormats/default.html.ep
Original file line number Diff line number Diff line change
Expand Up @@ -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}
% }
Expand Down

0 comments on commit 3e471c0

Please sign in to comment.