search

studentquiz / moodle-mod_studentquiz

Moodle-Plugin
GNU General Public License v3.0
38 stars 38 forks source link

Clean update bad data when upgrading. #412

Closed vuvanhieu143 closed 2 years ago

timhunt commented 2 years ago

Thanks for working on this Hieu. It looks to me that you have coded all the necessary clean-up, and it looks like it should work.

However, as I was reviewing it, I thought I could code some of the queries a bit more neatly, so I tried doing that (particularly because of point 2. below). My extra changes are in https://github.com/timhunt/moodle-mod_studentquiz/commit/wip-upgrade-fixes. (It may just be easier to read the final code, rather than the diff: https://github.com/timhunt/moodle-mod_studentquiz/blob/wip-upgrade-fixes/db/upgrade.php#L990.)

To explain what I did - roughly in order of the changes in the patch, rather than importance.

  1. I updated some of the comments.
  2. I was concerned by your queries at the end of the clean-up, which deleted data from the core_question DB tables without any check involving mod_studentquiz. This scares me, because if it went wrong, it could cause bad dataloss. The only way I could think of to handle this in a way that was safer (or seemed safer to me) was to start by marking all the studentquiz_question rows that we want to delete. I did this by setting studentquizid = -1 for those questions.
  3. This then allowed quite a lot of the other queries to be simplified, which is probably a good thing.
  4. And then, and the end, we can be sure to clean up only those bits of core question data which were once related to studentquiz, which hopefully deals with Point 2. above.
  5. I found a way to write all the queries wihout needing MySQL-specific code. At least, according to the testing I did, all the queries work in MySQL, Postgres, MS SQL and Oracle at http://sqlfiddle.com/#!9/18ee4b.

I hope this is right. Fundamentally, it is a small change on top of what you had worked out.

However, I am not set up to test this. Therefore, please could you:

  1. Review my changes, to ensure they make sense to you, and that I have not made any silly mistakes.
  2. Test them again please, in the same way that you tested your changes.

If they are all good, I think you should squash my changes into yours, so they go in as a single commit. (And, feel free to made any further improvements that you can think of while you do.)

Thanks for your collaboration on this.

vuvanhieu143 commented 2 years ago

Hi @timhunt , Thank you for your changes, I have reviewed and squash into a single commit. I have tested on mysql, mariadb and postgresql. It is working great. Could you please take a look and peer-review again?