Org settings partial updates improvements #20626
Conversation
Tool: gitpod/catfood.gitpod.cloud fix for getting settings (also resolve avatar) Tool: gitpod/catfood.gitpod.cloud
Tool: gitpod/catfood.gitpod.cloud
The hack was introduced in #20620 - Update welcome message configuration to handle optional enabled field - Simplify welcome message update logic in various components - Modify protobuf definitions to make enabled field optional - Remove redundant checks and simplify type handling in converters and components Tool: gitpod/catfood.gitpod.cloud
filiptronicek
force-pushed
the
ft/gpl/org-settings-refresh
branch
from
3334f6d to
8b563ec
Compare
filiptronicek
changed the title
| @@ -27,13 +28,12 @@ export type SuggestedOrgRepository = PlainMessage<SuggestedRepository> & { | |||
|
|
|||
| export function useOrgSuggestedRepos() { | |||
| const organizationId = useCurrentOrg().data?.id; | |||
| const orgSettings = useOrgSettingsQuery(); | |||
| throw new ApplicationError( | ||
| ErrorCodes.BAD_REQUEST, | ||
| "featuredMemberResolvedAvatarUrl is not allowed to be set", | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
It's ok to extract the code, but it should still be called here, not anywhere else: This is the one-and-only spot in our code base where we do domain-level validations. Also helps writing tests for it. 🙂
| settings.onboardingSettings.welcomeMessage.featuredMemberId = undefined; | ||
| settings.onboardingSettings.welcomeMessage.featuredMemberResolvedAvatarUrl = undefined; | ||
| } | ||
| if ( |
There was a problem hiding this comment.
This block should move out of the mergeSettings, and merged with the other welcomeMessage validation.
| @@ -658,6 +639,45 @@ export class OrganizationService { | |||
| return this.toSettings(await this.teamDB.setOrgSettings(orgId, settings, mergeSettings)); | |||
| } | |||
|
|
|||
| async getSettingsWithResolvedWelcomeMessage(userId: string, orgId: string): Promise<OrganizationSettings> { | |||
| const settings = await this.getSettings(userId, orgId); | |||
| return this.resolveWelcomeMessage(settings, orgId); | |||
There was a problem hiding this comment.
IMO the resolution of the welcomeMessage should never fail or block the retrieval of the org settings. Rather, we should return undefined for avatarUrl.
In general it might make sense to only extract the resolveFeaturedMemberAvatarURL functionality, and keep the rest where it was.
There was a problem hiding this comment.
Also, this method lags a comment. 🤝
| return this.resolveWelcomeMessage(settings, orgId); | ||
| } | ||
|
|
||
| async updateSettingsWithResolvedWelcomeMessage( |
There was a problem hiding this comment.
I don't think we need this, if we call the resolveFeaturedMemberAvatarURL() inside the update function above. That way only the retrieval path is special, which a) keeps variance low, and b) aligns nicely with user expectations IMO.
There was a problem hiding this comment.
I liked being explicit with both the update and get methods, but you're right — for the update case, we'd just be rolling out another layer, which can be bundled into updateSettings.
Description
This PR contains the following fixes on top of #20603:
resolveWelcomeMessagefnHow to test
https://ft-gpl-org7e7f3fc03d.preview.gitpod-dev.com/settings/onboarding
/hold