chore: Allow side effects for organization configuration updates#39692
chore: Allow side effects for organization configuration updates#39692nidhi-nair merged 1 commit intoreleasefrom
Conversation
WalkthroughThe pull request modifies the organization configuration update process in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant OS as OrganizationServiceCEImpl
participant CS as Cache Service
C->>OS: updateOrganizationConfiguration(oldConfig, newConfig)
OS->>OS: calculateOrganizationConfigurationUpdateSideEffects(oldConfig, newConfig)
OS->>CS: Evict cache
OS->>OS: Process side effects (Mono list)
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
|
github-actions
bot
added
the
skip-changelog
![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 (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java (2)
141-144: New protected method provides extension point for side effects.This is a good design choice that follows the Open/Closed Principle. The base implementation returns an empty list, allowing derived classes to override and extend with specific side effects without modifying the base class.
Consider adding a brief Javadoc comment explaining the purpose and expected behavior when overriding this method.
+ /** + * Calculates side effects that should be executed after an organization configuration update. + * Override this method in derived classes to implement specific side effects. + * + * @param oldConfig The previous organization configuration + * @param organizationConfiguration The new organization configuration + * @return List of side effect operations as Mono<Boolean> + */ protected List<Mono<Boolean>> calculateOrganizationConfigurationUpdateSideEffects( OrganizationConfiguration oldConfig, OrganizationConfiguration organizationConfiguration) { return new ArrayList<>(); }
121-137: Consider adding error handling for side effects.The current implementation will fail the entire operation if any side effect fails. Depending on the nature of these side effects, you might want to consider more resilient error handling.
List<Mono<Boolean>> sideEffectsMonos = calculateOrganizationConfigurationUpdateSideEffects(oldConfig, organizationConfiguration); Mono<List<Boolean>> allSideEffectsMono = - Flux.fromIterable(sideEffectsMonos).flatMap(x -> x).collectList(); + Flux.fromIterable(sideEffectsMonos) + .flatMap(mono -> mono.onErrorResume(e -> { + log.error("Error executing organization configuration side effect: {}", e.getMessage(), e); + return Mono.just(false); // Continue with other side effects even if one fails + })) + .collectList();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java (3)
32-33: Appropriate imports added for the new functionality.The addition of
ArrayListandListimports is necessary for implementing the new side effects functionality.
121-126: Implementation properly handles side effects in reactive flow.The code creates a list of side effect tasks by calling the new protected method and then converts them to a Flux for parallel processing. This follows good reactive programming patterns by:
- Collecting potential side effects
- Processing them efficiently with Reactor's reactive types
- Ensuring they're executed after the organization update
136-136: Side effects are properly sequenced in the reactive chain.The modification ensures side effects are executed after cache eviction but before returning the updated organization, maintaining proper execution order.
There was a problem hiding this comment.
@nidhi-nair do we have the corresponding change in EE, I wasn't able to make sense of this PR. Approving to unblock you here.
…smithorg#39692) ## Description > [!TIP] > _Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team)._ > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13811137863> > Commit: a3ebf0a > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13811137863&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Wed, 12 Mar 2025 12:56:52 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced the organization settings update process to ensure all related actions are executed smoothly, improving overall administrative consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13811137863
Commit: a3ebf0a
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 12 Mar 2025 12:56:52 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit