Skip to content

Commit

Permalink
Fix setting user overrides for dates on the user detail page for set …
Browse files Browse the repository at this point in the history 
…versions.

This addresses issue #2340.

Currently if an instructor unchecks a checkbox for a user's versioned
set on the user detail page (the page you can reach by clicking on the
"Assigned Sets" column for a user in the Accounts Manager), then when
dates are checked (in the `checkDates` method of `UserDetail.pm`), the
dates from the global set record are used. If those are valid dates
(they usually will be in this case), then those valid dates are returned
with no error.  Thus the database will be updated.  However, the logic
used to determine how the database is updated does not match the logic
used to check the sets, and since the override checkbox is not checked,
the date in the user's versioned set record is set to NULL.  So for
versioned sets this makes it so that if the checkbox is not checked or
the override date is not set, then the global set date is NOT used for a
fallback.  Instead 0 is used for a fallback.  Since that is not a valid
date, this forces an error, and the database is not updated.

This is actually a more general issue with the logic used when the dates
are checked and the logic used when the database is updated not being
the same.  So that is fixed.

Additionally when the dates in the database are updated, there are cases
where the user interface does not correctly reflect the changes made.
One such case is for a regular set if the checkbox for a date is checked
but the override date left empty.  Then the global set date is used
(meaning the user set value for that date is set to NULL in the
database), however the checkbox remains checked.  So the corresponding
parameters are updated so that the changes are reflected when the page
rerenders after saving.

Note that in the case that the dates are not updated in the database due
to an error in the given dates, the user interface does not update to
reflect the current database state.  I don't think the user interface
should be updated in this case, as the instructor might want an
opportunity to correct the error and not have their changes wiped out.

I also noticed that the "User overrides" header was being displayed over
the "Open", "Closes", "Answer" labels in the case that a set is not
assigned to a user (and so there are no override fields).  So that is
not shown in that case anymore.

I also removed the 10 year limit on dates.  I am not sure why we impose
this limit.  There is no reason to do so.  In fact, it is quite useful
in many situations to have a due date that is more than 10 years in the
future.  By the way, I was curious if these dates were affected by the
year 2038 problem.  So after removing this restriction I set a due date
to be in 2050.  This seems to work with no issue. So it seems that 64bit
integers are being used in this case which are not affected by the year
2038 problem.  So we have 292 million years before this will be an issue
again.
  • Loading branch information
@drgrice1
drgrice1 committed Feb 27, 2024
1 parent 65a1b74 commit bbdcbf0
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 25 deletions.
45 changes: 21 additions & 24 deletions lib/WeBWorK/ContentGenerator/Instructor/UserDetail.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,14 @@ sub initialize ($c) {
unless ($rh_dates->{error}) {
# If no error update database
for my $field (@{ DATE_FIELDS_ORDER() }) {
if (defined $c->param("set.$setID.$field.override")) {
if (defined $c->param("set.$setID.$field.override") && $c->param("set.$setID.$field") ne '') {
$userSetRecord->$field($rh_dates->{$field});
} else {
$userSetRecord->$field(undef); #stop override

# Update the parameters so the user interface reflects the changes made.
$c->param("set.$setID.$field.override", undef);
$c->param("set.$setID.$field", undef);
}
}
$db->putUserSet($userSetRecord);
Expand All @@ -113,10 +117,16 @@ sub initialize ($c) {
my $rh_dates = $c->checkDates($setVersionRecord, "$setID,v$ver");
unless ($rh_dates->{error}) {
for my $field (@{ DATE_FIELDS_ORDER() }) {
if (defined($c->param("set.$setID,v$ver.$field.override"))) {
if (defined $c->param("set.$setID,v$ver.$field.override")
&& $c->param("set.$setID,v$ver.$field") ne '')
{
$setVersionRecord->$field($rh_dates->{$field});
} else {
$setVersionRecord->$field(undef);

# Update the parameters so the user interface reflects the changes made.
$c->param("set.$setID,v$ver.$field.override", undef);
$c->param("set.$setID,v$ver.$field", undef);
}
}
$db->putSetVersion($setVersionRecord);
Expand Down Expand Up @@ -167,25 +177,27 @@ sub initialize ($c) {
}

sub checkDates ($c, $setRecord, $setID) {
my $error = 0;

# For each of the dates, use the override date if set. Otherwise use the value from the global set.
# Except in the case that this is a set version. In that case use 0 which will result in an error below.
# This will prevent the dates for the set version from being changed to invalid values in that case.
my %dates;
for my $field (@{ DATE_FIELDS_ORDER() }) {
$dates{$field} =
(defined $c->param("set.$setID.$field.override") && $c->param("set.$setID.$field") ne '')
? $c->param("set.$setID.$field")
: $setRecord->$field;
: ($setID =~ /,v\d+$/ ? 0 : $setRecord->$field);
}

my ($open_date, $reduced_scoring_date, $due_date, $answer_date) = map { $dates{$_} } @{ DATE_FIELDS_ORDER() };

unless ($answer_date && $due_date && $open_date) {
$c->addbadmessage("set $setID has errors in its dates: answer_date |$answer_date|, "
. "due date |$due_date|, open_date |$open_date|");
$error = 1;
$c->addbadmessage("Set $setID has errors in its dates: "
. "open_date |$open_date|, due date |$due_date|, answer_date |$answer_date|");
return { %dates, error => 1 };
}

my $error = 0;

if ($answer_date < $due_date || $answer_date < $open_date) {
$c->addbadmessage("Answers cannot be made available until on or after the due date in set $setID!");
$error = 1;
Expand All @@ -204,22 +216,7 @@ sub checkDates ($c, $setRecord, $setID) {
$error = 1;
}

# Make sure the dates are not more than 10 years in the future.
my $cutoff = time + 31_556_926 * 10;
if ($open_date > $cutoff) {
$c->addbadmessage("Error: open date cannot be more than 10 years from now in set $setID");
$error = 1;
}
if ($due_date > $cutoff) {
$c->addbadmessage("Error: due date cannot be more than 10 years from now in set $setID");
$error = 1;
}
if ($answer_date > $cutoff) {
$c->addbadmessage("Error: answer date cannot be more than 10 years from now in set $setID");
$error = 1;
}

$c->addbadmessage('No date changes were saved!') if ($error);
$c->addbadmessage('No date changes were saved!') if $error;

return { %dates, error => $error };
}
Expand Down
4 changes: 3 additions & 1 deletion templates/ContentGenerator/Instructor/UserDetail/set_date_table.html.ep
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
<table>
<tr>
<th scope="col" colspan="3">
<%= maketext("User overrides") =%>
% if (defined $userRecord) {
<%= maketext("User overrides") =%>
% }
</th>
<th scope="col">
<%= maketext("Set values") =%>
Expand Down

0 comments on commit bbdcbf0

Please sign in to comment.