-
-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvements to the course directory update method. #2173
Improvements to the course directory update method. #2173
Conversation
925be98
to
b8156fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcopyTheme in the course upgrades was bugging me this week on our production server. Good fix.
b8156fb
to
5a21be8
Compare
This now has conflicts. |
6d9b9d6
to
f487564
Compare
The conflicts are fixed. Thanks. |
595fe3c
to
80d9713
Compare
80d9713
to
7450d47
Compare
7450d47
to
1ca9b93
Compare
3cc6791
to
9503417
Compare
|
||
#FIXME this is hardwired for the time being. | ||
my %updateable_directories = (html_temp => 1, mailmerge => 1, tmpEditFileDir => 1, hardcopyThemes => 1); | ||
# All $courseDirs keys should be listed here except for root. The keys of the $courseDirs hash can not be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider sorting the keys? This is not tested, but I mean using sort like:
@course_dirs = sort { $courseDirs{$b} =~ /^$courseDirs{$a}/ } (keys %courseDirs);
so it sorts based on if a path is a super path to another path, by assuming the super path's string is a starting substring of the sub path's string.
It would correctly sort things so that for example aa/bb
would come before aa/bb/cc
. It would also harmlessly sort things like aa/bb
to come before aa/bbb
. It would not sort things like aa//bb
to come before aa/bb/cc
but that seems like someone shouldn't configure that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wondered if there is an OS utility to check if some path is a subpath of some other path. And it could be sorted that way more robustly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good idea. Although I think you meant
@course_dirs = sort { $courseDirs{$a} =~ /^$courseDirs{$b}/ } keys %courseDirs;
With your code it would be a reverse lexical sort on the directories, and so aa/bb
would come after aa/bb/cc
.
I have tested this with the forward sort, and I will add it to this pull request (also removing the root
directory).
We probably don't need to worry about things like aa//bb
. Although that sort of thing is not unlikely if a path is built from another path and the first path already has a forward slash at the end and the built path adds another (this is very common with paths in webwork). However, I don't think that has happened with these paths in the course environment (with default settings at least).
This always creates a course directory if it is missing. It also attempts to fix permissions if those are not correct. It does not attempt to change ownership or the group as that will almost always fail, and will need to be done manually. The directories that can be copied from the model course are copied if a directory is created. This is only done if the path did not exist to begin with.
1f5a535
to
cdf8114
Compare
|
||
#FIXME this is hardwired for the time being. | ||
my %updateable_directories = (html_temp => 1, mailmerge => 1, tmpEditFileDir => 1, hardcopyThemes => 1); | ||
# All $courseDirs keys should be listed here except for root. The keys of the $courseDirs hash can not be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wondered if there is an OS utility to check if some path is a subpath of some other path. And it could be sorted that way more robustly.
conf/defaults.config
Outdated
@@ -339,6 +339,10 @@ $webworkURLs{AuthorHelpURL} ='https://webwork.maa.org/wiki/Category:Au | |||
# Defaults for course-specific locations (directories and URLs) | |||
################################################################################ | |||
|
|||
# Developer note: Make sure to keep the list of directories in the updateCourseDirectories method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment needed now with the recent change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No its not. I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know of a utility that checks if a path is a sub directory of another directly. There are utilities that split a path into parts though. So you could split the path into parts, and then sort on the parts much like sorting by last name, then first name. If a path has more parts then another, then it would be sorted later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is path_is_subdir
in WeBWorK/Utils.pm
, though unsure how efficient that would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@somiaj: Yeah, we just discussed that in the meeting.
As suggested by @Alex-Jordan, sort by paths using keys from the courseDirs hash in the course enviroment.
cdf8114
to
4d78d25
Compare
This always creates a course directory if it is missing. It also attempts to fix permissions if those are not correct. It does not attempt to change ownership or the group as that will almost always fail, and will need to be done manually.
The directories that can be copied from the model course are copied if a directory is created. This is only done if the path did not exist to begin with.
This is a much improved approach to #2169 or #2170.