feat: remove generate new page button from datasource preview pages #38182
Conversation
…owSecondaryGenerateButton in datasourceEditorHooks
WalkthroughThe pull request focuses on removing the generate page button functionality from various components in the datasource preview page. The changes simplify the codebase by eliminating feature flag dependencies and removing conditional rendering logic related to page generation. The modifications span multiple files in the client-side application, primarily targeting the datasource editor and information components. Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (7)
💤 Files with no reviewable changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (8)
Finishing Touches
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
|
This reverts commit c0cd8f3.
…o feat/remove-generate-new-page-button
jacquesikot
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
![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 comments (6)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts (2)
Line range hint
77-78: Remove or fix the commented verificationThe comment indicates a flaky test. Either fix the verification or remove the commented code entirely.
Line range hint
89-91: Use constants for assertion valuesReplace hardcoded values with constants to improve maintainability and follow coding guidelines.
+const EXPECTED_SHIP_ID = "371681"; +const EXPECTED_VESSEL_TYPE = "Passenger"; -dataSources.AssertQueryTableResponse(0, "371681"); -dataSources.AssertQueryTableResponse(6, "Passenger"); +dataSources.AssertQueryTableResponse(0, EXPECTED_SHIP_ID); +dataSources.AssertQueryTableResponse(6, EXPECTED_VESSEL_TYPE);app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js (1)
Line range hint
82-182: Multiple test practices need improvementThe test case violates several Cypress best practices:
- Replace explicit waits with proper assertions or intercepts
- Remove force clicks and use proper element visibility handling
- Clean up commented code
- Use constants for selector strings
Here's how to improve the code:
- cy.get(datasourceEditor.AmazonS3).click({ force: true }).wait(1000); + cy.get(datasourceEditor.AmazonS3).should('be.visible').click(); - cy.wait(2000); + cy.get(generatePage.selectTableDropdown).should('be.visible'); - //Post Execute call not happening.. hence commenting it for this case - //cy.wait("@post_Execute").should("have.nested.property", "response.body.responseMeta.status", 200,); - // cy.get("@saveDatasource").then((httpResponse) => { - // datasourceName = httpResponse.response.body.data.name; - // }); - cy.get("span:contains('Got it')").click(); + cy.get(commonlocators.gotItButton).click();app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres1_Spec.ts (1)
Line range hint
89-91: Replace Sleep with Cypress's built-in retry-abilityUsing
agHelper.Sleep(3000)is not recommended in Cypress tests. Instead, use Cypress's automatic retry and wait mechanisms.- agHelper.Sleep(3000); + cy.waitUntil(() => table.WaitUntilTableLoad(0, 0, "v2"));app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts (2)
Line range hint
31-82: Move SQL data to fixturesLarge SQL statements should be moved to fixture files for better maintainability and reusability.
Consider creating a new fixture file
cypress/fixtures/stores.sqland importing it in the test.
Line range hint
92-127: Replace hardcoded waits with Cypress commandsMultiple instances of
agHelper.Sleep(3000)should be replaced with Cypress's built-in waiting mechanisms.- agHelper.Sleep(3000); //wait for table navigation to take effect! + cy.waitUntil(() => table.WaitUntilTableLoad(0, 0, "v2"));
🧹 Nitpick comments (5)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts (1)
11-11: Consider using a more specific type instead ofanyReplace
anywith a more specific type likestringto improve type safety.-let dsName: any; +let dsName: string;app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js (1)
Line range hint
1-30: Consider architectural improvementsWhile the test structure is generally good, consider these improvements:
- Move all selectors to locator files
- Add type definitions for the test data
- Consider using custom commands for repetitive operations
Example improvements:
+ // In GeneratePage.json + { + "gotItButton": "[data-cy='got-it-button']", + "generatePageTitle": "[data-cy='generate-page-title']" + } + // In commands.js + Cypress.Commands.add('fillS3Form', (formData) => { + // Common S3 form filling logic + });app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts (2)
Line range hint
16-24: Remove commented code blocksRemove commented code blocks that are no longer needed. If they're for reference, consider moving them to documentation.
- // beforeEach(function() { - // if (INTERCEPT.MYSQL) { - // cy.log("MySQL DB is not found. Using intercept"); - // //dataSources.StartInterceptRoutesForMySQL(); - // } else cy.log("MySQL DB is found, hence using actual DB"); - // });
Line range hint
207-219: Improve error handling test caseThe error handling test case could be more comprehensive. Consider adding more assertions for different error scenarios.
cy.wait("@postExecute").then(({ response }) => { expect(response?.body.data.isExecutionSuccess).to.eq(false); expect( response?.body.data.pluginErrorDetails.downstreamErrorMessage, ).to.contains("Unknown table 'fakeapi.productlines'"); + // Add more assertions for error details + expect(response?.body.data.pluginErrorDetails.error).to.exist; + expect(response?.body.data.pluginErrorDetails.errorType).to.exist; });app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts (1)
89-91: Track skipped test with issue numberThe skipped test is properly documented with issue #29870, but consider adding more context about the failure condition.
- it.skip("3. Verify Refresh table from Deploy page - on Stores & verify all updates persists : Skipped till #29870 gets fixed", () => { + it.skip("3. Verify Refresh table from Deploy page - on Stores & verify all updates persists : Skipped due to table v2 selection issues (#29870)", () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Mongo_Spec.ts(0 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts(2 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres1_Spec.ts(2 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts(1 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Mongo_Spec.ts
🧰 Additional context used
📓 Path-based instructions (5)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_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/GenerateCRUD/S3_Spec.js (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/GenerateCRUD/Postgres1_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/GenerateCRUD/Postgres2_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/GenerateCRUD/MySQL2_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 (2)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts (1)
Line range hint 13-103: LGTM! Changes align with PR objectives
The removal of generate page button related tests while maintaining core CRUD validation aligns well with the PR objectives. The remaining test cases provide good coverage of essential functionality.
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres1_Spec.ts (1)
Line range hint 1-14: Verify test coverage for remaining CRUD operations
With the removal of the generate page test case, ensure that the remaining test cases adequately cover CRUD operations through other means.
✅ Verification successful
Based on the search results, I can see comprehensive test coverage for CRUD operations across multiple test files (Postgres1_Spec.ts, Postgres2_Spec.ts, Mongo_Spec.ts, MongoURI_Spec.ts, MySQL2_Spec.ts). Each file contains tests for:
- Create operations (e.g., "Create new CRUD Table", "Insert document(s)")
- Read operations (e.g., "Select record", "Find document(s)")
- Update operations (e.g., "Verify Update data", "updateNVerify")
- Delete operations (e.g., "Verify Delete", "DeleteDatasource")
Test coverage for CRUD operations is well maintained across different database types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check coverage of CRUD operations in remaining tests
rg -A 5 "describe\(.*CRUD" app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/
Length of output: 87
Script:
#!/bin/bash
# Let's search for CRUD operation test cases with a broader pattern
rg -i "crud|create|read|update|delete" app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/ -A 5
# Also check for test descriptions that might indicate CRUD operations
rg "describe\(.*[\"'].*(?i)(crud|create|read|update|delete)" app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/ -A 5
# Search for specific Cypress test patterns that might indicate CRUD operations
rg "it\(.*[\"'].*(?i)(crud|create|read|update|delete)" app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/ -A 5
Length of output: 70911
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12432240679. |
|
Deploy-Preview-URL: https://ce-38182.dp.appsmith.com |
jacquesikot
added
ok-to-test
…o feat/remove-generate-new-page-button
jacquesikot
added
ok-to-test
…o feat/remove-generate-new-page-button
…o feat/remove-generate-new-page-button
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_spec.ts (1)
Line range hint
248-264: Improve test reliability with better assertionsThe test lacks proper assertions between page navigation steps. Consider:
- Adding wait conditions for page load
- Verifying the active tab state
- Using data-cy attributes instead of class selectors
it.skip("9. Verify selecting JSObject does not change the page", () => { PageList.AddNewPage("New blank page"); + cy.url().should('include', '/page'); PageLeftPane.switchSegment(PagePaneSegment.JS); agHelper.GetNClick(locators._createNew); agHelper.AssertContains("JSObject1", "exist", entityExplorer._entityName); agHelper.GetNClick(jsEditor._addJSObj); agHelper.AssertContains("JSObject2", "exist", entityExplorer._entityName); EditorNavigation.NavigateToPage("Page1", true); + cy.url().should('include', '/page1'); PageLeftPane.switchSegment(PagePaneSegment.JS); agHelper.GetNClick(locators._editorTab); PageList.VerifyIsCurrentPage("Page1"); EditorNavigation.NavigateToPage("Page2", true); + cy.url().should('include', '/page2'); agHelper.AssertContains("JSObject1", "exist", entityExplorer._entityName); agHelper.GetNClick(locators._editorTab); PageList.VerifyIsCurrentPage("Page2"); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/JSObject/JSObject_Tests_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.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
| it.skip("8. Verify JSObject with identical name should not exist Bug: #35385", () => { | ||
| for (let i = 0; i < 10; i++) { | ||
| agHelper.GetNClick(locators._createNew, 0, true, 0); | ||
| } | ||
| agHelper.AssertElementAbsence(locators._toastMsg); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded sleep with proper assertions
The test uses a loop to create objects but lacks proper assertions between actions. Consider:
- Using
cy.intercept()to wait for network requests - Adding assertions to verify each object creation
it.skip("8. Verify JSObject with identical name should not exist Bug: #35385", () => {
+ cy.intercept('POST', '/api/v1/collections/actions').as('createJSObject');
for (let i = 0; i < 10; i++) {
agHelper.GetNClick(locators._createNew, 0, true, 0);
+ cy.wait('@createJSObject');
+ agHelper.AssertElementExist(`${locators._entityName}:contains("JSObject${i + 1}")`);
}
agHelper.AssertElementAbsence(locators._toastMsg);
});Committable suggestion skipped: line range outside the PR's diff.
…o feat/remove-generate-new-page-button
…o feat/remove-generate-new-page-button
…o feat/remove-generate-new-page-button
…o feat/remove-generate-new-page-button
…o feat/remove-generate-new-page-button
…o feat/remove-generate-new-page-button
jacquesikot
added
ok-to-test
Description
Problem
The generate new page button was previously hidden only when the drag_drop_building_blocks feature flag is turned off. This was the expected behaviour when trying to launch building blocks earlier. Now, building blocks are not been used on the platform and we still want to hide the generate new page button.
Solution
Instead of removing the whole generate a pge functionality, or adding another redundant feature flag, we have decided to remove all instance of the generate new page button in the datasource preview pages. The generate page functionality remains untouched.
Fixes #38178
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12747890502
Commit: 9e4cfaf
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Github/EnableGithub_spec.ts
- cypress/e2e/Regression/ClientSide/PropertyPane/PropertyPane_Search_spec.ts
- cypress/e2e/Regression/ClientSide/VisualTests/JSEditorIndent_spec.js
List of identified flaky tests.Mon, 13 Jan 2025 14:25:30 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
DatasourceViewModeSchemaandGoogleSheetSchemacomponents by removing obsolete imports and functions, focusing on core functionalities.