chore: Disable default fallback when the form login enabled is null for org config #40304
Conversation
WalkthroughThis change updates the logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant OrganizationConfigurationCE
Caller->>OrganizationConfigurationCE: copyNonSensitiveValues(updatedConfig)
OrganizationConfigurationCE->>OrganizationConfigurationCE: isFormLoginEnabled = defaultIfNull(updatedConfig.isFormLoginEnabled, this.isFormLoginEnabled)
Note right of OrganizationConfigurationCE: No custom helper logic, only defaultIfNull
OrganizationConfigurationCE-->>Caller: updated OrganizationConfigurationCE
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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: 1
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java (1)
96-101: Consider removing unused helper methodThe
getComputedValuemethod is no longer used forisFormLoginEnabledand might be unused elsewhere. If this method is no longer needed, consider removing it to reduce maintenance overhead.#!/bin/bash # Check if the getComputedValue method is still being used in the codebase echo "Searching for usages of getComputedValue method in the codebase:" rg "getComputedValue\(" --type java
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java (1)
app/client/src/ce/selectors/organizationSelectors.tsx (1)
getIsFormLoginEnabled(48-49)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java (2)
176-179: Appropriate disabling of test due to behavior changeGood call on disabling the test with a clear TODO comment explaining why. The behavior change in
copyNonSensitiveValues(removing default fallback totrue) causes side effects that need to be addressed.In the future, update the test to account for the new behavior without relying on the removed fallback mechanism.
182-189: Test assumes form login is enabled by defaultThis part of the test assumes form login is enabled by default. With the behavior change in
copyNonSensitiveValues, this assumption may no longer be valid when using a fresh organization configuration.When fixing the test, verify if this assertion should be modified to check for
nullor if the test setup should explicitly enable form login.
| isFormLoginEnabled = | ||
| getComputedValue(true, organizationConfiguration.getIsFormLoginEnabled(), isFormLoginEnabled); | ||
| ObjectUtils.defaultIfNull(organizationConfiguration.getIsFormLoginEnabled(), isFormLoginEnabled); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Removed default fallback behavior for form login
The change removes the usage of the custom getComputedValue helper method which would default to true when both current and updated values were null. Now using ObjectUtils.defaultIfNull directly, which simply returns the current value if the updated value is null, without providing a fallback value.
This change aligns with the PR objective but affects the behavior when both values are null.
🏁 Script executed:
#!/bin/bash
# Check where the getComputedValue method is still being used in the codebase
echo "Searching for usages of getComputedValue method in the codebase:"
rg "getComputedValue\(" --type javaLength of output: 375
Review: Updated Fallback Logic for Form Login
The change replaces the custom getComputedValue helper with ObjectUtils.defaultIfNull, which means when both organizationConfiguration.getIsFormLoginEnabled() and the current isFormLoginEnabled are null, it will now return the existing isFormLoginEnabled (rather than defaulting to true). Please:
- Confirm that this revised fallback behavior matches the intended requirements.
- Remove the now-unused
getComputedValuemethod declaration, as it has no other usages.
Affected file:
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java
There was a problem hiding this comment.
I'm planning to come back to this again as we are seeing some unintended updates because of the fallback.
There was a problem hiding this comment.
@abhvsn Understood. Changing fallback behavior can sometimes lead to unexpected side effects in different parts of the system. When you're ready to revisit this issue, feel free to tag me, and I'd be happy to help analyze the unintended updates you're seeing.
(_/）
(•ㅅ•)
/ づ~
abhvsn
changed the title
Failed server tests
|
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/14514783524
Commit: 86104a1
Cypress dashboard.
Tags:
@tag.SettingsSpec:
Thu, 17 Apr 2025 11:55:36 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit