Skip to content

Commit 403aebd

Browse files
drgrice1drgrice1
committed
Improvements for the "Save As" form in the PG editor.
In general when editing any kind of problem the option to "Append to end of set" is now shown. Even for a new problem template or a sample problem. Furthermore, the option includes a select with which to choose a set for the course. When editing a problem that is in a set, the set the problem is in is selected by default. Otherwise, the default "Select a Set" option is selected. When the form is submitted and this option is selected, then validation occurs to ensure that a set has been chosen. Also, server side the parameter is validated. Note that if the server receives a request that has the radio selected but a target set is not in the parameters, then the file will be saved, but it will not be added to any set. However, this generally will not happen for those using the problem editor. It will only happen for someone that is properly authenticated and with sufficient permissions, that is hacking on parameters. Wnen editing a course info file, set header file, or hardcopy header file, do not show the "Copy auxiliary files" option. That option should only ever be shown for problem files. When editing a set or hardcopy header, don't show the options to "Replace current problem", "Append to end of set", or "Create unattached problem". Those don't make sense at all for a header file. They are not problems. Instead show options to "Set as set header for set" or "Create unattached header file". The option to "Set as set header for set" also includes a select with which to choose a set for the course that the file will be set as the header for. The set that the file is a set header for is selected by default. When a default set or hardcopy header is being edited don't include `opt/webwork/webwork2/pg` in the default save to file name that is shown. When a sample problem is being edited, use the original file name of the sample problem for the default file name to save the file to. This can still be changed by the author, but it doesn't need to be "newProblem.pg" for these. Note that the message that states that "You can change the file path for this problem manually from the Sets Manager page" when the file that is being saved already exists has been removed. That message has been there for a long time, but it doesn't really make sense to state it at this point. The user has chosen a file name, not knowing that a file by that name already exists. The intent was not to use the existing file for the problem. The intent was to save the current content as the chosen file name and use it for the problem. In addition, this message was shown for any file type, and does not apply to set headers at all. IMPORTANT NOTE: The problem editor no longer attempts to use a set version and nothing should try to open it with a set version. The only case where that was done has been removed. That is from the set detail page when editing a set version for a user. Now on that page, that only sends the set without the version. The reason this was done is because doing so in many cases results in an exception being thrown when you try to save or do many of the things in the problem editor. Even when saving doesn't result in an exception, the save doesn't go where you might think it would go. Also, don't try to use the effective user anywhere in the problem editor. That also does not do what you might think. Generally, these were things that were not really thought out. You really shouldn't be editing a problem for a particular user or set version. So the problem editor never tries to change the source file for a user problem. It always works with the global problem. Also there are a few minor changes to a couple of other tabs. These are below. Don't show the "Convert the code to PGML" and "Analyze code with PG Critic" options on the "Code Maintenance" tab when editing a set header. A set header might have antiquated methods such as BEGIN_TEXT/END_TEXT blocks, but I don't think that it is a very good idea to try the PGML conversion on these. Although the PG critic will work on these files, there are many things that the PG critic will report that don't apply at all to set headers, such as having metadata or a needing solution. Not showing "Convert the code to PGML" is perhaps debatable, but not showing "Analyze code with PG Critic" is really not debatable. On the "View/Reload" and "Generate Hardcopy" tabs do not show the options to change the seed when editing a set header. The seed doesn't really apply to set headers. I don't think a set header should ever use the random methods. Note that the primary intent of this pull request is to address issue #2992.
1 parent bc13c21 commit 403aebd

10 files changed

Lines changed: 298 additions & 238 deletions

File tree

‎htdocs/js/PGProblemEditor/pgproblemeditor.js‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,40 @@
380380
}
381381
});
382382

