feat: clean up datasource review page#34348
Conversation
WalkthroughThe updates primarily involve the removal of the functionality for generating CRUD pages from various components related to data source management in the app, including changes in test cases, hooks, and UI elements. This simplifies the data source review processes by eliminating the "Generate Page/CRUD" feature. Changes
Assessment against linked issues
Poem
Warning Review ran into problemsProblems (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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
Activation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (6)
app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.tsx (1)
Line range hint
76-76: Consider using optional chaining here to make the code more robust and prevent potential runtime errors.- datasourceStructure && datasourceStructure.tables && datasourceStructure.tables.length > 0 + datasourceStructure?.tables?.length > 0app/client/src/pages/Editor/DataSourceEditor/DSFormHeader.tsx (1)
Line range hint
74-74: ThePluginImagecomponent is missing analtattribute. Adding an alt attribute improves accessibility by providing a text alternative for screen readers.- <img {...props} /> + <img alt="Plugin" {...props} />Tools
Biome
[error] 225-225: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_2_spec.ts (1)
Line range hint
49-59: Convert these traditional function expressions to arrow functions to improve consistency and leverage lexical scoping wherethisis not required.- function () { + () => {Also applies to: 61-178, 249-304, 25-385
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts (1)
Line range hint
30-35: Consider refactoring this function expression to use an arrow function for better readability and to align with ES6+ standards.- beforeEach(function () { + beforeEach(() => {app/client/cypress/support/Pages/DataSources.ts (2)
Line range hint
1197-1197: Avoid redundant double-negation in boolean expressions.- if (!!preDSConfigAction) { + if (preDSConfigAction) {The double negation (
!!) is unnecessary here as the value is already being coerced into a boolean by theifstatement's condition.
Line range hint
1197-1197: The use of double negation (!!) is unnecessary.- if (!!preDSConfigAction) { + if (preDSConfigAction) {This change simplifies the condition by removing the redundant double negation, making the code cleaner and easier to understand.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts (1 hunks)
- app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_2_spec.ts (1 hunks)
- app/client/cypress/support/Pages/DataSources.ts (2 hunks)
- app/client/src/ce/hooks/datasourceEditorHooks.tsx (3 hunks)
- app/client/src/pages/Editor/DataSourceEditor/DSFormHeader.tsx (1 hunks)
- app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.tsx (4 hunks)
- app/client/src/pages/Editor/DatasourceInfo/GoogleSheetSchema.tsx (7 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/DatasourceInfo/GoogleSheetSchema.tsx
Additional context used
Biome
app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.tsx
[error] 76-76: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/client/src/pages/Editor/DataSourceEditor/DSFormHeader.tsx
[error] 74-74: Provide a text alternative through the alt, aria-label or aria-labelledby attribute (lint/a11y/useAltText)
Meaningful alternative text on elements helps users relying on screen readers to understand content's purpose within a page.
[error] 225-225: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_2_spec.ts
[error] 49-59: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 61-178: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 249-304: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 25-385: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts
[error] 30-35: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 741-760: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.app/client/cypress/support/Pages/DataSources.ts
[error] 1197-1197: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
Additional comments not posted (11)
app/client/src/ce/hooks/datasourceEditorHooks.tsx (6)
6-8: Reordered and added imports related to feature flags and permissions.
11-13: Added new utility imports for permissions. Ensure that these utilities are used appropriately in the hook logic.
29-29: Introduced a new interfaceHeaderActionPropsto define the properties expected by theuseHeaderActionshook.
Line range hint
44-44: The use of theuseFeatureFlaghook to determine feature availability is a good practice. It helps in making the feature management more dynamic and manageable.
Line range hint
78-78: TherouteToGeneratePagefunction encapsulates navigation logic, which is a good separation of concerns. It useshistory.pushwhich is a standard approach for programmatic navigation in React applications.
59-59: The conditional rendering logic inside theuseHeaderActionshook appears sound. However, ensure that theshowGenerateButtoncondition aligns with the business requirements, as the button's visibility is contingent upon this flag.Verification successful
To verify the alignment of the
showGenerateButtoncondition with business requirements, we need to search for its usage within the TypeScript/JavaScript codebase. The previous attempt incorrectly specified the file type as Python. We'll correct this and run the script again to gather the necessary information.
The search results show that
showGenerateButtonis used within thedatasourceEditorHooks.tsxfile. To ensure that theshowGenerateButtoncondition aligns with business requirements, we need to understand howuseShowPageGenerationOnHeaderis defined and used.Let's proceed by examining the implementation and usage of
useShowPageGenerationOnHeader.
The
useShowPageGenerationOnHeaderhook is defined inapp/client/src/pages/Editor/DataSourceEditor/hooks.tsand is imported and used inapp/client/src/ce/hooks/datasourceEditorHooks.tsx. To verify the business logic alignment, we need to examine the implementation ofuseShowPageGenerationOnHeader.Let's inspect the content of
useShowPageGenerationOnHeaderinhooks.ts.
The previous attempt to retrieve the implementation of
useShowPageGenerationOnHeaderdid not produce any output. This might be due to an overly restrictive regex pattern. Let's simplify the pattern to ensure we capture the implementation correctly.
The attempts to retrieve the implementation of
useShowPageGenerationOnHeaderhave not yielded any results. This might be due to the regex pattern not matching the actual code structure or the function being defined in a different manner.To proceed, let's simplify our approach by directly searching for the function name and extracting its surrounding context to ensure we capture its implementation.
The retrieved context of the
useShowPageGenerationOnHeaderfunction shows its implementation. The function checks various conditions such as plugin type, user permissions, and feature flags to determine whether the page generation button should be shown.Based on this, it appears that the
showGenerateButtoncondition is indeed aligned with several business logic checks, including user permissions and feature flags.Key points from the implementation:
- Checks the plugin type.
- Verifies user permissions.
- Considers feature flags like
isGACEnabled.- Specifically checks for Google Sheets plugin and other plugins allowed for preview mode.
Given these checks, the condition seems to be robust and aligned with business requirements.
app/client/src/pages/Editor/DataSourceEditor/hooks.ts: Implementation ofuseShowPageGenerationOnHeader.app/client/src/ce/hooks/datasourceEditorHooks.tsx: Usage ofuseShowPageGenerationOnHeader.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the business logic alignment for `showGenerateButton`. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python $'showGenerateButton'Length of output: 68
Script:
#!/bin/bash # Description: Verify the business logic alignment for `showGenerateButton`. # Test: Search for the function usage. Expect: Occurrences of `showGenerateButton` in TypeScript/JavaScript files. rg --type js --type ts 'showGenerateButton'Length of output: 339
Script:
#!/bin/bash # Description: Locate the implementation of `useShowPageGenerationOnHeader` and its usage. # Search for the definition of `useShowPageGenerationOnHeader`. rg 'function useShowPageGenerationOnHeader' --type js --type ts # Search for the usage of `useShowPageGenerationOnHeader` across the codebase. rg 'useShowPageGenerationOnHeader' --type js --type tsLength of output: 466
Script:
#!/bin/bash # Description: Retrieve the implementation of `useShowPageGenerationOnHeader`. # Extract the implementation of `useShowPageGenerationOnHeader` from the hooks.ts file. rg --multiline --multiline-dotall --type ts --type js 'export const useShowPageGenerationOnHeader = \(\$$_\) => \{[\s\S]*?\}' app/client/src/pages/Editor/DataSourceEditor/hooks.tsLength of output: 181
Script:
#!/bin/bash # Description: Retrieve the implementation of `useShowPageGenerationOnHeader`. # Extract the implementation of `useShowPageGenerationOnHeader` from the hooks.ts file. rg --multiline --multiline-dotall --type ts --type js 'export const useShowPageGenerationOnHeader = \(\$$_\) => \{[\s\S]*\}' app/client/src/pages/Editor/DataSourceEditor/hooks.tsLength of output: 180
Script:
#!/bin/bash # Description: Retrieve the implementation of `useShowPageGenerationOnHeader`. # Search for the definition of `useShowPageGenerationOnHeader` and extract the surrounding context. rg --context 20 'useShowPageGenerationOnHeader' app/client/src/pages/Editor/DataSourceEditor/hooks.tsLength of output: 1297
app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.tsx (3)
1-1: Import statements have been reordered and cleaned up for clarity and maintainability.Also applies to: 7-7, 8-8
16-16: Use of React hooks (useState,useEffect) is appropriate for managing component state and side effects based on the props and external changes.Also applies to: 17-17
18-18: The custom hookuseDatasourceQueryis used here, ensuring that data fetching logic is encapsulated and reusable, which is a good practice.app/client/src/pages/Editor/DataSourceEditor/DSFormHeader.tsx (1)
Line range hint
1-1: The file comment stresses the importance of not introducing page and application dependencies in this component, which is a good practice to maintain modularity and reusability.Tools
Biome
[error] 225-225: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts (1)
Line range hint
741-760: This function appears to be related to the CRUD page generation feature which is being removed. Please verify if this function is still needed or if it should also be removed.
This reverts commit 31a45f6.
…o feat/clean-datasource-review-page
app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.test.tsx
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 (3)
- app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.test.tsx (1 hunks)
- app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.tsx (2 hunks)
- app/client/src/pages/Editor/DatasourceInfo/GoogleSheetSchema.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.tsx
- app/client/src/pages/Editor/DatasourceInfo/GoogleSheetSchema.tsx
Additional comments not posted (1)
app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.test.tsx (1)
84-102: Test Coverage Verification RecommendedThe test ensures that the "generate page" button is not rendered when the feature flag
release_drag_drop_building_blocks_enabledis true, which aligns with the PR's objectives. However, consider adding tests to verify behavior when the feature flag is disabled to ensure comprehensive coverage.#!/bin/bash # Verify that tests cover scenarios where the feature flag is disabled. rg --type typescript "release_drag_drop_building_blocks_enabled" "app/client/src/pages/Editor/DatasourceInfo"
…o feat/clean-datasource-review-page
jacquesikot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/ce/hooks/datasourceEditorHooks.tsx (4 hunks)
- app/client/src/pages/Editor/DatasourceInfo/HideGeneratePageButton.test.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/hooks/datasourceEditorHooks.tsx
Additional context used
Learnings (1)
app/client/src/pages/Editor/DatasourceInfo/HideGeneratePageButton.test.tsx (1)
User: sneha122 PR: appsmithorg/appsmith#29377 File: app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx:316-329 Timestamp: 2023-12-07T09:53:32.153Z Learning: The user clarified that the feature being introduced is experimental and that automated tests will be added once the decision to keep the feature is made. This context is important for future reviews as it explains the absence of tests for certain features under development.
Additional comments not posted (3)
app/client/src/pages/Editor/DatasourceInfo/HideGeneratePageButton.test.tsx (3)
92-111: Confirm feature flag and rendering behavior in test cases.The test case verifies that the 'generate page' button is not rendered when the feature flag is enabled. This is crucial for ensuring that UI changes are feature-gated properly. However, ensure that there is a counterpart test that checks the behavior when the feature flag is disabled to cover both scenarios.
#!/bin/bash # Description: Verify the presence of the counterpart test for when the feature flag is disabled. # Test: Search for tests that check the disabled state of the feature flag. rg --type tsx --glob '*test.tsx' 'feature flag disabled'
113-138: Ensure comprehensive testing of button attributes.The test verifies that the 'New Query' button is rendered as primary when the feature flag is enabled. It's important to also verify other attributes such as the button's enabled/disabled state and visibility to ensure comprehensive coverage of the UI behavior.
#!/bin/bash # Description: Verify the comprehensive testing of all button attributes. # Test: Search for tests that check other attributes of the 'New Query' button. rg --type tsx --glob '*test.tsx' 'New Query button attributes'
141-160: Validate non-rendering of the 'generate page' button for GoogleSheetSchema.The test case correctly checks that the 'generate page' button is not rendered for the
GoogleSheetSchemawhen the feature flag is enabled. Consider adding a test for when the feature flag is disabled to ensure that the button behaves as expected in all configurations.#!/bin/bash # Description: Verify the presence of tests for the disabled state of the feature flag for GoogleSheetSchema. # Test: Search for tests related to GoogleSheetSchema when the feature flag is disabled. rg --type tsx --glob '*test.tsx' 'GoogleSheetSchema feature flag disabled'
app/client/src/pages/Editor/DatasourceInfo/HideGeneratePageButton.test.tsx
Show resolved
Hide resolved
app/client/src/pages/Editor/DatasourceInfo/HideGeneratePageButton.test.tsx
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9693381070. |
|
Deploy-Preview-URL: https://ce-34348.dp.appsmith.com |
This reverts commit ec5244c.
Reverts #34348 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced button display conditions based on plugin permissions in the Data Source Editor. - **Bug Fixes** - Corrected logic for feature flags in Data Source Editor to ensure consistent button behavior. - **Refactor** - Improved import structure and code readability in Data Source Editor components. - **Chores** - Removed outdated feature flag checks for `releaseDragDropBuildingBlocks` across multiple files. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Description Remove the "Generate new page" button from the datasource review page and make the "New Query" button a primary button. Tests related to the removed generate CRUD page feature have been removed. Fixes appsmithorg#31801 ## Automation /ok-to-test tags="@tag.Datasource" ### 🔍 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/9644141773> > Commit: b167102 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9644141773&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource` <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Removed outdated test cases that were no longer relevant or causing issues in MongoDB and S3 CRUD operations. - **New Tests** - Added `HideGeneratePageButton.test.tsx` to verify the visibility of buttons based on feature flags and user permissions. - **Refactor** - Improved code structure by reordering imports and simplifying logic in various datasource-related components. - Removed deprecated methods and functions related to page generation from datasource components. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Reverts appsmithorg#34348 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced button display conditions based on plugin permissions in the Data Source Editor. - **Bug Fixes** - Corrected logic for feature flags in Data Source Editor to ensure consistent button behavior. - **Refactor** - Improved import structure and code readability in Data Source Editor components. - **Chores** - Removed outdated feature flag checks for `releaseDragDropBuildingBlocks` across multiple files. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Remove the "Generate new page" button from the datasource review page and make the "New Query" button a primary button. Tests related to the removed generate CRUD page feature have been removed.
Fixes #31801
Automation
/ok-to-test tags="@tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9644141773
Commit: b167102
Cypress dashboard.
Tags:
@tag.DatasourceCommunication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
New Tests
HideGeneratePageButton.test.tsxto verify the visibility of buttons based on feature flags and user permissions.Refactor