-
-
Notifications
You must be signed in to change notification settings - Fork 166
allow customization of admin course ID #2295
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
Conversation
|
Several PRs are open at this time which make changes to (or about) the admin course: #2288, #2290, #2291, #2292, #2293. At least some of them probably assume a hard coded |
3059138 to
2a22d11
Compare
|
Not seeing this working. I changed the name of the admin_course_id to Also, will the database tables need to be updated? |
2a22d11 to
412f927
Compare
Just checking, but did you:
This does not take the existing
I don't think so. But I think you were thinking of this as the |
|
I see now that I will need to create the new admin course. That's how the database tables and directory will work. |
|
I want to point out that after making a new course that will become the admin course, the admin user needs to have admin privileges in order for the admin course to function--which is good that those are checked. |
pstaabp
left a comment
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.
After initial testing, this seems to be working fine. Didn't test all admin functions, but navigation works, which seems to be the primary goal of this PR.
412f927 to
d88b132
Compare
|
One thing that is a bit odd with this is that this I think that |
|
Some scripts probably assume the admin course name is |
|
Good catch. That should be added to this pull request. The The The I believe those are the only scripts that do anything with the admin course directly. |
c628aa9 to
ee367c1
Compare
|
I updated the scripts but did not test them. My school and its vendors have had power outages all week from a severe ice storm and tonight I can't get to the test server I use. I just edited these locally without testing on a real database. I think these changes work, but someone should take a look. |
ee367c1 to
7ad3968
Compare
drgrice1
left a comment
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 updates to the scripts work fine. I tested them. Thanks for adding that.
There are three lines of code changed. Line 53 of `templates/ContentGenerator/CourseAdmin.html.ep` was deleted. That line should not exist. That saves a list of all courses to the `@courseIDs` variable that is never used. The `listCourses` method is called again on line 56 (now 55). On line 56 (now 55) of that same file, the admin course is omitted from the listing. This is how it used to be, and was removed in openwebwork#2295. I don't believe that the admin course should be listed on the admin course listings page. On line 110 of `templates/ContentGenerator/CourseAdmin/add_course_form.html.ep` the admin course is kept in the listing. This is the list of courses that can be copied. This was how it was set up in openwebwork#2295, but was reverted in openwebwork#2290 (probably in merge resolution, or maybe my changes to that pull request?). The intent was to allow the admin course to be copied to change to a new course to use for the admin course. The first line changed is not controversial, but the second two are up for discussion.
There are three lines of code changed. Line 53 of `templates/ContentGenerator/CourseAdmin.html.ep` was deleted. That line should not exist. That saves a list of all courses to the `@courseIDs` variable that is never used. The `listCourses` method is called again on line 56 (now 55). On line 56 (now 55) of that same file, the admin course is omitted from the listing. This is how it used to be, and was removed in openwebwork#2295. I don't believe that the admin course should be listed on the admin course listings page. On line 110 of `templates/ContentGenerator/CourseAdmin/add_course_form.html.ep` the admin course is kept in the listing. This is the list of courses that can be copied. This was how it was set up in openwebwork#2295, but was reverted in openwebwork#2290 (probably in merge resolution, or maybe my changes to that pull request?). The intent was to allow the admin course to be copied to change to a new course to use for the admin course. The first line changed is not controversial, but the second two are up for discussion.
This allows a site to use something other than
adminfor the admin course ID. The purpose is to remove the ability for bad actors to know the location of the admin course.To test, after loading this branch, you need to immediately edit
site.confto get the new line that initializes$admin_course_id. Then restartwebwork2.One thing the PR does is allow the "admin" course (or whatever its name is changed to) to be the source course to copy from when creating a new course. I'm not sure if this is necessary and maybe we can remove that change. But in any case, next make a new course (you can call it "my-admin" for testing). Its templates folder can be copied from the "admin" course. (And if #2290 is merged, then more could be copied as well.)
Then in
site.conf, change$admin_course_idto be the ID of the new course, and restartwebwork2. Now if you go to "admin", it should look like a regular course. And if you go to the new course, it should function as the admin course. In practice you would then also clean up by copying archive files from the old "admin" course to the new course (since they live parallel totemplates/they did not copy over when creating the new course). And I think you would also archive and close the original "admin" course at that time.