chore: Turn off Allow filtering property on Table widget#34593
chore: Turn off Allow filtering property on Table widget#34593jacquesikot merged 8 commits intoreleasefrom
Conversation
WalkthroughThe primary change in this update involves disabling the "Allow filtering" property by default for the Changes
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9716053198. |
|
Deploy-Preview-URL: https://ce-34593.dp.appsmith.com |
github-actions
bot
added
Widgets Product
carinanfonseca
changed the title
github-actions
bot
added
the
skip-changelog
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/widget/index.tsx (1 hunks)
Additional context used
Biome
app/client/src/widgets/TableWidgetV2/widget/index.tsx
[error] 274-276: 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)
[error] 473-473: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 650-650: 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)
[error] 693-695: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 722-722: 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)
[error] 788-788: 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)
[error] 1935-1935: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 1936-1936: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 2147-2155: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 2178-2236: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 2190-2233: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 2207-2209: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 2480-2480: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 2866-2866: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 2876-2878: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
Additional comments not posted (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
213-213: Change aligns with PR objective.The property
isVisibleFiltershas been correctly updated tofalsein thegetDefaultsmethod, turning off the "Allow filtering" property by default.
rahulbarwal
added
ok-to-test
|
@rahulbarwal, don't we have a test to check defaults? If yes, won't this break that test? |
Yes @nerbos, In the initial tests run none of them failed which was surprising. In the second run we have 8 failing. @jacquesikot will take over this PR to ensure everything passes. |
@rahulbarwal the tests that failed where jest or cypress tests? |
@nerbos Cypress tests are failing. You can check the description of this PR. |
|
@rahulbarwal is it worth having a jest to test defaults? Or this is not a practice in Engineer? |
…o chore/change-filters-visibility-default
jacquesikot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (2 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_2_Spec.ts (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_1_Spec.ts (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_2_Spec.ts (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_DisplayText_spec.ts (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/server_side_filtering_spec_1.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_DisplayText_spec.ts
Additional context used
Path-based instructions (7)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/server_side_filtering_spec_1.ts (1)
Pattern
**/*.*: Do not use cy.wait in code.
Do call the add function.
Do not keep duplicate lines in code.
Descriptive test names are used to clearly convey the intent of each test.app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1)
Pattern
**/*.*: Do not use cy.wait in code.
Do call the add function.
Do not keep duplicate lines in code.
Descriptive test names are used to clearly convey the intent of each test.app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js (1)
Pattern
**/*.*: Do not use cy.wait in code.
Do call the add function.
Do not keep duplicate lines in code.
Descriptive test names are used to clearly convey the intent of each test.app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js (1)
Pattern
**/*.*: Do not use cy.wait in code.
Do call the add function.
Do not keep duplicate lines in code.
Descriptive test names are used to clearly convey the intent of each test.app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_2_Spec.ts (1)
Pattern
**/*.*: Do not use cy.wait in code.
Do call the add function.
Do not keep duplicate lines in code.
Descriptive test names are used to clearly convey the intent of each test.app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_2_Spec.ts (1)
Pattern
**/*.*: Do not use cy.wait in code.
Do call the add function.
Do not keep duplicate lines in code.
Descriptive test names are used to clearly convey the intent of each test.app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_1_Spec.ts (1)
Pattern
**/*.*: Do not use cy.wait in code.
Do call the add function.
Do not keep duplicate lines in code.
Descriptive test names are used to clearly convey the intent of each test.
Additional comments not posted (7)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/server_side_filtering_spec_1.ts (1)
37-38: Enable filtering explicitly for the table widget.The code correctly turns on filtering for the table widget as it is disabled by default in this PR.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1)
19-20: Enable filtering explicitly for the table widget.The code correctly turns on filtering for the table widget as it is disabled by default in this PR.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js (1)
43-44: Enable filtering explicitly for the table widget.The code correctly turns on filtering for the table widget as it is disabled by default in this PR.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js (1)
37-38: Enable filtering explicitly for the table widget.The code correctly turns on filtering for the table widget as it is disabled by default in this PR.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_2_Spec.ts (1)
19-20: Enable filtering explicitly for the table widget.The code correctly turns on filtering for the table widget as it is disabled by default in this PR.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_2_Spec.ts (1)
18-19: LGTM! The change aligns with the PR objective.The addition of code to enable filtering for the table aligns with the PR's objective to disable filtering by default. The rest of the function remains consistent with the test case.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_1_Spec.ts (1)
19-20: LGTM! The change aligns with the PR objective.The addition of code to enable filtering for the table aligns with the PR's objective to disable filtering by default. The rest of the function remains consistent with the test case.
jacquesikot
added
ok-to-test
jacquesikot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/checkboxCell_spec.js
jacquesikot
added
ok-to-test
…#34593) appsmithorg#34591 ## Description Making _Allow filtering_ option false by default. <img width="798" alt="Allow filtering off" src="https://github.com/appsmithorg/appsmith/assets/121817440/8ac64773-2385-433d-96fa-0a90080e2462"> Fixes appsmithorg#34591 _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.Table, @tag.Widget, @tag.Binding, @tag.Sanity" ### 🔍 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/9795462304> > Commit: e0c1e83 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9795462304&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Table, @tag.Widget, @tag.Binding, @tag.Sanity` <!-- 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 - **Bug Fixes** - Filtering is now disabled by default for the `TableWidgetV2`. - **Tests** - Updated test scripts to enable filtering for `TableWidgetV2` in multiple scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Jacques Ikot <jacquesikot@gmail.com>
4 participants
#34591
Description
Making Allow filtering option false by default.
Fixes #34591
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.Table, @tag.Widget, @tag.Binding, @tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9795462304
Commit: e0c1e83
Cypress dashboard.
Tags:
@tag.Table, @tag.Widget, @tag.Binding, @tag.SanityCommunication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
TableWidgetV2.Tests
TableWidgetV2in multiple scenarios.