[Issues #172 and 174] Improved course completion condition - #176
[Issues #172 and 174] Improved course completion condition#176Fragonite wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy single-course completion conditions with a new multi-course “Course completion” condition, and adds an upgrade step intended to migrate existing rule condition records to the new format.
Changes:
- Introduces
course_completioncondition supporting multiple courses, “any/all” selection, and before/after date filtering. - Adds an upgrade step to migrate legacy
course_completed/course_not_completedcondition records to the new condition/config format. - Removes legacy condition classes and their unit tests, replacing them with a new
course_completiontest suite.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
version.php |
Bumps plugin version/release for the upgrade step. |
db/upgrade.php |
Adds migration from legacy course completion conditions to the new course_completion condition. |
lang/en/tool_dynamic_cohorts.php |
Replaces legacy condition strings with new course_completion strings. |
classes/local/tool_dynamic_cohorts/condition/course_completion.php |
New condition implementation (form UI, description, SQL generation, broken checks, event subscriptions). |
classes/local/tool_dynamic_cohorts/condition/course_completed.php |
Removes legacy “course completed” condition. |
classes/local/tool_dynamic_cohorts/condition/course_not_completed.php |
Removes legacy “course not completed” condition. |
tests/local/tool_dynamic_cohorts/condition/course_completion_test.php |
Adds unit tests for the new course_completion condition. |
tests/local/tool_dynamic_cohorts/condition/course_completed_test.php |
Removes legacy tests. |
tests/local/tool_dynamic_cohorts/condition/course_not_completed_test.php |
Removes legacy tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
67cd783 to
45d1100
Compare
Fragonite
changed the title
|
Hi @dmitriim this is ready for review. I have looked into the test failures and they are unrelated to this patch. |
Done — I pushed commit |
| $string['condition:course_completion:have_completed'] = 'have completed'; | ||
| $string['condition:course_completion:have_not_completed'] = 'have not completed'; | ||
| $string['condition:course_completion:malformed'] = 'Condition configuration is malformed.'; | ||
| $string['condition:course_completion:missingcourse'] = 'A configured course no longer exists.'; |
There was a problem hiding this comment.
I was testing your last changes.
In the following example I have 3 courses configured for the condition. I deleted one of the courses and now I see the following error.
It's informative - no doubt, but really doesn't give you all information you would like to have:
- Missing other configured courses.
- Doesn't actually say what course no longer exists. This is important as you may would like to know what was set up and now is broken.
I'd recommend to modify the message so it has all required information - error, exact course id.
There was a problem hiding this comment.
Updated, it now reports all errors found along with relevant info such as course id.
| $string['condition:course_completed_description'] = 'Users who have completed course "{$a->course}" {$a->operator} {$a->timecompleted}'; | ||
| $string['condition:course_not_completed'] = 'Course not completed'; | ||
| $string['condition:course_not_completed_description'] = 'Users who have not completed course "{$a->course}"'; | ||
| $string['condition:course_completion:completionisdisabled'] = 'Completion tracking is disabled for a configured course.'; |
There was a problem hiding this comment.
Same here
I have one of the courses with disabled completion, but the message doesn't show me exact course. Would be nice to modify the message and bring all details.
There was a problem hiding this comment.
@Fragonite it still doesn't show me all configured courses. Only displays one with the error. Which is misleading. Also if you're really trying to make it useful and consistent, I'd recommend to stick with the same display pattern as you do for the courses with enabled completion: display name + link to the course. So the "broken" course still exists and we will provide a link to fix it. Also it will be easy to identify the course id you'd like to remove it form the list of configured course. With just ID it's hard to find out what course is actually broken.
There was a problem hiding this comment.
Also when I try to fix the condition it still let me to save it even one of the courses has disabled completion.
There was a problem hiding this comment.
I think we should fail validation and let the user what course is missing completion setting.
|
@Fragonite can you please update README to reflect changes? |
|
@Fragonite can you please fix CI complains related to this change? |
1f7bd3d to
f61b15a
Compare
ed409d7 to
30fb761
Compare
|
@dmitriim CI is green and the readme has been updated. |
This enhanced condition allows selecting multiple courses at once, improving performance and UX.
It migrates legacy course completion entries to the new format.
Resolves issues #172 and #174.