Skip to content
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

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

drgrice1
Copy link
Member

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.

@drgrice1 drgrice1 force-pushed the update-course-dirs-improved branch 3 times, most recently from 925be98 to b8156fb Compare August 11, 2023 23:04
Copy link
Member

@pstaabp pstaabp left a 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.

@Alex-Jordan
Copy link
Contributor

This now has conflicts.

@drgrice1 drgrice1 force-pushed the update-course-dirs-improved branch 2 times, most recently from 6d9b9d6 to f487564 Compare August 17, 2023 22:01
@drgrice1
Copy link
Member Author

The conflicts are fixed. Thanks.

@drgrice1 drgrice1 force-pushed the update-course-dirs-improved branch 2 times, most recently from 595fe3c to 80d9713 Compare August 23, 2023 01:33
@drgrice1 drgrice1 force-pushed the update-course-dirs-improved branch from 80d9713 to 7450d47 Compare September 11, 2023 21:53
@drgrice1 drgrice1 force-pushed the update-course-dirs-improved branch from 7450d47 to 1ca9b93 Compare September 19, 2023 18:39
@drgrice1 drgrice1 force-pushed the update-course-dirs-improved branch 4 times, most recently from 3cc6791 to 9503417 Compare October 11, 2023 21:05

#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
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

@drgrice1 drgrice1 Oct 22, 2023

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.
@drgrice1 drgrice1 force-pushed the update-course-dirs-improved branch 3 times, most recently from 1f5a535 to cdf8114 Compare October 22, 2023 20:47

#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
Copy link
Contributor

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.

@@ -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
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.
@drgrice1 drgrice1 force-pushed the update-course-dirs-improved branch from cdf8114 to 4d78d25 Compare October 23, 2023 19:34
@Alex-Jordan Alex-Jordan merged commit 519a837 into openwebwork:develop Oct 23, 2023
@drgrice1 drgrice1 deleted the update-course-dirs-improved branch October 23, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants