chore: refactor theme import flow to support dry ops for pg #34733
Conversation
WalkthroughThe recent updates enhance theme and application handling in the Appsmith server repository. They introduce new methods for saving, archiving, and updating themes and applications, ensuring better management and dry run operations for these entities. Additionally, imports and modifications in the test services align with these changes, ensuring comprehensive testing and integration. Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java (5 hunks)
Additional comments not posted (13)
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java (2)
5-7: New imports added forApplicationandThemeThe imports for
ApplicationandThemeare necessary for the new attributes in the class.
56-58: New attributes added for dry run queriesThe new attributes
themeDryRunQueriesandapplicationDryRunQueriesare maps that store lists ofThemeandApplicationobjects, respectively. These attributes will help manage dry run operations for themes and applications.app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCE.java (1)
64-65: New method signature added forgetOrSaveThemeThe new method signature
getOrSaveThemeincludes an additionalboolean isDryOpsparameter to handle dry run operations. This change is consistent with the PR objective to support dry operations.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java (4)
5-8: New imports added forAclPermission,Application,Theme, andDBOpsTypeThe imports for
AclPermission,Application,Theme, andDBOpsTypeare necessary for the new methods and modifications in the class.
30-35: New dependencies addedThe new dependencies
ThemeRepository,ThemeService, andApplicationRepositoryare necessary for managing themes and applications in the new methods.
59-74: New methods for managing themes and applicationsThe new methods
saveThemeToDb,archiveTheme,updateTheme, andupdateApplicationhandle theme and application operations. These methods are crucial for the new dry run operations.
97-134: Modifications toexecuteAllDbOpsmethodThe
executeAllDbOpsmethod has been enhanced to support theme and application operations. The new logic handles saving, updating, and archiving themes and applications during dry run operations.app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java (4)
Line range hint
9-22:
New imports added forDBOpsType,ArrayList,List, andMapThe imports for
DBOpsType,ArrayList,List, andMapare necessary for the new methods and modifications in the class.
Line range hint
77-103:
Modifications toimportEntitiesmethodThe
importEntitiesmethod has been modified to handle theme updates and dry run operations. This change is consistent with the PR objective to support dry operations.
110-160: New logic for updating themes from JSONThe new logic in the
updateExistingAppThemeFromJSONmethod handles dry run operations for themes. This includes saving, updating, and deleting themes based on the dry run operations.
182-189: New methods for managing dry run operationsThe new methods
addDryOpsForEntityandaddDryOpsForApplicationmanage dry run operations for themes and applications. These methods are crucial for the new dry run operations.app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.java (2)
436-437: Ensure correct usage of the new method.The existing method
getOrSaveThemenow calls the new method withisDryOpsset to false. This change appears correct.
Line range hint
439-461:
Verify the handling of theisDryOpsparameter.The new implementation correctly handles the
isDryOpsparameter by updating the theme for bulk write operations without saving it to the repository whenisDryOpsis true. This change appears correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java
Failed server tests
|
…o pg-transaction-theme-import-flow
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java (5 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (5 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ThemeImportableServiceCETest.java (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java
Additional comments not posted (13)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ThemeImportableServiceCETest.java (8)
32-32: Ensure the usage of@DirtiesContextis necessary.The
@DirtiesContextannotation is typically used to indicate that the context should be reset after a test method. Ensure that this is necessary for the test cases in this file, as it can slow down test execution.
43-43: Verify the necessity of@DirtiesContextat the class level.Using
@DirtiesContextat the class level will cause the Spring context to be reset after each test method, which can significantly increase test execution time. Ensure this is required for the tests in this class.
172-174: Initialization of new DTO variables.The new variables
MappedImportableResourcesDTOandImportingMetaDTOare initialized but not used until later in the method. Ensure that their initialization is necessary at this point.
175-185: Updated method logic for importing themes.The method now uses
MappedImportableResourcesDTOandImportingMetaDTOto handle the import process. Ensure that the logic correctly manages theme dry run queries and that thethenReturnstatement accurately reflects the intended behavior.
186-196: Assertions for theme dry run queries.The assertions check that two themes are saved and that their IDs are different. Ensure that these assertions accurately reflect the expected behavior and that the themes are correctly processed during the import.
212-214: Initialization of new DTO variables.The new variables
MappedImportableResourcesDTOandImportingMetaDTOare initialized but not used until later in the method. Ensure that their initialization is necessary at this point.
Line range hint
220-239:
Updated method logic for importing themes.The method now uses
MappedImportableResourcesDTOandImportingMetaDTOto handle the import process. Ensure that the logic correctly manages theme dry run queries and that thethenReturnstatement accurately reflects the intended behavior.
241-246: Assertions for theme dry run queries.The assertions check that the default theme is correctly set for both edit mode and published mode. Ensure that these assertions accurately reflect the expected behavior and that the themes are correctly processed during the import.
app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (5)
978-980: Verify tuple element access for application details.Ensure that accessing the application details directly from
tuple.getT7()is correct and that it aligns with the expected structure of the tuple.
1135-1137: Verify tuple element access for application details.Ensure that accessing the application details directly from
importDTO.getApplication()is correct and that it aligns with the expected structure of the tuple.
1293-1297: Verify tuple element access for application details.Ensure that accessing the application details directly from
applicationImportDTO.getApplication().getId()is correct and that it aligns with the expected structure of the tuple.
1515-1517: Verify tuple element access for application details.Ensure that accessing the application details directly from
applicationImportDTO.getApplication().getId()is correct and that it aligns with the expected structure of the tuple.
5272-5274: Verify tuple element access for application details.Ensure that accessing the application details directly from
applicationImportDTO.getApplication().getId()is correct and that it aligns with the expected structure of the tuple.
…o pg-transaction-theme-import-flow
Failed server tests
|
1 similar comment
Failed server tests
|
AnaghHegde
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java
There was a problem hiding this comment.
@AnaghHegde now that we have encountered the update and save ops, how are we making sure we are making these DB ops in chronological order? Even better can we find the final object that comes out of the service layer and stick to save DB call?
...r/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
Outdated
Show resolved
Hide resolved
...r/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
Outdated
Show resolved
Hide resolved
|
|
||
| Map<String, List<Theme>> themeDryRunQueries = new HashMap<>(); | ||
|
|
||
| Map<String, List<Application>> applicationDryRunQueries = new HashMap<>(); |
There was a problem hiding this comment.
Are we expecting the value as List<Application> in any of the flows?
There was a problem hiding this comment.
@abhvsn At this movement its just only one update, but once i move to refactoring the entities like page, application we will have multiple updates of application object.
There was a problem hiding this comment.
Yeah that's where this comment coming from. Considering this will be the same object instead of creating multiple entries can we just keep updating the existing application object. That way # of db ops will be reduced and also we will not face any issue around data loss because of overwriting.
There was a problem hiding this comment.
That is a good point but i am worried that due to parallel executions we might miss some updates. Hence i decided to keep this as list. And when we use bulk update we will not face much issues.
There was a problem hiding this comment.
@AnaghHegde after todays update are we aligned on keeping this as a single application instead of list of application?
There was a problem hiding this comment.
@abhvsn Yes correct, this will be updated but i will do this in page import PR. The current flow has only instance of application in the list.
...r/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
Outdated
Show resolved
Hide resolved
...server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java (4 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (5 hunks)
Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java
Failed server tests
|
AnaghHegde
added
ok-to-test
...r/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
Show resolved
Hide resolved
...r/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
Outdated
Show resolved
Hide resolved
...r/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java
Show resolved
Hide resolved
…o pg-transaction-theme-import-flow
Failed server tests
|
1 similar comment
Failed server tests
|
AnaghHegde
added
ok-to-test
Description
Please refer this document for more details - https://www.notion.so/appsmith/Transaction-Handling-in-PG-468cf8d4255749c3915699e59e91dc2f
Automation
/ok-to-test tags="@tag.Git, @tag.ImportExport, @tag.Templates"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10053864056
Commit: 0b83e89
Cypress dashboard.
Tags:
@tag.Git, @tag.ImportExport, @tag.TemplatesSpec:
Tue, 23 Jul 2024 06:45:31 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
These changes improve the robustness and flexibility of application and theme management during imports.