fix: filteredTableData property in autocomplete suggestions #35856
Conversation
WalkthroughThe changes introduce a new Changes
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: 0
Outside diff range, codebase verification and nitpick comments (5)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/check_filterTableData_auto_spec.js (5)
1-2: Use ES6 import for locators.Consider using ES6 import syntax for importing locators instead of
require.Here's how you can do it:
-import * as _ from "../../../../../support/Objects/ObjectsCore"; -const dynamicInputLocators = require("../../../../../locators/DynamicInput.json"); +import * as _ from "../../../../../support/Objects/ObjectsCore"; +import dynamicInputLocators from "../../../../../locators/DynamicInput.json";
55-58: Ensure meaningful test suite tags.The tags should be meaningful and help in categorizing the test suite effectively. Ensure that they align with your project's tagging strategy.
59-76: Avoid using plain strings for locators.Use locator variables instead of plain strings for better maintainability and readability.
Here's how you can improve it:
- cy.get(`${dynamicInputLocators.hints} li`) + cy.get(dynamicInputLocators.hintsList)Ensure
hintsListis defined in your JSON file.
59-76: Enhance assertions with multiple checks.Consider adding multiple assertions to ensure the robustness of your test case. For example, check the visibility and text of the suggestion.
Here's an example:
cy.get(dynamicInputLocators.hintsList) .should('be.visible') .and('contain.text', 'filteredTableData');
60-63: Consider adding teardown steps.Ensure that the test environment is reset after the test execution to avoid side effects on other tests.
You might want to add cleanup steps at the end of your test case.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/check_filterTableData_auto_spec.js (1 hunks)
- app/client/src/widgets/TableWidgetV2/widget/index.tsx (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/check_filterTableData_auto_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.
Additional comments not posted (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/check_filterTableData_auto_spec.js (1)
1-77: Adhere to Cypress best practices.Ensure that the test script adheres to Cypress best practices, such as avoiding
cy.waitand usingdata-*attributes for selectors.Review the entire test script to ensure compliance with Cypress best practices.
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
477-477: Great addition offilteredTableData!The introduction of
filteredTableDatato the autocomplete definitions enhances the developer experience by providing more precise suggestions. Ensure thatgenerateTypeDefis correctly handlingwidget.filteredTableData.Would you like to verify the usage of
generateTypeDefwithfilteredTableDatain the codebase?
|
@Pavan134 Can you please explain the requirement for cypress test case here? Also please add equivalent unit test cases. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
app/client/src/widgets/TableWidgetV2/checkFilteredDataProperty.test.ts (1)
16-19: Add comments to explain the purpose of each section.Adding comments to explain the purpose of each section in the test case will make it easier for others to understand the test case.
Here's an example of how you can add comments:
it("check filteredTableData property", () => { // Register the TableWidget registerWidgets([TableWidget]); // Define the properties for the TableWidget const tableWidgetProps: TableWidgetProps = { // ... (properties) }; // Create the data tree entity const dataTreeEntity: DataTreeEntityObject = { ENTITY_TYPE: ENTITY_TYPE.WIDGET, meta: {}, ...tableWidgetProps, }; // Define the data tree entity configuration const dataTreeEntityConfig: WidgetEntityConfig = { // ... (configuration) }; // Generate definitions and entity information const { def, entityInfo } = dataTreeTypeDefCreator( { Table1: dataTreeEntity, }, {}, dataTreeEntityConfig ); // Assert that the filteredTableData property exists expect(def).toHaveProperty("Table1.filteredTableData"); // Verify the entity information expect(entityInfo.get("Table1")).toStrictEqual({ type: ENTITY_TYPE.WIDGET, subType: "TABLE_WIDGET_V2", }); });
app/client/src/widgets/TableWidgetV2/checkFilteredDataProperty.test.ts
Outdated
Show resolved
Hide resolved
Pavan134
force-pushed
the
bug-filtered-table-data-property-of-table-widget-doesnt-show-up-on-auto-complete
branch
from
0f95934
to
2d39d27
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/widget/checkFilteredDataProperty.test.ts (1 hunks)
Additional comments not posted (4)
app/client/src/widgets/TableWidgetV2/widget/checkFilteredDataProperty.test.ts (4)
1-14: Good job on the imports!The necessary modules and types are correctly imported.
The code changes are approved.
15-15: Well-named test suite!The test suite name clearly indicates the purpose of the test.
The code changes are approved.
16-18: Great start with the test case!The test case correctly registers the
TableWidgetand sets up the initial properties.The code changes are approved.
158-199: Excellent verification of thefilteredTableDataproperty!The test case correctly verifies the presence of the
filteredTableDataproperty in the autocomplete definitions and the entity information.The code changes are approved.
app/client/src/widgets/TableWidgetV2/widget/checkFilteredDataProperty.test.ts
Outdated
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/widget/checkFilteredDataProperty.test.ts
Outdated
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/widget/checkFilteredDataProperty.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/widget/checkFilteredDataProperty.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/widget/checkFilteredDataProperty.test.ts
|
Hi @sagar-qa007 Gentle Remainder: Thanks for reviewing.I have added unit test cases. Can you please review it? |
|
@rahulbarwal Please check this PR. Let's plan this as shadow PR. |
| cy.get(`${dynamicInputLocators.hints} li`).contains("filteredTableData").should('be.visible').click(); | ||
|
|
||
| }) | ||
| }); |
There was a problem hiding this comment.
Please remove this red changes. Linter error. Kindly run linter.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hii @sagar-qa007 I created cypress test file for checking the 'filteredTableData' property is present in auto complete section.If the cypress file is not required can I delete the file. |
|
@Pavan134 what is the issue number that this PR is fixing. I don't see any linked issue. |
|
@rahulbarwal This is the issue link |
|
@Pavan134 a spec is failing due to new changes in the PR. It needs to be updated. Please fix it and let me know if you have any questions. |
app/client/src/widgets/TableWidgetV2/widget/checkFilteredDataProperty.test.ts
Show resolved
Hide resolved
|
@rahulbarwal If everything is ok can you review and merge this pr |
|
@Pavan134 we are revisiting the validity of this issue here: #26097 (comment) We might not need this fix at all. We appreciate your effort and will keep you updated. |
Cypress test execution:
Description
Summary by CodeRabbit
Summary by CodeRabbit
New Features
filteredTableDataproperty to enhance data handling in the Table Widget.Bug Fixes
Tests
filteredTableDataproperty to ensure correct behavior within autocomplete definitions.