cal.com
cal.com copied to clipboard
feat: reconnect bookingreference for new credential
What does this PR do?
- Fixes #16785
- Fixes CAL-4398 (Linear issue number - should be visible at the bottom of the GitHub issue description)
Before Changes: https://www.loom.com/share/a3ec8e7b7181419d91b12abfa9729a99?sid=a8440350-c26e-4778-abd9-73ce539338f6
After Changes https://www.loom.com/share/a36bf823820947d2b0d2225853363885?sid=866df2d7-dafb-4a63-8e72-cd3f5ed8f293
Mandatory Tasks (DO NOT REMOVE)
- [x] I have self-reviewed the code (A decent size PR without self-review might be rejected).
- [x] - N/A - I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
- [x] I confirm automated tests are in place that prove my fix is effective or that my feature works.
@vijayraghav-io is attempting to deploy a commit to the cal Team on Vercel.
A member of the Team first needs to authorize it.
Graphite Automations
"Add consumer team as reviewer" took an action on this PR • (09/30/24)
1 reviewer was added to this PR based on Keith Williams's automation.
"Add foundation team as reviewer" took an action on this PR • (09/30/24)
1 reviewer was added to this PR based on Keith Williams's automation.
"Add community label" took an action on this PR • (09/30/24)
1 label was added to this PR based on Keith Williams's automation.
"Add platform team as reviewer" took an action on this PR • (09/30/24)
1 reviewer was added to this PR based on Keith Williams's automation.
@vijayraghav-io Really appreciate your work on this PR. Helps us move forward in using repositories more.
A suggestion for the future: avoid big refactors with feature changes.
- If the refactor causes an issue for some reason and we need to revert/rollback, the new feature is also lost in the process. Ideally, the feature is added in 1 PR and the refactor is done in another PR to avoid this situation.
- It makes reviewing the PR harder because the context is wider.
@vijayraghav-io Really appreciate your work on this PR. Helps us move forward in using repositories more.
A suggestion for the future: avoid big refactors with feature changes.
- If the refactor causes an issue for some reason and we need to revert/rollback, the new feature is also lost in the process. Ideally, the feature is added in 1 PR and the refactor is done in another PR to avoid this situation.
- It makes reviewing the PR harder because the context is wider.
Thankyou! 🙏 @keithwillcode .
Sure, will take care of having refactor in a separate PR. I can split for this PR also, will update with new PR for refactor and current PR will have only issue fix.
Also will add tests for the fix.
This PR is being marked as stale due to inactivity.
sorry for the delay, in middle of other PR, will update this PR by this week.
creation of tests in progress....
Hi @keithwillcode, Sorry for the delay! Updated this PR to have issue fix only, without using Repository approach. Also, added tests.
Updates in comparison to previous approach :
- Reverted the use of
CredentialRepositorywherever the Credential is being created withprisma.credential.create()as it was before. - Calling the
reconnectWithNewCredential()function after thecredentialis created, wherever it is being created.
IMO the best place to have this function - reconnectWithNewCredential() is inside BookingReferenceRepository.
To do: Will create a new PR that will -
- Replace all
prisma.credential.create()withCredentialRepository.Create(). BookingReferenceRepository.reconnectWithNewCredential()will be called from within theCredentialRepository.Create(). This also ensures that in future if, we have a new req to write code to create newcredential, dev or reviewer need not remember to callBookingReferenceRepository.reconnectWithNewCredential()after credential creation. But just ensure to use the CredentialRepository.Create(), instead of directly using prisma.- Calls to
reconnectWithNewCredential()from outside the Repository introduced in this PR will have to be removed, after this current PR is merged.
@vijayraghav-io could pls fix the conflicts🙏
@vijayraghav-io could pls fix the conflicts🙏
🙏 sure will do
@anikdhabal , resolved merge conflicts.
This PR is being marked as stale due to inactivity.
Gentle reminder!
@emrysal Can you please review this? I know you had fixed a lot of the credentials issues with booking references in the db.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| cal | ⬜️ Ignored (Inspect) | Visit Preview | Dec 11, 2024 6:44pm |
hey @vijayraghav-io Thanks for the PR! Just noticed there aren’t any new tests to check the "reconnectWithNewCredential" functionality could you add some automated tests to make sure orphaned BookingReferences get reconnected properly for both user and team cases? Also, there are some merge conflicts right now, so please rebase your branch onto main, fix those up, and push the changes. Once you’ve added the tests and sorted the conflicts, just ping the reviewers so we can take another look. Thanks!
sure, will resolve merge conflicts and update!
@Devanshusharma2005 , @keithwillcode , @emrysal
🙏 To reiterate the approach and updates after resolving merge conflicts (since CredentialRepository was also created by other PR).
Leveraged existing CredentialRepository to reconnect orphaned BookingReferences.
- Reconnecting BookingReferences with Credential is called in 3 scenarios - i. When New Credential is created using CredentialRepository.create ii. When New SelectedCalendar is created using SelectedCalendarRepository.create iii. When SelectedCalendar is updated using SelectedCalendarRepository.upsert
- The combination of 3 fields - 'userId or teamId' , 'credentialType' , 'credentialId==null' is used to detect orphaned bookingReference. For Calendar Apps, additional field - 'externalCalendarId' is used to detect orphaned bookingReferences.
- Replaced all
prisma.credential.createin app-store packages withCredentialRepository.createso thatBookingReferences.reconnectWithNewCredential()is called.
Tested again the functionality, latest loom - https://www.loom.com/share/a8397107cbe3416ea7ac7997ee832e15?sid=51e7aac3-8ca1-4ce7-8bc5-b4578578de43
The failing tests E2E API v2 , E2E Embed React requires re-run, were passing in previous run.
hey @vijayraghav-io Thanks for the PR! Just noticed there aren’t any new tests to check the "reconnectWithNewCredential" functionality could you add some automated tests to make sure orphaned BookingReferences get reconnected properly for both user and team cases? Also, there are some merge conflicts right now, so please rebase your branch onto main, fix those up, and push the changes. Once you’ve added the tests and sorted the conflicts, just ping the reviewers so we can take another look. Thanks!
@Devanshusharma2005 , had already added the tests for same, please refer https://github.com/calcom/cal.com/pull/16878/files#diff-6a23fb219767e34088b3d5162be4eb918fe782756890d00bd33da6e2edf6d22a
All tests are passing!
@vijayraghav-io can you pls fix the conflicts?
Walkthrough
Replaces many direct Prisma credential creations with a repository abstraction (CredentialRepository.create) across numerous app-store handlers. Adds BookingReferenceRepository.reconnectWithNewCredential and helper findCredentialByIdWithSelectedCalendar; calls reconnectWithNewCredential after credential creation and when SelectedCalendar entries are created or upserted. Introduces discriminated union types for credential creation inputs and integration tests for BookingReferenceRepository.reconnectWithNewCredential covering calendar and video booking scenarios.
Assessment against linked issues
| Objective | Addressed | Explanation |
|---|---|---|
| Update existing BookingReferences to new Credential on Credential reconnect (CAL-4398, #16785) | ✅ |
Assessment against linked issues: Out-of-scope changes
| Code Change | Explanation |
|---|---|
| Mass replacement of direct Prisma credential creation with CredentialRepository.create across many app-store handlers (e.g. packages/app-store/giphy/api/add.ts, packages/app-store/* files) | This broad data-access refactor changes many unrelated handler files and is not required by CAL-4398, which focuses on booking-reference reconnection behavior. |
| SelectedCalendarRepository.create/upsert now calls BookingReferenceRepository.reconnectWithNewCredential (packages/lib/server/repository/selectedCalendar.ts) | Calling reconnect on selected-calendar create/upsert is an additional integration point beyond the explicit "credential reconnect" trigger described in CAL-4398. |
Possibly related PRs
- calcom/cal.com#23071 — Modifies CredentialRepository and touches credential creation / booking-reference reconnection logic; likely overlaps with the repository and reconnection changes in this PR.
✨ Finishing Touches
- [ ] 📝 Generate Docstrings
🧪 Generate unit tests
- [ ] Create PR with unit tests
- [ ] Post copyable unit tests in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
@anikdhabal , resolved merge conflicts and addressed latest coderabbit comments.
Thank you so much for your time and effort on this. I checked with the team, and it looks like this issue isn’t planned for the near future. If we need it later, we’ll be sure to revisit your work. For now, I’ll close the PR. Thank you again 🙏🏼