From 6c7aa6d3c5f6a5991eac1d64eb61ec9224b8c66a Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Tue, 27 Feb 2024 07:44:27 -0600 Subject: [PATCH] Fix setting user overrides for dates on the user detail page for set 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. --- .../ContentGenerator/Instructor/UserDetail.pm | 53 +++++++++---------- .../UserDetail/set_date_table.html.ep | 4 +- 2 files changed, 27 insertions(+), 30 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/UserDetail.pm b/lib/WeBWorK/ContentGenerator/Instructor/UserDetail.pm index e91aeac478..9065ebf42a 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/UserDetail.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/UserDetail.pm @@ -90,10 +90,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); @@ -110,18 +114,22 @@ sub initialize ($c) { if (defined $action) { if ($action eq 'assigned') { # This version is not to be deleted. - # Check to see if we're resetting the dates for this version. + # Check to see if the dates have been changed for this version. + # Note that dates are never reset (set to NULL) for a set version. 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"))) { - $setVersionRecord->$field($rh_dates->{$field}); - } else { - $setVersionRecord->$field(undef); - } + $setVersionRecord->$field($rh_dates->{$field}) + if defined $c->param("set.$setID,v$ver.$field.override") + && $c->param("set.$setID,v$ver.$field") ne ''; } $db->putSetVersion($setVersionRecord); } + # Reset the date inputs to the date in the database if they were empty. + for my $field (@{ DATE_FIELDS_ORDER() }) { + $c->param("set.$setID,v$ver.$field", $setVersionRecord->$field) + unless $c->param("set.$setID,v$ver.$field"); + } } elsif ($action eq 'delete') { # Delete this version. $db->deleteSetVersion($editForUserID, $setID, $ver); @@ -168,25 +176,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; @@ -205,22 +215,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 }; } diff --git a/templates/ContentGenerator/Instructor/UserDetail/set_date_table.html.ep b/templates/ContentGenerator/Instructor/UserDetail/set_date_table.html.ep index eb36e11f85..3a69c40412 100644 --- a/templates/ContentGenerator/Instructor/UserDetail/set_date_table.html.ep +++ b/templates/ContentGenerator/Instructor/UserDetail/set_date_table.html.ep @@ -10,7 +10,9 @@
- <%= maketext("User overrides") =%> + % if (defined $userRecord) { + <%= maketext("User overrides") =%> + % } <%= maketext("Set values") =%>