be able to copy more things from a course when adding a new course

[Alex-Jordan]

This changes the "Add Course" sub page of Course Admin, so that when you make a new course, there is more you can copy from some target course (currently, you can copy the templates folder, html folder, and simple.conf file).

Now you can also copy the course.conf file, although this is discouraged in the help file and not selected by default.

Addiitionally you can copy certain data from the database. Namely:

• non-student users
• sets (aka assignments). This includes the problems associated with the sets from the problem table, and includes any locations associated with those sets.
• achievements
• the course title and/or course institution, which (if selected) override whatever you typed into the input fields above

For these database-flavored components, I made them all selected for copying when the page loads. I realize it's a change in behavior, and at first I had them all unchecked for that reason. But the more I thought about it, the fewer use cases I could come up with where you would want any of these unchecked. But it's up for discussion.

One thing I did not do: if users and sets are copied, this does not keep users assigned to whichever sets they were assigned to in the older course. Same with achievements. That could be done if desired, but I would only want it to reflect the assignments, not any progress the user made on the sets or the achievements.

[Alex-Jordan]

Now it's back to only selecting "copy templates and html folders" when you first load the page, but there is a "select all" checkbox too. The "select all" does not include selecting the "copy config file" checkbox. I think this should only be done if someone knows they really want that. In general when creating a course we should let it generate a brand new course.conf file and not propagate something old.

[Alex-Jordan]

Can someone explain to me what a "non_native" table is?

Before the changes here, there is a check if the user table is "non_native". Whatever it means, I'm not sure why it's just a check of the user table, and not also for the password and permission table which also get added to in that code block.

Mimicking this, I put checks for "non_native" around the code blocks that do the new things (copy problem set records, etc.) But I'm not sure what I am doing there or why I am doing it. Or if checking the set table is not "non_native" is enough when there are also the problem and set_location tables being added to here.

[drgrice1]

The non-native tables are the tables in the database that do not belong to a course, as well as the virtual tables that are not actual tables at all. The latter tables you don't need to worry about for this. Those are the merged and versioned tables. The non-native tables that are actual tables are the "locations", "location_addresses", and "depths" tables.

A quick note on the database.conf.dist file. That file is perltidied in #2276. So your changes here will conflict with that. Your changes seem to be indentation cleanup which perltidy takes care of as well as much more.

[drgrice1]

Also, the check to see if the user table is non-native is probably there for a custom database layout (perhaps the sql_moodle layout) that uses a non-native user table that is not the usual course_id_user table. It is most likely a remnant that should be deleted, and support for using custom database layouts removed.

[Alex-Jordan]

I was hoping it was the right thing to just remove that check. I will do that. No rush on reviewing this, of course.

I isolated that indentation into one commit, so it will be easy to just revert that and avoid the conflict.

[Alex-Jordan]

I see a logic problem in one place. Together with the removal of those "non_native" checks, no one should look at this too much until I update.

[Alex-Jordan]

I updated a few things mentioned in this thread and this is ready for review.

[Alex-Jordan]

I made this so that when (non-student) users are copied from an old course:

• if sets were copied, then assign the same sets to each user in the new course as they had in the old course
• if achievements were copied, then assign the same achievements to each user in the new course as they had in the old course

[pstaabp]

Overall, I like this. I have a script that does a bunch of this right now, but this may make it superfluous. And I always need to make sure that instructors export their homework sets--which they often don't do. I think your defaults do the right thing even if we have some changing defaults.

[Alex-Jordan]

OK, I see that you have changes in #2172 that will impact much of this with conflicts. I think maybe one or the other of these two should be merged and then sort things out with the other one after that.

[pstaabp]

I just updated #2172 and hopefully we can get that merged soon.

[drgrice1]

There are some issues that I see. I will add a pull request that handles these things. It also tweaks the margins on the "Add Course" page. Those seem to have gotten out of sync.

[Alex-Jordan]

Thanks for all the reviewing you are doing. This is the first week of class at PCC, and I'm pretty busy with day job stuff. I will follow up with your feedback when I can. One thing caught my eye:

This checkbox does not work with the "select-all" checkbox, because it doesn't have the same name as the rest of the checkboxes in the select group. There is no need for this to have a different name. Delete line 368 of CourseAdmin.pm, change the name to copy_component, and the value to copyConfig, and it will work like the rest of the checkboxes in this select group.

So, right or wrong, that was intentional. I don't think it is a good idea to copy the course config file from one course to another new course. Even when you want to copy everything else. I don't know how much it matters, but it sets $pg{specialPGEnvironmentVars}{PRINT_FILE_NAMES_FOR} using the specific user name of the instructor that is added on course creation as the new instructor. So copying that to a new course with a new instructor seems like a mistake.

So I wanted to discourage copying course.conf unless a person really knows what they are doing. So it was intentionally removed from the select-all construction. You have to go the extra mile if you really want that copied.

IIRC, I think we hope to one day stop giving instructors access to course.conf at all. Anything the instructor should be able to adjust should be in simple.conf, ideally. In that future, it also seems unlikely there would be something important to copy from course.conf.

[drgrice1]

If that is the case, then the copy course configuration file checkbox should not be listed with the rest. It should be separated out below, and perhaps a comment added that gives a warning about using it (or some explanation as to why it is separate from the rest). It is non-intuitive when there is a select all checkbox, but yet it doesn't select "all". If I were to see that in a user interface with no explanation, I think something is broken.

Also, we want the course.conf file not to be editable by instructors, but this is the admin course and admin users only have access to this. We do allow admin users to edit the course.conf file.

[Alex-Jordan]

I can move it when I come back to this. (Or I can group it in with the select-all, if you think that is best.) Note that this issue mentioned (lightly) in the help file content that was added with this PR.

Also, we want the course.conf file not to be editable by instructors, but this is the admin course and admin users only have access to this. We do allow admin users to edit the course.conf file.

What I'm talking about is when I use the admin course to copy things from instructor A's old course to make a new course for instructor B. I might want the new course to have everything from the old course, but I am wary of bringing A's old course.conf file into B's new course. A and B are maybe not admin users. So I'm not sure what you mean.

[drgrice1]

I can move it when I come back to this. (Or I can group it in with the select-all, if you think that is best.) Note that this issue mentioned (lightly) in the help file content that was added with this PR.

I think that separating this checkbox from the others by moving it below, and adding a brief comment before it would alleviate the issue. If the checkbox is in the middle of all of the rest that are affected by the select all checkbox it is odd.

What I'm talking about is when I use the admin course to copy things from instructor A's old course to make a new course for instructor B. I might want the new course to have everything from the old course, but I am wary of bringing A's old course.conf file into B's new course. A and B are maybe not admin users. So I'm not sure what you mean.

I just mean that even though A and B are not admin users, the admin user that is doing the copying of the courses is an admin user that has the permission to edit the course.conf file. So if we have things set up right so that instructor A and B can not edit the course.conf file in any way, then it is all up to the admin user as to what is in the file. So presumably the admin user knows what is in the file, and knows if it will work for the new course or at least how to adapt it so that it will.

[Alex-Jordan]

It think this has conflicts repaired and externalPrograms removed now, and ready for people to test one more time.

[Alex-Jordan]

I tested and something is indeed broken here. I'll update once I have looked into it.

[Alex-Jordan]

OK, now I have tested this and it worked. A new course was made, and it copied all the things this is supposed to copy (and did not copy student accounts).

[pstaabp]

Looks good now.

[drgrice1]

I just noticed that the bin/addcourse script needs to be updated for the changes to the addCourse method's optional arguments. It uses the templatesFrom argument that has been replaced with the copyFrom argument.

[drgrice1]

Note that anyone that uses the addCourse method in their own script needs to be aware of the changes to the optional arguments in this pull request also. I have some scripts that I will need to update, and I know that there are others that have their own scripts that use this as well.

[drgrice1]

I added a pull request to this branch that updates the bin/addcourse script for the new options in the addCourse method. It just uses the simplest approach and preserves the existing behavior of the script. That is probably sufficient at this point. If you want to do more, feel free. For example, command line options could be added to the script to utilize the new addCourse options.

[drgrice1]

Once you either merge my pull request or update the bin/addcourse in another way (if desired), then lets get this in so that creating courses is not broken anymore due to the mistake on line 284 of lib/WeBWorK/Utils/CourseManagement.pm.

[Alex-Jordan]

I merged your PR with the 120 character issue and the bin/addcourse script.

[drgrice1]

I will merge this now with two approvals.