Introducing User Resource + removing User.find callsites #6222

albandum · 2024-07-15T13:41:48Z

Description

This PR introduces a new UserResource class to centralize user-related operations and replace direct User model interactions. It removes the renderUserType function and updates all callsites to use UserResource methods instead. The change aims to improve code organization, type safety, and consistency in user-related operations across the codebase.

This PR:

Removes renderUserType function
Introduces UserResource with the following methods:

- makeNew
- fetchAllByModelIds
- fetchAllByUsername
- fetchAllByEmail
- fetchByExternalId
- fetchByAuth0Sub
- fetchByEmail
- fetchByProvider
- getWorkspaceFirstAdmin
- update
- delete
- toJSON

Replaces all direct calls to User.findOne() with calling the resource.
There's only a few complex User.find(x) left about documenttracker. cc @fontanierh do we want to migrate that to the resource as well?

Risk

This change impacts user fetching and manipulation throughout the application. There's a risk of breaking user-related functionality if any use cases were missed during the refactoring. Thorough testing of user authentication, profile management, and any features involving user data is crucial.

Deploy Plan

Tested on edge

Registration
Inviting new user to workspace
Inviting existing dust user to workspace
Revoking user
auth0 login (google)
Switching workspaces
Removing user from poke
Conversations reactions
workspace deletion (scrub)
OKTA

To keep under surveillance

CustomerIO behavior

spolu

First batch of comments post reviewing the resource itself

front/lib/resources/user_resource.ts

front/admin/cli.ts

front/lib/resources/user_resource.ts

front/lib/iam/users.ts

front/lib/workspace.ts

front/poke/temporal/activities.ts

front/lib/resources/user_resource.ts

spolu

Left some questions to discuss with @flvndvd but this looks great!!

front/lib/iam/users.ts

front/lib/resources/user_resource.ts

front/migrations/20240423_backfill_customerio.ts

front/pages/api/login.ts

front/pages/api/w/[wId]/members/[uId]/index.ts

front/pages/no-workspace.tsx

flvndvd

🔥

flvndvd · 2024-07-16T07:14:32Z

front/lib/api/user.ts


 import { MembershipResource } from "../resources/membership_resource";

-export function renderUserType(user: User): UserType {


front/lib/api/user.ts

front/lib/iam/users.ts

front/lib/resources/user_resource.ts

front/migrations/20240423_backfill_customerio.ts

front/pages/api/create-new-workspace.ts

front/poke/temporal/activities.ts

front/pages/api/login.ts

front/pages/no-workspace.tsx

spolu

Getting closer!

Principles to ensure:

lib/api functions should not expose UserResource as return type if callers don't need to act on the resource. Keep UserType in that case
lib/api functions should not accept USerResource as arguments if they don't need to interact with the resource and instead continue rely on UserType

As a consequence all of tracking/* should not be changed and receive UserType with .toJSON calls when going into them.

front/lib/api/assistant/messages.ts

front/lib/api/workspace.ts

front/lib/iam/session.ts

front/lib/tracking/server.ts

front/lib/tracking/customerio/server.ts

front/pages/api/create-new-workspace.ts

front/pages/api/user/index.ts

front/poke/temporal/activities.ts

types/src/front/user.ts

front/lib/api/assistant/messages.ts

front/lib/api/workspace.ts

front/lib/iam/session.ts

front/lib/tracking/customerio/server.ts

albandum · 2024-07-17T08:05:09Z

front/pages/api/login.ts

@@ -400,7 +399,7 @@ async function handler(

      // Delete newly created user if SSO is mandatory.
      if (userCreated) {
-        await deleteUser(user);
+        await user.unsafeDelete();


I'm not super happy with that unsafeDelete() but I don't think I can get an Authenticator here as we don't seem to always know the workspaceId

spolu

LGTM

Given the blast radius of this PR. Let's block on a LGTM from @flvndvd as well 🙏

Can you also try to test most of the flows potentially impacted on front-edge (sign-in, membership edition, general use of the app) 🙏

front/lib/tracking/customerio/server.ts

spolu · 2024-07-17T09:45:55Z

Looks great!

front/lib/api/user.ts

flvndvd · 2024-07-17T11:48:24Z

front/lib/iam/users.ts

      }
    }

-    await user.save();
+    await user.updateInfo(


This is different from what we are currently doing, as user.save() was not doing a "saving" if the user has not been modified.

flvndvd · 2024-07-17T11:58:11Z

front/lib/resources/user_resource.ts

+    return users.map((user) => new UserResource(User, user.get()));
+  }
+
+  static async fetchNonNullableByModelId(


I'm not a big fan of this approach since it introduces more entry points and variations for the fetching method.

flvndvd

LGTM, left a few comments that can be addressed in a follow up PR as this one is already pretty big. Just fix the throw in this PR please 🙏

flvndvd · 2024-07-18T07:45:27Z

front/lib/api/user.ts

+  if (!memberships.length) {
+    const message = "Unreachable: user has no memberships.";
+    logger.error({ userId: user.id, panic: true }, message);
+    throw new Error(message);


We should not throw here, but instead return a result.

flvndvd · 2024-07-18T07:49:35Z

front/lib/iam/users.ts

-    await user.save();
+    if (Object.keys(updateArgs).length > 0) {
+      const needsUpdate = Object.entries(updateArgs).some(
+        ([key, value]) => user[key as keyof typeof user] !== value


nit not a big fan, could be removed if user.updateInfo was taking an object 😉

(can be fixed in a follow up PR)

flvndvd · 2024-07-18T07:53:14Z

front/lib/resources/user_resource.ts

+    transaction?: Transaction
+  ): Promise<Result<undefined, Error>> {
+    try {
+      await this.model.destroy({


nit at least we should ensure that the user does not have an active membership as this function should solely be used to delete users that did not pass the login.

(Can be fixed in a follow up PR).

flvndvd · 2024-07-18T07:54:18Z

front/lib/resources/user_resource.ts

+  getAttribute(attribute: string): any {
+    switch (attribute) {
+      case "fullName":
+        return [this.firstName, this.lastName].filter(Boolean).join(" ");
+      default:
+        return "";
+    }
+  }


IHMO over-engineered, a simple, would have done the job

Suggested change

getAttribute(attribute: string): any {

switch (attribute) {

case "fullName":

return [this.firstName, this.lastName].filter(Boolean).join(" ");

default:

return "";

}

}

get fullName(){

return [this.firstName, this.lastName].filter(Boolean).join(" ");

}

Hmm, you litterally told me to change this that was like that to a 'normal getter to which you pass an attribute' 🤔
Reverted

Misunderstood then 🙈

flvndvd · 2024-07-18T07:55:46Z

front/migrations/20240423_backfill_customerio.ts

-            CustomerioServerSideTracking.backfillUser({
-              user: u,
-            }).catch((err) => {
+          (async () => {


FWIW, map does not support async function. I'd simply comment out this migration as it was a one time thing.

flvndvd · 2024-07-18T08:40:41Z

front/pages/no-workspace.tsx

+    } else {
+      workspace = res.value;


You don't need the else as you throw.

* Introducing User Resource * Lint * Lint * Add transaction to fecthByProvider * Lint * Removing a lot more renderUserTypes() * Use UserResource almost everywhere * cleanup * Remove transaction from delete() * Migrate the last few things to userResource * fetchAll* -> list* * Return resource * Rename userRes -> user * userRes ToCIOpayload * Fullname getter * getFullName * fecthbyid * Remove deleteUser() * Move fetchRevokedWorkspace to lib/api * UpdateInfo and updateAuth0Sub * UpdateName * Use toJSON * Migrate to usertype * Add fetchNonNullableByModelId * Remove try catch * fectNonNullable * Introduce unsafeDelete * move getFullname() to getAttribute() * Remove nonNullable * Remove userRes * lint * Updateinfo fix * fullName() * Change throw to error * Removing else * typo

albandum changed the title ~~Introducing User Resource~~ Introducing User Resource + removing User.findOne callsites Jul 15, 2024

albandum requested a review from spolu July 15, 2024 13:44

spolu reviewed Jul 15, 2024

View reviewed changes

front/lib/resources/user_resource.ts Outdated Show resolved Hide resolved

front/lib/resources/user_resource.ts Outdated Show resolved Hide resolved

front/lib/resources/user_resource.ts Outdated Show resolved Hide resolved

front/lib/resources/user_resource.ts Outdated Show resolved Hide resolved

spolu reviewed Jul 15, 2024

View reviewed changes

albandum requested a review from flvndvd July 15, 2024 16:04

albandum changed the title ~~Introducing User Resource + removing User.findOne callsites~~ Introducing User Resource + removing User.find callsites Jul 15, 2024

spolu reviewed Jul 16, 2024

View reviewed changes

flvndvd reviewed Jul 16, 2024

View reviewed changes

albandum requested review from flvndvd and spolu July 16, 2024 09:49

albandum mentioned this pull request Jul 16, 2024

Let transcripts be processed by personal assistants #6196

Closed

spolu reviewed Jul 16, 2024

View reviewed changes

albandum commented Jul 17, 2024

View reviewed changes

albandum requested a review from spolu July 17, 2024 09:36

spolu approved these changes Jul 17, 2024

View reviewed changes

front/lib/tracking/customerio/server.ts Outdated Show resolved Hide resolved

flvndvd reviewed Jul 17, 2024

View reviewed changes

flvndvd approved these changes Jul 18, 2024

View reviewed changes

flvndvd reviewed Jul 18, 2024

View reviewed changes

albandum added 11 commits July 18, 2024 11:06

Introducing User Resource

c27cfa2

Lint

f7a4558

Lint

1446788

Add transaction to fecthByProvider

d3b587b

Lint

d1fdc14

Removing a lot more renderUserTypes()

8580a8f

Use UserResource almost everywhere

4aac6a8

cleanup

706e87e

Remove transaction from delete()

aa3d44d

Migrate the last few things to userResource

f6b5764

fetchAll* -> list*

6cf9e43

albandum added 25 commits July 18, 2024 11:06

Return resource

c23b2d4

Rename userRes -> user

32a8404

userRes ToCIOpayload

1631242

Fullname getter

bf92d6b

getFullName

bcf2f4b

fecthbyid

6f2d118

Remove deleteUser()

ba1c5f5

Move fetchRevokedWorkspace to lib/api

32afbcb

UpdateInfo and updateAuth0Sub

2b0477d

UpdateName

964f16e

Use toJSON

76726ca

Migrate to usertype

dc183e0

Add fetchNonNullableByModelId

f8c3997

Remove try catch

0991b3e

fectNonNullable

4260db4

Introduce unsafeDelete

4510ae4

move getFullname() to getAttribute()

851df18

Remove nonNullable

aa705a2

Remove userRes

25381a0

lint

25594df

Updateinfo fix

7f432c1

fullName()

4f29285

Change throw to error

dd0eb47

Removing else

faedfa1

typo

b8a2a15

albandum force-pushed the alban/user_resource branch from 9630190 to b8a2a15 Compare July 18, 2024 09:06

albandum merged commit 07072e4 into main Jul 18, 2024
9 checks passed

albandum deleted the alban/user_resource branch July 18, 2024 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing User Resource + removing User.find callsites #6222

Introducing User Resource + removing User.find callsites #6222

albandum commented Jul 15, 2024 •

edited

Loading

spolu left a comment

spolu left a comment

flvndvd left a comment

flvndvd Jul 16, 2024

spolu left a comment

albandum Jul 17, 2024

spolu left a comment

spolu commented Jul 17, 2024

flvndvd Jul 17, 2024

flvndvd Jul 17, 2024

flvndvd left a comment

flvndvd Jul 18, 2024

flvndvd Jul 18, 2024

flvndvd Jul 18, 2024

flvndvd Jul 18, 2024

albandum Jul 18, 2024 •

edited

Loading

flvndvd Jul 18, 2024

flvndvd Jul 18, 2024

flvndvd Jul 18, 2024


		import { MembershipResource } from "../resources/membership_resource";

		export function renderUserType(user: User): UserType {

Introducing User Resource + removing User.find callsites #6222

Introducing User Resource + removing User.find callsites #6222

Conversation

albandum commented Jul 15, 2024 • edited Loading

Description

Risk

Deploy Plan

spolu left a comment

Choose a reason for hiding this comment

spolu left a comment

Choose a reason for hiding this comment

flvndvd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spolu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spolu left a comment

Choose a reason for hiding this comment

spolu commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flvndvd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albandum Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albandum commented Jul 15, 2024 •

edited

Loading

albandum Jul 18, 2024 •

edited

Loading