Skip to content
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: multiple getSchedule requests on email change in booker #15616

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dilpreetsio
Copy link
Contributor

@dilpreetsio dilpreetsio commented Jun 28, 2024

What does this PR do?

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A-I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • N/A-I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Create an event-type
  • Create a booking
  • Open network tab in dev tools & update the email field.

There are no more requests for slots when the email is changed.

Screen.Recording.2024-06-28.at.7.52.55.PM.mov

Copy link

vercel bot commented Jun 28, 2024

@dilpreetsio is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Jun 28, 2024
@graphite-app graphite-app bot requested a review from a team June 28, 2024 14:24
@github-actions github-actions bot added booking-page area: booking page, public booking page, booker Medium priority Created by Linear-GitHub Sync performance area: performance, page load, slow, slow endpoints, loading screen, unresponsive 🐛 bug Something isn't working labels Jun 28, 2024
@dosubot dosubot bot added the platform Anything related to our platform plan label Jun 28, 2024
@dosubot dosubot bot added this to the v4.4 milestone Jun 28, 2024
@graphite-app graphite-app bot requested a review from a team June 28, 2024 14:25
Copy link

graphite-app bot commented Jun 28, 2024

Graphite Automations

"Add community label" took an action on this PR • (06/28/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 • (06/28/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add platform team as reviewer" took an action on this PR • (06/28/24)

1 reviewer was added to this PR based on Keith Williams's automation.

Copy link
Contributor

@anikdhabal anikdhabal left a comment

Choose a reason for hiding this comment

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

Hey, one more request is being sent - checkIfUserEmailVerificationRequired. This is from the useVerifyEmail hook. Do you think it would be optimal to not pass bookerEmail? This bookerEmail retrieves the email address of the owner of a contact in the CRM system.

@dilpreetsio
Copy link
Contributor Author

@anikdhabal The checkIfUserEmailVerificationRequired is basically checking if the email is blacklisted or not, so we need to keep it.

@nishchal27
Copy link

making a server request on every keystroke can be costly

@anikdhabal anikdhabal removed the platform Anything related to our platform plan label Jun 30, 2024
@dilpreetsio
Copy link
Contributor Author

The input is debounced to reduce the number of request. If we want to further reduce the number of requests one way to go ahead would be check if the email is valid before sending a request.

@dilpreetsio
Copy link
Contributor Author

Do you think it would be optimal to not pass bookerEmail? This bookerEmail retrieves the email address of the owner of a contact in the CRM system.

Do you mean not passing the bookerEmail to the useScheduleForEvent hook ?

Copy link
Contributor

@Udit-takkar Udit-takkar left a comment

Choose a reason for hiding this comment

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

@dilpreetsio how does this fix the issue?
bookerEmail was required in packages/trpc/server/routers/viewer/slots/util.ts

  const crmContactOwner = await getCRMContactOwnerForRRLeadSkip(
      input.bookerEmail,
      eventType?.metadata?.apps
    );
    

@anikdhabal
Copy link
Contributor

Do you mean not passing the bookerEmail to the useScheduleForEvent hook ?

Yes, @Udit-takkar shared the code above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
booking-page area: booking page, public booking page, booker 🐛 bug Something isn't working community Created by Linear-GitHub Sync Medium priority Created by Linear-GitHub Sync performance area: performance, page load, slow, slow endpoints, loading screen, unresponsive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-3974] Multiple Slots.getSchedule requests being sent when typing email on booker page
4 participants