fix: gsheets all sheets option enabled behind a feature flag #37942
Conversation
|
/build-deploy-preview skip-tests=true |
WalkthroughThe pull request introduces several changes related to the Google Sheets integration. Key modifications include the addition of a new feature flag, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
|
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12158380587. |
|
Deploy-Preview-URL: https://ce-37942.dp.appsmith.com |
github-actions
bot
added
Google Sheets
![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
🧹 Outside diff range and nitpick comments (8)
app/client/cypress/e2e/GSheet/ReadNWrite_Access_Spec.ts (3)
Line range hint
14-42: Replace direct cy.get with custom commands for better reliabilityThe test setup uses direct cy.get("@postExecute") which can lead to flaky tests. Consider creating a custom command to handle API response assertions.
- cy.get("@postExecute").then((interception: any) => { - expect(interception.response.body.data.body.properties.title).to.deep.equal(spreadSheetName); - }); + cy.validateApiResponse("@postExecute", (response) => { + expect(response.body.data.body.properties.title).to.deep.equal(spreadSheetName); + });
Line range hint
44-332: Replace Sleep and wait commands with proper assertionsMultiple test cases use agHelper.Sleep() and cy.wait which can make tests flaky and slow. Replace these with proper assertions and intercepts.
Examples to fix:
- agHelper.Sleep(500); + cy.waitUntil(() => cy.get('[data-cy="spreadsheet-dropdown"]').should('be.visible')); - cy.wait("@importNewApplication") + cy.intercept('POST', '**/importNewApplication').as('importApp'); + cy.wait('@importApp').its('response.statusCode').should('eq', 200);Use data- attributes for selectors*
Table assertions use direct selectors. Add data-* attributes for more reliable selection.
- table.ReadTableRowColumnData(0, 0, "v2") + table.ReadTableRowColumnData('[data-cy="table-cell-0-0"]')
Line range hint
334-348: Improve cleanup reliabilityThe cleanup section should handle failures gracefully and ensure resources are always cleaned up.
after("Delete spreadsheet and app", function () { + cy.log('Starting cleanup'); homePage.EditAppFromAppHover(appName); gsheetHelper.DeleteSpreadsheetQuery( dataSourceName.allAccess, spreadSheetName, ); - cy.get("@postExecute").then((interception: any) => { - expect(interception.response.body.data.body.message).to.deep.equal( - "Deleted spreadsheet successfully!", - ); - }); + cy.validateApiResponse("@postExecute", (response) => { + expect(response.body.data.body.message).to.deep.equal("Deleted spreadsheet successfully!"); + }).catch((error) => { + cy.log('Failed to delete spreadsheet:', error); + }); homePage.NavigateToHome(); homePage.DeleteApplication(appName); + cy.log('Cleanup completed'); });app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts (2)
Line range hint
1-24: Replace Sleep calls with proper wait strategiesThe test suite uses
agHelper.Sleep()which is discouraged according to the coding guidelines. Consider using Cypress's built-in retry-ability and assertions instead.- agHelper.Sleep(500); + cy.get(dataSources._spreadsheetDropdown).should('be.visible');
Line range hint
33-70: Avoid string literals in assertionsMove string literals to constants or test fixtures for better maintainability.
+ const EXPECTED_MESSAGES = { + DELETED_SPREADSHEET: "Deleted spreadsheet successfully!", + DELETED_ROW: "Deleted row successfully!" + }; - expect(interception.response.body.data.body.message).to.deep.equal("Deleted spreadsheet successfully!"); + expect(interception.response.body.data.body.message).to.deep.equal(EXPECTED_MESSAGES.DELETED_SPREADSHEET);app/client/cypress/e2e/GSheet/AllAccess_Spec.ts (2)
Line range hint
18-38: Use route aliases instead of cy.waitReplace cy.wait with proper network handling using route aliases.
- cy.wait("@postExecute") + cy.intercept('POST', '**/postExecute').as('postExecute') + cy.wait('@postExecute').its('response.statusCode').should('eq', 200)
Line range hint
150-200: Extract repeated test data assertions into helper functionsMultiple test cases contain duplicate assertion logic for table data. Consider extracting these into reusable helper functions.
const assertTableData = (rowIndex: number, expectedData: any) => { table.ReadTableRowColumnData(rowIndex, 0, "v2").then((cellData) => { expect(cellData).to.eq(expectedData); }); };app/client/cypress/e2e/GSheet/ReadOnly_Access_Spec.ts (1)
Line range hint
14-30: Consider using custom commands for common operationsThe test setup and verification steps are repeated across multiple test suites. Consider creating custom Cypress commands for these operations.
Example custom command:
Cypress.Commands.add('setupGSheetTest', (options) => { const uuid = Cypress._.random(0, 10000); const spreadSheetName = `test-sheet_${uuid}`; const appName = `gsheet-app-${uuid}`; // ... rest of the setup logic return { spreadSheetName, appName }; });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/client/cypress/e2e/GSheet/AllAccess_Spec.ts(1 hunks)app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts(1 hunks)app/client/cypress/e2e/GSheet/ReadNWrite_Access_Spec.ts(1 hunks)app/client/cypress/e2e/GSheet/ReadOnly_Access_Spec.ts(1 hunks)app/client/cypress/e2e/GSheet/WidgetBinding_AllAccess_Spec.ts(1 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/GoogleSheets_spec.ts(1 hunks)app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
app/client/cypress/e2e/GSheet/ReadOnly_Access_Spec.ts (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.
app/client/cypress/e2e/Regression/ServerSide/QueryPane/GoogleSheets_spec.ts (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.
app/client/cypress/e2e/GSheet/WidgetBinding_AllAccess_Spec.ts (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.
app/client/cypress/e2e/GSheet/ReadNWrite_Access_Spec.ts (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.
app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts (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.
app/client/cypress/e2e/GSheet/AllAccess_Spec.ts (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.
🔇 Additional comments (3)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/GoogleSheets_spec.ts (1)
31-33: LGTM: Test coverage added for new scope options
The new scope options align with the form configuration changes and follow Cypress best practices.
app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json (1)
41-52: Verify security implications of broader OAuth scopes
While the implementation is correct, broader OAuth scopes require security review:
https://www.googleapis.com/auth/driveprovides full access to all files- Consider documenting the security implications for users
✅ Verification successful
Let me search for any existing documentation about Google Sheets OAuth scopes and security implications.
Let me search for any documentation specifically about Google Sheets OAuth scopes in the plugin's codebase.
OAuth scopes are properly implemented with clear security boundaries
The implementation follows Google's OAuth2 best practices:
- Most restrictive scope
drive.fileis set as default - Each scope option clearly indicates its access level (Read/Write/Delete)
- Broader scopes require explicit user selection
- Scope combinations are valid and properly documented in form.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any security guidelines or documentation updates needed
fd -e md -e txt . | xargs rg -i "oauth|scope|permission|security"
Length of output: 10307
Script:
#!/bin/bash
# Check for documentation about Google Sheets OAuth scopes
rg -i "google.*sheets.*scope|sheets.*permission" -A 3 -B 3
# Also check for any existing security documentation specific to plugins
fd -e md . app/server/appsmith-plugins/googleSheetsPlugin/ | xargs rg -i "security|scope|permission"
Length of output: 130800
Script:
#!/bin/bash
# Search for OAuth scope documentation in the Google Sheets plugin
rg -i "oauth.*scope|scope.*permission" -A 3 -B 3 app/server/appsmith-plugins/googleSheetsPlugin/
# Also check for any security-related comments in the form.json file
cat app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json
Length of output: 13699
app/client/cypress/e2e/GSheet/WidgetBinding_AllAccess_Spec.ts (1)
24-24: LGTM: Re-enabling widget binding tests
Test suite properly validates widget binding functionality with Google Sheets and follows all testing best practices.
sneha122
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts (1)
25-25: Consider using Cypress environment variables for timeouts.Replace hardcoded timeout values with Cypress environment variables for better maintainability:
- timeout: Cypress.config("pageLoadTimeout") + timeout: Cypress.env('API_TIMEOUT')Also applies to: 61-61, 155-155
app/client/cypress/e2e/GSheet/AllAccess_Spec.ts (2)
Line range hint
77-82: Remove agHelper.Sleep() callAccording to our coding guidelines, we should avoid using agHelper.Sleep(). Consider using Cypress's built-in retry-ability and assertions instead.
- agHelper.Sleep(500); + cy.get('[data-cy="spreadsheet-dropdown"]').should('be.visible');
Line range hint
332-347: Replace after hook with cy.cleanup commandAccording to our coding guidelines, we should avoid using after and afterEach hooks in test cases.
- after("Delete spreadsheet and app", function () { - // Delete spreadsheet and app - homePage.EditAppFromAppHover(appName); - gsheetHelper.DeleteSpreadsheetQuery(dataSourceName, spreadSheetName); - cy.get("@postExecute").then((interception: any) => { - expect(interception.response.body.data.body.message).to.deep.equal( - "Deleted spreadsheet successfully!", - ); - }); - homePage.NavigateToHome(); - homePage.DeleteApplication(appName); - }); + it("Cleanup: Delete spreadsheet and app", function () { + homePage.EditAppFromAppHover(appName); + gsheetHelper.DeleteSpreadsheetQuery(dataSourceName, spreadSheetName); + cy.get("@postExecute").then((interception: any) => { + expect(interception.response.body.data.body.message).to.deep.equal( + "Deleted spreadsheet successfully!", + ); + }); + homePage.NavigateToHome(); + homePage.DeleteApplication(appName); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
app/client/cypress/e2e/GSheet/AllAccess_Spec.ts(2 hunks)app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts(2 hunks)app/client/cypress/e2e/GSheet/ReadNWrite_Access_Spec.ts(3 hunks)app/client/cypress/e2e/GSheet/ReadOnly_Access_Spec.ts(3 hunks)app/client/cypress/e2e/GSheet/WidgetBinding_AllAccess_Spec.ts(1 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/GoogleSheets_spec.ts(2 hunks)app/client/src/ce/entities/FeatureFlag.ts(2 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java(1 hunks)app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/cypress/e2e/Regression/ServerSide/QueryPane/GoogleSheets_spec.ts
- app/client/cypress/e2e/GSheet/ReadNWrite_Access_Spec.ts
- app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/form.json
- app/client/cypress/e2e/GSheet/ReadOnly_Access_Spec.ts
- app/client/cypress/e2e/GSheet/WidgetBinding_AllAccess_Spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/GSheet/AllAccess_Spec.ts (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.
app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts (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.
🔇 Additional comments (5)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java (1)
18-18: LGTM!
The feature flag follows the established naming convention and is correctly placed in the production feature flags section.
app/client/src/ce/entities/FeatureFlag.ts (1)
45-46: LGTM!
The feature flag is properly implemented with a safe default value of false.
Also applies to: 84-84
app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts (1)
42-45: LGTM!
The feature flag is properly intercepted in the before hook, ensuring consistent test behavior.
app/client/cypress/e2e/GSheet/AllAccess_Spec.ts (2)
3-3: LGTM: Feature flag setup is properly implemented
The feature flag intercept is correctly placed in the before hook and properly imported.
Also applies to: 26-29
19-25: Verify test tags are correctly configured
The test tags look good, but ensure they align with your team's tagging strategy for test categorization and filtering.
✅ Verification successful
Test tags are consistently configured across GSheet test files
All GSheet test files in the e2e directory use the same set of tags: "@tag.Datasource", "@tag.GSheet", "@tag.Git", and "@tag.AccessControl", showing proper alignment with the team's test categorization strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent tag usage across test files
rg -l '@tag\.' app/client/cypress/e2e/GSheet/ | xargs rg '@tag\.'
Length of output: 1022
sneha122
added
ok-to-test
sneha122
requested review from
AmanAgarwal041
and removed request for
ApekshaBhosale and
sagar-qa007
|
/build-deploy-preview skip-tests=true |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/components/formControls/RadioButtonControl.tsx(1 hunks)
🔇 Additional comments (1)
app/client/src/components/formControls/RadioButtonControl.tsx (1)
48-55: LGTM on making the radio group controlled
The addition of the value prop to sync with Redux Form's state is a good practice for form controls.
| @@ -45,12 +45,14 @@ function renderComponent(props: renderComponentProps) { | |||
|
|
|||
| const options = props.options || []; | |||
| const defaultValue = props.initialValue as string; | |||
| const currentValue = props.input?.value; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing defaultValue when using value prop
While adding the value prop is a good practice for controlled components, having both defaultValue and value props might lead to unexpected behavior. The defaultValue prop is typically used for uncontrolled components, while value is for controlled components.
Consider this change:
return (
<StyledRadioGroup
data-testid={props.input?.name}
- defaultValue={defaultValue}
onChange={onChangeHandler}
value={currentValue}
>Also applies to: 55-55
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts (2)
Line range hint
52-54: Replace hardcoded timeouts with custom commandsUsing
cy.get("@postExecute", { timeout: Cypress.config("pageLoadTimeout") })in multiple places creates maintenance overhead. Consider creating a custom command for handling API responses.// In commands.ts Cypress.Commands.add('waitForGSheetResponse', () => { return cy.get("@postExecute", { timeout: Cypress.config("pageLoadTimeout") }); }); // In test file cy.waitForGSheetResponse().then((interception: any) => { expect(interception.response.body.data.body.properties.title).to.deep.equal(spreadSheetName); });Also applies to: 144-146, 249-251
Line range hint
201-205: Use constants for test data assertionsHardcoded assertion strings make tests brittle and harder to maintain. Consider moving these to a test data constants file.
// In test-data-constants.ts export const TEST_DATA = { JAPANESE_NAME: "ホーンビィ 2014 カタログ", CURRENCIES: "₹, $, €, ¥, £", SPECIAL_CHARS: "!@#$%^&*", }; // In test file dataSources.AssertQueryTableResponse(1, TEST_DATA.JAPANESE_NAME); dataSources.AssertQueryTableResponse(2, TEST_DATA.CURRENCIES); dataSources.AssertQueryTableResponse(3, TEST_DATA.SPECIAL_CHARS);Also applies to: 216-220, 231-235
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/client/cypress/e2e/GSheet/AllAccess_Spec.ts(2 hunks)app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts(1 hunks)app/client/cypress/e2e/GSheet/ReadNWrite_Access_Spec.ts(2 hunks)app/client/cypress/e2e/GSheet/ReadOnly_Access_Spec.ts(2 hunks)app/client/cypress/e2e/GSheet/WidgetBinding_AllAccess_Spec.ts(1 hunks)app/client/cypress/support/Pages/IDE/BottomTabs/Response.ts(1 hunks)app/client/src/ce/entities/FeatureFlag.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/src/ce/entities/FeatureFlag.ts
- app/client/cypress/e2e/GSheet/ReadNWrite_Access_Spec.ts
- app/client/cypress/e2e/GSheet/WidgetBinding_AllAccess_Spec.ts
- app/client/cypress/e2e/GSheet/ReadOnly_Access_Spec.ts
- app/client/cypress/e2e/GSheet/AllAccess_Spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/support/Pages/IDE/BottomTabs/Response.ts (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.
app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts (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.
…mithorg/appsmith into chore/gs-all-sheets-enabled
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts (2)
Line range hint
27-27: Remove .skip to enable test suiteThe test suite is currently skipped. Remove the .skip to enable these tests.
-describe.skip( +describe(
Line range hint
42-76: Avoid using cy.get('@postExecute')Replace cy.get('@postExecute') with cy.wait('@postExecute') for better test stability. This ensures the request is complete before assertions.
- cy.get("@postExecute", { + cy.wait("@postExecute", { timeout: Cypress.config("pageLoadTimeout"), }).then((interception: any) => {app/client/cypress/e2e/GSheet/ReadOnly_Access_Spec.ts (3)
Line range hint
12-12: Remove .skip to enable test suiteThe test suite is currently skipped. Remove the .skip to enable these tests.
-describe.skip( +describe(
Line range hint
42-48: Avoid using cy.get('@postExecute')Replace cy.get('@postExecute') with cy.wait('@postExecute') for better test stability. This ensures the request is complete before assertions.
- cy.get("@postExecute").then((interception: any) => { + cy.wait("@postExecute").then((interception: any) => {This change should be applied to all similar occurrences in the file.
Line range hint
82-83: Remove unnecessary Sleep callThe agHelper.Sleep(500) call should be replaced with a proper wait condition.
- agHelper.Sleep(500); + agHelper.WaitUntilAllToastsDisappear();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/cypress/e2e/GSheet/AllAccess_Spec.ts(1 hunks)app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts(1 hunks)app/client/cypress/e2e/GSheet/ReadNWrite_Access_Spec.ts(1 hunks)app/client/cypress/e2e/GSheet/ReadOnly_Access_Spec.ts(1 hunks)app/client/cypress/e2e/GSheet/WidgetBinding_AllAccess_Spec.ts(1 hunks)app/client/src/ce/entities/FeatureFlag.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/src/ce/entities/FeatureFlag.ts
- app/client/cypress/e2e/GSheet/WidgetBinding_AllAccess_Spec.ts
- app/client/cypress/e2e/GSheet/AllAccess_Spec.ts
- app/client/cypress/e2e/GSheet/ReadNWrite_Access_Spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/GSheet/ReadOnly_Access_Spec.ts (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.
app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts (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.
🔇 Additional comments (1)
app/client/cypress/e2e/GSheet/GsheetMisc_Spec.ts (1)
19-19:
Add feature flag setup in before hook
The feature flag is imported but not used. Add it to the before hook to properly enable the feature flag.
before("Setup app and spreadsheet", function () {
+ featureFlagIntercept({
+ release_gs_all_sheets_options_enabled: true,
+ });
//Add a new app and an add new spreadsheet queryLikely invalid or redundant comment.
| @@ -1,5 +1,6 @@ | |||
| /// <reference types="Cypress" /> | |||
| import { GSHEET_DATA } from "../../fixtures/test-data-gsheet"; | |||
| import { featureFlagIntercept } from "../../support/Objects/FeatureFlags"; | |||
There was a problem hiding this comment.
Add feature flag setup in before hook
The feature flag is imported but not used. Add it to the before hook to properly enable the feature flag.
before("Setup app and spreadsheet", function () {
+ featureFlagIntercept({
+ release_gs_all_sheets_options_enabled: true,
+ });
//Add a new app and an add new spreadsheet queryCommittable suggestion skipped: line range outside the PR's diff.
Failed server tests
|
1 similar comment
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/components/formControls/RadioButtonControl.tsx (1)
48-50: Simplify the boolean conversion logicThe double negation (
!!) is unnecessary here as the ternary operation will already coerce the value to a boolean.- const defaultValue = !!selectedValue + const defaultValue = selectedValue ? selectedValue : (props.initialValue as string);🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/GoogleSheets_spec.ts(2 hunks)app/client/src/components/formControls/RadioButtonControl.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ServerSide/QueryPane/GoogleSheets_spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/components/formControls/RadioButtonControl.tsx
[error] 48-48: 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 (1)
app/client/src/components/formControls/RadioButtonControl.tsx (1)
Line range hint 53-55: Consider using controlled component pattern
Using both defaultValue and onChange can lead to unexpected behavior. Since you're already handling the onChange event, consider using the value prop instead of defaultValue for better control flow.
<StyledRadioGroup
data-testid={props.input?.name}
- defaultValue={defaultValue}
+ value={defaultValue}
onChange={onChangeHandler}
>🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: 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)
…horg#37942) ## Description This PR adds back all sheets option which had been removed in PR: appsmithorg#36125 Fixes appsmithorg#38002 _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.Datasource, @tag.Widget" ### 🔍 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/12311024852> > Commit: e177b64 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12311024852&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource, @tag.Widget` > Spec: > <hr>Fri, 13 Dec 2024 08:27:08 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** - Added new options for Google Sheets permissions, enhancing user selection capabilities. - Introduced a feature flag to control the visibility of new Google Sheets options. - **Bug Fixes** - Improved handling of feature flags in the datasource section rendering, ensuring correct visibility based on flags. - **Documentation** - Updated test specifications for Google Sheets datasource to include new dropdown options. - **Chores** - Enhanced the logic for setting default values in the RadioButtonControl component. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“sneha@appsmith.com”> Co-authored-by: Alex Golovanov <alex@appsmith.com>
…horg#37942) ## Description This PR adds back all sheets option which had been removed in PR: appsmithorg#36125 Fixes appsmithorg#38002 _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.Datasource, @tag.Widget" ### 🔍 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/12311024852> > Commit: e177b64 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12311024852&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource, @tag.Widget` > Spec: > <hr>Fri, 13 Dec 2024 08:27:08 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** - Added new options for Google Sheets permissions, enhancing user selection capabilities. - Introduced a feature flag to control the visibility of new Google Sheets options. - **Bug Fixes** - Improved handling of feature flags in the datasource section rendering, ensuring correct visibility based on flags. - **Documentation** - Updated test specifications for Google Sheets datasource to include new dropdown options. - **Chores** - Enhanced the logic for setting default values in the RadioButtonControl component. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“sneha@appsmith.com”> Co-authored-by: Alex Golovanov <alex@appsmith.com>
Description
This PR adds back all sheets option which had been removed in PR: #36125
Fixes #38002
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.Datasource, @tag.Widget"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12311024852
Commit: e177b64
Cypress dashboard.
Tags:
@tag.Datasource, @tag.WidgetSpec:
Fri, 13 Dec 2024 08:27:08 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores