WR-437576 disable and delete users #1
Conversation
Wrote disable account logic for student roles wrote unit tests but unit tests need to be fixed
code quality changes
This reverts commit 9fcbb2d.
There was a problem hiding this comment.
Thanks Waleed,
Generally the functionality we have so far works as it stands, but I think there are some changes required to ensure this is suitably generalised.
In particular, as indicated by some of my comments, the concept of a "student" is quite a fluid thing in Moodle, so the way we are currently deciding which accounts to disable/delete with roles is not universally applicable.
I would suggest that rather than using a list of roles for users who are or aren't included, we should have a "protected account" capability which is assigned by default to "teacher", "editingteacher", "manager" and "admin" archetypes. You can then check whether the user has that capability in any context. This will ensure that any roles based on these archetypes will be protected by default, but allows full control over which roles will or wont be expired by the task. This will also require further documentation, and possibly some information on the settings page to indicate which roles have the capability.
As an extension of this, I think we should reconsider the name of the plugin. disable_delete_users or disable_delete_accounts is probably more appropriate than disable_delete_students.
|
|
||
| ## Testing | ||
|
|
||
| The plugin includes PHPUnit tests to ensure functionality. To run the tests, navigate to the Moodle root directory and execute: |
| // You should have received a copy of the GNU General Public License | ||
| // along with Moodle. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| /** |
There was a problem hiding this comment.
You don't need a separate file-level docblock in a class file, it can be merged with the class's docblock.
| // You should have received a copy of the GNU General Public License | ||
| // along with Moodle. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| /** |
There was a problem hiding this comment.
As above, these docblocks can be merged.
| // You should have received a copy of the GNU General Public License | ||
| // along with Moodle. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| /** |
| * @package tool_disable_delete_students | ||
| * @category test | ||
| */ | ||
| class tool_disable_delete_students_generator extends testing_module_generator { |
There was a problem hiding this comment.
It doesn't look like this class is necessary? It doesn't appear to be used anywhere, and I don't think the methods belong in a generator class. If you need to do repeated set up operations like this, you can define them as methods in the test class. If I'm correct, the whole file can be removed.
| * @package tool_disable_delete_students | ||
| * @category test | ||
| * @copyright 2024 Catalyst IT {@link http://www.catalyst-eu.net/} | ||
| * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later |
There was a problem hiding this comment.
This class should have a @covers tag, which should be:
@covers \tool_disable_delete_students\task\cleanup_students
| * Verifies that student accounts are disabled correctly based on | ||
| * the course end date criteria. | ||
| */ | ||
| public function test_disable_after_course_end(): void { |
There was a problem hiding this comment.
This test should include a second user who is enrolled in another course that is not past the suspension threshold, to verify that only the correct user is suspended.
It should also assert that $student->suspended is false before the task is run, to prove that it is the task that changes this.
| * Verifies that student accounts are disabled correctly based on | ||
| * the account creation date criteria. | ||
| */ | ||
| public function test_disable_after_creation(): void { |
There was a problem hiding this comment.
As above, this should include a second user who is not past the creation date threshold.
| * Verifies that student accounts are deleted correctly based on | ||
| * the defined criteria for account deletion. | ||
| */ | ||
| public function test_delete_after_months(): void { |
There was a problem hiding this comment.
As above, this should test a second user who does not meet the deletion criteria.
| * Verifies that users with excluded roles are protected from | ||
| * account disabling and deletion processes. | ||
| */ | ||
| public function test_excluded_roles_protection(): void { |
There was a problem hiding this comment.
This should include a second user on the same course without a protected role.
No description provided.