-
Notifications
You must be signed in to change notification settings - Fork 19
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
Autogroup Bug #17
Autogroup Bug #17
Conversation
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.
Thanks again for sharing this fix.
Not tested (ping @emmarichardson could you test it ?) but reading code it should be OK.
Ideally could you :
- amend your commit and change the message for a more relevant one
- review my comments and fix them.
* @param int $new_group_id | ||
* @param \moodle_database $db | ||
*/ | ||
private function check_forums($user_id, $old_group_id, $new_group_id, \moodle_database $db){ |
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.
I think that a stuff like update_forum_group
would be a more relevant name. check_forums
suggests that this function is a readonly which "only" check forum consistency.
@@ -73,21 +73,23 @@ public function __construct ($group, \moodle_database $db) | |||
|
|||
/** | |||
* @param int $userid | |||
* @return bool |
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.
originally this function return void. If you change it to do conditional return (return true if changes happened and false otherwise) you should detailed it in the PHPDoc.
} | ||
|
||
/** | ||
* @param int $userid | ||
* @return bool |
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.
originally this function return void. If you change it to do conditional return (return true if changes happened and false otherwise) you should detailed it in the PHPDoc.
closing in favor to #22 |
No description provided.