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.

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

Database schema inconsistent with clean install #517

Closed
aspark21 opened this issue Jun 19, 2020 · 24 comments
Closed

Database schema inconsistent with clean install #517

aspark21 opened this issue Jun 19, 2020 · 24 comments

Comments

@aspark21
Copy link

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
Copy link
Contributor

Yeah, looks like this inconsistence comes from these commits:
8148c47
21883cf

The plagiarism_turnitin_files table is fixed in this commit:
24c781a

@jmcgettrick
Copy link
Contributor

@aspark21 - see #542

@aspark21
Copy link
Author

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
Copy link
Author

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

  • column 'userid' should be NOT NULL (I)

plagiarism_turnitin_courses

  • column 'courseid' should be NOT NULL (I)

@aspark21
Copy link
Author

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 - e009474

@golenkovm
Copy link
Contributor

golenkovm commented May 24, 2021

Yeah, I got the same @aspark21
userid and courseid columns still allow NULLs because there was no update step to alter then, just e009474 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
Copy link
Contributor

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
Copy link
Author

aspark21 commented Jun 7, 2021

I'll have a look

@golenkovm
Copy link
Contributor

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

@golenkovm
Copy link
Contributor

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

Cheers,
Mikhail

@golenkovm
Copy link
Contributor

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
Copy link
Author

aspark21 commented Dec 7, 2021

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

@golenkovm
Copy link
Contributor

Thank you @aspark21

TomoTsuyuki added a commit to catalyst/moodle-plagiarism_turnitin that referenced this issue Jan 12, 2022
@golenkovm
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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

@golenkovm
Copy link
Contributor

Thanks @aspark21 for having a look

@golenkovm
Copy link
Contributor

Hey @dominicgunn @stvleung @dwinn

Can somebody please merge this PR?

Kind regards,
Misha

@golenkovm
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Please, anyone @dominicgunn @stvleung @dwinn

@golenkovm
Copy link
Contributor

#585 has been rebased.
I also created #627 for develop branch.

@dwinn
Copy link
Contributor

dwinn commented Jul 18, 2022

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
Copy link

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.

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

No branches or pull requests

5 participants