-
Notifications
You must be signed in to change notification settings - Fork 7.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: location change during rescheduling #15810
Conversation
@dilpreetsio is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details:
|
Graphite Automations"Add community label" took an action on this PR • (07/17/24)1 label was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (07/17/24)1 reviewer was added to this PR based on Keith Williams's automation. |
f7744f2
to
1516ca5
Compare
Type checks are failing @dilpreetsio , please fix those 🙌 |
1516ca5
to
9bd23d4
Compare
@@ -227,6 +227,7 @@ const EventTypePage = (props: EventTypeSetupProps & { allActiveWorkflows?: Workf | |||
const [pendingRoute, setPendingRoute] = useState(""); | |||
const leaveWithoutAssigningHosts = useRef(false); | |||
const [animationParentRef] = useAutoAnimate<HTMLDivElement>(); | |||
|
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.
Let's remove this whitespace change.
readOnly = false; | ||
} | ||
// TODO: We need to ensure that location field is properly updated in Booking Success Page, email, calendar, webhook after reschedule | ||
readOnly = false; |
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.
Made this change because if (locationResponse?.value === "attendeeInPerson" || "phone") {
condition is faulty.
It is always truthy. So, it seems like location change is enabled for attendeeInPerson and phone but in fact it is enabled in all cases.
Introduced in #11651
I think with this PR we should intend to be able to make any change to location field and test it accordingly.
"system", // Deletable-No, CanChangeIdentifier-No, Toggleable-No, CanMarkOptional-No. | ||
"system-but-optional", // Deletable-No, CanChangeIdentifier-No, Toggleable-Yes, CanMarkOptional-Yes | ||
// TODO: Seems to be unused and could probably be deleted | ||
"system-but-hidden", // Deletable-No, CanChangeIdentifier-No, Always Hidden and thus Optional | ||
"user", // Fully editable. Deletable-Yes, CanChangeIdentifier-Yes, Toggleable-Yes, CanMarkOptional-Yes | ||
// TODO: Seems to be unused and could probably be deleted |
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.
Updated the comments to make it more readable.
@@ -391,7 +391,7 @@ export const Components: Record<FieldType, Component> = { | |||
if (!value) { | |||
setValue({ | |||
value: options[0]?.value, | |||
optionValue: "", | |||
optionValue: options[0]?.optionValue || "", |
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.
I don't understand this and other change in this component.
optionValue
can only be updated through ComponentForField
at line 469.
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.
The booking view expects the optionValue
to have the a valid value.
"optionValue" in bookingInfo.responses.location |
Currently the optionValue
is always empty regardless of the option selected.
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.
Thanks @dilpreetsio for the PR.
I don't understand right now how the changes are fixing the bug. While testing I could see the same behaviour even without the changes in components.tsx file.
Could you confirm that the behaviour(as per the recording your shared in the PR description) didn't exist in main?
@hariombalhara Yes the behaviour on main is not as expected. Screen.Recording.2024-07-18.at.3.12.10.PM.mov |
Same issue was flagged earlier #15108 (comment). Also add tests to ensure this doesn't break in the future. |
This PR is being marked as stale due to inactivity. |
Thanks for your contribution but closing in favour of #16162 |
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Inperson-location.mov
Meet.links.mov