-
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
chore: booking verification token and booking rejection logic from email #16324
Conversation
@anikdhabal is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/22/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (08/22/24)1 label was added to this PR based on Keith Williams's automation. |
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.
Thank you for your contribution @anikdhabal
Leaving some feedback to address.
I think we can simplify this even further.
I think naming the field oneTimePassword
instead of bookingSecret
is more intentional. Also booking.bookingSecret
sounds redundant. We're are already in the booking model.
Using the authenticator library is overkill IMO. The token is random and secret enough and should be used as a disposable password anyways.
Just got the idea of testing then possibility of including a text field directly in the email to provide the rejection reason. Will test locally and keep you posted.
packages/features/bookings/lib/handleNewBooking/createBooking.ts
Outdated
Show resolved
Hide resolved
Thanks @zomars for the review, yes we can simply it. Fixed it |
<Separator /> | ||
<CallToAction | ||
label={props.calEvent.organizer.language.translate("reject")} | ||
// href={`${actionHref}&action=reject`} |
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.
If omitted it will be a submit button for the form.
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.
Hey @anikdhabal made some changes. Can you verify if it's working properly?
Okay let me check |
…ing_otp/migration.sql
MailHog.and.16.more.pages.-.Personal.-.Microsoft.Edge.2024-08-23.11-19-34.mp4@zomars the token value is currently undefined, that's why not working. Also, reason is not passed to the api and zoderror occured. Isn't using a redirect for the workflow better and more understandable than a direct submit? |
E2E results are ready! |
@@ -901,6 +901,7 @@ async function handler( | |||
platformRescheduleUrl, | |||
platformCancelUrl, | |||
platformBookingUrl, | |||
oneTimePassword: isConfirmedByDefault ? null : undefined, |
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.
We need to update the one-time password of calEvent after the booking, or we will get an undefined value in the email template. Currently, this is happening
<textarea | ||
placeholder="Why are you rejecting?" | ||
style={{ |
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.
Need to set this field name to reason
.
@zomars is this ok, I think redirecting to our booking canceled page would be better, just like before. What do you think? |
@zomars take a look whenever got a chance |
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.
Blocking to put a feature flag before merging #16038
Signed-off-by: zomars <zomars@me.com> # Conflicts: # packages/emails/src/templates/OrganizerRequestEmail.tsx
Signed-off-by: zomars <zomars@me.com>
@@ -15,6 +15,7 @@ export { OrganizerCancelledEmail } from "./OrganizerCancelledEmail"; | |||
export { OrganizerLocationChangeEmail } from "./OrganizerLocationChangeEmail"; | |||
export { OrganizerPaymentRefundFailedEmail } from "./OrganizerPaymentRefundFailedEmail"; | |||
export { OrganizerRequestEmail } from "./OrganizerRequestEmail"; | |||
export { OrganizerRequestEmailV2 } from "./OrganizerRequestEmailV2"; |
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.
Moved to a new template so we can enable it on demand @anikdhabal
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.
Great workaround @zomars 🚀
Signed-off-by: zomars <zomars@me.com>
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 new tests and it's working flawlessly. Now we can also enable this for specific users and teams so we can test on multiple email clients before enabling globally. cc @emrysal @keithwillcode
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist