fixup: move getProfile from protocol to common

Author: AlexTugarev

components/gitpod-protocol/src/protocol.ts

@@ -157,24 +157,6 @@ export namespace User {
         user.additionalData.ideSettings = newIDESettings;
     }
 
-    // TODO: make it more explicit that these field names are relied for our tracking purposes
-    // and decouple frontend from relying on them - instead use user.additionalData.profile object directly in FE
-    export function getProfile(user: User): Profile {
-        return {
-            name: User.getName(user!) || "",
-            email: User.getPrimaryEmail(user!) || "",
-            company: user?.additionalData?.profile?.companyName,
-            avatarURL: user?.avatarUrl,
-            jobRole: user?.additionalData?.profile?.jobRole,
-            jobRoleOther: user?.additionalData?.profile?.jobRoleOther,
-            explorationReasons: user?.additionalData?.profile?.explorationReasons,
-            signupGoals: user?.additionalData?.profile?.signupGoals,
-            signupGoalsOther: user?.additionalData?.profile?.signupGoalsOther,
-            companySize: user?.additionalData?.profile?.companySize,
-            onboardedTimestamp: user?.additionalData?.profile?.onboardedTimestamp,
-        };
-    }
-
     // TODO: refactor where this is referenced so it's more clearly tied to just analytics-tracking
     // Let other places rely on the ProfileDetails type since that's what we store
     // This is the profile data we send to our Segment analytics tracking pipeline

components/public-api/typescript-common/src/user-utils.ts

@@ -6,6 +6,7 @@
 
 import { User } from "@gitpod/public-api/lib/gitpod/v1/user_pb";
 import { User as UserProtocol } from "@gitpod/gitpod-protocol/lib/protocol";
+import { Timestamp } from "@bufbuild/protobuf";
 
 /**
  * Returns a primary email address of a user.
@@ -17,37 +18,44 @@ import { User as UserProtocol } from "@gitpod/gitpod-protocol/lib/protocol";
  * @param user
  * @returns A primaryEmail, or undefined.
  */
