Improvements for the "Save As" form in the PG editor.#3017
Conversation
drgrice1
force-pushed
the
pgeditor-improvements
branch
5 times, most recently
from
403aebd to
1535cb8
Compare
|
Wow, this is quite an evolution! Thanks for making this happen. How does this interact with the existing Append tab? Is the intent that one tab is for "save file and append" and the other is just for "append"? I also don't understand the following sequence.
OK, the file was not saved since that filename is already in use. Good. And I can check that nothing was appended to the set I chose. Good. But now I do have an "Append" tab. Why do I have an Append tab now but not before? |
|
Yes, the "Append Tab" is just to append the already saved file to a set and works as before. In your sequence, you now have the append tab because it switched to the existing file. That file is saved in the course's templates directory, and so it can be appended to a set. |
|
In general the new problem handling is not intuitive. I think that the file name that is offered to be saved should not be "newProblem.pg" that would fix part of the issue. But in general, the message should not say |
|
So is the only way to not have an Append tab, is when you visit Problem Editor and choose New problem template? |
|
Or a sample problem? |
|
We could probably merge "New problem template" and "Sample problem", and just make something like the current "New problem template" be the default option in the "Sample problem" list. |
|
I'm losing an understanding of why we would keep the Append tab. What is a situation when you would want to use the Append tab, and using the Save As tab with this new append option would be bad? That would mean the page loaded with an actual file. And you have made changes to that file in the editor, but for some reason you do not want to save those changes (to the original file, or as a new file) and you want to append the problem you started with, before your edits, to a set. That seems like an unlikely scenario. |
|
OK. If it is an OPL problem that you clicked to edit from an assignment. And you wish to append that problem to an assignment without saving a local copy. Although, this seems like an abuse of the "editor" if your objective was to append a certain OPL problem to a set. |
|
The problem is that the "save as" tab will not work if the file already exists. Also the "append tab" is not a save. |
|
@Alex-Jordan one use case could be you have a problem you want to add to multiple sets. You write it, you save it (maybe append it to one set), then can also append it other sets. That being said, I've never used the append tab, but it is something different than save as and maybe some use it. |
|
@somiaj But in that use case, it is harmless to use "Save As" with append repeatedly. I see other use cases though where Append is needed. I just can't shake a feeling that something is off. (To be sure, the changes here make things much better.) What if the Append tab were always present, but sometimes grayed out? With a hover explanation for why it's grayed out? |
|
Sorry, not "Save As" with append repeatedly. I was thinking "Save" with append. Which now I realize is not a thing with this PR. Sorry for that noise. |
|
OK, well potential tangential improvements to all of this aside, this PR is good and I marked it approved. |
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 openwebwork#2992.
drgrice1
force-pushed
the
pgeditor-improvements
branch
2 times, most recently
from
ab6e2e2 to
9d3f738
Compare
|
Note, that I just made a few changes to validation. It now also validates the filename. Basic validation at least. It just ensures that the file name is non-empty. Validation that the file is writable, and doesn't already exist has to be done server side. Also, I am still working on some things with this, so please wait to merge. I noticed that the "Save" tab is being displayed in many cases when it shouldn't be. For example, if you are editing the default set header (the file |
drgrice1
force-pushed
the
pgeditor-improvements
branch
from
9d3f738 to
34d8c10
Compare
|
Okay, now I think this is ready. Basically, any file that is not in the course's templates directory is now considered a template, and for these files the "save" tab is not shown. Note that this uses the |
|
There is further work that is needed on the problem editor. The redirects when "save as" or "append" are used need to be entirely removed. The only valid redirects that should occur are when the "view" or "save" tabs are used and "Open in new window" is checked. This has been something that I have wanted to fix for a long time, but it will take some work to ensure that the file paths are correctly set up for rendering after the handlers are called. This is the current reason for the redirect hacks. So that the |
|
Just to clarify my last comment, that is not something that will be part of this pull request, and is later work. This pull request is ready for review. |
…s considered a template.
Basically, any file that is not in the course's templates directory is
now considered a template, and for these files the "save" tab is not
shown. Note that this uses the `WeBWorK::Utils::Files::path_is_subdir`
method, and so files that are linked to from the course's templates
directory are not considered templates. Although, if those files are not
writable by the server, then the message `The file "[_1]" is protected.
You may use "Save As" to create a new file.` will be shown. So OPL or
Contrib problems will not be considered templates, but if file
permissions are correct on your server, then the message above will be
shown. However, if a file is in a location in `$webworkDirs{valid_symlinks}`
array, and file permissions are correct on your server, then no message
will be shown, and the file can be saved.
drgrice1
force-pushed
the
pgeditor-improvements
branch
from
78ffb2f to
a9a1457
Compare
|
Doing some initial testing. I did notice another thing (not new to this PR). But if editing a problem, the append tab does have an option to append it as a set or hard copy header. Should this menu be updated to only include appending as a header when editing set headers? |
|
When someone goes to save as, and tries to use save as to append the file to a different set but doesn't change the file name, the form can be submitted but an error occurs. I know in general file verification cannot be done client side, but what about the case the file is currently saved and they don't change the file name to a new name? So have some client side verification if the user doesn't change the filename to a new name? |
I can do that (for file types that those tabs apply to) if that is what we want. I am not sure what the real benefit of that is though. |
That isn't really an option. The thing is that the problem editor really doesn't know generally if a file is supposed to be a problem or a set header. It bases its decision on what it is told when it first opens. However, when editing a problem from the file manager, there is no valid determination. The problem editor is told it is a problem, but it really could be a set header. Note that not all set headers have "header" in their name, and there is no reason they should need to. |
At best client side can only guess on this. I am not sure that this is really feasible. What could perhaps be done is that when the user gives a file name that exists, respond with some confirmation page asking if the user wants to overwrite the existing file. That would take a little redesign of how the save as tab works though. |
But the append tab only works with saved files. So when opening a saved file the editor may know if it is a problem or set header? At least that was my thought, since append requires the file to be saved the editor might know what it is being used for in some cases. But I guess there is a case someone edits a problem, deletes it and creates a set header, saves it as a new file, and then appends it afterwards. I didn't consider that case. What about three cases, if the file unattached show all options, if it is already attached to a set as a problem or a set header, only show the appropriate options? |
Doesn't the editor/client know the name of the saved file being worked on. I was only suggesting require the file name be changed to something different than the already known saved file name. But I do like the idea of creating some sort of confirmation if the file already exists. I would think there should be three options:
|
But that is exactly what I just said. When you open a file from the file manager, that IS a saved file. And no, the editor does not know if it is a problem or a set header.
What if someone starts with a problem in a set, then changes all of the content and wants to make it a set header? Or the opposite. I see no reason to prevent that case. I just don't understand what benefit there is to making this proposed change. |
The editor server side knows the name of the file. Client side not so much. I mean, the full path for the file is in a hidden input, but the client does not know what the course templates directory really is. So it cannot properly deconstruct that, and really shouldn't. This is just not a sane approach. Adding a confirm page can be done, but that is not for this pull request. |
If there is a benefit (and I concede this is an if) it is about educating users about the options and conditions that feed all of this. If I see a grayed out tab, I am made aware that generally this could be an option, but there is something going on right now that blocks me from using that option. Maybe next I'd hover over the disabled tab to see if it will tell me why it is disabled. And whatever it said would help me (as a generic user) start to understand the distinction between files and templates. Contrast this with just not showing tabs. A user might have a sense that sometimes things show up and sometimes they don't, and not really have a way to learn why. Except indirectly, from experience. And still might not fully appreciate the file vs template distinction. |
drgrice1
force-pushed
the
pgeditor-improvements
branch
from
73fa467 to
c8b3e65
Compare
I am fine with this being done. However, I think that I am also going to insist that this is not for this pull request. Doing this is going to take some reworking of the way that the tabs and their content are inserted into the page. If this is done the tab should be inserted, but its content really shouldn't be. This is completely different than the only tab that is grayed out at this point which is the "revert" tab. That tab is added because JavaScript can activate it, and so its content is also needed. Also, care will be needed to ensure the tabs are not shown for the file types for which they never apply (for instance course headers can never use the "append" tab). Note that another way of educating the user on behavior is via the help. That could certainly be improved regarding when a tab is usable. Currently it merely states, "If an action cannot be executed it will not appear." |
|
Sounds good. Thanks for thinking it through. |
3 participants
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/pgin 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.