Skip to content
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

feat(): add IS_MULTIWORKSPACE_ENABLE flag #8656

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

AMoreaux
Copy link
Contributor

Summary

Add support for multi-workspace feature and adjust configurations and states accordingly.

  • Introduced new state isMultiWorkspaceEnabledState.
  • Updated ClientConfigProviderEffect component to handle multi-workspace.
  • Modified GraphQL schema and queries to include multi-workspace related configurations.
  • Adjusted server environment variables and their respective documentation to support multi-workspace toggle.
  • Updated server-side logic to handle new multi-workspace configurations and conditions.

Replaced the 'IS_SIGN_UP_DISABLED' flag with 'IS_MULTIWORKSPACE_ENABLED' to control workspace creation. This change allows for conditional workspace setup based on the new environment variable. Also updated related service, resolver, and entity files.
Updated the IS_MULTIWORKSPACE_ENABLED setting description to indicate support for multiple workspaces. This update clarifies that a web server capable of managing subdomain wildcards is required.
Updated the .env.example file to replace the IS_SIGN_UP_DISABLED variable with IS_MULTIWORKSPACE_ENABLED. Removed the IS_SIGN_UP_DISABLED property from environment-variables.ts as it is no longer needed.
…nabled

Removed isSignUpDisabledState and associated references, and replaced them with isMultiworkspaceEnabledState. Updated GraphQL queries and related components to reflect this change, and ensured multi-workspace functionality is enabled where applicable.
Corrected the casing of the `isMultiWorkspaceEnabled` variable in the NavigationDrawerHeader component. This ensures consistent naming conventions and avoids potential bugs related to variable misnaming.
Copy link

github-actions bot commented Nov 21, 2024

Warnings
⚠️ Changes were made to the environment variables, but not to the documentation - Please review your changes and check if a change needs to be documented!

Generated by 🚫 dangerJS against 270694e

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR introduces multi-workspace functionality by replacing the sign-up disabled flag with a multi-workspace enabled flag across the application.

  • Inconsistent casing in state key naming (isMultiworkspaceEnabled vs isMultiWorkspaceEnabled) in /packages/twenty-front/src/modules/client-config/states/isMultiWorkspaceEnabledState.ts
  • Default value mismatch between frontend (true) and server (false) for isMultiWorkspaceEnabled could cause inconsistent behavior
  • Removal of signUpDisabled state without clear alternative mechanism needs review for impact on user registration flows
  • Added note in documentation about wildcard subdomain requirement for multi-workspace feature
  • Server-side logic in sign-in-up.service.ts now blocks creation of additional workspaces when multi-workspace is disabled

12 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one comment about default value

Corrected the typo from isMultiworkspaceEnabled to isMultiWorkspaceEnabled. This change affects multiple files to ensure consistency across the codebase, including GraphQL queries, state management, and server configurations.
@AMoreaux
Copy link
Contributor Author

⚠️ Before merging it wait for the PR that adds subdomains.

@AMoreaux AMoreaux marked this pull request as draft November 21, 2024 16:58
@AMoreaux
Copy link
Contributor Author

AMoreaux commented Nov 21, 2024

@martmull I converted this PR to a draft to avoid the merge since we must wait for the review on child PR to have the full feature.

Especially this one #8654
And another one about subdomains, I will open soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants