Remove the course_id from the past_answer table.

It is time for this to go. It is an entirely unnecessary column. This is taken care of by the usual course upgrade procedure in the admin course. Don't test this on a course that you want to go back to other branches with! It isn't so easy to add the course_id column back. It can be done though if needed.
drgrice1 · Sep 19, 2023 · ff45eb8 · ff45eb8
1 parent e7fb7c4
commit ff45eb8
Show file tree

Hide file tree

Showing 16 changed files with 182 additions and 103 deletions.
diff --git a/bin/dump_past_answers b/bin/dump_past_answers
@@ -239,7 +239,7 @@ foreach my $courseID (@courses) {
 					$row[24] = defined($tags->{keywords}) ? join(',', @{ $tags->{keywords} }) : '';
 				}
 
-				my @answerIDs = $db->listProblemPastAnswers($courseID, $userID, $setID, $problemID);
+				my @answerIDs = $db->listProblemPastAnswers($userID, $setID, $problemID);
 				my @answers   = $db->getPastAnswers(\@answerIDs);
 
 				# go through attempts

diff --git a/lib/Caliper/Entity.pm b/lib/Caliper/Entity.pm
@@ -285,8 +285,7 @@ sub answer {
 	my $resource_iri = Caliper::ResourseIri->new($ce);
 
 	my $last_answer_id =
-		$db->latestProblemPastAnswer($ce->{"courseName"}, $user_id, ($version_id ? "$set_id,v$version_id" : $set_id),
-			$problem_id);
+		$db->latestProblemPastAnswer($user_id, ($version_id ? "$set_id,v$version_id" : $set_id), $problem_id);
 	my $last_answer = $db->getPastAnswer($last_answer_id);
 	my @answers     = split(/\t/, $last_answer->answer_string());
 
@@ -322,14 +321,10 @@ sub answer_attempt {
 		? $db->getMergedProblemVersion($user_id, $set_id, $version_id, $problem_id)
 		: $db->getMergedProblem($user_id, $set_id, $problem_id);
 	my $last_answer_id =
-		$db->latestProblemPastAnswer($ce->{"courseName"}, $user_id, ($version_id ? "$set_id,v$version_id" : $set_id),
-			$problem_id);
+		$db->latestProblemPastAnswer($user_id, ($version_id ? "$set_id,v$version_id" : $set_id), $problem_id);
 	my $last_answer = $db->getPastAnswer($last_answer_id);
-	my $attempt =
-		$version_id
-		? $version_id
-		: scalar $db->listProblemPastAnswers($ce->{"courseName"}, $user_id, $set_id, $problem_id);
-	my $score = $problem_user->status || 0;
+	my $attempt     = $version_id ? $version_id : scalar $db->listProblemPastAnswers($user_id, $set_id, $problem_id);
+	my $score       = $problem_user->status || 0;
 	$score = 0 if ($score > 1 || $score < 0);
 
 	my $answer_attempt = {
@@ -369,7 +364,7 @@ sub problem_set_attempt {
 	} else {
 		my @problem_ids = $db->listGlobalProblems($set_id);
 		for my $problem_id (@problem_ids) {
-			$attempt += scalar $db->listProblemPastAnswers($ce->{"courseName"}, $user_id, $set_id, $problem_id);
+			$attempt += scalar $db->listProblemPastAnswers($user_id, $set_id, $problem_id);
 		}
 	}
 

diff --git a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm
@@ -1354,7 +1354,7 @@ sub upgrade_course_confirm ($c) {
 
 		# Report on database status
 		my ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($upgrade_courseID);
-		my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $db_report) =
+		my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, $db_report) =
 			$c->formatReportOnDatabaseTables($dbStatus, $upgrade_courseID);
 
 		my $course_output = $c->c;
@@ -1413,6 +1413,21 @@ sub upgrade_course_confirm ($c) {
 			);
 		}
 
+		if ($rebuild_table_indexes) {
+			push(
+				@$course_output,
+				$c->tag(
+					'p',
+					class => 'text-danger fw-bold',
+					$c->maketext(
+						'There are extra database fields which are not defined in the schema and were part of the key '
+							. 'for at least one table. These fields must be deleted and the table indexes rebuilt. '
+							. 'Warning: This will destroy all data contained in the field and is not undoable!'
+					)
+				)
+			);
+		}
+
 		# Report on directory status
 		my ($directories_ok, $directory_report) = $CIchecker->checkCourseDirectories;
 		push(@$course_output, $c->tag('div', class => 'mb-2', $c->maketext('Directory structure:')));
@@ -1496,7 +1511,7 @@ sub do_upgrade_course ($c) {
 		# Analyze database status and prepare status report
 		($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($upgrade_courseID);
 
-		my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $db_report) =
+		my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, $db_report) =
 			$c->formatReportOnDatabaseTables($dbStatus);
 
 		# Prepend course name
@@ -2299,6 +2314,7 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) {
 	my $all_tables_ok         = 1;
 	my $extra_database_tables = 0;
 	my $extra_database_fields = 0;
+	my $rebuild_table_indexes = 0;
 
 	my $db_report = $c->c;
 
@@ -2334,25 +2350,35 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) {
 				my $field_report = $c->c("$key: $field_status_message{$field_status}");
 
 				if ($field_status == WeBWorK::Utils::CourseIntegrityCheck::ONLY_IN_B) {
-					$extra_database_fields = 1;
-					push(
-						@$field_report,
-						$c->tag(
-							'span',
-							class => 'form-check d-inline-block',
-							$c->tag(
-								'label',
-								class => 'form-check-label',
-								$c->c(
-									$c->check_box(
-										"$courseID.$table.delete_fieldIDs" => $key,
-										class                              => 'form-check-input'
-									),
-									$c->maketext('Delete field when upgrading')
-								)->join('')
-							)
-						)
-					) if defined $courseID;
+					if ($fieldInfo{$key}[1]) {
+						$rebuild_table_indexes = 1;
+					} else {
+						$extra_database_fields = 1;
+					}
+					if (defined $courseID) {
+						if ($fieldInfo{$key}[1]) {
+							push(@$field_report, $c->hidden_field("$courseID.$table.delete_fieldIDs" => $key));
+						} else {
+							push(
+								@$field_report,
+								$c->tag(
+									'span',
+									class => 'form-check d-inline-block',
+									$c->tag(
+										'label',
+										class => 'form-check-label',
+										$c->c(
+											$c->check_box(
+												"$courseID.$table.delete_fieldIDs" => $key,
+												class                              => 'form-check-input'
+											),
+											$c->maketext('Delete field when upgrading')
+										)->join('')
+									)
+								)
+							);
+						}
+					}
 				} elsif ($field_status == WeBWorK::Utils::CourseIntegrityCheck::ONLY_IN_A) {
 					$all_tables_ok = 0;
 				}
@@ -2367,7 +2393,10 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) {
 
 	push(@$db_report, $c->tag('p', class => 'text-success', $c->maketext('Database tables are ok'))) if $all_tables_ok;
 
-	return ($all_tables_ok, $extra_database_tables, $extra_database_fields, $db_report->join(''));
+	return (
+		$all_tables_ok,         $extra_database_tables, $extra_database_fields,
+		$rebuild_table_indexes, $db_report->join('')
+	);
 }
 
 1;
diff --git a/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm b/lib/WeBWorK/ContentGenerator/GatewayQuiz.pm
@@ -279,12 +279,10 @@ sub attemptResults ($c, $pg, $showCorrectAnswers, $showAttemptResults, $showSumm
 sub get_instructor_comment ($c, $problem) {
 	return unless ref($problem) =~ /ProblemVersion/;
 
-	my $db               = $c->db;
-	my $userPastAnswerID = $db->latestProblemPastAnswer(
-		$c->ce->{courseName},
-		$problem->user_id, $problem->set_id . ',v' . $problem->version_id,
-		$problem->problem_id
-	);
+	my $db = $c->db;
+	my $userPastAnswerID =
+		$db->latestProblemPastAnswer($problem->user_id, $problem->set_id . ',v' . $problem->version_id,
+			$problem->problem_id);
 
 	if ($userPastAnswerID) {
 		my $userPastAnswer = $db->getPastAnswer($userPastAnswerID);
@@ -1113,7 +1111,6 @@ async sub pre_header_initialize ($c) {
 
 				# Add to PastAnswer db
 				my $pastAnswer = $db->newPastAnswer();
-				$pastAnswer->course_id($c->stash('courseID'));
 				$pastAnswer->user_id($problem->user_id);
 				$pastAnswer->set_id($setVName);
 				$pastAnswer->problem_id($problem->problem_id);

diff --git a/lib/WeBWorK/ContentGenerator/Hardcopy.pm b/lib/WeBWorK/ContentGenerator/Hardcopy.pm
@@ -1347,8 +1347,8 @@ async sub write_problem_tex ($c, $FH, $TargetUser, $MergedSet, $themeTree, $prob
 	}
 
 	if ($showComments) {
-		my $userPastAnswerID = $db->latestProblemPastAnswer($c->stash('courseID'),
-			$MergedProblem->user_id, $versionName, $MergedProblem->problem_id);
+		my $userPastAnswerID =
+			$db->latestProblemPastAnswer($MergedProblem->user_id, $versionName, $MergedProblem->problem_id);
 
 		my $pastAnswer = $userPastAnswerID                          ? $db->getPastAnswer($userPastAnswerID) : 0;
 		my $comment    = $pastAnswer && $pastAnswer->comment_string ? $pastAnswer->comment_string           : "";

diff --git a/lib/WeBWorK/ContentGenerator/Instructor/ProblemGrader.pm b/lib/WeBWorK/ContentGenerator/Instructor/ProblemGrader.pm
@@ -99,8 +99,7 @@ async sub initialize ($c) {
 			next unless defined $_->{problem};
 			my $versionID = ref($_->{problem}) =~ /::ProblemVersion/ ? $_->{problem}->version_id : 0;
 			my $userPastAnswerID =
-				$db->latestProblemPastAnswer($courseName, $user->user_id, $setID . ($versionID ? ",v$versionID" : ''),
-					$problemID);
+				$db->latestProblemPastAnswer($user->user_id, $setID . ($versionID ? ",v$versionID" : ''), $problemID);
 			$_->{past_answer} = $db->getPastAnswer($userPastAnswerID) if ($userPastAnswerID);
 			($_->{problemNumber}, $_->{pageNumber}) = getTestProblemPosition($db, $_->{problem}) if $versionID;
 

diff --git a/lib/WeBWorK/ContentGenerator/Problem.pm b/lib/WeBWorK/ContentGenerator/Problem.pm
@@ -1451,11 +1451,8 @@ sub output_misc ($c) {
 sub output_comments ($c) {
 	my $db = $c->db;
 
-	my $userPastAnswerID = $db->latestProblemPastAnswer(
-		$c->stash('courseID'),
-		$c->param('effectiveUser'),
-		$c->stash('setID'), $c->stash('problemID')
-	);
+	my $userPastAnswerID =
+		$db->latestProblemPastAnswer($c->param('effectiveUser'), $c->stash('setID'), $c->stash('problemID'));
 
 	# If there is a comment then display it.
 	if ($userPastAnswerID) {

diff --git a/lib/WeBWorK/DB.pm b/lib/WeBWorK/DB.pm
@@ -1102,21 +1102,12 @@ BEGIN {
 sub countProblemPastAnswers { return scalar shift->listPastAnswers(@_) }
 
 sub listProblemPastAnswers {
-	my ($self, $courseID, $userID, $setID, $problemID);
+	my ($self, $userID, $setID, $problemID);
 	$self = shift;
-	$self->checkArgs(\@_, qw/course_id? user_id set_id problem_id/);
+	$self->checkArgs(\@_, qw/user_id set_id problem_id/);
 
-	#if a courseID is not provided then just do the search without a course
-	#id.  This is ok becaus the table is course specific.
-	my $where;
-	if ($#_ == 3) {
-		($courseID, $userID, $setID, $problemID) = @_;
-		$where = [ course_id_eq_user_id_eq_set_id_eq_problem_id_eq => $courseID, $userID, $setID, $problemID ];
-
-	} else {
-		($userID, $setID, $problemID) = @_;
-		$where = [ user_id_eq_set_id_eq_problem_id_eq => $userID, $setID, $problemID ];
-	}
+	($userID, $setID, $problemID) = @_;
+	my $where = [ user_id_eq_set_id_eq_problem_id_eq => $userID, $setID, $problemID ];
 
 	my $order = ['answer_id'];
 
@@ -1128,21 +1119,12 @@ sub listProblemPastAnswers {
 }
 
 sub latestProblemPastAnswer {
-	my ($self, $courseID, $userID, $setID, $problemID);
+	my ($self, $userID, $setID, $problemID);
 	$self = shift;
-	$self->checkArgs(\@_, qw/course_id? user_id set_id problem_id/);
-
-	#if a courseID is not provided then just do the search without a course
-	#id.  This is ok becaus the table is course specific.
-	my @answerIDs;
-	if ($#_ == 3) {
-		($courseID, $userID, $setID, $problemID) = @_;
-		@answerIDs = $self->listProblemPastAnswers($courseID, $userID, $setID, $problemID);
+	$self->checkArgs(\@_, qw/user_id set_id problem_id/);
 
-	} else {
-		($userID, $setID, $problemID) = @_;
-		@answerIDs = $self->listProblemPastAnswers($userID, $setID, $problemID);
-	}
+	($userID, $setID, $problemID) = @_;
+	my @answerIDs = $self->listProblemPastAnswers($userID, $setID, $problemID);
 
 	#array should already be returned from lowest id to greatest.  Latest answer is greatest
 	return $answerIDs[$#answerIDs];
@@ -1166,11 +1148,6 @@ sub getPastAnswers {
 sub addPastAnswer {
 	my ($self, $pastAnswer) = shift->checkArgs(\@_, qw/REC:past_answer/);
 
-	#       we dont have a course table yet but when we do we should check this
-
-	#	croak "addPastAnswert: course ", $pastAnswer->course_id, " not found"
-	#		unless $self->{course}->exists($pastAnswer->course_id);
-
 	croak "addPastAnswert: user problem ", $pastAnswer->user_id, " ",
 		$pastAnswer->set_id, " ", $pastAnswer->problem_id, " not found"
 		unless $self->{problem_user}->exists($pastAnswer->user_id, $pastAnswer->set_id, $pastAnswer->problem_id);

diff --git a/lib/WeBWorK/DB/Record/PastAnswer.pm b/lib/WeBWorK/DB/Record/PastAnswer.pm
@@ -29,7 +29,6 @@ BEGIN {
 	__PACKAGE__->_fields(
 
 		answer_id      => { type => "INT AUTO_INCREMENT",    key => 1 },
-		course_id      => { type => "VARCHAR(40) NOT NULL",  key => 1 },
 		user_id        => { type => "VARCHAR(100) NOT NULL", key => 1 },
 		set_id         => { type => "VARCHAR(100) NOT NULL", key => 1 },
 		problem_id     => { type => "INT NOT NULL",          key => 1 },

diff --git a/lib/WeBWorK/DB/Schema/NewSQL.pm b/lib/WeBWorK/DB/Schema/NewSQL.pm
@@ -57,12 +57,6 @@ sub where_answer_id_eq {
 	return { answer_id => $answer_id };
 }
 
-# can be used for past_answers
-sub where_course_id_eq_user_id_eq_set_id_eq_problem_id_eq {
-	my ($self, $flags, $course_id, $user_id, $set_id, $problem_id) = @_;
-	return { course_id => $course_id, user_id => $user_id, set_id => $set_id, problem_id => $problem_id };
-}
-
 sub where_user_id_eq_set_id_eq_problem_id_eq {
 	my ($self, $flags, $user_id, $set_id, $problem_id) = @_;
 	return { user_id => $user_id, set_id => $set_id, problem_id => $problem_id };

diff --git a/lib/WeBWorK/DB/Schema/NewSQL/Std.pm b/lib/WeBWorK/DB/Schema/NewSQL/Std.pm
@@ -384,6 +384,71 @@ sub _drop_column_field_stmt {
 	my $sql_field_name = $self->sql_field_name($field_name);
 	return "Alter table `$sql_table_name` drop column `$sql_field_name` ";
 }
+
+####################################################
+# rebuild indexes for the table
+####################################################
+
+sub rebuild_indexes {
+	my ($self) = @_;
+
+	my $sql_table_name  = $self->sql_table_name;
+	my $field_data      = $self->field_data;
+	my %override_fields = reverse %{ $self->{params}{fieldOverride} };
+
+	# A key field column is going to be removed.  The schema will not have the information for this column.  So the
+	# indexes need to be obtained from the database.  Note that each element of the returned array is an array reference
+	# of the form [ Table, Non_unique, Key_name, Seq_in_index, Column_name, ... ] (the information indicated by the
+	# ellipsis is not needed here).  Only the first column in each sequence is needed.
+	my @indexes = grep { $_->[3] == 1 } @{ $self->dbh->selectall_arrayref("SHOW INDEXES FROM `$sql_table_name`") };
+
+	# The columns need to be obtained from the database to determine the types of the columns.  The information from the
+	# schema can not be trusted because it doesn't have information about the field being dropped.  Note that each
+	# element of the returned array is an array reference of the form [ Field, Type, Null, Key, Default, Extra ] and
+	# Extra contains AUTO_INCREMENT for those fields that have that attribute.
+	my $columns = $self->dbh->selectall_arrayref("SHOW COLUMNS FROM `$sql_table_name`");
+
+	# First drop all indexes for the table.
+	my @auto_increment_fields;
+	for my $index (@indexes) {
+		# If a field has the AUTO_INCREMENT attribute, then that needs to be removed before the index can be dropped.
+		my $column = (grep { $index->[4] eq $_->[0] } @$columns)[0];
+		if (defined $column && $column->[5] =~ m/AUTO_INCREMENT/i) {
+			$self->dbh->do("ALTER TABLE `$sql_table_name` MODIFY `$column->[0]` $column->[1]");
+			push @auto_increment_fields, $override_fields{ $column->[0] } // $column->[0];
+		}
+
+		$self->dbh->do("ALTER TABLE `$sql_table_name` DROP INDEX `$index->[2]`");
+	}
+
+	# Add the indices for the table according to the schema.
+	my @keyfields = $self->keyfields;
+	for my $start (0 .. $#keyfields) {
+		my @index_components;
+		my $sql_field_name = $self->sql_field_name($keyfields[$start]);
+
+		for my $component (@keyfields[ $start .. $#keyfields ]) {
+			my $sql_field_name   = $self->sql_field_name($component);
+			my $sql_field_type   = $field_data->{$component}{type};
+			my $length_specifier = $sql_field_type =~ /(text|blob)/i ? '(100)' : '';
+			push @index_components, "`$sql_field_name`$length_specifier";
+		}
+
+		my $index_string = join(', ', @index_components);
+		my $index_type   = $start == 0 ? 'UNIQUE KEY' : 'KEY';
+
+		$self->dbh->do("ALTER TABLE `$sql_table_name` ADD $index_type ($index_string)");
+	}
+
+	# Finally add the AUTO_INCREMENT attribute back to those columns that is was removed from.
+	for my $field (@auto_increment_fields) {
+		my $sql_field_name = $self->sql_field_name($field);
+		$self->dbh->do("ALTER TABLE `$sql_table_name` MODIFY `$sql_field_name` $field_data->{$field}{type}");
+	}
+
+	return 1;
+}
+
 ####################################################
 # checking Tables
 ####################################################
@@ -885,4 +950,3 @@ sub handle_error {
 sub DESTROY {
 }
 1;
-
diff --git a/lib/WeBWorK/HTML/SingleProblemGrader.pm b/lib/WeBWorK/HTML/SingleProblemGrader.pm
@@ -44,7 +44,7 @@ sub new ($class, $c, $pg, $userProblem) {
 
 	# Retrieve the latest past answer and comment (if any).
 	my $userPastAnswerID =
-		$db->latestProblemPastAnswer($courseID, $studentID, $setID . ($versionID ? ",v$versionID" : ''), $problemID);
+		$db->latestProblemPastAnswer($studentID, $setID . ($versionID ? ",v$versionID" : ''), $problemID);
 	my $pastAnswer = $userPastAnswerID ? $db->getPastAnswer($userPastAnswerID) : 0;
 	my $comment    = $pastAnswer       ? $pastAnswer->comment_string           : '';