fix: skipped unnecessary code for js object updates (#37125)
## Description This PR is in continuation to [PR](https://github.com/appsmithorg/appsmith/pull/37062) which we had merged earlier, In previous, we skipped the redundant calls to update page layout when updating each js object action, as we already have a call for [updating the page layout for actionCollection](27bdeb92b6/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (L411)) Since we have skipped the update page layout for each js action, we no longer need the code part after this, which basically fetches page data from DB and updates the `errorReports` in actionDTO based on layout `layoutOnLoadActionErrors`. This PR skips this unnecessary part too for each js action as we do [set the errorReport for actionCollection in the end](27bdeb92b6/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (L430)) ### Will this have any impact on error messages shown to user? In order to understand this, checked out the frontend code to see if errorReports from individual js action is getting consumed on updating js object, looks like [it is not](e7e3d5e002/app/client/src/sagas/JSPaneSagas.ts (L316)), so we can safely remove this piece of code. However this points to existing bug in the code (as errorReports is not even getting consumed from actionCollection), which is, when there is cyclic dependency created for js object with a widget, we don't get any toast message. Since this is existing issue which is not caused by any of the above PR implementations, creating separate issue for it and tracking it [here](https://github.com/appsmithorg/appsmith/issues/37129) Fixes #37114 _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.JS, @tag.JS" ### 🔍 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/11698258739> > Commit: 9fbde996545451c1a12736e0b8a9252ad4ab69bd > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11698258739&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS, @tag.JS` > Spec: > <hr>Wed, 06 Nov 2024 06:50:05 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Streamlined layout update process for actions, enhancing performance and clarity. - **Bug Fixes** - Improved test reliability by monitoring interactions with the layout service and handling cyclic dependencies. - **Documentation** - Updated comments to clarify the logic behind layout updates for JS actions. - **Tests** - Enhanced test descriptions and assertions for better clarity and validation of method interactions, including cyclic dependency scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“sneha@appsmith.com”>
This commit is contained in:
sneha122
GitHub
parent
3650796d3c
commit
756dc5421e
|
|
@ -249,36 +249,40 @@ public class LayoutActionServiceCEImpl implements LayoutActionServiceCE {
|
|||
String pageId = newAction.getUnpublishedAction().getPageId();
|
||||
action.setApplicationId(null);
|
||||
action.setPageId(null);
|
||||
return updateSingleAction(newAction.getId(), action)
|
||||
.name(UPDATE_SINGLE_ACTION)
|
||||
.tap(Micrometer.observation(observationRegistry))
|
||||
.flatMap(updatedAction -> {
|
||||
// Update page layout is skipped for JS actions here because when JSobject is updated, we first
|
||||
// update all actions, action
|
||||
// collection and then we update the page layout, hence updating page layout with each action update
|
||||
// is not required here
|
||||
if (action.getPluginType() != PluginType.JS) {
|
||||
return updateLayoutService
|
||||
.updatePageLayoutsByPageId(pageId)
|
||||
.name(UPDATE_PAGE_LAYOUT_BY_PAGE_ID)
|
||||
.tap(Micrometer.observation(observationRegistry))
|
||||
.thenReturn(updatedAction);
|
||||
}
|
||||
return Mono.just(updatedAction);
|
||||
})
|
||||
.zipWhen(actionDTO -> newPageService.findPageById(pageId, pagePermission.getEditPermission(), false))
|
||||
.map(tuple2 -> {
|
||||
ActionDTO actionDTO = tuple2.getT1();
|
||||
PageDTO pageDTO = tuple2.getT2();
|
||||
// redundant check
|
||||
if (pageDTO.getLayouts().size() > 0) {
|
||||
actionDTO.setErrorReports(pageDTO.getLayouts().get(0).getLayoutOnLoadActionErrors());
|
||||
}
|
||||
log.debug(
|
||||
"Update action based on context type completed, returning actionDTO with action id: {}",
|
||||
actionDTO != null ? actionDTO.getId() : null);
|
||||
return actionDTO;
|
||||
});
|
||||
|
||||
// Update page layout is skipped for JS actions here because when JSobject is updated, we first
|
||||
// update all actions, action
|
||||
// collection and then we update the page layout, hence updating page layout with each action update
|
||||
// is not required here
|
||||
if (action.getPluginType() == PluginType.JS) {
|
||||
return updateSingleAction(newAction.getId(), action)
|
||||
.name(UPDATE_SINGLE_ACTION)
|
||||
.tap(Micrometer.observation(observationRegistry));
|
||||
} else {
|
||||
return updateSingleAction(newAction.getId(), action)
|
||||
.name(UPDATE_SINGLE_ACTION)
|
||||
.tap(Micrometer.observation(observationRegistry))
|
||||
.flatMap(updatedAction -> updateLayoutService
|
||||
.updatePageLayoutsByPageId(pageId)
|
||||
.name(UPDATE_PAGE_LAYOUT_BY_PAGE_ID)
|
||||
.tap(Micrometer.observation(observationRegistry))
|
||||
.thenReturn(updatedAction))
|
||||
.zipWhen(
|
||||
actionDTO -> newPageService.findPageById(pageId, pagePermission.getEditPermission(), false))
|
||||
.map(tuple2 -> {
|
||||
ActionDTO actionDTO = tuple2.getT1();
|
||||
PageDTO pageDTO = tuple2.getT2();
|
||||
// redundant check
|
||||
if (pageDTO.getLayouts().size() > 0) {
|
||||
actionDTO.setErrorReports(
|
||||
pageDTO.getLayouts().get(0).getLayoutOnLoadActionErrors());
|
||||
}
|
||||
log.debug(
|
||||
"Update action based on context type completed, returning actionDTO with action id: {}",
|
||||
actionDTO != null ? actionDTO.getId() : null);
|
||||
return actionDTO;
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@Override
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@ import com.appsmith.server.dtos.PageDTO;
|
|||
import com.appsmith.server.dtos.PluginWorkspaceDTO;
|
||||
import com.appsmith.server.dtos.RefactorEntityNameDTO;
|
||||
import com.appsmith.server.dtos.WorkspacePluginStatus;
|
||||
import com.appsmith.server.exceptions.AppsmithError;
|
||||
import com.appsmith.server.helpers.MockPluginExecutor;
|
||||
import com.appsmith.server.helpers.PluginExecutorHelper;
|
||||
import com.appsmith.server.layouts.UpdateLayoutService;
|
||||
|
|
@ -73,6 +74,7 @@ import static com.appsmith.server.constants.FieldName.DEVELOPER;
|
|||
import static com.appsmith.server.constants.FieldName.VIEWER;
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
import static org.junit.jupiter.api.Assertions.assertNull;
|
||||
|
||||
@SpringBootTest
|
||||
@Slf4j
|
||||
|
|
@ -708,7 +710,8 @@ public class ActionCollectionServiceTest {
|
|||
|
||||
@Test
|
||||
@WithUserDetails(value = "api_user")
|
||||
public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageLayoutOnlyOnce() {
|
||||
public void
|
||||
testUpdateUnpublishedActionCollection_withValidCollection_callsPageLayoutOnlyOnceAndAssertCyclicDependencyError() {
|
||||
Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(pluginExecutor));
|
||||
Mockito.when(pluginExecutor.getHintMessages(Mockito.any(), Mockito.any()))
|
||||
.thenReturn(Mono.zip(Mono.just(new HashSet<>()), Mono.just(new HashSet<>())));
|
||||
|
|
@ -727,7 +730,7 @@ public class ActionCollectionServiceTest {
|
|||
ActionDTO action1 = new ActionDTO();
|
||||
action1.setName("testAction1");
|
||||
action1.setActionConfiguration(new ActionConfiguration());
|
||||
action1.getActionConfiguration().setBody("mockBody");
|
||||
action1.getActionConfiguration().setBody("initial body");
|
||||
action1.getActionConfiguration().setIsValid(false);
|
||||
|
||||
ActionDTO action2 = new ActionDTO();
|
||||
|
|
@ -744,15 +747,35 @@ public class ActionCollectionServiceTest {
|
|||
|
||||
actionCollectionDTO.setActions(List.of(action1, action2, action3));
|
||||
|
||||
Layout layout = testPage.getLayouts().get(0);
|
||||
ArrayList dslList = (ArrayList) layout.getDsl().get("children");
|
||||
JSONObject tableDsl = (JSONObject) dslList.get(0);
|
||||
tableDsl.put("tableData", "{{testCollection1.testAction1.data}}");
|
||||
JSONArray temp2 = new JSONArray();
|
||||
temp2.add(new JSONObject(Map.of("key", "tableData")));
|
||||
tableDsl.put("dynamicBindingPathList", temp2);
|
||||
JSONArray temp3 = new JSONArray();
|
||||
temp3.add(new JSONObject(Map.of("key", "tableData")));
|
||||
tableDsl.put("dynamicPropertyPathList", temp3);
|
||||
layout.getDsl().put("widgetName", "MainContainer");
|
||||
|
||||
testPage.setLayouts(List.of(layout));
|
||||
PageDTO updatedPage =
|
||||
newPageService.updatePage(testPage.getId(), testPage).block();
|
||||
|
||||
// Create Js object
|
||||
ActionCollectionDTO createdActionCollectionDTO =
|
||||
layoutCollectionService.createCollection(actionCollectionDTO).block();
|
||||
assert createdActionCollectionDTO != null;
|
||||
assert createdActionCollectionDTO.getId() != null;
|
||||
String createdActionCollectionId = createdActionCollectionDTO.getId();
|
||||
|
||||
applicationPageService.publish(testApp.getId(), true).block();
|
||||
|
||||
actionCollectionDTO.getActions().get(0).getActionConfiguration().setBody("updatedBody");
|
||||
// Update JS object to create cyclic dependency
|
||||
actionCollectionDTO.getActions().stream()
|
||||
.filter(action -> "testAction1".equals(action.getName()))
|
||||
.findFirst()
|
||||
.ifPresent(action ->
|
||||
action.getActionConfiguration().setBody("function () {\n return Table1.tableData;\n}"));
|
||||
|
||||
final Mono<ActionCollectionDTO> updatedActionCollectionDTO =
|
||||
layoutCollectionService.updateUnpublishedActionCollection(
|
||||
|
|
@ -766,6 +789,18 @@ public class ActionCollectionServiceTest {
|
|||
// collection as expected
|
||||
Mockito.verify(updateLayoutService, Mockito.times(2))
|
||||
.updatePageLayoutsByPageId(Mockito.anyString());
|
||||
|
||||
assertEquals(1, actionCollectionDTO1.getErrorReports().size());
|
||||
assertEquals(
|
||||
AppsmithError.CYCLICAL_DEPENDENCY_ERROR.getAppErrorCode(),
|
||||
actionCollectionDTO1.getErrorReports().get(0).getCode());
|
||||
|
||||
// Iterate over each action and assert that errorReports is null as action collection already has
|
||||
// error reports
|
||||
// it's not required in each action
|
||||
actionCollectionDTO
|
||||
.getActions()
|
||||
.forEach(action -> assertNull(action.getErrorReports(), "Error reports should be null"));
|
||||
})
|
||||
.verifyComplete();
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user