383+
// Validation of the target set for the save as tab.
384+
const saveAsSaveModeRadios = document.getElementsByName('action.save_as.saveMode');
385+
const saveToTargetSetRadio = Array.from(saveAsSaveModeRadios).find(
386+
(r) => r.id === 'action_save_as_saveMode_new_problem_id' || r.id === 'action_save_as_saveMode_set_header_id'
387+
);
388+
const targetSetSelect = document.getElementsByName('action.save_as.targetSet')?.[0];
389+
const actionSaveAs = document.getElementById('save_as');
390+
const removeSetSelectErrors = () => {
391+
targetSetSelect?.classList.remove('is-invalid');
392+
targetSetSelect?.setCustomValidity('');
393+
};
394+
for (const radio of saveAsSaveModeRadios) {
395+
radio.addEventListener('change', removeSetSelectErrors);
396+
}
397+
const saveToTargetSetSelected = () => {
398+
saveToTargetSetRadio.checked = true;
399+
removeSetSelectErrors();
400+
};
401+
targetSetSelect?.addEventListener('change', saveToTargetSetSelected);
402+
targetSetSelect?.addEventListener('focusin', saveToTargetSetSelected);
403+
404+
document.forms.editor?.addEventListener('submit', (e) => {
405+
if (actionSaveAs && actionSaveAs.classList.contains('active')) {
406+
if (saveToTargetSetRadio?.checked && !targetSetSelect?.value) {
407+
e.preventDefault();
408+
targetSetSelect.classList.add('is-invalid');
409+
targetSetSelect.setCustomValidity(targetSetSelect.dataset.errorMessage ?? 'Please select a set.');
410+
targetSetSelect.reportValidity();
411+
return;
412+
}
413+
removeSetSelectErrors();
414+
}
415+
});
416+
383417
const fileType = document.getElementsByName('file_type')[0]?.value;
384418

385419
// This is either the div containing the CodeMirror editor or the problemContents textarea in the case that

‎lib/WeBWorK/ContentGenerator/Instructor/PGProblemEditor.pm‎

