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

feat: Create UserCreationService and use in API V1 create user endpoint #19150

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

Conversation

joeauyeung
Copy link
Contributor

@joeauyeung joeauyeung commented Feb 7, 2025

What does this PR do?

  • Introduces a UserCreationService
  • Uses the service in API v1 create new user endpoint

Mandatory Tasks (DO NOT REMOVE)

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

How should this be tested?

  • Use v1 API to create a new user

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

Copy link

vercel bot commented Feb 7, 2025

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

Name Status Preview Comments Updated (UTC)
cal-com-ui-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 10, 2025 4:21am
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Feb 10, 2025 4:21am
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Feb 10, 2025 4:21am

identityProvider?: IdentityProvider;
}

export class UserCreationService {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is exactly what we are looking for with application services coordinating these calls. Very nice.

Comment on lines -596 to -597
const password = createHash("md5").update(`${email}${process.env.CALENDSO_ENCRYPTION_KEY}`).digest("hex");
const hashedPassword = await hashPassword(password);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving this business logic to the service

const t = await getTranslation("en", "common");
const availability = getAvailabilityFromSchedule(DEFAULT_SCHEDULE);

return await prisma.user.create({
data: {
username: slugify(username),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also moving this business logic to the service

locked: boolean;
}
) {
const organizationId = data.organizationId;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to get organizationId like this because typescript may see organizationId as a part of the ...rest below

import slugify from "../../slugify";
import { UserRepository } from "../repository/user";

interface CreateUserInput {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the service I don't want any dependence on Prisma so I'm declaring the input here. When calling the UserRepository the type is Prisma.UserCreateInput so it'll throw an type error if this input is misalligned with what Prisma expects.

static async createUser(data: CreateUserInput) {
const { email, password, username } = data;

const shouldLockByDefault = await checkIfEmailIsBlockedInWatchlistController(email);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're introducing this service, we'll check the watchlist for any blocked emails/domains whenever we create a new user.

Comment on lines +50 to +52
const { locked, ...rest } = user;

return rest;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't return the locked status back to the user

@@ -90,6 +90,7 @@ export class OrganizationRepository {
email: owner.email,
username: ownerUsernameInOrg,
organizationId: organization.id,
locked: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll check the new org owner against the watch list in this PR #19201

Copy link

graphite-app bot commented Feb 10, 2025

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (02/10/25)

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

Copy link
Contributor

E2E results are ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api area: API, enterprise API, access token, OAuth consumer core area: core, team members only ✨ feature New feature or request ready-for-e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants