-
Notifications
You must be signed in to change notification settings - Fork 17
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
EW-1019: TSP Testing #5375
Open
mkreuzkam-cap
wants to merge
47
commits into
main
Choose a base branch
from
EW-1019
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
EW-1019: TSP Testing #5375
Changes from 1 commit
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
0ce5351
Add empty line.
mkreuzkam-cap 7bbabe7
log token
mkreuzkam-cap fcd8d6d
Remove pLimit temporarily for testing.
mkreuzkam-cap 49bfdda
Manual batching of data.
mkreuzkam-cap 628d8c3
Merge branch 'main' into EW-1019
mkreuzkam-cap 397dff6
Fix array size.
mkreuzkam-cap e4548fc
Better logging.
mkreuzkam-cap 634049c
Merge branch 'main' into EW-1019
mkreuzkam-cap 0f7acca
Even more logs!
mkreuzkam-cap 1c342a0
Logging of errors.
mkreuzkam-cap a9669a5
Use bulk operations for sync. (Very WIP!!!)
mkreuzkam-cap 3633cc0
EW-1019 Fix Linter warnings
SimoneRadtke-Cap ac863a7
EW-1019 Fix linter warnings
SimoneRadtke-Cap 551f0e6
Use bulk operations for migration.
mkreuzkam-cap 3c805b4
Merge branch 'main' into EW-1019
mkreuzkam-cap 79c6a7c
EW-1019 Add docker compose file to gitignore
SimoneRadtke-Cap 18217e6
fix linter warning
mkreuzkam-cap 643c843
Bulk migration for students.
mkreuzkam-cap 0a38d25
Merge branch 'main' into EW-1019
mkreuzkam-cap a04d22d
Use configService directly and don't read values in constructor.
mkreuzkam-cap c16a7bf
Add batching.
mkreuzkam-cap 92e1b49
Merge branch 'main' into EW-1019
mkreuzkam-cap 7dc774f
remove console log, add comments
mkreuzkam-cap 2587c90
Clean up migration and move into service class.
mkreuzkam-cap 470329c
Clean up loggables.
mkreuzkam-cap 4a163ae
EW.1019 Add saveAll to account-idm-service
SimoneRadtke-Cap 29534a2
adjust config var usage.
mkreuzkam-cap 8d19ebf
adjust tests (wip)
mkreuzkam-cap a34c3a8
Fix findMany test in accoundIdm service.
mkreuzkam-cap 1b0627b
Fix tsp sync strategy test.
mkreuzkam-cap 216dfb0
Merge branch 'main' into EW-1019
mkreuzkam-cap c734169
Add tests for user service and repo.
mkreuzkam-cap 91d5632
Merge branch 'main' into EW-1019
mkreuzkam-cap 54a3d91
EW-1019 Adjust tsp sync service test
SimoneRadtke-Cap a6e97fa
Fix tests which broke for some reason.
mkreuzkam-cap b9edb39
Fix another test which broke for no reason.
mkreuzkam-cap 9f3e9f1
Add tests for accounts.
mkreuzkam-cap e64b935
Fix an oopsie.
mkreuzkam-cap c58364b
EW-1019 Add test
SimoneRadtke-Cap 905685d
Finish tsp sync migration service test.
mkreuzkam-cap b49aedb
Merge branch 'main' into EW-1019
mkreuzkam-cap d534101
Reduce amount of awaits.
mkreuzkam-cap 27c73ea
remove async/await.
mkreuzkam-cap 45c7d41
Remove line.
mkreuzkam-cap b002a07
Update apps/server/src/modules/account/domain/services/account-db.ser…
mkreuzkam-cap c6bd5ec
Add return type.
mkreuzkam-cap a7b728d
fix linter.
mkreuzkam-cap File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Clean up migration and move into service class.
- Loading branch information
commit 2587c90e1744f327cd17d34e70a49eefa283dace
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26 changes: 26 additions & 0 deletions
26
apps/server/src/infra/sync/tsp/loggable/tsp-migration-batch-summary.loggable.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { Loggable, LogMessage } from '@src/core/logger'; | ||
|
||
export class TspMigrationBatchSummaryLoggable implements Loggable { | ||
constructor( | ||
private readonly batchSize: number, | ||
private readonly usersUpdated: number, | ||
private readonly accountsUpdated: number, | ||
private readonly totalDone: number, | ||
private readonly totalMigrations: number | ||
) {} | ||
|
||
public getLogMessage(): LogMessage { | ||
const message: LogMessage = { | ||
message: `Migrated ${this.usersUpdated} users and ${this.accountsUpdated} accounts in batch of size ${this.batchSize} (total done: ${this.totalDone}, total migrations: ${this.totalMigrations})`, | ||
data: { | ||
batchSize: this.batchSize, | ||
usersUpdated: this.usersUpdated, | ||
accountsUpdated: this.accountsUpdated, | ||
totalDone: this.totalDone, | ||
totalMigrations: this.totalMigrations, | ||
}, | ||
}; | ||
|
||
return message; | ||
} | ||
} |
6 changes: 3 additions & 3 deletions
6
apps/server/src/infra/sync/tsp/loggable/tsp-teachers-fetched.loggable.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
10 changes: 8 additions & 2 deletions
10
apps/server/src/infra/sync/tsp/loggable/tsp-users-migrated.loggable.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
140 changes: 140 additions & 0 deletions
140
apps/server/src/infra/sync/tsp/tsp-sync-migration.service.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
import { Account, AccountService } from '@modules/account'; | ||
import { System } from '@modules/system'; | ||
import { UserService } from '@modules/user'; | ||
import { Injectable } from '@nestjs/common'; | ||
import { ConfigService } from '@nestjs/config'; | ||
import { UserDO } from '@shared/domain/domainobject'; | ||
import { UserSourceOptions } from '@shared/domain/domainobject/user-source-options.do'; | ||
import { Logger } from '@src/core/logger'; | ||
import { TspTeachersFetchedLoggable } from './loggable/tsp-teachers-fetched.loggable'; | ||
import { TspSyncConfig } from './tsp-sync.config'; | ||
import { TspMigrationBatchSummaryLoggable } from './loggable/tsp-migration-batch-summary.loggable'; | ||
|
||
@Injectable() | ||
export class TspSyncMigrationService { | ||
constructor( | ||
private readonly logger: Logger, | ||
private readonly userService: UserService, | ||
private readonly accountService: AccountService, | ||
private readonly configService: ConfigService<TspSyncConfig, true> | ||
) { | ||
this.logger.setContext(TspSyncMigrationService.name); | ||
} | ||
|
||
public async migrateTspUsers( | ||
system: System, | ||
oldToNewMappings: Map<string, string> | ||
): Promise<{ | ||
totalAmount: number; | ||
totalUsers: number; | ||
totalAccounts: number; | ||
}> { | ||
const totalIdCount = oldToNewMappings.size; | ||
this.logger.info(new TspTeachersFetchedLoggable(totalIdCount)); | ||
|
||
const batches = this.getOldIdBatches(oldToNewMappings); | ||
|
||
let totalAmount = 0; | ||
let totalUsers = 0; | ||
let totalAccounts = 0; | ||
for await (const oldIdsBatch of batches) { | ||
const { users, accounts, accountsForUserId } = await this.loadUsersAndAccounts(oldIdsBatch); | ||
const updated = this.updateUsersAndAccounts(system.id, oldToNewMappings, users, accountsForUserId); | ||
await this.saveUsersAndAccounts(users, accounts); | ||
|
||
totalAmount += oldIdsBatch.length; | ||
totalUsers += updated.usersUpdated; | ||
totalAccounts += updated.accountsUpdated; | ||
|
||
this.logger.info( | ||
new TspMigrationBatchSummaryLoggable( | ||
oldIdsBatch.length, | ||
updated.usersUpdated, | ||
updated.accountsUpdated, | ||
totalAmount, | ||
totalIdCount | ||
) | ||
); | ||
} | ||
|
||
return { | ||
totalAmount, | ||
totalUsers, | ||
totalAccounts, | ||
}; | ||
} | ||
|
||
private updateUsersAndAccounts( | ||
systemId: string, | ||
oldToNewMappings: Map<string, string>, | ||
users: UserDO[], | ||
accountsForUserId: Map<string, Account> | ||
): { usersUpdated: number; accountsUpdated: number } { | ||
let usersUpdated = 0; | ||
let accountsUpdated = 0; | ||
users.forEach((user) => { | ||
const oldId = user.sourceOptions?.tspUid; | ||
|
||
if (!oldId) { | ||
return; | ||
} | ||
|
||
const newUid = oldToNewMappings.get(oldId); | ||
|
||
if (!newUid) { | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this case be logged? |
||
} | ||
|
||
const newEmailAndUsername = `${newUid}@schul-cloud.org`; | ||
|
||
user.email = newEmailAndUsername; | ||
user.externalId = newUid; | ||
user.previousExternalId = oldId; | ||
user.sourceOptions = new UserSourceOptions({ tspUid: newUid }); | ||
usersUpdated += 1; | ||
|
||
const account = accountsForUserId.get(user.id ?? ''); | ||
if (account) { | ||
account.username = newEmailAndUsername; | ||
account.systemId = systemId; | ||
accountsUpdated += 1; | ||
} | ||
}); | ||
|
||
return { usersUpdated, accountsUpdated }; | ||
} | ||
|
||
private getOldIdBatches(oldToNewMappings: Map<string, string>): string[][] { | ||
const oldIds = Array.from(oldToNewMappings.keys()); | ||
const batchSize = this.configService.get<number>('TSP_SYNC_MIGRATION_LIMIT', 100); | ||
|
||
const batchCount = Math.ceil(oldIds.length / batchSize); | ||
const batches: string[][] = []; | ||
for (let i = 0; i < batchCount; i += 1) { | ||
const start = i * batchSize; | ||
const end = Math.min((i + 1) * batchSize, oldIds.length); | ||
batches.push(oldIds.slice(start, end)); | ||
} | ||
|
||
return batches; | ||
} | ||
|
||
private async loadUsersAndAccounts( | ||
tspUids: string[] | ||
): Promise<{ users: UserDO[]; accounts: Account[]; accountsForUserId: Map<string, Account> }> { | ||
const users = await this.userService.findByTspUids(tspUids); | ||
|
||
const userIds = users.map((user) => user.id ?? ''); | ||
const accounts = await this.accountService.findMultipleByUserId(userIds); | ||
|
||
const accountsForUserId = new Map<string, Account>(); | ||
accounts.forEach((account) => accountsForUserId.set(account.userId ?? '', account)); | ||
|
||
return { users, accounts, accountsForUserId }; | ||
} | ||
|
||
private async saveUsersAndAccounts(users: UserDO[], accounts: Account[]): Promise<void> { | ||
await this.userService.saveAll(users); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could these 2 be in a Promise.all? |
||
await this.accountService.saveAll(accounts); | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this case be logged?