From 70a0e60d7f0fa77f2a1821a5cf0ba0c6e03b21fc Mon Sep 17 00:00:00 2001 From: Cedric Evers <12080057+CeEv@users.noreply.github.com> Date: Thu, 7 Dec 2023 14:01:33 +0100 Subject: [PATCH 1/8] Add commits from meeting --- .../modules/lesson/service/lesson.service.ts | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/apps/server/src/modules/lesson/service/lesson.service.ts b/apps/server/src/modules/lesson/service/lesson.service.ts index 75cd3e459cb..f9dfdffad5f 100644 --- a/apps/server/src/modules/lesson/service/lesson.service.ts +++ b/apps/server/src/modules/lesson/service/lesson.service.ts @@ -5,6 +5,7 @@ import { Counted, EntityId } from '@shared/domain/types'; import { AuthorizationLoaderService } from '@src/modules/authorization'; import { LessonRepo } from '../repository'; +//missing interface for dependency inversion over token @Injectable() export class LessonService implements AuthorizationLoaderService { constructor( @@ -13,6 +14,23 @@ export class LessonService implements AuthorizationLoaderService { ) {} async deleteLesson(lesson: LessonEntity): Promise { + // shedule the event + /* + This is wrong! Because if we add more parent to filestorage the developer must know that + they must modified the parent deletion. For example in user. If we add user as possible parent. + + Expected result: + filestorage implement a event that listenc "lesson deleted" "user deleted" + "user deleted" -> remove removeUserFromFileRecord() => for each fileRecord.cretor() + "lesson deleted" -> rabbitMQ(apiLayer) > UC no auhtorisation > deletFileRecordsOfParent(lessonId, { deletedAt: Date.now() }) -> delte all fileRecords delete all S3 binary files + // -> after 7 days + + !!! important this is the smae between course and lesson for example !!! + + // Question when event "user deleted" event is popup from which instance ? After 14 days this event is shedulled. + // for filesstorage execution is zero days -> all fine S3 cleanup data and mongoDB too by it self + // console application + */ await this.filesStorageClientAdapterService.deleteFilesOfParent(lesson.id); await this.lessonRepo.delete(lesson); @@ -33,6 +51,9 @@ export class LessonService implements AuthorizationLoaderService { } async deleteUserDataFromLessons(userId: EntityId): Promise { + // lesson + // + // const lessons = await this.lessonRepo.findByUserId(userId); const updatedLessons = lessons.map((lesson: LessonEntity) => { @@ -46,6 +67,15 @@ export class LessonService implements AuthorizationLoaderService { }); await this.lessonRepo.save(updatedLessons); + /* + // + } if(lesson.getUserCound()<=1) { + // we are in conflict with the same operation in course + const result = await this.deleteLesson(); + // required event that external systems can be react on it + ...can do by application or a later cleanup job that remove all what have no user anymore + // S3 can only came to limits ..deletion request are smaller but + } */ return updatedLessons.length; } From 729bbc088d4c193e6ac6f2b424c7bd7b254ffa01 Mon Sep 17 00:00:00 2001 From: Cedric Evers <12080057+CeEv@users.noreply.github.com> Date: Thu, 7 Dec 2023 14:11:06 +0100 Subject: [PATCH 2/8] Ad more comments --- .../files-storage/controller/files-storage.consumer.ts | 1 + apps/server/src/modules/lesson/service/lesson.service.ts | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/apps/server/src/modules/files-storage/controller/files-storage.consumer.ts b/apps/server/src/modules/files-storage/controller/files-storage.consumer.ts index 38073e709ac..1ea32233377 100644 --- a/apps/server/src/modules/files-storage/controller/files-storage.consumer.ts +++ b/apps/server/src/modules/files-storage/controller/files-storage.consumer.ts @@ -36,6 +36,7 @@ export class FilesStorageConsumer { const { userId, source, target } = payload; const [response] = await this.filesStorageService.copyFilesOfParent(userId, source, { target }); + // at this place do not response, create only a new event with information return { message: response }; } diff --git a/apps/server/src/modules/lesson/service/lesson.service.ts b/apps/server/src/modules/lesson/service/lesson.service.ts index f9dfdffad5f..4455fc2dc62 100644 --- a/apps/server/src/modules/lesson/service/lesson.service.ts +++ b/apps/server/src/modules/lesson/service/lesson.service.ts @@ -5,7 +5,7 @@ import { Counted, EntityId } from '@shared/domain/types'; import { AuthorizationLoaderService } from '@src/modules/authorization'; import { LessonRepo } from '../repository'; -//missing interface for dependency inversion over token +// missing interface for dependency inversion over token, or maybe we do not need this when the event is used @Injectable() export class LessonService implements AuthorizationLoaderService { constructor( @@ -21,7 +21,8 @@ export class LessonService implements AuthorizationLoaderService { Expected result: filestorage implement a event that listenc "lesson deleted" "user deleted" - "user deleted" -> remove removeUserFromFileRecord() => for each fileRecord.cretor() + !!! each delete of a entity bring us to lost a litte bit more connection, than bring us to situation that is harder to cleanup later !!! + "user deleted" -> remove removeUserFromFileRecord() => for each fileRecord.cretor() "lesson deleted" -> rabbitMQ(apiLayer) > UC no auhtorisation > deletFileRecordsOfParent(lessonId, { deletedAt: Date.now() }) -> delte all fileRecords delete all S3 binary files // -> after 7 days From 18d2415ac8e722c596946692cc9cba34c98b8116 Mon Sep 17 00:00:00 2001 From: Cedric Evers <12080057+CeEv@users.noreply.github.com> Date: Thu, 7 Dec 2023 14:19:38 +0100 Subject: [PATCH 3/8] Add comments --- .../src/modules/lesson/service/lesson.service.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/apps/server/src/modules/lesson/service/lesson.service.ts b/apps/server/src/modules/lesson/service/lesson.service.ts index 4455fc2dc62..7c0b203a78c 100644 --- a/apps/server/src/modules/lesson/service/lesson.service.ts +++ b/apps/server/src/modules/lesson/service/lesson.service.ts @@ -24,13 +24,13 @@ export class LessonService implements AuthorizationLoaderService { !!! each delete of a entity bring us to lost a litte bit more connection, than bring us to situation that is harder to cleanup later !!! "user deleted" -> remove removeUserFromFileRecord() => for each fileRecord.cretor() "lesson deleted" -> rabbitMQ(apiLayer) > UC no auhtorisation > deletFileRecordsOfParent(lessonId, { deletedAt: Date.now() }) -> delte all fileRecords delete all S3 binary files - // -> after 7 days + // -> default deletedAt is 7 days !!! important this is the smae between course and lesson for example !!! // Question when event "user deleted" event is popup from which instance ? After 14 days this event is shedulled. // for filesstorage execution is zero days -> all fine S3 cleanup data and mongoDB too by it self - // console application + // console application sheduled the user deletion event */ await this.filesStorageClientAdapterService.deleteFilesOfParent(lesson.id); @@ -75,7 +75,12 @@ export class LessonService implements AuthorizationLoaderService { const result = await this.deleteLesson(); // required event that external systems can be react on it ...can do by application or a later cleanup job that remove all what have no user anymore - // S3 can only came to limits ..deletion request are smaller but + // S3 can only came to limits ..deletion request are smaller but should use a batch + + if we do not implement the "if" now, + than the cleanup job for entities without user can only work when events are implemented + and they shedule this events. + But if we lost the user before we can not execute the fileRecord.creator event anymore. } */ return updatedLessons.length; From b0d9cbdf884a3288273013e42eee326b45350ec2 Mon Sep 17 00:00:00 2001 From: Cedric Evers <12080057+CeEv@users.noreply.github.com> Date: Thu, 7 Dec 2023 14:23:12 +0100 Subject: [PATCH 4/8] Add comments --- apps/server/src/modules/lesson/service/lesson.service.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/apps/server/src/modules/lesson/service/lesson.service.ts b/apps/server/src/modules/lesson/service/lesson.service.ts index 7c0b203a78c..7c0e61109ec 100644 --- a/apps/server/src/modules/lesson/service/lesson.service.ts +++ b/apps/server/src/modules/lesson/service/lesson.service.ts @@ -81,6 +81,11 @@ export class LessonService implements AuthorizationLoaderService { than the cleanup job for entities without user can only work when events are implemented and they shedule this events. But if we lost the user before we can not execute the fileRecord.creator event anymore. + + If we lost the lesson, task, or submission we can not cleanup fileRecords that are placed in this context + without implement a cleanup job that make a lookup to parentSource and fileRecords. + This cleanup job must also trigger the S3 binary deletions for S3 files and previews and so on. + A lot of work...much more then impplement the amqp queue before and implement the events. } */ return updatedLessons.length; From 5c135afeee935a49f2d2c0be4de717f43740c506 Mon Sep 17 00:00:00 2001 From: Cedric Evers <12080057+CeEv@users.noreply.github.com> Date: Thu, 7 Dec 2023 14:26:54 +0100 Subject: [PATCH 5/8] comment in user service --- apps/server/src/modules/user/service/user.service.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/apps/server/src/modules/user/service/user.service.ts b/apps/server/src/modules/user/service/user.service.ts index 4e8f2fe2394..0b262f4e964 100644 --- a/apps/server/src/modules/user/service/user.service.ts +++ b/apps/server/src/modules/user/service/user.service.ts @@ -129,6 +129,10 @@ export class UserService { return deletedUserNumber; } + // mark for deletion + // but the "user deletion event" must sheduled 14 days later from the cronjob/console job. + // because many reaction places ...delete directly if the event is sheduled. + async getParentEmailsFromUser(userId: EntityId): Promise { const parentEmails = this.userRepo.getParentEmailsFromUser(userId); From 19e06301184015b96627e832661f3f06990ae33a Mon Sep 17 00:00:00 2001 From: Cedric Evers <12080057+CeEv@users.noreply.github.com> Date: Thu, 7 Dec 2023 14:30:00 +0100 Subject: [PATCH 6/8] clarify one sentence --- apps/server/src/modules/lesson/service/lesson.service.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/apps/server/src/modules/lesson/service/lesson.service.ts b/apps/server/src/modules/lesson/service/lesson.service.ts index 7c0e61109ec..9045be7e8a8 100644 --- a/apps/server/src/modules/lesson/service/lesson.service.ts +++ b/apps/server/src/modules/lesson/service/lesson.service.ts @@ -16,8 +16,11 @@ export class LessonService implements AuthorizationLoaderService { async deleteLesson(lesson: LessonEntity): Promise { // shedule the event /* - This is wrong! Because if we add more parent to filestorage the developer must know that - they must modified the parent deletion. For example in user. If we add user as possible parent. + The "await this.filesStorageClientAdapterService.deleteFilesOfParent(lesson.id);" + is wrong! Because if we add more parent to filestorage the developer must know that + they must modified the parent deletion. + For example in user. If we add user as possible parent. The files are not deleted without connection. + But it is not visible in the entity that files for user can be exists. Expected result: filestorage implement a event that listenc "lesson deleted" "user deleted" From d6a90718535d7b403847232c40b19be60f08f621 Mon Sep 17 00:00:00 2001 From: Cedric Evers <12080057+CeEv@users.noreply.github.com> Date: Thu, 7 Dec 2023 14:31:02 +0100 Subject: [PATCH 7/8] Update lesson.service.ts --- apps/server/src/modules/lesson/service/lesson.service.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/server/src/modules/lesson/service/lesson.service.ts b/apps/server/src/modules/lesson/service/lesson.service.ts index 9045be7e8a8..a2b28d37693 100644 --- a/apps/server/src/modules/lesson/service/lesson.service.ts +++ b/apps/server/src/modules/lesson/service/lesson.service.ts @@ -23,7 +23,9 @@ export class LessonService implements AuthorizationLoaderService { But it is not visible in the entity that files for user can be exists. Expected result: - filestorage implement a event that listenc "lesson deleted" "user deleted" + filestorage implement a event listener that react on "lesson deleted", "user deleted",.. + ..based on the parents that they know + !!! each delete of a entity bring us to lost a litte bit more connection, than bring us to situation that is harder to cleanup later !!! "user deleted" -> remove removeUserFromFileRecord() => for each fileRecord.cretor() "lesson deleted" -> rabbitMQ(apiLayer) > UC no auhtorisation > deletFileRecordsOfParent(lessonId, { deletedAt: Date.now() }) -> delte all fileRecords delete all S3 binary files From 28f7c2ba3f2b4372bba0061e9a1e6be37f7b3c86 Mon Sep 17 00:00:00 2001 From: Cedric Evers <12080057+CeEv@users.noreply.github.com> Date: Thu, 7 Dec 2023 14:31:57 +0100 Subject: [PATCH 8/8] typo --- apps/server/src/modules/lesson/service/lesson.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/lesson/service/lesson.service.ts b/apps/server/src/modules/lesson/service/lesson.service.ts index a2b28d37693..881a331f5e4 100644 --- a/apps/server/src/modules/lesson/service/lesson.service.ts +++ b/apps/server/src/modules/lesson/service/lesson.service.ts @@ -75,7 +75,7 @@ export class LessonService implements AuthorizationLoaderService { await this.lessonRepo.save(updatedLessons); /* // - } if(lesson.getUserCound()<=1) { + } if(lesson.getUserCount()<=1) { // we are in conflict with the same operation in course const result = await this.deleteLesson(); // required event that external systems can be react on it