-export function getPrimaryEmail(user: User): string | undefined {
+export function getPrimaryEmail(user: User | UserProtocol): string | undefined {
     // If the accounts is owned by an organization, use the email of the most recently
     // used SSO identity.
     if (isOrganizationOwned(user)) {
-        const compareTime = (a?: string, b?: string) => (a || "").localeCompare(b || "");
+        const timestampToString = (a?: string | Timestamp) =>
+            a instanceof Timestamp ? a?.toDate()?.toISOString() : a || "";
+        const compareTime = (a?: string | Timestamp, b?: string | Timestamp) => {
+            return timestampToString(a).localeCompare(timestampToString(b));
+        };
         const recentlyUsedSSOIdentity = user.identities
-            .sort((a, b) =>
-                compareTime(a.lastSigninTime?.toDate()?.toISOString(), b.lastSigninTime?.toDate()?.toISOString()),
-            )
+            .sort((a, b) => compareTime(a.lastSigninTime, b.lastSigninTime))
             // optimistically pick the most recent one
             .reverse()[0];
         return recentlyUsedSSOIdentity?.primaryEmail;
     }
 
     // In case of a personal account, check for the email stored by the user.
-    if (!isOrganizationOwned(user) && user.profile?.emailAddress) {
-        return user.profile?.emailAddress;
+    if (!isOrganizationOwned(user)) {
+        const emailAddress = UserProtocol.is(user)
+            ? user.additionalData?.profile?.emailAddress
+            : user.profile?.emailAddress;
+        if (emailAddress) {
+            return emailAddress;
+        }
     }
 
     // Otherwise pick any
     // FIXME(at) this is still not correct, as it doesn't distinguish between
     // sign-in providers and additional Git hosters.
-    const identities = user.identities.filter((i) => !!i.primaryEmail);
-    if (identities.length <= 0) {
+    const primaryEmails: string[] = user.identities.map((i) => i.primaryEmail || "").filter((e) => !!e);
+    if (primaryEmails.length <= 0) {
         return undefined;
     }
 
-    return identities[0].primaryEmail || undefined;
+    return primaryEmails[0] || undefined;
 }
 
-export function getName(user: User): string | undefined {
+export function getName(user: User | UserProtocol): string | undefined {
     const name = /* user.fullName ||*/ user.name;
     if (name) {
         return name;
@@ -61,23 +69,24 @@ export function getName(user: User): string | undefined {
     return undefined;
 }
 
-export function isOrganizationOwned(user: User) {
+export function isOrganizationOwned(user: User | UserProtocol) {
     return !!user.organizationId;
 }
 
 // FIXME(at) get rid of this Nth indirection to read attributes of User entity
-export function getProfile(user: User): UserProtocol.Profile {
+export function getProfile(user: User | UserProtocol): UserProtocol.Profile {
+    const profile = UserProtocol.is(user) ? user.additionalData?.profile : user.profile;
     return {
-        name: getName(user!) || "",
-        email: getPrimaryEmail(user!) || "",
-        company: user?.profile?.companyName,
+        name: getName(user) || "",
+        email: getPrimaryEmail(user) || "",
+        company: profile?.companyName,
         avatarURL: user?.avatarUrl,
-        jobRole: user?.profile?.jobRole,
-        jobRoleOther: user?.profile?.jobRoleOther,
-        explorationReasons: user?.profile?.explorationReasons,
-        signupGoals: user?.profile?.signupGoals,
-        signupGoalsOther: user?.profile?.signupGoalsOther,
-        companySize: user?.profile?.companySize,
-        onboardedTimestamp: user?.profile?.onboardedTimestamp,
+        jobRole: profile?.jobRole,
+        jobRoleOther: profile?.jobRoleOther,
+        explorationReasons: profile?.explorationReasons,
+        signupGoals: profile?.signupGoals,
+        signupGoalsOther: profile?.signupGoalsOther,
+        companySize: profile?.companySize,
+        onboardedTimestamp: profile?.onboardedTimestamp,
     };
 }

components/server/src/user/user-service.ts

@@ -24,6 +24,7 @@ import { CreateUserParams } from "./user-authentication";
 import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics";
 import { TransactionalContext } from "@gitpod/gitpod-db/lib/typeorm/transactional-db-impl";
 import { RelationshipUpdater } from "../authorization/relationship-updater";
+import { getProfile } from "@gitpod/public-api-common/src/user-utils";
 
 @injectable()
 export class UserService {
@@ -105,7 +106,7 @@ export class UserService {
         await this.authorizer.checkPermissionOnUser(userId, "write_info", user.id);
 
         //hang on to user profile before it's overwritten for analytics below
-        const oldProfile = User.getProfile(user);
+        const oldProfile = getProfile(user);
 
         const allowedFields: (keyof User)[] = ["fullName", "additionalData"];
         for (const p of allowedFields) {
@@ -117,8 +118,8 @@ export class UserService {
         await this.userDb.updateUserPartial(user);
 
         //track event and user profile if profile of partialUser changed
-        const newProfile = User.getProfile(user);
-        if (User.Profile.hasChanges(oldProfile, newProfile)) {
+        const newProfile = getProfile(user);
+        if (this.hasChanges(oldProfile, newProfile)) {
             this.analytics.track({
                 userId: user.id,
                 event: "profile_changed",
@@ -132,6 +133,21 @@ export class UserService {
         return user;
     }
 
+    private hasChanges(before: User.Profile, after: User.Profile) {
+        return (
+            before.name !== after.name ||
+            before.email !== after.email ||
+            before.company !== after.company ||
+            before.avatarURL !== after.avatarURL ||
+            before.jobRole !== after.jobRole ||
+            before.jobRoleOther !== after.jobRoleOther ||
+            // not checking explorationReasons or signupGoals atm as it's an array - need to check deep equality
+            before.signupGoalsOther !== after.signupGoalsOther ||
+            before.onboardedTimestamp !== after.onboardedTimestamp ||
+            before.companySize !== after.companySize
+        );
+    }
+
     async updateWorkspaceTimeoutSetting(
         userId: string,
         targetUserId: string,