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
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
💤 Files with no reviewable 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
|
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
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/QueryPane_Postgres_Spec.js (3)
Line range hint
15-19: Remove commented out afterEach blockThe commented out afterEach block with
Cypress.runner.stop()is an anti-pattern. If test failure handling is needed, consider using Cypress's built-in retry and failure handling mechanisms.- // afterEach(function() { - // if (this.currentTest.state === "failed") { - // Cypress.runner.stop(); - // } - // });
Line range hint
91-93: Replace cy.wait with proper assertionsUsing
cy.wait(3000)is an anti-pattern. Instead, wait for specific elements or network requests to complete.- cy.wait(3000); + cy.get('[data-cy="query-response"]').should('exist');
Line range hint
33-43: Use data-cy attributes for selectorsReplace class-based selectors with data-cy attributes for better test stability.
- cy.dragAndDropToCanvas("tablewidgetv2", { x: 100, y: 100 }); + cy.get('[data-cy="widget-tablewidgetv2"]').dragTo('[data-cy="canvas"]', { x: 100, y: 100 }); - agHelper.TypeDynamicInputValueNValidate( - "select * from users limit {{Table1.pageSize}} OFFSET {{((Table1.pageNo - 1)*Table1.pageSize)}}", - ".CodeEditorTarget", + agHelper.TypeDynamicInputValueNValidate( + "select * from users limit {{Table1.pageSize}} OFFSET {{((Table1.pageNo - 1)*Table1.pageSize)}}", + '[data-cy="code-editor"]',
🧹 Nitpick comments (4)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/QueryPane_Postgres_Spec.js (1)
Line range hint
44-147: Improve test isolationThe CRUD tests are tightly coupled and depend on the sequence of execution. Consider:
- Using
beforeEachto set up the test state- Cleaning up data after each test
- Making tests independent of each other
Example approach:
beforeEach(() => { // Setup fresh test data cy.task('db:seed'); }); afterEach(() => { // Clean up test data cy.task('db:cleanup'); });app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_2_spec.ts (3)
Line range hint
29-37: Remove commented-out afterEach blocksThese commented blocks are dead code and should be removed to maintain code cleanliness.
- // afterEach(function() { - // if (this.currentTest.state === "failed") { - // Cypress.runner.stop(); - // } - // }); - - // afterEach(() => { - // if (queryName) - // cy.actionContextMenuByEntityName(queryName); - // });
Line range hint
55-77: Enhance test case robustness and clarityWhile the test follows most best practices, consider these improvements:
- Make the test title more descriptive of what's being tested
- Add assertions to verify widget properties and data after they're added
- it("1. Verify 'Add to widget [Widget Suggestion]' functionality - S3", () => { + it("1. Should successfully add and render suggested widgets from S3 list files query", () => { EditorNavigation.SelectEntityByName("Page1", EntityType.Page); dataSources.CreateQueryForDS(datasourceName); agHelper.GetObjectName().then(($queryName) => { dataSources.ValidateNSelectDropdown("Command", "List files in bucket"); agHelper.UpdateCodeInput(formControls.s3BucketName, bucketName); dataSources.RunQuery(); dataSources.AddSuggestedWidget(Widgets.Dropdown); + // Add assertions for dropdown widget + agHelper.AssertElementExist("data-testid='dropdown-widget'"); propPane.DeleteWidgetDirectlyFromPropertyPane(); EditorNavigation.SelectEntityByName($queryName, EntityType.Query); dataSources.AddSuggestedWidget(Widgets.Table); table.WaitUntilTableLoad(0, 0, "v2"); + // Add assertions for table widget + agHelper.AssertElementExist("data-testid='table-widget'"); + agHelper.AssertElementVisible("data-testid='table-column-headers'");
Line range hint
80-82: Improve cleanup documentationThe comment about the 409 status code could be more descriptive to help other developers understand the expected behavior.
- dataSources.DeleteDatasourceFromWithinDS(datasourceName, 409); //since crud page is still active + dataSources.DeleteDatasourceFromWithinDS(datasourceName, 409); // Expect 409 Conflict as the datasource is still in use by the active CRUD page
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts(4 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/QueryPane_Postgres_Spec.js(1 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_Spec.js(2 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js(0 hunks)app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_2_spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_1_spec.js
🧰 Additional context used
📓 Path-based instructions (4)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_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/QueryPane/QueryPane_Postgres_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/QueryPane/S3_2_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/Mongo1_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/Querypane_Mongo_Spec.js (2)
9-13: LGTM!
The import reordering maintains alphabetical order and improves readability.
241-241: Test case renumbering aligns with PR objectives.
The test case has been renumbered from 10 to 6, reflecting the removal of page generation related tests, which aligns with the PR objective of removing the "generate new page" functionality.
app/client/cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts (1)
768-770: Test case renumbering is consistent with PR objectives.
The test cases have been correctly renumbered after removing page generation related tests:
- Test 15 (previously 17): Validate Drop of Collection
- Test 16 (previously 18): Verify wrong collection handling
- Test 17 (previously 19): Verify date parsing
- Test 18 (previously 20): Verify datasource deletion
This change maintains test suite consistency while removing page generation functionality.
Also applies to: 787-789, 807-809, 914-915
jacquesikot
added
the
Widgets & Accelerators Pod
| dataSources.GeneratePageForDS(dsName); | ||
| agHelper.GetNClick(dataSources._selectTableDropdown, 0, true); | ||
| agHelper.GetNClickByContains(dataSources._dropdownOption, "coffeeCafe"); | ||
| GenerateCRUDNValidateDeployPage("", "", "Washington, US", 11); |
There was a problem hiding this comment.
I see that at the bottom of this file we have definition of this function GenerateCRUDNValidateDeployPage. Any particular reason, why we are not removing the implementation?
There was a problem hiding this comment.
It is used in the test case above
There was a problem hiding this comment.
@rahulbarwal @jacquesikot why we did remove this cases ? Are we removing feature here or covering cases somewhwere else?
| const shouldShowSecondaryGenerateButton = releaseDragDropBuildingBlocks | ||
| ? false | ||
| : !!isPluginAllowedToPreviewData; | ||
| const shouldShowSecondaryGenerateButton = !!isPluginAllowedToPreviewData; |
There was a problem hiding this comment.
Should this not go away as well?
I mean technically, there is no button to show or hide, right? Am I missing something here?
…o feat/remove-generate-new-page-button
…o feat/remove-generate-new-page-button
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_2_spec.ts (2)
Line range hint
27-35: Remove commented-out afterEach blocksClean up the code by removing the commented-out afterEach blocks as they are no longer needed and add noise to the codebase.
Line range hint
55-80: Enhance test reliability with additional assertionsWhile the test follows best practices, consider adding assertions to verify:
- The widget's presence after addition
- The correct data binding in the widgets
- The successful deletion of widgets
Example addition:
dataSources.AddSuggestedWidget(Widgets.Table); +agHelper.AssertElementExist(table.table); +agHelper.AssertElementVisible(table.tableRow); table.WaitUntilTableLoad(0, 0, "v2");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_2_spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ServerSide/QueryPane/S3_2_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/Regression/ServerSide/QueryPane/S3_2_spec.ts (1)
81-81: Verify the expected response code change
The deletion response code expectation has changed from 409 to 200. Please verify this is the correct expectation when the CRUD page is not active.
✅ Verification successful
Let me search for instances where the response code is 409 to understand the change better.
Based on the search results, I can see a clear pattern in the usage of DeleteDatasourceFromWithinDS. The response code 409 is consistently used when there are active queries, pages, or other dependencies using the datasource, while 200 is used when there are no active dependencies.
For example:
409- "Since all queries exists", "Since crud page is active", "Cannot delete datasource since it has 1 query using it"200- "since crud page is not active", no active dependencies mentioned
Response code change from 409 to 200 is correct
The change in response code from 409 to 200 in S3_2_spec.ts is appropriate since the comment indicates "crud page is not active", meaning there are no active dependencies preventing the deletion of the datasource.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of DeleteDatasourceFromWithinDS to verify consistent response code expectations
rg -A 2 "DeleteDatasourceFromWithinDS.*200" --type ts
Length of output: 6154
Script:
#!/bin/bash
# Search for other instances of DeleteDatasourceFromWithinDS with 409 response code
rg -A 2 "DeleteDatasourceFromWithinDS.*409" --type ts
# Also search for the implementation of DeleteDatasourceFromWithinDS to understand the response codes
ast-grep --pattern 'DeleteDatasourceFromWithinDS($$$)'
Length of output: 75331
| dataSources.GeneratePageForDS(dsName); | ||
| agHelper.GetNClick(dataSources._selectTableDropdown, 0, true); | ||
| agHelper.GetNClickByContains(dataSources._dropdownOption, "coffeeCafe"); | ||
| GenerateCRUDNValidateDeployPage("", "", "Washington, US", 11); |
There was a problem hiding this comment.
@rahulbarwal @jacquesikot why we did remove this cases ? Are we removing feature here or covering cases somewhwere else?
jacquesikot
added
ok-to-test
jacquesikot
dismissed
sagar-qa007’s stale review
We are removing the feature from there
|
/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
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/12501844185
Commit: d1aeda9
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.js
- cypress/e2e/Regression/ClientSide/VisualTests/JSEditorIndent_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/Button/Button2_spec.ts
- cypress/e2e/Regression/ServerSide/ApiTests/API_Edit_spec.js
- cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js
List of identified flaky tests.Thu, 26 Dec 2024 10:01:11 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.