Skip to content
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

6255 move services from messaging common module into the correct module and refactor them #6409

Conversation

bosiraphael
Copy link
Contributor

@bosiraphael bosiraphael commented Jul 25, 2024

Closes #6255

  • Move files from messaging/common into the correct module
  • Remove common module between calendar and messaging calendar-messaging-participant-manager
  • Update and fix massaging and calendar participant matching
  • Create MatchParticipantModule

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This pull request refactors and relocates services from the messaging/common module to their appropriate modules, aiming to avoid circular dependencies and improve code organization.

  • File Relocations: Moved is-email-blocklisted.util.ts and its test to blocklist module. Updated import paths accordingly.
  • Service Deletions: Removed add-person-id-and-workspace-member-id.service.ts from calendar-messaging-participant-manager.
  • Method Updates: Added workspaceId parameter in calendar-save-events.service.ts and renamed methods in calendar-event-participant.service.ts.
  • Module Refactors: Updated messaging-common.module.ts to remove several services and streamlined messaging-import-manager.module.ts with new imports and providers.
  • Job and Utility Adjustments: Updated method calls and import paths in various job and utility files to reflect the new module structure.

30 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

…ule-into-the-correct-module-and-refactor-them
Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvement, I've left additional comments

@@ -19,6 +21,7 @@ export class CalendarEventParticipantService {
public async upsertAndDeleteCalendarEventParticipants(
participantsToSave: CalendarEventParticipantWithCalendarEventId[],
participantsToUpdate: CalendarEventParticipantWithCalendarEventId[],
workspaceId: string,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you have to pass the workspaceId here, you could get it from the scopedWorkspaceContextFactory (see twenty-orm.manager.ts). This would avoid the whole drill down and secure the behavior to avoid proposing where the workspace can be provided. That's being said I'm not 100% sure about this pattern of auto-resolving the workspaceId from the request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


@Module({
imports: [WorkspaceDataSourceModule],
providers: [AddPersonIdAndWorkspaceMemberIdService],
providers: [],
exports: [],
})
export class CalendarCommonModule {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need this one? looks like nothing is provided or exported

@charlesBochet charlesBochet merged commit d0db3b7 into main Jul 27, 2024
6 checks passed
@charlesBochet charlesBochet deleted the 6255-move-services-from-messaging-common-module-into-the-correct-module-and-refactor-them branch July 27, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move services from messaging common module into the correct module and refactor them
2 participants