cal.com icon indicating copy to clipboard operation
cal.com copied to clipboard

feat: reconnect bookingreference for new credential

Open vijayraghav-io opened this issue 1 year ago • 16 comments

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 avatar Sep 30 '24 06:09 vijayraghav-io

@vijayraghav-io is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Sep 30 '24 06:09 vercel[bot]

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.

graphite-app[bot] avatar Sep 30 '24 06:09 graphite-app[bot]

@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.

keithwillcode avatar Oct 01 '24 17:10 keithwillcode

E2E results are ready!

github-actions[bot] avatar Oct 01 '24 17:10 github-actions[bot]

@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.

vijayraghav-io avatar Oct 02 '24 01:10 vijayraghav-io

This PR is being marked as stale due to inactivity.

github-actions[bot] avatar Oct 17 '24 00:10 github-actions[bot]

sorry for the delay, in middle of other PR, will update this PR by this week.

vijayraghav-io avatar Oct 17 '24 00:10 vijayraghav-io

creation of tests in progress....

vijayraghav-io avatar Oct 23 '24 11:10 vijayraghav-io

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 :

  1. Reverted the use of CredentialRepository wherever the Credential is being created with prisma.credential.create() as it was before.
  2. Calling the reconnectWithNewCredential() function after the credential is 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 -

  1. Replace all prisma.credential.create() with CredentialRepository.Create().
  2. BookingReferenceRepository.reconnectWithNewCredential() will be called from within the CredentialRepository.Create(). This also ensures that in future if, we have a new req to write code to create new credential , dev or reviewer need not remember to call BookingReferenceRepository.reconnectWithNewCredential() after credential creation. But just ensure to use the CredentialRepository.Create(), instead of directly using prisma.
  3. Calls to reconnectWithNewCredential() from outside the Repository introduced in this PR will have to be removed, after this current PR is merged.

vijayraghav-io avatar Oct 24 '24 17:10 vijayraghav-io

@vijayraghav-io could pls fix the conflicts🙏

anikdhabal avatar Nov 13 '24 16:11 anikdhabal

@vijayraghav-io could pls fix the conflicts🙏

🙏 sure will do

vijayraghav-io avatar Nov 13 '24 16:11 vijayraghav-io

@anikdhabal , resolved merge conflicts.

vijayraghav-io avatar Nov 13 '24 17:11 vijayraghav-io

This PR is being marked as stale due to inactivity.

github-actions[bot] avatar Dec 03 '24 00:12 github-actions[bot]

Gentle reminder!

vijayraghav-io avatar Dec 03 '24 10:12 vijayraghav-io

@emrysal Can you please review this? I know you had fixed a lot of the credentials issues with booking references in the db.

keithwillcode avatar Dec 11 '24 16:12 keithwillcode

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

vercel[bot] avatar Dec 11 '24 18:12 vercel[bot]

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!

vijayraghav-io avatar Jun 26 '25 09:06 vijayraghav-io

@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.

  1. 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
  2. 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.
  3. Replaced all prisma.credential.create in app-store packages with CredentialRepository.create so that BookingReferences.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.

vijayraghav-io avatar Jun 30 '25 10:06 vijayraghav-io

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

packages/lib/server/repository/bookingReference.test.ts

vijayraghav-io avatar Jun 30 '25 10:06 vijayraghav-io

All tests are passing!

vijayraghav-io avatar Jul 03 '25 11:07 vijayraghav-io

@vijayraghav-io can you pls fix the conflicts?

anikdhabal avatar Sep 08 '25 04:09 anikdhabal

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Sep 08 '25 04:09 coderabbitai[bot]

@anikdhabal , resolved merge conflicts and addressed latest coderabbit comments.

vijayraghav-io avatar Sep 08 '25 10:09 vijayraghav-io

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 🙏🏼

dhairyashiil avatar Nov 21 '25 19:11 dhairyashiil