search

turnitin / moodle-plagiarism_turnitin

Turnitin Plagiarism plugin for Moodle
http://www.turnitin.com
GNU General Public License v3.0
45 stars 69 forks source link

Database schema inconsistent with clean install #517

Closed aspark21 closed 1 month ago

aspark21 commented 4 years ago

Run: php admin/cli/check_database_schema.php

Database schema inconsistent with clean install. Running this for a core issue in 3.9 where there were some steps missing from the db/upgrade.php compared to the install.xml (MDL-69049) we have picked up on the following inconsistencies with this plugin:

-------------------------------------------------------------------------------
plagiarism_turnitin_files
 * column 'student_read' should allow NULL (I)
-------------------------------------------------------------------------------
plagiarism_turnitin_users
 * column 'userid' should be NOT NULL (I)
 * column 'turnitin_uid' should be NOT NULL (I)
 * column 'turnitin_utp' should be NOT NULL (I)
-------------------------------------------------------------------------------
plagiarism_turnitin_courses
 * column 'courseid' should be NOT NULL (I)
 * column 'ownerid' should be NOT NULL (I)
 * column 'turnitin_ctl' should be NOT NULL (X)
 * column 'turnitin_cid' should be NOT NULL (I)
golenkovm commented 4 years ago

Yeah, looks like this inconsistence comes from these commits: https://github.com/turnitin/moodle-plagiarism_turnitin/commit/8148c4772c2e371c70239dd81d1ebd53bfc2edf1 https://github.com/turnitin/moodle-plagiarism_turnitin/commit/21883cf6b01987288d93d07a50534558c2f8dcd3

The plagiarism_turnitin_files table is fixed in this commit: https://github.com/turnitin/moodle-plagiarism_turnitin/commit/24c781a57da4bdc5fc8252e34ca25af5adf09e4e

jmcgettrick commented 4 years ago

@aspark21 - see https://github.com/turnitin/moodle-plagiarism_turnitin/pull/542

aspark21 commented 3 years ago

Sorry, haven't really had a chance to go through this in any detail and about to go on leave but this seems to roughly be doing what's needed - i.e. either fix install.xml or upgrade.php for fields and then have an upgrade script which cleans things up.

Would it be worth us upgrading on one of our test sites? If so, @cybernotic can pick up while I'm away

aspark21 commented 3 years ago

I've just run an upgrade with develop branch of plagiarism plugin (+ PR 553 which is not relevant to this issue but is for our own pre-3.9 testing)

plagiarism_turnitin_users

aspark21 commented 3 years ago

Just had a look back at https://github.com/turnitin/moodle-plagiarism_turnitin/pull/542/files#diff-fcb317575e9e6245fdf1590e51a86685b61c2cc9573a9df8034323ccb3ef614fR447 and it does seem like there was no upgrade steps for those 2 fields.

Might have been intentional as I can see it was reverted here - https://github.com/turnitin/moodle-plagiarism_turnitin/pull/542/commits/e009474ed340bc5bd541ab8fdc756808f6ff0566

golenkovm commented 3 years ago

Yeah, I got the same @aspark21 userid and courseid columns still allow NULLs because there was no update step to alter then, just https://github.com/turnitin/moodle-plagiarism_turnitin/pull/542/commits/e009474ed340bc5bd541ab8fdc756808f6ff0566 that changes existing upgrade steps which are never called twice, so this didin't work for existing sites. I'm happy to make a new PR for to fix remaning columns @jmcgettrick

golenkovm commented 3 years ago

Hey @aspark21 @jmcgettrick could you please check if #585 would work for you? I had to drop and recreate a unique index in these columns because moodle has a dependency check and throws column "plagiarism_turnitin_users->userid" cannot be modified. Dependency found with index "mdl_plagturnuser_use_uix (userid)"

aspark21 commented 3 years ago

I'll have a look

golenkovm commented 3 years ago

Hi @aspark21 Have you had a chance to have a look?

golenkovm commented 2 years ago

Hey @aspark21 @jmcgettrick Is there a chance you could have a look at #585 ? I've rebased my pull branch.

Cheers, Mikhail

golenkovm commented 2 years ago

Hi @aspark21 @jmcgettrick

Please feel free to let me know if there anything I can help with this one. The PR seems straightforward, but if there is anything you'd want to discuss please let me know. I'm looking forward to seeing this integrated.

Kind regards, Misha

aspark21 commented 2 years ago

Ah yes sorry this dropped off my radar, I'll try and look at it this week.

golenkovm commented 2 years ago

Thank you @aspark21

golenkovm commented 2 years ago

Hi @aspark21

Sorry for chasing you, but we have an open ticket wich I can't close untill this is fixed. Is there any chance to have a look at the issue? If there anything that needs to be changed in the PR I'm happy to do so.

Kind regards, Misha

aspark21 commented 2 years ago

I've finally taken a look apologies for delay.

Starting point: Moodle 3.9.11+ & plagiarism_turnitin 2021060801

-------------------------------------------------------------------------------
plagiarism_turnitin_users
 * column 'userid' should be NOT NULL (I)
-------------------------------------------------------------------------------
plagiarism_turnitin_courses
 * column 'courseid' should be NOT NULL (I)
 * column 'ownerid' is not expected (I)
 -------------------------------------------------------------------------------

Upgraded to latest plagiarism_turnitin origin master (2021091501)

-------------------------------------------------------------------------------
plagiarism_turnitin_users
 * column 'userid' should be NOT NULL (I)
-------------------------------------------------------------------------------
plagiarism_turnitin_courses
 * column 'courseid' should be NOT NULL (I)
-------------------------------------------------------------------------------

Upgraded to golenkovm/issue517_v2

No more warnings from php admin/cli/check_database_schema.php

Unfortunately John has left Turnitin so it looks like @dominicgunn or @stvleung as the current owners of the Turnitin Github will have to review instead

aspark21 commented 2 years ago

Can always try @dwinn but is a long shot, he might also have left.

golenkovm commented 2 years ago

Thanks @aspark21 for having a look

golenkovm commented 2 years ago

Hey @dominicgunn @stvleung @dwinn

Can somebody please merge this PR?

Kind regards, Misha

golenkovm commented 2 years ago

Hi @dominicgunn @stvleung @dwinn

Is there a chance you could have a look at this PR? Thanks in advance. Appreciate this.

Kind regards, Misha

golenkovm commented 2 years ago

Hi @dominicgunn @stvleung @dwinn

Please feel free to let me know if I can do anything to help you with this PR. Appreciate this.

Kind regards, Misha

golenkovm commented 2 years ago

Please, anyone @dominicgunn @stvleung @dwinn

golenkovm commented 2 years ago

585 has been rebased.

I also created #627 for develop branch.

dwinn commented 2 years ago

Hi @golenkovm , Thank you for reporting the issue and the pull request, we will try to get this released as soon as we can.

carl-hostrander commented 1 month ago

Thank you for reporting this. Because the latest version of the plagiarism plugin is supported for versions of Moodle 4.1 and higher, I am closing this ticket. However, if you find this issue is occurring with the latest version of the plugin in any of the supported Moodle versions, please create a new ticket and we will address it.