[REFACTOR] Refactor Scheduler following Clean Architecture by anagperal · Pull Request #988 · EyeSeeTea/metadata-synchronization

anagperal · 2025-02-26T09:01:48Z

📌 References

Issue: Closes #8698310pt Refactor Scheduler following Clean Architecture

📝 New refactor tasks

Decouple configRepository from other repositories that only need it to use the dataStoreClient
Think about how to refactor RepositoryFactory concept
Do not pass RepositoryFactory and Instance un use cases, only pass the necessary repositories
In the constructor of data repositories pass instead of Instance we should get D2Api or directly DataStoreClient when different instances are not needed
CompositionRoot needs to be refactored
Remove RepositoryConstructor concept from domain repositories where is not neccesary
Improve and refactor Scheduler tests using ts-mockito and adding integration tests

📝 Implementation

Refactor Scheduler following Clean Architecture: using MVP pattern
Refactor old code for a better understanding
Fix feedback component error when init app when not having feedback config in appConfig
Refactor SchedulerRepository to SchedulerExecutionInfoRepository and use cases that uses this repository
Decouple configRepository in SchedulerExecutionInfoD2ApiRepository and SyncRuleJobConfigD2ApiRepository
Fix add lastExecution also to scheduler-executions in datastore
First version of Scheduler test --> for the future use ts-mockito
Add new futures

📹 Screenshots/Screen capture

🔥 Is there anything the reviewer should know to test it?

Create or edit a sync rule having the scheduler enabled
Run scheduler using the corresponding config: yarn start-scheduler -c config.json
Check if data is sync correctly

📑 Others

Any change in the GUI library? If so, what branch/PR?
Any change in the D2 Api? If so, what branch/PR?

ifoche · 2025-02-26T09:01:52Z

Task linked: CU-8698310pt Refactor Scheduler

…cRuleJobConfigD2Repository

…y in scheduler

9sneha-n

@anagperal Awesome first PR for refactor.
Some comments below, we will probably need architect's opinion for some of them, then we can make it a standard and use it in all the next refactor PRs.

9sneha-n · 2025-03-03T15:21:41Z

src/data/scheduler/SchedulerExecutionInfoD2ApiRepository.ts

@@ -0,0 +1,27 @@
+import { Instance } from "../../domain/instance/entities/Instance";


Shall we add a decorator for all refactored files? So that we can keep track of all the files that are pending refactor?

I have added comments on the refactored files and TODOs commenting on what they need to be fully refactored. Also in PR I have added some common reactoring tasks

9sneha-n · 2025-03-03T15:25:18Z

src/data/scheduler/SyncRuleJobConfigD2ApiRepository.ts

+export class SyncRuleJobConfigD2ApiRepository implements SyncRuleJobConfigRepository {
+    private dataStoreClient: StorageDataStoreClient;
+
+    constructor(private instance: Instance) {


currently in our projects, we are pass private api: D2Api to all repo constructors and then get the dataStoreClient from it. I am not sure why that is used instead of instance. We can check with Arnau and if that is recommended, shall we start using that pattern in all refactored repos?

Code comment and task added for future implementation

9sneha-n · 2025-03-03T15:27:31Z

src/domain/common/factories/RepositoryFactory.ts

-    public schedulerRepository(instance: Instance) {
-        const config = this.configRepository(instance);
-        return this.get<SchedulerRepositoryConstructor>(Repositories.SchedulerRepository, [config]);
+    public schedulerExecutionInfoRepository(instance: Instance) {


do we need to use this concept of repository-factory? I dont see it in our standard apps. We can create a separate PR to handle these "common" refactors, just making a note here for us.

Code comment and task added for future implementation

9sneha-n · 2025-03-03T15:29:04Z

src/domain/scheduler/repositories/SchedulerExecutionInfoRepositoryConstructor.ts

+import { Instance } from "../../instance/entities/Instance";
+import { SchedulerExecutionInfo } from "../entities/SchedulerExecutionInfo";
+
+export interface SchedulerExecutionInfoRepositoryConstructor {


Again this comment in for separate PR for "common" refactor - should we used this concept of Constructor ?

Code comment and task added for future implementation

9sneha-n · 2025-03-03T15:30:19Z

src/domain/scheduler/usecases/GetLastSchedulerExecutionInfoUseCase.ts

+export class GetLastSchedulerExecutionInfoUseCase implements UseCase {
+    constructor(private repositoryFactory: RepositoryFactory, private localInstance: Instance) {}
+
+    public execute(): Promise<SchedulerExecutionInfo> {


can we use Future instead of Promise? If its too big a change, we can add a note for future refactor and take care of it in separate PR

I have added new Future entity and changed from Promise to Future

9sneha-n · 2025-03-03T15:33:32Z

src/presentation/CompositionRoot.ts

 import { FileRulesDefaultRepository } from "../data/rules/FileRulesDefaultRepository";
 import { RulesD2ApiRepository } from "../data/rules/RulesD2ApiRepository";
-import { SchedulerD2ApiRepository } from "../data/scheduler/SchedulerD2ApiRepository";
+import { SchedulerExecutionInfoD2ApiRepository } from "../data/scheduler/SchedulerExecutionInfoD2ApiRepository";


common refactor note - We need to move to current way of implementing Composition root

Code comment and task added for future implementation

9sneha-n · 2025-03-03T15:35:59Z

src/scheduler/SchedulerPresenter.ts

+import { SchedulerExecutionInfo } from "../domain/scheduler/entities/SchedulerExecutionInfo";
+import { Logger } from "./entities/Logger";
+
+export class SchedulerPresenter {


is SchedulerPresenter name too generic? Should we rename to something like SchedulerCLI or something similar?

9sneha-n · 2025-03-03T15:37:28Z

src/scheduler/SchedulerPresenter.ts

+
+                // Format date to keep timezone offset
+                const nextDate = moment(job.nextExecution.toISOString()).toISOString(true);
+                this.options.logger.info("scheduler", `Scheduling new sync rule ${name} (${id}) at ${nextDate}`);


if we deconstruct options in the beginning one time, code would be more readable without all the 'this.options.' no?

9sneha-n · 2025-03-03T15:39:36Z

src/scheduler/entities/ScheduledJob.ts

@@ -0,0 +1,6 @@
+export type ScheduledJob = {


Should these entities be moved to domain layer?

9sneha-n · 2025-03-03T15:45:25Z

src/scheduler/entities/SchedulerContract.ts

@@ -0,0 +1,7 @@
+import { ScheduledJob } from "./ScheduledJob";
+
+export interface SchedulerContract {


i have note seen interfaces with functions used in this way in our standard apps. Shall we check if this type of usage is recommended or needs to be changed?

I would change this together with @xurxodev PR comments because I think this is more related with the architecture

anagperal · 2025-03-05T15:54:36Z

@anagperal Awesome first PR for refactor. Some comments below, we will probably need architect's opinion for some of them, then we can make it a standard and use it in all the next refactor PRs.

Thanks for all the comments @9sneha-n ! I have added comments in code and in the PR for common refactor tasks that we need to do in the future, also I have added them in the whiteboard of MD Sync (CC @adrianq).

xurxodev

thanks @anagperal

I leave things to change that we have seen in the live review:

SyncRuleJobConfigD2ApiRepository and the use case that invoke it doesn't make sense. Better to use ListRulesUseCase passing required filter to avoid duplicate logic to retrive
Move contracts used by SchedulerCli from domain to near of schedulerCli. This contracts are only used by SchedulerCli and it's contracts of presentation.
Maybe make sense in the scheduler use cases pass directly the required repository because here the instance is not dynamic
For tests better to use ts-mockito to create test doubles as we saw in the coding dojo

…rom list rules usecase. Improve tests and enabled filter name

anagperal · 2025-03-07T09:55:18Z

thanks @anagperal

I leave things to change that we have seen in the live review:

SyncRuleJobConfigD2ApiRepository and the use case that invoke it doesn't make sense. Better to use ListRulesUseCase passing required filter to avoid duplicate logic to retrive

Move contracts used by SchedulerCli from domain to near of schedulerCli. This contracts are only used by SchedulerCli and it's contracts of presentation.

Maybe make sense in the scheduler use cases pass directly the required repository because here the instance is not dynamic

For tests better to use ts-mockito to create test doubles as we saw in the coding dojo

Thanks @xurxodev!! All done except the part about changing the tests to use ts-mockito as you are adding it to another PR. I have noted this task for the future: Improve and refactor Scheduler tests using ts-mockito and adding integration tests to also add some integration testing to test syncs between two instances using the scheduler

xurxodev

Thanks @anagperal

9sneha-n

Thanks @anagperal !

Refactor Scheduler following MVP pattern

af21e76

anagperal added 3 commits February 26, 2025 15:02

Improve current usecases and repositories of scheduler and create Syn…

f4515f5

…cRuleJobConfigD2Repository

Add Api to repository name

0b824bc

Decouple configRepository in scheduler repositories and log4js librar…

dc37f70

…y in scheduler

anagperal requested a review from adrianq February 26, 2025 15:19

anagperal added 2 commits February 26, 2025 16:36

Merge development and solve conflicts

9afe5cd

Add first version of tests for Scheduler

d3abb40

anagperal marked this pull request as ready for review February 27, 2025 14:08

anagperal requested a review from 9sneha-n February 27, 2025 14:34

9sneha-n suggested changes Mar 3, 2025

View reviewed changes

adrianq requested a review from xurxodev March 3, 2025 16:21

anagperal added 5 commits March 5, 2025 16:20

Update Future entity and apiToFuture function

6d84394

Change from Promise to Future

ab651e4

Move entities, rename scheduler and add refactor comments

befd332

Split Scheduler Info in presentation and render logic

941438c

Mark refactored files

f067e27

anagperal requested a review from 9sneha-n March 5, 2025 15:54

anagperal changed the title ~~Refactor Scheduler following Clean Architecture~~ [REFACTOR] Refactor Scheduler following Clean Architecture Mar 5, 2025

xurxodev requested changes Mar 6, 2025

View reviewed changes

anagperal added 2 commits March 7, 2025 10:39

Remove SyncRuleJobConfigD2ApiRepository and get rules for Scheduler f…

b188e7b

…rom list rules usecase. Improve tests and enabled filter name

Pass directly repositories to scheduler use cases and remove fluture lib

b87cd8b

anagperal requested a review from xurxodev March 7, 2025 09:53

xurxodev approved these changes Mar 10, 2025

View reviewed changes

9sneha-n approved these changes Mar 12, 2025

View reviewed changes

adrianq merged commit 670d1cd into development Mar 13, 2025
3 checks passed

		@@ -0,0 +1,27 @@
		import { Instance } from "../../domain/instance/entities/Instance";

		@@ -0,0 +1,7 @@
		import { ScheduledJob } from "./ScheduledJob";

		export interface SchedulerContract {

Conversation

anagperal commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📌 References

📝 New refactor tasks

📝 Implementation

📹 Screenshots/Screen capture

🔥 Is there anything the reviewer should know to test it?

📑 Others

Uh oh!

ifoche commented Feb 26, 2025

Uh oh!

9sneha-n left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anagperal commented Mar 5, 2025

Uh oh!

xurxodev left a comment

Choose a reason for hiding this comment

Uh oh!

anagperal commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xurxodev left a comment

Choose a reason for hiding this comment

Uh oh!

9sneha-n left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

anagperal commented Feb 26, 2025 •

edited

Loading

anagperal commented Mar 7, 2025 •

edited

Loading