feat: Autofill feature #15437
feat: Autofill feature #15437
Conversation
|
@Amit91848 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
consumer
Medium priority
Graphite Automations"Add community label" took an action on this PR • (06/14/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/14/24)1 reviewer was added to this PR based on Keith Williams's automation. |
dosubot
bot
added
bookings
| ]); | ||
|
|
||
| // When initialValues is available(after doing async schema parsing) or session is available(so that we can prefill logged-in user email and name), we need to reset the form with the initialValues | ||
| // We also need the key to change if the bookingId changes, so that the form is reset and rerendered with the new initialValues | ||
| const key = `${Object.keys(initialValues).length}_${hasSession ? 1 : 0}_${initialValues?.bookingId ?? 0}`; | ||
| const key = `${prevResponse ? JSON.stringify(initialValues).length : Object.keys(initialValues).length}_${ |
There was a problem hiding this comment.
When previous responses were fetched, key was immediately updated and useEffect was executed before the initalValues could update. Not sure if this is the best way
|
I don't think it makes sense to prefill guests, notes and other things from previous booking. Ideally, only "the name, email, phone number and maybe some other user property" remains same across bookings. |
| @@ -88,6 +97,7 @@ export const BookerWebWrapper = (props: BookerWebWrapperAtomProps) => { | |||
| hasSession, | |||
| extraOptions: routerQuery, | |||
| prefillFormParams, | |||
| prevResponse: prevResponse?.responses as Record<string, string>, | |||
There was a problem hiding this comment.
prevResponse?.responses can be undefined as well which doesn't match the assertion used.
keithwillcode
added
the
community-interns
packages/trpc/server/routers/viewer/bookings/prevResponse.handler.ts
Outdated
Show resolved
Hide resolved
| const { eventTypeId } = input; | ||
| const { user, prisma } = ctx; | ||
|
|
||
| return await prisma.booking.findFirst({ |
There was a problem hiding this comment.
We need the latest one and not the first one
There was a problem hiding this comment.
Yeah forgot to add orderBy, will fix it
There was a problem hiding this comment.
@Amit91848 Left a few comments. I think we need to discuss this feature properly.
When you say some other user property, what all should it include then? Should we include other booking questions or only stick to name, email and phone number? |
|
This PR is being marked as stale due to inactivity. |
I think name, email and phone number is what makes the most sense at the moment. We can add more if there is a good reasoning. |
| }; | ||
|
|
||
| // Should be used when we know that not all fields responses are present | ||
| // - Can happen when we are parsing the prefill query string | ||
| // - Can happen when we are parsing a booking's responses (which was created before we added a new required field) | ||
| export default function getBookingResponsesSchema({ bookingFields, view }: CommonParams) { |
There was a problem hiding this comment.
I don't think it makes sense to pass prevResponses to a function that builds the schema.
If you would see this schema when built, is passed the responses but no responses are passed to it while building the schema.
There was a problem hiding this comment.
Wes should se the values here instead https://github.com/calcom/cal.com/blob/main/packages/features/bookings/Booker/components/hooks/useInitialFormValues.ts#L69-L82
It also let's us decide among various sources of name and email where should prevResponse.email's preference lie.
e.g. If a user is loggedIn, shouldn't the loggedIn user's name and email be used instead of using the localstorage one.
| @@ -86,6 +87,8 @@ export const BookerWebWrapper = (props: BookerWebWrapperAtomProps) => { | |||
| }; | |||
| }, [searchParams, firstNameQueryParam, lastNameQueryParam]); | |||
|
|
|||
| const prevResponse = JSON.parse(localStorage.getItem(`prevEventTypeResp`) ?? "{}"); | |||
There was a problem hiding this comment.
Should we store the info per event-type as well? i.e. if a user books using different names for different events, should we honour that? I think if we do want to do it we can do it later.
Better name could be lastBookingResponse as it is the response for any of the bookings made by the user across any of the organizers.
As the customer is looking for feature parity with Calendly here, can we test out their behaviour in terms of booking different events with different emails.
There was a problem hiding this comment.
Since it would only be name and email, I think storing it per event type would be a bit too much. But as you said in case the need arises, that would be better done in a follow up PR.
I checked out how calendly does it, and it also stores a common one across all event types.
| if (values.responses) { | ||
| const prevResponse = Object.fromEntries( | ||
| Object.entries(values.responses).filter(([key]) => prevResponsesToStore.includes(key)) | ||
| ); | ||
| localStorage.setItem(`prevEventTypeResp`, JSON.stringify(prevResponse)); | ||
| } |
There was a problem hiding this comment.
It would be best to abstract getting and setting prevBookingResponse through a new module say previousBookingResponse.ts, so that it can be unit tested.
|
@hariombalhara I have made suggested changes. Currently we are only storing email and name because phone number identifier is dynamic |
|
@hariombalhara FYI that @anikdhabal is going to push this one through. |
What does this PR do?
Recording.2024-06-14.204324.mp4
Mandatory Tasks (DO NOT REMOVE)