emmarichardson / local_autogroup

A local plugin for Moodle 2.7 onwards which handles the dynamic creation, population and cleanup of groups on courses.
6 stars 19 forks source link

Upgrading to v2.8 #44

Closed caiadogithub closed 1 year ago

caiadogithub commented 1 year ago

Moodle 3.11.12+ & Moodle 4.1.2 Postgres 12 PHP 7.4

Informações de depuração: ERROR: syntax error at or near "LAM" LINE 1: DELETE LAM FROM mdl_local_autogroup_manual LAM JOIN mdl_grou... ^ DELETE LAM FROM mdl_local_autogroup_manual LAM JOIN mdl_groups G ON (G.id = LAM.groupid) WHERE G.idnumber NOT LIKE $1 ESCAPE '\' -- line 97 of /local/autogroup/db/upgrade.php: call to pgsql_native_moodle_database->execute() [array ( 0 => 'autogroup|%', )] Error code: dmlwriteexception Rastreamento de pilha:

line 489 of /lib/dml/moodle_database.php: dml_write_exception thrown
line 293 of /lib/dml/moodle_read_slave_trait.php: call to moodle_database->query_end()
line 338 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->read_slave_query_end()
line 836 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
line 97 of /local/autogroup/db/upgrade.php: call to pgsql_native_moodle_database->execute()
line 703 of /lib/upgradelib.php: call to xmldb_local_autogroup_upgrade()
line 1929 of /lib/upgradelib.php: call to upgrade_plugins()
line 713 of /admin/index.php: call to upgrade_noncore()
emmarichardson commented 1 year ago

I am looking into this....

emmarichardson commented 1 year ago

I just ran a fresh install on 4.1.2 and an upgrade with no issue at all.

caiadogithub commented 1 year ago

Which DB are you using?

Same error on Moodle 3.11.12+ & Moodle 4.1.2+

emmarichardson commented 1 year ago

Mysql 7.3

emmarichardson commented 1 year ago

Make that 7.4.21

caiadogithub commented 1 year ago

The problem is related to PostgreSQL

emmarichardson commented 1 year ago

Sadly, I have zero experience with PostgreSQL...not sure if anyone else can assist @ak4t0sh?

caiadogithub commented 1 year ago

Thanks, Emma.

I have been using this plugin for a long time and it is very useful.

I never had any incompatibility issues because we use PostgreSQL.

Ricardo

emmarichardson commented 1 year ago

I have pinged the author of the recent pull request that I believe is causing this to see if he can figure it out.

Kemmotar83 commented 1 year ago

Hi @caiadogithub ,

you're right, there's an error in the sql query. I'll discuss with @emmarichardson in order to fix it asap.

Giorgio

caiadogithub commented 1 year ago

Hi @caiadogithub ,

you're right, there's an error in the sql query. I'll discuss with @emmarichardson in order to fix it asap.

Giorgio

Hi, Giorgio.

Any update on this?

Ricardo

izendegi commented 1 year ago

Hi,

The reason behind this issue may be that PostgreSQL doesn't support DELETE + JOIN sentences (they aren't SQL standard), so to fix this issue that query should be refactored.

Something like this will work on PostgreSQL:

_DELETE FROM mdl_local_autogroup_manual LAM WHERE LAM.groupid in (select G.id from mdlgroups g WHERE G.idnumber NOT LIKE 'autogroup|%')

Kemmotar83 commented 1 year ago

Hi @caiadogithub ,

@emmarichardson should have already fixed this bug and released a new version.

Could you please verify?

Giorgio

emmarichardson commented 1 year ago

@Kemmotar83 my bad - somehow I did not see your email but just found it in the trash. I will get it fixed today.

emmarichardson commented 1 year ago

Uploaded revised version. @izendegi Please report back after testing.

izendegi commented 1 year ago

Hi @emmarichardson

It actually doesn't work. As I mentioned in the previous message, the issue comes from using a JOIN clause inside a DELETE sentence, and that change doesn't change that.

Changing the SQL to this works in PG and I think it is standard SQL so it should still work in other databases:

DELETE FROM mdl_local_autogroup_manual LAM
 WHERE LAM.groupid in (select G.id from mdl_groups g WHERE G.idnumber NOT LIKE 'autogroup|%')
emmarichardson commented 1 year ago

Ok, I am still looking at this. I am having to uninstall completely to install an older version in order to test - I will try and get this done today or tomorrow.

caiadogithub commented 1 year ago

Hi @caiadogithub ,

@emmarichardson should have already fixed this bug and released a new version.

Could you please verify?

Giorgio

https://github.com/emmarichardson/local_autogroup/issues/46

emmarichardson commented 1 year ago

@Kemmotar83 I made the changes that you mentioned in the email you sent me but that did not resolve it. The proposed solution above appears ok but needs to be tested on sql - your email only said to remove LAM from the DELETE line...

Kemmotar83 commented 1 year ago

Hi @emmarichardson , I will look at it immediately.

Kemmotar83 commented 1 year ago

@izendegi your solution doesn't work on MySQL. I'll find the right solution using only the native DB Manipulation API, as I should have done from the start.

Kemmotar83 commented 1 year ago

Ok,

this single query should work:

$DB->delete_records_select('local_autogroup_manual', 'groupid IN (SELECT id FROM {groups} WHERE idnumber NOT LIKE ?)', ['autogroup|%']);

@caiadogithub @izendegi Could you please check if this works on PostgreSQL? It should, since I've used the API.

I'm really sorry for this blocking issue.

caiadogithub commented 1 year ago

Ok,

this single query should work:

$DB->delete_records_select('local_autogroup_manual', 'groupid IN (SELECT id FROM {groups} WHERE idnumber NOT LIKE ?)', ['autogroup|%']);

@caiadogithub @izendegi Could you please check if this works on PostgreSQL? It should, since I've used the API.

I'm really sorry for this blocking issue.

Did you update the code on Moodle site?

emmarichardson commented 1 year ago

Not yet, have you actually tested it? "should work"... I need to confirm it works before I update the master and the plugins database..

caiadogithub commented 1 year ago

Not yet, have you actually tested it? "should work"... I need to confirm it works before I update the master and the plugins database..

Not tested yet. Waiting to update the code on Moodle site,

emmarichardson commented 1 year ago

I am not going to update the code on the Moodle site until it has been tested - can you not just manually change out the line and test it?

caiadogithub commented 1 year ago

I am not going to update the code on the Moodle site until it has been tested - can you not just manually change out the line and test it?

I had uninstalled the plugin on my Moodle site because the error message stopped any update process. And now I can't install it again.

emmarichardson commented 1 year ago

Why can't you install it again? Just manually edit the update file and see if it installs...you have a dev site right?

ak4t0sh commented 1 year ago

Hi @emmarichardson sorry I did not see this psql incompatibility during review (and thought you were testing both mysql/mariadb and pgsql). I am going to test it using pgsql and integrate it in a new release asap (today I think)

ak4t0sh commented 1 year ago

I can confim the patch provided by @Kemmotar83 work correctly on postgresql.

ak4t0sh commented 1 year ago

reopened until public release on moodle db

ak4t0sh commented 1 year ago

Fix in 2.8.2 now available in Moodle plugin database. https://moodle.org/plugins/local_autogroup/2.8.2/28892

emmarichardson commented 1 year ago

Thank you so much Arnaud - I did not even consider psql as it is fairly rare...

On Apr 6, 2023 at 3:16 AM -0600, Arnaud Trouvé @.***>, wrote:

Fix in 2.8.2 now available in Moodle plugin database. https://moodle.org/plugins/local_autogroup/2.8.2/28892 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

Kemmotar83 commented 1 year ago

Thank you @ak4t0sh for the test on psql.