Fixes and improvements for the new "Make Archive" page.

Fix the action. There needs to be a hidden "confirm" input. Improve layouts. Also other clean up suggested in my review of openwebwork#2163. Don't use `chdir`. The IO::Compression::Zip module is too limited. It can not add directories to the zip file. Empty directories in particular should be. So use Archive::Zip instead. This module is already in use by webwork as well.
drgrice1 · Aug 16, 2023 · cdba313 · cdba313
1 parent 925037a
commit cdba313
Show file tree

Hide file tree

Showing 4 changed files with 77 additions and 51 deletions.
diff --git a/htdocs/js/FileManager/filemanager.js b/htdocs/js/FileManager/filemanager.js
@@ -43,19 +43,17 @@
 	};
 
 	// Used for the archive subpage to highlight all in the Select
-	const selectAllButton = document.getElementById('select-all-files-button');
-	selectAllButton?.addEventListener('click', () => {
-		const n = document.getElementById('archive-files').options.length;
-		for (const opt of document.getElementById('archive-files').options) {
-			opt.selected = 'selected';
+	document.getElementById('select-all-files-button')?.addEventListener('click', () => {
+		for (const option of document.getElementById('archive-files').options) {
+			option.selected = 'selected';
 		}
 	});
 
-
-	for (const r of document.querySelectorAll('input[name="archive_type"]')) {
-		r.addEventListener('click', () => {
-			const suffix = document.querySelector('input[name="archive_type"]:checked').value;
-			document.getElementById('filename_suffix').innerText = '.' + suffix;
+	for (const archiveTypeInput of document.querySelectorAll('input[name="archive_type"]')) {
+		archiveTypeInput.addEventListener('click', () => {
+			document.getElementById('filename_suffix').innerText = `.${
+				document.querySelector('input[name="archive_type"]:checked').value
+			}`;
 		});
 	}
 

diff --git a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm
@@ -27,8 +27,8 @@ use File::Copy;
 use File::Spec;
 use String::ShellQuote;
 use Archive::Extract;
-use IO::Compress::Zip qw(zip $ZipError);
 use Archive::Tar;
+use Archive::Zip qw(:ERROR_CODES);
 
 use WeBWorK::Utils qw(readDirectory readFile sortByName listFilesRecursive);
 use WeBWorK::Upload;
@@ -356,33 +356,59 @@ sub MakeArchive ($c) {
 	}
 
 	my $dir = "$c->{courseRoot}/$c->{pwd}";
+
 	if ($c->param('confirmed')) {
-		chdir($dir);
-		# remove any directories
-		my @files_to_compress = grep { -f $_ } @files;
+		my $action = $c->param('action')          || 'Cancel';
+		return $c->Refresh if $action eq 'Cancel' || $action eq $c->maketext('Cancel');
 
-		unless ($c->param('archive_filename') && scalar(@files_to_compress) > 0) {
-			$c->addbadmessage($c->maketext('The filename cannot be empty.'))      unless $c->param('archive_filename');
-			$c->addbadmessage($c->maketext('At least one file must be selected')) unless scalar(@files_to_compress) > 0;
+		unless ($c->param('archive_filename')) {
+			$c->addbadmessage($c->maketext('The filename cannot be empty.'));
+			return $c->include('ContentGenerator/Instructor/FileManager/archive', dir => $dir, files => \@files);
+		}
+
+		unless (@files > 0) {
+			$c->addbadmessage($c->maketext('At least one file must be selected'));
 			return $c->include('ContentGenerator/Instructor/FileManager/archive', dir => $dir, files => \@files);
 		}
 
 		my $archive = $c->param('archive_filename');
 		my ($error, $ok);
 		if ($c->param('archive_type') eq 'zip') {
 			$archive .= '.zip';
-			$ok    = zip \@files_to_compress => $archive;
-			$error = $ZipError unless $ok;
+			my $zip = Archive::Zip->new();
+			$zip->storeSymbolicLink(1);
+			for (@files) {
+				my $fullFile = "$dir/$_";
+
+				# Skip symbolic links for now. As of yet, I have not found a perl module that can add symbolic links to
+				# zip files correctly.  Archive::Zip should be able to do this, but has permissions issues doing so.
+				next if -l $fullFile;
+
+				if (-d $fullFile) {
+					$zip->addDirectory($fullFile => $_);
+				} else {
+					$zip->addFile($fullFile => $_);
+				}
+			}
+			$ok = $zip->writeToFileNamed("$dir/$archive") == AZ_OK;
+			# FIXME: This should check the error code, and give a more specific error message.
+			$error = 'Unable to create zip archive.' unless $ok;
 		} else {
 			$archive .= '.tgz';
-			$ok    = Archive::Tar->create_archive($archive, COMPRESS_GZIP, @files_to_compress);
-			$error = $Archive::Tar::error unless $ok;
+			my $tar = Archive::Tar->new;
+			$tar->add_files(map {"$dir/$_"} @files);
+			# Make file names in the archive relative to the current working directory.
+			for ($tar->get_files) {
+				$tar->rename($_->full_path, $_->full_path =~ s!^$dir/!!r);
+			}
+			$ok    = $tar->write("$dir/$archive", COMPRESS_GZIP);
+			$error = $tar->error unless $ok;
 		}
 		if ($ok) {
-			my $n = scalar(@files);
-			$c->addgoodmessage($c->maketext('Archive "[_1]" created successfully ([quant,_2,file])', $archive, $n));
+			$c->addgoodmessage(
+				$c->maketext('Archive "[_1]" created successfully ([quant,_2,file])', $archive, scalar(@files)));
 		} else {
-			$c->addbadmessage($c->maketext(q{Can't create archive "[_1]": command returned [_2]}, $archive, $error));
+			$c->addbadmessage($c->maketext(q{Can't create archive "[_1]": [_2]}, $archive, $error));
 		}
 		return $c->Refresh;
 	} else {

diff --git a/templates/ContentGenerator/Instructor/FileManager/archive.html.ep b/templates/ContentGenerator/Instructor/FileManager/archive.html.ep
@@ -1,12 +1,13 @@
-% # template for the archive subpage.
-<div class="card w-75 mx-auto">
+% use Mojo::File qw(path);
+%
+<div class="card mx-auto" style="max-width:700px">
 	<div class="card-body">
 		<div class="mb-3">
 			<b><%= maketext('The following files have been selected for archiving.  Select the type '
-				. 'of archive and any subset of the requested files.') =%> </b>
+				. 'of archive and any subset of the requested files.') =%></b>
 		</div>
 		<div class="row">
-		  <div class="col-4">
+			<div class="col-12">
 				<div class="input-group input-group-sm mb-2">
 					<span class="input-group-text"><%= maketext('Archive Filename') %>:</span>
 					<%= text_field archive_filename => '',
@@ -20,43 +21,44 @@
 			<div class="input-group input-group-sm mb-2">
 				<span class="input-group-text"><%= maketext('Archive Type') %>:</span>
 				<div class="input-group-text flex-grow-1">
-					<label class="form-check-label me-4">
+					<label class="form-check-label me-2">
 						<%= radio_button archive_type => 'zip', checked => undef, class => 'form-check-input mx-1' =%>
 						<%= maketext('Zip File') =%>
 					</label>
-					<label class="form-check-label me-4">
+					<label class="form-check-label">
 						<%= radio_button archive_type => 'tgz', class => 'form-check-input mx-1' =%>
 						<%= maketext('Compressed Tar') =%>
 					</label>
-					<button type="button" class="btn btn-sm btn-secondary" id="select-all-files-button">
-						<%= maketext('Select All Files') =%>
-					</button>
 				</div>
 			</div>
 		</div>
-
-
+		<div class="row mb-2">
+			<div class="col-12">
+				<button type="button" class="btn btn-sm btn-secondary" id="select-all-files-button">
+					<%= maketext('Select All Files') =%>
+				</button>
+			</div>
+		</div>
+		%
 		% my @files_to_compress;
-		% chdir($dir);
-		% for my $f (@$files) {
-		% 	push(@files_to_compress, glob("$f/**")) if -d $f;
-		% 	push(@files_to_compress, $f)            if -f $f;
+		% for my $file (@$files) {
+			% push(@files_to_compress, $file);
+			% my $path = path("$dir/$file");
+			% push(@files_to_compress, @{ $path->list_tree({ hidden => 1 })->map('to_rel', $dir) })
+				% if (-d $path && !-l $path);
 		% }
-		% # remove any directories
-		% @files_to_compress = grep { -f $_ } @files_to_compress;
-
-		<%= select_field files => \@files_to_compress,
-					id    => 'archive-files',
-					class => 'form-select',
-					size => 20,
-					multiple => undef
-				=%>
-
+		%
+		% # Select all files initially. Even those that are in previously selected directories or subdirectories.
+		% param('files', \@files_to_compress);
+		<%= select_field files => \@files_to_compress, id => 'archive-files', class => 'form-select mb-2',
+			size => 20, multiple => undef =%>
+		%
 		<p><%= maketext('Create the archive of the select files?') %></p>
 		<div class="d-flex justify-content-evenly">
 			<%= submit_button maketext('Cancel'), name => 'action', class => 'btn btn-sm btn-secondary' =%>
 			<%= submit_button maketext('Make Archive'), name => 'action', class => 'btn btn-sm btn-primary' =%>
 		</div>
 	</div>
 </div>
+<%= hidden_field confirmed => 'MakeArchive' =%>
 <%= $c->HiddenFlags =%>
diff --git a/templates/ContentGenerator/Instructor/FileManager/refresh.html.ep b/templates/ContentGenerator/Instructor/FileManager/refresh.html.ep
@@ -34,7 +34,7 @@
 			dir => 'ltr', size => 17, multiple => undef =%>
 	</div>
 	<div class="col-md-4 mb-2">
-		<div class="d-md-flex flex-column justify-content-evenly">
+		<div class="d-flex flex-md-column flex-wrap flex-md-nowrap justify-content-evenly gap-1 gap-md-0">
 			<%= submit_button maketext('View'), id => 'View', %button =%>
 			<%= submit_button maketext('Edit'), id => 'Edit', %button =%>
 			<%= submit_button maketext('Download'), id => 'Download', %button =%>
@@ -50,7 +50,7 @@
 			% unless ($c->{courseName} eq 'admin') {
 				<%= submit_button maketext('Archive Course'), id => 'ArchiveCourse', %button =%>
 			% }
-			<div style="height: 10px"></div>
+			<div class="d-none d-md-block" style="height: 10px"></div>
 			<%= submit_button maketext('New File'), id => 'NewFile', %button =%>
 			<%= submit_button maketext('New Folder'), id => 'NewFolder', %button =%>
 			<%= submit_button maketext('Refresh'), id => 'Refresh', %button =%>