-
Notifications
You must be signed in to change notification settings - Fork 9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: delete past bookings #19120
base: main
Are you sure you want to change the base?
feat: delete past bookings #19120
Conversation
@hp77-creator is attempting to deploy a commit to the cal-staging Team on Vercel. A member of the Team first needs to authorize it. |
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details:
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (02/05/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (02/05/25)1 label was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (02/07/25)1 label was added to this PR based on Keith Williams's automation. "Add foundation team as reviewer" took an action on this PR • (02/07/25)1 reviewer was added to this PR based on Keith Williams's automation. |
Hi @hp77-creator, thanks for your PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screen.Recording.2025-02-06.at.9.34.55.PM.mov@retrogtx added the change and the vid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works, lgtm
E2E results are ready! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only marking as requested changes to ensure this gets a review from Foundation before merge due to the high-risk nature of this. @emrysal @zomars @ThyMinimalDev @eunjae-lee
hey, the tests in that directory is just run. you can trigger it yourself by writing |
indeed we can, I was confused because I thought this needs to go into suite as well, i will follow what you have mentioned. Thanks. |
@keithwillcode Thanks for the change request of adding test, It made me add check for unauthorized user, I have added tests, @retrogtx you may review, Thanks for your patience. |
8628f43
to
65b25a3
Compare
thank you for the PR @hp77-creator since this deals with risky parts (like permanently deleting) this will take some time! |
Thanks for the contribution , currently we going ahead with #19118 |
Any reasons @TusharBhatt1 ? |
TBH nothing major , it's just that we had 2 PRs for the same issue , both were mergeable (though I was reviewing the other one , it LGTM) |
|
||
if (booking.userId !== user.id) { | ||
throw new Error("Unauthorized: You don't have permission to delete this booking"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we handle the case where it was a team booking where Admin or Owner
are only allowed for this operation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are we expecting in that case? Are we expecting that others will be able to delete it? I have just added this functionality for users who created the booking, they can delete it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For team booking only Admin or Owner
are allowed for this operation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will add that check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apart from the check that needs to be added, everything else seems well
please resolve the conflicts 🙏
What does this PR do?
I have added two options for the user to delete past bookings.
They can delete individual bookings like this:


Or they can delete all the bookings in one go using the main button:

Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
/claim #18787