Rework how set definition files from OPL and Contrib are shown.

First, the `WeBWorK::Utils::Instructor::getDefList` method no longer searches in the `capaLibrary` directory (if that link exists in the course's templates directory). Those set definition files are in Contrib, and so are already listed when the set definition files in Contrib are listed. That method also now sorts first by library, then depth, and then upper case set name. The library order is first local set definitions (anything not in Library or Contrib), then the one set in Library, and then the sets in Contrib. The method also does not return the Library or Contrib set definitions at all unless the Library or Contrib links exist and are readable in the course's templates directory. Next, the `useOPLsetDefs` and `useContribSetDefs` options in `defaults.conf` and `localOverrides.conf.dist` have been removed, and are no longer honored. Instead there are now check boxes in the import form on the "Sets Manager" page and when viewing set definition files in the "Library Browser" that when checked include the set definition files from OPL or Contrib. The check boxes are not checked by default. These check boxes are not shown if the Library or Contrib links do not exist or are not readable. The "Include OPL" and "Include Contrib" check boxes that determine if OPL or Contrib problems are included when listing problems on the "Open Problem Library" page of the library browser were moved from the "View Problems" line below to below the buttons on the right, and are only shown when browsing OPL problems, and not for all of the other buttons (like "Course Files", "Course Sets", "Set Definitions Files", etc.) for which those check boxes do not apply. This fixes issue openwebwork#2361 (and does more).
drgrice1 · Apr 13, 2024 · cff9fd4 · cff9fd4
1 parent 7e7bff4
commit cff9fd4
Show file tree

Hide file tree

Showing 13 changed files with 156 additions and 64 deletions.
diff --git a/conf/defaults.config b/conf/defaults.config
@@ -1042,20 +1042,14 @@ $pg{additionalPGEditorDisplayModes} = [
 # Whether the homework editor pages should show options for conditional release
 $options{enableConditionalRelease} = 0;
 
-# In the set manager, how deep to search within templates for .def files.
-# Note that this does not apply to the Library and Contrib directories.
-# Those directories are not searched in any case (see below).
-# 0 means only within templates.
+##########################################################################################
+# Searching for set.def files to import
+##########################################################################################
+# In the set manager and the library browser, the directory depth to search
+# within templates for .def files.  Note that this does not apply to the Library
+# and Contrib directories.  Those directories are not searched in any case.
 $options{setDefSearchDepth} = 4;
 
-# In the set manager, also list OPL or Contrib set defintion files.   Note
-# that the directories are not searched, but these lists are loaded from the
-# files htdocs/DATA/library-set-defs.json and htdocs/DATA/contrib-set-defs.json
-# which are generated by running bin/generate-OPL-set-def-lists.pl (which is
-# also run if you run bin/OPL-update).
-$options{useOPLdefFiles} = 1;
-$options{useContribDefFiles} = 1;
-
 ##########################################################################################
 #### Default settings for the problem editor pages
 ##########################################################################################

diff --git a/conf/localOverrides.conf.dist b/conf/localOverrides.conf.dist
@@ -646,12 +646,11 @@ $mail{feedbackRecipients}    = [
 ################################################################################
 # Searching for set.def files to import
 ################################################################################
-## Uncomment below so that when the Library Browser searches for set def
-## files, it searches beyond templates; it can search deeper subfolders of
-## templates, and optionally also descend into Library
+# In the set manager and the library browser, the directory depth to search
+# within templates for .def files.  Note that this does not apply to the Library
+# and Contrib directories.  Those directories are not searched in any case.
 
-#$options{setDefSearchDepth}=4; #search down 4 levels
-#$options{useOPLdefFiles}=1;
+#$options{setDefSearchDepth} = 4;
 
 ################################################################################
 # Permission overrides (e.g. "admin", "professor", "ta", "student", "guest"

diff --git a/htdocs/js/ProblemSetList/problemsetlist.js b/htdocs/js/ProblemSetList/problemsetlist.js
@@ -235,7 +235,7 @@
 					}
 				})
 			],
-			onReady(selectedDates) {
+			onReady() {
 				// Flatpickr hides the original input and adds the alternate input after it.  That messes up the
 				// bootstrap input group styling.  So move the now hidden original input after the created alternate
 				// input to fix that.
@@ -277,4 +277,28 @@
 			}
 		});
 	}
+
+	const importSourceSelect = document.getElementById('import_source_select');
+	const listOPLSetsCheck = document.getElementById('list_opl_sets');
+	const listContribSetsCheck = document.getElementById('list_contrib_sets');
+
+	if (importSourceSelect && (listOPLSetsCheck || listContribSetsCheck)) {
+		const allOptions = Array.from(importSourceSelect.options);
+
+		const updateAvailableOptions = () => {
+			for (const option of Array.from(importSourceSelect.options)) option.value && option.remove();
+			for (const option of allOptions) {
+				if (option.value.startsWith('Library/')) {
+					if (listOPLSetsCheck.checked) importSourceSelect.add(option);
+				} else if (option.value.startsWith('Contrib/')) {
+					if (listContribSetsCheck.checked) importSourceSelect.add(option);
+				} else importSourceSelect.add(option);
+			}
+		};
+
+		listOPLSetsCheck?.addEventListener('change', updateAvailableOptions);
+		listContribSetsCheck?.addEventListener('change', updateAvailableOptions);
+
+		updateAvailableOptions();
+	}
 })();
diff --git a/htdocs/js/SetMaker/setmaker.js b/htdocs/js/SetMaker/setmaker.js
@@ -509,4 +509,29 @@
 	document
 		.querySelectorAll('.lb-problem-add [data-bs-toggle], .lb-problem-icons [data-bs-toggle=tooltip]')
 		.forEach((el) => new bootstrap.Tooltip(el, { fallbackPlacements: [] }));
+
+	// If this is the browse setdef panel, then set up the check boxes for including OPL or Contrib sets.
+	const setDefSelect = document.getElementsByName('library_sets')[0];
+	const listOPLSetsCheck = document.getElementById('list_opl_sets');
+	const listContribSetsCheck = document.getElementById('list_contrib_sets');
+
+	if (setDefSelect && (listOPLSetsCheck || listContribSetsCheck)) {
+		const allOptions = Array.from(setDefSelect.options);
+
+		const updateAvailableOptions = () => {
+			for (const option of Array.from(setDefSelect.options)) option.value && option.remove();
+			for (const option of allOptions) {
+				if (option.value.startsWith('Library/')) {
+					if (listOPLSetsCheck.checked) setDefSelect.add(option);
+				} else if (option.value.startsWith('Contrib/')) {
+					if (listContribSetsCheck.checked) setDefSelect.add(option);
+				} else setDefSelect.add(option);
+			}
+		};
+
+		listOPLSetsCheck?.addEventListener('change', updateAvailableOptions);
+		listContribSetsCheck?.addEventListener('change', updateAvailableOptions);
+
+		updateAvailableOptions();
+	}
 })();
