Skip to content
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.

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

fix: added an autocommit migration for theme settings. #35554

Merged
merged 3 commits into from
Aug 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,10 @@ private ApplicationJson migrateServerSchema(ApplicationJson applicationJson) {
case 6:
MigrationHelperMethods.ensureXmlParserPresenceInCustomJsLibList(applicationJson);
applicationJson.setServerSchemaVersion(7);
case 7:
case 8:
MigrationHelperMethods.migrateThemeSettingsForAnvil(applicationJson);
applicationJson.setServerSchemaVersion(9);
default:
// Unable to detect the serverSchema
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.appsmith.server.migrations;

import com.appsmith.external.annotations.FeatureFlagged;
import com.appsmith.external.enums.FeatureFlagEnum;
import org.springframework.stereotype.Component;

/**
Expand All @@ -20,8 +18,7 @@ public class JsonSchemaVersions extends JsonSchemaVersionsFallback {
* @return an Integer which is server version
*/
@Override
@FeatureFlagged(featureFlagName = FeatureFlagEnum.release_git_autocommit_feature_enabled)
public Integer getServerVersion() {
return super.getServerVersion() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will you be cleaning this up later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that is when we remove a secondary class with autocommit flags.

return super.getServerVersion();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

@Component
public class JsonSchemaVersionsFallback {
private static final Integer serverVersion = 7;
private static final Integer serverVersion = 9;
public static final Integer clientVersion = 1;

public Integer getServerVersion() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import com.appsmith.external.models.Property;
import com.appsmith.server.constants.ApplicationConstants;
import com.appsmith.server.constants.ResourceModes;
import com.appsmith.server.domains.Application;
import com.appsmith.server.domains.ApplicationDetail;
import com.appsmith.server.domains.ApplicationPage;
import com.appsmith.server.domains.CustomJSLib;
import com.appsmith.server.domains.NewAction;
Expand Down Expand Up @@ -1181,4 +1183,52 @@ private static Set<String> getInvalidDynamicBindingPathsInAction(
}
return pathsToRemove;
}

public static void migrateThemeSettingsForAnvil(ApplicationJson applicationJson) {
if (applicationJson == null || applicationJson.getExportedApplication() == null) {
return;
}

Application exportedApplication = applicationJson.getExportedApplication();
ApplicationDetail applicationDetail = exportedApplication.getApplicationDetail();
ApplicationDetail unpublishedApplicationDetail = exportedApplication.getUnpublishedApplicationDetail();

if (applicationDetail == null) {
applicationDetail = new ApplicationDetail();
exportedApplication.setApplicationDetail(applicationDetail);
}

if (unpublishedApplicationDetail == null) {
unpublishedApplicationDetail = new ApplicationDetail();
exportedApplication.setUnpublishedApplicationDetail(unpublishedApplicationDetail);
}

Application.ThemeSetting themeSetting = applicationDetail.getThemeSetting();
Application.ThemeSetting unpublishedThemeSetting = unpublishedApplicationDetail.getThemeSetting();
if (themeSetting == null) {
themeSetting = new Application.ThemeSetting();
}

if (unpublishedThemeSetting == null) {
unpublishedThemeSetting = new Application.ThemeSetting();
}

applicationDetail.setThemeSetting(themeSetting);
unpublishedApplicationDetail.setThemeSetting(unpublishedThemeSetting);
}

public static void setThemeSettings(Application.ThemeSetting themeSetting) {
if (themeSetting.getAppMaxWidth() == null) {
themeSetting.setAppMaxWidth(Application.ThemeSetting.AppMaxWidth.LARGE);
}

// since these are primitive values we don't have concept of null, hence putting it to the default of 1.
if (themeSetting.getDensity() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When would we arrive at this scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not expected to land on this scenario by default, Even if we don't have the density and sizing values not present while deserialising we would start with 1, right? (if not then it's for that).

themeSetting.setDensity(1);
}

if (themeSetting.getSizing() == 0) {
themeSetting.setSizing(1);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2505,8 +2505,6 @@ public void discardChange_addNavigationAndThemeSettingAfterImport_addedNavigatio
StepVerifier.create(resultMonoWithDiscardOperation)
.assertNext(application -> {
assertThat(application.getWorkspaceId()).isNotNull();
assertThat(application.getUnpublishedApplicationDetail()).isNull();
assertThat(application.getPublishedApplicationDetail()).isNull();
})
.verifyComplete();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,6 @@ public void migrateArtifactToLatestSchema_whenFeatureFlagIsOn_returnsIncremented
gitFileSystemTestHelper.getApplicationJson(this.getClass().getResource("application.json"));

ArtifactExchangeJson artifactExchangeJson = jsonSchemaMigration.migrateArtifactToLatestSchema(applicationJson);

assertThat(artifactExchangeJson.getServerSchemaVersion())
.isNotEqualTo(jsonSchemaVersionsFallback.getServerVersion());
assertThat(artifactExchangeJson.getServerSchemaVersion()).isEqualTo(jsonSchemaVersions.getServerVersion());
assertThat(artifactExchangeJson.getClientSchemaVersion()).isEqualTo(jsonSchemaVersions.getClientVersion());
assertThat(artifactExchangeJson.getClientSchemaVersion())
Expand All @@ -103,8 +100,6 @@ public void migrateApplicationJsonToLatestSchema_whenFeatureFlagIsOn_returnsIncr
jsonSchemaMigration.migrateApplicationJsonToLatestSchema(applicationJson);
StepVerifier.create(applicationJsonMono)
.assertNext(appJson -> {
assertThat(appJson.getServerSchemaVersion())
.isNotEqualTo(jsonSchemaVersionsFallback.getServerVersion());
assertThat(appJson.getServerSchemaVersion()).isEqualTo(jsonSchemaVersions.getServerVersion());
assertThat(appJson.getClientSchemaVersion()).isEqualTo(jsonSchemaVersions.getClientVersion());
assertThat(appJson.getClientSchemaVersion())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@ public void getServerVersion_whenFeatureFlagIsOn_returnsIncremented() {
Mockito.when(featureFlagService.getCachedTenantFeatureFlags())
.thenAnswer((Answer<CachedFeatures>) invocations -> cachedFeatures);

assertThat(jsonSchemaVersions.getServerVersion()).isNotEqualTo(jsonSchemaVersionsFallback.getServerVersion());
assertThat(jsonSchemaVersions.getServerVersion()).isEqualTo(jsonSchemaVersionsFallback.getServerVersion() + 1);

assertThat(jsonSchemaVersions.getServerVersion()).isEqualTo(jsonSchemaVersionsFallback.getServerVersion());
assertThat(jsonSchemaVersions.getClientVersion()).isEqualTo(jsonSchemaVersionsFallback.getClientVersion());
}
}
Loading