chore: Adding no access state for Datasource tab based on permissions given via GAC #38268
Conversation
…o chore/no-access-state-ds-bottom-tab
…o chore/no-access-state-ds-bottom-tab
…o chore/no-access-state-ds-bottom-tab
WalkthroughThis pull request introduces comprehensive modifications to the datasource management components, focusing on improving user experience, permission handling, and error management. The changes primarily involve renaming the Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 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
|
![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
🧹 Nitpick comments (4)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceSelector/index.tsx (1)
22-23: Consider enhancing type safety in the conditional check.While the current implementation works, we can make it more robust with explicit undefined checking or optional chaining.
- return props.plugin && - API_FORM_COMPONENTS.includes(props.plugin.uiComponent) ? ( + return props.plugin?.uiComponent && + API_FORM_COMPONENTS.includes(props.plugin.uiComponent) ? (app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/CurrentDataSource.tsx (1)
34-34: Consider handling empty datasourceName more robustlyWhile using
||for the fallback message works, consider using nullish coalescing (??) to only show the SELECT_DATASOURCE message when datasourceName is null/undefined, not when it's an empty string.- {datasourceName || createMessage(SELECT_DATASOURCE)} + {datasourceName ?? createMessage(SELECT_DATASOURCE)}app/client/src/pages/Editor/IDE/hooks.ts (1)
30-31: LGTM! Enhanced navigation context with new hooks.The addition of
useEditorTypeanduseParentEntityInfohooks provides better context for navigation.This change improves the modularity by separating editor type and parent entity concerns.
Also applies to: 63-65
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceTab.tsx (1)
30-35: Enhanced permission handling with feature flags.Good addition of permission checks with feature flag integration. This improves access control granularity.
Consider extracting these permission checks into a custom hook for reusability across other components.
Also applies to: 77-85
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/CurrentDataSource.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceInfo.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceSelector/PluginDatasourceSelector.tsx(4 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceSelector/index.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceTab.tsx(9 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/index.tsx(1 hunks)app/client/src/ce/PluginActionEditor/components/PluginActionResponse/hooks/usePluginActionResponseTabs.tsx(3 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/selectors/appIDESelectors.ts(2 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx(4 hunks)app/client/src/pages/Editor/IDE/hooks.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/index.tsx
🔇 Additional comments (14)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceSelector/index.tsx (1)
22-27: LGTM! Clean and readable conditional rendering.
The simplified conditional logic improves readability while maintaining the same functionality.
app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx (2)
Line range hint 1-738: LGTM! Test coverage looks comprehensive
The test file maintains good coverage across different scenarios:
- Blank states (fullscreen & split screen)
- API routes
- Postgres routes
- Google Sheets routes
Each scenario is tested in both fullscreen and split screen modes with appropriate assertions.
347-347: Verify the impact of removing PluginActionContextProvider
The removal of PluginActionContextProvider wrapper simplifies the test structure, but we should ensure this doesn't reduce test coverage for plugin-specific behavior.
Also applies to: 405-405, 555-555, 606-606
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/CurrentDataSource.tsx (1)
28-32: LGTM: Good fallback implementation for datasource icon
The fallback to a default icon when datasourceIcon is not available improves UI consistency.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceInfo.tsx (1)
Line range hint 58-67: LGTM: Improved edit button visibility logic
Adding the datasourceName check ensures the edit button only appears for valid datasources, preventing potential user confusion.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceSelector/PluginDatasourceSelector.tsx (1)
73-76: LGTM: Clear permission variable naming
Renaming to isActionChangePermitted better reflects the variable's purpose.
app/client/src/ce/PluginActionEditor/components/PluginActionResponse/hooks/usePluginActionResponseTabs.tsx (2)
27-27: LGTM! Import statement updated correctly.
The import statement has been updated to use the renamed DatasourceTab component.
Line range hint 67-71: LGTM! Component usage updated consistently.
The DatasourceTab component is used consistently with the same props structure in both API and DB plugin type cases.
Also applies to: 122-126
app/client/src/pages/Editor/IDE/hooks.ts (1)
77-77: LGTM! Consistent URL construction updates.
All navigation URLs have been updated to use baseParentEntityId consistently.
Also applies to: 82-82, 87-87
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceTab.tsx (2)
43-43: LGTM! Component renamed consistently.
The component has been renamed from Datasource to DatasourceTab, improving clarity about its purpose.
Also applies to: 228-228
150-150: LGTM! Status state now respects permissions.
The status state logic now correctly considers the user's ability to create datasource actions.
app/client/src/ce/constants/messages.ts (1)
397-398: LGTM! The new message constants follow the established patterns.
The added constants follow the codebase conventions for message strings and provide clear, concise messages for their respective use cases.
app/client/src/ce/selectors/appIDESelectors.ts (2)
Line range hint 77-84: LGTM! Clean implementation maintaining selector patterns
The changes to getLastQueryTab are well-structured and maintain consistency with Redux selector patterns.
11-11: Verify the impact of switching to basePageId
The change from getCurrentPageId to getCurrentBasePageId and subsequent parameter updates look correct, but let's ensure this change is consistent throughout the codebase.
Also applies to: 77-80
✅ Verification successful
Change from getCurrentPageId to getCurrentBasePageId is consistent with codebase
The switch from getCurrentPageId to getCurrentBasePageId in the import statement and its usage is part of a larger refactoring effort to use base page IDs consistently. The codebase shows that getCurrentBasePageId is properly implemented in selectors/editorSelectors.tsx and is being used correctly across multiple components and sagas.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining usage of getCurrentPageId and potential inconsistencies
rg "getCurrentPageId" --type ts
# Check the usage pattern of getCurrentBasePageId
rg "getCurrentBasePageId" --type ts -A 2
# Look for potential missed updates of pageId to basePageId
rg "pageId.*getQueryEntityItemUrl" --type ts
Length of output: 42033
Script:
#!/bin/bash
# Check for getQueryEntityItemUrl usage and its parameters
ast-grep --pattern 'getQueryEntityItemUrl($$$)'
# Look for any remaining references to pageId in the file
rg -A 2 "pageId" "app/client/src/ce/selectors/appIDESelectors.ts"
Length of output: 67038
...luginActionResponse/components/DatasourceTab/DatasourceSelector/PluginDatasourceSelector.tsx
Show resolved
Hide resolved
…o chore/no-access-state-ds-bottom-tab
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/client/src/sagas/DatasourcesSagas.ts (1)
1574-1642: Consider restructuring nested try-catch blocksThe nested try-catch blocks make the code harder to read and maintain. Consider extracting the inner try-catch block into a separate function for better readability.
- try { - try { - const response: ApiResponse = yield DatasourcesApi.fetchDatasourceStructure( - action.payload.id, - action.payload.ignoreCache, - ); - // ... rest of the inner try block - } catch (error) { - // ... inner catch block - } - } catch (error) { - // ... outer catch block - } + try { + const response: ApiResponse = yield call( + fetchAndValidateDatasourceStructure, + action.payload.id, + action.payload.ignoreCache, + ); + // ... rest of the code + } catch (error) { + // ... error handling + }🧰 Tools
🪛 Biome (1.9.4)
[error] 1615-1615: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
app/client/src/ce/utils/permissionHelpers.tsx (1)
22-22: Consider documenting the permissive behaviorThe
hasReadDatasourcePermissionfunction always returns true, which appears intentional for the CE version. Consider adding a comment explaining this design decision for future maintainers.Also applies to: 89-90
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/CurrentDataSourceLink.tsx (1)
24-24: LGTM! Consider future refactoringThe changes look good. The
pluginIdprop is correctly passed to the child component while maintaining existing navigation functionality.Consider using a custom hook to handle the datasource-related props and navigation logic together, making the component more focused on presentation.
// Example refactor: const useDatasourceNavigation = (datasourceId: string, pluginId: string) => { const { goToDatasource } = useGoToDatasource(); const handleClick = useCallback( () => goToDatasource(datasourceId), [datasourceId, goToDatasource], ); return { handleClick }; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/CurrentDataSource.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/CurrentDataSourceLink.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceSelector/PluginDatasourceSelector.tsx(4 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceTab.tsx(9 hunks)app/client/src/ce/PluginActionEditor/components/PluginActionResponse/hooks/usePluginActionResponseTabs.tsx(3 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/selectors/entitiesSelector.ts(1 hunks)app/client/src/ce/utils/BusinessFeatures/permissionPageHelpers.tsx(2 hunks)app/client/src/ce/utils/permissionHelpers.tsx(2 hunks)app/client/src/sagas/DatasourcesSagas.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/src/ce/constants/messages.ts
- app/client/src/ce/PluginActionEditor/components/PluginActionResponse/hooks/usePluginActionResponseTabs.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceTab.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceSelector/PluginDatasourceSelector.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/sagas/DatasourcesSagas.ts
[error] 1615-1615: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (4)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/CurrentDataSource.tsx (2)
2-2: LGTM! Good fallback handling for missing icons
The addition of the Icon component with a default "datasource-v3" fallback improves the UI resilience when datasourceIcon is unavailable.
Also applies to: 21-25
9-10: Verify the impact of prop changes
The switch from datasourceId to pluginId looks correct, but we should verify all component usages are updated.
Also applies to: 13-14
✅ Verification successful
All component usages are properly updated
The search results show that all usages of CurrentDataSource component have been updated to use pluginId prop:
- In
CurrentDataSourceLink.tsx, it correctly passespluginId - In
PluginDatasourceSelector.tsx, it passesplugin?.idaspluginId
No instances of passing datasourceId to CurrentDataSource were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CurrentDataSource component usage
rg -l "CurrentDataSource" | xargs rg "datasourceId|pluginId" -A 2 -B 2
Length of output: 11304
app/client/src/ce/utils/BusinessFeatures/permissionPageHelpers.tsx (1)
8-10: LGTM! Consistent implementation pattern
The new permission helper follows the established pattern for feature flag-based permission handling between CE and EE versions.
Also applies to: 64-70
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/CurrentDataSourceLink.tsx (1)
9-9: Verify the necessity of maintaining both IDs
The component now requires both datasourceId and pluginId. Let's verify if this dual ID approach is intentional and necessary for backward compatibility.
Also applies to: 13-13
✅ Verification successful
Both datasourceId and pluginId are valid requirements
The dual ID approach is intentional and necessary based on the codebase structure. Both fields are used in core interfaces like ApiDatasourceForm and ReduxStateProps, serving different purposes in the datasource management system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using both IDs to understand the pattern
rg -A 2 "datasourceId.*pluginId|pluginId.*datasourceId" --type ts --type tsx
Length of output: 106
Script:
#!/bin/bash
# Search for usage patterns of datasourceId and pluginId with proper file type
rg -t typescript -t javascriptreact -t typescriptreact -A 2 "datasourceId.*pluginId|pluginId.*datasourceId"
# Search for files containing both terms to understand the relationship
rg -t typescript -t javascriptreact -t typescriptreact -l "datasourceId" | xargs rg -t typescript -t javascriptreact -t typescriptreact -l "pluginId"
# Look for type definitions or interfaces containing both fields
ast-grep --pattern 'interface $_ {
$$$
datasourceId: $_
$$$
pluginId: $_
$$$
}'
Length of output: 5345
ankitakinger
changed the title
github-actions
bot
added
the
skip-changelog
github-actions
bot
added
IDE Pod
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12431487836. |
|
Deploy-Preview-URL: https://ce-38268.dp.appsmith.com |
ankitakinger
force-pushed
the
chore/no-access-state-ds-bottom-tab
branch
from
7dfc97a to
ee5bc17
Compare
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12436774024. |
|
Deploy-Preview-URL: https://ce-38268.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/locators/QueryEditor.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/locators/QueryEditor.json (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
| @@ -9,7 +9,7 @@ | |||
| "addDatasource": ".t--add-datasource", | |||
| "editDatasourceButton": ".t--edit-datasource", | |||
| "switch": ".t--form-control-SWITCH input", | |||
| "queryResponse": "(//div[@class='table']//div[@class='tr'])[3]//div[@class='td']", | |||
| "queryResponse": "(//div[@class='table']//div[@class='tr'])[3]//div[@class='td mp-mask']", | |||
There was a problem hiding this comment.
Replace XPath selector with data-testid attribute
The current selector violates our coding guidelines by using XPath and depending on specific table structure. This makes the tests brittle and harder to maintain.
Replace with a data-testid based selector:
- "queryResponse": "(//div[@class='table']//div[@class='tr'])[3]//div[@class='td mp-mask']",
+ "queryResponse": "[data-testid=query-response-cell]",Remember to add the corresponding data-testid attribute to the component:
<div className="td mp-mask" data-testid="query-response-cell">
{/* cell content */}
</div>
ankitakinger
force-pushed
the
chore/no-access-state-ds-bottom-tab
branch
from
a4c08e7 to
ee5bc17
Compare
Description
BEFORE:
When view access is not given:
When create action permission is not given but view access is given:
AFTER:
When view access is not given:
When create action permission is not given but view access is given:
Fixes #38093
Automation
/ok-to-test tags="@tag.Sanity, @tag.Datasource, @tag.IDE, @tag.JS, @tag.Git"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12452839374
Commit: ee5bc17
Cypress dashboard.
Tags: @tag.Sanity, @tag.Datasource, @tag.IDE, @tag.JS, @tag.Git
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ServerSide/QueryPane/AddWidgetTableAndBind_spec.js
List of identified flaky tests.Sun, 22 Dec 2024 10:48:37 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Enhancements
Tests
Chores