Skip to content
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.

Sign up for GitHub

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

Fixes for Moodle 4.1 compatibility #153

Merged
merged 7 commits into from
Feb 6, 2023
Merged

Fixes for Moodle 4.1 compatibility #153

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9d7a24a
[#152] tests: Prevent unit test failures when run together
marxjohnson Dec 21, 2022
16e62b4
[#152] lib: Replace deprecated user fields function
marxjohnson Dec 21, 2022
dbcfb0b
[#152] users: Remove obsolete user profile fields
marxjohnson Dec 21, 2022
1fac15d
[#152] users: Skip user changes if no users are selected
marxjohnson Dec 21, 2022
e6cbf37
[#152] Bump version and supported versions
marxjohnson Feb 3, 2023
18bb0be
[#152] Add MOODLE_311_STABLE branch to README
marxjohnson Feb 3, 2023
75c7982
[#152] Update Github actions matrix for new stable branch
marxjohnson Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions cleaner/users/classes/clean.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ public static function execute() {

self::$idstoupdate = self::create_user_id_list_to_update();

if (empty(self::$idstoupdate)) {
echo "No users to update.\n";
return;
}

if (self::$options['dryrun']) {
echo "Dry run mode, no records were updated.\n";
return;
Expand Down Expand Up @@ -124,11 +129,6 @@ private static function set_fixed_fields() {
'picture' => 0,
'description' => '',
'lastip' => '',
'icq' => '',
'skype' => '',
'yahoo' => '',
'aim' => '',
'msn' => '',
'phone1' => '',
'phone2' => '',
'idnumber' => '',
Expand Down
2 changes: 1 addition & 1 deletion cli/lib.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ function safety_checks($dryrun) {
AND u.deleted = 0";

if ($DB->count_records_sql($csql, $params)) {
$namefields = "u." . implode(', u.', get_all_user_name_fields());
$namefields = "u." . implode(', u.', \core_user\fields::get_name_fields());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this break in older moodles where this doesn't exist yet? Either we need to detect the version and call the old function of pick a version and cut a new stable branch. It would be better have this in a unit test to expose that issue across versions too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function was introduced on Moodle 3.11, so you're right that it won't work on Moodle 3.10 which this branch supports (although it's no longer a supported release). Personally I prefer separate stable branches over version-detection code, as the latter feels like technical debt. Shall I create a new branch and change this PR's target?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup please cut a new stable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think you'll have to do that for me. I don't have write access and GitHub doesn't have a way for me to submit a new branch from my repo to this one. I think it would make sense to call it MOODLE_311_STABLE as this and the removed user fields both apply from 3.11 onwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the new branch as a clone of the 310 one so you can make new prs against that now thanks


$sql = "SELECT u.id, u.username, {$namefields}
FROM {user} u
Expand Down
13 changes: 13 additions & 0 deletions tests/unit/custom_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,19 @@
* @SuppressWarnings(public) Allow as many methods as needed.
*/
class local_datacleaner_custom_sql_test extends advanced_testcase {

/**
* Initialise a cleaner object to reset static options.
*
* This prevents impact on other tests which assume default options.
*
* @return void
* @throws coding_exception
*/
public function tearDown(): void {
new \cleaner_custom_sql_pre\clean(['verbose' => false, 'dryrun' => false]);
}

public function test_executes_sql() {
global $DB;

Expand Down