[api, server, dashboard] Cleanup UpdateOrganizationSettings API

Tool: gitpod/catfood.gitpod.cloud

Author: geropl

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

@@ -8,16 +8,13 @@ import { useMutation } from "@tanstack/react-query";
 import { useOrgSettingsQueryInvalidator } from "./org-settings-query";
 import { useCurrentOrg } from "./orgs-query";
 import { organizationClient } from "../../service/public-api";
-import {
-    OrganizationSettings,
-    UpdateOrganizationSettingsRequest,
-} from "@gitpod/public-api/lib/gitpod/v1/organization_pb";
+import { OrganizationSettings } from "@gitpod/public-api/lib/gitpod/v1/organization_pb";
 import { ErrorCode } from "@gitpod/gitpod-protocol/lib/messaging/error";
 import { useOrgWorkspaceClassesQueryInvalidator } from "./org-workspace-classes-query";
 import { useOrgRepoSuggestionsInvalidator } from "./suggested-repositories-query";
 import { PartialMessage } from "@bufbuild/protobuf";
 
-export type UpdateOrganizationSettingsArgs = PartialMessage<UpdateOrganizationSettingsRequest>;
+export type UpdateOrganizationSettingsArgs = PartialMessage<OrganizationSettings>;
 
 export const useUpdateOrgSettingsMutation = () => {
     const org = useCurrentOrg().data;
@@ -28,10 +25,9 @@ export const useUpdateOrgSettingsMutation = () => {
 
     return useMutation<OrganizationSettings, Error, UpdateOrganizationSettingsArgs>({
         mutationFn: async (partialUpdate) => {
-            const update: PartialMessage<UpdateOrganizationSettingsRequest> = {
+            const update: UpdateOrganizationSettingsArgs = {
                 ...partialUpdate,
             };
-            update.organizationId = organizationId;
             update.updatePinnedEditorVersions = update.pinnedEditorVersions !== undefined;
             update.updateRestrictedEditorNames = update.restrictedEditorNames !== undefined;
             update.updateRoleRestrictions = update.roleRestrictions !== undefined;
@@ -44,7 +40,10 @@ export const useUpdateOrgSettingsMutation = () => {
                 }
             }
 
-            const settings = await organizationClient.updateOrganizationSettings(update);
+            const settings = await organizationClient.updateOrganizationSettings({
+                organizationId: organizationId,
+                settings: update,
+            });
             return settings.settings!;
         },
         onSuccess: () => {

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) {
+        const settings = request.settings;
+        if (!settings) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "nothing to update");
+        }
+
+        if (
+            settings.restrictedEditorNames &&
+            settings.restrictedEditorNames.length > 0 &&
+            !settings.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 (
+            settings.allowedWorkspaceClasses &&
+            settings.allowedWorkspaceClasses.length > 0 &&
+            !settings.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 (settings.pinnedEditorVersions && !settings.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 (settings.roleRestrictions && settings.roleRestrictions.length > 0 && !settings.updateRoleRestrictions) {
+            throw new ApplicationError(
+                ErrorCodes.BAD_REQUEST,
+                "updateRoleRestrictions is required to be true when updating roleRestrictions",
+            );
+        }
+        if (
+            settings.onboardingSettings?.recommendedRepositories &&
+            settings.onboardingSettings.recommendedRepositories.length > 0 &&
+            !settings.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(settings 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

@@ -86,20 +86,62 @@ message OnboardingSettings {
 }
 
 message OrganizationSettings {
-  bool workspace_sharing_disabled = 1;
-  string default_workspace_image = 2;
+  optional bool workspace_sharing_disabled = 1;
+
+  // pass empty string to reset to the installation default workspace image
+  optional string default_workspace_image = 2;
+
+  // allowed_workspace_classes are the IDs of classes, which can be used by
+  // workspaces in an organization. Pass an empty array to allow all workspace
+  // classes.
+  // Only updates if update_allowed_workspace_classes is true.
   repeated string allowed_workspace_classes = 3;
+
+  // restricted_editor_names updates the list of restricted editor names that
+  // are not allowed to be used by workspaces in an organization. If empty, all
+  // editors are allowed.
+  // Only updates if update_restricted_editor_names is true.
   repeated string restricted_editor_names = 4;
+
+  // pinned_editor_versions updates the pinned version for the corresponding
+  // editor.
+  // Only updates if update_pinned_editor_versions is true.
   map<string, string> pinned_editor_versions = 5;
-  string default_role = 6;
-  TimeoutSettings timeout_settings = 7;
+
+  // default_role is the default role for new members in the organization
+  optional string default_role = 6;
+
+  // timeout_settings are the settings for workspace timeouts
+  optional TimeoutSettings timeout_settings = 7;
+
+  // Only updates if update_role_restrictions is true.
   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;
+
+  // onboarding_settings are the settings for the organization's onboarding
+  optional OnboardingSettings onboarding_settings = 10;
+
+  // annotate_git_commits specifies whether to annotate git commits created in
+  // Gitpod workspaces with the gitpod host
+  optional bool annotate_git_commits = 11;
+
+  // update_role_restrictions specifies whether role_restrictions should be
+  // updated
+  optional bool update_allowed_workspace_classes = 12;
+
+  // Specifies whether restricted_workspace_classes should be updated
+  optional bool update_restricted_editor_names = 13;
+
+  // Specifies whether pinned_editor_versions should be updated
+  optional bool update_pinned_editor_versions = 14;
+
+  // update_role_restrictions specifies whether role_restrictions should be
+  // updated
+  optional bool update_role_restrictions = 15;
 }
 
 service OrganizationService {
@@ -187,62 +229,10 @@ message UpdateOrganizationSettingsRequest {
   // organization_id is the ID of the organization to update the settings for
   string organization_id = 1;
 
-  optional bool workspace_sharing_disabled = 3;
-
-  // pass empty string to reset to the installation default workspace image
-  optional string default_workspace_image = 4;
-
-  // allowed_workspace_classes are the IDs of classes, which can be used by
-  // workspaces in an organization. Pass an empty array to allow all workspace
-  // classes.
-  // Only updates if update_allowed_workspace_classes is true.
-  repeated string allowed_workspace_classes = 5;
-
-  // restricted_editor_names updates the list of restricted editor names that
-  // are not allowed to be used by workspaces in an organization. If empty, all
-  // editors are allowed.
-  // Only updates if update_restricted_editor_names is true.
-  repeated string restricted_editor_names = 6;
-
-  // Specifies whether restricted_workspace_classes should be updated
-  optional bool update_restricted_editor_names = 7;
-
-  // pinned_editor_versions updates the pinned version for the corresponding
-  // editor.
-  // Only updates if update_pinned_editor_versions is true.
-  map<string, string> pinned_editor_versions = 8;
+  // fields 2-28 were prior used for what's now combined in OrganizationSettings
 
-  // Specifies whether pinned_editor_versions should be updated
-  optional bool update_pinned_editor_versions = 9;
-
-  // default_role is the default role for new members in the organization
-  optional string default_role = 10;
-
-  // timeout_settings are the settings for workspace timeouts
-  optional TimeoutSettings timeout_settings = 11;
-
-  // Only updates if update_role_restrictions is true.
-  repeated RoleRestrictionEntry role_restrictions = 12;
-
-  // update_role_restrictions specifies whether role_restrictions should be
-  // updated
-  optional bool update_role_restrictions = 13;
-
-  // 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
-  optional int32 max_parallel_running_workspaces = 15;
-
-  // onboarding_settings are the settings for the organization's onboarding
-  optional OnboardingSettings onboarding_settings = 16;
-
-  // annotate_git_commits specifies whether to annotate git commits created in
-  // Gitpod workspaces with the gitpod host
-  optional bool annotate_git_commits = 17;
-
-  // update_role_restrictions specifies whether role_restrictions should be
-  // updated
-  optional bool update_allowed_workspace_classes = 18;
+  // settings to persist
+  OrganizationSettings settings = 19;
 }
 
 message UpdateOrganizationSettingsResponse {