refactor(game-session-schedule): add support for cascade deletion #600
Conversation
Preview Deployment Status✅ Storybook Preview: https://691d21e5.uabc-storybook.pages.dev 🚫 Frontend Preview: Failed |
Coverage Report
File CoverageNo changed files found. |
jeffplays2005
left a comment
jeffplays2005
left a comment
There was a problem hiding this comment.
I haven't reviewed your tests yet as the foundation code logic needs some changes. Please take a look at the comments I've left behind! Thanks 👍
|
|
||
| export const gameSessionWithScheduleCreateMock: CreateGameSessionData = { | ||
| ...gameSessionCreateMock, | ||
| gameSessionSchedule: gameSessionScheduleMock, | ||
| } | ||
|
|
There was a problem hiding this comment.
this mock isn't getting used
There was a problem hiding this comment.
I didn't create the mock it was already there from a merge
| * @param transactionID an optional transaction ID for the request, useful for tracing | ||
| * @private | ||
| */ |
There was a problem hiding this comment.
I also understand that instantiations of data services without needing a constructor is poor practice but should be addressed outside of this ticket
please use public async instead of public static async
|
|
||
| return bulkDeletionResult.docs | ||
| } |
| const relatedBookings = ( | ||
| await payload.find({ | ||
| collection: "booking", | ||
| where: { | ||
| gameSession: { | ||
| equals: sessionId, | ||
| }, | ||
| }, | ||
| pagination: false, | ||
| }) | ||
| ).docs | ||
| const relatedBookingIds = relatedBookings.map((booking) => booking.id) | ||
|
|
||
| const bulkDeletionResult = await payload.delete({ | ||
| collection: "booking", | ||
| where: { | ||
| id: { in: relatedBookingIds }, | ||
| }, | ||
| req: { | ||
| transactionID, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
I'd recommend looking into advanced queries again, you could just delete all bookings where the gameSession equals the input id instead of fetching all bookings with target game session, then deleting it again.
tl;dr you're doing double the db operations when you could combine everything together
There was a problem hiding this comment.
Oops, don't know what I was thinking there
| const { id } = await params | ||
| const cascade = req.nextUrl.searchParams.get("cascade") === "true" | ||
| const gameSessionDataService = new GameSessionDataService() |
There was a problem hiding this comment.
we've shifted to use deleteRelatedDocs
| try { | ||
| const gameSessionSchedule = await gameSessionDataService.getGameSessionScheduleById(id) | ||
|
|
||
| const { docs } = await payload.find({ | ||
| collection: "gameSession", | ||
| where: { | ||
| or: [ | ||
| { | ||
| gameSessionSchedule: { | ||
| equals: id, | ||
| }, | ||
| }, | ||
| { | ||
| gameSessionSchedule: { | ||
| equals: gameSessionSchedule, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }) | ||
| const gameSession = docs[0] |
There was a problem hiding this comment.
use data service methods instead of directly using the db adapter here
this isn't too accurate as there could be multiple game sessions tied to a single schedule object. you should be creating something similar to the deleteRelatedBookingsForSession method but to delete game session's based on a game session schedule
jeffplays2005
changed the title
jeffplays2005
changed the title
|
@6xtvo Just a reminder to please make sure your PR title and description match the original issue, I've modified them, please take a look to ensure your future work is more precise! |
| import type { PaginatedDocs, Sort, Where } from "payload" | ||
| import { NotFound, type PaginatedDocs, type Sort, type Where } from "payload" | ||
| import { payload } from "@/data-layer/adapters/Payload" |
There was a problem hiding this comment.
question: why was NotFound added?
There was a problem hiding this comment.
Accidentally imported it when adding NotFound to JSDoc
| */ | ||
| public async deleteRelatedBookingsForSchedule( | ||
| scheduleId: string, |
There was a problem hiding this comment.
for consistency, stick for ...By... rather than ...For...
There was a problem hiding this comment.
also should be ...ScheduleId
| */ | ||
| public async deleteAllGameSessionsByScheduleId(scheduleId: string): Promise<GameSession[]> { | ||
| return ( |
There was a problem hiding this comment.
any reason why this doesn't support transactions?
There was a problem hiding this comment.
also I think it isn't relevant to return the deleted docs, it should be Promise instead (please update tests)
There was a problem hiding this comment.
I copied the method from another method where it returned docs for testing
There was a problem hiding this comment.
There's no need to return docs for testing here as we can use getById to see if it returns a NotFound error (it's rather inefficient to be parsing the data when it isn't needed).
| await gameSessionDataService.deleteGameSessionSchedule(id) | ||
| await gameSessionDataService.deleteAllGameSessionsByScheduleId(id) | ||
| await bookingDataService.deleteRelatedBookingsForSchedule(id, transactionID) |
There was a problem hiding this comment.
all three of these should be using transactions, please modify the first two!
Sorry this PR was created when the issue had the wrong name, I forgot to update it |
Description
Delete
GameSessionandBookingdocuments upon deletion ofGameSessionScheduleFixes #545
Type of change
How Has This Been Tested?
Checklist before requesting a review