Comments

• 

  John Okely

  Wed, 2 Nov 2016, 4:23 PM

  Hi Andrew. Thanks for sharing this plugin with the community. I am going to review it to see if it's ready for the plugins database or if there's any work required.
• 

  Plugins bot

  Wed, 2 Nov 2016, 4:49 PM

  Prechecker results: 124 errors/18 warnings
  https://integration.moodle.org/job/Precheck%20remote%20branch/29004/artifact/work/smurf.html
• 

  John Okely

  Thu, 3 Nov 2016, 4:45 PM

  Thanks for sharing this plugin with the Moodle community! This is a helpful tool that will help a lot of institutions.
  
  I have a few comments here, but I have gone into more detail on your github issue tracker. So feel free to just read there as it will be easier to just address each one in turn. One thing I did notice that I didn’t create an issue for is it does delete data for modules regardless of what the completion criteria is. E.g. if a module is not required for completion, the submissions for that module are still deleted. But that is okay, we just need to make it clear in plugin text and documentation
  
  There is a CSRF vulnerability: Currently a malicious attacker could link a student to reset_user_completion.php and if the unsuspecting student clicks the link they will have all their work in the course destroyed. To solve this, block_resetcompletion.php should pass the sess key to reset_user_completion.php and reset_user_completion.php should verify that sess key. For more information see https://docs.moodle.org/dev/Security:Cross-site_request_forgery
  
  reset_user_completion.php does not check if the user has completed the course like block_resetcompletion.php does. So a user can remember the url to delete their course early.
  
  The text mentions that it deletes completion data, however it also deletes quiz attempts, choice submissions etc. Perhaps the text could be modified to say that it will also delete all submitted data.
  
  Currently this plugin manually deletes content from choice, scorm and quiz. These plugins, although installed by default are not required to run moodle. So there's no guarentee they will be there. On a related note, assignment submission data, forum posts etc are not deleted even though they affect course completion too. This is probably the hardest problem to solve with this plugin. We need to be able to remove submission data in a consistent way across modules without knowledge of how the module stores submission information (so we can support other plugins besides core one). Course reset is a good example of a place we do this in core, however there would be work required to make it not unenrol students, only do one student at a time etc.
  
  It would be great to have a confirmation screen once you click reset, since it will remove all the participant’s work. It would be a shame if a simple miss click or curious click would end in disaster.
  
  It is encouraged to follow Moodle’s coding style as outlined in: http://docs.moodle.org/dev/Coding_style and http://docs.moodle.org/dev/Coding The code checker plugin can be quite helpful in fine tuning your code and can be found at: https://moodle.org/plugins/view.php?plugin=local_codechecker You may wish to consider using that tool to further improve your plugin.
  
  I noticed that the recommended boilerplate was not used in the version.php file. The boilerplate is recommended at the beginning of each file and makes explicit the GPL license. You may want to review http://docs.moodle.org/dev/Coding_style#Files to learn more about the boilerplate comments.
  
  I noticed that the php files do not have copyrights. It is preferred that all of the files that you have created for this plugin have your name added as a copyright holder.
  
  There are no language strings for some of the text used in this plugin (see block_resetcompletion.php). It would be helpful to create strings for these so the plugin can be translated into other languages.
  
  For now, I am going to mark this plugin as needing more work until we get these issues resolved. Thanks for your patience with the review and approval process. I hope my review was helpful and let me know if I can provide any further insight into how we can get this plugin ready for the plugins DB.
• 

  Andrew P

  Tue, 8 Nov 2016, 3:25 AM

  Thank you for the detailed review and feedback.
  I've resolved the issues that you brought up here/submitted on Github, with the exception of "Plugin dependency & missing plugin support (https://github.com/ajhpark/reset-completion/issues/5)"
  I'm still looking into ways to delete user submitted data without worrying about how the module stores the information, as you mentioned in the Github ticket. It will probably involve modifying/using the reset_course_userdata function.
  For now, this block's been tested to work for courses that do not have forum posts or assignments as completion reqs. If anyone needs to reset assignments or forum posts, or any third party module, they can add $DB->delete_records_select() for those modules' tables, like how I am doing now with quizzes and choices.
  
• 

  John Okely

  Tue, 8 Nov 2016, 4:17 PM

  Thanks Andrew. The important points have been fixed. There are a few things remaining, so I left comments on the issues on github. But nothing remains that should block this from the plugins DB
  
  As you say, this plugin is useful, even though it’s plugin support is incomplete. Could you add to the description the list of modules that are supported so those that come to use your plugin will know?
  
  A simpler solution that allows this plugin to work if some plugins are not installed is just to check if each table exists first https://github.com/ajhpark/reset-completion/issues/9. That will solve one half of the plugin-plugin dependency problem in a simple/easy way
• 

  John Okely

  Tue, 8 Nov 2016, 4:32 PM

  If you add scorm, quiz and choice as dependencies for this plugin, this will be accepted into the plugins DB. See https://docs.moodle.org/dev/version.php for info on how to do that.
  
  For a later time, if you can move towards using more and more of our APIs rather than manual DB deletion, although I understand that may not always be possible/existant