diff --git a/lib/WeBWorK/ContentGenerator/Instructor/SetMaker.pm b/lib/WeBWorK/ContentGenerator/Instructor/SetMaker.pm
@@ -471,7 +471,7 @@ sub pre_header_initialize ($c) {
 	$count++ if ($j > 0);
 
 	# Default of which problem selector to display
-	my $browse_which = $c->param('browse_which') || 'browse_npl_library';
+	my $browse_which = $c->param('browse_which') || 'browse_opl';
 
 	# Check for problem lib buttons
 	my $browse_lib = '';
@@ -484,7 +484,7 @@ sub pre_header_initialize ($c) {
 
 	# Start the logic through if elsif elsif ...
 	debug("browse_lib",         $c->param("$browse_lib"));
-	debug("browse_npl_library", $c->param("browse_npl_library"));
+	debug("browse_opl",         $c->param("browse_opl"));
 	debug("browse_course_sets", $c->param("browse_course_sets"));
 	debug("browse_setdefs",     $c->param("browse_setdefs"));
 	# Asked to browse certain problems
@@ -493,8 +493,8 @@ sub pre_header_initialize ($c) {
 		$c->{current_library_set} = "";
 		$use_previous_problems    = 0;
 		@pg_files                 = ();
-	} elsif ($c->param('browse_npl_library')) {
-		$browse_which             = 'browse_npl_library';
+	} elsif ($c->param('browse_opl')) {
+		$browse_which             = 'browse_opl';
 		$c->{current_library_set} = "";
 		$use_previous_problems    = 0;
 		@pg_files                 = ();

diff --git a/lib/WeBWorK/Utils/Instructor.pm b/lib/WeBWorK/Utils/Instructor.pm
@@ -671,33 +671,45 @@ sub getDefList {
 
 	my @found_set_defs;
 
-	# get_set_defs_wanted is a closure over @found_set_defs
-	my $get_set_defs_wanted = sub {
-		if ($File::Find::dir =~ /^$topdir\/Library/ || $File::Find::dir =~ /^$topdir\/Contrib/) {
-			$File::Find::prune = 1;
-			return;
-		}
-		if (@{ [ $File::Find::dir =~ /\//g ] } > $max_depth) { $File::Find::prune = 1; return; }
-		push @found_set_defs, $_ =~ s|^$topdir/?||r if m|/set[^/]*\.def$|;
-	};
-
-	find({ wanted => $get_set_defs_wanted, follow_fast => 1, no_chdir => 1, follow_skip => 2 }, $topdir);
+	find(
+		{
+			wanted => sub {
+				if ($File::Find::dir =~ /^$topdir\/Library/
+					|| $File::Find::dir =~ /^$topdir\/Contrib/
+					|| $File::Find::dir =~ /^$topdir\/capaLibrary/)
+				{
+					$File::Find::prune = 1;
+					return;
+				}
+				if (@{ [ $File::Find::dir =~ /\//g ] } > $max_depth) { $File::Find::prune = 1; return; }
+				push @found_set_defs, $_ =~ s|^$topdir/?||r if m|/set[^/]*\.def$|;
+			},
+			follow_fast => 1,
+			no_chdir    => 1,
+			follow_skip => 2
+		},
+		$topdir
+	);
 
 	# Load the OPL set definition files from the list file.
 	push(@found_set_defs, loadSetDefListFile("$ce->{webworkDirs}{htdocs}/DATA/library-set-defs.json"))
-		if ($ce->{options}{useOPLdefFiles});
+		if -d "$ce->{courseDirs}{templates}/Library" && -r "$ce->{courseDirs}{templates}/Library";
 
 	# Load the Contrib set definition files from the list file.
 	push(@found_set_defs, loadSetDefListFile("$ce->{webworkDirs}{htdocs}/DATA/contrib-set-defs.json"))
-		if ($ce->{options}{useContribDefFiles});
+		if -d "$ce->{courseDirs}{templates}/Contrib" && -r "$ce->{courseDirs}{templates}/Contrib";
 
+	my @lib_order;
 	my @depths;
 	my @caps;
 	for (@found_set_defs) {
+		push(@lib_order, $_ =~ m|^Library/| ? 2 : $_ =~ m|^Contrib/| ? 3 : 1);
 		push @depths, scalar(@{ [ $_ =~ /\//g ] });
 		push @caps,   uc($_);
 	}
-	return @found_set_defs[ sort { $depths[$a] <=> $depths[$b] || $caps[$a] cmp $caps[$b] } 0 .. $#found_set_defs ];
+	return @found_set_defs[
+		sort { $lib_order[$a] <=> $lib_order[$b] || $depths[$a] <=> $depths[$b] || $caps[$a] cmp $caps[$b] }
+		0 .. $#found_set_defs ];
 }
 
 =back

diff --git a/templates/ContentGenerator/Instructor/ProblemSetList/import_form.html.ep b/templates/ContentGenerator/Instructor/ProblemSetList/import_form.html.ep
@@ -25,6 +25,25 @@
 				: () =%>
 		</div>
 	</div>
+	%
+	% my $listLibrarySets = -d "$ce->{courseDirs}{templates}/Library" && -r "$ce->{courseDirs}{templates}/Library";
+	% my $listContribSets = -d "$ce->{courseDirs}{templates}/Contrib" && -r "$ce->{courseDirs}{templates}/Contrib";
+	% if ($listLibrarySets || $listContribSets) {
+		<div class="mb-2">
+			% if ($listLibrarySets) {
+				<div class="form-check form-check-inline">
+					<%= check_box list_opl_sets => 0, id => 'list_opl_sets', class => 'form-check-input' =%>
+					<%= label_for list_opl_sets => maketext('List OPL Sets'), class => "form-check-label" =%>
+				</div>
+			% }
+			% if ($listContribSets) {
+				<div class="form-check form-check-inline">
+					<%= check_box list_contrib_sets => 0, id => 'list_contrib_sets', class => 'form-check-input' =%>
+					<%= label_for list_contrib_sets => maketext('List Contrib Sets'), class => "form-check-label" =%>
+				</div>
+			% }
+		</div>
+	% }
 	<div class="row mb-2">
 		<%= label_for import_text => maketext('Import sets with names') . ':',
 			class => 'col-form-label col-form-label-sm col-md-auto' =%>

diff --git a/templates/ContentGenerator/Instructor/SetMaker/browse_library_panel_advanced.html.ep b/templates/ContentGenerator/Instructor/SetMaker/browse_library_panel_advanced.html.ep
@@ -104,7 +104,8 @@
 			<%= submit_button maketext('Reset'), name => 'library_reset',
 				class => 'btn btn-secondary btn-sm mb-1 ms-sm-0 ms-2 library-panel-btn' =%>
 			<%= submit_button maketext('Basic Search'), name => 'library_basic',
-				class => 'btn btn-secondary btn-sm mb-1 ms-sm-0 ms-2 library-panel-btn' =%>
+				class => 'btn btn-secondary btn-sm mb-2 ms-sm-0 ms-2 library-panel-btn' =%>
+			<%= include 'ContentGenerator/Instructor/SetMaker/library-include-checks' =%>
 		</div>
 	</div>
 	<%= include 'ContentGenerator/Instructor/SetMaker/view_problems_line', internal_name => 'lib_view' =%>

diff --git a/templates/ContentGenerator/Instructor/SetMaker/browse_library_panel_simple.html.ep b/templates/ContentGenerator/Instructor/SetMaker/browse_library_panel_simple.html.ep
@@ -40,7 +40,8 @@
 		</div>
 		<div class="col-md-3 col-sm-4 mb-1 d-flex flex-sm-column justify-content-sm-start justify-content-center">
 			<%= submit_button maketext('Advanced Search'), name => 'library_advanced',
-				class => 'btn btn-secondary btn-sm library-panel-btn' =%>
+				class => 'btn btn-secondary btn-sm library-panel-btn mb-2' =%>
+			<%= include 'ContentGenerator/Instructor/SetMaker/library-include-checks' =%>
 		</div>
 	</div>
 	<%= include 'ContentGenerator/Instructor/SetMaker/view_problems_line', internal_name => 'lib_view' =%>

diff --git a/templates/ContentGenerator/Instructor/SetMaker/browse_setdef_panel.html.ep b/templates/ContentGenerator/Instructor/SetMaker/browse_setdef_panel.html.ep
@@ -15,10 +15,32 @@
 	<div class="mb-2">
 		<label class="col-form-label-sm"><%= maketext('Browse from:') %></label>
 		<%= select_field library_sets => [
-				$selected_library eq '' ? [ maketext('Select a Set Definition File') => '', selected => undef, disabled => 'disabled' ] : (),
+				$selected_library eq '' ?
+					[ maketext('Select a Set Definition File') => '', selected => undef, disabled => 'disabled' ]
+					: (),
 				@list_of_set_defs
 			],
 			class => 'form-select form-select-sm d-inline w-auto' =%>
 	</div>
+	%
+	% my $listLibrarySets = -d "$ce->{courseDirs}{templates}/Library" && -r "$ce->{courseDirs}{templates}/Library";
+	% my $listContribSets = -d "$ce->{courseDirs}{templates}/Contrib" && -r "$ce->{courseDirs}{templates}/Contrib";
+	% if ($listLibrarySets || $listContribSets) {
+		<div class="mb-2 font-sm">
+			% if ($listLibrarySets) {
+				<div class="form-check form-check-inline">
+					<%= check_box list_opl_sets => 0, id => 'list_opl_sets', class => 'form-check-input' =%>
+					<%= label_for list_opl_sets => maketext('List OPL Sets'), class => "form-check-label" =%>
+				</div>
+			% }
+			% if ($listContribSets) {
+				<div class="form-check form-check-inline">
+					<%= check_box list_contrib_sets => 0, id => 'list_contrib_sets', class => 'form-check-input' =%>
+					<%= label_for list_contrib_sets => maketext('List Contrib Sets'), class => "form-check-label" =%>
+				</div>
+			% }
+		</div>
+	% }
+	%
 	<%= include 'ContentGenerator/Instructor/SetMaker/view_problems_line', internal_name => 'view_setdef_set' =%>
 </div>
diff --git a/templates/ContentGenerator/Instructor/SetMaker/library-include-checks.html.ep b/templates/ContentGenerator/Instructor/SetMaker/library-include-checks.html.ep
@@ -0,0 +1,14 @@
+% if (-d "$ce->{courseDirs}{templates}/Contrib" && -r "$ce->{courseDirs}{templates}/Contrib") {
+	<div class="form-check mb-2 font-sm">
+		% param('includeOPL', 'on') unless defined param('includeOPL');
+		<%= check_box includeOPL => 'on', id => 'includeOPL', class => 'form-check-input' =%>
+		<%= label_for includeOPL => maketext('Include OPL'), class => 'form-check-label' =%>
+		<%= hidden_field includeOPL => 0 =%>
+	</div>
+	<div class="form-check font-sm">
+		<%= check_box includeContrib => 'on', id => 'includeContrib', class => 'form-check-input' =%>
+		<%= label_for includeContrib => maketext('Include Contrib'), class => 'form-check-label' =%>
+	</div>
+% } else {
+	<%= hidden_field includeOPL => 1 =%>
+% }
diff --git a/templates/ContentGenerator/Instructor/SetMaker/top_row.html.ep b/templates/ContentGenerator/Instructor/SetMaker/top_row.html.ep
@@ -39,9 +39,9 @@
 <div class="d-flex justify-content-center">
 	<label class="col-form-label-sm mb-2 me-1"><%= maketext('Browse') %></label>
 	<div class="d-flex flex-wrap justify-content-center">
-		<%= submit_button maketext('Open Problem Library'), name => 'browse_npl_library',
+		<%= submit_button maketext('Open Problem Library'), name => 'browse_opl',
 			class => 'browse-lib-btn btn btn-secondary btn-sm mb-2 mx-1',
-			$browse_which eq 'browse_npl_library' ? (disabled => undef) : () =%>
+			$browse_which eq 'browse_opl' ? (disabled => undef) : () =%>
 		<%= submit_button maketext('Course Files'), name => 'browse_local',
 			class => 'browse-lib-btn btn btn-secondary btn-sm mb-2 mx-1',
 			$browse_which eq 'browse_local' ? (disabled => undef) : () =%>
@@ -73,7 +73,7 @@
 % } elsif ($browse_which eq 'browse_course_sets') {
 	<%= include 'ContentGenerator/Instructor/SetMaker/browse_course_sets_panel',
 		selected_library => $c->{current_library_set} =%>
-% } elsif ($browse_which eq 'browse_npl_library') {
+% } elsif ($browse_which eq 'browse_opl') {
 	<%= include 'ContentGenerator/Instructor/SetMaker/browse_library_panel' =%>
 % } elsif ($browse_which eq 'browse_setdefs') {
 	<%= include 'ContentGenerator/Instructor/SetMaker/browse_setdef_panel',

diff --git a/templates/ContentGenerator/Instructor/SetMaker/view_problems_line.html.ep b/templates/ContentGenerator/Instructor/SetMaker/view_problems_line.html.ep
@@ -1,12 +1,11 @@
 % use WeBWorK::PG;
 %
-% my $contrib_exists = -r $ce->{courseDirs}{templates} . '/Contrib';
-%
 <div class="d-flex flex-wrap justify-content-center">
 	<%= submit_button maketext('View Problems'), name => $internal_name, class => 'btn btn-secondary btn-sm mb-2' =%>
 	%
 	<div class="d-inline-block ms-2 mb-2">
-		<%= label_for 'problem_displaymode' => maketext('Display Mode:'), class => 'col-form-label col-form-label-sm' =%>
+		<%= label_for 'problem_displaymode' => maketext('Display Mode:'),
+			class => 'col-form-label col-form-label-sm' =%>
 		<%= select_field 'problem_displaymode' => [
 				(
 					map { [ $_ => $_, $_ eq $ce->{pg}{options}{displayMode} ? (selected => undef) : () ] }
@@ -25,26 +24,8 @@
 		<%= select_field max_shown => [ 5, 10, 15, [ 20 => 20, selected => undef ], 25, 30, 50, 'All' ],
 			id => 'max_shown', class => 'form-select form-select-sm d-inline w-auto' =%>
 	</div>
-	% # Option of whether to show hints and solutions
+	% # Options for whether to show hints and solutions.
 	<div class="d-inline-block ms-2 mb-2">
-		% if ($contrib_exists) {
-			<div class="form-check form-check-inline ms-2">
-				<label class="form-check-label col-form-label-sm">
-					% param('includeOPL', 'on') unless defined param('includeOPL');
-					<%= check_box includeOPL => 'on', class => 'form-check-input me-1' =%>
-					<%= maketext('Include OPL') =%>
-					<%= hidden_field includeOPL => 0 =%>
-				</label>
-			</div>
-			<div class="form-check form-check-inline ms-2">
-				<label class="form-check-label col-form-label-sm">
-					<%= check_box includeContrib => 'on', class => 'form-check-input me-1' =%>
-					<%= maketext('Include Contrib') =%>
-				</label>
-			</div>
-		% } else {
-			<%= hidden_field includeOPL => 1 =%>
-		% }
 		<div class="form-check form-check-inline ms-2">
 			<label class="form-check-label col-form-label-sm">
 				<%= check_box showHints => 'on', class => 'form-check-input me-1' =%>