Skip to content

Refactor/email confirm links#6151

Merged
zomars merged 9 commits intomainfrom
refactor/email-confirm-links
Dec 22, 2022
Merged

Refactor/email confirm links#6151
zomars merged 9 commits intomainfrom
refactor/email-confirm-links

Conversation

@zomars
Copy link
Copy Markdown
Contributor

@zomars zomars commented Dec 21, 2022

What does this PR do?

  • Reverts some changes made in Direct links to accept/reject a booking #5939
  • Moves confirmation logic to /api/link
  • Reuses Booking UI to display status
  • Drops adding rejecting reason support (for now)
  • Reuses tRPC logic
  • Uses encryption/decryption in a single token instead of hashing

To do (maybe)

  • Add backwards compatibility with old token (IDK if worth it)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

  • Book a event with "requires confirmation"
  • Test Accept/Reject buttons

@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 21, 2022

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

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Dec 22, 2022 at 0:17AM (UTC)

@zomars zomars requested review from a team, emrysal and leog and removed request for leog December 21, 2022 21:18
Copy link
Copy Markdown
Contributor Author

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Ready for review

Comment thread apps/web/next.config.js
Comment on lines +230 to +234
{
source: "/booking/direct/:action/:email/:bookingUid/:oldToken",
destination: "/api/link?action=:action&email=:email&bookingUid=:bookingUid&oldToken=:oldToken",
permanent: true,
},
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So old links gets redirected

async function sessionGetter() {
return {
user: {
id: userId,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This comes from the encrypted token

attendee.timeZone = dynamic;
attendee.language = dynamic;
}
body.payload.organizer.id = dynamic;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We now need this organizer id to generate the token

Copy link
Copy Markdown
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

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

It looks good to me, nice cleanup, have a look at my comments and see if there's any merrit to my thought - will turn off automerge so you can eval.

Comment thread apps/web/pages/api/link.ts
Comment thread packages/lib/constants.ts Outdated
Copy link
Copy Markdown
Contributor

@alannnc alannnc left a comment

Choose a reason for hiding this comment

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

It works and couldn't find any security leak.

@github-actions github-actions Bot removed ♻️ autoupdate tells kodiak to keep this branch up-to-date automerge labels Dec 22, 2022
Copy link
Copy Markdown
Contributor

@leog leog left a comment

Choose a reason for hiding this comment

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

LGTM. Found some NITs comparing to old approach:

  1. When an error happens (invalid token, booking already confirmed or rejected), we should redirect to page 500 with an error query param with the message
  2. I think we should remove the following section of the booking page for this feature
    image

@zomars zomars merged commit 23450b6 into main Dec 22, 2022
@zomars zomars deleted the refactor/email-confirm-links branch December 22, 2022 00:15
@zomars
Copy link
Copy Markdown
Contributor Author

zomars commented Dec 22, 2022

LGTM. Found some NITs comparing to old approach:

  1. When an error happens (invalid token, booking already confirmed or rejected), we should redirect to page 500 with an error query param with the message

Agreed, we can follow up tho. Not critical IMO.

  1. I think we should remove the following section of the booking page for this feature
    image

We can discuss this. I don't think is that much of an issue.

@leog
Copy link
Copy Markdown
Contributor

leog commented Dec 22, 2022

Agreed, we can follow up tho. Not critical IMO.

Agreed, which is why I approved it anyway. NITs shouldn't stop a PR.

We can discuss this. I don't think is that much of an issue.

Not an issue, it just looks weird, an oversight. The person accepting/rejecting already has an account, it just looks sloppy IMO.

haffla pushed a commit to tourlane/cal.com that referenced this pull request Jan 25, 2023
* Booking confirmation link improvements

* Cleanup

* Update webhooks payload

* Removing unneeded dependency

* Feedback

* Matches enum style to rest of codebase

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Co-authored-by: Leo Giovanetti <hello@leog.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants