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] Add patch script for account request and notifications to amend createdAt field #13077

Merged

Conversation

NicolasCwy
Copy link
Contributor

@NicolasCwy NicolasCwy commented Apr 22, 2024

Part of #12048

Context
Previously during the non-course migration, createdAt was not migrated due to having the @createdTimestamp annotation. This was removed in the v9-course-migration branch and can be used for this as well.

Outline of Solution
Add 2 scripts based of PatchDataMigrationForUsageStatisticsSql to fetch existing account requests and notifications using fields which are identifiers for the entity and amend their createdAt timestamp. Most of the changes are in MigrateEntity method. Verification Scripts can handle the case where an entity exist in postgres but not in datastore (since we have started created new account requests and notification in postgres)

Fields which are identifiers

  • notifications: title
  • accountRequest: email and institute

@NicolasCwy NicolasCwy added s.ToReview The PR is waiting for review(s) c.Task Other non-user-facing works, e.g. refactoring, adding tests labels Apr 22, 2024
@NicolasCwy NicolasCwy self-assigned this Apr 22, 2024
// Get first items since there is guaranteed to be one
teammates.storage.sqlentity.AccountRequest newAccountReq = matchingAccounts.get(0);
newAccountReq.setCreatedAt(oldEntity.getCreatedAt());
HibernateUtil.commitTransaction();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're fetching an existing entity, changing the object attribute will save the changes in the DB

Copy link
Contributor

@ziqing26 ziqing26 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ziqing26 ziqing26 added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Apr 23, 2024
@ziqing26 ziqing26 modified the milestones: V9.0.0-beta.6, V9.0.0-beta.7 Apr 23, 2024
Copy link
Contributor

@FergusMok FergusMok left a comment

Choose a reason for hiding this comment

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

Only the indexing 0 should be problem, LGTM

Comment on lines +126 to +133
if (matchingAccounts.size() > 1) {
throw new Error("More than one matching account request found");
} else if (matchingAccounts.size() == 0){
throw new Error("No matching account found");
}

// Get first items since there is guaranteed to be one
teammates.storage.sqlentity.AccountRequest newAccountReq = matchingAccounts.get(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the account request has already been processed / deleted in SQL? I didn't look into how accounts are created, but does it delete the request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like processing the account (approving / rejecting) does not delete the SQL entity. But its possible that prof manually deletes them let me check in with him
image

* Returns the filter query.
*/
protected Query<teammates.storage.entity.AccountRequest> getFilterQuery() {
return ofy().load().type(teammates.storage.entity.AccountRequest.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important, only if you have time.
https://cloud.google.com/datastore/docs/concepts/queries#projection_queries
Not sure if we can do a projection query to speed up, since we're only interested in institute, email, name and createdAt. Objectify has a project()

@NicolasCwy NicolasCwy merged commit 620e05d into TEAMMATES:v9-course-migration Apr 24, 2024
1 check passed
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.FinalReview The PR is ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants