I spend some time reading through the code and trying out the plugin, it's a great job, thanks everyone!
My main concerns about the code lead more towards the maintainability and robustness of the code. Currently it is really procedural and there are bits and pieces of code duplicated in different areas (lib, index.php, CLI), they could certainly be merged into one single place. Also, the CLI and the UI differ a bit in their implementation, and would be hard to maintain as both side should benefit any further improvement.
I would highly recommend to wrap all the functionality in a class, which would include its own constants, different methods for each different operations (detect enrolment settings, restore a course, reset, ...), keep track of the amount of courses updated/deleted/created, the upload mode, etc.... It would also be easier to perform the checks like "If this course exists, then update the shortname and consider that it does not exist", and easier to perform the capability checks for specific fields (moodle/course:changecategory, moodle/course:changeidnumber, moodle/course:changeshortname) if need be.
But most importantly, there is currently no Unit Test coverage, and while it would surely be possible to write some basic tests with the existing code, having a class would make it a lot easier and granular.
In regard with the current functionalities, I wonder if we should really allow for course deletion as part of uploadcourses tool. Perhaps deletion should be a sub part of the plugin and completely distinct from the upload part. But then, maybe the module should be renamed 'Bulk course operations' or something... food for thoughts.
About the current patch
Here are some comments based on a use in the latest 2.6-dev.
Need unit testing
We do need a serious unit testing to prevent horrible things happening on existing courses!
Use of deprecated objects
- make_categories_list() is deprecated.
- get_context_instance() is deprecated.
- PARAM_MULTILANG is deprecated.
I am not sure about the current policy in regard with course restore and the capabilities associated to it, but should there be more granular checks when updating a current course? Some examples are:
Also, at the beginning of the script, the user requires course:create, course:update and course:delete but that might be too restrictive if the user is not trying to delete or update courses.
More importantly, the tool needs to define its own capabilities and stop using 'moodle/site:uploadusers'. Also in settings.php, we should definitely not make use of 'moodle/restore:restorecourse', but of the soon-to-be-introduced capability.
- What is $bulk? It does not seem to be ever set to something else than 0.
- standardshortnames is quite ambiguous. What does it do? Shouldn't we ALWAYS clean the shortname?
- When is $backupfile not empty? And what is $roles in the logic associated to it?
- Is $CFG->keeptempdirectoriesonbackup relevant here, as far as I'm concerned, we are not really backuping a course, we're using this method to restore some content.
- The case where $optype === CC_COURSE_ADDNEW then if ($existingcourse) should never happen. Around line 500.
- In form::get_data() what is this description field?
Here are some comments on things that I think we should consider. Also, as this script could eventually process a lot of courses, keeping in mind performances is important.
- I am not a fan of oldshortname, could we consider keeping the existing shortname in its column and having another column rename_shortname_to or new_shortname?
- templatename considers none as empty, this should be avoided and replaced with a proper empty value.
- It seems quite expensive to extract a backup just to make sure it's valid. Shouldn't we extract it once and for all as it's going to be used by each course?
- I dislike enrolment_1 then using enroloption_1, I think it's more clear to use enrolment_[0-9]<option>. That would also allow for more same settings of the sort _anotherthing_1_option later on.
- Role renaming should be applied to all existing roles, not only the default ones.
- Deleting the course could happen right after checking if the course exists, rather than later.
- Support multilang filter. Ie: <span lang="en" class="multilang">English</span><span lang="fr" class="multilang">French</span> Cat. Detecting the course category does not support it.
- Speaking of categories... even though category IDs are harder to work with, they would solve any potential issue in resolving the path. Also we could have an option to fallback on the default one if the ID is not found. (Or category ID number, or one or the other)
- In the same was than 'Allow deletes' or 'Allow renames', there should be an 'Allow resets'.
- We should clearly define a list of mandatory fields for each upload type.
- There is a die() in the middle of the process when a backup could not be restored for some reason. I don't think we should halt the whole process, but perhaps just skip the current course.
- Do we really want to restore from another course, and then restore from a course file, and then reset eventually? Those 3 actions can be executed in a row.
- When restoring from a file, do we ensure that we do not restore user data? If not, should we?
- We usually only have one @copyright per file, the person who created the file writes his name there and it does not change: http://docs.moodle.org/dev/Coding_style#.40copyright
- Could use _DIR_ with ../ instead of dirname().
- We do not use underscores in variable names.
- in if (in_array($field, $stdfields) or in_array($lcfield, $stdfields)) the later is enough, our fields should never use caps.
- Adding support for Summary files (new in 2.5)
- Form headers addElement('header') should have a name in order to be collapsible.
- ccshortname help button mentions to view the 'help' for examples, what help?
- There should not be any default 'grouping' as they are defined in the course itself.
- Do we need to flush some cache after/during the process?
- The dropdown to select a course to use as a template could potentially contain hundreds of courses, it would make it unusable. Perhaps, we should not support this and rely on the uploaded restore file only.
- The default course settings should reflect the new tree/structure/sections/labels/values introduced in Moodle 2.5
- The use of file as a reference to the CSV file is rather confusing, I often asked myself if we were referring to the restore file or the CSV. Rewording my help here (applies to UI and CLI).
- Strings in the lang file should ideally be alphabetically ordered
- It might be safer for later use to place the defaults values in the form in their respective array instead of mixing them with the options: $formdata['defaults']['foo'] = 'bar'; And if so, perhaps the function tool_uploadcourse_process_course_upload() could get the defaults as an argument.
- CC* variables and constants should be renamed to match the frankestyle name of this tool.
- The form should use setType() on 'ccshortname' and 'contextid'
- In tool_uploadcourse_process_template(), the letters flu do not match sfi in the callback function tool_uploadcourse_process_template_callback().
I am willing to take over the work to be done from here, but we first need to decide whether we want to change the structure of this code or not?
Thanks everyone for your involvement in this issue!