Skip to content

fix: avoiding unnecessary update page layouts in js action update #37062

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
sneha122 merged 6 commits into from
Oct 25, 2024
Merged

fix: avoiding unnecessary update page layouts in js action update #37062

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7769933
fix: avoiding unnecessary update page layouts in js action update
Oct 24, 2024
08baaa7
Merge branch 'release' into fix/remove-unnecessary-update-page-layout…
Oct 24, 2024
6f27a7b
fix: updated comment
Oct 24, 2024
1a91165
mono execution issue fixed
Oct 24, 2024
3b5e6c7
fix: added test cases
Oct 25, 2024
95cc7e7
fix: failing tests issue fixed
Oct 25, 2024
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
20 changes: 15 additions & 5 deletions ...smith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.appsmith.external.models.ActionDTO;
import com.appsmith.external.models.CreatorContextType;
import com.appsmith.external.models.Datasource;
import com.appsmith.external.models.PluginType;
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.datasources.base.DatasourceService;
Expand Down Expand Up @@ -246,11 +247,20 @@ protected Mono<ActionDTO> updateActionBasedOnContextType(NewAction newAction, Ac
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))
.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();
Expand Down
73 changes: 73 additions & 0 deletions ...psmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.appsmith.server.dtos.WorkspacePluginStatus;
import com.appsmith.server.helpers.MockPluginExecutor;
import com.appsmith.server.helpers.PluginExecutorHelper;
import com.appsmith.server.layouts.UpdateLayoutService;
import com.appsmith.server.newactions.base.NewActionService;
import com.appsmith.server.newpages.base.NewPageService;
import com.appsmith.server.plugins.base.PluginService;
Expand Down Expand Up @@ -134,6 +135,9 @@ public class ActionCollectionServiceTest {
@MockBean
PluginExecutor pluginExecutor;

@MockBean
UpdateLayoutService updateLayoutService;

Application testApp = null;

PageDTO testPage = null;
Expand Down Expand Up @@ -700,4 +704,73 @@ public void testDeleteActionCollection_afterApplicationPublish_clearsActionColle
})
.verifyComplete();
}

@Test
@WithUserDetails(value = "api_user")
public void testUpdateUnpublishedActionCollection_withValidCollection_callsPageLayoutOnlyOnce() {
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<>())));

ActionCollectionDTO actionCollectionDTO = new ActionCollectionDTO();
actionCollectionDTO.setName("testCollection1");
actionCollectionDTO.setPageId(testPage.getId());
actionCollectionDTO.setApplicationId(testApp.getId());
actionCollectionDTO.setWorkspaceId(workspaceId);
actionCollectionDTO.setPluginId(datasource.getPluginId());
actionCollectionDTO.setVariables(List.of(new JSValue("test", "String", "test", true)));
actionCollectionDTO.setBody("collectionBody");
actionCollectionDTO.setPluginType(PluginType.JS);

// Create actions
ActionDTO action1 = new ActionDTO();
action1.setName("testAction1");
action1.setActionConfiguration(new ActionConfiguration());
action1.getActionConfiguration().setBody("mockBody");
action1.getActionConfiguration().setIsValid(false);

ActionDTO action2 = new ActionDTO();
action2.setName("testAction2");
action2.setActionConfiguration(new ActionConfiguration());
action2.getActionConfiguration().setBody("mockBody");
action2.getActionConfiguration().setIsValid(false);

ActionDTO action3 = new ActionDTO();
action3.setName("testAction3");
action3.setActionConfiguration(new ActionConfiguration());
action3.getActionConfiguration().setBody("mockBody");
action3.getActionConfiguration().setIsValid(false);

actionCollectionDTO.setActions(List.of(action1, action2, action3));

Mockito.when(updateLayoutService.updatePageLayoutsByPageId(Mockito.anyString()))
.thenAnswer(invocationOnMock -> {
return Mono.just(testPage.getId());
});

@coderabbitai coderabbitai Bot Oct 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using Mockito's answer method for cleaner mock setup.

The mock setup can be simplified using Mockito's answer method.

- Mockito.when(updateLayoutService.updatePageLayoutsByPageId(Mockito.anyString()))
-         .thenAnswer(invocationOnMock -> {
-             return Mono.just(testPage.getId());
-         });
+ Mockito.when(updateLayoutService.updatePageLayoutsByPageId(Mockito.anyString()))
+         .thenReturn(Mono.just(testPage.getId()));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Mockito.when(updateLayoutService.updatePageLayoutsByPageId(Mockito.anyString()))
.thenAnswer(invocationOnMock -> {
return Mono.just(testPage.getId());
});
Mockito.when(updateLayoutService.updatePageLayoutsByPageId(Mockito.anyString()))
.thenReturn(Mono.just(testPage.getId()));

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");

final Mono<ActionCollectionDTO> updatedActionCollectionDTO =
layoutCollectionService.updateUnpublishedActionCollection(
createdActionCollectionId, actionCollectionDTO);

StepVerifier.create(updatedActionCollectionDTO)
.assertNext(actionCollectionDTO1 -> {
assertEquals(createdActionCollectionId, actionCollectionDTO1.getId());

// This invocation will happen here twice, once during create collection and once during update
// collection as expected
Mockito.verify(updateLayoutService, Mockito.times(2))
.updatePageLayoutsByPageId(Mockito.anyString());
})
.verifyComplete();
}
Comment on lines +708 to +771

@coderabbitai coderabbitai Bot Oct 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider extracting test setup to improve readability.

The test is well-structured but contains repetitive action setup code. Consider extracting the action creation logic into a helper method.

+ private ActionDTO createTestAction(String name, String body) {
+     ActionDTO action = new ActionDTO();
+     action.setName(name);
+     action.setActionConfiguration(new ActionConfiguration());
+     action.getActionConfiguration().setBody(body);
+     action.getActionConfiguration().setIsValid(false);
+     return action;
+ }

Then use it in the test:

- ActionDTO action1 = new ActionDTO();
- action1.setName("testAction1");
- action1.setActionConfiguration(new ActionConfiguration());
- action1.getActionConfiguration().setBody("mockBody");
- action1.getActionConfiguration().setIsValid(false);
+ ActionDTO action1 = createTestAction("testAction1", "mockBody");

Committable suggestion was skipped due to low confidence.

}