Documents rework #3019
Documents rework #3019
Conversation
|
WalkthroughThe changes update the database schema and application modules to enhance document management capabilities. Updates include modifying the subproject commit reference and several SQL migrations that add new ENUMs, tables (Document, DocumentFile), and columns (such as Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DocumentController
participant DocumentService
participant DocumentRepository
Client->>DocumentController: Send createDocument request with data and file
DocumentController->>DocumentService: Forward createDocument(data, file)
DocumentService->>DocumentRepository: Create new Document record
DocumentRepository-->>DocumentService: Return created Document
DocumentService-->>DocumentController: Forward response with Document info
DocumentController-->>Client: Return success response
sequenceDiagram
participant Client
participant FileService
participant CustomerService
participant StorageService
Client->>FileService: Call uploadNewFile(projectId, workflowRuntimeData, file)
FileService->>CustomerService: Fetch customer for projectId
CustomerService-->>FileService: Return customer details
FileService->>StorageService: Request file transfer with processed filename
StorageService-->>FileService: Return file storage details (e.g., URL)
FileService-->>Client: Return upload success with file info
Suggested reviewers
Poem
✨ Finishing Touches
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: 1
🧹 Nitpick comments (14)
services/workflows-service/src/document/document.controller.external.ts (3)
37-107: Return only newly created document to reduce redundant queries.
Currently, after creating a new document, the method fetches and returns all documents for the given entity, which can be costly when the entity holds a large number of documents. Consider returning only the newly created document (and its related file info) for improved efficiency and clarity.
140-166: Optimize by returning only the updated document.
Similar to the create method, the current design fetches and returns all documents for the project upon updating a single one. Consider returning just the updated document to reduce database load and response size.
168-189: Consider returning minimal info after bulk deletions.
After deleting documents, the service returns the entire set of remaining documents, which could be inefficient. Restricting the response to confirm which documents were removed or returning a success status might be more performant.services/workflows-service/src/document/document.service.ts (3)
24-108: Ensure atomicity by leveraging transactions where possible.
Wrapping file upload and the subsequent repository operations within a single transaction ensures data consistency if any step fails. Consider invoking a transaction by default for critical create flows.
138-162: Return the updated document rather than fetching all documents.
To improve efficiency, call a repository method that returns just the updated document and its files, instead of retrieving the entire set post-update.
164-187: Limit the scope of returned data after deletions.
Fetching and returning all documents once deletions complete can significantly impact performance. Restricting the result to confirm deletion success or returning only the affected items will optimize bandwidth and client-side handling.services/workflows-service/src/storage/storage.module.ts (1)
1-1: Consider refactoring the circular dependency between StorageModule and FileModule.While
forwardRefresolves the circular dependency, it might indicate a need for restructuring. Consider:
- Extracting shared functionality into a new module
- Reviewing the responsibilities of each module to ensure proper separation of concerns
- Using events/message patterns instead of direct dependencies
This would improve maintainability and testability of the code.
Also applies to: 9-9, 12-12
services/workflows-service/src/document/dtos/document.dto.ts (1)
4-18: Consider adding field validations and documentation.While the schema is well-structured, consider:
- Adding validation for
issuingVersionformat- Adding min/max constraints for
version- Documenting the expected structure of
propertiesExample improvements:
export const DocumentSchema = Type.Object({ id: Type.String(), category: Type.String(), type: Type.String(), - issuingVersion: Type.String(), + issuingVersion: Type.String({ pattern: '^\\d+\\.\\d+\\.\\d+$' }), // Semantic version format issuingCountry: Type.String(), - version: Type.Integer(), + version: Type.Integer({ minimum: 1 }), // Version should be positive status: Type.Enum(DocumentStatus), decision: Type.Optional(Type.Enum(DocumentDecision)), - properties: Type.Record(Type.String(), Type.Any()), + // Properties schema for document-specific metadata + properties: Type.Record(Type.String(), Type.Any(), { + description: 'Document-specific metadata key-value pairs' + }), businessId: Type.Optional(Type.String()), endUserId: Type.Optional(Type.String()), workflowRuntimeDataId: Type.Optional(Type.String()), projectId: Type.String(), });services/workflows-service/src/document-file/dtos/document-file.dto.ts (1)
5-13: LGTM! Well-structured schema definition.The schema properly defines all required fields with appropriate types and uses Prisma enums for type safety.
Consider adding string validation rules (e.g., min/max length, format) for
id,documentId,fileId, andprojectIdfields to ensure data quality.export const DocumentFileSchema = Type.Object({ - id: Type.String(), + id: Type.String({ minLength: 1, maxLength: 255 }), - documentId: Type.String(), + documentId: Type.String({ minLength: 1, maxLength: 255 }), - fileId: Type.String(), + fileId: Type.String({ minLength: 1, maxLength: 255 }), - projectId: Type.String(), + projectId: Type.String({ minLength: 1, maxLength: 255 }), });services/workflows-service/src/document-file/document-file.service.ts (1)
10-61: LGTM! Well-implemented service with proper transaction support.The service follows good practices:
- Clean delegation to repository
- Consistent method signatures
- Transaction support
Consider adding error handling and logging for repository operations:
async create( data: Prisma.DocumentFileUncheckedCreateInput, args?: Prisma.DocumentFileCreateArgs, transaction?: PrismaTransactionClient, ) { + try { return await this.repository.create(data, args, transaction); + } catch (error) { + // Log the error with context + throw new Error(`Failed to create document file: ${error.message}`); + } }services/workflows-service/src/document/document.repository.ts (1)
35-51: Consider improving type safety for entity handling.The
findByEntityIdAndWorkflowIdmethod uses a genericentityIdthat could be either a business or end user ID. Consider using a discriminated union type to make this distinction explicit.type EntityReference = | { type: 'business'; businessId: string } | { type: 'endUser'; endUserId: string }; async findByEntityIdAndWorkflowId( entity: EntityReference, workflowRuntimeDataId: string, // ... rest of the parameters ) { return transaction.document.findMany({ ...args, where: { ...args?.where, ...(entity.type === 'business' ? { businessId: entity.businessId } : { endUserId: entity.endUserId }), workflowRuntimeDataId, projectId: { in: projectIds }, }, }); }services/workflows-service/src/document-file/document-file.repository.ts (1)
66-96: Consider adding optimistic concurrency control for delete operations.The delete operations use
deleteManywhich doesn't provide feedback on the number of affected rows. Consider adding version tracking for optimistic concurrency control.async deleteById( id: string, projectIds: TProjectId[], args?: Prisma.DocumentFileDeleteManyArgs, transaction: PrismaTransactionClient = this.prismaService, ) { - return transaction.documentFile.deleteMany({ + const result = await transaction.documentFile.deleteMany({ ...args, where: { ...args?.where, id, projectId: { in: projectIds }, }, }); + if (result.count === 0) { + throw new Error('Document file not found or was already deleted'); + } + return result; }services/workflows-service/src/storage/storage.service.ts (1)
21-21: Remove unused import.The
randomUUIDimport is not used in this file.-import { randomUUID } from 'crypto';services/workflows-service/src/providers/file/file.service.ts (1)
362-393: Consider adding file validation.Add validation for file size and type to prevent potential security issues and ensure data integrity.
async uploadNewFile( projectId: string, workflowRuntimeData: WorkflowRuntimeData, file: Express.Multer.File, ) { + // Validate file size (e.g., max 10MB) + const maxSize = 10 * 1024 * 1024; // 10MB + if (file.size > maxSize) { + throw new BadRequestException('File size exceeds maximum limit'); + } + + // Validate file type + const allowedTypes = ['image/jpeg', 'image/png', 'application/pdf']; + if (!allowedTypes.includes(file.mimetype)) { + throw new BadRequestException('Invalid file type'); + } + // upload file into a customer folder const customer = await this.customerService.getByProjectId(projectId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/prisma/migrations/20250129142137_documents_init/migration.sql(1 hunks)services/workflows-service/prisma/migrations/20250129151319_documents_project/migration.sql(1 hunks)services/workflows-service/prisma/migrations/20250129160127_documents_issuing_country/migration.sql(1 hunks)services/workflows-service/prisma/schema.prisma(6 hunks)services/workflows-service/scripts/generate-end-user.ts(0 hunks)services/workflows-service/src/app.module.ts(2 hunks)services/workflows-service/src/business/business.controller.external.ts(0 hunks)services/workflows-service/src/business/business.controller.ts(0 hunks)services/workflows-service/src/document-file/document-file.module.ts(1 hunks)services/workflows-service/src/document-file/document-file.repository.ts(1 hunks)services/workflows-service/src/document-file/document-file.service.ts(1 hunks)services/workflows-service/src/document-file/dtos/document-file.dto.ts(1 hunks)services/workflows-service/src/document/document.controller.external.ts(1 hunks)services/workflows-service/src/document/document.module.ts(1 hunks)services/workflows-service/src/document/document.repository.ts(1 hunks)services/workflows-service/src/document/document.service.ts(1 hunks)services/workflows-service/src/document/dtos/document.dto.ts(1 hunks)services/workflows-service/src/providers/file/file-service.module.ts(0 hunks)services/workflows-service/src/providers/file/file.module.ts(1 hunks)services/workflows-service/src/providers/file/file.service.ts(3 hunks)services/workflows-service/src/storage/storage.module.ts(1 hunks)services/workflows-service/src/storage/storage.service.ts(2 hunks)services/workflows-service/src/workflow/workflow.module.ts(2 hunks)services/workflows-service/src/workflow/workflow.service.ts(1 hunks)
💤 Files with no reviewable changes (4)
- services/workflows-service/src/business/business.controller.ts
- services/workflows-service/src/providers/file/file-service.module.ts
- services/workflows-service/scripts/generate-end-user.ts
- services/workflows-service/src/business/business.controller.external.ts
✅ Files skipped from review due to trivial changes (2)
- services/workflows-service/src/workflow/workflow.service.ts
- services/workflows-service/prisma/data-migrations
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Analyze (javascript)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
🔇 Additional comments (31)
services/workflows-service/src/document/document.controller.external.ts (1)
109-138: Straightforward retrieval logic.
The endpoint name and method logic are clear and consistent with documented functionality. No issues found.services/workflows-service/src/document/document.service.ts (2)
110-136: Well-structured retrieval logic.
Fetching documents along with their file info and returning signed URLs is implemented cleanly. No major issues observed.
189-220: Concurrency approach is solid.
Using nestedPromise.allto process files concurrently is a good pattern. The code is readable and matches common best practices for handling concurrent I/O operations.services/workflows-service/src/document-file/document-file.module.ts (1)
6-11: Module structure is clearly defined.
The module imports the necessary Prisma module, provides the service and repository, and exports the service for external use. No issues found.services/workflows-service/src/document/document.module.ts (1)
1-16: Well-structured module following NestJS best practices!The module demonstrates clear separation of concerns with dedicated controller, service, and repository layers. Dependencies are properly injected and only necessary components are exported.
services/workflows-service/src/providers/file/file.module.ts (2)
1-9: Well-structured module following NestJS best practices!The module demonstrates clear separation of concerns and proper dependency injection setup.
Also applies to: 16-20
11-15: Configure HTTP retry mechanism.The TODO comment indicates that HTTP retry mechanism configuration is pending. This is important for handling transient failures in HTTP calls.
Would you like me to help implement the retry mechanism configuration for HTTP calls? I can provide an example configuration using NestJS's built-in retry capabilities.
services/workflows-service/src/document/dtos/document.dto.ts (1)
20-26: LGTM! Well-structured derived schemas.The derived schemas (
CreateDocumentSchema,UpdateDocumentSchema, andDeleteDocumentsSchema) are properly defined using TypeBox utilities.services/workflows-service/src/workflow/workflow.module.ts (1)
47-48: LGTM!The module dependencies are properly configured with
forwardRefto handle circular dependencies.Also applies to: 60-60
services/workflows-service/src/app.module.ts (1)
52-52: LGTM!The
DocumentModuleis properly integrated into the application's module system.Also applies to: 137-137
services/workflows-service/src/providers/file/file.service.ts (1)
362-393: LGTM! The implementation is clean and secure.The method properly handles file uploads with appropriate error handling and file name sanitization.
services/workflows-service/prisma/migrations/20250129160127_documents_issuing_country/migration.sql (2)
7-8: Review SQL Column Addition Command
The SQL command to add theissuingCountrycolumn is syntactically correct. Ensure that a plan is in place for handling existing data as noted in the comments.
1-6:⚠️ Potential issueAttention: Column Addition Without Default Value
The warning explicitly notes that adding the non-nullableissuingCountrycolumn without a default value may cause issues if theDocumenttable is already populated. Please confirm that either the table is empty or that you have a plan (for example, a one-off data migration) to set the column for existing rows.services/workflows-service/prisma/migrations/20250129151319_documents_project/migration.sql (4)
8-10: ALTER TABLE on Document
The command to add theprojectIdcolumn to theDocumenttable is correct. Again, ensure consistent handling of existing data.
11-13: ALTER TABLE on DocumentFile
The command correctly adds theprojectIdcolumn to theDocumentFiletable. Please double-check the consistency with your data migration strategy.
14-19: Foreign Key Constraints Addition
Foreign key constraints forprojectIdon both tables are well defined with appropriate actions (RESTRICT on delete and CASCADE on update). Ensure that referencedProjectIDs are valid when the migration runs.
1-7:⚠️ Potential issueWarning on Required Column Additions Without Defaults
The script warns that adding the non-nullableprojectIdcolumn to bothDocumentandDocumentFilewithout default values will fail if those tables contain data. Verify that either the tables are empty or that a migration plan exists to populate these fields before enforcing NOT NULL constraints.services/workflows-service/prisma/migrations/20250129142137_documents_init/migration.sql (6)
7-18: New ENUM Types Creation
The ENUM types (DocumentStatus,DocumentDecision,DocumentFileType, andDocumentFileVariant) are defined correctly. Confirm that their values are consistent with the application logic and Prisma schema expectations.
22-39: Creation of the Document Table
The newDocumenttable is created with appropriate columns and a primary key. Confirm that the field types, especially for JSONB and timestamp columns, align with your application requirements and that the absence of a default value on certain fields (like foreign keys) is intentional.
41-51: Creation of the DocumentFile Table
TheDocumentFiletable is defined correctly with a primary key constraint. Ensure that any foreign key relationships not present in this script (handled later) match with your application model.
53-67: Index Creation for Performance Optimizations
The indexes defined on bothDocumentandDocumentFiletables appear to be set up correctly. Verify that these indexes align with your anticipated query patterns.
68-81: Addition of Foreign Key Constraints
All foreign key constraints, including those forbusinessId,endUserId, andworkflowRuntimeDataIdonDocumentas well asdocumentIdandfileIdonDocumentFile, are correctly defined. Confirm that the cascading options and deletion rules match the desired behavior.
19-21:⚠️ Potential issueDropping the 'documents' Column on Business
The ALTER TABLE command for dropping thedocumentscolumn is properly formatted. Please verify that a backup or migration plan is in place before data is lost.services/workflows-service/prisma/schema.prisma (8)
86-91: Addition of 'documents' Field to EndUser Model
The newdocuments Document[]field in theEndUsermodel introduces a one-to-many relationship withDocument. Verify that the inverse relation (i.e. theendUserIdfield in theDocumentmodel) is correctly maintained and that the relation behavior (e.g. onDelete, onUpdate) is as intended.
140-143: Addition of 'documents' Field to Business Model
The addition of thedocuments Document[]field enhances the KYB process by associating documents with a business. Ensure that this relation is synchronized with the migration changes and that any legacy references to documents are properly refactored.
306-308: Linking Files to Document Files
The addition of thedocumentFiles DocumentFile[]field in theFilemodel properly establishes the relationship withDocumentFile. Verify that queries that depend on these relations are updated accordingly.
416-418: Extending the Project Model with Document Relations
The new additionsdocuments Document[]anddocumentFiles DocumentFile[]in theProjectmodel expand the scope of associated documents and files. Confirm that these relationships are correctly referenced in your business logic and that the indexes created in the migration scripts are consistent.
955-988: New Document Model Definition
The newly addedDocumentmodel is comprehensive and closely mirrors the migration script:
- It includes all the necessary fields (e.g.,
issuingCountry,issuingVersion,version, etc.)- The relationships to
Business,EndUser,WorkflowRuntimeData, andProjectare correctly defined.Double-check that the model’s field types and constraints (e.g., non-nullable fields) match the intended database schema.
990-1000: ENUM Definitions for Document Status and Decision
The enumsDocumentStatusandDocumentDecisionare defined correctly. Ensure that their values align with your application logic and the expectations in both the migration scripts and any business rules.
1002-1012: ENUM Definitions for Document File Types and Variants
The enumsDocumentFileTypeandDocumentFileVariantare well defined. Verify that these definitions match the front-end or API expectations for document file metadata.
1014-1031: New DocumentFile Model Definition
TheDocumentFilemodel is structured correctly:
- All relations to
DocumentandFileare clearly specified.- The inclusion of a
projectIdfield reinforces the link to theProjectmodel consistently with the migration changes.Ensure that the indexing and relation integrity meet your application requirements.
services/workflows-service/prisma/migrations/20250129142137_documents_init/migration.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
services/workflows-service/prisma/migrations/20250202144546_added_document_file_cascade/migration.sql (1)
4-6: Review Drop Foreign Key for fileId
The second drop operation for the "fileId" foreign key is straightforward. As with the previous drop, consider using "IF EXISTS" if there’s any chance the constraint might already be absent in some environments.services/workflows-service/prisma/schema.prisma (3)
86-90: New Relationship in EndUser Model
The addition of thedocuments Document[]field in theEndUsermodel establishes a one-to-many relationship between an end user and related documents. This enhancement aligns with the new document management requirements.Consider also reviewing the nullable/required nature of the foreign key in the Document model to ensure it matches your business logic.
955-988: New Model: Document
A comprehensive newDocumentmodel has been introduced. It captures key fields such ascategory,type,issuingVersion,issuingCountry,version, and document state (with associated enums). Relationships to Business, EndUser, WorkflowRuntimeData, and Project are properly defined.Points to consider:
- Verify that using
Stringfor fields likecategoryandtypeis ideal; in some cases, enums might provide better constraints.- Ensure that the optional relationships (e.g.,
businessIdandendUserId) align with your overall data integrity rules.
1014-1031: New Model: DocumentFile
A newDocumentFilemodel has been added with essential fields includingtype,variant, andpagealong with defined relationships to theDocumentandFilemodels. The use ofonDelete: Cascadeon the relations is consistent with the migration updates and ensures clean-up of orphaned records. The indexes ondocumentIdandfileIdwill help optimize query performance.Consider whether additional indexes (e.g., on
projectId) might be beneficial based on your expected query patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/workflows-service/prisma/migrations/20250202144546_added_document_file_cascade/migration.sql(1 hunks)services/workflows-service/prisma/schema.prisma(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: lint
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (9)
services/workflows-service/prisma/migrations/20250202144546_added_document_file_cascade/migration.sql (3)
1-3: Review Drop Foreign Key for documentId
The script correctly drops the existing foreign key constraint for the "documentId" column. Double-check in your production environment that this constraint exists or consider using an "IF EXISTS" clause for added resilience.
7-9: Review Add Foreign Key for documentId
The new foreign key for "documentId" is added with ON DELETE CASCADE and ON UPDATE CASCADE, ensuring that deletions or updates will correctly propagate. This is in line with enforcing referential integrity for document-related files.
10-12: Review Add Foreign Key for fileId
Similarly, the new constraint for "fileId" is correctly defined with cascading update and delete rules. Confirm that cascading deletions are the intended behavior for your application’s logic concerning related file deletions.services/workflows-service/prisma/schema.prisma (6)
142-142: New Relationship in Business Model
A newdocuments Document[]field has been added to theBusinessmodel, which will allow business records to be associated with multiple documents (e.g., KYB documentation). This addition appears consistent with the intended data model for document management.
307-308: Addition to File Model: documentFiles Field
The File model now includes adocumentFiles DocumentFile[]field. This creates a link between files and their corresponding document file records, which is necessary given the new DocumentFile model and its cascading behavior defined elsewhere.
990-994: New Enum: DocumentStatus
TheDocumentStatusenum defines status values (provided,unprovided,requested) for documents. This simple enumeration seems adequate.
996-1000: New Enum: DocumentDecision
TheDocumentDecisionenum, with values such asapproved,rejected, andrevisions, is now present. Make sure these values match the decisions handled within your business logic.
1002-1006: New Enum: DocumentFileType
TheDocumentFileTypeenum now categorizes file types (e.g.,selfie,document, andother). This addition should facilitate proper type checking and documentation classification.
1008-1012: New Enum: DocumentFileVariant
TheDocumentFileVariantenum differentiates between file variants such asfront,back, andother. This is useful for managing multi-page or variant documents.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/workflows-service/src/collection-flow/collection-flow.service.ts(0 hunks)services/workflows-service/src/collection-flow/controllers/collection-flow.files.controller.ts(3 hunks)
💤 Files with no reviewable changes (1)
- services/workflows-service/src/collection-flow/collection-flow.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: build (windows-latest)
- GitHub Check: test_linux
- GitHub Check: Analyze (javascript)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
🔇 Additional comments (3)
services/workflows-service/src/collection-flow/controllers/collection-flow.files.controller.ts (3)
24-25: LGTM!The new imports for FileService and WorkflowService are correctly added and follow the project's import conventions.
31-36: LGTM!The constructor is properly updated with the new dependencies, maintaining consistent access modifiers and following dependency injection best practices.
66-70: Add error handling and clarify the empty options object.The call to
getWorkflowRuntimeDataByIdneeds improvement:
- Missing error handling if the workflow runtime data is not found
- The purpose of the empty object parameter is unclear
Could you clarify the purpose of the empty object parameter? Consider adding proper error handling:
const workflowRuntimeData = await this.workflowService.getWorkflowRuntimeDataById( tokenScope.workflowRuntimeDataId, - {}, + { /* what options are supported here? */ }, [tokenScope.projectId], ); +if (!workflowRuntimeData) { + throw new errors.NotFoundException('Workflow runtime data not found'); +}
services/workflows-service/src/collection-flow/controllers/collection-flow.files.controller.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/prisma/migrations/20250129142137_documents_init/migration.sql
Outdated
Show resolved
Hide resolved
| CREATE TYPE "DocumentStatus" AS ENUM ('provided', 'unprovided', 'requested'); | ||
|
|
||
| -- CreateEnum | ||
| CREATE TYPE "DocumentDecision" AS ENUM ('approved', 'rejected', 'revisions'); |
There was a problem hiding this comment.
Just making sure we are aligned, in the revision flow what will happen is:
- user upload doc 1
- agent will ask for reupload
- status changed to revisions on doc 1
- user upload another doc, we create doc 2 (same category and so on, but we bump the version)
- now we have doc 1 (v1) still in revision (which is okay i think because its kind of an audit log as well), and doc 2 (v2) in blank/new state
There was a problem hiding this comment.
I am missing some details, I'll have an easier time to implement that flow separately and end-to-end.
| "status" "DocumentStatus" NOT NULL, | ||
| "decision" "DocumentDecision", | ||
| "properties" JSONB NOT NULL, | ||
| "businessId" TEXT, |
There was a problem hiding this comment.
I don't think we raised this option in the planning discussion, but having a relationship table for each entity is probably the complete solution here, as it will preserve database constraints.
on the other hand, doing the migration if the future to support it should be relatively straightforward
There was a problem hiding this comment.
For properties? How would we support the various schemas?
There was a problem hiding this comment.
no, for entities (business, enduser..)
...workflows-service/prisma/migrations/20250202144546_added_document_file_cascade/migration.sql
Outdated
Show resolved
Hide resolved
services/workflows-service/src/collection-flow/collection-flow.service.ts
Outdated
Show resolved
Hide resolved
| import { DocumentFileRepository } from './document-file.repository'; | ||
| import { PrismaModule } from '@/prisma/prisma.module'; | ||
|
|
||
| @Module({ |
There was a problem hiding this comment.
Does DocumentFile need to have a module or live inside a document module?
There was a problem hiding this comment.
It is a separate repository and service. Creating a controller feels like overkill but I'd like to avoid what happened with workflow service for runtime data + workflow definition as much as I can.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
services/workflows-service/src/document-file/document-file.repository.ts (4)
10-19: Consider adding error handling.While the implementation is correct, consider adding error handling for Prisma operations and input validation for required fields.
async create( data: Prisma.DocumentFileUncheckedCreateInput, args?: Prisma.DocumentFileCreateArgs, transaction: PrismaTransactionClient = this.prismaService, ) { + try { return transaction.documentFile.create({ ...args, data, }); + } catch (error) { + if (error instanceof Prisma.PrismaClientKnownRequestError) { + // Handle specific Prisma errors (e.g., unique constraint violations) + throw new Error(`Failed to create document file: ${error.message}`); + } + throw error; + } }
21-30: Add error handling for bulk operations.Similar to the create method, consider adding error handling for bulk operations.
async createMany( data: Prisma.Enumerable<Prisma.DocumentFileCreateManyInput>, args?: Prisma.DocumentFileCreateManyArgs, transaction: PrismaTransactionClient = this.prismaService, ) { + try { return transaction.documentFile.createMany({ ...args, data, }); + } catch (error) { + if (error instanceof Prisma.PrismaClientKnownRequestError) { + throw new Error(`Failed to create document files in bulk: ${error.message}`); + } + throw error; + } }
64-78: Consider using delete instead of deleteMany for single ID operations.Since you're deleting by a specific ID, consider using
deleteinstead ofdeleteManyfor better type safety and to ensure only one record is affected.async deleteById( id: string, projectIds: TProjectId[], args?: Prisma.DocumentFileDeleteManyArgs, transaction: PrismaTransactionClient = this.prismaService, ) { + if (!projectIds?.length) { + throw new Error('At least one project ID must be provided'); + } - return transaction.documentFile.deleteMany({ + return transaction.documentFile.delete({ ...args, where: { ...args?.where, id, projectId: { in: projectIds }, }, }); }
80-94: Add validation for projectIds array.Similar to other methods, validate that the projectIds array is not empty before executing the delete operation.
async deleteByDocumentId( documentId: string, projectIds: TProjectId[], args?: Prisma.DocumentFileDeleteManyArgs, transaction: PrismaTransactionClient = this.prismaService, ) { + if (!projectIds?.length) { + throw new Error('At least one project ID must be provided'); + } return transaction.documentFile.deleteMany({ ...args, where: { ...args?.where, documentId, projectId: { in: projectIds }, }, }); }services/workflows-service/src/document/document.controller.external.ts (1)
130-138: Consider implementing pagination for document retrieval.The
getDocumentsByEntityIdAndWorkflowIdendpoint might return a large number of documents. Consider implementing pagination to improve performance and prevent memory issues.services/workflows-service/src/document/document.service.ts (1)
196-220: Optimize batch file processing.The nested Promise.all calls could be flattened for better performance and error handling.
- return await Promise.all( - documents?.map(async document => { - const files = await Promise.all( - document.files?.map(async file => { - const uploadedFile = await this.storageService.fetchFileContent({ - id: file.fileId, - projectIds: [document.projectId], - format, - }); + const fileRequests = documents?.flatMap(document => + (document.files ?? []).map(file => ({ + file, + document, + promise: this.storageService.fetchFileContent({ + id: file.fileId, + projectIds: [document.projectId], + format, + }), + })) + ) ?? []; + const uploadedFiles = await Promise.all( + fileRequests.map(async ({ promise }) => promise) + ); + const documentsMap = new Map( + documents?.map(document => [document.id, { ...document, files: [] }]) + ); + fileRequests.forEach(({ file, document }, index) => { + const uploadedFile = uploadedFiles[index]; + const documentWithFiles = documentsMap.get(document.id); + if (documentWithFiles) { + documentWithFiles.files.push({ + ...file, + mimeType: uploadedFile.mimeType, + signedUrl: uploadedFile.signedUrl, + }); + } + }); + return Array.from(documentsMap.values());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/ubos-form-json-definition.ts(2 hunks)services/workflows-service/src/document-file/document-file.repository.ts(1 hunks)services/workflows-service/src/document-file/document-file.service.ts(1 hunks)services/workflows-service/src/document/document.controller.external.ts(1 hunks)services/workflows-service/src/document/document.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/document-file/document-file.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (windows-latest)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: lint
- GitHub Check: Analyze (javascript)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (3)
apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/ubos-form-json-definition.ts (1)
1-218: LGTM: Comprehensive UBO data collection form.The form structure is well-designed with:
- Comprehensive field collection for UBO information
- Proper validation through required fields
- Specialized UI components for specific fields (PhoneInput, DateInput, CountryPicker, NationalityPicker)
services/workflows-service/src/document-file/document-file.repository.ts (1)
1-9: LGTM! Well-structured class setup following NestJS best practices.The implementation properly uses dependency injection and follows the repository pattern.
services/workflows-service/src/document/document.controller.external.ts (1)
37-46: 🛠️ Refactor suggestionConsider adding file type validation in the FileInterceptor configuration.
While the file size limit is enforced, there's no explicit file type validation in the interceptor configuration. This could potentially allow upload of malicious files.
Let's verify the file filter implementation:
apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/ubos-form-json-definition.ts
Show resolved
Hide resolved
services/workflows-service/src/document-file/document-file.repository.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/document-file/document-file.repository.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/document/document.controller.external.ts
Outdated
Show resolved
Hide resolved
* feat(workflows-service): implemented document controller, service, repository, and dto * fix(workflows-service): added cascase on delete * refactor(workflows-service): removed uploadFile method from collection flow service * feat(workflows-service): added the ability to reupload document (#3019) * feat: implemented documents upload * feat: reworked creation ui * feat: implemented document creation & deletion * feat: finalized entity group field * fix: fixed tests * fix: cleanup * fix: format fix * fix: fixed build --------- Co-authored-by: Omri Levy <[email protected]> Co-authored-by: Omri Levy <[email protected]> Co-authored-by: Shane <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
Refactor