From ba7de1d0fb7dcc45bf29b27e646cc12a714bf969 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Sat, 30 Mar 2024 13:43:58 -0500 Subject: [PATCH] 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 #2361 (and does more). --- conf/defaults.config | 18 +++------ conf/localOverrides.conf.dist | 9 ++--- htdocs/js/ProblemSetList/problemsetlist.js | 26 +++++++++++- htdocs/js/SetMaker/setmaker.js | 25 ++++++++++++ .../ContentGenerator/Instructor/SetMaker.pm | 8 ++-- lib/WeBWorK/Utils/Instructor.pm | 40 ++++++++++++------- .../ProblemSetList/import_form.html.ep | 19 +++++++++ .../browse_library_panel_advanced.html.ep | 3 +- .../browse_library_panel_simple.html.ep | 3 +- .../SetMaker/browse_setdef_panel.html.ep | 24 ++++++++++- .../SetMaker/library-include-checks.html.ep | 14 +++++++ .../Instructor/SetMaker/top_row.html.ep | 6 +-- .../SetMaker/view_problems_line.html.ep | 25 ++---------- 13 files changed, 156 insertions(+), 64 deletions(-) create mode 100644 templates/ContentGenerator/Instructor/SetMaker/library-include-checks.html.ep diff --git a/conf/defaults.config b/conf/defaults.config index 08fbd048ed..ee219e01cf 100644 --- 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 index d3e2e5c529..02b3f88e39 100644 --- 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 index 4af5a7cc1d..c980d76c22 100644 --- 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 index 581c165371..a505ac4286 100644 --- 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 index d45f25b9b0..e15b2dff18 100644 --- 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 index 0c5de936c8..cb651e03af 100644 --- 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 index f0543cf973..844554d518 100644 --- a/templates/ContentGenerator/Instructor/ProblemSetList/import_form.html.ep +++ b/templates/ContentGenerator/Instructor/ProblemSetList/import_form.html.ep @@ -25,6 +25,25 @@ : () =%> + % + % 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) { +
+ % if ($listLibrarySets) { +
+ <%= 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" =%> +
+ % } + % if ($listContribSets) { +
+ <%= 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" =%> +
+ % } +
+ % }
<%= 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 index d3438c8028..bc3c24af09 100644 --- 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' =%>
<%= 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 index 7b457f45fa..2f5bcc60e4 100644 --- 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 @@
<%= 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' =%>
<%= 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 index 76b83d3c5a..435b85f846 100644 --- a/templates/ContentGenerator/Instructor/SetMaker/browse_setdef_panel.html.ep +++ b/templates/ContentGenerator/Instructor/SetMaker/browse_setdef_panel.html.ep @@ -15,10 +15,32 @@
<%= 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' =%>
+ % + % 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) { +
+ % if ($listLibrarySets) { +
+ <%= 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" =%> +
+ % } + % if ($listContribSets) { +
+ <%= 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" =%> +
+ % } +
+ % } + % <%= include 'ContentGenerator/Instructor/SetMaker/view_problems_line', internal_name => 'view_setdef_set' =%> diff --git a/templates/ContentGenerator/Instructor/SetMaker/library-include-checks.html.ep b/templates/ContentGenerator/Instructor/SetMaker/library-include-checks.html.ep new file mode 100644 index 0000000000..5c2f0e34b6 --- /dev/null +++ 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") { +
+ % 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 =%> +
+
+ <%= check_box includeContrib => 'on', id => 'includeContrib', class => 'form-check-input' =%> + <%= label_for includeContrib => maketext('Include Contrib'), class => 'form-check-label' =%> +
+% } 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 index 1761931337..904e30342b 100644 --- a/templates/ContentGenerator/Instructor/SetMaker/top_row.html.ep +++ b/templates/ContentGenerator/Instructor/SetMaker/top_row.html.ep @@ -39,9 +39,9 @@
- <%= 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 index 6a477bda0e..8436a67ec3 100644 --- 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'; -%
<%= submit_button maketext('View Problems'), name => $internal_name, class => 'btn btn-secondary btn-sm 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' =%>
- % # Option of whether to show hints and solutions + % # Options for whether to show hints and solutions.
- % if ($contrib_exists) { -
- -
-
- -
- % } else { - <%= hidden_field includeOPL => 1 =%> - % }