Convert isViewer and isAdmin to getters (#6724)

app/models/User.ts

@@ -52,10 +52,7 @@ class User extends ParanoidModel {
   email: string;
 
   @observable
-  isAdmin: boolean;
-
-  @observable
-  isViewer: boolean;
+  role: UserRole;
 
   @observable
   lastActiveAt: string;
@@ -68,11 +65,27 @@ class User extends ParanoidModel {
     return (this.name ? this.name[0] : "?").toUpperCase();
   }
 
-  @computed
+  /**
+   * Whether the user has been invited but not yet signed in.
+   */
   get isInvited(): boolean {
     return !this.lastActiveAt;
   }
 
+  /**
+   * Whether the user is an admin.
+   */
+  get isAdmin(): boolean {
+    return this.role === UserRole.Admin;
+  }
+
+  /**
+   * Whether the user is a viewer.
+   */
+  get isViewer(): boolean {
+    return this.role === UserRole.Viewer;
+  }
+
   /**
    * Whether the user has been recently active. Recently is currently defined
    * as within the last 5 minutes.
@@ -84,17 +97,6 @@ class User extends ParanoidModel {
     return new Date(this.lastActiveAt) > subMinutes(now(10000), 5);
   }
 
-  @computed
-  get role(): UserRole {
-    if (this.isAdmin) {
-      return UserRole.Admin;
-    } else if (this.isViewer) {
-      return UserRole.Viewer;
-    } else {
-      return UserRole.Member;
-    }
-  }
-
   /**
    * Returns whether this user is using a separate editing mode behind an "Edit"
    * button rather than seamless always-editing.

app/models/base/Model.ts

@@ -116,7 +116,12 @@ export default abstract class Model {
 
   updateData = action((data: Partial<Model>) => {
     for (const key in data) {
-      this[key] = data[key];
+      try {
+        this[key] = data[key];
+      } catch (error) {
+        // Temporary as isViewer and isAdmin properties changed to getters
+        Logger.warn(`Error setting ${key} on model`, error);
+      }
     }
 
     this.isNew = false;

server/commands/accountProvisioner.ts

@@ -1,4 +1,5 @@
 import invariant from "invariant";
+import { UserRole } from "@shared/types";
 import WelcomeEmail from "@server/emails/templates/WelcomeEmail";
 import {
   InvalidAuthenticationError,
@@ -132,7 +133,7 @@ async function accountProvisioner({
     name: userParams.name,
     email: userParams.email,
     language: userParams.language,
-    isAdmin: isNewTeam || undefined,
+    role: isNewTeam ? UserRole.Admin : undefined,
     avatarUrl: userParams.avatarUrl,
     teamId: team.id,
     ip,

server/commands/userDestroyer.ts

@@ -1,4 +1,5 @@
 import { Op, Transaction } from "sequelize";
+import { UserRole } from "@shared/types";
 import { Event, User } from "@server/models";
 import { ValidationError } from "../errors";
 
@@ -29,7 +30,7 @@ export default async function userDestroyer({
   if (user.isAdmin) {
     const otherAdminsCount = await User.count({
       where: {
-        isAdmin: true,
+        role: UserRole.Admin,
         teamId,
         id: {
           [Op.ne]: user.id,

server/commands/userInviter.ts

@@ -58,8 +58,12 @@ export default async function userInviter({
       teamId: user.teamId,
       name: invite.name,
       email: invite.email,
-      isAdmin: user.isAdmin && invite.role === UserRole.Admin,
-      isViewer: user.isViewer || invite.role === UserRole.Viewer,
+      role:
+        user.isAdmin && invite.role === UserRole.Admin
+          ? UserRole.Admin
+          : user.isViewer || invite.role === UserRole.Viewer
+          ? UserRole.Viewer
+          : UserRole.Member,
       invitedById: user.id,
       flags: {
         [UserFlag.InviteSent]: 1,

server/commands/userProvisioner.test.ts

@@ -1,5 +1,6 @@
 import { faker } from "@faker-js/faker";
 import { v4 as uuidv4 } from "uuid";
+import { UserRole } from "@shared/types";
 import { TeamDomain } from "@server/models";
 import {
   buildUser,
@@ -185,8 +186,7 @@ describe("userProvisioner", () => {
     expect(authentication?.scopes.length).toEqual(1);
     expect(authentication?.scopes[0]).toEqual("read");
     expect(user.email).toEqual("test@example.com");
-    expect(user.isAdmin).toEqual(false);
-    expect(user.isViewer).toEqual(false);
+    expect(user.role).toEqual(UserRole.Member);
     expect(isNewUser).toEqual(true);
   });
 
@@ -200,7 +200,7 @@ describe("userProvisioner", () => {
       name: "Test Name",
       email: "test@example.com",
       teamId: team.id,
-      isAdmin: true,
+      role: UserRole.Admin,
       ip,
       authentication: {
         authenticationProviderId: authenticationProvider.id,
@@ -210,12 +210,12 @@ describe("userProvisioner", () => {
       },
     });
     const { user } = result;
-    expect(user.isAdmin).toEqual(true);
+    expect(user.role).toEqual(UserRole.Admin);
   });
 
   it("should prefer defaultUserRole when isAdmin is undefined or false", async () => {
     const team = await buildTeam({
-      defaultUserRole: "viewer",
+      defaultUserRole: UserRole.Viewer,
     });
     const authenticationProviders = await team.$get("authenticationProviders");
     const authenticationProvider = authenticationProviders[0];
@@ -232,13 +232,11 @@ describe("userProvisioner", () => {
       },
     });
     const { user: tname } = result;
-    expect(tname.isAdmin).toEqual(false);
-    expect(tname.isViewer).toEqual(true);
+    expect(tname.role).toEqual(UserRole.Viewer);
     const tname2Result = await userProvisioner({
       name: "Test2 Name",
       email: "tes2@example.com",
       teamId: team.id,
-      isAdmin: false,
       ip,
       authentication: {
         authenticationProviderId: authenticationProvider.id,
@@ -248,8 +246,7 @@ describe("userProvisioner", () => {
       },
     });
     const { user: tname2 } = tname2Result;
-    expect(tname2.isAdmin).toEqual(false);
-    expect(tname2.isViewer).toEqual(true);
+    expect(tname2.role).toEqual(UserRole.Viewer);
   });
 
   it("should create a user from an invited user", async () => {

server/commands/userProvisioner.ts

@@ -1,4 +1,5 @@
 import { InferCreationAttributes } from "sequelize";
+import { UserRole } from "@shared/types";
 import InviteAcceptedEmail from "@server/emails/templates/InviteAcceptedEmail";
 import {
   DomainNotAllowedError,
@@ -22,8 +23,8 @@ type Props = {
   email: string;
   /** The language of the user, if known */
   language?: string;
-  /** Provision the new user as an administrator */
-  isAdmin?: boolean;
+  /** The role for new user, Member if none is provided */
+  role?: UserRole;
   /** The public url of an image representing the user */
   avatarUrl?: string | null;
   /**
@@ -52,7 +53,7 @@ type Props = {
 export default async function userProvisioner({
   name,
   email,
-  isAdmin,
+  role,
   language,
   avatarUrl,
   teamId,
@@ -230,15 +231,12 @@ export default async function userProvisioner({
       throw DomainNotAllowedError();
     }
 
-    const defaultUserRole = team?.defaultUserRole;
-
     const user = await User.create(
       {
         name,
         email,
         language,
-        isAdmin: typeof isAdmin === "boolean" && isAdmin,
-        isViewer: isAdmin === true ? false : defaultUserRole === "viewer",
+        role: role ?? team?.defaultUserRole,
         teamId,
         avatarUrl,
         authentications: authentication ? [authentication] : [],

server/migrations/20240327235446-role-non-nullable.js

@@ -0,0 +1,15 @@
+'use strict';
+
+module.exports = {
+  async up (queryInterface, Sequelize) {
+    await queryInterface.sequelize.query(
+      'ALTER TABLE users ALTER COLUMN role SET NOT NULL;'
+    );
+  },
+
+  async down (queryInterface) {
+    await queryInterface.sequelize.query(
+      'ALTER TABLE users ALTER COLUMN role DROP NOT NULL;'
+    );
+  }
+};

server/models/Team.ts

@@ -34,6 +34,7 @@ import {
   CollectionPermission,
   TeamPreference,
   TeamPreferences,
+  UserRole,
 } from "@shared/types";
 import { getBaseDomain, RESERVED_SUBDOMAINS } from "@shared/utils/domains";
 import env from "@server/env";
@@ -151,10 +152,10 @@ class Team extends ParanoidModel<
   @Column
   memberCollectionCreate: boolean;
 
-  @Default("member")
-  @IsIn([["viewer", "member"]])
-  @Column
-  defaultUserRole: string;
+  @Default(UserRole.Member)
+  @IsIn([[UserRole.Viewer, UserRole.Member]])
+  @Column(DataType.STRING)
+  defaultUserRole: UserRole;
 
   @AllowNull
   @Column(DataType.JSONB)

server/models/User.ts

@@ -29,7 +29,6 @@ import {
   IsDate,
   AllowNull,
   AfterUpdate,
-  BeforeSave,
 } from "sequelize-typescript";
 import { UserPreferenceDefaults } from "@shared/constants";
 import { languages } from "@shared/i18n";
@@ -137,14 +136,6 @@ class User extends ParanoidModel<
   @Column
   name: string;
 
-  @Default(false)
-  @Column
-  isAdmin: boolean;
-
-  @Default(false)
-  @Column
-  isViewer: boolean;
-
   @Default(UserRole.Member)
   @Column(DataType.ENUM(...Object.values(UserRole)))
   role: UserRole;
@@ -251,6 +242,14 @@ class User extends ParanoidModel<
     return !this.lastActiveAt;
   }
 
+  get isAdmin() {
+    return this.role === UserRole.Admin;
+  }
+
+  get isViewer() {
+    return this.role === UserRole.Viewer;
+  }
+
   get color() {
     return stringToColor(this.id);
   }
@@ -551,7 +550,7 @@ class User extends ParanoidModel<
     const res = await (this.constructor as typeof User).findAndCountAll({
       where: {
         teamId: this.teamId,
-        isAdmin: true,
+        role: UserRole.Admin,
         id: {
           [Op.ne]: this.id,
         },
@@ -562,21 +561,9 @@ class User extends ParanoidModel<
 
     if (res.count >= 1) {
       if (to === UserRole.Member) {
-        await this.update(
-          {
-            isAdmin: false,
-            isViewer: false,
-          },
-          options
-        );
+        await this.update({ role: to }, options);
       } else if (to === UserRole.Viewer) {
-        await this.update(
-          {
-            isAdmin: false,
-            isViewer: true,
-          },
-          options
-        );
+        await this.update({ role: to }, options);
         await UserMembership.update(
           {
             permission: CollectionPermission.Read,
@@ -599,13 +586,7 @@ class User extends ParanoidModel<
   promote: (
     options?: InstanceUpdateOptions<InferAttributes<User>>
   ) => Promise<User> = (options) =>
-    this.update(
-      {
-        isAdmin: true,
-        isViewer: false,
-      },
-      options
-    );
+    this.update({ role: UserRole.Admin }, options);
 
   // hooks
 
@@ -628,20 +609,6 @@ class User extends ParanoidModel<
     });
   };
 
-  /**
-   * Temporary hook to double write role while we transition to the new field.
-   */
-  @BeforeSave
-  static doubleWriteRole = async (model: User) => {
-    if (model.isAdmin) {
-      model.role = UserRole.Admin;
-    } else if (model.isViewer) {
-      model.role = UserRole.Viewer;
-    } else {
-      model.role = UserRole.Member;
-    }
-  };
-
   @BeforeCreate
   static setRandomJwtSecret = (model: User) => {
     model.jwtSecret = crypto.randomBytes(64).toString("hex");
@@ -677,8 +644,8 @@ class User extends ParanoidModel<
     const countSql = `
       SELECT
         COUNT(CASE WHEN "suspendedAt" IS NOT NULL THEN 1 END) as "suspendedCount",
-        COUNT(CASE WHEN "isAdmin" = true THEN 1 END) as "adminCount",
-        COUNT(CASE WHEN "isViewer" = true THEN 1 END) as "viewerCount",
+        COUNT(CASE WHEN "role" = :roleAdmin THEN 1 END) as "adminCount",
+        COUNT(CASE WHEN "role" = :roleViewer THEN 1 END) as "viewerCount",
         COUNT(CASE WHEN "lastActiveAt" IS NULL THEN 1 END) as "invitedCount",
         COUNT(CASE WHEN "suspendedAt" IS NULL AND "lastActiveAt" IS NOT NULL THEN 1 END) as "activeCount",
         COUNT(*) as count
@@ -690,6 +657,8 @@ class User extends ParanoidModel<
       type: QueryTypes.SELECT,
       replacements: {
         teamId,
+        roleAdmin: UserRole.Admin,
+        roleViewer: UserRole.Viewer,
       },
     });
 

server/policies/collection.test.ts

@@ -1,4 +1,4 @@
-import { CollectionPermission } from "@shared/types";
+import { CollectionPermission, UserRole } from "@shared/types";
 import { UserMembership, Collection } from "@server/models";
 import {
   buildUser,
@@ -217,7 +217,7 @@ describe("viewer", () => {
     it("should allow read permissions for viewer", async () => {
       const team = await buildTeam();
       const user = await buildUser({
-        isViewer: true,
+        role: UserRole.Viewer,
         teamId: team.id,
       });
       const collection = await buildCollection({
@@ -235,7 +235,7 @@ describe("viewer", () => {
     it("should override read membership permission", async () => {
       const team = await buildTeam();
       const user = await buildUser({
-        isViewer: true,
+        role: UserRole.Viewer,
         teamId: team.id,
       });
       const collection = await buildCollection({
@@ -264,7 +264,7 @@ describe("viewer", () => {
     it("should allow override with read_write membership permission", async () => {
       const team = await buildTeam();
       const user = await buildUser({
-        isViewer: true,
+        role: UserRole.Viewer,
         teamId: team.id,
       });
       const collection = await buildCollection({
@@ -294,7 +294,7 @@ describe("viewer", () => {
     it("should allow no permissions for viewer", async () => {
       const team = await buildTeam();
       const user = await buildUser({
-        isViewer: true,
+        role: UserRole.Viewer,
         teamId: team.id,
       });
       const collection = await buildCollection({
@@ -310,7 +310,7 @@ describe("viewer", () => {
     it("should allow override with team member membership permission", async () => {
       const team = await buildTeam();
       const user = await buildUser({
-        isViewer: true,
+        role: UserRole.Viewer,
         teamId: team.id,
       });
       const collection = await buildCollection({

server/policies/document.test.ts

@@ -1,4 +1,4 @@
-import { CollectionPermission } from "@shared/types";
+import { CollectionPermission, UserRole } from "@shared/types";
 import { Document } from "@server/models";
 import {
   buildUser,
@@ -39,7 +39,7 @@ describe("read_write collection", () => {
     const team = await buildTeam();
     const user = await buildUser({
       teamId: team.id,
-      isViewer: true,
+      role: UserRole.Viewer,
     });
     const collection = await buildCollection({
       teamId: team.id,

server/presenters/__snapshots__/user.test.ts.snap

@@ -11,6 +11,7 @@ exports[`presents a user 1`] = `
   "isViewer": false,
   "lastActiveAt": undefined,
   "name": "Test User",
+  "role": "member",
   "updatedAt": undefined,
 }
 `;
@@ -26,6 +27,7 @@ exports[`presents a user without slack data 1`] = `
   "isViewer": false,
   "lastActiveAt": undefined,
   "name": "Test User",
+  "role": "member",
   "updatedAt": undefined,
 }
 `;

server/presenters/user.ts

@@ -1,4 +1,4 @@
-import { NotificationSettings, UserPreferences } from "@shared/types";
+import { NotificationSettings, UserPreferences, UserRole } from "@shared/types";
 import env from "@server/env";
 import { User } from "@server/models";
 
@@ -14,6 +14,7 @@ type UserPresentation = {
   updatedAt: Date;
   lastActiveAt: Date | null;
   color: string;
+  role: UserRole;
   isAdmin: boolean;
   isSuspended: boolean;
   isViewer: boolean;
@@ -32,6 +33,7 @@ export default function presentUser(
     name: user.name,
     avatarUrl: user.avatarUrl,
     color: user.color,
+    role: user.role,
     isAdmin: user.isAdmin,
     isSuspended: user.isSuspended,
     isViewer: user.isViewer,

server/routes/api/teams/teams.ts

@@ -1,5 +1,6 @@
 import invariant from "invariant";
 import Router from "koa-router";
+import { UserRole } from "@shared/types";
 import teamCreator from "@server/commands/teamCreator";
 import teamDestroyer from "@server/commands/teamDestroyer";
 import teamUpdater from "@server/commands/teamUpdater";
@@ -164,7 +165,7 @@ router.post(
         teamId: team.id,
         name: user.name,
         email: user.email,
-        isAdmin: true,
+        role: UserRole.Admin,
       },
       { transaction }
     );

server/routes/api/users/users.test.ts

@@ -1,4 +1,4 @@
-import { TeamPreference } from "@shared/types";
+import { TeamPreference, UserRole } from "@shared/types";
 import {
   buildTeam,
   buildAdmin,
@@ -368,8 +368,7 @@ describe("#users.invite", () => {
     const body = await res.json();
     expect(res.status).toEqual(200);
     expect(body.data.sent.length).toEqual(1);
-    expect(body.data.users[0].isViewer).toBeTruthy();
-    expect(body.data.users[0].isAdmin).toBeFalsy();
+    expect(body.data.users[0].role).toEqual(UserRole.Viewer);
   });
 
   it("should require authentication", async () => {
@@ -383,7 +382,6 @@ describe("#users.delete", () => {
     const user = await buildAdmin();
     await buildUser({
       teamId: user.teamId,
-      isAdmin: false,
     });
     const res = await server.post("/api/users.delete", {
       body: {
@@ -397,7 +395,6 @@ describe("#users.delete", () => {
     const user = await buildAdmin();
     await buildUser({
       teamId: user.teamId,
-      isAdmin: false,
     });
     const res = await server.post("/api/users.delete", {
       body: {
@@ -572,11 +569,7 @@ describe("#users.demote", () => {
   it("should demote an admin", async () => {
     const team = await buildTeam();
     const admin = await buildAdmin({ teamId: team.id });
-    const user = await buildUser({ teamId: team.id });
-
-    await user.update({
-      isAdmin: true,
-    }); // Make another admin
+    const user = await buildAdmin({ teamId: team.id });
 
     const res = await server.post("/api/users.demote", {
       body: {
@@ -590,11 +583,7 @@ describe("#users.demote", () => {
   it("should demote an admin to viewer", async () => {
     const team = await buildTeam();
     const admin = await buildAdmin({ teamId: team.id });
-    const user = await buildUser({ teamId: team.id });
-
-    await user.update({
-      isAdmin: true,
-    }); // Make another admin
+    const user = await buildAdmin({ teamId: team.id });
 
     const res = await server.post("/api/users.demote", {
       body: {
@@ -609,11 +598,7 @@ describe("#users.demote", () => {
   it("should demote an admin to member", async () => {
     const team = await buildTeam();
     const admin = await buildAdmin({ teamId: team.id });
-    const user = await buildUser({ teamId: team.id });
-
-    await user.update({
-      isAdmin: true,
-    }); // Make another admin
+    const user = await buildAdmin({ teamId: team.id });
 
     const res = await server.post("/api/users.demote", {
       body: {
@@ -748,9 +733,8 @@ describe("#users.count", () => {
 
   it("should count admin users", async () => {
     const team = await buildTeam();
-    const user = await buildUser({
+    const user = await buildAdmin({
       teamId: team.id,
-      isAdmin: true,
     });
     const res = await server.post("/api/users.count", {
       body: {

server/routes/api/users/users.ts

@@ -1,6 +1,6 @@
 import Router from "koa-router";
 import { Op, WhereOptions } from "sequelize";
-import { UserPreference } from "@shared/types";
+import { UserPreference, UserRole } from "@shared/types";
 import { UserValidation } from "@shared/validations";
 import userDemoter from "@server/commands/userDemoter";
 import userDestroyer from "@server/commands/userDestroyer";
@@ -59,17 +59,17 @@ router.post(
       }
 
       case "viewers": {
-        where = { ...where, isViewer: true };
+        where = { ...where, role: UserRole.Viewer };
         break;
       }
 
       case "admins": {
-        where = { ...where, isAdmin: true };
+        where = { ...where, role: UserRole.Admin };
         break;
       }
 
       case "members": {
-        where = { ...where, isAdmin: false, isViewer: false };
+        where = { ...where, role: UserRole.Member };
         break;
       }
 

server/scripts/seed.ts

@@ -1,4 +1,5 @@
 import "./bootstrap";
+import { UserRole } from "@shared/types";
 import teamCreator from "@server/commands/teamCreator";
 import env from "@server/env";
 import { Team, User } from "@server/models";
@@ -23,8 +24,7 @@ export default async function main(exit = false) {
           teamId: team.id,
           name: email.split("@")[0],
           email,
-          isAdmin: true,
-          isViewer: false,
+          role: UserRole.Admin,
         },
         {
           transaction,

server/test/factories.ts

@@ -11,6 +11,7 @@ import {
   IntegrationService,
   IntegrationType,
   NotificationEventType,
+  UserRole,
 } from "@shared/types";
 import {
   Share,
@@ -213,11 +214,11 @@ export async function buildUser(overrides: Partial<User> = {}) {
 }
 
 export async function buildAdmin(overrides: Partial<User> = {}) {
-  return buildUser({ ...overrides, isAdmin: true });
+  return buildUser({ ...overrides, role: UserRole.Admin });
 }
 
 export async function buildViewer(overrides: Partial<User> = {}) {
-  return buildUser({ ...overrides, isViewer: true });
+  return buildUser({ ...overrides, role: UserRole.Viewer });
 }
 
 export async function buildInvite(overrides: Partial<User> = {}) {

yarn.lock

@@ -1079,16 +1079,7 @@
     debug "^4.3.1"
     globals "^11.1.0"
 
-"@babel/types@^7.0.0", "@babel/types@^7.18.6", "@babel/types@^7.18.9", "@babel/types@^7.20.2", "@babel/types@^7.20.5", "@babel/types@^7.22.15", "@babel/types@^7.22.5", "@babel/types@^7.23.0", "@babel/types@^7.23.6", "@babel/types@^7.3.0", "@babel/types@^7.3.3", "@babel/types@^7.4.4", "@babel/types@^7.7.0":
-  version "7.23.6"
-  resolved "https://registry.yarnpkg.com/@babel/types/-/types-7.23.6.tgz#be33fdb151e1f5a56877d704492c240fc71c7ccd"
-  integrity "sha1-vjP9sVHh9aVod9cESSwkD8ccfM0= sha512-+uarb83brBzPKN38NX1MkB6vb6+mwvR6amUulqAE7ccQw1pEl+bCia9TbdG1lsnFP7lZySvUn37CHyXQdfTwzg=="
-  dependencies:
-    "@babel/helper-string-parser" "^7.23.4"
-    "@babel/helper-validator-identifier" "^7.22.20"
-    to-fast-properties "^2.0.0"
-
-"@babel/types@^7.23.4":
+"@babel/types@^7.0.0", "@babel/types@^7.18.6", "@babel/types@^7.18.9", "@babel/types@^7.20.2", "@babel/types@^7.20.5", "@babel/types@^7.22.15", "@babel/types@^7.22.5", "@babel/types@^7.23.0", "@babel/types@^7.23.4", "@babel/types@^7.23.6", "@babel/types@^7.3.0", "@babel/types@^7.3.3", "@babel/types@^7.4.4", "@babel/types@^7.7.0":
   version "7.24.0"
   resolved "https://registry.yarnpkg.com/@babel/types/-/types-7.24.0.tgz#3b951f435a92e7333eba05b7566fd297960ea1bf"
   integrity sha512-+j7a5c253RfKh8iABBhywc8NSfP5LURe7Uh4qpsh6jc+aLJguvmIUBdjSdEMQv2bENrCR5MfRdjGo7vzS/ob7w==