feat: Add integration with: Sharepoint #693
Conversation
|
Qovery can create a Preview Environment for this PR.
This comment has been generated from Qovery AI 🤖.
|
|
|
|
|
Caution Review failedThe pull request is closed. WalkthroughWalkthroughThis pull request introduces significant enhancements to the project management and file storage systems, particularly through the integration with SharePoint. Key changes include the addition of a new Changes
Assessment against linked issues
Possibly related PRs
Tip OpenAI O1 model for chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
packages/api/swagger/swagger-spec.yamlis excluded by!**/*.yaml
Files selected for processing (36)
- packages/api/prisma/schema.prisma (2 hunks)
- packages/api/scripts/connectorUpdate.js (5 hunks)
- packages/api/src/@core/connections/filestorage/services/sharepoint/sharepoint.service.ts (2 hunks)
- packages/api/src/@core/sync/sync.service.ts (1 hunks)
- packages/api/src/@core/utils/types/original/original.file-storage.ts (4 hunks)
- packages/api/src/filestorage/drive/drive.module.ts (2 hunks)
- packages/api/src/filestorage/drive/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/drive/services/sharepoint/mappers.ts (1 hunks)
- packages/api/src/filestorage/drive/services/sharepoint/types.ts (1 hunks)
- packages/api/src/filestorage/file/file.module.ts (2 hunks)
- packages/api/src/filestorage/file/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/file/services/sharepoint/mappers.ts (1 hunks)
- packages/api/src/filestorage/file/services/sharepoint/types.ts (1 hunks)
- packages/api/src/filestorage/folder/folder.module.ts (2 hunks)
- packages/api/src/filestorage/folder/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/folder/services/sharepoint/mappers.ts (1 hunks)
- packages/api/src/filestorage/folder/services/sharepoint/types.ts (1 hunks)
- packages/api/src/filestorage/group/group.module.ts (2 hunks)
- packages/api/src/filestorage/group/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/group/services/sharepoint/mappers.ts (1 hunks)
- packages/api/src/filestorage/group/services/sharepoint/types.ts (1 hunks)
- packages/api/src/filestorage/permission/permission.module.ts (2 hunks)
- packages/api/src/filestorage/permission/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/permission/services/sharepoint/mappers.ts (1 hunks)
- packages/api/src/filestorage/permission/services/sharepoint/types.ts (1 hunks)
- packages/api/src/filestorage/sharedlink/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/sharedlink/services/sharepoint/mappers.ts (1 hunks)
- packages/api/src/filestorage/sharedlink/services/sharepoint/types.ts (1 hunks)
- packages/api/src/filestorage/sharedlink/sharedlink.module.ts (3 hunks)
- packages/api/src/filestorage/user/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/user/services/sharepoint/mappers.ts (1 hunks)
- packages/api/src/filestorage/user/services/sharepoint/types.ts (1 hunks)
- packages/api/src/filestorage/user/user.module.ts (2 hunks)
- packages/shared/src/connectors/enum.ts (1 hunks)
- packages/shared/src/connectors/index.ts (1 hunks)
- packages/shared/src/connectors/metadata.ts (1 hunks)
Additional context used
Biome
packages/api/src/filestorage/group/services/sharepoint/index.ts
[error] 22-22: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 54-54: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/filestorage/permission/services/sharepoint/index.ts
[error] 22-22: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 77-77: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/filestorage/sharedlink/services/sharepoint/index.ts
[error] 23-23: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 78-78: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/filestorage/file/services/sharepoint/index.ts
[error] 89-89: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/filestorage/file/services/sharepoint/mappers.ts
[error] 46-46: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
packages/api/src/filestorage/folder/services/sharepoint/index.ts
[error] 69-69: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
[error] 86-88: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 160-160: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/scripts/connectorUpdate.js
[error] 268-271: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 420-420: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
Additional comments not posted (61)
packages/shared/src/connectors/index.ts (1)
7-7: Approved addition of 'sharepoint' to FILESTORAGE_PROVIDERS.The addition of 'sharepoint' to the
FILESTORAGE_PROVIDERSarray is consistent with the PR's objective to integrate SharePoint into the file storage vertical. This change should enhance the module's capabilities without affecting existing functionality.Run the following script to verify the consistent usage of
FILESTORAGE_PROVIDERSacross the codebase:Verification successful
Consistent usage of
FILESTORAGE_PROVIDERSverified.The addition of 'sharepoint' to the
FILESTORAGE_PROVIDERSarray is consistently used across the codebase, particularly in utility and service files related to file storage operations. This change aligns with the PR's objective and does not affect existing functionality.
packages/shared/src/utils.ts: Used in provider merging functions.packages/api/src/filestorage/group/sync/sync.service.ts: Iterates over providers for syncing operations.packages/api/src/filestorage/user/sync/sync.service.ts: Similar usage for user syncing.packages/api/src/filestorage/folder/sync/sync.service.ts: Used for folder syncing.packages/api/src/filestorage/drive/sync/sync.service.ts: Used for drive syncing.packages/api/src/filestorage/file/sync/sync.service.ts: Used for file syncing.packages/api/src/ats/tag/sync/sync.service.ts: Used in ATS tag syncing.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistent usage of `FILESTORAGE_PROVIDERS` across the codebase. # Test: Search for the usage of `FILESTORAGE_PROVIDERS`. Expect: Consistent usage across the codebase. rg --type typescript -A 5 $'FILESTORAGE_PROVIDERS'Length of output: 84
Script:
#!/bin/bash # Description: Verify the consistent usage of `FILESTORAGE_PROVIDERS` across the codebase. # Test: Search for the usage of `FILESTORAGE_PROVIDERS`. Expect: Consistent usage across the codebase. rg -A 5 'FILESTORAGE_PROVIDERS'Length of output: 10842
packages/shared/src/connectors/enum.ts (2)
30-30: Approved addition of 'SHAREPOINT' to FilestorageConnectors enum.The addition of
SHAREPOINT = 'sharepoint'to theFilestorageConnectorsenum is a necessary update to represent SharePoint as a file storage option, aligning with the PR's objectives.
2-25: Formatting changes approved.The reformatting of enum members improves code readability and maintainability without altering functionality.
packages/api/src/filestorage/drive/drive.module.ts (1)
1-2: Approved addition of SharePoint services.The addition of
SharepointDriveMapperandSharepointServiceto thedrive.module.tsfile is consistent with the PR's objective to integrate SharePoint functionality. These services should enhance the module's capabilities by allowing it to interact with SharePoint services effectively.Run the following script to verify the correct implementation and integration of
SharepointServiceandSharepointDriveMapper:Also applies to: 22-23
Verification successful
Integration of SharePoint Services Verified
The
SharepointServiceandSharepointDriveMapperare correctly implemented and integrated into thedrive.module.tsfile, aligning with the PR's objective to enhance SharePoint functionality. Their presence in the module's providers confirms their integration.
drive.module.tsimports and integrates bothSharepointServiceandSharepointDriveMapper.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct implementation and integration of SharePoint services. # Test: Search for the implementation details of `SharepointService` and `SharepointDriveMapper`. Expect: Correct implementation and integration. rg --type typescript -A 10 $'SharepointService|SharepointDriveMapper'Length of output: 105
Script:
#!/bin/bash # Description: Verify the correct implementation and integration of SharePoint services. # Search for the implementation details of `SharepointService` and `SharepointDriveMapper` in TypeScript files. rg --glob '*.ts' -A 10 'SharepointService|SharepointDriveMapper'Length of output: 26654
packages/api/src/filestorage/file/file.module.ts (2)
1-2: Import statements are correct.The import paths and syntax are correctly used for
SharepointFileMapperandSharepointService.
29-30: Proper integration of SharePoint services in the providers array.The addition of
SharepointServiceandSharepointFileMapperto the providers array is correctly done. Ensure that these services are implemented correctly and tested to verify that they integrate smoothly with the existing functionalities.Run the following script to verify the implementation of these services:
packages/api/src/filestorage/user/user.module.ts (2)
1-2: Import statements are correct.The import paths and syntax are correctly used for
SharepointUserMapperandSharepointService.
28-29: Proper integration of SharePoint services in the providers array.The addition of
SharepointServiceandSharepointUserMapperto the providers array is correctly done. Ensure that these services are implemented correctly and tested to verify that they integrate smoothly with the existing functionalities.Run the following script to verify the implementation of these services:
Verification successful
Verification successful: SharePoint services are correctly implemented and integrated.
The
SharepointServiceandSharepointUserMapperare properly implemented and integrated into the codebase. Their addition to the providers array inuser.module.tsis appropriate and consistent with their usage across different modules. No issues were found with their implementation.
SharepointServiceis implemented in multiple modules, indicating its versatility.SharepointUserMapperis implemented and registered with a mappers registry.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of SharepointService and SharepointUserMapper. # Test: Search for the implementation of these services. Expect: Proper implementation and no errors. rg --type typescript -A 5 $'SharepointService|SharepointUserMapper'Length of output: 103
Script:
#!/bin/bash # Description: Verify the implementation of SharepointService and SharepointUserMapper. # Test: Search for the implementation of these services. Expect: Proper implementation and no errors. rg --type ts -A 5 $'SharepointService|SharepointUserMapper'Length of output: 17550
packages/api/src/filestorage/group/group.module.ts (2)
1-2: Import statements are correct.The import paths and syntax are correctly used for
SharepointGroupMapperandSharepointService.
29-30: Proper integration of SharePoint services in the providers array.The addition of
SharepointServiceandSharepointGroupMapperto the providers array is correctly done. Ensure that these services are implemented correctly and tested to verify that they integrate smoothly with the existing functionalities.Run the following script to verify the implementation of these services:
Verification successful
Verification of SharePoint services implementation successful.
The
SharepointServiceandSharepointGroupMapperare correctly implemented and integrated across various modules, including the group module. This confirms that the integration of these services is consistent with the intended design and functionality.
SharepointServiceis implemented in multiple contexts, each serving a specific service interface.SharepointGroupMapperis implemented and registered in the group services.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of SharepointService and SharepointGroupMapper. # Test: Search for the implementation of these services. Expect: Proper implementation and no errors. rg --type typescript -A 5 $'SharepointService|SharepointGroupMapper'Length of output: 104
Script:
#!/bin/bash # Description: Verify the implementation of SharepointService and SharepointGroupMapper. # Test: Search for the implementation of these services in .ts files. rg 'SharepointService|SharepointGroupMapper' --glob '*.ts' -A 5Length of output: 17567
packages/api/src/filestorage/folder/folder.module.ts (1)
1-2: Verify import paths.The imports for
SharepointFolderMapperandSharepointServiceare correctly placed. However, ensure that the paths are correct and the files exist at these locations.Verification successful
Verify the existence of
SharepointFolderMapper.The file
mappers.tswas not found in the expected directory, which suggests that the import path forSharepointFolderMappermight be incorrect or the file is missing. I will search for the definition ofSharepointFolderMapperin the codebase to verify its location.
Search for
SharepointFolderMapperin the codebase.The previous attempt to locate
SharepointFolderMapperdid not yield any results. I will now search for any mention ofSharepointFolderMapperin the codebase to gather more information about its definition or usage.
Import paths are correct.
The import paths for
SharepointFolderMapperandSharepointServiceare correct. TheSharepointFolderMapperis defined inmappers.ts, andindex.tsexists forSharepointService.
SharepointFolderMapperis correctly imported from./services/sharepoint/mappers.SharepointServiceis correctly imported from./services/sharepoint.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the imported files. fd 'sharepoint/mappers.ts' packages/api/src/filestorage/folder/services/sharepoint fd 'index.ts' packages/api/src/filestorage/folder/services/sharepointLength of output: 217
Script:
#!/bin/bash # Search for the definition of SharepointFolderMapper in the codebase. ast-grep --lang typescript --pattern 'class SharepointFolderMapper { $$$ }'Length of output: 77
Script:
#!/bin/bash # Search for any mention of SharepointFolderMapper in the codebase. rg 'SharepointFolderMapper'Length of output: 363
packages/api/src/filestorage/sharedlink/services/sharepoint/types.ts (1)
1-29: Well-documented type definitions.The type definitions for SharePoint shared links are well-documented and include references to the official Microsoft documentation. Ensure that these types are used consistently across the application and match the data structures returned by SharePoint.
Verification successful
Types are well-integrated and used consistently.
The
SharepointSharedLinkInputandSharepointSharedLinkOutputtypes are used in multiple files across the codebase, indicating they are well-integrated into the application's logic. They are part of the service and mapping layers, as well as the broader type system for handling shared links. No further action is required.
- Files where types are used:
packages/api/src/filestorage/sharedlink/services/sharepoint/index.tspackages/api/src/filestorage/sharedlink/services/sharepoint/mappers.tspackages/api/src/@core/utils/types/original/original.file-storage.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of SharePoint types across the application. fd 'SharepointSharedLinkInput|SharepointSharedLinkOutput' packages/api/srcLength of output: 76
Script:
#!/bin/bash # Use ripgrep to search for the usage of SharepointSharedLinkInput and SharepointSharedLinkOutput across the entire codebase. rg 'SharepointSharedLinkInput|SharepointSharedLinkOutput' --context 5Length of output: 11527
packages/api/src/filestorage/user/services/sharepoint/index.ts (1)
13-63: Approve the implementation of SharePoint user service.The
SharepointServiceclass for user synchronization is well-implemented, with clear logging and error handling. The use of template literals and structured error handling in this class is commendable.packages/api/src/filestorage/user/services/sharepoint/mappers.ts (2)
11-20: Well-structured class with proper service registration.The
SharepointUserMapperclass is well-structured and correctly uses dependency injection and service registration. This setup enhances modularity and maintainability.
32-53: Robust implementation ofunifymethod.The
unifymethod handles both single and multiple user inputs effectively. UsingPromise.allfor handling multiple inputs concurrently is a good practice for performance.packages/api/src/filestorage/group/services/sharepoint/mappers.ts (2)
11-20: Consistent class structure with proper service registration.The
SharepointGroupMapperclass follows a consistent structure with the user mapper, which is good for maintainability. Proper use of dependency injection and service registration is noted.
32-53: Effective implementation ofunifymethod.The
unifymethod effectively handles both single and multiple group inputs, usingPromise.allfor concurrency. This approach is efficient and should be maintained.packages/api/src/filestorage/drive/services/sharepoint/index.ts (2)
15-27: Well-structured class with proper service registration.The
SharepointServiceclass is well-structured and correctly uses dependency injection and service registration. This setup enhances modularity and maintainability.
37-70: Robust implementation ofsyncmethod.The
syncmethod is well-implemented with comprehensive error handling and logging. This method effectively handles the synchronization of SharePoint drives, ensuring robust operation and error management.packages/api/src/filestorage/drive/services/sharepoint/mappers.ts (3)
12-25: Constructor implementation is correct.The constructor properly sets up dependency injection and registers the
SharepointDriveMapperservice. This is a standard practice in NestJS for service registration and dependency management.
37-58: Well-implementedunifymethod.The
unifymethod efficiently handles both single and array inputs ofSharepointDriveOutput. The use ofPromise.allfor parallel processing of array inputs is appropriate and follows best practices for asynchronous operations.
60-85: Correct implementation ofmapSingleDriveToUnified.The
mapSingleDriveToUnifiedmethod is well-implemented, providing a robust mechanism for mapping SharePoint drive data to a unified format. The handling of custom field mappings is particularly well done, ensuring flexibility in data integration.packages/api/src/filestorage/permission/services/sharepoint/mappers.ts (3)
12-24: Ensure proper service registration and dependency injection.The
SharepointPermissionMapperclass is correctly decorated with@Injectable(), ensuring it is properly managed by NestJS's dependency injection system. The registration of this service within themappersRegistryis done in the constructor, which is a good practice as it encapsulates the registration logic with the service creation.
36-63: Review and optimize theunifymethod.The
unifymethod handles both single and array inputs ofSharepointPermissionOutput, which is a good approach for flexibility. UsingPromise.allfor handling arrays is efficient for parallel processing. However, ensure that error handling is robust, especially when dealing with external data transformations.Consider adding error handling or logging mechanisms to capture and respond to any issues during the permission mapping process.
65-94: Validate custom field mapping logic.The
mapSinglePermissionToUnifiedmethod correctly handles custom field mappings and converts SharePoint permission data to a unified format. The use of a dynamic object forfield_mappingsand the conditional mapping based oncustomFieldMappingsare well-implemented. However, ensure that thepermissionobject properties accessed viamapping.remote_idare always defined to avoid runtime errors.Add checks or TypeScript type guards to ensure that all accessed properties exist on the
permissionobject to prevent potential runtime errors.packages/api/src/filestorage/sharedlink/services/sharepoint/mappers.ts (3)
14-27: Ensure proper service registration and dependency injection.The
SharepointSharedLinkMapperclass is correctly decorated with@Injectable(), ensuring it is properly managed by NestJS's dependency injection system. The registration of this service within themappersRegistryis done in the constructor, which is a good practice as it encapsulates the registration logic with the service creation.
39-65: Review and optimize theunifymethod.The
unifymethod handles both single and array inputs ofSharepointSharedLinkOutput, which is a good approach for flexibility. UsingPromise.allfor handling arrays is efficient for parallel processing. However, ensure that error handling is robust, especially when dealing with external data transformations.Consider adding error handling or logging mechanisms to capture and respond to any issues during the shared link mapping process.
68-94: Validate custom field mapping logic.The
mapSingleSharedLinkToUnifiedmethod correctly handles custom field mappings and converts SharePoint shared link data to a unified format. The use of a dynamic object forfield_mappingsand the conditional mapping based oncustomFieldMappingsare well-implemented. However, ensure that thesharedlinkobject properties accessed viamapping.remote_idare always defined to avoid runtime errors.Add checks or TypeScript type guards to ensure that all accessed properties exist on the
sharedlinkobject to prevent potential runtime errors.packages/api/src/@core/utils/types/original/original.file-storage.ts (1)
60-60: Review and approve the updated and new type definitions.The modifications and additions to the type definitions in
original.file-storage.tsare well-implemented. The inclusion of SharePoint types alongside Box types for various file storage entities ensures that the system can handle data from both services effectively. This change is crucial for supporting a multi-provider architecture and enhances the system's flexibility and interoperability.The new types for shared links (
OriginalSharedlinkInputandOriginalSharedlinkOutput) are also correctly defined, encapsulating inputs and outputs from both Box and SharePoint, which is essential for comprehensive shared link management.Also applies to: 63-63, 66-66, 72-72, 75-75, 78-78, 92-92, 95-95, 98-98, 104-104, 107-107, 110-110, 121-127
packages/api/src/filestorage/file/services/sharepoint/index.ts (2)
1-12: Imports are correctly used.All imports are necessary and correctly used in the file.
13-25: Class setup and dependency injection are correctly implemented.The
SharepointServiceclass is properly decorated and implements the required interface. Dependency injection and service registration are handled appropriately.packages/api/src/filestorage/file/services/sharepoint/mappers.ts (3)
1-17: Imports are correctly used.All imports are necessary and correctly used in the file.
18-31: Class setup and dependency injection are correctly implemented.The
SharepointFileMapperclass is properly decorated and implements the required interface. Dependency injection and service registration are handled appropriately.
33-135: Refactor to use modern JavaScript standards and verify data transformation logic.The methods
desunifyandunifyhandle data transformation correctly. However, to align with modern JavaScript standards, consider usingNumber.parseIntinstead ofparseIntat line 46.- size: parseInt(source.size), + size: Number.parseInt(source.size),Additionally, ensure that the logic for handling custom field mappings and transforming data is thoroughly tested, especially when dealing with arrays and complex data structures.
Tools
Biome
[error] 46-46: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
packages/api/src/filestorage/folder/services/sharepoint/mappers.ts (3)
1-15: Imports are correctly used.All imports are necessary and correctly used in the file.
16-29: Class setup and dependency injection are correctly implemented.The
SharepointFolderMapperclass is properly decorated and implements the required interface. Dependency injection and service registration are handled appropriately.
31-145: Verify data transformation logic and ensure comprehensive testing.The methods
desunifyandunifyhandle data transformation correctly. Ensure that the logic for handling custom field mappings and transforming data is thoroughly tested, especially when dealing with arrays and complex data structures.packages/api/src/filestorage/drive/services/sharepoint/types.ts (7)
5-32: Well-defined TypeScript interface for SharePoint DriveThe
SharepointDriveOutputinterface is well-structured and aligns with the Microsoft Graph API documentation. The use ofreadonlyproperties ensures immutability where appropriate, and the optional properties are correctly marked.
38-59: Comprehensive Identity Set InterfaceThe
IdentitySetinterface is flexible and adequately represents various identity types that might be associated with a SharePoint resource. The optional properties allow for customization based on the presence or absence of specific identity information.
65-74: Flexible Generic Identity InterfaceThe
Identityinterface is well-designed to represent a generic identity within SharePoint. The optional properties provide flexibility, allowing the interface to adapt to different scenarios where some identity details might not be available.
79-92: Detailed and Immutable Quota InterfaceThe
Quotainterface is thoroughly defined, covering all aspects of storage quota management in SharePoint. The use ofreadonlyproperties ensures that the quota details are immutable, which is appropriate for this type of data.
97-100: Concise Storage Plan Information InterfaceThe
StoragePlanInformationinterface is effectively focused on providing information about storage plan upgrades. ThereadonlypropertyupgradeAvailableis appropriately used to represent immutable data.
105-120: Comprehensive SharePoint Identifiers InterfaceThe
SharepointIdsinterface is well-defined, covering all necessary SharePoint-specific identifiers for an item. The use ofreadonlyproperties ensures the immutability of these identifiers, which is crucial for maintaining data integrity.
125-128: Flexible System Metadata InterfaceThe
SystemFacetinterface is minimalistic yet flexible, providing a means to include system-specific metadata for a drive. The use of an index signature allows for the addition of properties as needed, while maintaining immutability.packages/api/src/filestorage/folder/services/sharepoint/index.ts (1)
1-17: Well-structured imports and setup.The imports and initial setup using
@Injectable()are correctly implemented and follow NestJS best practices.packages/api/src/@core/connections/filestorage/services/sharepoint/sharepoint.service.ts (1)
125-126: Approved URL change for OAuth token requests.The update to use
https://login.microsoftonline.com/common/oauth2/v2.0/tokenaligns with modern OAuth2 practices and enhances security. This change is a significant improvement.Also applies to: 218-219
packages/api/src/filestorage/group/services/sharepoint/types.ts (1)
1-128: Well-documented and comprehensive type definitions for SharePoint groups.The type definitions are thorough and include detailed documentation, which will be very helpful for developers integrating with SharePoint. Great job on covering all necessary attributes and providing clear descriptions.
packages/api/src/filestorage/file/services/sharepoint/types.ts (1)
1-210: Well-structured TypeScript definitions for SharePoint integration.The file is well-organized with clear documentation and appropriate use of TypeScript features. The modular imports and use of readonly properties where necessary are commendable.
packages/api/src/filestorage/user/services/sharepoint/types.ts (1)
1-203: Well-structured TypeScript definitions for SharePoint user integration.The file is well-organized with clear documentation and appropriate use of TypeScript features. The modular imports and use of readonly properties where necessary are commendable.
packages/api/prisma/schema.prisma (1)
2133-2146: Well-structured addition ofprojects_pull_frequencymodel.The new model
projects_pull_frequencyis well-defined and aligns with the existing schema's design principles. It effectively captures pull frequency data across different verticals, enhancing the schema's capability to manage project-related data synchronization.packages/shared/src/connectors/metadata.ts (1)
2767-2767: Approved: Activation of SharePoint ConnectorThe change to set the
activeproperty of the SharePoint connector totrueis approved. This change is crucial for enabling the SharePoint integration as part of the file storage vertical.packages/api/src/filestorage/permission/services/sharepoint/types.ts (6)
11-30: Well-defined TypeScript interface for SharePoint permissions.The
SharepointPermissionOutputinterface is correctly defined and well-documented, aligning with the Microsoft Graph API specifications. The use of optional fields and specific types forrolesenhances type safety and API usability.
36-43: Correctly defined interface for sharing invitations.The
SharingInvitationinterface is well-structured and appropriately uses read-only properties to ensure data integrity. This aligns with the expected behavior of invitation data, which should not be modified after being set.
49-62: Accurately defined interface for sharing links.The
SharingLinkinterface is comprehensive and well-documented, with appropriate use of optional fields and specific types for properties liketypeandscope. This ensures flexibility and type safety in the implementation.
68-73: Well-extended interface for SharePoint identity.The
SharePointIdentitySetinterface smartly extendsIdentitySetby adding SharePoint-specific fields likesiteGroupandsiteUser. This is a good example of extending base interfaces to add functionality specific to a domain.
79-96: Comprehensive interface for SharePoint permission input.The
SharepointPermissionInputinterface is thoroughly defined, covering all necessary aspects of permission handling in SharePoint. The detailed documentation and use of specific types for fields likerolesenhance clarity and enforce correct usage.
102-109: Effectively defined interface for drive recipients.The
DriveRecipientinterface is well-designed, providing essential fields to identify recipients in sharing scenarios. The use of optional fields allows for flexibility in specifying recipient details.packages/api/src/filestorage/folder/services/sharepoint/types.ts (4)
1-6: Review imports and dependencies.The imports from
@filestorage/drive/services/sharepoint/typesand@filestorage/permission/services/sharepoint/typesare correctly specified. Ensure that these modules are properly versioned and maintained as they are critical dependencies for the SharePoint integration.
70-80: Review theFileSystemInfointerface.The
FileSystemInfointerface is well-defined with clear documentation on each property. The use ofreadonlyfor all properties is appropriate, ensuring that file system metadata cannot be altered once set.This interface adheres to best practices for data integrity and immutability.
101-121: Review theItemReferenceinterface.The
ItemReferenceinterface is crucial for linking items within SharePoint. It includes essential identifiers and metadata for drive items and list items. The use ofreadonlyfor most properties is appropriate, emphasizing the non-mutable nature of these references.Ensure that the
pathproperty is correctly handled in URL encoding and decoding scenarios to prevent issues with special characters.
82-99: Review theFolderViewinterface.The
FolderViewinterface provides options for sorting and viewing the contents of a folder. The use of enums or string literals forsortBy,sortOrder, andviewTypeis a good approach as it restricts the values to predefined options, reducing errors.However, ensure that all possible values for these properties are documented and supported in the implementation to avoid confusion and runtime errors.
| SharepointService, | ||
| SharepointFolderMapper, |
There was a problem hiding this comment.
Ensure integration and testing of new services.
SharepointService and SharepointFolderMapper have been correctly added to the providers array. Ensure that these services are fully integrated and functional within the module. Consider adding unit tests to verify their behavior and integration.
Would you like help with writing the unit tests for these services?
| SharepointService, | ||
| SharepointPermissionMapper, |
There was a problem hiding this comment.
Ensure integration and testing of new services.
SharepointService and SharepointPermissionMapper have been correctly added to the providers array. Ensure that these services are fully integrated and functional within the module. Consider adding unit tests to verify their behavior and integration.
Would you like help with writing the unit tests for these services?
| import { SharepointPermissionMapper } from './services/sharepoint/mappers'; | ||
| import { SharepointService } from './services/sharepoint'; |
There was a problem hiding this comment.
Tip
Codebase Verification
Issues found with import paths.
The file mappers.ts for SharepointPermissionMapper was not found in the specified directory, indicating a potential issue with the import path. However, index.ts exists, which may correctly export SharepointService.
- Verify the location or existence of
mappers.tsforSharepointPermissionMapper. - Confirm that
SharepointServiceis exported fromindex.ts.
Analysis chain
Verify import paths.
The imports for SharepointPermissionMapper and SharepointService are correctly placed. However, ensure that the paths are correct and the files exist at these locations.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the imported files.
fd 'sharepoint/mappers.ts' packages/api/src/filestorage/permission/services/sharepoint
fd 'index.ts' packages/api/src/filestorage/permission/services/sharepoint
Length of output: 229
| import { SharepointSharedLinkMapper } from './services/sharepoint/mappers'; | ||
| import { BoxSharedLinkMapper } from './services/box/mappers'; | ||
| import { SharepointService } from './services/sharepoint'; | ||
| import { BoxService } from './services/box'; |
There was a problem hiding this comment.
Remove duplicate import and approve new service integrations.
The addition of SharepointSharedLinkMapper, BoxSharedLinkMapper, SharepointService, and BoxService is crucial for the integration with SharePoint and Box. However, there is a duplicate import for BoxSharedLinkMapper that should be removed to clean up the code.
Apply this diff to remove the duplicate import:
- BoxSharedLinkMapper,Also applies to: 37-40
| @Injectable() | ||
| export class SharepointService implements IGroupService { | ||
| constructor( | ||
| private prisma: PrismaService, | ||
| private logger: LoggerService, | ||
| private cryptoService: EncryptionService, | ||
| private registry: ServiceRegistry, | ||
| ) { | ||
| this.logger.setContext( | ||
| FileStorageObject.group.toUpperCase() + ':' + SharepointService.name, | ||
| ); | ||
| this.registry.registerService('sharepoint', this); | ||
| } | ||
|
|
||
| async sync(data: SyncParam): Promise<ApiResponse<SharepointGroupOutput[]>> { | ||
| try { | ||
| const { linkedUserId } = data; | ||
| const connection = await this.prisma.connections.findFirst({ | ||
| where: { | ||
| id_linked_user: linkedUserId, | ||
| provider_slug: 'sharepoint', | ||
| vertical: 'filestorage', | ||
| }, | ||
| }); | ||
| const resp = await axios.get(`${connection.account_url}/groups`, { | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${this.cryptoService.decrypt( | ||
| connection.access_token, | ||
| )}`, | ||
| }, | ||
| }); | ||
|
|
||
| this.logger.log(`Synced sharepoint groups !`); | ||
|
|
||
| return { | ||
| data: resp.data.value, | ||
| message: 'Sharepoint groups retrieved', | ||
| statusCode: 200, | ||
| }; | ||
| } catch (error) { | ||
| throw error; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor to use template literals and remove redundant catch clause.
The implementation of the SharepointService class is generally solid. However, there are a couple of improvements suggested by static analysis:
- Use template literals for string concatenation to improve readability and performance.
- Remove the redundant catch clause that only rethrows the error, as it adds unnecessary complexity.
Apply these diffs to address the issues:
- FileStorageObject.group.toUpperCase() + ':' + SharepointService.name,
+ `${FileStorageObject.group.toUpperCase()}:${SharepointService.name}`,
- } catch (error) {
- throw error;
- }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Injectable() | |
| export class SharepointService implements IGroupService { | |
| constructor( | |
| private prisma: PrismaService, | |
| private logger: LoggerService, | |
| private cryptoService: EncryptionService, | |
| private registry: ServiceRegistry, | |
| ) { | |
| this.logger.setContext( | |
| FileStorageObject.group.toUpperCase() + ':' + SharepointService.name, | |
| ); | |
| this.registry.registerService('sharepoint', this); | |
| } | |
| async sync(data: SyncParam): Promise<ApiResponse<SharepointGroupOutput[]>> { | |
| try { | |
| const { linkedUserId } = data; | |
| const connection = await this.prisma.connections.findFirst({ | |
| where: { | |
| id_linked_user: linkedUserId, | |
| provider_slug: 'sharepoint', | |
| vertical: 'filestorage', | |
| }, | |
| }); | |
| const resp = await axios.get(`${connection.account_url}/groups`, { | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${this.cryptoService.decrypt( | |
| connection.access_token, | |
| )}`, | |
| }, | |
| }); | |
| this.logger.log(`Synced sharepoint groups !`); | |
| return { | |
| data: resp.data.value, | |
| message: 'Sharepoint groups retrieved', | |
| statusCode: 200, | |
| }; | |
| } catch (error) { | |
| throw error; | |
| } | |
| } | |
| } | |
| @Injectable() | |
| export class SharepointService implements IGroupService { | |
| constructor( | |
| private prisma: PrismaService, | |
| private logger: LoggerService, | |
| private cryptoService: EncryptionService, | |
| private registry: ServiceRegistry, | |
| ) { | |
| this.logger.setContext( | |
| `${FileStorageObject.group.toUpperCase()}:${SharepointService.name}`, | |
| ); | |
| this.registry.registerService('sharepoint', this); | |
| } | |
| async sync(data: SyncParam): Promise<ApiResponse<SharepointGroupOutput[]>> { | |
| try { | |
| const { linkedUserId } = data; | |
| const connection = await this.prisma.connections.findFirst({ | |
| where: { | |
| id_linked_user: linkedUserId, | |
| provider_slug: 'sharepoint', | |
| vertical: 'filestorage', | |
| }, | |
| }); | |
| const resp = await axios.get(`${connection.account_url}/groups`, { | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${this.cryptoService.decrypt( | |
| connection.access_token, | |
| )}`, | |
| }, | |
| }); | |
| this.logger.log(`Synced sharepoint groups !`); | |
| return { | |
| data: resp.data.value, | |
| message: 'Sharepoint groups retrieved', | |
| statusCode: 200, | |
| }; | |
| } | |
| } | |
| } |
Tools
Biome
[error] 22-22: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 54-54: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
| /** | ||
| * Represents the input for a folder item in OneDrive. | ||
| * @see https://learn.microsoft.com/en-us/graph/api/resources/driveitem?view=graph-rest-1.0 | ||
| */ | ||
| export interface SharepointFolderInput { | ||
| /** The unique identifier of the item within the Drive. */ | ||
| readonly id?: string; | ||
| /** The name of the item (filename and extension). */ | ||
| name?: string; | ||
| /** The URL that displays the resource in the browser. */ | ||
| readonly webUrl?: string; | ||
| /** Folder metadata. */ | ||
| folder?: Folder; | ||
| /** File system information on the client. */ | ||
| fileSystemInfo?: FileSystemInfo; | ||
| /** Parent information, if the item has a parent. */ | ||
| parentReference?: ItemReference; | ||
| /** The unique identifier of the drive instance that contains the driveItem. */ | ||
| readonly driveId?: string; | ||
| /** Identifies the type of drive. */ | ||
| readonly driveType?: string; | ||
| /** Information about the deleted state of the item. */ | ||
| deleted?: Deleted; | ||
| /** Description of the item. */ | ||
| description?: string; | ||
| /** Indicates the number of children contained immediately within this folder. */ | ||
| readonly childCount?: number; | ||
| /** Information about pending operations on the item. */ | ||
| pendingOperations?: PendingOperations; | ||
| /** View recommendations for the folder. */ | ||
| folderView?: FolderView; | ||
| /** SharePoint identifiers useful for REST compatibility. */ | ||
| readonly sharepointIds?: SharepointIds; | ||
| /** Special folder metadata. */ | ||
| readonly specialFolder?: SpecialFolder; | ||
| /** Identity of the user who created the folder. */ | ||
| readonly createdByUser?: IdentitySet; | ||
| /** Identity of the user who last modified the folder. */ | ||
| readonly lastModifiedByUser?: IdentitySet; | ||
| /** Permissions associated with the folder. */ | ||
| permissions?: SharepointPermissionOutput[]; | ||
| /** Date and time the item was last modified. Read-only. */ | ||
| readonly lastModifiedDateTime?: string; | ||
| /** Date and time of item creation. Read-only. */ | ||
| readonly createdDateTime?: string; | ||
| /** Size of the item in bytes. Read-only. */ | ||
| readonly size?: number; | ||
| /** Identity of the user, device, and application that created the item. Read-only. */ | ||
| readonly createdBy?: IdentitySet; | ||
| /** Identity of the user, device, and application that last modified the item. Read-only. */ | ||
| readonly lastModifiedBy?: IdentitySet; | ||
| } |
There was a problem hiding this comment.
Review the SharepointFolderInput interface.
This interface is well-documented and covers a comprehensive set of properties for a SharePoint folder. Each property is appropriately typed and includes read-only attributes where mutation should be restricted. This aligns well with the principles of immutability for data that should not change after creation.
Consider adding more specific types or enums for fields like driveType, sortBy, sortOrder, and viewType to enforce stricter validation rules and reduce the risk of runtime errors due to incorrect values.
| /** | ||
| * Represents the folder metadata. | ||
| */ | ||
| export interface Folder { | ||
| /** The number of children contained immediately within this container. */ | ||
| readonly childCount?: number; | ||
| /** A collection of properties defining the recommended view for the folder. */ | ||
| view?: FolderView; | ||
| } |
There was a problem hiding this comment.
Review the Folder interface.
The Folder interface is succinct and relevant. It correctly uses the readonly modifier for childCount to prevent modifications, which is a good practice. The optional view property allows flexibility in specifying folder views.
Consider providing more detailed documentation for the view property to clarify what the "recommended view" entails and how it should be used.
| /** | ||
| * Represents information about pending operations on an item. | ||
| */ | ||
| export interface PendingOperations { | ||
| /** Indicates that an operation that might update the binary content of a file is pending completion. */ | ||
| readonly pendingContentUpdate?: PendingContentUpdate; | ||
| } | ||
|
|
||
| /** | ||
| * Represents information about an operation that might affect the binary content of the driveItem. | ||
| */ | ||
| export interface PendingContentUpdate { | ||
| /** Date and time the pending binary operation was queued in UTC time. */ | ||
| readonly queuedDateTime?: string; | ||
| } |
There was a problem hiding this comment.
Review the PendingOperations and PendingContentUpdate interfaces.
These interfaces define the structure for handling operations that are pending on SharePoint items. The use of readonly in PendingContentUpdate is appropriate for the timestamp, ensuring that the operation time cannot be altered.
The documentation is clear, but consider adding examples or more detailed descriptions of scenarios where these operations might be used to enhance understanding.
| /** | ||
| * Represents special folder metadata. | ||
| */ | ||
| export interface SpecialFolder { | ||
| /** The unique identifier for this item in the /drive/special collection. */ | ||
| readonly name?: string; | ||
| } | ||
|
|
||
| /** | ||
| * Represents information about the deleted state of an item. | ||
| */ | ||
| export interface Deleted { | ||
| /** Represents the state of the deleted item. */ | ||
| state?: string; | ||
| } |
There was a problem hiding this comment.
Review the Deleted and SpecialFolder interfaces.
The Deleted interface provides a simple yet effective way to handle deleted state information. The optional state property allows flexibility in representing different states of deletion.
The SpecialFolder interface is minimalistic and serves its purpose. However, consider adding more properties if there are other relevant metadata aspects that could be useful for special folders.
| state?: string; | ||
| } | ||
|
|
||
| export type SharepointFolderOutput = SharepointFolderInput; |
There was a problem hiding this comment.
Confirm the type alias for SharepointFolderOutput.
The type alias SharepointFolderOutput is set to be identical to SharepointFolderInput, which simplifies the handling of folder data. This is a practical approach, especially if the input and output structures are expected to be the same.
Ensure that this design choice is consistent with the overall architecture and that any potential deviations between input and output structures in the future are carefully managed.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
packages/api/swagger/swagger-spec.yamlis excluded by!**/*.yaml
Files selected for processing (2)
- packages/api/src/@core/sync/sync.service.ts (1 hunks)
- packages/shared/src/connectors/metadata.ts (1 hunks)
Additional comments not posted (2)
packages/api/src/@core/sync/sync.service.ts (1)
541-541: Removeconsole.logand rely on the existinglogger.errormethod.The previous review comment is still applicable:
Using
console.logcan lead to performance issues and does not align with best practices for error handling in production environments. It is recommended to rely on the existinglogger.errormethod, which integrates with the application's centralized logging system.Please remove the newly added
console.log(error);statement and continue usinglogger.errorfor consistent error logging.packages/shared/src/connectors/metadata.ts (1)
Line range hint
2771-2778: LGTM!The code changes are approved. Enabling the SharePoint connector by setting
activetotruewill allow it to participate in the application's functionality, enhancing its usability and operational capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- packages/shared/src/connectors/metadata.ts (1 hunks)
Additional comments not posted (2)
packages/shared/src/connectors/metadata.ts (2)
2763-2771: LGTM!The code changes to expand the
scopesfor the 'sharepoint' connector are approved. The added scopesSites.Read.AllandSites.ReadWrite.Allwill enhance the connector's capabilities for accessing SharePoint resources, allowing for more comprehensive interactions with SharePoint data.
2771-2771: LGTM, but verify the impact of activating the connector.The code change to activate the 'sharepoint' connector by setting
activetotrueis approved.However, activating a previously dormant connector can potentially trigger new features or integrations. Ensure that this change is thoroughly tested to confirm that it behaves as expected and does not introduce any breaking changes or unintended side effects.
To verify the impact of activating the 'sharepoint' connector:
Identify and analyze all code paths that depend on the
activeproperty of the 'sharepoint' connector. This can be done by searching for usages of the 'sharepoint' key within theCONNECTORS_METADATAobject.Review the identified code paths to understand how the application behaves when the connector is active vs inactive. Look for any conditional logic, feature flags, or configuration settings that may be affected by this change.
Conduct thorough testing of the affected functionality to ensure that activating the connector does not introduce any bugs, performance issues, or unintended behavior changes. This may involve manual testing, automated tests, and monitoring of the application in a staging or production environment.
Engage with relevant stakeholders, such as product managers, designers, and end-users, to validate that the impact of activating the connector aligns with their expectations and requirements. Seek their feedback and approval before proceeding with the change.
By following these steps, you can verify the impact of activating the 'sharepoint' connector and ensure a smooth rollout of the associated features and integrations.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- packages/shared/src/connectors/metadata.ts (1 hunks)
Additional comments not posted (2)
packages/shared/src/connectors/metadata.ts (2)
2763-2763: LGTM!The changes to expand the scopes for the SharePoint connector look good. The additional permissions will enable broader access to SharePoint resources and facilitate improved interactions with SharePoint data.
2771-2771: LGTM!Activating the SharePoint connector by setting
activetotruelooks good. This signifies that the connector is now operational.
amuwal
had a problem deploying
to
SELF_HOSTED
GitHub Actions
Failure
amuwal
had a problem deploying
to
SELF_HOSTED
GitHub Actions
Failure
amuwal
had a problem deploying
to
SELF_HOSTED
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 10
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (20)
- packages/api/src/@core/connections/filestorage/services/sharepoint/sharepoint.service.ts (3 hunks)
- packages/api/src/@core/sync/sync.service.ts (1 hunks)
- packages/api/src/@core/utils/types/original/original.file-storage.ts (5 hunks)
- packages/api/src/filestorage/drive/drive.module.ts (2 hunks)
- packages/api/src/filestorage/file/file.module.ts (2 hunks)
- packages/api/src/filestorage/folder/folder.module.ts (2 hunks)
- packages/api/src/filestorage/folder/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/group/group.module.ts (2 hunks)
- packages/api/src/filestorage/group/services/onedrive/mappers.ts (1 hunks)
- packages/api/src/filestorage/group/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/group/services/sharepoint/mappers.ts (1 hunks)
- packages/api/src/filestorage/permission/permission.module.ts (2 hunks)
- packages/api/src/filestorage/permission/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/sharedlink/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/sharedlink/sharedlink.module.ts (2 hunks)
- packages/api/src/filestorage/user/services/sharepoint/index.ts (1 hunks)
- packages/api/src/filestorage/user/user.module.ts (2 hunks)
- packages/shared/src/connectors/enum.ts (1 hunks)
- packages/shared/src/connectors/index.ts (1 hunks)
- packages/shared/src/connectors/metadata.ts (2 hunks)
Additional context used
Biome
packages/api/src/filestorage/group/services/sharepoint/index.ts
[error] 58-58: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/filestorage/permission/services/sharepoint/index.ts
[error] 77-77: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/filestorage/sharedlink/services/sharepoint/index.ts
[error] 78-78: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/filestorage/folder/services/sharepoint/index.ts
[error] 87-89: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
[error] 161-161: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
Additional comments not posted (45)
packages/shared/src/connectors/index.ts (1)
7-7: LGTM!The addition of 'sharepoint' to the
FILESTORAGE_PROVIDERSarray is necessary to support the SharePoint integration and is consistent with the PR objective. The change is straightforward and does not introduce any issues.packages/shared/src/connectors/enum.ts (4)
2-8: Formatting changes look good!The indentation adjustments enhance code readability and maintainability. Great job ensuring consistent formatting across the enum members.
12-16: Formatting changes look good!The indentation adjustments enhance code readability and maintainability. Great job ensuring consistent formatting across the enum members.
20-25: Formatting changes look good!The indentation adjustments enhance code readability and maintainability. Great job ensuring consistent formatting across the enum members.
29-31: Great addition of theSHAREPOINTconnector!The inclusion of the
SHAREPOINTmember in theFilestorageConnectorsenum expands the range of supported file storage integrations. This change aligns perfectly with the PR objective of integrating SharePoint into the file storage vertical. Well done!packages/api/src/filestorage/drive/drive.module.ts (2)
3-4: LGTM!The imports for
SharepointDriveMapperandSharepointServiceare correctly added, following the existing import style. This indicates that SharePoint integration is being introduced to the module.
26-27: LGTM!The
SharepointServiceandSharepointDriveMapperare correctly added to the module'sprovidersarray. This makes them available for dependency injection within the module, enabling the SharePoint integration functionality.packages/api/src/filestorage/file/file.module.ts (2)
1-2: LGTM!The imports for
SharepointFileMapperandSharepointServiceare correctly specified and align with the existing import pattern in the file. These imports are part of the SharePoint integration enhancement, as indicated in the AI-generated summary.
31-32: LGTM!The addition of
SharepointServiceandSharepointFileMapperto theprovidersarray in the@Moduledecorator is correct and follows the existing pattern in the file. This change ensures that these services are available for dependency injection within the module, enabling the integration with SharePoint for file storage operations, as indicated in the AI-generated summary.packages/api/src/filestorage/user/user.module.ts (2)
1-2: LGTM!The new imports for
SharepointUserMapperandSharepointServiceare correctly added, following the existing import style. This change extends the module's functionality to integrate with SharePoint for user management.
30-31: Looks good!The
SharepointServiceandSharepointUserMapperare correctly added to the module'sprovidersarray, following the existing pattern. This change makes the services available for dependency injection, enabling the module to utilize SharePoint-related functionality.packages/api/src/filestorage/group/group.module.ts (2)
31-31: LGTM!The addition of
SharepointServiceto theprovidersarray is consistent with the objective of integrating SharePoint into the file storage vertical. This change enables theGroupModuleto leverage the functionality provided by theSharepointService.
32-32: LGTM!The addition of
SharepointGroupMapperto theprovidersarray is consistent with the objective of integrating SharePoint into the file storage vertical. This change enables theGroupModuleto leverage the functionality provided by theSharepointGroupMapper, which is likely responsible for mapping SharePoint-specific group data to a format that can be consumed by the application.packages/api/src/filestorage/folder/folder.module.ts (2)
1-2: The past review comment on lines 31-32 is still applicable here.The comment has already covered the key points about ensuring integration and testing of the new
SharepointServiceandSharepointFolderMapperservices.
31-32: The past review comment on these lines is still applicable and covers the necessary points.packages/api/src/filestorage/permission/permission.module.ts (2)
1-2: The past review comment flagging the potential issue with the import path forSharepointPermissionMapperis still valid. Please verify the location or existence ofmappers.tsforSharepointPermissionMapper.
36-37: The past review comment suggesting to ensure integration and testing ofSharepointServiceandSharepointPermissionMapperis still valid. Please consider adding unit tests to verify their behavior and integration.packages/api/src/filestorage/sharedlink/sharedlink.module.ts (4)
1-2: LGTM!The imports for
SharepointSharedLinkMapperandSharepointServiceare necessary for the SharePoint integration.
5-5: Remove the duplicate import forBoxSharedLinkMapper.The past review comment flagging the duplicate import for
BoxSharedLinkMapperis still valid.Apply this diff to remove the duplicate import:
-import { BoxSharedLinkMapper } from './services/box/mappers';
36-37: LGTM!The addition of
SharepointServiceandSharepointSharedLinkMapperto the module providers is crucial for the SharePoint integration.
38-39: LGTM!The addition of
OnedriveServiceandOnedriveSharedLinkMapperto the module providers is crucial for the OneDrive integration.packages/api/src/filestorage/group/services/onedrive/mappers.ts (1)
79-79: LGTM!Initializing the
usersproperty to an empty array is a good practice as it ensures that the property is always an array, even if it is empty. This change simplifies the downstream logic that interacts with this property, as it eliminates the need for additional checks fornullvalues.packages/api/src/filestorage/group/services/sharepoint/index.ts (1)
57-59: Remove the redundant catch clause.The catch clause at lines 57-59 only rethrows the original error without any additional handling. It is redundant and can be removed to improve code clarity.
Apply this diff to remove the redundant catch clause:
- } catch (error) { - throw error; - }Tools
Biome
[error] 58-58: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/filestorage/user/services/sharepoint/index.ts (2)
14-25: LGTM!The
SharepointServiceclass structure and dependency injection look good. The necessary dependencies are injected, and the service is registered with theServiceRegistry.
27-66: Overall, thesyncmethod implementation looks good!The method retrieves the SharePoint connection details, makes the necessary API request, and returns the retrieved users with appropriate success message and status code. The error logging is also handled correctly.
packages/api/src/filestorage/group/services/sharepoint/mappers.ts (2)
13-20: LGTM!The constructor method correctly registers the service with the mappers registry.
32-53: LGTM!The
unifymethod correctly handles both single and array input and delegates the mapping of a single group to a private method.packages/api/src/filestorage/permission/services/sharepoint/index.ts (1)
27-79: Remove redundant catch block and clarify theextraparameter.The past review comment is still applicable. Please address the following:
- Remove the redundant catch block that merely rethrows the error, as it does not add value and is flagged by static analysis tools.
- } catch (error) { - throw error; - }
- Clarify the source and purpose of the
extraparameter in thesyncmethod comment at line 32. The current comment does not provide sufficient information.Tools
Biome
[error] 77-77: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
packages/api/src/@core/utils/types/original/original.file-storage.ts (10)
95-98: LGTM!The addition of
SharepointFileInputto theOriginalFileInputunion type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling file inputs from different providers.
101-104: LGTM!The addition of
SharepointFolderInputto theOriginalFolderInputunion type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling folder inputs from different providers.
122-125: LGTM!The addition of
SharepointGroupInputto theOriginalGroupInputunion type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling group inputs from different providers.
128-131: LGTM!The addition of
SharepointUserInputto theOriginalUserInputunion type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling user inputs from different providers.
145-148: LGTM!The addition of
SharepointFileOutputto theOriginalFileOutputunion type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling file outputs from different providers.
151-154: LGTM!The addition of
SharepointFolderOutputto theOriginalFolderOutputunion type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling folder outputs from different providers.
172-175: LGTM!The addition of
SharepointGroupOutputto theOriginalGroupOutputunion type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling group outputs from different providers.
178-181: LGTM!The addition of
SharepointUserOutputto theOriginalUserOutputunion type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling user outputs from different providers.
194-195: LGTM!The addition of
OnedriveSharedLinkInputandSharepointSharedLinkInputto theOriginalSharedlinkInputunion type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling shared link inputs from different providers.
199-200: LGTM!The addition of
OnedriveSharedLinkOutputandSharepointSharedLinkOutputto theOriginalSharedlinkOutputunion type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling shared link outputs from different providers.packages/api/src/filestorage/folder/services/sharepoint/index.ts (1)
165-187: LGTM!The
syncmethod is correctly implemented and uses theiterativeGetSharepointFoldersmethod to retrieve the folders. The logging statements provide useful information about the sync process.packages/api/src/@core/connections/filestorage/services/sharepoint/sharepoint.service.ts (1)
167-167: LGTM!The usage of
site_idin theaccount_urlconstruction looks good. This change aligns with the new mechanism introduced to dynamically fetch thesite_idusing the Microsoft Graph API.Also applies to: 186-186
packages/api/src/@core/sync/sync.service.ts (1)
541-541: Removeconsole.logfor error logging in production.Using
console.logcan lead to performance issues and does not align with best practices for error handling in production environments. Please remove theconsole.logstatement and rely on the existinglogger.errormethod, which integrates with the application's centralized logging system.packages/shared/src/connectors/metadata.ts (4)
2721-2721: LGTM!The expanded scopes allow for more comprehensive access to user and group data, enabling richer interactions with OneDrive resources.
2748-2748: LGTM!The updated scopes significantly increase the connector's capabilities, enabling it to perform a wider range of operations on SharePoint resources.
2751-2752: LGTM!The modified
apiUrlproperty allows for dynamically constructing the API URL based on the providedsite_id, providing flexibility to interact with different SharePoint sites. The URL construction logic is correct.
2756-2756: Verify the SharePoint connector's functionality.The
activeproperty has been set totrue, indicating that the SharePoint connector is now enabled within the application. Please ensure that the connector is functioning as expected and users can successfully utilize its features.Run the following script to verify the SharePoint connector's functionality:
Verification successful
SharePoint connector is present and likely functional
The codebase analysis confirms the presence of a SharePoint connector implementation. The
SharepointConnectionServiceclass and its integration with the service registry indicate that the connector is set up for use. While we couldn't directly observe the 'active' property change, the use ofCONNECTORS_METADATAfor SharePoint configuration suggests that the connector is intended to be operational.To ensure full functionality:
- Test the SharePoint connector in the running application
- Verify that SharePoint-related features are accessible to users
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the SharePoint connector's functionality. # Test: Search for code related to the SharePoint connector. Expect: Relevant code segments. rg --type typescript -A 5 $'sharepoint' # Test: Search for the SharePoint connector's usage in the codebase. Expect: Code segments demonstrating the connector's integration. rg --type typescript -A 5 $'CONNECTORS_METADATA\\.sharepoint'Length of output: 170
Script:
#!/bin/bash # Description: Verify the SharePoint connector's functionality using alternative methods. # Find files containing 'sharepoint' (case-insensitive) and display their contents fd -i sharepoint -x cat {} # Search for 'sharepoint' in all files (case-insensitive) grep -ri 'sharepoint' . # Search for 'CONNECTORS_METADATA.sharepoint' in all files grep -r 'CONNECTORS_METADATA\.sharepoint' . # Find files with '.ts' extension containing 'sharepoint' (case-insensitive) fd -e ts -i sharepoint -x cat {}Length of output: 52635
| import { EncryptionService } from '@@core/@core-services/encryption/encryption.service'; | ||
| import { LoggerService } from '@@core/@core-services/logger/logger.service'; | ||
| import { PrismaService } from '@@core/@core-services/prisma/prisma.service'; | ||
| import { ApiResponse } from '@@core/utils/types'; | ||
| import { SyncParam } from '@@core/utils/types/interface'; | ||
| import { FileStorageObject } from '@filestorage/@lib/@types'; | ||
| import { IGroupService } from '@filestorage/group/types'; | ||
| import { Injectable } from '@nestjs/common'; | ||
| import axios from 'axios'; | ||
| import { ServiceRegistry } from '../registry.service'; | ||
| import { SharepointGroupOutput } from './types'; | ||
|
|
||
| @Injectable() | ||
| export class SharepointService implements IGroupService { | ||
| constructor( | ||
| private prisma: PrismaService, | ||
| private logger: LoggerService, | ||
| private cryptoService: EncryptionService, | ||
| private registry: ServiceRegistry, | ||
| ) { | ||
| this.logger.setContext( | ||
| `${FileStorageObject.group.toUpperCase()}:${SharepointService.name}`, | ||
| ); | ||
| this.registry.registerService('sharepoint', this); | ||
| } | ||
|
|
||
| async sync(data: SyncParam): Promise<ApiResponse<SharepointGroupOutput[]>> { | ||
| try { | ||
| const { linkedUserId } = data; | ||
| const connection = await this.prisma.connections.findFirst({ | ||
| where: { | ||
| id_linked_user: linkedUserId, | ||
| provider_slug: 'sharepoint', | ||
| vertical: 'filestorage', | ||
| }, | ||
| }); | ||
| // remove /sites/site_id from account_url | ||
| const url = connection.account_url.replace(/\/sites\/.+$/, ''); | ||
|
|
||
| // ref: https://learn.microsoft.com/en-us/graph/api/user-list?view=graph-rest-1.0&tabs=http | ||
| const resp = await axios.get(`${url}/groups`, { | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${this.cryptoService.decrypt( | ||
| connection.access_token, | ||
| )}`, | ||
| }, | ||
| }); | ||
|
|
||
| this.logger.log(`Synced sharepoint groups !`); | ||
|
|
||
| return { | ||
| data: resp.data.value, | ||
| message: 'Sharepoint groups retrieved', | ||
| statusCode: 200, | ||
| }; | ||
| } catch (error) { | ||
| throw error; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
LGTM with minor improvements!
The implementation of the SharepointService class looks good overall. It correctly retrieves SharePoint groups using the Microsoft Graph API.
Just a couple of suggestions to improve the code:
- Use template literals for string concatenation to improve readability and performance, as mentioned in the past review comments.
- Remove the redundant catch clause, as flagged by the static analysis hint and the past review comments.
With these minor improvements, the code changes can be approved.
Tools
Biome
[error] 58-58: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
| const resp = await axios.get(`${url}/users`, { | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${this.cryptoService.decrypt( | ||
| connection.access_token, | ||
| )}`, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Consider adding validation for the API response status code and data structure.
To ensure the API request was successful and the response data has the expected structure, it's a good practice to add some validation checks.
You can check the response status code and verify that the value property exists in the response data:
const resp = await axios.get(`${url}/users`, {
headers: {
'Content-Type': 'application/json',
Authorization: `Bearer ${this.cryptoService.decrypt(
connection.access_token,
)}`,
},
});
+if (resp.status !== 200) {
+ throw new Error(`Failed to retrieve SharePoint users. Status code: ${resp.status}`);
+}
+
+if (!resp.data.value) {
+ throw new Error('Unexpected response data structure. Missing "value" property.');
+}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resp = await axios.get(`${url}/users`, { | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${this.cryptoService.decrypt( | |
| connection.access_token, | |
| )}`, | |
| }, | |
| }); | |
| const resp = await axios.get(`${url}/users`, { | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| Authorization: `Bearer ${this.cryptoService.decrypt( | |
| connection.access_token, | |
| )}`, | |
| }, | |
| }); | |
| if (resp.status !== 200) { | |
| throw new Error(`Failed to retrieve SharePoint users. Status code: ${resp.status}`); | |
| } | |
| if (!resp.data.value) { | |
| throw new Error('Unexpected response data structure. Missing "value" property.'); | |
| } |
| const connection = await this.prisma.connections.findFirst({ | ||
| where: { | ||
| id_linked_user: linkedUserId, | ||
| provider_slug: 'sharepoint', | ||
| vertical: 'filestorage', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Consider adding error handling for the case when the connection is not found.
If the findFirst query doesn't find a matching connection, it will return null. This case should be handled to avoid potential TypeErrors when accessing properties on null.
You can add a null check and throw an appropriate error if the connection is not found:
const connection = await this.prisma.connections.findFirst({
where: {
id_linked_user: linkedUserId,
provider_slug: 'sharepoint',
vertical: 'filestorage',
},
});
+if (!connection) {
+ throw new Error('SharePoint connection not found for the given user');
+}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const connection = await this.prisma.connections.findFirst({ | |
| where: { | |
| id_linked_user: linkedUserId, | |
| provider_slug: 'sharepoint', | |
| vertical: 'filestorage', | |
| }, | |
| }); | |
| const connection = await this.prisma.connections.findFirst({ | |
| where: { | |
| id_linked_user: linkedUserId, | |
| provider_slug: 'sharepoint', | |
| vertical: 'filestorage', | |
| }, | |
| }); | |
| if (!connection) { | |
| throw new Error('SharePoint connection not found for the given user'); | |
| } |
| // todo: do something about users | ||
| // https://graph.microsoft.com/groups/group-id/members | ||
|
|
There was a problem hiding this comment.
Address the TODO comment and add error handling.
The TODO comment about handling users within groups indicates an incomplete feature that needs to be addressed to ensure full functionality.
Additionally, consider adding error handling and logging to enhance the robustness and maintainability of the method. For example, handle potential errors when making the API call to retrieve group members and log any errors encountered during the mapping process.
| ): Promise<ApiResponse<SharepointSharedLinkOutput[]>> { | ||
| try { | ||
| const { linkedUserId, extra } = data; | ||
| // TODO: where it comes from ?? extra?: { object_name: 'folder' | 'file'; value: string }, |
There was a problem hiding this comment.
Clarify the source and purpose of the extra parameter.
The comment at line 33 indicates that the source and purpose of the extra parameter are unclear. Please provide more context about where this parameter comes from and how it is used within the sync method.
| export type OriginalPermissionInput = | ||
| | any | ||
| | OnedrivePermissionInput | ||
| | SharepointPermissionInput; |
There was a problem hiding this comment.
Consider removing the any type from the union.
The addition of SharepointPermissionInput to the OriginalPermissionInput union type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling permission inputs from different providers.
However, the presence of the any type in the union may lead to a lack of type safety. Consider removing the any type and replacing it with a more specific type or creating a separate type for the cases where any is currently used.
| export type OriginalDriveInput = | ||
| | any | ||
| | OnedriveDriveInput | ||
| | SharepointDriveInput; |
There was a problem hiding this comment.
Consider removing the any type from the union.
The addition of SharepointDriveInput to the OriginalDriveInput union type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling drive inputs from different providers.
However, the presence of the any type in the union may lead to a lack of type safety. Consider removing the any type and replacing it with a more specific type or creating a separate type for the cases where any is currently used.
| export type OriginalPermissionOutput = | ||
| | any | ||
| | OnedrivePermissionOutput | ||
| | SharepointPermissionOutput; |
There was a problem hiding this comment.
Consider removing the any type from the union.
The addition of SharepointPermissionOutput to the OriginalPermissionOutput union type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling permission outputs from different providers.
However, the presence of the any type in the union may lead to a lack of type safety. Consider removing the any type and replacing it with a more specific type or creating a separate type for the cases where any is currently used.
| export type OriginalDriveOutput = | ||
| | any | ||
| | OnedriveDriveOutput | ||
| | SharepointDriveOutput; |
There was a problem hiding this comment.
Consider removing the any type from the union.
The addition of SharepointDriveOutput to the OriginalDriveOutput union type is consistent with the overall objective of integrating SharePoint types into the existing type definitions. It allows for a unified approach to handling drive outputs from different providers.
However, the presence of the any type in the union may lead to a lack of type safety. Consider removing the any type and replacing it with a more specific type or creating a separate type for the cases where any is currently used.
| // get site_id from tenant and sitename | ||
| const site_details = await axios.get( | ||
| `https://graph.microsoft.com/v1.0/sites/${tenant}.sharepoint.com:/sites/${site}`, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${data.access_token}`, | ||
| }, | ||
| }, | ||
| ); | ||
| const site_id = site_details.data.id; |
There was a problem hiding this comment.
Approve the changes with a suggestion to add error handling.
The code changes introduce a new mechanism to dynamically fetch the site_id using the Microsoft Graph API, which is an improvement over using the site parameter directly. This change aligns with the best practices for accessing SharePoint resources by using the correct identifiers in API calls.
The axios library is being used to make the API call and the access_token from the OAuth response is being used for authentication, which looks good.
Consider adding error handling for the API call to gracefully handle any errors that may occur during the request. You can use a try-catch block to catch any errors and log them using the logger service.
naelob
had a problem deploying
to
SELF_HOSTED
GitHub Actions
Failure
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 9198067 | Triggered | Generic Password | b938345 | .env.example | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Add sharepoint integration to filestorage vertical #691
Objects
demo: https://drive.google.com/file/d/1xoi82RnrPExNGc7aF1MrVhFnoWy6_hmk/view?usp=drive_link
closes #691
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor