fix:ensure save row button is disabled when date column is required #34711

saiprabhu-dandanayak · 2024-07-04T08:44:49Z

Description

Bug : Table date column bug

I have raised this PR In-order to ensure that the save row button is disabled when the date column is set to required

📸 Screenshot

when department and doj are set to required save row is disabled

when department has value and doj doesn't have , still the save row button is enabled

when department and doj are set to required save row button is disabled and doj column indicating error

when department has value and doj doesn't have , still the save row button is disabled

when both the fieilds have values save row button is enabled

📸 Cypress Testing Video

AddNewRow_Date_Column_Validation_Spec.js.mp4

Caution

If you modify the content in this section, you are likely to disrupt the CI result for your PR.

Communication

Should the DevRel and Marketing teams inform users about this change?

Yes
No

Summary by CodeRabbit

New Features
- Added date validation for "date" type columns in the table widget.
Bug Fixes
- Improved date formatting in date picker cells.
Improvements
- Simplified page size calculation for better performance.

coderabbitai · 2024-07-04T08:44:57Z

Walkthrough

The updates to the TableWidgetV2 widget focus on enhancing date handling and validation. The DateCell component now uses a useEffect hook to validate dates, formats dates upon selection, and checks validity when the popover closes. In derived.js, the logic for calculating page size is streamlined, and date columns are now included in the validation process.

Changes

Files	Change Summaries
`app/client/src/widgets/TableWidgetV2/component/cellComponents/DateCell.tsx`	Added `useEffect` for validation, formatted date in `onDateSelected`, and checked validity on popover close.
`app/client/src/widgets/TableWidgetV2/widget/derived.js`	Simplified `getPageSize` logic and included "date" in `validatableColumns`.

Poem

In the rows of data, dates now glow bright,
Validation keeps them just right.
A tweak to the size, a simpler guise,
Now tables shine with clearer skies.
Widgets of change, a rabbit's code delight,
Enhancing functions, day and night.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

github-actions · 2024-07-11T16:35:55Z

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6c48a39 and 9562033.

Files selected for processing (2)

app/client/src/widgets/TableWidgetV2/component/cellComponents/DateCell.tsx (6 hunks)
app/client/src/widgets/TableWidgetV2/widget/derived.js (4 hunks)

Additional comments not posted (5)

app/client/src/widgets/TableWidgetV2/component/cellComponents/DateCell.tsx (3)

1-1: LGTM!

The inclusion of useEffect hook is necessary for the newly added hook.

199-205: LGTM!

The useEffect hook correctly handles the validation based on isRequired and value.

Line range hint 224-239:
LGTM!

The changes ensure correct date formatting and validation in onDateSelected and correct state update in onPopoverClosed.

Also applies to: 253-255

app/client/src/widgets/TableWidgetV2/widget/derived.js (2)

788-788: LGTM!

Including the date type in validatableColumns ensures that date columns are validated.

Line range hint 840-846:
LGTM!

The additional validations correctly handle min and max constraints for date type columns.

saiprabhu-dandanayak · 2024-07-16T03:51:17Z

Actionable comments posted: 0

Review details

Added a comment on the issue to review it.

…smith into bug-date-column-set-as-required-does-not-making-it-mandatory-for-adding-new-table-row-34258

rahulbarwal · 2024-07-16T11:31:20Z

@saiprabhu-dandanayak The assets in PR description are not loading for me. Can you please re-upload the images and video?

saiprabhu-dandanayak · 2024-07-16T11:36:28Z

@saiprabhu-dandanayak The assets in PR description are not loading for me. Can you please re-upload the images and video?

Hi @rahulbarwal , i have attached the same

could you pls , check them once.

rahulbarwal

Functionally seems fine. Can you please also add some tests for these change?

app/client/src/widgets/TableWidgetV2/widget/derived.js

rahulbarwal · 2024-07-22T08:13:18Z

app/client/src/widgets/TableWidgetV2/component/cellComponents/DateCell.tsx

@@ -241,8 +250,9 @@ export const DateCell = (props: DateComponentProps) => {
  };

  const onPopoverClosed = () => {
+    const isValidValue = value !== undefined && value !== null;


Suggested change

const isValidValue = value !== undefined && value !== null;

const isValidValue = !!value;

Also Don't we have to consider isRequired here?

we have to considering isRequired also

Also, Due to this check we are unable to put any value if the col already did not have a value.
Steps:

drop table

Add data source with date col and it not having some data in some cols.

Make editable, type:date and set required.

Now, edit the col which does not have a value.

Expected: should be able to add new date value.
Actual, nothing happens; datepicker does not close.
Video:
https://github.com/user-attachments/assets/45309303-ca35-4db6-9a2e-f54d1c392875

Hi @rahulbarwal , will look into this and update the pr with expected results.

saiprabhu-dandanayak · 2024-07-22T08:37:56Z

Functionally seems fine. Can you please also add some tests for these change?

Hi @rahulbarwal , will update the pr , with tests , for the current changes done.

rahulbarwal · 2024-07-25T06:19:31Z

@saiprabhu-dandanayak any updates here?

saiprabhu-dandanayak · 2024-07-25T06:20:36Z

Working on testcases @rahulbarwal

rahulbarwal · 2024-07-30T12:41:16Z

@saiprabhu-dandanayak
Hey checking up on this again. Are you still working on this? or can we take this ahead on our own.
If you are stuck, can we assist you with something?
cc: @jacquesikot

saiprabhu-dandanayak · 2024-07-30T12:45:34Z

@saiprabhu-dandanayak Hey checking up on this again. Are you still working on this? or can we take this ahead on our own. If you are stuck, can we assist you with something? cc: @jacquesikot

Hi @rahulbarwal , it is working fine for new row , but not for existing one , i am working on it.

jacquesikot · 2024-08-02T10:04:51Z

@saiprabhu-dandanayak Is there anyway I can help with this? Are there any specific areas in the code you are struggling with?

saiprabhu-dandanayak · 2024-08-05T05:19:57Z

Hi @jacquesikot , when we add date in validatedcolumns i.e here and adding relevant switch case , for newly added row save row button is being disabled but , for the existing rows , we are unable to add a date.

jacquesikot · 2024-08-05T12:24:18Z

@saiprabhu-dandanayak I will look into this today. Thanks

jacquesikot · 2024-08-05T17:29:05Z

Hello @saiprabhu-dandanayak I don't understand why we need to make changes in the derived.js file. I have made one change in the DateCell.tsx component, and it seems to solve all the problems reported without breaking the existing logic for existing cell values. I added

  useEffect(() => {
    if (isNewRow && isRequired && !value) {
      setIsValid(false);
    } else {
      setIsValid(true);
    }
  }, [value, isRequired, isNewRow]);

No other change was required, check this branch when you get the chance - fix/disable-new-row-when-required-table-date-validation

Let me know if this solves your problems. If it does, you can go ahead with tests and prop the final PR for review. Thanks again for the effort.

saiprabhu-dandanayak · 2024-08-05T17:32:48Z

Hi @jacquesikot , i have added useEffect in datecell component to check empty values and added date to validatedcells in derived.js file.

jacquesikot · 2024-08-05T17:34:07Z

@saiprabhu-dandanayak Can you check my branch and see if my change solves the issues? I don't think you need to add date in validateCells in derived.js file

saiprabhu-dandanayak · 2024-08-05T17:40:19Z

Hi @jacquesikot , thanks for your support and will checkout to your branch and check functionality and move ahead with testcases , can you pls confirm check #29662 this issue , i am not able to reproduce. if it is already fixed pls close the issue.
Thank you.

jacquesikot · 2024-08-05T17:50:22Z

@saiprabhu-dandanayak To the best of my knowledge, the issues has not been fixed. I will try to reproduce on my end and send a loom video if i can.

jacquesikot · 2024-08-05T18:24:06Z

@saiprabhu-dandanayak I am also not able to reproduce, I will check the status of this and update you. Thanks

saiprabhu-dandanayak · 2024-08-05T18:24:52Z

Ok @jacquesikot

jacquesikot · 2024-08-06T10:31:14Z

Hey @saiprabhu-dandanayak any update? need any more help?

saiprabhu-dandanayak · 2024-08-06T10:32:52Z

Hi @jacquesikot , I haven't actually checked it , will check and let you know. Thank you.

…smith into bug-date-column-set-as-required-does-not-making-it-mandatory-for-adding-new-table-row-34258

saiprabhu-dandanayak · 2024-08-06T16:54:00Z

Hi @jacquesikot , after incorporating your chnages the issue still not yet resolved , below is the screenshot for your reference.

Screenshot

jacquesikot · 2024-08-07T08:27:07Z

@saiprabhu-dandanayak Did you check my branch to be sure it works appropriately there? The change I recommended is a standalone change, it should not be included with any other changes

jacquesikot · 2024-08-07T22:17:28Z

@saiprabhu-dandanayak Please review my branch again, I misunderstood your requirements, I have made some changes to the derived.js file within that branch, check if this solves your problems completely.

saiprabhu-dandanayak · 2024-08-08T02:44:08Z

Ok @jacquesikot , will check and let you know

jacquesikot · 2024-08-08T07:29:48Z

@saiprabhu-dandanayak Due to our priorities, I will be picking this issue up, I will update the PR i sent with tests and you can review when you get the chance

…smith into bug-date-column-set-as-required-does-not-making-it-mandatory-for-adding-new-table-row-34258

jacquesikot · 2024-08-23T09:27:24Z

@saiprabhu-dandanayak Thanks for helping with this, here is the merged PR that fixed the issue - #35613

fix:ensure save row button is disabled when date column is required

9562033

github-actions bot added the Stale label Jul 11, 2024

coderabbitai bot reviewed Jul 11, 2024

View reviewed changes

Nikhil-Nandagopal requested a review from rahulbarwal July 16, 2024 06:28

Merge branch 'release' of https://github.com/saiprabhu-dandanayak/app…

3b96a12

…smith into bug-date-column-set-as-required-does-not-making-it-mandatory-for-adding-new-table-row-34258

github-actions bot removed the Stale label Jul 16, 2024

rahulbarwal requested changes Jul 22, 2024

View reviewed changes

rahulbarwal assigned jacquesikot Aug 5, 2024

jacquesikot self-requested a review August 5, 2024 10:19

Merge branch 'release' of https://github.com/saiprabhu-dandanayak/app…

e53ec43

…smith into bug-date-column-set-as-required-does-not-making-it-mandatory-for-adding-new-table-row-34258

saiprabhu-dandanayak added 2 commits August 11, 2024 09:50

Merge branch 'release' of https://github.com/saiprabhu-dandanayak/app…

5d6fc8e

…smith into bug-date-column-set-as-required-does-not-making-it-mandatory-for-adding-new-table-row-34258

chore: took recent changes from release

11bb4d2

jacquesikot closed this Aug 23, 2024

	const isValidValue = value !== undefined && value !== null;
	const isValidValue = !!value;

fix:ensure save row button is disabled when date column is required #34711

fix:ensure save row button is disabled when date column is required #34711

Conversation

saiprabhu-dandanayak commented Jul 4, 2024 • edited by coderabbitai bot Loading

Description

Bug : Table date column bug

📸 Screenshot

when department and doj are set to required save row is disabled

when department has value and doj doesn't have , still the save row button is enabled

when department and doj are set to required save row button is disabled and doj column indicating error

when department has value and doj doesn't have , still the save row button is disabled

when both the fieilds have values save row button is enabled

📸 Cypress Testing Video

Communication

Summary by CodeRabbit

coderabbitai bot commented Jul 4, 2024 • edited Loading

Walkthrough

Changes

Poem

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

github-actions bot commented Jul 11, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

saiprabhu-dandanayak commented Jul 16, 2024

rahulbarwal commented Jul 16, 2024

saiprabhu-dandanayak commented Jul 16, 2024

rahulbarwal left a comment

Choose a reason for hiding this comment

rahulbarwal Jul 22, 2024

Choose a reason for hiding this comment

saiprabhu-dandanayak Jul 22, 2024

Choose a reason for hiding this comment

rahulbarwal Jul 22, 2024

Choose a reason for hiding this comment

saiprabhu-dandanayak Jul 22, 2024

Choose a reason for hiding this comment

saiprabhu-dandanayak commented Jul 22, 2024

rahulbarwal commented Jul 25, 2024

saiprabhu-dandanayak commented Jul 25, 2024

rahulbarwal commented Jul 30, 2024

saiprabhu-dandanayak commented Jul 30, 2024

jacquesikot commented Aug 2, 2024

saiprabhu-dandanayak commented Aug 5, 2024

jacquesikot commented Aug 5, 2024

jacquesikot commented Aug 5, 2024

saiprabhu-dandanayak commented Aug 5, 2024

jacquesikot commented Aug 5, 2024 • edited Loading

saiprabhu-dandanayak commented Aug 5, 2024 • edited Loading

jacquesikot commented Aug 5, 2024

jacquesikot commented Aug 5, 2024

saiprabhu-dandanayak commented Aug 5, 2024

jacquesikot commented Aug 6, 2024

saiprabhu-dandanayak commented Aug 6, 2024

saiprabhu-dandanayak commented Aug 6, 2024

Screenshot

jacquesikot commented Aug 7, 2024

jacquesikot commented Aug 7, 2024

saiprabhu-dandanayak commented Aug 8, 2024

jacquesikot commented Aug 8, 2024

jacquesikot commented Aug 23, 2024

saiprabhu-dandanayak commented Jul 4, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 4, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

jacquesikot commented Aug 5, 2024 •

edited

Loading

saiprabhu-dandanayak commented Aug 5, 2024 •

edited

Loading