fix: avoiding unnecessary update page layouts in js action update #37062
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes introduce a new import for Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Integrations Pod General
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11497722454. |
sneha122
requested review from
NilanshBansal
and removed request for
nidhi-nair and
sondermanish
|
Deploy-Preview-URL: https://ce-37062.dp.appsmith.com |
nidhi-nair
requested changes
Oct 24, 2024
There was a problem hiding this comment.
@sneha122 this change needs to be accompanied by test cases.
@nidhi-nair Do you have any suggestions for the test cases? What assertions can be made? I was also thinking of test cases but thought this is simply removing the redundant calls |
Failed server tests
|
NilanshBansal
previously approved these changes
Oct 24, 2024
There was a problem hiding this comment.
LGTM under the assumption that existing tests does cover this change.
@sneha122 please check above comment from Nidhi and move forward only post resolution of that.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (3 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (2)
138-140: LGTM: Mock bean addition is well-structured.The UpdateLayoutService mock bean is properly annotated and follows consistent naming conventions.
769-771: LGTM: Clear verification comment.The comment clearly explains the expected number of invocations, which helps in understanding the test's purpose.
Comment on lines
746
to
750
| Mockito.when(updateLayoutService.updatePageLayoutsByPageId(Mockito.anyString())) | ||
| .thenAnswer(invocationOnMock -> { | ||
| return Mono.just(testPage.getId()); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ 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())); | |
Comment on lines
708
to
775
| @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()); | ||
| }); | ||
|
|
||
| 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(); | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
Failed server tests
|
nidhi-nair
approved these changes
Oct 25, 2024
2 tasks
sneha122
added a commit
that referenced
this pull request
Nov 6, 2024
## 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” <“[email protected]”>
This was referenced Nov 15, 2024
github-actions bot
pushed a commit
to Zeral-Zhang/appsmith
that referenced
this pull request
Nov 20, 2024
…psmithorg#37062) ## Description When JS object is updated we have a JS object update API call going in (api/v1/collections/actions) This API does mainly does following things: 1. Updates all actions (Js functions) of the JS object 2. Updates action collection (Js Object) 3. Updates the page layout and on load actions The issue was with 1st part where, every time we would update the action (JS function), we were also calling update page layout (3rd part), which means if we are updating 20 actions, update page layout was getting called 20 times for each action update. This is in addition to page layout update that happens after action collection is updated. This PR fixes that issue and removes those redundant calls for updating page layout with each action update. Fixes appsmithorg#37046 _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.Sanity" ### 🔍 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/11513970737> > Commit: 95cc7e7 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11513970737&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS, @tag.Sanity` > Spec: > <hr>Fri, 25 Oct 2024 08:27:48 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** - Enhanced action processing by introducing conditional layout updates based on action type, specifically for JavaScript actions. - **Bug Fixes** - Improved efficiency by preventing unnecessary layout updates when actions are of type JavaScript. - **Tests** - Added new test coverage for layout update functionality, ensuring correct invocation of layout services during action collection updates. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“[email protected]”>
github-actions bot
pushed a commit
to Zeral-Zhang/appsmith
that referenced
this pull request
Nov 20, 2024
## Description This PR is in continuation to [PR](appsmithorg#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](appsmithorg#37129) Fixes appsmithorg#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” <“[email protected]”>
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When JS object is updated we have a JS object update API call going in (api/v1/collections/actions) This API does mainly does following things:
The issue was with 1st part where, every time we would update the action (JS function), we were also calling update page layout (3rd part), which means if we are updating 20 actions, update page layout was getting called 20 times for each action update. This is in addition to page layout update that happens after action collection is updated.
This PR fixes that issue and removes those redundant calls for updating page layout with each action update.
Fixes #37046
or
Fixes
Issue URLWarning
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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11513970737
Commit: 95cc7e7
Cypress dashboard.
Tags:
@tag.JS, @tag.SanitySpec:
Fri, 25 Oct 2024 08:27:48 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests