refactor: Remove strong dependency from feature flag enum to (de)serialise organization object #40374
Conversation
…alise organization object
WalkthroughThis change refactors the handling of feature flags with pending migrations by replacing the use of the Changes
Sequence Diagram(s)sequenceDiagram
participant Service as OrganizationServiceCEImpl
participant Helper as FeatureFlagMigrationHelperCEImpl
participant Org as Organization
Service->>Helper: getUpdatedFlagsWithPendingMigration(Org)
Helper-->>Service: Mono<Map<String, FeatureMigrationType>>
Service->>Helper: checkAndExecuteMigrationsForFeatureFlag(featureFlagEnum, Org)
Helper-->>Service: Migration result
Service->>Org: Remove featureFlagKey from pending migrations
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java (1)
10-10: Remove unused import.The
ScheduledTaskCEImplimport is only referenced in a comment at line 34 but not used in the code.-import com.appsmith.server.solutions.ce.ScheduledTaskCEImpl;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java(9 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/FeatureFlagMigrationHelperTest.java(7 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (30)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java (1)
60-60: Appropriate key type change for mapThe change from
Map<FeatureFlagEnum, FeatureMigrationType>toMap<String, FeatureMigrationType>properly implements the decoupling of feature flag enums from the organization configuration. This reduces direct dependencies and facilitates easier serialization/deserialization.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java (2)
341-345: Test updated correctly to use string keysThe test data initialization has been properly updated to use string keys (enum names) instead of enum values directly. This aligns with the refactoring changes in the main code.
370-374: Test updated correctly to use string keysSimilar to the previous test case, the map initialization properly uses string keys via
name()method on the enum constants. This maintains consistency with the refactoring approach.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/FeatureFlagServiceCETest.java (4)
229-229: Mock return value updated to use string keysMock data properly updated to use string representation of feature flag enum as key.
240-240: Assertion updated to use string keysAssertion correctly updated to verify using string keys instead of enum constants.
252-252: Mock return value updated to use string keysMock data properly updated for enable migration test case.
263-263: Assertion updated to use string keysAssertion correctly updated for enable migration test case.
app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/FeatureFlagMigrationHelperTest.java (9)
67-67: Return type updated to use string keysMethod return type correctly updated to use
Map<String, FeatureMigrationType>instead of enum-based map.
74-75: Assertion updated to use string keysMap lookup in assertion properly uses the string representation of enum via
.name().
105-106: Return type updated consistentlyReturn type consistently updated in another test method.
112-113: Assertion updated consistentlyMap lookup in assertion consistently updated in another test method.
136-137: Return type updated consistentlyReturn type consistently updated in third test method.
174-175: Return type updated consistentlyReturn type consistently updated in fourth test method.
200-201: Test data updated to use string keysMap initialization using
Map.of()correctly uses string representation of enum constant.
233-234: Test data updated to use string keysMap initialization for pending migration test case correctly uses string keys.
258-259: Return type updated consistentlyReturn type consistently updated in last test method.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCE.java (2)
12-12: Method signature change aligns with refactoring strategy.The return type change from
Mono<Map<FeatureFlagEnum, FeatureMigrationType>>toMono<Map<String, FeatureMigrationType>>is part of the broader refactoring to decouple feature flag serialization from the enum type.
14-15: Method signature change maintains consistency across the interface.This overloaded method signature has been updated to match the refactoring pattern of using string keys instead of enum keys.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java (5)
293-294: Map type change aligns with domain model changes.The map type has been updated from
Map<FeatureFlagEnum, FeatureMigrationType>toMap<String, FeatureMigrationType>to match the changes inOrganizationConfigurationCE.
296-309: Well-implemented error handling for unknown feature flags.The new code properly handles unknown feature flags by safely converting string keys to enum values with appropriate error handling. This is more robust than the previous implementation which would have thrown exceptions for unknown flags.
310-310: Using the converted enum value in method call.The code correctly passes the converted enum value to the helper method while handling the case where conversion might have failed (finalFeatureFlagEnum could be null).
313-313: Consistent use of string key for map operations.The code correctly uses the original string key for removing items from the map after successful migration, maintaining consistency with the new string-based approach.
326-326: Using enum value in exception message.The code uses the converted enum in the error message, which provides better context for troubleshooting migration failures.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/FeatureFlagMigrationHelperCEImpl.java (7)
40-43: Method signature updated to match interface changes.The implementation correctly aligns with the interface changes, returning a map with string keys instead of enum keys.
53-55: Method signature updated consistently.The overloaded method implementation also correctly matches the interface changes.
131-148: Map operations consistently use string keys.The helper method has been updated to work with string keys throughout. The error handling at lines 140-144 has been simplified, which is appropriate since the direct enum conversion is no longer needed.
150-192: Updated map handling logic for feature flags.The method has been refactored to use string keys throughout, maintaining consistency with the broader refactoring. All map operations and comparisons now work with strings instead of enum values.
214-224: Consistent use of enum name for map lookups.The code now uses
featureFlagEnum.name()to look up values in the map, which is consistent with the refactoring to use string keys.
235-253: Feature migration map type updated for consistency.The map type and lookups have been updated to use string keys while maintaining the core business logic.
262-267: Added clarifying comment for placeholder implementation.The comment clarifies that this is a placeholder meant to be extended by subclasses, which improves code maintainability.
Description
Refactor
Tests
/test Settings
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14657519125
Commit: ab4973c
Cypress dashboard.
Tags:
@tag.SettingsSpec:
Fri, 25 Apr 2025 05:39:47 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Refactor
Tests