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

Add student progress migration #7049

Merged
merged 7 commits into from
Aug 10, 2023
Merged

Add student progress migration #7049

merged 7 commits into from
Aug 10, 2023

Conversation

merkushin
Copy link
Member

@merkushin merkushin commented Jul 31, 2023

Resolves #5437

  • This PR doesn't introduce a data migration mechanism. Only an interface for migrations and the migration for student progress comments.
  • It doesn't work with transactions at this moment. We decided to consider them later.
  • Only a very basic test added for the migration. Tests are WIP.

Proposed Changes

  • Add a migration to copy status comments into custom tables.
  • Add a tool to run migration manually.

Testing Instructions

  1. Enable the feature: add_filter( 'sensei_feature_flag_tables_based_progress', '__return_true' );.
  2. Go to Sensei LMS -> Tools.
  3. Run Migrate comment-based progress.
  4. You can run it several times. It processes 1000 comments per run.
  5. If you see the message "Progress entries created based on comments: 0." it means there is nothing to migrate already.
  6. To run it from the beginning you need to clean at least the option: DELETE FROM wp_options where option_name = 'sensei_migrated_progress_last_comment_id';
  7. However, it won't insert duplicates, so you need to delete progress rows as well: TRUNCATE TABLE wp_sensei_lms_progress;

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #7049 (d70ce71) into trunk (71616ad) will increase coverage by 0.16%.
Report is 2 commits behind head on trunk.
The diff coverage is 75.09%.

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7049      +/-   ##
============================================
+ Coverage     48.08%   48.25%   +0.16%     
- Complexity    10467    10534      +67     
============================================
  Files           573      575       +2     
  Lines         44164    44441     +277     
  Branches        402      402              
============================================
+ Hits          21235    21443     +208     
- Misses        22602    22671      +69     
  Partials        327      327              
Files Changed Coverage Δ
includes/class-sensei.php 23.41% <0.00%> (-0.12%) ⬇️
...al/student-progress/tools/class-migration-tool.php 31.03% <31.03%> (ø)
...er/migrations/class-student-progress-migration.php 80.89% <80.89%> (ø)

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 6738bcf...d70ce71. Read the comment docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to leave this interface here as is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to move this class to includes/internal/student-progress/migrations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't move yet, not sure.

@merkushin merkushin changed the title WIP: Student progress migration Add student progress migration Jul 31, 2023
@merkushin merkushin self-assigned this Jul 31, 2023
@merkushin merkushin added the HPPS label Jul 31, 2023
@merkushin merkushin added this to the 5.0.0 milestone Jul 31, 2023
@merkushin merkushin requested a review from m1r0 July 31, 2023 23:17
@merkushin merkushin marked this pull request as ready for review August 1, 2023 00:56
Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do in-depth testing but it worked great on what I've tested! I will do some more edge-case testing next week.

I remember you mentioning the handling of trashed comments. Is that part done or it will be in another PR?

}

if ( Course_Progress::STATUS_COMPLETE === $comment->comment_approved ) {
$completed_at = $comment->comment_date;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we plan to store dates in GMT for the new tables? If so, I don't think the comment_date is in GMT and might need some conversion. Maybe we can use comment_date_gmt instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m1r0 added usage of GMT date and conversion for the start date.

@merkushin merkushin modified the milestones: 5.0.0, 4.16.1 Aug 9, 2023
Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I wasn't able to find any issues. 🙂

@merkushin merkushin merged commit 5052125 into trunk Aug 10, 2023
20 checks passed
@merkushin merkushin deleted the add/data-migrator branch August 10, 2023 13:15
@m1r0 m1r0 mentioned this pull request Oct 18, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a migration that copies the progress data to the new tables
2 participants