-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepare workspace for search features at creation #7467
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This pull request introduces changes to enable search-related feature flags during workspace creation, enhancing the search functionality for new workspaces.
- Added
enableDefaultFeatureFlags
method inWorkspaceService
to set search-related feature flags upon workspace activation - Created new file
default-feature-flags.ts
definingDEFAULT_FEATURE_FLAGS
array withIsSearchEnabled
andIsWorkspaceMigratedForSearch
flags - Imported
FeatureFlagModule
inworkspace.module.ts
to support feature flag functionality - Modified
WorkspaceService
constructor to injectFeatureFlagEntity
repository for managing feature flags - Note:
IsQueryRunnerTwentyORMEnabled
flag, required for front-end functionality, is not included in this update
3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @ijreilly
@@ -152,4 +158,14 @@ export class WorkspaceService extends TypeOrmQueryService<Workspace> { | |||
); | |||
} | |||
} | |||
|
|||
private async enableDefaultFeatureFlags(workspaceId: string) { | |||
DEFAULT_FEATURE_FLAGS.forEach(async (featureFlagKey) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do a batch insert by passing an array instead of calling save n times 👍
await this.featureFlagRepository.save(
DEFAULT_FEATURE_FLAGS.map((featureFlagKey) => ({
key: featureFlagKey,
value: true,
workspaceId,
})),
);
workspaceId: string, | ||
) { | ||
await this.featureFlagRepository.save({ | ||
key: featureFlagKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about this one, if we don't have a unicity constraint on the key (which I think we don't), won't it insert multiple times the same key (with false and true potentially), this could lead to issues during the fetch (if we get the first one for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine since you check before calling this function. However this function is called "enableFeatureFlag" so it might get confusing? Wdyt? Also should we move this to featureFlagService?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Enabling feature flags IsSearchEnabled and IsWorkspaceMigratedForSearch at workspace creation to ensure workspaces have the searchVector fields and indexes created. For the feature to be enabled in the front-end we will also need IsQueryRunnerTwentyORMEnabled to be enabled but that is an independent topic.
Enabling feature flags IsSearchEnabled and IsWorkspaceMigratedForSearch at workspace creation to ensure workspaces have the searchVector fields and indexes created.
For the feature to be enabled in the front-end we will also need IsQueryRunnerTwentyORMEnabled to be enabled but that is an independent topic.