fix: Make sure if header is set already, it isn't set again#12194
Merged
keithwillcode merged 2 commits intomainfrom Nov 2, 2023
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
5 Ignored Deployments
|
Member
Author
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
Contributor
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link to collect XP and win prizes! |
hariombalhara
changed the title
hariombalhara
force-pushed
the
11-02-Make_sure_if_header_is_set_already_it_isnt_set_again
branch
from
f64f6c8 to
0091ecf
Compare
hariombalhara
commented
Nov 2, 2023
| const result = await f(req, res); | ||
| ok = true; | ||
| if (result) res.json(result); | ||
| if (result && !res.writableEnded) { |
Member
Author
There was a problem hiding this comment.
There could be cases where header is already set and result is also set. e.g. res.redirect unlike res.json returns a non nullish value and thus we would endup setting json in that case as well which would cause 'header already set error'
So, we should set this json if something isn't already written to the stream.
Contributor
There was a problem hiding this comment.
Nice find. Unit testable (could be follow up)?
Contributor
📦 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! 🙌 |
Current Playwright Test Results Summary✅ 251 Passing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 11/02/2023 05:57:01am UTC) Run DetailsRunning Workflow PR Update on Github Actions Commit: ff4b6d3 Started: 11/02/2023 05:53:10am UTC
|
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
2FA Tests should allow a user to enable 2FA and login using 2FA
Retry 1 • Initial Attempt |
0.39% (1)1 / 255 runfailed over last 7 days |
33.73% (86)86 / 255 runsflaked over last 7 days |
📄 apps/web/playwright/booking-pages.e2e.ts • 1 Flake
Test Case Results
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
pro user Time slots are not reserved when going back via Cancel button on Event Form
Retry 1 • Initial Attempt |
0.46% (1)1 / 216 runfailed over last 7 days |
0.46% (1)1 / 216 runflaked over last 7 days |
📄 apps/web/playwright/integrations-stripe.e2e.ts • 1 Flake
Test Case Results
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
Stripe integration Can book a paid booking
Retry 1 • Initial Attempt |
2.64% (7)7 / 265 runsfailed over last 7 days |
3.40% (9)9 / 265 runsflaked over last 7 days |
📄 apps/web/playwright/booking/addressQuestione2e/addressQuestion.e2e.ts • 1 Flake
Test Case Results
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
Booking With Address Question and Each Other Question Booking With Address Question and Short text question Address and Short text not required
Retry 1 • Initial Attempt |
0% (0)0 / 186 runsfailed over last 7 days |
2.15% (4)4 / 186 runsflaked over last 7 days |
📄 apps/web/playwright/event-types.e2e.ts • 1 Flake
Test Case Results
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
Event Types tests user Different Locations Tests Can remove location from multiple locations that are saved
Retry 1 • Initial Attempt |
0% (0)0 / 253 runsfailed over last 7 days |
7.11% (18)18 / 253 runsflaked over last 7 days |
📄 packages/embeds/embed-core/playwright/tests/action-based.e2e.ts • 2 Flakes
Top 1 Common Error Messages
|
|
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 |
2.92% (8)8 / 274 runsfailed over last 7 days |
51.82% (142)142 / 274 runsflaked over last 7 days |
|
Popup Tests should be able to reschedule
Retry 2 • Retry 1 • Initial Attempt |
20.07% (55)55 / 274 runsfailed over last 7 days |
77.01% (211)211 / 274 runsflaked over last 7 days |
📄 apps/web/playwright/teams.e2e.ts • 1 Flake
Test Case Results
| Test Case | Last 7 days Failures | Last 7 days Flakes |
|---|---|---|
|
Teams - NonOrg Can create a private team
Retry 1 • Initial Attempt |
0% (0)0 / 178 runsfailed over last 7 days |
14.61% (26)26 / 178 runsflaked over last 7 days |
Udit-takkar
reviewed
Nov 2, 2023
Contributor
Udit-takkar
left a comment
Udit-takkar
left a comment
There was a problem hiding this comment.
Wait is the endpoint api/integrations/zoho_calendar or api/integrations/zohocalendar ?
I see api/integrations/zoho_calendar when installing app
keithwillcode
approved these changes
Nov 2, 2023
| const result = await f(req, res); | ||
| ok = true; | ||
| if (result) res.json(result); | ||
| if (result && !res.writableEnded) { |
Contributor
There was a problem hiding this comment.
Nice find. Unit testable (could be follow up)?
keithwillcode
deleted the
11-02-Make_sure_if_header_is_set_already_it_isnt_set_again
branch
zomars
pushed a commit
that referenced
this pull request
Jan 29, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes the following error. Though as far as I tested, the error arises only when there was already an error in the callback endpoint
In case of zohocalendar, the error was coming when this line was executed https://github.com/calcom/cal.com/blob/0091ecf572c063e899ec664e2e3f6aa6e1c6499c/packages/app-store/zohocalendar/api/callback.ts#L58. It returned a non nullish value causing the bug in
defaultResponder.Type of change
How should this be tested?
Mandatory Tasks
Checklist