[api, server, dashboard] Cleanup UpdateOrganizationSettings API

Tool: gitpod/catfood.gitpod.cloud

Author: geropl

components/dashboard/src/data/organizations/update-org-settings-mutation.ts

@@ -28,7 +28,7 @@ export const useUpdateOrgSettingsMutation = () => {
 
     return useMutation<OrganizationSettings, Error, UpdateOrganizationSettingsArgs>({
         mutationFn: async (partialUpdate) => {
-            const update: PartialMessage<UpdateOrganizationSettingsRequest> = {
+            const update: UpdateOrganizationSettingsArgs = {
                 ...partialUpdate,
             };
             update.organizationId = organizationId;

components/dashboard/src/service/json-rpc-organization-client.ts

@@ -4,7 +4,7 @@
  * See License.AGPL.txt in the project root for license information.
  */
 
-import { PartialMessage } from "@bufbuild/protobuf";
+import { PartialMessage, PlainMessage } from "@bufbuild/protobuf";
 import { CallOptions, PromiseClient } from "@connectrpc/connect";
 import { OrganizationService } from "@gitpod/public-api/lib/gitpod/v1/organization_connect";
 import {
@@ -41,7 +41,6 @@ import {
 import { getGitpodService } from "./service";
 import { converter } from "./public-api";
 import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
-import { OrgMemberRole, RoleRestrictions } from "@gitpod/gitpod-protocol";
 
 export class JsonRpcOrganizationClient implements PromiseClient<typeof OrganizationService> {
     async createOrganization(
@@ -228,56 +227,62 @@ export class JsonRpcOrganizationClient implements PromiseClient<typeof Organizat
         if (!request.organizationId) {
             throw new ApplicationError(ErrorCodes.BAD_REQUEST, "organizationId is required");
         }
-        const update: Partial<OrganizationSettings> = {
-            workspaceSharingDisabled: request?.workspaceSharingDisabled,
-            defaultWorkspaceImage: request?.defaultWorkspaceImage,
-            allowedWorkspaceClasses: request?.allowedWorkspaceClasses,
-            restrictedEditorNames: request?.restrictedEditorNames,
-            defaultRole: request?.defaultRole,
-        };
-        if (request.updatePinnedEditorVersions) {
-            update.pinnedEditorVersions = request.pinnedEditorVersions;
-        } else if (request.pinnedEditorVersions && Object.keys(request.pinnedEditorVersions).length > 0) {
+
+        if (
+            request.restrictedEditorNames &&
+            request.restrictedEditorNames.length > 0 &&
+            !request.updateRestrictedEditorNames
+        ) {
             throw new ApplicationError(
                 ErrorCodes.BAD_REQUEST,
-                "updatePinnedEditorVersions is required to be true to update pinnedEditorVersions",
+                "updateRestrictedEditorNames is required to be true to update restrictedEditorNames",
             );
         }
-        if (request.updateRestrictedEditorNames) {
-            update.restrictedEditorNames = request.restrictedEditorNames;
-        } else if (request.restrictedEditorNames && request.restrictedEditorNames.length > 0) {
+
+        if (
+            request.allowedWorkspaceClasses &&
+            request.allowedWorkspaceClasses.length > 0 &&
+            !request.updateAllowedWorkspaceClasses
+        ) {
             throw new ApplicationError(
                 ErrorCodes.BAD_REQUEST,
-                "updateRestrictedEditorNames is required to be true to update restrictedEditorNames",
+                "updateAllowedWorkspaceClasses is required to be true to update allowedWorkspaceClasses",
             );
         }
-        const roleRestrictions: RoleRestrictions = {};
-        if (request.updateRoleRestrictions) {
-            for (const roleRestriction of request?.roleRestrictions ?? []) {
-                if (!roleRestriction.role) {
-                    throw new ApplicationError(ErrorCodes.BAD_REQUEST, "role is required");
-                }
-                const role = converter.fromOrgMemberRole(roleRestriction.role);
-                const permissions = roleRestriction?.permissions?.map((p) => converter.fromOrganizationPermission(p));
 
-                roleRestrictions[role] = permissions;
-            }
-        } else if (request.roleRestrictions && Object.keys(request.roleRestrictions).length > 0) {
+        if (
+            request.pinnedEditorVersions &&
+            Object.keys(request.pinnedEditorVersions).length > 0 &&
+            !request.updatePinnedEditorVersions
+        ) {
             throw new ApplicationError(
                 ErrorCodes.BAD_REQUEST,
-                "updateRoleRestrictions is required to be true to update roleRestrictions",
+                "updatePinnedEditorVersions is required to be true to update pinnedEditorVersions",
             );
         }
 
-        await getGitpodService().server.updateOrgSettings(request.organizationId, {
-            ...update,
-            defaultRole: request.defaultRole as OrgMemberRole,
-            timeoutSettings: {
-                inactivity: converter.toDurationStringOpt(request.timeoutSettings?.inactivity),
-                denyUserTimeouts: request.timeoutSettings?.denyUserTimeouts,
-            },
-            roleRestrictions,
-        });
+        if (request.roleRestrictions && request.roleRestrictions.length > 0 && !request.updateRoleRestrictions) {
+            throw new ApplicationError(
+                ErrorCodes.BAD_REQUEST,
+                "updateRoleRestrictions is required to be true when updating roleRestrictions",
+            );
+        }
+        if (
+            request.onboardingSettings?.recommendedRepositories &&
+            request.onboardingSettings.recommendedRepositories.length > 0 &&
+            !request.onboardingSettings.updateRecommendedRepositories
+        ) {
+            throw new ApplicationError(
+                ErrorCodes.BAD_REQUEST,
+                "recommendedRepositories can only be set when updateRecommendedRepositories is true",
+            );
+        }
+
+        // gpl: We accept the little bit of uncertainty here because a) the partial/not-partial mismatch is only about
+        // technical/private fields and b) this path should not be exercised anymore anyway.
+        const update = converter.fromOrganizationSettings(request as PlainMessage<OrganizationSettings>);
+
+        await getGitpodService().server.updateOrgSettings(request.organizationId, update);
         return new UpdateOrganizationSettingsResponse();
     }
 }

components/dashboard/src/teams/TeamOnboarding.tsx

@@ -54,7 +54,6 @@ export default function TeamOnboardingPage() {
             }
             try {
                 await updateTeamSettings.mutateAsync({
-                    ...settings,
                     ...newSettings,
                 });
                 toast("Organization settings updated");
@@ -66,7 +65,7 @@ export default function TeamOnboardingPage() {
                 console.error(error);
             }
         },
-        [updateTeamSettings, org?.id, isOwner, settings, toast],
+        [updateTeamSettings, org?.id, isOwner, toast],
     );
 
     const handleUpdateInternalLink = useCallback(

components/gitpod-db/src/typeorm/entity/db-team-settings.ts

@@ -25,34 +25,34 @@ export class DBOrgSettings implements OrganizationSettings {
     workspaceSharingDisabled?: boolean;
 
     @Column("varchar", { nullable: true })
-    defaultWorkspaceImage?: string | null;
+    defaultWorkspaceImage?: string;
 
     @Column("json", { nullable: true })
-    allowedWorkspaceClasses?: string[] | null;
+    allowedWorkspaceClasses?: string[];
 
     @Column("json", { nullable: true })
-    pinnedEditorVersions?: { [key: string]: string } | null;
+    pinnedEditorVersions?: { [key: string]: string };
 
     @Column("json", { nullable: true })
-    restrictedEditorNames?: string[] | null;
+    restrictedEditorNames?: string[];
 
     @Column("varchar", { nullable: true })
-    defaultRole?: OrgMemberRole | undefined;
+    defaultRole?: OrgMemberRole;
 
     @Column("json", { nullable: true })
-    timeoutSettings?: TimeoutSettings | undefined;
+    timeoutSettings?: TimeoutSettings;
 
     @Column("json", { nullable: true })
-    roleRestrictions?: RoleRestrictions | undefined;
+    roleRestrictions?: RoleRestrictions;
 
     @Column({ type: "int", default: 0 })
     maxParallelRunningWorkspaces: number;
 
     @Column("json", { nullable: true })
-    onboardingSettings?: OnboardingSettings | undefined;
+    onboardingSettings?: OnboardingSettings;
 
     @Column({ type: "boolean", default: false })
-    annotateGitCommits?: boolean | undefined;
+    annotateGitCommits?: boolean;
 
     @Column()
     deleted: boolean;

components/gitpod-protocol/src/teams-projects-protocol.ts

@@ -221,15 +221,15 @@ export interface Organization {
 
 export interface OrganizationSettings {
     workspaceSharingDisabled?: boolean;
-    // null or empty string to reset to default
-    defaultWorkspaceImage?: string | null;
+    // undefined or empty string to reset to default
+    defaultWorkspaceImage?: string;
 
     // empty array to allow all kind of workspace classes
-    allowedWorkspaceClasses?: string[] | null;
+    allowedWorkspaceClasses?: string[];
 
-    pinnedEditorVersions?: { [key: string]: string } | null;
+    pinnedEditorVersions?: { [key: string]: string };
 
-    restrictedEditorNames?: string[] | null;
+    restrictedEditorNames?: string[];
 
     // what role new members will get, default is "member"
     defaultRole?: OrgMemberRole;

components/public-api/gitpod/v1/organization.proto

@@ -85,21 +85,22 @@ message OnboardingSettings {
   optional WelcomeMessage welcome_message = 4;
 }
 
+// KEEP ALIGNED WITH OrganizationSettings shape below!!!
 message OrganizationSettings {
-  bool workspace_sharing_disabled = 1;
-  string default_workspace_image = 2;
+  optional bool workspace_sharing_disabled = 1;
+  optional string default_workspace_image = 2;
   repeated string allowed_workspace_classes = 3;
   repeated string restricted_editor_names = 4;
   map<string, string> pinned_editor_versions = 5;
-  string default_role = 6;
-  TimeoutSettings timeout_settings = 7;
+  optional string default_role = 6;
+  optional TimeoutSettings timeout_settings = 7;
   repeated RoleRestrictionEntry role_restrictions = 8;
   // max_parallel_running_workspaces is the maximum number of workspaces that a
   // single user can run in parallel. 0 resets to the default, which depends on
   // the org plan
-  int32 max_parallel_running_workspaces = 9;
-  OnboardingSettings onboarding_settings = 10;
-  bool annotate_git_commits = 11;
+  optional int32 max_parallel_running_workspaces = 9;
+  optional OnboardingSettings onboarding_settings = 10;
+  optional bool annotate_git_commits = 11;
 }
 
 service OrganizationService {