fix: view team bookings #13591
fix: view team bookings #13591
Conversation
|
@SomayChauhan is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
bookings
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
| role: { | ||
| in: ["ADMIN", "OWNER"], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This fixes the issue, but:
As I understand a user can belong to only 1 organization (team), but what if this changes in future ? In such a case, I think code changes would allow:
- admin A invites Alice to team A and creates managed event for her
- admin B invites Alice to team B
- Alice gets a booking from managed event created by admin A
- Because admin B is a team member of Alice, I think he would see booking from team A
So I think this query is too wide and should be narrowed down unless I am missing something. What do you think?
There was a problem hiding this comment.
hmmm.. 🤔,
you're right, both team admins can see bookings of each other if they have a common user
There was a problem hiding this comment.
can't think of any to way to narrow down the query, i will let smarter people handle this one 😅
There was a problem hiding this comment.
Maybe when Managed event is created we can pass admin id and connect it to users of event type. I think right now only the owner of event is connected to there. So we might need to adjust how Managed event type is created and then query it.
There was a problem hiding this comment.
yup!, you are right, will have to try something hacky, can't think of any direct method
There was a problem hiding this comment.
@supalarry how about now ?
There was a problem hiding this comment.
@SomayChauhan just so I can follow - what part exactly in the new code guarantees that admin B would not see booking from team A compared to how it was before?
|
@SomayChauhan wanna write a test? |
|
Yes I'm working on it @PeerRich , |
There was a problem hiding this comment.
looks fine to me. will wait for @supalarry or keith before merging
keithwillcode
dismissed
supalarry’s stale review
Requested changes have been made
There was a problem hiding this comment.
Thank you for your contribution 🙏 @SomayChauhan
Just as a postmortem.
The addition of this specific query it is causing the DB CPU to be at 100%. Causing connection issues in the whole app. We will have to revert this PR in order fix it.
My main guess is that this query is missing a select statement. Causing to query ALL data from the booking.
My other guess is that this Promise.all is reaching its limit. While it might indeed be "faster" to fetch these all at once. It is also true that this will be causing CPU spikes as we seen today in the v3.8.8 release.
Thanks again for the patience 🙏🏽
This reverts commit 4585199.
What does this PR do?
Fixes #13582