fix: mandatory date column enforcement #35613
Conversation
jacquesikot
added
Table Widget V2
jacquesikot
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
github-actions
bot
added
Bug
jacquesikot
added
ok-to-test
WalkthroughThe changes enhance the validation logic for the date column in the Table widget, ensuring that required fields are enforced when adding new rows. New Cypress tests validate this functionality, while the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableWidget
participant DateCell
participant Validation
User->>TableWidget: Attempts to add new row (empty date)
TableWidget->>DateCell: Validate date entry
DateCell->>Validation: Check isEditableCellValid
Validation-->>DateCell: Return validation status
DateCell-->>TableWidget: Notify validity
TableWidget-->>User: Display error message (if invalid)
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 Configuration File (
|
![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/AddNewRow2_spec.js (3)
90-96: Avoid usingcy.waitfor better test performance.The use of
cy.wait(500)is not recommended as it can lead to flaky tests. Instead, use assertions to wait for specific conditions.- cy.wait(500); + cy.get(`.t--inlined-cell-editor-has-error`).should("exist");
90-96: Use data- attributes for selectors.*Using class selectors like
.t--dropdown-optionis not recommended. Prefer usingdata-*attributes for more robust and maintainable tests.- cy.get(".t--dropdown-option").children().contains("Date").click(); + cy.get("[data-testid='t--dropdown-option']").children().contains("Date").click();
90-96: Ensure locators use variables.Avoid hardcoding locators. Use locator variables for better maintainability and readability.
const dateDropdownOption = "[data-testid='t--dropdown-option']"; cy.get(dateDropdownOption).children().contains("Date").click();app/client/src/widgets/TableWidgetV2/component/cellComponents/DateCell.tsx (2)
Line range hint
172-199: Ensure prop types are well-defined.The addition of
isEditableCellValidshould be reflected in the prop types to ensure type safety.Ensure that the
DateComponentPropstype includesisEditableCellValidfor better type checking and documentation.
199-199: Improve naming for clarity.Consider renaming
isCellCompletelyValidto something more descriptive, such asisCellValidationComplete, to convey its purpose more clearly.- const isCellCompletelyValid = isEditableCellValid && isValid; + const isCellValidationComplete = isEditableCellValid && isValid;
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow2_spec.js (1 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/DateCell.tsx (3 hunks)
- app/client/src/widgets/TableWidgetV2/widget/derived.js (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow2_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 (1)
app/client/src/widgets/TableWidgetV2/widget/derived.js (1)
854-854: Verify the impact of adding "date" tovalidatableColumns.Adding "date" to the
validatableColumnsarray is a significant change. Ensure that all necessary validation logic for date columns is implemented and tested.
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/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow2_spec.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow2_spec.js
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (9)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow2_spec.js (9)
Line range hint
10-10: Avoid usingcy.wait.Using
cy.waitwith arbitrary timeouts can lead to flaky tests. Consider using more reliable commands likecy.interceptto wait for specific network requests orcy.getwith appropriate assertions.
Line range hint
16-16: Use data- attributes for selectors.*Avoid using class selectors like
.t--inlined-cell-editor-has-error. Instead, usedata-*attributes for more reliable and maintainable selectors.Also applies to: 20-20, 24-24, 28-28, 32-32, 36-36, 40-40
Line range hint
16-16: Use multiple assertions forexpectstatements.Avoid using strings directly in assertions. Use multiple
expectstatements to provide clarity and robustness to the test.Also applies to: 20-20, 24-24, 28-28, 32-32, 36-36, 40-40
Line range hint
107-107: Avoid usingcy.wait.Using
cy.waitwith arbitrary timeouts can lead to flaky tests. Consider using more reliable commands likecy.interceptto wait for specific network requests orcy.getwith appropriate assertions.
Line range hint
107-107: Use data- attributes for selectors.*Avoid using class selectors like
.t--inlined-cell-editor-has-error. Instead, usedata-*attributes for more reliable and maintainable selectors.Also applies to: 111-111, 115-115
Line range hint
125-125: Avoid usingcy.wait.Using
cy.waitwith arbitrary timeouts can lead to flaky tests. Consider using more reliable commands likecy.interceptto wait for specific network requests orcy.getwith appropriate assertions.
Line range hint
125-125: Use data- attributes for selectors.*Avoid using class selectors like
.t--inlined-cell-editor-has-error. Instead, usedata-*attributes for more reliable and maintainable selectors.Also applies to: 129-129
Line range hint
139-139: Avoid usingcy.wait.Using
cy.waitwith arbitrary timeouts can lead to flaky tests. Consider using more reliable commands likecy.interceptto wait for specific network requests orcy.getwith appropriate assertions.Also applies to: 143-143
Line range hint
139-139: Use data- attributes for selectors.*Avoid using class selectors like
.t--inlined-cell-editor-has-error. Instead, usedata-*attributes for more reliable and maintainable selectors.Also applies to: 143-143, 147-147
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow2_spec.js (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow2_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.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_editing_1_spec.ts (3)
Line range hint
18-33:
Avoid usingagHelper.Sleep()in tests.Using
agHelper.Sleep(1000)is not recommended as it can lead to flaky tests. Instead, use Cypress commands likecy.wait()with network requests orcy.get()with assertions to wait for specific conditions.- agHelper.Sleep(1000); + // Consider using a more reliable wait strategy here
Line range hint
118-135:
Avoid usingagHelper.Sleep()in tests.Using
agHelper.Sleep(2000)is not recommended as it can lead to flaky tests. Instead, use Cypress commands likecy.wait()with network requests orcy.get()with assertions to wait for specific conditions.- agHelper.Sleep(2000); + // Consider using a more reliable wait strategy here
Line range hint
137-164:
Avoid usingagHelper.Sleep()in tests.Using
agHelper.Sleep(2000)is not recommended as it can lead to flaky tests. Instead, use Cypress commands likecy.wait()with network requests orcy.get()with assertions to wait for specific conditions.- agHelper.Sleep(2000); + // Consider using a more reliable wait strategy here
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_editing_1_spec.ts (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_editing_1_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 not posted (4)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_editing_1_spec.ts (4)
Line range hint
35-81:
Great use of multiple assertions!The test case effectively checks the date editing functionality with multiple assertions, ensuring comprehensive coverage.
Line range hint
83-99:
Well-structured test case!The test case correctly verifies the functionality of changing the date display format with appropriate assertions.
Line range hint
101-116:
Good job on verifying the date picker's first day of the week!The test case effectively checks the change in the date picker's starting day with clear assertions.
Line range hint
166-205:
Comprehensive test for date cell behavior!The test case effectively checks the behavior of date cells when adding a new row, with a focus on CSS changes. The assertions are well-implemented.
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow2_spec.js
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10365486253. |
|
Deploy-Preview-URL: https://ce-35613.dp.appsmith.com |
…o fix/mandatory-date-column-enforcement
jacquesikot
dismissed stale reviews from rahulbarwal and sagar-qa007
via
874a636
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/component/cellComponents/DateCell.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/DateCell.tsx
…on is handled in DateCell
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10453192428. |
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/index.tsx (1 hunks)
Additional comments not posted (1)
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
2657-2679: Great job on theonDateSavemethod!The method correctly handles the update of transient table data and the execution of onSubmit actions. The use of batch updates ensures efficient state management.
|
Deploy-Preview-URL: https://ce-35613.dp.appsmith.com |
Description
Problem:
When the
isRequiredproperty is set for date columns in the table widget, the validation doesn't work as expected. Users can add new rows without filling in the date field, even though it is marked as required. This results in rows being added with missing date values, which can lead to incomplete or invalid data entries.Root Cause:
The validation logic for the date column is currently handled within the DateCell component. However, the isRequired validation functionality was not implemented within this component. Additionally, the general validation logic in the
getEditableCellValidityfunction, located in the derived.js file, does not account date cells in its isRequired validation.Solution:
To fix this issue:
Enhance the
getEditableCellValidityfunction: Extend the existing validation logic in getEditableCellValidity to include the date cell in its validation, specifically checking for the isRequired property.Integrate with DateCell validation: Ensure that the isRequired validation is properly executed in conjunction with the existing date validations inside the DateCell component. This will enforce the requirement and prevent new rows from being added if the date field is left empty.
Test Plan
https://www.notion.so/appsmith/Test-Plan-Date-Column-Marked-as-Required-Doesn-t-Enforce-Mandatory-Entry-When-Adding-New-Table-Row-c73b764af60842a188cba056bdda6d79?pvs=4
Fixes #34258
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10453174231
Commit: 40fe2ea
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 19 Aug 2024 13:17:23 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes