BC-6420 - unsynced users deletion #4868
Conversation
…rservice, ande test cases
| import { AccountModule } from '../account'; | ||
| import { defaultMikroOrmOptions } from '../server'; | ||
| import { FileEntity } from '../files/entity'; | ||
| import { UserModule } from '../user/user.module'; |
There was a problem hiding this comment.
remove unnecessary imports
| @@ -1,4 +1,6 @@ | |||
| import { Injectable } from '@nestjs/common'; | |||
| import { AccountService } from '@src/modules/account'; | |||
| import { UserService } from '@src/modules/user'; | |||
There was a problem hiding this comment.
keep to convention for imports from modules @modules/....
WojciechGrancow
added
waiting for review
and removed
WIP
There was a problem hiding this comment.
Adding Services as Providers directly is NOT the solution to circular dependencies. It does not solve them, it only makes them more narrow and harder to detect. They seem to be solved, but they are not, they will come back to bite us later.
It is absolutely forbidden to provide things that are already provided by another module.
Why do these circular dependencies appear? there should be no other module with any dependency on deletion console right?
Possibly they are caused by other circular dependencies that are already in the system, but hidden because other people tried to trick them by hiding them in explicit imports. Thats exactly the reason why you cant provide the UserService in your module, it will cause an dependency cycle in some harmless change later down the line in a different PR that has nothing to do with this one.
We need to figure out where the circular dependencies come from, what modules actually form the circle (modules, not files), and then either untangling or avoid using them.
| @@ -25,6 +51,13 @@ import { BatchDeletionUc, DeletionExecutionUc } from './uc'; | |||
| DeletionExecutionUc, | |||
| DeletionQueueConsole, | |||
| DeletionExecutionConsole, | |||
| UserService, | |||
There was a problem hiding this comment.
these services need to go, if you need them import the modules instead.
Some of the Repos are an exception since they are not part of a module, but you should avoid using the repos at all directly, and instead introduce service methods for things you need to do from the outside.
There was a problem hiding this comment.
Fixed in 074568b - all the non-local dependencies were moved to imports, in providers there are just the local dependencies now.
|
* crate entity, repo, do, mapper for synchronization and deletion-reconcilation.uc * add synchronization service and tests * changes in reconcilation UC * add lastSyncedAt to userEntity and change create in synchronization service * create synchronization module and move files from deletion module into it * add new interface, builders, method in userrepo and new methos in userservice, ande test cases * some changes * mofdify acount, user service and synchronization UC * add some tests, remove userIdAndExternalId interface, builder * changes in synchronizationEntity * add staus to synchronizationEntity * add update method in repo in synchronization module * fix imports, add update method and test in service * add synchromization.module, fix import, modify uc * fix in acount service test * change folder structure in synchronization module, fix test in repo * coments about chunk * add loggable, and extension of UC * add test cases to loggable and to userService * add tests to useCase in synchronizationModule * add test to UC * BC-6223-add chunks * add test cases in UC * add tests * remove private methon * refactor * add tests and change loop * add test to UC * add config * sum with promise.all * fixes typo, changes test in UC * rename testConfig file * add new command (for queueing unsynchronized entities for deletion) * add new Ansible role for moin.schule sync with users deletion queueing CronJob task * Update apps/server/src/shared/repo/user/user.repo.integration.spec.ts Co-authored-by: Bartosz Nowicki <[email protected]> * fix some issues after review * fiz error cases in uc * add test in UC * modify SynchronizationErrorLoggableException * main logic impl * fix tests * remove files that were not resolved automatically after merge * Pr fix and test implementation part1 * test impl part2 * test impl part 3 * test implementation part 3 * add needed modules and providers * add mongoDb flag * fix import order * import fix * import chnages * refactor dependencies --------- Co-authored-by: WojciechGrancow <[email protected]> Co-authored-by: Wojciech Grancow <[email protected]> Co-authored-by: micners <[email protected]> Co-authored-by: WojciechGrancow <[email protected]> Co-authored-by: Szymon Szafoni <[email protected]> Co-authored-by: sszafGCA <[email protected]>
* crate entity, repo, do, mapper for synchronization and deletion-reconcilation.uc * add synchronization service and tests * changes in reconcilation UC * add lastSyncedAt to userEntity and change create in synchronization service * create synchronization module and move files from deletion module into it * add new interface, builders, method in userrepo and new methos in userservice, ande test cases * some changes * mofdify acount, user service and synchronization UC * add some tests, remove userIdAndExternalId interface, builder * changes in synchronizationEntity * add staus to synchronizationEntity * add update method in repo in synchronization module * fix imports, add update method and test in service * add synchromization.module, fix import, modify uc * fix in acount service test * change folder structure in synchronization module, fix test in repo * coments about chunk * add loggable, and extension of UC * add test cases to loggable and to userService * add tests to useCase in synchronizationModule * add test to UC * BC-6223-add chunks * add test cases in UC * add tests * remove private methon * refactor * add tests and change loop * add test to UC * add config * sum with promise.all * fixes typo, changes test in UC * rename testConfig file * add new command (for queueing unsynchronized entities for deletion) * add new Ansible role for moin.schule sync with users deletion queueing CronJob task * Update apps/server/src/shared/repo/user/user.repo.integration.spec.ts Co-authored-by: Bartosz Nowicki <[email protected]> * fix some issues after review * fiz error cases in uc * add test in UC * modify SynchronizationErrorLoggableException * main logic impl * fix tests * remove files that were not resolved automatically after merge * Pr fix and test implementation part1 * test impl part2 * test impl part 3 * test implementation part 3 * add needed modules and providers * add mongoDb flag * fix import order * import fix * import chnages * refactor dependencies --------- Co-authored-by: WojciechGrancow <[email protected]> Co-authored-by: Wojciech Grancow <[email protected]> Co-authored-by: micners <[email protected]> Co-authored-by: WojciechGrancow <[email protected]> Co-authored-by: Szymon Szafoni <[email protected]> Co-authored-by: sszafGCA <[email protected]>
Description
Links to Tickets or other pull requests
https://ticketsystem.dbildungscloud.de/browse/BC-6420
Changes
Datasecurity
Deployment
New Repos, NPM pakages or vendor scripts
Approval for review
generate-client:serverwas executed in vue frontend and changes were tested and put in a PR with the same branch name.