Conversation
…uding database schema changes and API updates
Louis-rollet
requested review from
TheodoreBillotte,
alexis-belmonte and
Copilot
There was a problem hiding this comment.
Pull request overview
Adds backend support for forking a published (“COMPLETED”) project from the HUB, including a new API endpoint and a DB self-reference to track the source project.
Changes:
- Added
POST /projects/{id}/forkendpoint and corresponding Swagger schema/DTO. - Implemented
ProjectService.fork()to create the forked project and copy release content (and project image) in S3. - Extended Prisma
Projectmodel withforkedFromId(+ migration) to track fork lineage.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| swagger.json | Documents the new fork endpoint and adds forkedFromId + fork response schema. |
| src/routes/project/project.service.ts | Implements the fork() service method, including S3 release/image copying. |
| src/routes/project/project.service.spec.ts | Updates mocks for new Prisma field (forkedFromId). |
| src/routes/project/project.controller.ts | Exposes POST :id/fork controller route and Swagger decorators. |
| src/routes/project/dto/project-response.dto.ts | Adds forkedFromId to response DTOs and defines ForkProjectResponseDto. |
| prisma/models/project.prisma | Adds forkedFromId field and self-referential relations (forkedFrom / forks). |
| prisma/migrations/20260405081129_add_forked_from/migration.sql | Adds forked_from_id column + FK constraint. |
| .gitignore | Ignores generated_client/*. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const newProject = await this.prisma.project.create({ | ||
| data: { | ||
| name: `Fork of ${sourceProject.name}`, | ||
| shortDesc: sourceProject.shortDesc, | ||
| longDesc: sourceProject.longDesc, | ||
| forkedFrom: { connect: { id: sourceProjectId } }, | ||
| creator: { connect: { id: userId } }, | ||
| collaborators: { connect: [{ id: userId }] } | ||
| }, |
There was a problem hiding this comment.
fork() creates the new Project row before doing the S3 release copy. If downloadFile/uploadFile throws, the request fails but the forked project remains persisted without an initial save, leaving the system in an inconsistent state. Consider wrapping the post-create S3 steps in a try/catch and compensating (e.g., delete the newly created project and any partially uploaded objects) when the copy fails, or otherwise making the operation idempotent/rollback-safe.
| try { | ||
| const imageKey = `projects/${sourceProjectId}/image`; | ||
| const imageExists = await this.s3Service.fileExists(imageKey); | ||
| if (imageExists) { | ||
| const imageFile = await this.s3Service.downloadFile({ key: imageKey }); | ||
| const newImageKey = `projects/${newProject.id}/image`; | ||
| await this.s3Service.uploadFile({ | ||
| file: imageFile, | ||
| keyName: newImageKey | ||
| }); | ||
| await this.s3Service.setObjectPublicRead(newImageKey); | ||
| } | ||
| } catch { | ||
| // Image copy failure is non-critical | ||
| } |
There was a problem hiding this comment.
The image-copy block swallows all errors (catch {}) with no logging. If S3 permissions/keys are misconfigured this will fail silently and be hard to diagnose. Consider at least logging the exception (with sourceProjectId/newProject.id) or narrowing the catch to expected "not found" cases while surfacing unexpected failures.
| async fork(sourceProjectId: number, userId: number): Promise<ProjectEx> { | ||
| const sourceProject = await this.prisma.project.findUnique({ | ||
| where: { id: sourceProjectId } | ||
| }); | ||
|
|
||
| if (!sourceProject) { | ||
| throw new NotFoundException( | ||
| `Project with ID ${sourceProjectId} not found` | ||
| ); | ||
| } | ||
|
|
||
| if (sourceProject.status !== "COMPLETED") { | ||
| throw new BadRequestException( | ||
| "Only published projects can be forked" | ||
| ); | ||
| } |
There was a problem hiding this comment.
fork() introduces a new behavior path (validations + DB create + S3 copy) but there are currently no unit tests covering success and failure scenarios (e.g., non-existent source project, non-published source, missing user, S3 download/upload failure cleanup). Adding tests in project.service.spec.ts for this method would help prevent regressions and ensure the rollback/compensation behavior is correct.
|
|
||
| const newProject = await this.prisma.project.create({ | ||
| data: { | ||
| name: `Fork of ${sourceProject.name}`, |
There was a problem hiding this comment.
What would happen if someone forks a project that was already forked ? Would it be named "Fork of Fork of originalProjectName" ?
Wouldn't it be better to let the user name the new project whatever they want, but add an indicator that it is a fork from another project ?
OR, name it originalProjectName and add an indicator that it was forked
| const user = await this.prisma.user.findUnique({ | ||
| where: { id: userId } | ||
| }); | ||
|
|
||
| if (!user) { | ||
| throw new NotFoundException(`User with ID ${userId} not found`); | ||
| } |
There was a problem hiding this comment.
Whose userId is this ?
If it's the user doing the query, then the JWTGuard already ensures that the user exists, making this query and the check after this one redondant.
There was a problem hiding this comment.
True, doesn't make sense. Please do the necessary changes
| const user = await this.prisma.user.findUnique({ | ||
| where: { id: userId } | ||
| }); | ||
|
|
||
| if (!user) { | ||
| throw new NotFoundException(`User with ID ${userId} not found`); | ||
| } |
There was a problem hiding this comment.
True, doesn't make sense. Please do the necessary changes
| await this.s3Service.setObjectPublicRead(newImageKey); | ||
| } | ||
| } catch { | ||
| // Image copy failure is non-critical |
There was a problem hiding this comment.
I don't really like this: In which case would this happen? catching and doing nothing hides nasty bugs, so please avoid doing this
There was a problem hiding this comment.
Please remove this file from this PR, this will make my merge easier on my end
|
|
||
| // Relations | ||
| forkedFrom Project? @relation("ProjectForks", fields: [forkedFromId], references: [id], onDelete: SetNull) | ||
| forks Project[] @relation("ProjectForks") |
There was a problem hiding this comment.
Is this a list of projects that forked from this one?
There was a problem hiding this comment.
no it's a list of project that forked the current project
Jira ticket
https://naucto.atlassian.net/browse/NCTO-179
What does your MR do ?
Adds the option to fork a published project on the HUB, added the api paths to fork the project and db shemas to have a fork project Id ref
How to test it
Go onto the HUB and click on the fork button on any project, linked to the frontend PR
Screenshot
Notes