Lines changed: 125 additions & 126 deletions
Original file line numberDiff line numberDiff line change
@@ -145,26 +145,12 @@ sub pre_header_initialize ($c) {
145145
$c->{setID} = $c->stash('setID');
146146
$c->{problemID} = $c->stash('problemID');
147147

148-
# Parse setID which may come in with version data
149-
$c->{fullSetID} = $c->{setID};
150-
if (defined $c->{fullSetID} && $c->{fullSetID} =~ /^([^,]*),v(\d+)$/) {
151-
$c->{setID} = $1;
152-
$c->{versionID} = $2;
153-
}
154-
155148
# Determine displayMode and problemSeed that are needed for viewing the problem.
156149
# They are also two of the parameters which can be set by the editor.
157150
# Note that the problem seed may be overridden by the value obtained from the problem record later.
158151
$c->{displayMode} = $c->param('displayMode') // $ce->{pg}{options}{displayMode};
159152
$c->{problemSeed} = (($c->param('problemSeed') // '') =~ s/^\s*|\s*$//gr) || DEFAULT_SEED();
160153

161-
# Save file to permanent or temporary file, then redirect for viewing if it was requested to view in a new window.
162-
# Any problem file "saved as" should be assigned to "Undefined_Set" and redirected to be viewed again in the editor.
163-
# Problems "saved" or 'refreshed' are to be redirected to the Problem.pm module
164-
# Set headers which are "saved" are to be redirected to the ProblemSet.pm page
165-
# Hardcopy headers which are "saved" are also to be redirected to the ProblemSet.pm page
166-
# Course info files are redirected to the ProblemSets.pm page
167-
168154
# Insure that file_type is defined
169155
$c->{file_type} = ($c->param('file_type') // '') =~ s/^\s*|\s*$//gr;
170156

@@ -239,7 +225,7 @@ sub initialize ($c) {
239225
}
240226

241227
# Tell the templates if we are working on a PG file
242-
$c->{is_pg} = !$c->{file_type} || ($c->{file_type} ne 'course_info' && $c->{file_type} ne 'hardcopy_theme');
228+
$c->{is_pg} = $c->{file_type} =~ /problem/ || $c->{file_type} =~ /header/;
243229

244230
# Check permissions
245231
return
@@ -341,6 +327,8 @@ sub initialize ($c) {
341327
$c->{prettyProblemNumber} = join('.', jitar_id_to_seq($c->{prettyProblemNumber}))
342328
if $c->{set} && $c->{set}->assignment_type eq 'jitar';
343329

330+
$c->{globalSets} = [ map { $_->[0] } $db->listGlobalSetsWhere({}, 'set_id') ] if $c->{is_pg};
331+
344332
return;
345333
}
346334

@@ -523,36 +511,28 @@ sub getFilePaths ($c) {
523511
} elsif ($c->{file_type} eq 'set_header' || $c->{file_type} eq 'hardcopy_header') {
524512
my $set_record = $db->getGlobalSet($c->{setID});
525513

526-
if (defined $set_record) {
527-
my $header_file = $set_record->{ $c->{file_type} };
528-
if ($header_file && $header_file ne 'defaultHeader') {
529-
if ($header_file =~ m|^/|) {
530-
# Absolute address
531-
$editFilePath = $header_file;
532-
} else {
533-
$editFilePath = "$ce->{courseDirs}{templates}/$header_file";
534-
}
514+
my $header_file = defined $set_record ? $set_record->{ $c->{file_type} } : '';
515+
if ($header_file && $header_file ne 'defaultHeader') {
516+
if ($header_file =~ m|^/|) {
517+
# Absolute address
518+
$editFilePath = $header_file;
535519
} else {
536-
# If the set record doesn't specify the filename for a header or it specifies the defaultHeader,
537-
# then the set uses the default from assets/pg.
538-
$editFilePath = $ce->{webworkFiles}{screenSnippets}{setHeader}
539-
if $c->{file_type} eq 'set_header';
540-
$editFilePath = $ce->{webworkFiles}{hardcopySnippets}{setHeader}
541-
if $c->{file_type} eq 'hardcopy_header';
520+
$editFilePath = "$ce->{courseDirs}{templates}/$header_file";
542521
}
543522
} else {
544-
$c->addbadmessage("Cannot find a set record for set $c->{setID}");
545-
return;
523+
# If the set record does not exist, or the set record doesn't specify the filename for a header or it
524+
# specifies the defaultHeader, then the set uses the default from assets/pg.
525+
$editFilePath = $ce->{webworkFiles}{screenSnippets}{setHeader}
526+
if $c->{file_type} eq 'set_header';
527+
$editFilePath = $ce->{webworkFiles}{hardcopySnippets}{setHeader}
528+
if $c->{file_type} eq 'hardcopy_header';
546529
}
547530
} elsif ($c->{file_type} eq 'problem') {
548-
# First try getting the merged problem for the effective user.
549-
my $effectiveUserName = $c->param('effectiveUser');
550-
my $problem_record =
551-
$c->{versionID}
552-
? $db->getMergedProblemVersion($effectiveUserName, $c->{setID}, $c->{versionID}, $c->{problemID})
553-
: $db->getMergedProblem($effectiveUserName, $c->{setID}, $c->{problemID});
554-
555-
# If that doesn't work, then the problem is not yet assigned. So get the global record.
531+
# First try getting the merged problem for the current user.
532+
my $problem_record = $db->getMergedProblem($c->param('user'), $c->{setID}, $c->{problemID});
533+
534+
# If that doesn't work, then the problem is not assigned to this user (or this problem belongs to a gateway
535+
# test, since the problem editor can not deal with problems from versioned sets). So get the global record.
556536
$problem_record = $db->getGlobalProblem($c->{setID}, $c->{problemID}) unless defined $problem_record;
557537

558538
if (defined $problem_record) {
@@ -866,6 +846,7 @@ sub hardcopy_handler ($c) {
866846
hardcopy_theme => $c->param('action.hardcopy.theme')
867847
}
868848
));
849+
return;
869850
}
870851

871852
sub add_problem_handler ($c) {
@@ -1131,19 +1112,20 @@ sub save_as_handler ($c) {
11311112
'File "[_1]" exists. File not saved. No changes have been made.',
11321113
$c->shortPath($outputFilePath)
11331114
));
1134-
$c->addbadmessage(
1135-
$c->maketext('You can change the file path for this problem manually from the "Sets Manager" page'))
1136-
if defined $c->{setID};
11371115
}
11381116

11391117
if ($do_not_save) {
11401118
$c->addgoodmessage($c->maketext(
11411119
'The text box now contains the source of the original problem. '
11421120
. 'You can recover lost edits by using the Back button on your browser.'
11431121
));
1144-
}
11451122

1146-
unless ($do_not_save) {
1123+
# If the save mode is 'add_to_set_as_new_problem', but this problem is not in a set (for example for a sample
1124+
# problem), then the redirect for this save mode will fail since there is no set or problem id. So switch to the
1125+
# 'new_independent_file' save mode. That will work since it fills in the 'Undefined_Set' for the set id and 1
1126+
# for the problem id.
1127+
$saveMode = 'new_independent_file' if $saveMode eq 'add_to_set_as_new_problem' && !defined $c->{setID};
1128+
} else {
11471129
$c->{editFilePath} = $outputFilePath;
11481130
# saveFileChanges will update the tempFilePath and inputFilePath as needed. Don't do that here.
11491131

@@ -1155,91 +1137,108 @@ sub save_as_handler ($c) {
11551137
# presented in the form. So set that here so that the correct redirect is chosen below.
11561138
$saveMode = "new_$file_type";
11571139
} elsif ($saveMode eq 'rename' && -r $outputFilePath) {
1158-
# Modify source file path in problem.
1159-
if ($file_type eq 'set_header') {
1160-
my $setRecord = $db->getGlobalSet($c->{setID});
1161-
$setRecord->set_header($new_file_name);
1162-
if ($db->putGlobalSet($setRecord)) {
1163-
$c->addgoodmessage($c->maketext(
1164-
'The set header for set [_1] has been renamed to "[_2]".', $c->{setID},
1165-
$c->shortPath($outputFilePath)
1166-
));
1167-
} else {
1168-
$c->addbadmessage($c->maketext(
1169-
'Unable to change the set header for set [_1]. Unknown error.', $c->{setID}));
1170-
}
1171-
} elsif ($file_type eq 'hardcopy_header') {
1172-
my $setRecord = $db->getGlobalSet($c->{setID});
1173-
$setRecord->hardcopy_header($new_file_name);
1174-
if ($db->putGlobalSet($setRecord)) {
1175-
$c->addgoodmessage($c->maketext(
1176-
'The hardcopy header for set [_1] has been renamed to "[_2]".', $c->{setID},
1177-
$c->shortPath($outputFilePath)
1178-
));
1179-
} else {
1180-
$c->addbadmessage($c->maketext(
1181-
'Unable to change the hardcopy header for set [_1]. Unknown error.',
1182-
$c->{setID}
1183-
));
1184-
}
1140+
my $problemRecord = $db->getGlobalProblem($c->{setID}, $c->{problemID});
1141+
$problemRecord->source_file($new_file_name);
1142+
if ($db->putGlobalProblem($problemRecord)) {
1143+
$c->addgoodmessage($c->maketext(
1144+
'The source file for "set [_1] / problem [_2]" has been changed from "[_3]" to "[_4]".',
1145+
$c->{setID},
1146+
$c->{prettyProblemNumber},
1147+
$c->shortPath($c->{sourceFilePath}),
1148+
$c->shortPath($outputFilePath)
1149+
));
11851150
} else {
1186-
my $problemRecord;
1187-
if ($c->{versionID}) {
1188-
$problemRecord =
1189-
$db->getMergedProblemVersion($c->param('effectiveUser'), $c->{setID}, $1, $c->{problemID});
1190-
} else {
1191-
$problemRecord = $db->getGlobalProblem($c->{setID}, $c->{problemID});
1192-
}
1193-
$problemRecord->source_file($new_file_name);
1194-
my $result =
1195-
$c->{versionID} ? $db->putProblemVersion($problemRecord) : $db->putGlobalProblem($problemRecord);
1196-
1197-
if ($result) {
1198-
$c->addgoodmessage($c->maketext(
1199-
'The source file for "set [_1] / problem [_2]" has been changed from "[_3]" to "[_4]".',
1200-
$c->{fullSetID},
1201-
$c->{prettyProblemNumber},
1202-
$c->shortPath($c->{sourceFilePath}),
1203-
$c->shortPath($outputFilePath)
1204-
));
1205-
} else {
1206-
$c->addbadmessage($c->maketext(
1207-
'Unable to change the source file path for set [_1], problem [_2]. Unknown error.',
1208-
$c->{fullSetID}, $c->{prettyProblemNumber}
1209-
));
1151+
$c->addbadmessage($c->maketext(
1152+
'Unable to change the source file path for set [_1], problem [_2]. Unknown error.',
1153+
$c->{setID}, $c->{prettyProblemNumber}
1154+
));
1155+
}
1156+
} elsif ($saveMode eq 'set_as_heaader_for_set' && -r $outputFilePath) {
1157+
my $setID = $c->param('action.save_as.targetSet');
1158+
if (defined $setID && $setID =~ /\S/) {
1159+
$c->{setID} = $setID;
1160+
if ($file_type eq 'set_header') {
1161+
my $setRecord = $db->getGlobalSet($c->{setID});
1162+
$setRecord->set_header($new_file_name);
1163+
if ($db->putGlobalSet($setRecord)) {
1164+
$c->addgoodmessage($c->maketext(
1165+
'The set header for set [_1] has been set to "[_2]".', $c->{setID},
1166+
$c->shortPath($outputFilePath)
1167+
));
1168+
} else {
1169+
$c->addbadmessage($c->maketext(
1170+
'Unable to change the set header for set [_1]. Unknown error.',
1171+
$c->{setID}
1172+
));
1173+
}
1174+
} elsif ($file_type eq 'hardcopy_header') {
1175+
my $setRecord = $db->getGlobalSet($c->{setID});
1176+
$setRecord->hardcopy_header($new_file_name);
1177+
if ($db->putGlobalSet($setRecord)) {
1178+
$c->addgoodmessage($c->maketext(
1179+
'The hardcopy header for set [_1] has been set to "[_2]".', $c->{setID},
1180+
$c->shortPath($outputFilePath)
1181+
));
1182+
} else {
1183+
$c->addbadmessage($c->maketext(
1184+
'Unable to change the hardcopy header for set [_1]. Unknown error.',
1185+
$c->{setID}
1186+
));
1187+
}
12101188
}
1189+
} else {
1190+
my $headerType =
1191+
$file_type eq 'set_header' ? $c->maketext('set header') : $c->maketext('hardcopy header');
1192+
$c->addbadmessage($c->maketext(
1193+
'A new file has been created at "[_1]" with the contents below. However, '
1194+
. 'the file has not been set as the [_2] for a set, since no target set was specified.',
1195+
$c->shortPath($outputFilePath),
1196+
$headerType
1197+
));
1198+
$saveMode = 'new_independent_file';
12111199
}
12121200
} elsif ($saveMode eq 'add_to_set_as_new_problem') {
1213-
my $set = $db->getGlobalSet($c->{setID});
1201+
my $setID = $c->param('action.save_as.targetSet');
1202+
if (defined $setID && $setID =~ /\S/) {
1203+
$c->{setID} = $c->param('action.save_as.targetSet');
1204+
my $set = $db->getGlobalSet($c->{setID});
1205+
1206+
# For jitar sets new problems are put as top level problems at the end.
1207+
if ($set->assignment_type eq 'jitar') {
1208+
my @problemIDs = $db->listGlobalProblems($c->{setID});
1209+
@problemIDs = sort { $a <=> $b } @problemIDs;
1210+
my @seq = jitar_id_to_seq($problemIDs[-1]);
1211+
$targetProblemNumber = seq_to_jitar_id($seq[0] + 1);
1212+
} else {
1213+
$targetProblemNumber = 1 + max($db->listGlobalProblems($c->{setID}));
1214+
}
12141215

1215-
# For jitar sets new problems are put as top level problems at the end.
1216-
if ($set->assignment_type eq 'jitar') {
1217-
my @problemIDs = $db->listGlobalProblems($c->{setID});
1218-
@problemIDs = sort { $a <=> $b } @problemIDs;
1219-
my @seq = jitar_id_to_seq($problemIDs[-1]);
1220-
$targetProblemNumber = seq_to_jitar_id($seq[0] + 1);
1216+
my $problemRecord = addProblemToSet(
1217+
$db, $c->ce->{problemDefaults},
1218+
setName => $c->{setID},
1219+
sourceFile => $new_file_name,
1220+
problemID => $targetProblemNumber,
1221+
);
1222+
assignProblemToAllSetUsers($db, $problemRecord);
1223+
$c->addgoodmessage($c->maketext(
1224+
'Added [_1] to [_2] as problem [_3].',
1225+
$new_file_name,
1226+
$c->{setID},
1227+
(
1228+
$set->assignment_type eq 'jitar'
1229+
? join('.', jitar_id_to_seq($targetProblemNumber))
1230+
: $targetProblemNumber
1231+
)
1232+
));
12211233
} else {
1222-
$targetProblemNumber = 1 + max($db->listGlobalProblems($c->{setID}));
1234+
$c->addbadmessage($c->maketext(
1235+
'A new file has been created at "[_1]" with the contents below. '
1236+
. 'However, the problem has not been added to a set, since no target set was specified.',
1237+
$c->shortPath($outputFilePath)
1238+
));
1239+
$saveMode = 'new_independent_file';
12231240
}
1224-
1225-
my $problemRecord = addProblemToSet(
1226-
$db, $c->ce->{problemDefaults},
1227-
setName => $c->{setID},
1228-
sourceFile => $new_file_name,
1229-
problemID => $targetProblemNumber, # Added to end of set
1230-
);
1231-
assignProblemToAllSetUsers($db, $problemRecord);
1232-
$c->addgoodmessage($c->maketext(
1233-
'Added [_1] to [_2] as problem [_3].',
1234-
$new_file_name,
1235-
$c->{setID},
1236-
(
1237-
$set->assignment_type eq 'jitar'
1238-
? join('.', jitar_id_to_seq($targetProblemNumber))
1239-
: $targetProblemNumber
1240-
)
1241-
));
1242-
} elsif ($saveMode eq 'new_independent_problem') {
1241+
} elsif ($saveMode eq 'new_independent_file') {
12431242
$c->addgoodmessage($c->maketext(
12441243
'A new file has been created at "[_1]" with the contents below.',
12451244
$c->shortPath($outputFilePath)
@@ -1260,15 +1259,15 @@ sub save_as_handler ($c) {
12601259
if ($saveMode eq 'new_course_info') {
12611260
$problemPage = $c->url_for('instructor_problem_editor');
12621261
$new_file_type = 'course_info';
1263-
} elsif ($saveMode eq 'new_independent_problem') {
1262+
} elsif ($saveMode eq 'new_independent_file') {
12641263
$problemPage =
12651264
$c->url_for('instructor_problem_editor_withset_withproblem', setID => 'Undefined_Set', problemID => 1);
1266-
$new_file_type = 'source_path_for_problem_file';
1265+
$new_file_type = $file_type =~ /header/ ? $file_type : 'source_path_for_problem_file';
12671266
} elsif ($saveMode eq 'new_hardcopy_theme') {
12681267
$problemPage = $c->url_for('instructor_problem_editor');
12691268
$new_file_type = 'hardcopy_theme';
12701269
$extra_params{hardcopy_theme} = $new_file_name =~ s|^.*\/([^/]*\.xml)|$1|r;
1271-
} elsif ($saveMode eq 'rename') {
1270+
} elsif ($saveMode eq 'rename' || $saveMode eq 'set_as_heaader_for_set') {
12721271
$problemPage = $c->url_for(
12731272
'instructor_problem_editor_withset_withproblem',
12741273
setID => $c->{setID},

‎lib/WeBWorK/ContentGenerator/Instructor/ProblemSetDetail.pm‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1298,7 +1298,6 @@ sub initialize ($c) {
12981298
my $setID = $c->stash('setID');
12991299

13001300
# Make sure these are defined for the templates.
1301-
$c->stash->{fullSetID} = $setID;
13021301
$c->stash->{headers} = HEADER_ORDER();
13031302
$c->stash->{field_properties} = FIELD_PROPERTIES();
13041303
$c->stash->{display_modes} = WeBWorK::PG::DISPLAY_MODES();

0 commit comments

Comments
 (0)