fix: Invalid Date Display in Table Widget's Date Column When Using Unix Timestamp (ms) #36455
Conversation
…o fix/incorrect-selected-date-in-table-date-cell
jacquesikot
added
ok-to-test
jacquesikot
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe pull request introduces enhancements to the date column editing functionality in a table widget. It adds a test case that verifies the acceptance of Unix timestamps in milliseconds, ensuring they do not trigger errors during inline editing. Additionally, modifications are made to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Bug
![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 and nitpick comments (1)
app/client/src/widgets/TableWidgetV2/constants.ts (1)
222-224: New enumMomentDateInputFormatadded for Moment.js timestamp formatAlright class, let's take a look at this new enum that's been introduced. It seems we have a
MomentDateInputFormatenum with aMILLISECONDSmember assigned the value"x".Now, it's important to note that we already have a
DateInputFormatenum with aMILLISECONDSmember, but with a different string value. While this new enum itself doesn't change any existing code, we need to be cautious about the potential for confusion due to the similar naming.Can you explain the specific use case for introducing this new enum and constant? It would be helpful for the class to understand the context and how it fits into the overall solution.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_editing_2_spec.ts (1 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/DateCell.tsx (3 hunks)
- app/client/src/widgets/TableWidgetV2/constants.ts (1 hunks)
- app/client/src/widgets/TableWidgetV2/widget/reactTableUtils/transformDataPureFn.tsx (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_editing_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 not posted (4)
app/client/src/widgets/TableWidgetV2/widget/reactTableUtils/transformDataPureFn.tsx (1)
47-50: Great work on refining the date validation logic! 👍Class, let's take a closer look at this code segment. The condition
!isNumber(value) && type !== DateInputFormat.MILLISECONDSis checking if thevalueis not a number and thetypeis notDateInputFormat.MILLISECONDS. If both conditions are true, it setsisValidDatetofalse.This change ensures that only non-millisecond date formats are subjected to the number check, enhancing the specificity of the validation logic for different date formats. It prevents incorrect validation of millisecond date formats.
Keep up the excellent work in improving the accuracy of date validation! 🎉
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Date_column_editing_2_spec.ts (1)
96-203: Great job on this new test case, class! 👏The test case is well-structured and follows a logical sequence of steps to verify the handling of Unix timestamps in milliseconds for the date column editing functionality. It covers all the essential aspects, including updating the table data, changing the column type, configuring the date format properties, and performing inline editing.
I appreciate that you've used appropriate locators and assertions to interact with the table widget and validate the expected behavior. The code also adheres to the Cypress best practices and coding guidelines, such as using locator variables, avoiding unnecessary waits, and performing assertions.
Keep up the excellent work! 🌟
app/client/src/widgets/TableWidgetV2/component/cellComponents/DateCell.tsx (2)
12-16: Excellent work importing the necessary constants! 👍The
DateInputFormatandMomentDateInputFormatimports from thewidgets/TableWidgetV2/constantsmodule are essential for the changes made in theonDateSelectedfunction. Keep up the good work!
Line range hint
225-247: Great job enhancing the date formatting logic! 🎉The introduction of the
momentAdjustedInputFormatvariable and the conditional assignment based on theinputFormatvalue is a clever way to handle specific formatting requirements for millisecond input formats. By mappingDateInputFormat.MILLISECONDStoMomentDateInputFormat.MILLISECONDS, you ensure that the date is formatted correctly using the appropriate moment format string.Using
momentAdjustedInputFormatin the date formatting logic instead of directly usinginputFormatprovides flexibility and allows for proper handling of different input format types. This change will definitely improve the accuracy and consistency of date formatting in theDateCellcomponent.Well done on implementing this enhancement! Your attention to detail and understanding of the formatting requirements is commendable. Keep up the excellent work! 👏
| // Get string format for tomorrow date - Sat Sep 21 2024 format | ||
| const tomorrow = new Date(); | ||
| tomorrow.setDate(tomorrow.getDate() + 1); | ||
| const formattedTomorrowDateVerbose = tomorrow |
There was a problem hiding this comment.
Can we please add this in helper function with better parameters to make a good usage of it?
| propPane.UpdatePropertyFieldValue( | ||
| "Table data", | ||
| ` | ||
| {{ |
There was a problem hiding this comment.
Can we please existing fixture here? or atleast add this in fixture folder and update in test.
| momentAdjustedInputFormat = MomentDateInputFormat.MILLISECONDS; | ||
| const formattedDate = date | ||
| ? moment(date).format(momentAdjustedInputFormat) | ||
| : ""; |
There was a problem hiding this comment.
Query : Blank value return means error will come or what will be the output?
There was a problem hiding this comment.
empty value will come. essentially, the date itself is not provided, it is a falsey value(null/undefined/""/0)
| if ( | ||
| type !== DateInputFormat.EPOCH && | ||
| type !== DateInputFormat.MILLISECONDS | ||
| ) { | ||
| inputFormat = type; | ||
| moment(value as MomentInput, inputFormat); | ||
| } else if (!isNumber(value)) { | ||
| } else if ( |
There was a problem hiding this comment.
This condition and the condition above is confusing, can we simplify this logic?
Also, the top of this function there is a comment stating the need to unit test this, while we are at it, lets take care of that as well.
One suggestion, right now the variable type is of any, which is not ideal, either declare an interface for it, or explain in comments what all values can it assume.
| if (inputFormat === DateInputFormat.MILLISECONDS) | ||
| momentAdjustedInputFormat = MomentDateInputFormat.MILLISECONDS; |
There was a problem hiding this comment.
What is rationale behind this condition? Its unclear; add comment.
also, we have an if statement, lets put it inside block scope {}.
jacquesikot
added
ok-to-test
Description
Problem
When populating a table widget with data that includes dates in Unix timestamp (milliseconds) format, setting the column type to "Date," the input format to "Unix timestamp (ms)," and selecting a display format leads to an issue during inline editing. While the date picker behaves correctly, selecting a new date results in the table cell showing an "Invalid Date" error.
Root Cause
The platform currently uses DateInputFormat.MILLISECONDS for Unix timestamp (ms) formatting. However, this value is not a valid option for the moment.format() function, which expects the input format to be 'x' for Unix timestamps in milliseconds. This mismatch leads to the "Invalid Date" error.
Solution
Modify the logic to map DateInputFormat.MILLISECONDS to the correct moment format string 'x'.
Adjust the table's transformDataPureFn to correctly process and display dates in Unix timestamp (ms) format after inline edits, ensuring the moment library can handle the input properly.
Outcome
This fix ensures that when a user selects a date via the date picker in inline editing mode, the selected date is displayed correctly in the table without any errors.
Fixes #35631
Automation
/ok-to-test tags="@tag.Sanity, @tag.Binding, @tag.Table, @tag.Datepicker"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10960871756
Commit: 2621b9a
Cypress dashboard.
Tags:
@tag.Sanity, @tag.Binding, @tag.Table, @tag.DatepickerSpec:
Fri, 20 Sep 2024 15:30:27 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation