learnweb / moodle-lifecycletrigger_byrole Goto Github PK

I would suggest an a bit more readable version of the if statement nesting in handle course.
Either:

if ($intable === false && $hasrole === false) {
  \\ Code
} else if ($intable === false && $hasrole === false) {
  \\ Code
} else if ($intable === false && $hasrole === false) {
  \\ Code
}
return false;

or

if ($intable === true) {
    if ($hasrole === true) {
        \\ Code
    } else {
        \\ Code
    }
} else {
    if ($hasrole === true) {
        \\ Code
    } else {
        \\ Code
    }
}

If a course has only teachers which are enrolled by category enrolment, this course is detected by this trigger.

This enrolment is not found under the contextid of this course, but under the contextid of the category. 😞

The name byrole_test sound as if it would be a testcase itself.

I think it is better to create a new database field courseid. Usually in moodle each table has an id field, which is just a sequence. Then you no longer need to set custom sequence to true in your DB calls.
To still guarantee that courseid is unique in your database table, you should create a unique key for this column.

Can you specify the PHPDoc for the two parameters? Especially for $hasrole.
Further scenario 1 of this function is difficult to understand. What is meant with "the course has no role"?

We already discussed about this earlier. I am still not satisfied with the way handle_course() first removes the database record from your db table and then returning true, without the guarantee that the process is actually triggered.
I would suggest to add events to the core API, when a process for a course is triggered. Then you could listen for this event and remove the db entry only in this case.

You should apply a validation to the settings page that at least one role has to be selected.
Additionally, you should catch the exception thrown in get_roles() within check_course(), which should result in an error log and returning next without calling handle_course().