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

[#12048] Fix course entity migration script #13082

Closed
wants to merge 11 commits into from

Conversation

yuanxi1
Copy link
Contributor

@yuanxi1 yuanxi1 commented Apr 22, 2024

Part of #12048

Outline of Solution
Trying to test the whole migration scripts. Here are some questions and issues found:

  • Missing call to verification script [fixed in this pr]
  • new students are not added to the entitySavingBuffer [fixed in this pr]
  • Seed for deadline extension and instructor is missing
  • Seed for account’s google id and student’s google id does not match (email also does not match, which is needed because in the current implementation of how migrated accounts are retrieved based on email), hence whether account is properly linked is not yet tested (fixed in [#12048] Fix seed, migration and verification script for course verification #13086)
  • Verification fails, yet to investigate the reason. (by manually query the sql db, seems like the entities counts are correct) - (partially fixed in [#12048] Fix seed, migration and verification script for course verification #13086)
    Questions:
  • In DataMigrationForCourseEntitySql::migrateUserAccounts,
    • should we use googleId to query the existing migrated account for students or is email reliable enough
  • Added a function in SeedDb.java to clear the datastore for reseeding

@yuanxi1 yuanxi1 requested a review from FergusMok April 22, 2024 18:41
@NicolasCwy NicolasCwy added s.Ongoing The PR is being worked on by the author(s) c.Task Other non-user-facing works, e.g. refactoring, adding tests labels Apr 23, 2024
@NicolasCwy
Copy link
Contributor

Partially fixed in #13086. The changes in this PR have also been merged there, will close this PR

@NicolasCwy NicolasCwy closed this Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.Ongoing The PR is being worked on by the author(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants