Skip to content

Safety check creates SQL aggregating error #195

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

Closed
ScottVerbeek opened this issue Mar 12, 2025 · 6 comments · Fixed by #197, #199 or #198
Closed

Safety check creates SQL aggregating error #195

ScottVerbeek opened this issue Mar 12, 2025 · 6 comments · Fixed by #197, #199 or #198
Assignees

Comments

@ScottVerbeek
Copy link
Contributor

The SQL query here https://github.com/catalyst/moodle-local_datacleaner/blob/MOODLE_311_STABLE/cli/lib.php#L106-L112

Uses a group by statement incorrectly. Changing the select to select distinct and removing the group by will resolve the issue.

@ScottVerbeek
Copy link
Contributor Author

The group by is actually completely redundant.

@picnicpete picnicpete self-assigned this Mar 24, 2025
@picnicpete picnicpete linked a pull request Mar 24, 2025 that will close this issue
@picnicpete
Copy link
Contributor

I had to fix a couple more little bugs to make the test run.
I did not create a unit test for this.

@ScottVerbeek
Copy link
Contributor Author

ScottVerbeek commented Mar 25, 2025

The changes applied in #197 were only applied to branch MOODLE_311_STABLE. To close also port these changes to master and MOODLE_310_STABLE.

EDIT: And branch TOTARA_13.

@picnicpete
Copy link
Contributor

There are pull requests for master and _310 branches. I have tested theses branches on my local docker-dev.
BTW: Those branches are looking very old. They have missed many updates that went to 311 branch. It might be time to close them off.

@ScottVerbeek
Copy link
Contributor Author

The pull requests are merged. The branches may be older. The scope of this issue is will focus on that SQL what we have done. Feel free to create a new issue to maybe close down some branches.

There is still one branch remaining: TOTARA_13

@picnicpete
Copy link
Contributor

Patches have been applied to master, MOODLE_310_STABLE, MOODLE_311_STABLE, and TOTARA_13 branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment