fix: skipped unnecessary code for js object updates (#37125)

## Description

This PR is in continuation to
[PR](#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](https://github.com/appsmithorg/appsmith/blob/27bdeb92b6a0b27e6afcbfe80a5cb0705c0812ac/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](https://github.com/appsmithorg/appsmith/blob/27bdeb92b6a0b27e6afcbfe80a5cb0705c0812ac/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](https://github.com/appsmithorg/appsmith/blob/e7e3d5e00290919c1df0767fdefed67458ec3cc9/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](#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: 9fbde99
> <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”>

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java

@@ -249,36 +249,40 @@ protected Mono<ActionDTO> updateActionBasedOnContextType(NewAction newAction, Ac
         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

app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java

@@ -26,6 +26,7 @@
 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.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 void testDeleteActionCollection_afterApplicationPublish_clearsActionColle
 
     @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 void testUpdateUnpublishedActionCollection_withValidCollection_callsPageL
         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 void testUpdateUnpublishedActionCollection_withValidCollection_callsPageL
 
         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 void testUpdateUnpublishedActionCollection_withValidCollection_callsPageL
                     // 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();
     }