-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
fix: Embed Plus Org - Adds missing embed route, also fixes infinite redirection for migrated user in embed #12071
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
5 Ignored Deployments
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link to collect XP and win prizes! |
apps/web/lib/withEmbedSsr.tsx
Outdated
@@ -15,10 +15,9 @@ export default function withEmbedSsr(getServerSideProps: GetServerSideProps) { | |||
const destinationUrlObj = new URL(ssrResponse.redirect.destination, "https://base"); | |||
|
|||
// Make sure that redirect happens to /embed page and pass on embed query param as is for preserving Cal JS API namespace | |||
const newDestinationUrl = `${ | |||
const newDestinationUrl = `${destinationUrlObj.origin}${ |
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.
With the recent redirect being on another domain, it is important to use the origin as well otherwise it causes infinite redirect.
Flow:
User Migrated to Org Redirect(to org domain) -> This embed redirect (Stays on org domain) -> On Org domain, no redirect needed.
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 caused an org member profile page to give 404 in embed.
a47cc7f
to
d57938e
Compare
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.
Missing routes, caused an org member profile page to give 404 in embed.
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 New Page AddedThe following page was added to the bundle from the code in this PR:
|
Current Playwright Test Results Summary✅ 208 Passing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 10/26/2023 03:55:37am UTC) Run DetailsRunning Workflow PR Update on Github Actions Commit: 96f6b88 Started: 10/26/2023 03:52:23am UTC
|
|
2 Test Cases Affected |
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Popup Tests should open embed iframe on click - Configured with light theme
Retry 1 • Initial Attempt |
0.69% (2)2 / 291 runsfailed over last 7 days |
46.39% (135)135 / 291 runsflaked over last 7 days |
Popup Tests should be able to reschedule
Retry 1 • Initial Attempt |
8.62% (25)25 / 290 runsfailed over last 7 days |
88.62% (257)257 / 290 runsflaked over last 7 days |
d57938e
to
c0d01c9
Compare
c0d01c9
to
4d3044a
Compare
return { | ||
redirect: { | ||
permanent: false, | ||
destination: eventTypeSlug ? `${redirect.toUrl}/${eventTypeSlug}` : redirect.toUrl, | ||
destination: `${newDestination}${currentQueryString ? `?${currentQueryString}` : ""}`, |
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.
Current query params aren't automatically forwarded, so this does that.
4d3044a
to
dfbf1fe
Compare
6e59fc9
to
8ba8781
Compare
8ba8781
to
60af584
Compare
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.
LGTM
…edirection for migrated user in embed (#12071)
…edirection for migrated user in embed (#12071)
…edirection for migrated user in embed (#12071)
…edirection for migrated user in embed (calcom#12071)
…edirection for migrated user in embed (calcom#12071)
What does this PR do?
Added unit tests as well for the two important functions involved here.
Type of change
How should this be tested?
Mandatory Tasks
Checklist