chore: Dynamically fetch system generated users via DB query #40323
Conversation
WalkthroughA new immutable Java record, Changes
Sequence Diagram(s)sequenceDiagram
participant Test as CustomUserRepositoryCEImplTest
participant Repo as CustomUserRepositoryCEImpl
participant DB as Database
Test->>Repo: getSystemGeneratedUserEmails(organizationId)
Repo->>DB: Query users where isSystemGenerated = true and organizationId matches
DB-->>Repo: Return matching user emails
Repo-->>Test: Flux<String> of system-generated emails
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (6)
🪧 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 (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java (1)
34-39: Consider performance optimization for isUsersEmptyThe current implementation makes two sequential database queries - first collecting all system-generated emails, then querying for non-system users. This could be inefficient with a large number of system users.
Consider a single query approach:
@Override public Mono<Boolean> isUsersEmpty() { - return getSystemGeneratedUserEmails().collectList().flatMap(systemGeneratedUserEmails -> queryBuilder() - .criteria(Bridge.notIn(User.Fields.email, systemGeneratedUserEmails)) - .limit(1) - .all(IdOnly.class) - .count() - .map(count -> count == 0)); + return queryBuilder() + .criteria(Bridge.equal(User.Fields.isSystemGenerated, false)) + .limit(1) + .all(IdOnly.class) + .count() + .map(count -> count == 0); }app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java (2)
26-26: Consider making constant privateThe
PREDEFINED_SYSTEM_USERSconstant is only used internally in this test class.- private static final Set<String> PREDEFINED_SYSTEM_USERS = Set.of("anonymousUser"); + private static final Set<String> PREDEFINED_SYSTEM_USERS = Set.of("anonymousUser");
40-71: Comprehensive test for system user emailsThis test covers the case when both system and non-system users exist, with proper assertions.
Consider using non-blocking approaches for test setup/teardown:
- User savedSystemUser = userRepository.save(systemUser).block(); + User savedSystemUser = userRepository.save(systemUser) + .doOnNext(savedUsers::add) + .block(); - savedUsers.add(savedSystemUser);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/projections/EmailOnly.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (8)
app/server/appsmith-server/src/main/java/com/appsmith/server/projections/EmailOnly.java (1)
1-5: Well-designed record for email projectionThis is a clean implementation of a Java record for email projection with proper null-safety using
@NonNull. Records provide an excellent way to handle database projections with minimal boilerplate.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java (2)
6-6: Appropriate import for reactive FluxAdded import for Reactor's Flux class to support the updated method signature.
15-15: Good migration to reactive return typeChanging from
Set<String>toFlux<String>aligns with the repository's reactive pattern, enabling non-blocking database operations.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java (3)
8-8: Required import for EmailOnly projectionImport added for the new EmailOnly projection class.
13-13: Flux import for reactive streamsImport added for Reactor's Flux class to support the reactive implementation.
43-48: Good implementation of reactive database queryThe implementation correctly fetches system-generated users dynamically from the database instead of using hardcoded values. The use of projection and method reference mapping is efficient and follows reactive best practices.
app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImplTest.java (2)
28-38: Proper test setup and teardownThe setup and cleanup methods are well-implemented to ensure test isolation.
73-93: Well-structured negative test caseThis test properly verifies that only predefined system users are returned when no additional system users exist.
…sers and add corresponding test case
…move-appsmith-bot-users-from-seat-count
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.
/test Settings
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14586059630
Commit: 12091dc
Cypress dashboard.
Tags:
@tag.SettingsSpec:
Tue, 22 Apr 2025 03:49:21 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit