Skip to content

chore: Disable default fallback when the form login enabled is null for org config #40304

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 17, 2025
Merged

chore: Disable default fallback when the form login enabled is null for org config #40304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...mith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public void copyNonSensitiveValues(OrganizationConfiguration organizationConfigu

googleMapsKey = ObjectUtils.defaultIfNull(organizationConfiguration.getGoogleMapsKey(), googleMapsKey);
isFormLoginEnabled =
getComputedValue(true, organizationConfiguration.getIsFormLoginEnabled(), isFormLoginEnabled);
ObjectUtils.defaultIfNull(organizationConfiguration.getIsFormLoginEnabled(), isFormLoginEnabled);
Comment on lines 83 to +84
Copy link
Contributor

@coderabbitai coderabbitai bot Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

💡 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 java

Length 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 getComputedValue method declaration, as it has no other usages.

Affected file:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to come back to this again as we are seeing some unintended updates because of the fallback.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

(_/）
(•ㅅ•)
/ づ~

isSignupDisabled = ObjectUtils.defaultIfNull(organizationConfiguration.getIsSignupDisabled(), isSignupDisabled);
instanceName = ObjectUtils.defaultIfNull(organizationConfiguration.getInstanceName(), instanceName);
emailVerificationEnabled = ObjectUtils.defaultIfNull(
Expand Down
3 changes: 3 additions & 0 deletions ...smith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -172,8 +173,10 @@ void setMapsKeyWithoutAuthorization() {
.verify();
}

// TODO @Abhijeet: Fix the side-effect of having default fallback when form login is null and enable the test
@Test
@WithUserDetails("api_user")
@Disabled
void updateOrganizationConfiguration_updateFormLoginEnabled_success() {

// Ensure that default value for form login is enabled
Expand Down