Skip to content

fix: stripe integration in EventTypeSettings atom #20919

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Ryukemeister
Copy link
Contributor

@Ryukemeister Ryukemeister commented Apr 23, 2025

What does this PR do?

  • Fixes stripe atom not working in EventTypeSettings atom
  • Fixes CAL-5556 (Linear issue number - should be visible at the bottom of the GitHub issue description)

Image Demo:

Before
Screenshot 2025-04-24 at 12 46 24 AM

After
Screenshot 2025-04-24 at 12 45 37 AM

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • (N/A) I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • (N/A) I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

This can be tested in the examples app

Copy link

linear bot commented Apr 23, 2025

@keithwillcode keithwillcode added core area: core, team members only platform Anything related to our platform plan labels Apr 23, 2025
@@ -299,7 +299,7 @@ export class EventTypesAtomService {
userCredentialIds,
invalidCredentialIds,
teams,
isInstalled: !!userCredentialIds.length || !!teams.length || app.isGlobal,
isInstalled: (!!userCredentialIds.length || !!teams.length || app.isGlobal) ?? app.installed,
Copy link
Contributor Author

@Ryukemeister Ryukemeister Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turns out for an individual user sometimes no user credentials or app.isGlobal property is present (which is weird) which results in isInstalled property set to undefined which we don't want, hence we should fallback to app.installed which is a depreceated property but should work for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will never happen for team events since there will always be a team length present if its a team event

@Ryukemeister Ryukemeister marked this pull request as ready for review April 23, 2025 19:20
@Ryukemeister Ryukemeister requested review from a team as code owners April 23, 2025 19:20
Copy link

vercel bot commented Apr 23, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Apr 26, 2025 10:24am
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Apr 26, 2025 10:24am

@dosubot dosubot bot added the 🐛 bug Something isn't working label Apr 23, 2025
Copy link

@mrge-io mrge-io bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mrge found 2 issues across 1 file. View them in mrge.io

@@ -299,7 +299,7 @@ export class EventTypesAtomService {
userCredentialIds,
invalidCredentialIds,
teams,
isInstalled: !!userCredentialIds.length || !!teams.length || app.isGlobal,
isInstalled: (!!userCredentialIds.length || !!teams.length || app.isGlobal) ?? app.installed,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a logical OR (||) instead of nullish coalescing operator (??), as it better matches the intent of providing a fallback when the left side is falsy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just thinking of this

@@ -299,7 +299,7 @@
userCredentialIds,
invalidCredentialIds,
teams,
isInstalled: !!userCredentialIds.length || !!teams.length || app.isGlobal,
isInstalled: (!!userCredentialIds.length || !!teams.length || app.isGlobal) ?? app.installed,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rule violated: Enforce Singular Naming for Single-Item Functions

  Function name 'getEventTypesAppIntegration' uses plural form but returns a single item

Copy link

graphite-app bot commented Apr 23, 2025

Graphite Automations

"Add platform team as reviewer" took an action on this PR • (04/23/25)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add ready-for-e2e label" took an action on this PR • (04/24/25)

1 label was added to this PR based on Keith Williams's automation.

SomayChauhan
SomayChauhan previously approved these changes Apr 24, 2025
@ThyMinimalDev ThyMinimalDev marked this pull request as ready for review April 24, 2025 07:27
@dosubot dosubot bot added the event-types area: event types, event-types label Apr 24, 2025
Copy link

@mrge-io mrge-io bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mrge reviewed 1 file and found no issues. Review this PR in mrge.io.

@@ -299,7 +299,7 @@ export class EventTypesAtomService {
userCredentialIds,
invalidCredentialIds,
teams,
isInstalled: !!userCredentialIds.length || !!teams.length || app.isGlobal,
isInstalled: !!userCredentialIds.length || !!teams.length || app.isGlobal || app.installed,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure to understand teams.length check in this but it's not related to this pr, we might want to add comments if this keep getting more and more complicated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logic was referenced from getConnectedApps.ts which we use in the webapp, but after debugging a bit more found out the issue lies somewhere hence turned this PR into draft

Copy link
Contributor

github-actions bot commented Apr 24, 2025

E2E results are ready!

Copy link

@mrge-io mrge-io bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mrge found 3 issues across 1 file. View them in mrge.io

// We only add delegationCredentials if the request for location options is for a user because DelegationCredential Credential is applicable to Users only.
const { credentials: allCredentials } = await enrichUserWithDelegationConferencingCredentialsWithoutOrgId(

const enabledApps = await getEnabledAppsFromCredentials(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type assertion from credentials to CredentialDataWithTeamName[] is a code smell that suggests a type mismatch. Consider fixing the types properly.

@@ -226,20 +226,26 @@ export class EventTypesAtomService {
} else {
credentials = credentials.concat(teamAppCredentials);
}
} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing rationale for special treatment of Stripe integration in comments

@@ -226,20 +226,26 @@ export class EventTypesAtomService {
} else {
credentials = credentials.concat(teamAppCredentials);
}
} else {
if (slug !== "stripe") {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider inverting the condition to return early and reduce nesting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working core area: core, team members only event-types area: event types, event-types platform Anything related to our platform plan ready-for-e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants