BC-7466 - Switch SHD administration of schools (students) to new deletion routines #5478
Conversation
…nto BC-7466-shd-new-deletion-routines-for-students
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
apps/server/src/modules/deletion/api/controller/deletion-batch.controller.ts
Fixed
Show fixed
Hide fixed
| const validUserIds: EntityId[] = []; | ||
| const invalidUserIds: EntityId[] = []; | ||
|
|
||
| for (const userId of targetRefIds) { |
There was a problem hiding this comment.
could be rewritten using a map, and then Promise.all
| const invalidUserIds: EntityId[] = []; | ||
|
|
||
| for (const userId of targetRefIds) { | ||
| const userExists = await this.userRepo.findByIdOrNull(userId); |
There was a problem hiding this comment.
I am wondering why is even possible...but we should not be able to access user repo directly. It should be accessed via a service which user module exports.
| public async createDeletionBatch( | ||
| params: CreateDeletionBatchParams, | ||
| validUserIds: EntityId[], | ||
| invalidUserIds: EntityId[] | undefined |
There was a problem hiding this comment.
| invalidUserIds: EntityId[] | undefined | |
| invalidUserIds?: EntityId[] |
| } | ||
|
|
||
| // TODO implement as join on deletionbatches.targetRefIds to avoid N+1 | ||
| public async getUsersByRoles(userIds: EntityId[]): Promise<UsersByRole[]> { |
| for (const targetRefId of batch.targetRefIds) { | ||
| const deleteNow = new Date(Date.now()); | ||
| await this.deletionRequestService.createDeletionRequest(targetRefId, DomainName.USER, deleteNow); | ||
| } |
There was a problem hiding this comment.
| for (const targetRefId of batch.targetRefIds) { | |
| const deleteNow = new Date(Date.now()); | |
| await this.deletionRequestService.createDeletionRequest(targetRefId, DomainName.USER, deleteNow); | |
| } | |
| const deleteNow = new Date(); | |
| const deletionRequests = batch.targetRefIds.map(targetRefId => this.deletionRequestService.createDeletionRequest(targetRefId, DomainName.USER, deleteNow) | |
| ); | |
| await Promise.all(deletionRequests); |
There was a problem hiding this comment.
on a second thought, this Promise.all might not be a good idea
Because it can be that we try to create a deletion request for a user id which already exists, in which case we should get an exception. And promise.all would stop
| } | ||
|
|
||
| public async createDeletionRequestForBatch(batchId: EntityId): Promise<DeletionBatchSummary> { | ||
| const deletionBatch = await this.deletionBatchRepo.findById(batchId); |
There was a problem hiding this comment.
we should not access repo directly in the uc, but via service...
a simple wrapper function in service should do
| ], | ||
| exports: [DeletionRequestService, DeletionLogService], | ||
| exports: [DeletionRequestService, DeletionLogService, DeletionBatchService, DeletionBatchRepo], |
There was a problem hiding this comment.
Repo does not belong here... is needed because is injected in the uc, but there we should also not access the repo directly, but instead access the service
| constructor( | ||
| private readonly deletionBatchService: DeletionBatchService, | ||
| private readonly deletionBatchRepo: DeletionBatchRepo, | ||
| private readonly userRepo: UserRepo |
There was a problem hiding this comment.
repo should not be accessed directly in uc
Description
Switch SHD administration of schools (students) to new deletion routines
Links to Tickets or other pull requests
BC-7466
hpi-schul-cloud/superhero-dashboard#278
Approval for review
generate-client:serverwas executed in vue frontend and changes were tested and put in a PR with the same branch name.