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

Fixed #1015 - Teams user registration is broken #1090

Merged
merged 7 commits into from Nov 11, 2021

Conversation

emrysal
Copy link
Contributor

@emrysal emrysal commented Oct 31, 2021

The necessary check got removed in https://github.com/calendso/calendso/commit/13486d9988a11a93eeee85b0396a57c0dbad43d4#diff-bc80461208be7df84b92ad01439aea7d4dcee60b443e1ba998a77cebed65c1e2

Continued expanding the fields.tsx file with additional features; deprecating UsernameInput in the process. I tried to keep the implementation side as lean as possible.

As soon as /availability is merged we can replace <form> with <Form> and we'll no longer need to manually import FormProvider, neatening it up further.

Note: This refactor focusses on react-hook-forms, it does not remove the fetch (one step at a time..)

@emrysal emrysal requested a review from KATT October 31, 2021 22:03
@vercel
Copy link

vercel bot commented Oct 31, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/cal/calendso/3cqx2x1H6pFujyXWjarjTZMCEAPA
✅ Preview: https://calendso-git-bugfix-signup-page-email-already-registered-cal.vercel.app

@vercel vercel bot temporarily deployed to Preview October 31, 2021 22:05 Inactive
@emrysal emrysal linked an issue Oct 31, 2021 that may be closed by this pull request
Copy link
Contributor

@baileypumfleet baileypumfleet left a comment

Choose a reason for hiding this comment

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

Looks good to me. Hopefully we can get the /availability revamp merged today, so you can update this PR to use

and then merge

@baileypumfleet
Copy link
Contributor

Now that the /availability PR is merged, the modification for this PR can be made

…dy-registered

# Conflicts:
#	components/form/fields.tsx
#	pages/auth/signup.tsx
#	public/static/locales/en/common.json
@vercel vercel bot temporarily deployed to Preview November 11, 2021 04:40 Inactive
@vercel vercel bot temporarily deployed to Preview November 11, 2021 04:46 Inactive
Copy link
Member

@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.

Made some fixes to the onboarding. I think this is ready to merge.

Comment on lines +96 to +124
type FormProps<T> = { form: UseFormReturn<T>; handleSubmit: SubmitHandler<T> } & Omit<
JSX.IntrinsicElements["form"],
"onSubmit"
>;

const PlainForm = <T extends FieldValues>(props: FormProps<T>, ref: Ref<HTMLFormElement>) => {
const { form, handleSubmit, ...passThrough } = props;

return (
<FormProvider {...form}>
<form
onSubmit={
handleSubmit
? form.handleSubmit(async (...args) => {
try {
await handleSubmit(...args);
} catch (_err) {
const err = getErrorFromUnknown(_err);
handleError(err);
}
})
: undefined
}
ref={ref}
onSubmit={(event) => {
form
.handleSubmit(handleSubmit)(event)
.catch((err) => {
showToast(`${getErrorFromUnknown(err).message}`, "error");
});
}}
{...passThrough}>
{props.children}
</form>
</FormProvider>
);
}
};

export const Form = forwardRef(PlainForm) as <T extends FieldValues>(
p: FormProps<T> & { ref?: Ref<HTMLFormElement> }
) => ReactElement;
Copy link
Member

Choose a reason for hiding this comment

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

Preserve form value types from generics

@@ -313,7 +313,7 @@ export default function Onboarding(props: inferSSRProps<typeof getServerSideProp
title: t("set_availability"),
description: t("set_availability_instructions"),
Component: (
<Form
<Form<ScheduleFormValues>
Copy link
Member

Choose a reason for hiding this comment

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

So our values in handleSubmit are typed.

Tested and working correctly
@vercel vercel bot temporarily deployed to Preview November 11, 2021 05:07 Inactive
Comment on lines +51 to +73
await prisma.user.update({
where: {
userId,
id: userId,
},
});
await Promise.all(
availability.map((schedule: Availability) =>
prisma.availability.create({
data: {
days: schedule.days,
startTime: schedule.startTime,
endTime: schedule.endTime,
user: {
connect: {
id: userId,
},
data: {
availability: {
/* We delete user availabilty */
deleteMany: {
userId: {
equals: userId,
},
},
})
)
);
/* So we can replace it */
createMany: {
data: availability.map((schedule) => ({
days: schedule.days,
startTime: schedule.startTime,
endTime: schedule.endTime,
})),
},
},
},
});
Copy link
Member

@zomars zomars Nov 11, 2021

Choose a reason for hiding this comment

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

Made this query safer and more legible (IMO) Tested and working 👍🏽

@emrysal
Copy link
Contributor Author

emrysal commented Nov 11, 2021

Feel free to merge when ready @zomars - most of the content in this PR has been discussed at length and I'll tweak using the fields.tsx modifications later.

@vercel vercel bot temporarily deployed to Preview November 11, 2021 05:40 Inactive
@zomars zomars added ♻️ autoupdate tells kodiak to keep this branch up-to-date 👀 ready for review labels Nov 11, 2021
@zomars zomars enabled auto-merge (squash) November 11, 2021 05:41
Comment on lines -33 to -48
// sets the desired time in current date, needs to be current date for proper DST translation
const defaultDayRange: TimeRange = {
start: new Date(new Date().setHours(9, 0, 0, 0)),
end: new Date(new Date().setHours(17, 0, 0, 0)),
};

export const DEFAULT_SCHEDULE: ScheduleType = [
[],
[defaultDayRange],
[defaultDayRange],
[defaultDayRange],
[defaultDayRange],
[defaultDayRange],
[],
];

Copy link
Member

Choose a reason for hiding this comment

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

Moved so we can reuse in the seeder

Comment on lines -20 to -47
const availability = req.body.schedule.reduce(
(availability: Availability[], times: TimeRange[], day: number) => {
const addNewTime = (time: TimeRange) =>
({
days: [day],
startTime: time.start,
endTime: time.end,
} as Availability);

const filteredTimes = times.filter((time) => {
let idx;
if (
(idx = availability.findIndex(
(schedule) => schedule.startTime === time.start && schedule.endTime === time.end
)) !== -1
) {
availability[idx].days.push(day);
return false;
}
return true;
});
filteredTimes.forEach((time) => {
availability.push(addNewTime(time));
});
return availability;
},
[] as Availability[]
);
Copy link
Member

Choose a reason for hiding this comment

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

Moved so we can reuse the logic in the seeder

Comment on lines +30 to +34
availability: {
createMany: {
data: getAvailabilityFromSchedule(DEFAULT_SCHEDULE),
},
},
Copy link
Member

Choose a reason for hiding this comment

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

Tests were failing because there were no availability in a fresh DB

@zomars zomars merged commit bf659c0 into main Nov 11, 2021
@zomars zomars deleted the bugfix/signup-page-email-already-registered branch November 11, 2021 05:44
@zomars
Copy link
Member

zomars commented Nov 11, 2021

Merged and going to sleep 👍🏽

KATT pushed a commit that referenced this pull request Nov 15, 2021
* Fixed #1015 - Teams user registration is broken

* Type fixes for avilability form in onboarding

* Re adds missing strings

* Updates user availability in one query

Tested and working correctly

* Fixes seeder and tests

Co-authored-by: Omar López <zomars@me.com>
@PeerRich PeerRich added the core area: core, team members only label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♻️ autoupdate tells kodiak to keep this branch up-to-date core area: core, team members only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Registration upon user invite through Teams is broken
4 participants