Add quiz migration #7123

m1r0 · 2023-08-24T12:45:15Z

Resolves #5438

Proposed Changes

Add a migration that migrates all the quiz data into the custom tables.
Update the migration job scheduler to run multiple jobs, one after another.
Refactor the migrations code a bit.

Testing Instructions

Have some quiz entries.
Enable the feature: add_filter( 'sensei_feature_flag_tables_based_progress', '__return_true' );.
Go to Sensei LMS -> Tools.
Run Migrate comment-based progress.
It should first migrate the student progress progress and then run the quiz migration.
Check the quiz tables and verify that the data is correct.
To rerun the migration, truncate the tables and delete the sensei_migrated_quiz_last_comment_id option.

Pre-Merge Checklist

github-actions · 2023-08-24T12:52:29Z

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit d8ec1c6 and trunk. Please review and confirm the following are correct before merging.

No changes detected in the current commit. But the comment was left so it is possible to check for the edit history.

This comment was automatically generated by the github-action-wordpress-dependencies-report action.

codecov · 2023-08-29T20:44:59Z

Codecov Report

Merging #7123 (d8ec1c6) into trunk (c3b56a7) will increase coverage by 0.12%.
Report is 96 commits behind head on trunk.
The diff coverage is 80.83%.

@@             Coverage Diff              @@
##              trunk    #7123      +/-   ##
============================================
+ Coverage     49.47%   49.60%   +0.12%     
- Complexity    10569    10609      +40     
============================================
  Files           578      580       +2     
  Lines         44602    44791     +189     
  Branches        402      402              
============================================
+ Hits          22069    22220     +151     
- Misses        22206    22244      +38     
  Partials        327      327

Files Changed	Coverage Δ
includes/class-sensei.php	`22.98% <0.00%> (-0.16%)`	⬇️
...es/internal/migration/class-migration-abstract.php	`0.00% <0.00%> (ø)`
...cludes/internal/migration/class-migration-tool.php	`100.00% <ø> (ø)`
...on/migrations/class-student-progress-migration.php	`82.97% <66.66%> (ø)`
...rnal/migration/migrations/class-quiz-migration.php	`79.88% <79.88%> (ø)`
...ternal/migration/class-migration-job-scheduler.php	`100.00% <100.00%> (ø)`
...ncludes/internal/migration/class-migration-job.php	`100.00% <100.00%> (ø)`

... and 10 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3b56a7...d8ec1c6. Read the comment docs.

merkushin

Thanks @m1r0!

Looks working. I left a few minor comments. None is blocking.

Now we need the verification tool to be sure everything migrated properly 😅

merkushin · 2023-09-01T11:09:29Z

includes/internal/migration/class-migration-job.php

 		$this->migration = $migration;
-		$this->job_name  = 'progress_migration';
+		$this->name      = strtolower(
+			( new ReflectionClass( $migration ) )->getShortName()


I'd like to replace this reflection with a public method in a migration.

Reflection is more expensive. But in our case it isn't so important.

That's an "implicit interface". Trying to find an analogy, I though up a terrible parallel that I don't want to share 😅 Anyway, I prefer explicit over implicit.
What do you think?

Makes perfect sense. Fixed in e7332eb.

merkushin · 2023-09-01T11:12:02Z

includes/internal/migration/migrations/class-quiz-migration.php

+	 *
+	 * @return string
+	 */
+	public function target_version(): string {


Initially, I though we'll use these migrations as part of the installation/update process.
But we decided it is important to do them manually.
Do we still need this method then? Maybe we can remove them?

Yeah, it's not needed so I've removed it in a993d34.

merkushin · 2023-09-01T11:21:03Z

includes/internal/migration/migrations/class-quiz-migration.php

+		);
+
+		if ( $this->dry_run ) {
+			echo esc_html( $comments_query . "\n" );


Is it supposed to be run in console or only in web?
esc_html is not a good escaping function for console :)
And for the web the line ending isn't suitable :)
However, we can consider that as a tradeoff.

Yeah, you're right. I've made it consistent with the other migration but we should revisit this once we have a CLI command that actually uses the dry run feature. At this stage, we can consider it as a placeholder. 🙂

merkushin

👍

m1r0 added 3 commits August 24, 2023 15:18

Move the migration logic to the "Migration" namespace

494e81e

Prepare the Migration_Job class to be used for the quiz migration

6fd4aba

Fix the return type of the Migration->run method

7fdc51c

m1r0 self-assigned this Aug 24, 2023

m1r0 added this to the 4.16.2 milestone Aug 24, 2023

m1r0 added HPPS No Changelog labels Aug 24, 2023

Refactor Migration_Job_Scheduler to schedule multiple jobs

39a3776

Base automatically changed from add/recurring-job to trunk August 28, 2023 12:56

Add the quiz migration

523bd38

m1r0 added 4 commits August 30, 2023 00:34

Move common migration logic to an abstract class

5e9d242

Improve the structure of the Quiz_Migration class

f7d503e

Add tests for Quiz_Migration

30d258b

Fix psalm issues

bef4b79

m1r0 marked this pull request as ready for review August 31, 2023 18:22

m1r0 requested a review from a team August 31, 2023 18:23

merkushin previously approved these changes Sep 1, 2023

View reviewed changes

m1r0 added 3 commits September 4, 2023 18:41

Make the migration job name explicit

e7332eb

Remove the leftover migration interface

e06db11

Remove the migration target_version method because it's not needed

a993d34

m1r0 dismissed merkushin’s stale review via a993d34 September 4, 2023 15:46

Fix tests

d8ec1c6

m1r0 requested a review from merkushin September 4, 2023 15:55

merkushin approved these changes Sep 4, 2023

View reviewed changes

m1r0 merged commit 10761b9 into trunk Sep 4, 2023
24 checks passed

m1r0 deleted the add/quiz-migration branch September 4, 2023 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quiz migration #7123

Add quiz migration #7123

m1r0 commented Aug 24, 2023 •

edited

Loading

github-actions bot commented Aug 24, 2023 •

edited

Loading

codecov bot commented Aug 29, 2023 •

edited

Loading

merkushin left a comment

merkushin Sep 1, 2023

m1r0 Sep 4, 2023

merkushin Sep 1, 2023

m1r0 Sep 4, 2023

merkushin Sep 1, 2023

m1r0 Sep 4, 2023

merkushin left a comment

Add quiz migration #7123

Add quiz migration #7123

Conversation

m1r0 commented Aug 24, 2023 • edited Loading

Proposed Changes

Testing Instructions

Pre-Merge Checklist

github-actions bot commented Aug 24, 2023 • edited Loading

WordPress Dependencies Report

codecov bot commented Aug 29, 2023 • edited Loading

Codecov Report

merkushin left a comment

Choose a reason for hiding this comment

merkushin Sep 1, 2023

Choose a reason for hiding this comment

m1r0 Sep 4, 2023

Choose a reason for hiding this comment

merkushin Sep 1, 2023

Choose a reason for hiding this comment

m1r0 Sep 4, 2023

Choose a reason for hiding this comment

merkushin Sep 1, 2023

Choose a reason for hiding this comment

m1r0 Sep 4, 2023

Choose a reason for hiding this comment

merkushin left a comment

Choose a reason for hiding this comment

m1r0 commented Aug 24, 2023 •

edited

Loading

github-actions bot commented Aug 24, 2023 •

edited

Loading

codecov bot commented Aug 29, 2023 •

edited

Loading